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.
