[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 8/8] x86/cpuid: Enable MSR_SPEC_CTRL in SVM guests by default
On 26.01.2022 09:44, Andrew Cooper wrote: > With all other pieces in place, MSR_SPEC_CTRL is fully working for HVM guests. > > Update the CPUID derivation logic (both PV and HVM to avoid losing subtle > changes), and explicitly enable the CPUID bits for HVM guests. > > Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx> > --- > CC: Jan Beulich <JBeulich@xxxxxxxx> > CC: Roger Pau Monné <roger.pau@xxxxxxxxxx> > CC: Wei Liu <wl@xxxxxxx> > > Given the adjustment to calculate_pv_max_policy(), we could use 'A' rather > than 'S' which would avoid a second same-sized diff to cpufeatureset.h, but > it's also a bit misleading to say 'A' when the PV side won't engage at all > yet. I agree with using 'S' at this point for most of them. I'm unsure for SSB_NO, though - there 'A' would seem more appropriate, and afaict it would then also be visible to PV guests (since you validly don't make it dependent upon IBRS or anything else). Aiui this could have been done even ahead of this work of yours. > --- a/xen/arch/x86/cpuid.c > +++ b/xen/arch/x86/cpuid.c > @@ -433,6 +433,8 @@ static void __init > guest_common_feature_adjustments(uint32_t *fs) > */ > if ( test_bit(X86_FEATURE_IBRSB, fs) ) > __set_bit(X86_FEATURE_STIBP, fs); > + if ( test_bit(X86_FEATURE_IBRS, fs) ) > + __set_bit(X86_FEATURE_AMD_STIBP, fs); > > /* > * On hardware which supports IBRS/IBPB, we can offer IBPB independently Following this comment is a cross-vendor adjustment. Shouldn't there be more of these now? Or has this been a one-off for some reason? > @@ -456,11 +458,14 @@ static void __init calculate_pv_max_policy(void) > pv_featureset[i] &= pv_max_featuremask[i]; > > /* > - * If Xen isn't virtualising MSR_SPEC_CTRL for PV guests because of > - * administrator choice, hide the feature. > + * If Xen isn't virtualising MSR_SPEC_CTRL for HVM guests (functional > + * availability, or admin choice), hide the feature. > + */ Unintended replacement of "PV" by "HVM"? > if ( !boot_cpu_has(X86_FEATURE_SC_MSR_PV) ) > + { > __clear_bit(X86_FEATURE_IBRSB, pv_featureset); > + __clear_bit(X86_FEATURE_IBRS, pv_featureset); > + } > > guest_common_feature_adjustments(pv_featureset); What I had done in a local (and incomplete) patch is pass X86_FEATURE_SC_MSR_{PV,HVM} into guest_common_feature_adjustments() and do what you extend above (and then again for HVM) centrally there. (My gen-cpuid.py adjustment was smaller, so there were even more bits to clear, and hence it became yet more relevant to avoid doing this in two places.) > --- a/xen/tools/gen-cpuid.py > +++ b/xen/tools/gen-cpuid.py > @@ -290,6 +290,11 @@ def crunch_numbers(state): > > # In principle the TSXLDTRK insns could also be considered > independent. > RTM: [TSXLDTRK], > + > + # AMD speculative controls > + IBRS: [AMD_STIBP, AMD_SSBD, PSFD, > + IBRS_ALWAYS, IBRS_FAST, IBRS_SAME_MODE], > + AMD_STIBP: [STIBP_ALWAYS], > } Could I talk you into moving this ahead of RTM, such that it sits next to the related Intel stuff? Jan
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |