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 and Hygon Family 18h processors 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.

Do the context switching of the SSBD selection in LS_CFG between
hypervisor and guest in the same handler that's already used to switch
the value of VIRT_SPEC_CTRL.

Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v3:
 - Align ssbd per-core struct to a cache line.
 - Open code a simple spinlock to avoid playing tricks with the lock
   detector.
 - s/ssbd_core/ssbd_ls_cfg/.
 - Fix log message wording.
 - Fix define name and remove comment.
 - Also handle Hygon processors (Fam18h).
 - Add changelog entry.

Changes since v2:
 - Fix codding style issues.
 - Use AMD_ZEN1_MAX_SOCKETS to define the max number of possible
   sockets in Zen1 systems.

Changes since v1:
 - Report legacy SSBD support using a global variable.
 - Use ro_after_init for ssbd_max_cores.
 - Handle boot_cpu_data.x86_num_siblings < 1.
 - Add comment regarding _irqsave usage in amd_set_legacy_ssbd.
---
 xen/arch/x86/cpu/amd.c         | 119 ++++++++++++++++++++++++++++-----
 xen/arch/x86/cpuid.c           |  10 +++
 xen/arch/x86/hvm/svm/svm.c     |   5 ++
 xen/arch/x86/include/asm/amd.h |   4 ++
 xen/arch/x86/spec_ctrl.c       |   4 +-
 5 files changed, 124 insertions(+), 18 deletions(-)

diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index 4999f8be2b..a911e2e50a 100644
--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -48,6 +48,7 @@ boolean_param("allow_unsafe", opt_allow_unsafe);
 
 /* Signal whether the ACPI C1E quirk is required. */
 bool __read_mostly amd_acpi_c1e_quirk;
+bool __ro_after_init amd_legacy_ssbd;
 
 static inline int rdmsr_amd_safe(unsigned int msr, unsigned int *lo,
 				 unsigned int *hi)
@@ -685,23 +686,10 @@ void amd_init_lfence(struct cpuinfo_x86 *c)
  * Refer to the AMD Speculative Store Bypass whitepaper:
  * https://developer.amd.com/wp-content/resources/124441_AMD64_SpeculativeStoreBypassDisable_Whitepaper_final.pdf
  */
-void amd_init_ssbd(const struct cpuinfo_x86 *c)
+static bool set_legacy_ssbd(const struct cpuinfo_x86 *c, bool enable)
 {
 	int bit = -1;
 
-	if (cpu_has_ssb_no)
-		return;
-
-	if (cpu_has_amd_ssbd) {
-		/* Handled by common MSR_SPEC_CTRL logic */
-		return;
-	}
-
-	if (cpu_has_virt_ssbd) {
-		wrmsrl(MSR_VIRT_SPEC_CTRL, opt_ssbd ? SPEC_CTRL_SSBD : 0);
-		return;
-	}
-
 	switch (c->x86) {
 	case 0x15: bit = 54; break;
 	case 0x16: bit = 33; break;
@@ -715,20 +703,117 @@ void amd_init_ssbd(const struct cpuinfo_x86 *c)
 		if (rdmsr_safe(MSR_AMD64_LS_CFG, val) ||
 		    ({
 			    val &= ~mask;
-			    if (opt_ssbd)
+			    if (enable)
 				    val |= mask;
 			    false;
 		    }) ||
 		    wrmsr_safe(MSR_AMD64_LS_CFG, val) ||
 		    ({
 			    rdmsrl(MSR_AMD64_LS_CFG, val);
-			    (val & mask) != (opt_ssbd * mask);
+			    (val & mask) != (enable * mask);
 		    }))
 			bit = -1;
 	}
 
-	if (bit < 0)
+	return bit >= 0;
+}
+
+void amd_init_ssbd(const struct cpuinfo_x86 *c)
+{
+	if (cpu_has_ssb_no)
+		return;
+
+	if (cpu_has_amd_ssbd) {
+		/* Handled by common MSR_SPEC_CTRL logic */
+		return;
+	}
+
+	if (cpu_has_virt_ssbd) {
+		wrmsrl(MSR_VIRT_SPEC_CTRL, opt_ssbd ? SPEC_CTRL_SSBD : 0);
+		return;
+	}
+
+	if (!set_legacy_ssbd(c, opt_ssbd)) {
 		printk_once(XENLOG_ERR "No SSBD controls available\n");
+		if (amd_legacy_ssbd)
+			panic("CPU feature mismatch: no legacy SSBD\n");
+	} else if (c == &boot_cpu_data)
+		amd_legacy_ssbd = true;
+}
+
+static struct ssbd_ls_cfg {
+    bool locked;
+    unsigned int count;
+} __cacheline_aligned *ssbd_ls_cfg;
+static unsigned int __ro_after_init ssbd_max_cores;
+#define AMD_FAM17H_MAX_SOCKETS 2
+
+bool __init amd_setup_legacy_ssbd(void)
+{
+	unsigned int i;
+
+	if ((boot_cpu_data.x86 != 0x17 && boot_cpu_data.x86 != 0x18) ||
+	    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;
+
+	ssbd_ls_cfg = xzalloc_array(struct ssbd_ls_cfg,
+	                            ssbd_max_cores * AMD_FAM17H_MAX_SOCKETS);
+	if (!ssbd_ls_cfg)
+		return false;
+
+	for (i = 0; i < ssbd_max_cores * AMD_FAM17H_MAX_SOCKETS; i++)
+		/* Record initial state, also applies to any hotplug CPU. */
+		if (opt_ssbd)
+			ssbd_ls_cfg[i].count = boot_cpu_data.x86_num_siblings;
+
+	return true;
+}
+
+void amd_set_legacy_ssbd(bool enable)
+{
+	const struct cpuinfo_x86 *c = &current_cpu_data;
+	struct ssbd_ls_cfg *status;
+
+	if (c->x86 != 0x17 || c->x86_num_siblings <= 1) {
+		BUG_ON(!set_legacy_ssbd(c, enable));
+		return;
+	}
+
+	BUG_ON(c->phys_proc_id >= AMD_FAM17H_MAX_SOCKETS);
+	BUG_ON(c->cpu_core_id >= ssbd_max_cores);
+	status = &ssbd_ls_cfg[c->phys_proc_id * ssbd_max_cores +
+	                      c->cpu_core_id];
+
+	/*
+	 * Open code a very simple spinlock: this function is used with GIF==0
+	 * and different IF values, so would trigger the checklock detector.
+	 * Instead of trying to workaround the detector, use a very simple lock
+	 * implementation: it's better to reduce the amount of code executed
+	 * with GIF==0.
+	 */
+	while ( test_and_set_bool(status->locked) )
+	    cpu_relax();
+	status->count += enable ? 1 : -1;
+	ASSERT(status->count <= c->x86_num_siblings);
+	if (enable ? status->count == 1 : !status->count)
+		BUG_ON(!set_legacy_ssbd(c, enable));
+	barrier();
+	write_atomic(&status->locked, false);
 }
 
 void __init detect_zen2_null_seg_behaviour(void)
diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index 9a8c73f067..7607155875 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -544,6 +544,16 @@ static void __init calculate_hvm_max_policy(void)
     if ( !boot_cpu_has(X86_FEATURE_VIRT_SC_MSR_HVM) )
         /* Clear VIRT_SSBD if VIRT_SPEC_CTRL is not exposed to guests. */
         __clear_bit(X86_FEATURE_VIRT_SSBD, hvm_featureset);
+    else
+        /*
+         * Expose VIRT_SSBD if VIRT_SPEC_CTRL is supported, as that implies the
+         * underlying hardware is capable of setting SSBD using
+         * non-architectural way or VIRT_SSBD is available.
+         *
+         * Note that if the hardware supports VIRT_SSBD natively this setting
+         * will just override an already set bit.
+         */
+        __set_bit(X86_FEATURE_VIRT_SSBD, hvm_featureset);
 
     /*
      * If Xen isn't virtualising MSR_SPEC_CTRL for HVM guests (functional
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index e15c9754d7..cee11bb244 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -3126,6 +3126,8 @@ void vmexit_virt_spec_ctrl(void)
 
     if ( cpu_has_virt_ssbd )
         wrmsr(MSR_VIRT_SPEC_CTRL, val, 0);
+    else
+         amd_set_legacy_ssbd(opt_ssbd);
 }
 
 /* Called with GIF=0. */
@@ -3138,6 +3140,9 @@ void vmentry_virt_spec_ctrl(void)
 
     if ( cpu_has_virt_ssbd )
         wrmsr(MSR_VIRT_SPEC_CTRL, current->arch.msrs->virt_spec_ctrl.raw, 0);
+    else
+        amd_set_legacy_ssbd(current->arch.msrs->virt_spec_ctrl.raw &
+                            SPEC_CTRL_SSBD);
 }
 
 /*
diff --git a/xen/arch/x86/include/asm/amd.h b/xen/arch/x86/include/asm/amd.h
index a82382e6bf..6a42f68542 100644
--- a/xen/arch/x86/include/asm/amd.h
+++ b/xen/arch/x86/include/asm/amd.h
@@ -151,4 +151,8 @@ void check_enable_amd_mmconf_dmi(void);
 extern bool amd_acpi_c1e_quirk;
 void amd_check_disable_c1e(unsigned int port, u8 value);
 
+extern bool amd_legacy_ssbd;
+bool amd_setup_legacy_ssbd(void);
+void amd_set_legacy_ssbd(bool enable);
+
 #endif /* __AMD_H__ */
diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
index 0d5ec877d1..495e6f9405 100644
--- a/xen/arch/x86/spec_ctrl.c
+++ b/xen/arch/x86/spec_ctrl.c
@@ -22,6 +22,7 @@
 #include <xen/param.h>
 #include <xen/warning.h>
 
+#include <asm/amd.h>
 #include <asm/hvm/svm/svm.h>
 #include <asm/microcode.h>
 #include <asm/msr.h>
@@ -1073,7 +1074,8 @@ void __init init_speculation_mitigations(void)
     }
 
     /* Support VIRT_SPEC_CTRL.SSBD if AMD_SSBD is not available. */
-    if ( opt_msr_sc_hvm && !cpu_has_amd_ssbd && cpu_has_virt_ssbd )
+    if ( opt_msr_sc_hvm && !cpu_has_amd_ssbd &&
+         (cpu_has_virt_ssbd || (amd_legacy_ssbd && amd_setup_legacy_ssbd())) )
         setup_force_cpu_cap(X86_FEATURE_VIRT_SC_MSR_HVM);
 
     /* If we have IBRS available, see whether we should use it. */
-- 
2.35.1

On 27.04.2022 12:47, Roger Pau Monne wrote:> Changes since v3:>  - Align ssbd per-core struct to a cache line.>  - Open code a simple spinlock to avoid playing tricks with the lock>    detector.>  - s/ssbd_core/ssbd_ls_cfg/.>  - Fix log message wording.>  - Fix define name and remove comment.>  - Also handle Hygon processors (Fam18h).>  - Add changelog entry.
What is this last line about?

