Re: [PATCH v5 2/3] amd/msr: allow passthrough of VIRT_SPEC_CTRL for HVM guests

[Jan Beulich <jbeulich@xxxxxxxx>]

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