Re: [PATCH v2] x86/SVM: restrict hardware SSBD update upon guest VIRT_SPEC_CTRL write

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 15/12/2022 8:20 am, Jan Beulich wrote:
> core_set_legacy_ssbd() counts the number of times SSBD is being enabled
> via LS_CFG on a core. This assumes that calls there only occur if the
> state actually changes. While svm_ctxt_switch_{to,from}() conform to
> this, guest_wrmsr() doesn't: It also calls the function when the bit
> doesn't actually change. Make core_set_legacy_ssbd() track per-thread
> enabled state by converting the "count" field to a bitmap, thus allowing
> to skip redundant enable/disable requests, constraining
> amd_setup_legacy_ssbd() accordingly.
>
> Fixes: b2030e6730a2 ("amd/virt_ssbd: set SSBD at vCPU context switch")
> Reported-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> This wants properly testing on affected hardware. From Andrew's
> description it's also not clear whether this really is addressing that
> problem, or yet another one in this same area.
> ---
> v2: Change core_set_legacy_ssbd() itself rather than the problematic
>     caller.

I think this patch will fix one of the errors.  I've lost count of how
many others issues I've found now that I've looked at the code in detail
for the first time.

However...

> --- a/xen/arch/x86/cpu/amd.c
> +++ b/xen/arch/x86/cpu/amd.c
> @@ -744,7 +744,7 @@ void amd_init_ssbd(const struct cpuinfo_
>  
>  static struct ssbd_ls_cfg {
>      spinlock_t lock;
> -    unsigned int count;
> +    unsigned long enabled;
>  } __cacheline_aligned *ssbd_ls_cfg;
>  static unsigned int __ro_after_init ssbd_max_cores;
>  #define AMD_FAM17H_MAX_SOCKETS 2
> @@ -757,6 +757,11 @@ bool __init amd_setup_legacy_ssbd(void)
>           boot_cpu_data.x86_num_siblings <= 1 || opt_ssbd)
>               return true;
>  
> +     if (boot_cpu_data.x86_num_siblings > BITS_PER_LONG ||
> +         (boot_cpu_data.x86_num_siblings &
> +          (boot_cpu_data.x86_num_siblings - 1)))
> +             return false;

... this is nonsense.  There is no such thing as an Zen1 uarch with more
than 2 threads per core, and attempts cope with it (as opposed to
rejecting it outright) makes ...

> +
>       /*
>        * One could be forgiven for thinking that c->x86_max_cores is the
>        * correct value to use here.
> @@ -800,10 +805,12 @@ static void core_set_legacy_ssbd(bool en
>                             c->cpu_core_id];
>  
>       spin_lock_irqsave(&status->lock, flags);
> -     status->count += enable ? 1 : -1;
> -     ASSERT(status->count <= c->x86_num_siblings);
> -     if (enable ? status->count == 1 : !status->count)
> +     if (!enable)
> +             __clear_bit(c->apicid & (c->x86_num_siblings - 1), 
> &status->enabled);
> +     if (!status->enabled)
>               BUG_ON(!set_legacy_ssbd(c, enable));
> +     if (enable)
> +             __set_bit(c->apicid & (c->x86_num_siblings - 1), 
> &status->enabled);

... this mess even worse.

I am rewriting the legacy SSBD code so that it is fit for purpose.

~Andrew