1. Patchew
  2. Xen
  3. View series

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

Jan Beulich posted 1 patch 1 year, 5 months ago
Failed in applying to current master (apply log)
[PATCH v2] x86/SVM: restrict hardware SSBD update upon guest VIRT_SPEC_CTRL write
Posted by Jan Beulich 1 year, 5 months ago 
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@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
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.

--- 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;
+
 	/*
 	 * 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);
 	spin_unlock_irqrestore(&status->lock, flags);
 }
Re: [PATCH v2] x86/SVM: restrict hardware SSBD update upon guest VIRT_SPEC_CTRL write
Posted by Andrew Cooper 1 year, 2 months ago 
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@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> 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
Re: [PATCH v2] x86/SVM: restrict hardware SSBD update upon guest VIRT_SPEC_CTRL write
Posted by Jan Beulich 1 year, 2 months ago 
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@citrix.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> 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

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.