On 17.05.2022 15:45, Roger Pau Monné wrote: > 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?
Yes (with - nit - the double blank dealt with, perhaps by inserting "it" and maybe a comma). Jan
