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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.03.2023 21:55, Andrew Cooper wrote:
> 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,

There's no guard anywhere here against us running virtualized ourselves,
and hence this extra check was to guard against anomalous topologies
which we might be presented (just like we present bogus topology to our
guests).

> 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.

I'll take that to mean that there's no point in me trying to make a v3
then. Let me know if that's a misunderstanding of mine.

Jan