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

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

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.  In any case I'm unable to test
the patch because there's no hw with the feature that I'm aware of.

Thanks, Roger.