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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Oct 13, 2022 at 04:43:15PM +0200, Jan Beulich wrote:
> 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?

Right, I assumed that cpu_has_ssb_no would be exclusive with any other
SSBD mechanism, but that doesn't need to be true.

> 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).

Indeed, the whole function uses boot_cpu_has() so it seemed clearer to
also use it for SSB_NO.

Thanks, Roger.