[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v13 13/23] x86: refactor psr: CDP: implement CPU init flow.
On 17-07-12 13:52:35, Jan Beulich wrote: > >>> Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx> 07/06/17 4:07 AM >>> > >This patch implements the CPU init flow for CDP. The flow is almost > >same as L3 CAT. > > > >Note: CDP does NOT work until you apply the later patches of CDP. > >"x86: refactor psr: CDP: implement get hw info flow." > >"x86: refactor psr: CDP: implement set value callback function." > > This is _not_ what I did ask for in what I assume ... > > >v13: > >- add commit message. > > ... this refers to: The problem isn't that it won't work, but that it'll > crash the > hypervisor. So what I had expected you to add is a note _after_ the first > --- separator that this patch should not be applied without the other two > ones. Or alternatively for this one to add stubs which the subsequent > patches would then fill. > Oh, ok. I will move the comments under the first '---'. Furthermore, I'd like to add a 'stub_write_msr()' here to avoid crash. Will replace the stub function to real function when implementing the write flow. > >@@ -262,6 +280,29 @@ static int cat_init_feature(const struct cpuid_leaf > >*regs, > > > >break; > > > >+ case FEAT_TYPE_L3_CDP: > >+ { > >+ uint64_t val; > >+ > >+ if ( feat->cos_max < 3 ) > >+ return -ENOENT; > > In the admittedly unlikely event that this return path gets taken (or any > other > current or future one), ... > > >@@ -1277,11 +1331,20 @@ static void psr_cpu_init(void) > >{ > >cpuid_count_leaf(PSR_CPUID_LEVEL_CAT, 1, ®s); > > > >- feat = feat_l3_cat; > >- feat_l3_cat = NULL; > >- > >- if ( !cat_init_feature(®s, feat, info, FEAT_TYPE_L3_CAT) ) > >- feat_props[FEAT_TYPE_L3_CAT] = &l3_cat_props; > >+ if ( (regs.c & PSR_CAT_CDP_CAPABILITY) && (opt_psr & PSR_CDP) ) > >+ { > >+ feat = feat_l3_cdp; > >+ feat_l3_cdp = NULL; > >+ if ( !cat_init_feature(®s, feat, info, FEAT_TYPE_L3_CDP) ) > >+ feat_props[FEAT_TYPE_L3_CDP] = &l3_cdp_props; > >+ } > >+ else > >+ { > >+ feat = feat_l3_cat; > >+ feat_l3_cat = NULL; > >+ if ( !cat_init_feature(®s, feat, info, FEAT_TYPE_L3_CAT) ) > >+ feat_props[FEAT_TYPE_L3_CAT] = &l3_cat_props; > >+ } > > ... wouldn't it be a good idea to then try to setup plain L3 CAT here? > DYM if CDP init fails, enter CAT init flow to make CAT work at least? > I also notice that (already in patch 4) you leak feat here in case > cat_init_feature() fails. You should put it back into the static helper > variable > you've taken it from. Furthermore, since you use at most one of feat_l3_cat > and feat_l3_cdp, so I don't see why you need to allocate two of them. > Ok, I will keep the helper variable if cat_init_feature() fails. Will converge the 'feat_l3_cat' and 'feat_l3_cdp' to one. > Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |