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
