On Tue, May 17, 2022 at 02:10:29PM +0200, Jan Beulich wrote: > On 09.05.2022 12:23, Roger Pau Monné wrote: > > On Fri, May 06, 2022 at 02:15:47PM +0200, Jan Beulich wrote: > >> On 03.05.2022 10:26, Roger Pau Monne wrote: > >>> --- a/xen/arch/x86/cpuid.c > >>> +++ b/xen/arch/x86/cpuid.c > >>> @@ -541,6 +541,9 @@ static void __init calculate_hvm_max_policy(void) > >>> raw_cpuid_policy.basic.sep ) > >>> __set_bit(X86_FEATURE_SEP, hvm_featureset); > >>> > >>> + if ( boot_cpu_has(X86_FEATURE_VIRT_SC_MSR_HVM) ) > >>> + __set_bit(X86_FEATURE_VIRT_SSBD, hvm_featureset); > >>> + > >>> /* > >>> * If Xen isn't virtualising MSR_SPEC_CTRL for HVM guests (functional > >>> * availability, or admin choice), hide the feature. > >> > >> Especially with the setting of VIRT_SSBD below here (from patch 1) I > >> don't think this can go without comment. The more that the other > >> instance ... > >> > >>> @@ -597,6 +600,13 @@ static void __init calculate_hvm_def_policy(void) > >>> guest_common_feature_adjustments(hvm_featureset); > >>> guest_common_default_feature_adjustments(hvm_featureset); > >>> > >>> + /* > >>> + * Only expose VIRT_SSBD if AMD_SSBD is not available, and thus > >>> + * VIRT_SC_MSR_HVM is set. > >>> + */ > >>> + if ( boot_cpu_has(X86_FEATURE_VIRT_SC_MSR_HVM) ) > >>> + __set_bit(X86_FEATURE_VIRT_SSBD, hvm_featureset); > >>> + > >>> sanitise_featureset(hvm_featureset); > >>> cpuid_featureset_to_policy(hvm_featureset, p); > >>> recalculate_xstate(p); > >> > >> ... here is about default exposure, so cannot really be extended to > >> the condition under which this is put in "max" (except that of course > >> "max" needs to include everything "def" has). > > > > Would you be OK with adding: > > > > /* > > * VIRT_SC_MSR_HVM ensures the selection of SSBD is context > > * switched between the hypervisor and guest selected values for > > * HVM when the platform doesn't expose AMD_SSBD support. > > */ > > I'm afraid this doesn't explain what I'm after. In > calculate_hvm_def_policy() the comment explains why / when the feature > is exposed by _default_. Taking into account patch 1 (where another > maximum exposure of the feature was introduced), I'd like the > comment in calculate_hvm_max_policy() to focus on the difference > between default and maximum exposure (which could be as simple as "if > exposed by default, also needs exposing in max, irrespective of the > further max exposure below(?)").
So something like: /* * When VIRT_SSBD is exposed in the default policy as a result of * VIRT_SC_MSR_HVM being set also needs exposing in the max policy. */ Would address your concerns? Thanks, Roger.
