Re: [PATCH v4 3/3] amd/msr: implement VIRT_SPEC_CTRL for HVM guests using legacy SSBD

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

On Fri, Apr 29, 2022 at 12:59:58PM +0200, Jan Beulich wrote:
> On 27.04.2022 12:47, Roger Pau Monne wrote:> Changes since v3:>  - Align ssbd 
> per-core struct to a cache line.>  - Open code a simple spinlock to avoid 
> playing tricks with the lock>    detector.>  - s/ssbd_core/ssbd_ls_cfg/.>  - 
> Fix log message wording.>  - Fix define name and remove comment.>  - Also 
> handle Hygon processors (Fam18h).>  - Add changelog entry.
> What is this last line about?

Hm, seems like I forgot to do a patch refresh... So new version will
have an entry about adding VIRT_SSBD support to HVM guests. Sorry for
spoiling the surprise.

> > +bool __init amd_setup_legacy_ssbd(void)
> > +{
> > +   unsigned int i;
> > +
> > +   if ((boot_cpu_data.x86 != 0x17 && boot_cpu_data.x86 != 0x18) ||
> > +       boot_cpu_data.x86_num_siblings <= 1)
> > +           return true;
> > +
> > +   /*
> > +    * One could be forgiven for thinking that c->x86_max_cores is the
> > +    * correct value to use here.
> > +    *
> > +    * However, that value is derived from the current configuration, and
> > +    * c->cpu_core_id is sparse on all but the top end CPUs.  Derive
> > +    * max_cpus from ApicIdCoreIdSize which will cover any sparseness.
> > +    */
> > +   if (boot_cpu_data.extended_cpuid_level >= 0x80000008) {
> > +           ssbd_max_cores = 1u << MASK_EXTR(cpuid_ecx(0x80000008), 0xf000);
> > +           ssbd_max_cores /= boot_cpu_data.x86_num_siblings;
> > +   }
> > +   if (!ssbd_max_cores)
> > +           return false;
> > +
> > +   ssbd_ls_cfg = xzalloc_array(struct ssbd_ls_cfg,
> > +                               ssbd_max_cores * AMD_FAM17H_MAX_SOCKETS);
> > +   if (!ssbd_ls_cfg)
> > +           return false;
> > +
> > +   for (i = 0; i < ssbd_max_cores * AMD_FAM17H_MAX_SOCKETS; i++)
> > +           /* Record initial state, also applies to any hotplug CPU. */
> > +           if (opt_ssbd)
> > +                   ssbd_ls_cfg[i].count = boot_cpu_data.x86_num_siblings;
> 
> Perhaps flip if() and for()?

Indeed, thanks.

> > +void amd_set_legacy_ssbd(bool enable)
> > +{
> > +   const struct cpuinfo_x86 *c = &current_cpu_data;
> > +   struct ssbd_ls_cfg *status;
> > +
> > +   if (c->x86 != 0x17 || c->x86_num_siblings <= 1) {
> > +           BUG_ON(!set_legacy_ssbd(c, enable));
> > +           return;
> > +   }
> > +
> > +   BUG_ON(c->phys_proc_id >= AMD_FAM17H_MAX_SOCKETS);
> > +   BUG_ON(c->cpu_core_id >= ssbd_max_cores);
> > +   status = &ssbd_ls_cfg[c->phys_proc_id * ssbd_max_cores +
> > +                         c->cpu_core_id];
> > +
> > +   /*
> > +    * Open code a very simple spinlock: this function is used with GIF==0
> > +    * and different IF values, so would trigger the checklock detector.
> > +    * Instead of trying to workaround the detector, use a very simple lock
> > +    * implementation: it's better to reduce the amount of code executed
> > +    * with GIF==0.
> > +    */
> > +   while ( test_and_set_bool(status->locked) )
> > +       cpu_relax();
> > +   status->count += enable ? 1 : -1;
> > +   ASSERT(status->count <= c->x86_num_siblings);
> > +   if (enable ? status->count == 1 : !status->count)
> > +           BUG_ON(!set_legacy_ssbd(c, enable));
> 
> What are the effects of ASSERT() or BUG_ON() triggering in a GIF=0
> region?

So AFAICT the BUG itself works, the usage of a crash kernel however
won't work as it's booted with GIF==0.

Maybe we need to issue an stgi as part of BUG_FRAME if required?
(maybe that's too naive...)

> > --- a/xen/arch/x86/cpuid.c
> > +++ b/xen/arch/x86/cpuid.c
> > @@ -544,6 +544,16 @@ static void __init calculate_hvm_max_policy(void)
> >      if ( !boot_cpu_has(X86_FEATURE_VIRT_SC_MSR_HVM) )
> >          /* Clear VIRT_SSBD if VIRT_SPEC_CTRL is not exposed to guests. */
> >          __clear_bit(X86_FEATURE_VIRT_SSBD, hvm_featureset);
> > +    else
> > +        /*
> > +         * Expose VIRT_SSBD if VIRT_SPEC_CTRL is supported, as that 
> > implies the
> > +         * underlying hardware is capable of setting SSBD using
> > +         * non-architectural way or VIRT_SSBD is available.
> > +         *
> > +         * Note that if the hardware supports VIRT_SSBD natively this 
> > setting
> > +         * will just override an already set bit.
> > +         */
> > +        __set_bit(X86_FEATURE_VIRT_SSBD, hvm_featureset);
> 
> With the 's' annotation gone from the public header, is this last
> sentence of the comment actually true? Aiui code near the top of
> the function would have zapped the bit from hvm_featureset[].

This comment is now gone, and there are no changes to
calculate_hvm_max_policy in this patch anymore.

> > --- a/xen/arch/x86/hvm/svm/svm.c
> > +++ b/xen/arch/x86/hvm/svm/svm.c
> > @@ -3126,6 +3126,8 @@ void vmexit_virt_spec_ctrl(void)
> >  
> >      if ( cpu_has_virt_ssbd )
> >          wrmsr(MSR_VIRT_SPEC_CTRL, val, 0);
> > +    else
> > +         amd_set_legacy_ssbd(opt_ssbd);
> 
> Nit: Indentation is off by one here. Of course this alone could
> easily be adjusted while committing.
> 
> > @@ -3138,6 +3140,9 @@ void vmentry_virt_spec_ctrl(void)
> >  
> >      if ( cpu_has_virt_ssbd )
> >          wrmsr(MSR_VIRT_SPEC_CTRL, current->arch.msrs->virt_spec_ctrl.raw, 
> > 0);
> > +    else
> > +        amd_set_legacy_ssbd(current->arch.msrs->virt_spec_ctrl.raw &
> > +                            SPEC_CTRL_SSBD);
> 
> Would seem cheaper to use !val here (and then val for symmetry in
> the other function).

I could even use !opt_ssbd, and that would be more similar to what's
done in vmexit_virt_spec_ctrl?

Thanks, Roger.