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

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

On Thu, Apr 21, 2022 at 11:50:16AM +0200, Jan Beulich wrote:
> On 31.03.2022 11:27, Roger Pau Monne wrote:
> > Expose VIRT_SSBD to guests if the hardware supports setting SSBD in
> > the LS_CFG MSR (a.k.a. non-architectural way). Different AMD CPU
> > families use different bits in LS_CFG, so exposing VIRT_SPEC_CTRL.SSBD
> > allows for an unified way of exposing SSBD support to guests on AMD
> > hardware that's compatible migration wise, regardless of what
> > underlying mechanism is used to set SSBD.
> > 
> > Note that on AMD Family 17h (Zen 1) the value of SSBD in LS_CFG is
> > shared between threads on the same core, so there's extra logic in
> > order to synchronize the value and have SSBD set as long as one of the
> > threads in the core requires it to be set. Such logic also requires
> > extra storage for each thread state, which is allocated at
> > initialization time.
> 
> So where exactly is the boundary? If I'm not mistaken Zen2 is also
> Fam17 (and only Zen3 is Fam19), yet here and elsewhere you look to
> take Zen1 == Fam17.

Right, but Zen2 already has AMD_SSBD (ie: SPEC_CTRL), so it's not
using this logic.

The AMD whitepaper is more clear about this: any Fam17h processor that
is using the non-architectural MSRs to set SSBD and has more than 1
logical processor for each logical core must synchronize the setting
of SSBD.

I think just dropping the mention of Zen 1 in the commit message
should remove your concerns?

> Just one further nit on the code:
> 
> > +static struct ssbd_core {
> > +    spinlock_t lock;
> > +    unsigned int count;
> > +} *ssbd_core;
> > +static unsigned int __ro_after_init ssbd_max_cores;
> > +#define AMD_ZEN1_MAX_SOCKETS 2
> 
> This #define looks to render ...
> 
> > +bool __init amd_setup_legacy_ssbd(void)
> > +{
> > +   unsigned int i;
> > +
> > +   if (boot_cpu_data.x86 != 0x17 || 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;
> > +
> > +   /* Max is two sockets for Fam17h hardware. */
> > +   ssbd_core = xzalloc_array(struct ssbd_core,
> > +                             ssbd_max_cores * AMD_ZEN1_MAX_SOCKETS);
> 
> ... largely redundant the comment here; if anywhere I think the comment
> would want to go next to the #define.

I guess I should also rename the define to FAM17H instead of ZEN1, as
all Fam17h is limited to two sockets, then the comment can be removed
as I think the define is self-explanatory.

Thanks, Roger.