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.