> +bool __init amd_setup_legacy_ssbd(void)
> +{
> +	unsigned int i;
> +
> +	if ((boot_cpu_data.x86 != 0x17 && boot_cpu_data.x86 != 0x18) ||
> +	    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;
> +
> +	ssbd_ls_cfg = xzalloc_array(struct ssbd_ls_cfg,
> +	                            ssbd_max_cores * AMD_FAM17H_MAX_SOCKETS);
> +	if (!ssbd_ls_cfg)
> +		return false;
> +
> +	for (i = 0; i < ssbd_max_cores * AMD_FAM17H_MAX_SOCKETS; i++)
> +		/* Record initial state, also applies to any hotplug CPU. */
> +		if (opt_ssbd)
> +			ssbd_ls_cfg[i].count = boot_cpu_data.x86_num_siblings;

Perhaps flip if() and for()?

> +void amd_set_legacy_ssbd(bool enable)
> +{
> +	const struct cpuinfo_x86 *c = &current_cpu_data;
> +	struct ssbd_ls_cfg *status;
> +
> +	if (c->x86 != 0x17 || c->x86_num_siblings <= 1) {
> +		BUG_ON(!set_legacy_ssbd(c, enable));
> +		return;
> +	}
> +
> +	BUG_ON(c->phys_proc_id >= AMD_FAM17H_MAX_SOCKETS);
> +	BUG_ON(c->cpu_core_id >= ssbd_max_cores);
> +	status = &ssbd_ls_cfg[c->phys_proc_id * ssbd_max_cores +
> +	                      c->cpu_core_id];
> +
> +	/*
> +	 * Open code a very simple spinlock: this function is used with GIF==0
> +	 * and different IF values, so would trigger the checklock detector.
> +	 * Instead of trying to workaround the detector, use a very simple lock
> +	 * implementation: it's better to reduce the amount of code executed
> +	 * with GIF==0.
> +	 */
> +	while ( test_and_set_bool(status->locked) )
> +	    cpu_relax();
> +	status->count += enable ? 1 : -1;
> +	ASSERT(status->count <= c->x86_num_siblings);
> +	if (enable ? status->count == 1 : !status->count)
> +		BUG_ON(!set_legacy_ssbd(c, enable));

What are the effects of ASSERT() or BUG_ON() triggering in a GIF=0
region?

> --- a/xen/arch/x86/cpuid.c
> +++ b/xen/arch/x86/cpuid.c
> @@ -544,6 +544,16 @@ static void __init calculate_hvm_max_policy(void)
>      if ( !boot_cpu_has(X86_FEATURE_VIRT_SC_MSR_HVM) )
>          /* Clear VIRT_SSBD if VIRT_SPEC_CTRL is not exposed to guests. */
>          __clear_bit(X86_FEATURE_VIRT_SSBD, hvm_featureset);
> +    else
> +        /*
> +         * Expose VIRT_SSBD if VIRT_SPEC_CTRL is supported, as that implies the
> +         * underlying hardware is capable of setting SSBD using
> +         * non-architectural way or VIRT_SSBD is available.
> +         *
> +         * Note that if the hardware supports VIRT_SSBD natively this setting
> +         * will just override an already set bit.
> +         */
> +        __set_bit(X86_FEATURE_VIRT_SSBD, hvm_featureset);

With the 's' annotation gone from the public header, is this last
sentence of the comment actually true? Aiui code near the top of
the function would have zapped the bit from hvm_featureset[].

> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -3126,6 +3126,8 @@ void vmexit_virt_spec_ctrl(void)
>  
>      if ( cpu_has_virt_ssbd )
>          wrmsr(MSR_VIRT_SPEC_CTRL, val, 0);
> +    else
> +         amd_set_legacy_ssbd(opt_ssbd);

Nit: Indentation is off by one here. Of course this alone could
easily be adjusted while committing.

> @@ -3138,6 +3140,9 @@ void vmentry_virt_spec_ctrl(void)
>  
>      if ( cpu_has_virt_ssbd )
>          wrmsr(MSR_VIRT_SPEC_CTRL, current->arch.msrs->virt_spec_ctrl.raw, 0);
> +    else
> +        amd_set_legacy_ssbd(current->arch.msrs->virt_spec_ctrl.raw &
> +                            SPEC_CTRL_SSBD);

Would seem cheaper to use !val here (and then val for symmetry in
the other function).

Jan

On Fri, Apr 29, 2022 at 12:59:58PM +0200, Jan Beulich wrote:
> On 27.04.2022 12:47, Roger Pau Monne wrote:> Changes since v3:>  - Align ssbd per-core struct to a cache line.>  - Open code a simple spinlock to avoid playing tricks with the lock>    detector.>  - s/ssbd_core/ssbd_ls_cfg/.>  - Fix log message wording.>  - Fix define name and remove comment.>  - Also handle Hygon processors (Fam18h).>  - Add changelog entry.
> What is this last line about?

Hm, seems like I forgot to do a patch refresh... So new version will
have an entry about adding VIRT_SSBD support to HVM guests. Sorry for
spoiling the surprise.

> > +bool __init amd_setup_legacy_ssbd(void)
> > +{
> > +	unsigned int i;
> > +
> > +	if ((boot_cpu_data.x86 != 0x17 && boot_cpu_data.x86 != 0x18) ||
> > +	    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;
> > +
> > +	ssbd_ls_cfg = xzalloc_array(struct ssbd_ls_cfg,
> > +	                            ssbd_max_cores * AMD_FAM17H_MAX_SOCKETS);
> > +	if (!ssbd_ls_cfg)
> > +		return false;
> > +
> > +	for (i = 0; i < ssbd_max_cores * AMD_FAM17H_MAX_SOCKETS; i++)
> > +		/* Record initial state, also applies to any hotplug CPU. */
> > +		if (opt_ssbd)
> > +			ssbd_ls_cfg[i].count = boot_cpu_data.x86_num_siblings;
> 
> Perhaps flip if() and for()?

Indeed, thanks.

> > +void amd_set_legacy_ssbd(bool enable)
> > +{
> > +	const struct cpuinfo_x86 *c = &current_cpu_data;
> > +	struct ssbd_ls_cfg *status;
> > +
> > +	if (c->x86 != 0x17 || c->x86_num_siblings <= 1) {
> > +		BUG_ON(!set_legacy_ssbd(c, enable));
> > +		return;
> > +	}
> > +
> > +	BUG_ON(c->phys_proc_id >= AMD_FAM17H_MAX_SOCKETS);
> > +	BUG_ON(c->cpu_core_id >= ssbd_max_cores);
> > +	status = &ssbd_ls_cfg[c->phys_proc_id * ssbd_max_cores +
> > +	                      c->cpu_core_id];
> > +
> > +	/*
> > +	 * Open code a very simple spinlock: this function is used with GIF==0
> > +	 * and different IF values, so would trigger the checklock detector.
> > +	 * Instead of trying to workaround the detector, use a very simple lock
> > +	 * implementation: it's better to reduce the amount of code executed
> > +	 * with GIF==0.
> > +	 */
> > +	while ( test_and_set_bool(status->locked) )
> > +	    cpu_relax();
> > +	status->count += enable ? 1 : -1;
> > +	ASSERT(status->count <= c->x86_num_siblings);
> > +	if (enable ? status->count == 1 : !status->count)
> > +		BUG_ON(!set_legacy_ssbd(c, enable));
> 
> What are the effects of ASSERT() or BUG_ON() triggering in a GIF=0
> region?

So AFAICT the BUG itself works, the usage of a crash kernel however
won't work as it's booted with GIF==0.

Maybe we need to issue an stgi as part of BUG_FRAME if required?
(maybe that's too naive...)

> > --- a/xen/arch/x86/cpuid.c
> > +++ b/xen/arch/x86/cpuid.c
> > @@ -544,6 +544,16 @@ static void __init calculate_hvm_max_policy(void)
> >      if ( !boot_cpu_has(X86_FEATURE_VIRT_SC_MSR_HVM) )
> >          /* Clear VIRT_SSBD if VIRT_SPEC_CTRL is not exposed to guests. */
> >          __clear_bit(X86_FEATURE_VIRT_SSBD, hvm_featureset);
> > +    else
> > +        /*
> > +         * Expose VIRT_SSBD if VIRT_SPEC_CTRL is supported, as that implies the
> > +         * underlying hardware is capable of setting SSBD using
> > +         * non-architectural way or VIRT_SSBD is available.
> > +         *
> > +         * Note that if the hardware supports VIRT_SSBD natively this setting
> > +         * will just override an already set bit.
> > +         */
> > +        __set_bit(X86_FEATURE_VIRT_SSBD, hvm_featureset);
> 
> With the 's' annotation gone from the public header, is this last
> sentence of the comment actually true? Aiui code near the top of
> the function would have zapped the bit from hvm_featureset[].

This comment is now gone, and there are no changes to
calculate_hvm_max_policy in this patch anymore.

> > --- a/xen/arch/x86/hvm/svm/svm.c
> > +++ b/xen/arch/x86/hvm/svm/svm.c
> > @@ -3126,6 +3126,8 @@ void vmexit_virt_spec_ctrl(void)
> >  
> >      if ( cpu_has_virt_ssbd )
> >          wrmsr(MSR_VIRT_SPEC_CTRL, val, 0);
> > +    else
> > +         amd_set_legacy_ssbd(opt_ssbd);
> 
> Nit: Indentation is off by one here. Of course this alone could
> easily be adjusted while committing.
> 
> > @@ -3138,6 +3140,9 @@ void vmentry_virt_spec_ctrl(void)
> >  
> >      if ( cpu_has_virt_ssbd )
> >          wrmsr(MSR_VIRT_SPEC_CTRL, current->arch.msrs->virt_spec_ctrl.raw, 0);
> > +    else
> > +        amd_set_legacy_ssbd(current->arch.msrs->virt_spec_ctrl.raw &
> > +                            SPEC_CTRL_SSBD);
> 
> Would seem cheaper to use !val here (and then val for symmetry in
> the other function).

I could even use !opt_ssbd, and that would be more similar to what's
done in vmexit_virt_spec_ctrl?

Thanks, Roger.

On 29.04.2022 17:49, Roger Pau Monné wrote:
> On Fri, Apr 29, 2022 at 12:59:58PM +0200, Jan Beulich wrote:
>> On 27.04.2022 12:47, Roger Pau Monne wrote:
>>> @@ -3138,6 +3140,9 @@ void vmentry_virt_spec_ctrl(void)
>>>  
>>>      if ( cpu_has_virt_ssbd )
>>>          wrmsr(MSR_VIRT_SPEC_CTRL, current->arch.msrs->virt_spec_ctrl.raw, 0);
>>> +    else
>>> +        amd_set_legacy_ssbd(current->arch.msrs->virt_spec_ctrl.raw &
>>> +                            SPEC_CTRL_SSBD);
>>
>> Would seem cheaper to use !val here (and then val for symmetry in
>> the other function).
> 
> I could even use !opt_ssbd, and that would be more similar to what's
> done in vmexit_virt_spec_ctrl?

Might be an option, yet when using "val" there as well I don't see
benefit similarity-wise. Using a local variable would imo still be
cheaper than accessing a global one. But that's a matter of taste
to a fair degree, so I'll leave it to you - all I'd really prefer
to have is the functions using as similar / symmetric as possible
conditions.

Jan

On Fri, Apr 29, 2022 at 05:49:42PM +0200, Roger Pau Monné wrote:
> On Fri, Apr 29, 2022 at 12:59:58PM +0200, Jan Beulich wrote:
> > On 27.04.2022 12:47, Roger Pau Monne wrote:> Changes since v3:>  - Align ssbd per-core struct to a cache line.>  - Open code a simple spinlock to avoid playing tricks with the lock>    detector.>  - s/ssbd_core/ssbd_ls_cfg/.>  - Fix log message wording.>  - Fix define name and remove comment.>  - Also handle Hygon processors (Fam18h).>  - Add changelog entry.
> > What is this last line about?
> 
> Hm, seems like I forgot to do a patch refresh... So new version will
> have an entry about adding VIRT_SSBD support to HVM guests. Sorry for
> spoiling the surprise.
> 
> > > +bool __init amd_setup_legacy_ssbd(void)
> > > +{
> > > +	unsigned int i;
> > > +
> > > +	if ((boot_cpu_data.x86 != 0x17 && boot_cpu_data.x86 != 0x18) ||
> > > +	    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;
> > > +
> > > +	ssbd_ls_cfg = xzalloc_array(struct ssbd_ls_cfg,
> > > +	                            ssbd_max_cores * AMD_FAM17H_MAX_SOCKETS);
> > > +	if (!ssbd_ls_cfg)
> > > +		return false;
> > > +
> > > +	for (i = 0; i < ssbd_max_cores * AMD_FAM17H_MAX_SOCKETS; i++)
> > > +		/* Record initial state, also applies to any hotplug CPU. */
> > > +		if (opt_ssbd)
> > > +			ssbd_ls_cfg[i].count = boot_cpu_data.x86_num_siblings;
> > 
> > Perhaps flip if() and for()?
> 
> Indeed, thanks.
> 
> > > +void amd_set_legacy_ssbd(bool enable)
> > > +{
> > > +	const struct cpuinfo_x86 *c = &current_cpu_data;
> > > +	struct ssbd_ls_cfg *status;
> > > +
> > > +	if (c->x86 != 0x17 || c->x86_num_siblings <= 1) {
> > > +		BUG_ON(!set_legacy_ssbd(c, enable));
> > > +		return;
> > > +	}
> > > +
> > > +	BUG_ON(c->phys_proc_id >= AMD_FAM17H_MAX_SOCKETS);
> > > +	BUG_ON(c->cpu_core_id >= ssbd_max_cores);
> > > +	status = &ssbd_ls_cfg[c->phys_proc_id * ssbd_max_cores +
> > > +	                      c->cpu_core_id];
> > > +
> > > +	/*
> > > +	 * Open code a very simple spinlock: this function is used with GIF==0
> > > +	 * and different IF values, so would trigger the checklock detector.
> > > +	 * Instead of trying to workaround the detector, use a very simple lock
> > > +	 * implementation: it's better to reduce the amount of code executed
> > > +	 * with GIF==0.
> > > +	 */
> > > +	while ( test_and_set_bool(status->locked) )
> > > +	    cpu_relax();
> > > +	status->count += enable ? 1 : -1;
> > > +	ASSERT(status->count <= c->x86_num_siblings);
> > > +	if (enable ? status->count == 1 : !status->count)
> > > +		BUG_ON(!set_legacy_ssbd(c, enable));
> > 
> > What are the effects of ASSERT() or BUG_ON() triggering in a GIF=0
> > region?
> 
> So AFAICT the BUG itself works, the usage of a crash kernel however
> won't work as it's booted with GIF==0.
> 
> Maybe we need to issue an stgi as part of BUG_FRAME if required?
> (maybe that's too naive...)

Well, better in panic() or kexec_crash() likely.

Roger.

On 29.04.2022 18:11, Roger Pau Monné wrote:
> On Fri, Apr 29, 2022 at 05:49:42PM +0200, Roger Pau Monné wrote:
>> On Fri, Apr 29, 2022 at 12:59:58PM +0200, Jan Beulich wrote:
>>> On 27.04.2022 12:47, Roger Pau Monne wrote:
>>>> +void amd_set_legacy_ssbd(bool enable)
>>>> +{
>>>> +	const struct cpuinfo_x86 *c = &current_cpu_data;
>>>> +	struct ssbd_ls_cfg *status;
>>>> +
>>>> +	if (c->x86 != 0x17 || c->x86_num_siblings <= 1) {
>>>> +		BUG_ON(!set_legacy_ssbd(c, enable));
>>>> +		return;
>>>> +	}
>>>> +
>>>> +	BUG_ON(c->phys_proc_id >= AMD_FAM17H_MAX_SOCKETS);
>>>> +	BUG_ON(c->cpu_core_id >= ssbd_max_cores);
>>>> +	status = &ssbd_ls_cfg[c->phys_proc_id * ssbd_max_cores +
>>>> +	                      c->cpu_core_id];
>>>> +
>>>> +	/*
>>>> +	 * Open code a very simple spinlock: this function is used with GIF==0
>>>> +	 * and different IF values, so would trigger the checklock detector.
>>>> +	 * Instead of trying to workaround the detector, use a very simple lock
>>>> +	 * implementation: it's better to reduce the amount of code executed
>>>> +	 * with GIF==0.
>>>> +	 */
>>>> +	while ( test_and_set_bool(status->locked) )
>>>> +	    cpu_relax();
>>>> +	status->count += enable ? 1 : -1;
>>>> +	ASSERT(status->count <= c->x86_num_siblings);
>>>> +	if (enable ? status->count == 1 : !status->count)
>>>> +		BUG_ON(!set_legacy_ssbd(c, enable));
>>>
>>> What are the effects of ASSERT() or BUG_ON() triggering in a GIF=0
>>> region?
>>
>> So AFAICT the BUG itself works, the usage of a crash kernel however
>> won't work as it's booted with GIF==0.
>>
>> Maybe we need to issue an stgi as part of BUG_FRAME if required?
>> (maybe that's too naive...)
> 
> Well, better in panic() or kexec_crash() likely.

Yeah, lifting it too early may be detrimental.

Jan