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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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.

Jan