Re: [PATCH 4/4] amd/virt_ssbd: add to max HVM policy when SSB_NO is available

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.10.2022 16:06, Roger Pau Monné wrote:
> On Wed, Oct 12, 2022 at 10:36:57AM +0200, Jan Beulich wrote:
>> On 11.10.2022 18:02, Roger Pau Monne wrote:
>>> --- a/xen/arch/x86/cpu/amd.c
>>> +++ b/xen/arch/x86/cpu/amd.c
>>> @@ -814,7 +814,9 @@ void amd_set_ssbd(bool enable)
>>>             wrmsr(MSR_VIRT_SPEC_CTRL, enable ? SPEC_CTRL_SSBD : 0, 0);
>>>     else if ( amd_legacy_ssbd )
>>>             core_set_legacy_ssbd(enable);
>>> -   else
>>> +   else if ( cpu_has_ssb_no ) {
>>
>> Nit: While already an issue in patch 1, it is actually the combination
>> of inner blanks and brace placement which made me spot the style issue
>> here.
> 
> Oh, indeed, extra spaces.
> 
>>> +           /* Nothing to do. */
>>
>> How is the late placement here in line with ...
>>
>>> --- a/xen/arch/x86/cpuid.c
>>> +++ b/xen/arch/x86/cpuid.c
>>> @@ -558,11 +558,16 @@ static void __init calculate_hvm_max_policy(void)
>>>          __clear_bit(X86_FEATURE_IBRSB, hvm_featureset);
>>>          __clear_bit(X86_FEATURE_IBRS, hvm_featureset);
>>>      }
>>> -    else if ( boot_cpu_has(X86_FEATURE_AMD_SSBD) )
>>> +    else if ( boot_cpu_has(X86_FEATURE_AMD_SSBD) ||
>>> +              boot_cpu_has(X86_FEATURE_SSB_NO) )
>>>          /*
>>>           * If SPEC_CTRL.SSBD is available VIRT_SPEC_CTRL.SSBD can be 
>>> exposed
>>>           * and implemented using the former. Expose in the max policy only 
>>> as
>>>           * the preference is for guests to use SPEC_CTRL.SSBD if available.
>>> +         *
>>> +         * Allow VIRT_SSBD in the max policy if SSB_NO is exposed for 
>>> migration
>>> +         * compatibility reasons.  If SSB_NO is present setting
>>> +         * VIRT_SPEC_CTRL.SSBD is a no-op.
>>>           */
>>>          __set_bit(X86_FEATURE_VIRT_SSBD, hvm_featureset);
>>
>> ... this comment addition talking about "no-op"?
> 
> We need the empty `else if ...` body in order to avoid hitting the
> ASSERT, but a guest setting VIRT_SPEC_CTRl.SSBD on a system that has
> SSB_NO will not result in any setting being propagated to the
> hardware.  I can make that clearer.

I guess my question was more towards: Shouldn't that check in
amd_set_ssbd() move ahead?

As an aside I notice you use cpu_has_ssb_no there but not here. I
might guess this is because of the adjacent existing
boot_cpu_has(), but it still strikes me as a little odd (as in:
undue open-coding).

Jan