[PATCH for-4.17 0/4] amd/virt_ssbd: refactoring and cleanup

[PATCH for-4.17 0/4] amd/virt_ssbd: refactoring and cleanup

[Roger Pau Monne]

Hello,

The following series aims to remove running C code with GIF=0 on the AMD
vm{entry,exit} paths.  As a result, the context switching of SSBD is
done when context switching vCPUs, and hence Xen code is run with the
guest selection of SSBD.

First patch is the one strictly needed, but patches 2 and 3 are also
desirable as cleanups and fixes to the documentation.

Patch 4 is untested, as there's no hardware with SSB_NO.

I tested on Naples and Milan CPUs (and migrating from Naples to Milan
correctly carrying the VIRT_SSBD bit), but I haven't tested on a
platform that exposes VIRT_SSBD itself.  I think the path is
sufficiently similar to the legacy one.

Currently running a gitlab CI loop in order to check everything is OK.

Roger Pau Monne (4):
  amd/virt_ssbd: set SSBD at vCPU context switch
  amd: remove VIRT_SC_MSR_HVM synthetic feature
  amd/ssbd: remove hypervisor SSBD selection
  amd/virt_ssbd: add to max HVM policy when SSB_NO is available

 docs/misc/xen-command-line.pandoc      |  8 +---
 xen/arch/x86/cpu/amd.c                 | 54 +++++++++++++-------------
 xen/arch/x86/cpuid.c                   | 16 +++++---
 xen/arch/x86/hvm/svm/entry.S           |  6 +--
 xen/arch/x86/hvm/svm/svm.c             | 45 ++++++++-------------
 xen/arch/x86/include/asm/amd.h         |  3 +-
 xen/arch/x86/include/asm/cpufeatures.h |  2 +-
 xen/arch/x86/include/asm/spec_ctrl.h   |  1 -
 xen/arch/x86/msr.c                     |  7 ++++
 xen/arch/x86/spec_ctrl.c               | 27 +++----------
 10 files changed, 73 insertions(+), 96 deletions(-)

-- 
2.37.3

[PATCH for-4.17 1/4] amd/virt_ssbd: set SSBD at vCPU context switch

[Roger Pau Monne]

The current logic for AMD SSBD context switches it on every
vm{entry,exit} if the Xen and guest selections don't match.  This is
expensive when not using SPEC_CTRL, and hence should be avoided as
much as possible.

When SSBD is not being set from SPEC_CTRL on AMD don't context switch
at vm{entry,exit} and instead only context switch SSBD when switching
vCPUs.  This has the side effect of running Xen code with the guest
selection of SSBD, which renders the ssbd option without effect (in a
similar way to what already happens on Intel hardware).

This fixes an issue with running C code in a GIF=0 region, that's
problematic when using UBSAN or other instrumentation techniques.

As a result of no longer running the code to set SSBD in a GIF=0
region the locking of amd_set_legacy_ssbd() can be done using normal
spinlocks, and some more checks can be added to assure it works as
intended.

Finally it's also worth noticing that since the guest SSBD selection
is no longer set on vmentry the VIRT_SPEC_MSR handling needs to
propagate the value to the hardware as part of handling the wrmsr.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/cpu/amd.c         | 50 ++++++++++++++++++----------------
 xen/arch/x86/hvm/svm/entry.S   |  6 +---
 xen/arch/x86/hvm/svm/svm.c     | 45 ++++++++++++------------------
 xen/arch/x86/include/asm/amd.h |  2 +-
 xen/arch/x86/msr.c             |  7 +++++
 5 files changed, 52 insertions(+), 58 deletions(-)

diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index 98c52d0686..a1582e1cc9 100644
--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -742,7 +742,7 @@ void amd_init_ssbd(const struct cpuinfo_x86 *c)
 }
 
 static struct ssbd_ls_cfg {
-    bool locked;
+    spinlock_t lock;
     unsigned int count;
 } __cacheline_aligned *ssbd_ls_cfg;
 static unsigned int __ro_after_init ssbd_max_cores;
@@ -776,46 +776,48 @@ bool __init amd_setup_legacy_ssbd(void)
 	if (!ssbd_ls_cfg)
 		return false;
 
-	if (opt_ssbd)
-		for (i = 0; i < ssbd_max_cores * AMD_FAM17H_MAX_SOCKETS; i++)
-			/* Set initial state, applies to any (hotplug) CPU. */
-			ssbd_ls_cfg[i].count = boot_cpu_data.x86_num_siblings;
+	for (i = 0; i < ssbd_max_cores * AMD_FAM17H_MAX_SOCKETS; i++) {
+		/* Set initial state, applies to any (hotplug) CPU. */
+		ssbd_ls_cfg[i].count = opt_ssbd ? boot_cpu_data.x86_num_siblings
+		                                : 0;
+		spin_lock_init(&ssbd_ls_cfg[i].lock);
+	}
 
 	return true;
 }
 
-/*
- * Executed from GIF==0 context: avoid using BUG/ASSERT or other functionality
- * that relies on exceptions as those are not expected to run in GIF==0
- * context.
- */
-void amd_set_legacy_ssbd(bool enable)
+static void core_set_legacy_ssbd(bool enable)
 {
 	const struct cpuinfo_x86 *c = &current_cpu_data;
 	struct ssbd_ls_cfg *status;
+	unsigned long flags;
 
 	if ((c->x86 != 0x17 && c->x86 != 0x18) || c->x86_num_siblings <= 1) {
-		set_legacy_ssbd(c, enable);
+		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();
+	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)
-		set_legacy_ssbd(c, enable);
-	barrier();
-	write_atomic(&status->locked, false);
+		BUG_ON(!set_legacy_ssbd(c, enable));
+	spin_unlock_irqrestore(&status->lock, flags);
+}
+
+void amd_set_ssbd(bool enable)
+{
+	if ( cpu_has_virt_ssbd )
+		wrmsr(MSR_VIRT_SPEC_CTRL, enable ? SPEC_CTRL_SSBD : 0, 0);
+	else if ( amd_legacy_ssbd )
+		core_set_legacy_ssbd(enable);
+	else
+		ASSERT_UNREACHABLE();
 }
 
 /*
diff --git a/xen/arch/x86/hvm/svm/entry.S b/xen/arch/x86/hvm/svm/entry.S
index a26589aa9a..94089e61bc 100644
--- a/xen/arch/x86/hvm/svm/entry.S
+++ b/xen/arch/x86/hvm/svm/entry.S
@@ -59,9 +59,6 @@ __UNLIKELY_END(nsvm_hap)
 
         clgi
 
-        ALTERNATIVE "", STR(call vmentry_virt_spec_ctrl), \
-                        X86_FEATURE_VIRT_SC_MSR_HVM
-
         /* WARNING! `ret`, `call *`, `jmp *` not safe beyond this point. */
         /* SPEC_CTRL_EXIT_TO_SVM       Req: b=curr %rsp=regs/cpuinfo, Clob: acd */
         .macro svm_vmentry_spec_ctrl
@@ -131,8 +128,6 @@ __UNLIKELY_END(nsvm_hap)
         ALTERNATIVE "", svm_vmexit_spec_ctrl, X86_FEATURE_SC_MSR_HVM
         /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
 
-        ALTERNATIVE "", STR(call vmexit_virt_spec_ctrl), \
-                        X86_FEATURE_VIRT_SC_MSR_HVM
 
         /*
          * STGI is executed unconditionally, and is sufficiently serialising
@@ -140,6 +135,7 @@ __UNLIKELY_END(nsvm_hap)
          */
         stgi
 GLOBAL(svm_stgi_label)
+
         mov  %rsp,%rdi
         call svm_vmexit_handler
         jmp  .Lsvm_do_resume
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 1aeaabcb13..0c5ffad05d 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -973,6 +973,14 @@ static void cf_check svm_ctxt_switch_from(struct vcpu *v)
 
     /* Resume use of ISTs now that the host TR is reinstated. */
     enable_each_ist(idt_tables[cpu]);
+
+    /*
+     * Clear previous guest selection of SSBD if set.  Note that SPEC_CTRL.SSBD
+     * is already cleared by svm_vmexit_spec_ctrl.
+     */
+    if ( v->domain->arch.cpuid->extd.virt_ssbd &&
+         (v->arch.msrs->virt_spec_ctrl.raw & SPEC_CTRL_SSBD) )
+        amd_set_ssbd(false);
 }
 
 static void cf_check svm_ctxt_switch_to(struct vcpu *v)
@@ -1000,6 +1008,11 @@ static void cf_check svm_ctxt_switch_to(struct vcpu *v)
 
     if ( cpu_has_msr_tsc_aux )
         wrmsr_tsc_aux(v->arch.msrs->tsc_aux);
+
+    /* Load SSBD if set by the guest. */
+    if ( v->domain->arch.cpuid->extd.virt_ssbd &&
+         (v->arch.msrs->virt_spec_ctrl.raw & SPEC_CTRL_SSBD) )
+        amd_set_ssbd(true);
 }
 
 static void noreturn cf_check svm_do_resume(void)
@@ -2518,6 +2531,10 @@ static void cf_check svm_set_reg(struct vcpu *v, unsigned int reg, uint64_t val)
         vmcb->spec_ctrl = val;
         break;
 
+    case MSR_VIRT_SPEC_CTRL:
+        amd_set_ssbd(v->arch.msrs->virt_spec_ctrl.raw);
+        break;
+
     default:
         printk(XENLOG_G_ERR "%s(%pv, 0x%08x, 0x%016"PRIx64") Bad register\n",
                __func__, v, reg, val);
@@ -3116,34 +3133,6 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
     vmcb_set_vintr(vmcb, intr);
 }
 
-/* Called with GIF=0. */
-void vmexit_virt_spec_ctrl(void)
-{
-    unsigned int val = opt_ssbd ? SPEC_CTRL_SSBD : 0;
-
-    if ( val == current->arch.msrs->virt_spec_ctrl.raw )
-        return;
-
-    if ( cpu_has_virt_ssbd )
-        wrmsr(MSR_VIRT_SPEC_CTRL, val, 0);
-    else
-        amd_set_legacy_ssbd(val);
-}
-
-/* Called with GIF=0. */
-void vmentry_virt_spec_ctrl(void)
-{
-    unsigned int val = current->arch.msrs->virt_spec_ctrl.raw;
-
-    if ( val == (opt_ssbd ? SPEC_CTRL_SSBD : 0) )
-        return;
-
-    if ( cpu_has_virt_ssbd )
-        wrmsr(MSR_VIRT_SPEC_CTRL, val, 0);
-    else
-        amd_set_legacy_ssbd(val);
-}
-
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/x86/include/asm/amd.h b/xen/arch/x86/include/asm/amd.h
index 6a42f68542..81ed71710f 100644
--- a/xen/arch/x86/include/asm/amd.h
+++ b/xen/arch/x86/include/asm/amd.h
@@ -153,6 +153,6 @@ 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);
+void amd_set_ssbd(bool enable);
 
 #endif /* __AMD_H__ */
diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index 95416995a5..a4c28879bc 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -697,7 +697,14 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
                 msrs->spec_ctrl.raw &= ~SPEC_CTRL_SSBD;
         }
         else
+        {
             msrs->virt_spec_ctrl.raw = val & SPEC_CTRL_SSBD;
+            /*
+             * Propagate the value to hardware, as it won't be context switched
+             * on vmentry.
+             */
+            goto set_reg;
+        }
         break;
 
     case MSR_AMD64_DE_CFG:
-- 
2.37.3

[PATCH for-4.17 2/4] amd: remove VIRT_SC_MSR_HVM synthetic feature

[Roger Pau Monne]

Since the VIRT_SPEC_CTRL.SSBD selection is no longer context switched
on vm{entry,exit} there's no need to use a synthetic feature bit for
it anymore.

Remove the bit and instead use a global variable.

No functional change intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/cpu/amd.c                 | 1 +
 xen/arch/x86/cpuid.c                   | 9 +++++----
 xen/arch/x86/include/asm/amd.h         | 1 +
 xen/arch/x86/include/asm/cpufeatures.h | 2 +-
 xen/arch/x86/spec_ctrl.c               | 8 ++++----
 5 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index a1582e1cc9..c28f2d5220 100644
--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -49,6 +49,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;
+bool __ro_after_init amd_virt_spec_ctrl;
 
 static inline int rdmsr_amd_safe(unsigned int msr, unsigned int *lo,
 				 unsigned int *hi)
diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index 822f9ace10..acc2f606ce 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -3,6 +3,7 @@
 #include <xen/param.h>
 #include <xen/sched.h>
 #include <xen/nospec.h>
+#include <asm/amd.h>
 #include <asm/cpuid.h>
 #include <asm/hvm/hvm.h>
 #include <asm/hvm/nestedhvm.h>
@@ -543,9 +544,9 @@ static void __init calculate_hvm_max_policy(void)
 
     /*
      * VIRT_SSBD is exposed in the default policy as a result of
-     * VIRT_SC_MSR_HVM being set, it also needs exposing in the max policy.
+     * amd_virt_spec_ctrl being set, it also needs exposing in the max policy.
      */
-    if ( boot_cpu_has(X86_FEATURE_VIRT_SC_MSR_HVM) )
+    if ( amd_virt_spec_ctrl )
         __set_bit(X86_FEATURE_VIRT_SSBD, hvm_featureset);
 
     /*
@@ -606,9 +607,9 @@ static void __init calculate_hvm_def_policy(void)
 
     /*
      * Only expose VIRT_SSBD if AMD_SSBD is not available, and thus
-     * VIRT_SC_MSR_HVM is set.
+     * amd_virt_spec_ctrl is set.
      */
-    if ( boot_cpu_has(X86_FEATURE_VIRT_SC_MSR_HVM) )
+    if ( amd_virt_spec_ctrl )
         __set_bit(X86_FEATURE_VIRT_SSBD, hvm_featureset);
 
     sanitise_featureset(hvm_featureset);
diff --git a/xen/arch/x86/include/asm/amd.h b/xen/arch/x86/include/asm/amd.h
index 81ed71710f..5c100784dd 100644
--- a/xen/arch/x86/include/asm/amd.h
+++ b/xen/arch/x86/include/asm/amd.h
@@ -152,6 +152,7 @@ extern bool amd_acpi_c1e_quirk;
 void amd_check_disable_c1e(unsigned int port, u8 value);
 
 extern bool amd_legacy_ssbd;
+extern bool amd_virt_spec_ctrl;
 bool amd_setup_legacy_ssbd(void);
 void amd_set_ssbd(bool enable);
 
diff --git a/xen/arch/x86/include/asm/cpufeatures.h b/xen/arch/x86/include/asm/cpufeatures.h
index 3895de4faf..efd3a667ef 100644
--- a/xen/arch/x86/include/asm/cpufeatures.h
+++ b/xen/arch/x86/include/asm/cpufeatures.h
@@ -24,7 +24,7 @@ XEN_CPUFEATURE(APERFMPERF,        X86_SYNTH( 8)) /* APERFMPERF */
 XEN_CPUFEATURE(MFENCE_RDTSC,      X86_SYNTH( 9)) /* MFENCE synchronizes RDTSC */
 XEN_CPUFEATURE(XEN_SMEP,          X86_SYNTH(10)) /* SMEP gets used by Xen itself */
 XEN_CPUFEATURE(XEN_SMAP,          X86_SYNTH(11)) /* SMAP gets used by Xen itself */
-XEN_CPUFEATURE(VIRT_SC_MSR_HVM,   X86_SYNTH(12)) /* MSR_VIRT_SPEC_CTRL exposed to HVM */
+/* Bit 12 unused. */
 XEN_CPUFEATURE(IND_THUNK_LFENCE,  X86_SYNTH(13)) /* Use IND_THUNK_LFENCE */
 XEN_CPUFEATURE(IND_THUNK_JMP,     X86_SYNTH(14)) /* Use IND_THUNK_JMP */
 XEN_CPUFEATURE(SC_NO_BRANCH_HARDEN, X86_SYNTH(15)) /* (Disable) Conditional branch hardening */
diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
index 4e53056624..0b94af6b86 100644
--- a/xen/arch/x86/spec_ctrl.c
+++ b/xen/arch/x86/spec_ctrl.c
@@ -514,12 +514,12 @@ static void __init print_details(enum ind_thunk thunk, uint64_t caps)
            (boot_cpu_has(X86_FEATURE_SC_MSR_HVM) ||
             boot_cpu_has(X86_FEATURE_SC_RSB_HVM) ||
             boot_cpu_has(X86_FEATURE_IBPB_ENTRY_HVM) ||
-            boot_cpu_has(X86_FEATURE_VIRT_SC_MSR_HVM) ||
+            amd_virt_spec_ctrl ||
             opt_eager_fpu || opt_md_clear_hvm)       ? ""               : " None",
            boot_cpu_has(X86_FEATURE_SC_MSR_HVM)      ? " MSR_SPEC_CTRL" : "",
            (boot_cpu_has(X86_FEATURE_SC_MSR_HVM) ||
-            boot_cpu_has(X86_FEATURE_VIRT_SC_MSR_HVM)) ? " MSR_VIRT_SPEC_CTRL"
-                                                       : "",
+            amd_virt_spec_ctrl)                      ? " MSR_VIRT_SPEC_CTRL"
+                                                     : "",
            boot_cpu_has(X86_FEATURE_SC_RSB_HVM)      ? " RSB"           : "",
            opt_eager_fpu                             ? " EAGER_FPU"     : "",
            opt_md_clear_hvm                          ? " MD_CLEAR"      : "",
@@ -1247,7 +1247,7 @@ 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 || (amd_legacy_ssbd && amd_setup_legacy_ssbd())) )
-        setup_force_cpu_cap(X86_FEATURE_VIRT_SC_MSR_HVM);
+        amd_virt_spec_ctrl = true;
 
     /* Figure out default_xen_spec_ctrl. */
     if ( has_spec_ctrl && ibrs )
-- 
2.37.3

[PATCH for-4.17 3/4] amd/ssbd: remove hypervisor SSBD selection

[Roger Pau Monne]

Like on Intel AMD guests are now capable of setting SSBD on their own,
either from SPEC_CTRL or from VIRT_SPEC_CTRL.  As a result the
unconditional setting of SSBD from Xen in order to cope with the bit
not being exposed to guests is no longer needed.

Remove the Xen command line `spec-ctrl=ssbd` option and related
settings.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 docs/misc/xen-command-line.pandoc    |  8 +-------
 xen/arch/x86/cpu/amd.c               | 11 ++++-------
 xen/arch/x86/include/asm/spec_ctrl.h |  1 -
 xen/arch/x86/spec_ctrl.c             | 19 +------------------
 4 files changed, 6 insertions(+), 33 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index 68389843b2..f2666b881a 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -2297,7 +2297,7 @@ By default SSBD will be mitigated at runtime (i.e `ssbd=runtime`).
 ### spec-ctrl (x86)
 > `= List of [ <bool>, xen=<bool>, {pv,hvm}=<bool>,
 >              {msr-sc,rsb,md-clear,ibpb-entry}=<bool>|{pv,hvm}=<bool>,
->              bti-thunk=retpoline|lfence|jmp, {ibrs,ibpb,ssbd,psfd,
+>              bti-thunk=retpoline|lfence|jmp, {ibrs,ibpb,psfd,
 >              eager-fpu,l1d-flush,branch-harden,srb-lock,
 >              unpriv-mmio}=<bool> ]`
 
@@ -2365,12 +2365,6 @@ On hardware supporting STIBP (Single Thread Indirect Branch Predictors), the
 By default, Xen will use STIBP when IBRS is in use (IBRS implies STIBP), and
 when hardware hints recommend using it as a blanket setting.
 
-On hardware supporting SSBD (Speculative Store Bypass Disable), the `ssbd=`
-option can be used to force or prevent Xen using the feature itself.  On AMD
-hardware, this is a global option applied at boot, and not virtualised for
-guest use.  On Intel hardware, the feature is virtualised for guests,
-independently of Xen's choice of setting.
-
 On hardware supporting PSFD (Predictive Store Forwarding Disable), the `psfd=`
 option can be used to force or prevent Xen using the feature itself.  By
 default, Xen will not use PSFD.  PSFD is implied by SSBD, and SSBD is off by
diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index c28f2d5220..cfeb8d1daf 100644
--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -730,11 +730,12 @@ void amd_init_ssbd(const struct cpuinfo_x86 *c)
 	}
 
 	if (cpu_has_virt_ssbd) {
-		wrmsrl(MSR_VIRT_SPEC_CTRL, opt_ssbd ? SPEC_CTRL_SSBD : 0);
+		/* Handled by context switch logic. */
 		return;
 	}
 
-	if (!set_legacy_ssbd(c, opt_ssbd)) {
+	/* Test whether legacy SSBD is available. */
+	if (!set_legacy_ssbd(c, 0)) {
 		printk_once(XENLOG_ERR "No SSBD controls available\n");
 		if (amd_legacy_ssbd)
 			panic("CPU feature mismatch: no legacy SSBD\n");
@@ -777,12 +778,8 @@ bool __init amd_setup_legacy_ssbd(void)
 	if (!ssbd_ls_cfg)
 		return false;
 
-	for (i = 0; i < ssbd_max_cores * AMD_FAM17H_MAX_SOCKETS; i++) {
-		/* Set initial state, applies to any (hotplug) CPU. */
-		ssbd_ls_cfg[i].count = opt_ssbd ? boot_cpu_data.x86_num_siblings
-		                                : 0;
+	for (i = 0; i < ssbd_max_cores * AMD_FAM17H_MAX_SOCKETS; i++)
 		spin_lock_init(&ssbd_ls_cfg[i].lock);
-	}
 
 	return true;
 }
diff --git a/xen/arch/x86/include/asm/spec_ctrl.h b/xen/arch/x86/include/asm/spec_ctrl.h
index 9403b81dc7..ee5c7b8d54 100644
--- a/xen/arch/x86/include/asm/spec_ctrl.h
+++ b/xen/arch/x86/include/asm/spec_ctrl.h
@@ -66,7 +66,6 @@ void init_speculation_mitigations(void);
 void spec_ctrl_init_domain(struct domain *d);
 
 extern int8_t opt_ibpb_ctxt_switch;
-extern bool opt_ssbd;
 extern int8_t opt_eager_fpu;
 extern int8_t opt_l1d_flush;
 
diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
index 0b94af6b86..dcee9795a5 100644
--- a/xen/arch/x86/spec_ctrl.c
+++ b/xen/arch/x86/spec_ctrl.c
@@ -56,7 +56,6 @@ static enum ind_thunk {
 
 static int8_t __initdata opt_ibrs = -1;
 int8_t __initdata opt_stibp = -1;
-bool __ro_after_init opt_ssbd;
 int8_t __initdata opt_psfd = -1;
 
 int8_t __ro_after_init opt_ibpb_ctxt_switch = -1;
@@ -126,7 +125,6 @@ static int __init cf_check parse_spec_ctrl(const char *s)
             opt_thunk = THUNK_JMP;
             opt_ibrs = 0;
             opt_ibpb_ctxt_switch = false;
-            opt_ssbd = false;
             opt_l1d_flush = 0;
             opt_branch_harden = false;
             opt_srb_lock = 0;
@@ -263,8 +261,6 @@ static int __init cf_check parse_spec_ctrl(const char *s)
             opt_ibrs = val;
         else if ( (val = parse_boolean("stibp", s, ss)) >= 0 )
             opt_stibp = val;
-        else if ( (val = parse_boolean("ssbd", s, ss)) >= 0 )
-            opt_ssbd = val;
         else if ( (val = parse_boolean("psfd", s, ss)) >= 0 )
             opt_psfd = val;
 
@@ -471,7 +467,7 @@ static void __init print_details(enum ind_thunk thunk, uint64_t caps)
                "\n");
 
     /* Settings for Xen's protection, irrespective of guests. */
-    printk("  Xen settings: BTI-Thunk %s, SPEC_CTRL: %s%s%s%s%s, Other:%s%s%s%s%s\n",
+    printk("  Xen settings: BTI-Thunk %s, SPEC_CTRL: %s%s%s%s, Other:%s%s%s%s%s\n",
            thunk == THUNK_NONE      ? "N/A" :
            thunk == THUNK_RETPOLINE ? "RETPOLINE" :
            thunk == THUNK_LFENCE    ? "LFENCE" :
@@ -482,9 +478,6 @@ static void __init print_details(enum ind_thunk thunk, uint64_t caps)
            (!boot_cpu_has(X86_FEATURE_STIBP) &&
             !boot_cpu_has(X86_FEATURE_AMD_STIBP))    ? "" :
            (default_xen_spec_ctrl & SPEC_CTRL_STIBP) ? " STIBP+" : " STIBP-",
-           (!boot_cpu_has(X86_FEATURE_SSBD) &&
-            !boot_cpu_has(X86_FEATURE_AMD_SSBD))     ? "" :
-           (default_xen_spec_ctrl & SPEC_CTRL_SSBD)  ? " SSBD+" : " SSBD-",
            (!boot_cpu_has(X86_FEATURE_PSFD) &&
             !boot_cpu_has(X86_FEATURE_INTEL_PSFD))   ? "" :
            (default_xen_spec_ctrl & SPEC_CTRL_PSFD)  ? " PSFD+" : " PSFD-",
@@ -1274,16 +1267,6 @@ void __init init_speculation_mitigations(void)
                        boot_cpu_has(X86_FEATURE_AMD_STIBP)) )
         default_xen_spec_ctrl |= SPEC_CTRL_STIBP;
 
-    if ( opt_ssbd && (boot_cpu_has(X86_FEATURE_SSBD) ||
-                      boot_cpu_has(X86_FEATURE_AMD_SSBD)) )
-    {
-        /* SSBD implies PSFD */
-        if ( opt_psfd == -1 )
-            opt_psfd = 1;
-
-        default_xen_spec_ctrl |= SPEC_CTRL_SSBD;
-    }
-
     /*
      * Don't use PSFD by default.  AMD designed the predictor to
      * auto-clear on privilege change.  PSFD is implied by SSBD, which is
-- 
2.37.3

[PATCH 4/4] amd/virt_ssbd: add to max HVM policy when SSB_NO is available

[Roger Pau Monne]

Hardware that exposes SSB_NO can implement the setting of SSBD as a
no-op because it's not affected by SSB.

Take advantage of that and allow exposing VIRT_SPEC_CTRL.SSBD to guest
running on hadrware that has SSB_NO.  Only set VIRT_SSBD on the max
policy though, as the feature is only intended to be used for
migration compatibility.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
As there's no hardware with SSB_NO so far the patch is untested.  Post
it for reference if there's hardware with the bit set.
---
 xen/arch/x86/cpu/amd.c | 4 +++-
 xen/arch/x86/cpuid.c   | 7 ++++++-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index cfeb8d1daf..74cfeffc29 100644
--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -814,7 +814,9 @@ void amd_set_ssbd(bool enable)
 		wrmsr(MSR_VIRT_SPEC_CTRL, enable ? SPEC_CTRL_SSBD : 0, 0);
 	else if ( amd_legacy_ssbd )
 		core_set_legacy_ssbd(enable);
-	else
+	else if ( cpu_has_ssb_no ) {
+		/* Nothing to do. */
+	} else
 		ASSERT_UNREACHABLE();
 }
 
diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index acc2f606ce..e394dbe669 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -558,11 +558,16 @@ static void __init calculate_hvm_max_policy(void)
         __clear_bit(X86_FEATURE_IBRSB, hvm_featureset);
         __clear_bit(X86_FEATURE_IBRS, hvm_featureset);
     }
-    else if ( boot_cpu_has(X86_FEATURE_AMD_SSBD) )
+    else if ( boot_cpu_has(X86_FEATURE_AMD_SSBD) ||
+              boot_cpu_has(X86_FEATURE_SSB_NO) )
         /*
          * If SPEC_CTRL.SSBD is available VIRT_SPEC_CTRL.SSBD can be exposed
          * and implemented using the former. Expose in the max policy only as
          * the preference is for guests to use SPEC_CTRL.SSBD if available.
+         *
+         * Allow VIRT_SSBD in the max policy if SSB_NO is exposed for migration
+         * compatibility reasons.  If SSB_NO is present setting
+         * VIRT_SPEC_CTRL.SSBD is a no-op.
          */
         __set_bit(X86_FEATURE_VIRT_SSBD, hvm_featureset);
 
-- 
2.37.3

Re: [PATCH for-4.17 1/4] amd/virt_ssbd: set SSBD at vCPU context switch

[Jan Beulich]

On 11.10.2022 18:02, Roger Pau Monne wrote:
> @@ -140,6 +135,7 @@ __UNLIKELY_END(nsvm_hap)
>           */
>          stgi
>  GLOBAL(svm_stgi_label)
> +
>          mov  %rsp,%rdi
>          call svm_vmexit_handler
>          jmp  .Lsvm_do_resume

Seemingly stray change?

> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -973,6 +973,14 @@ static void cf_check svm_ctxt_switch_from(struct vcpu *v)
>  
>      /* Resume use of ISTs now that the host TR is reinstated. */
>      enable_each_ist(idt_tables[cpu]);
> +
> +    /*
> +     * Clear previous guest selection of SSBD if set.  Note that SPEC_CTRL.SSBD
> +     * is already cleared by svm_vmexit_spec_ctrl.
> +     */
> +    if ( v->domain->arch.cpuid->extd.virt_ssbd &&

With this false, can ...

> +         (v->arch.msrs->virt_spec_ctrl.raw & SPEC_CTRL_SSBD) )

... this bit ever be set? IOW if the former condition actually needed here?

> +        amd_set_ssbd(false);
>  }
>  
>  static void cf_check svm_ctxt_switch_to(struct vcpu *v)
> @@ -1000,6 +1008,11 @@ static void cf_check svm_ctxt_switch_to(struct vcpu *v)
>  
>      if ( cpu_has_msr_tsc_aux )
>          wrmsr_tsc_aux(v->arch.msrs->tsc_aux);
> +
> +    /* Load SSBD if set by the guest. */
> +    if ( v->domain->arch.cpuid->extd.virt_ssbd &&
> +         (v->arch.msrs->virt_spec_ctrl.raw & SPEC_CTRL_SSBD) )
> +        amd_set_ssbd(true);
>  }

Same here then.

> @@ -2518,6 +2531,10 @@ static void cf_check svm_set_reg(struct vcpu *v, unsigned int reg, uint64_t val)
>          vmcb->spec_ctrl = val;
>          break;
>  
> +    case MSR_VIRT_SPEC_CTRL:
> +        amd_set_ssbd(v->arch.msrs->virt_spec_ctrl.raw);

Would seem cheaper to pass "val & SPEC_CTRL_SSBD" here.

Jan

Re: [PATCH for-4.17 2/4] amd: remove VIRT_SC_MSR_HVM synthetic feature

[Jan Beulich]

On 11.10.2022 18:02, Roger Pau Monne wrote:
> Since the VIRT_SPEC_CTRL.SSBD selection is no longer context switched
> on vm{entry,exit} there's no need to use a synthetic feature bit for
> it anymore.
> 
> Remove the bit and instead use a global variable.
> 
> No functional change intended.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Re: [PATCH for-4.17 3/4] amd/ssbd: remove hypervisor SSBD selection

[Jan Beulich]

On 11.10.2022 18:02, Roger Pau Monne wrote:
> @@ -2365,12 +2365,6 @@ On hardware supporting STIBP (Single Thread Indirect Branch Predictors), the
>  By default, Xen will use STIBP when IBRS is in use (IBRS implies STIBP), and
>  when hardware hints recommend using it as a blanket setting.
>  
> -On hardware supporting SSBD (Speculative Store Bypass Disable), the `ssbd=`
> -option can be used to force or prevent Xen using the feature itself.

Why would we want to take away this level of control? Shouldn't we turn this
on while in Xen if so requested? Which would then either mean enabling it on
VMEXIT if a guest has it off, or running with it turned on using the OR of
guest and host settings.

Jan

Re: [PATCH 4/4] amd/virt_ssbd: add to max HVM policy when SSB_NO is available

[Jan Beulich]

On 11.10.2022 18:02, Roger Pau Monne wrote:
> --- a/xen/arch/x86/cpu/amd.c
> +++ b/xen/arch/x86/cpu/amd.c
> @@ -814,7 +814,9 @@ void amd_set_ssbd(bool enable)
>  		wrmsr(MSR_VIRT_SPEC_CTRL, enable ? SPEC_CTRL_SSBD : 0, 0);
>  	else if ( amd_legacy_ssbd )
>  		core_set_legacy_ssbd(enable);
> -	else
> +	else if ( cpu_has_ssb_no ) {

Nit: While already an issue in patch 1, it is actually the combination
of inner blanks and brace placement which made me spot the style issue
here.

> +		/* Nothing to do. */

How is the late placement here in line with ...

> --- a/xen/arch/x86/cpuid.c
> +++ b/xen/arch/x86/cpuid.c
> @@ -558,11 +558,16 @@ static void __init calculate_hvm_max_policy(void)
>          __clear_bit(X86_FEATURE_IBRSB, hvm_featureset);
>          __clear_bit(X86_FEATURE_IBRS, hvm_featureset);
>      }
> -    else if ( boot_cpu_has(X86_FEATURE_AMD_SSBD) )
> +    else if ( boot_cpu_has(X86_FEATURE_AMD_SSBD) ||
> +              boot_cpu_has(X86_FEATURE_SSB_NO) )
>          /*
>           * If SPEC_CTRL.SSBD is available VIRT_SPEC_CTRL.SSBD can be exposed
>           * and implemented using the former. Expose in the max policy only as
>           * the preference is for guests to use SPEC_CTRL.SSBD if available.
> +         *
> +         * Allow VIRT_SSBD in the max policy if SSB_NO is exposed for migration
> +         * compatibility reasons.  If SSB_NO is present setting
> +         * VIRT_SPEC_CTRL.SSBD is a no-op.
>           */
>          __set_bit(X86_FEATURE_VIRT_SSBD, hvm_featureset);

... this comment addition talking about "no-op"?

Jan

Re: [PATCH for-4.17 1/4] amd/virt_ssbd: set SSBD at vCPU context switch

[Roger Pau Monné]

On Wed, Oct 12, 2022 at 10:26:19AM +0200, Jan Beulich wrote:
> On 11.10.2022 18:02, Roger Pau Monne wrote:
> > @@ -140,6 +135,7 @@ __UNLIKELY_END(nsvm_hap)
> >           */
> >          stgi
> >  GLOBAL(svm_stgi_label)
> > +
> >          mov  %rsp,%rdi
> >          call svm_vmexit_handler
> >          jmp  .Lsvm_do_resume
> 
> Seemingly stray change?

Urg, this was a squash of initially two separate patches and I didn't
pay enough attention at the end result not introducing such spurious
changes, sorry.

> > --- a/xen/arch/x86/hvm/svm/svm.c
> > +++ b/xen/arch/x86/hvm/svm/svm.c
> > @@ -973,6 +973,14 @@ static void cf_check svm_ctxt_switch_from(struct vcpu *v)
> >  
> >      /* Resume use of ISTs now that the host TR is reinstated. */
> >      enable_each_ist(idt_tables[cpu]);
> > +
> > +    /*
> > +     * Clear previous guest selection of SSBD if set.  Note that SPEC_CTRL.SSBD
> > +     * is already cleared by svm_vmexit_spec_ctrl.
> > +     */
> > +    if ( v->domain->arch.cpuid->extd.virt_ssbd &&
> 
> With this false, can ...
> 
> > +         (v->arch.msrs->virt_spec_ctrl.raw & SPEC_CTRL_SSBD) )
> 
> ... this bit ever be set? IOW if the former condition actually needed here?

Hm, right, I guess it's not helpful to gate accessing the field to the
CPUID bit, as it should never be set otherwise.

> > +        amd_set_ssbd(false);
> >  }
> >  
> >  static void cf_check svm_ctxt_switch_to(struct vcpu *v)
> > @@ -1000,6 +1008,11 @@ static void cf_check svm_ctxt_switch_to(struct vcpu *v)
> >  
> >      if ( cpu_has_msr_tsc_aux )
> >          wrmsr_tsc_aux(v->arch.msrs->tsc_aux);
> > +
> > +    /* Load SSBD if set by the guest. */
> > +    if ( v->domain->arch.cpuid->extd.virt_ssbd &&
> > +         (v->arch.msrs->virt_spec_ctrl.raw & SPEC_CTRL_SSBD) )
> > +        amd_set_ssbd(true);
> >  }
> 
> Same here then.
> 
> > @@ -2518,6 +2531,10 @@ static void cf_check svm_set_reg(struct vcpu *v, unsigned int reg, uint64_t val)
> >          vmcb->spec_ctrl = val;
> >          break;
> >  
> > +    case MSR_VIRT_SPEC_CTRL:
> > +        amd_set_ssbd(v->arch.msrs->virt_spec_ctrl.raw);
> 
> Would seem cheaper to pass "val & SPEC_CTRL_SSBD" here.

Yes, a couple less dereferences.

Thanks, Roger.

Re: [PATCH for-4.17 3/4] amd/ssbd: remove hypervisor SSBD selection

[Roger Pau Monné]

On Wed, Oct 12, 2022 at 10:30:45AM +0200, Jan Beulich wrote:
> On 11.10.2022 18:02, Roger Pau Monne wrote:
> > @@ -2365,12 +2365,6 @@ On hardware supporting STIBP (Single Thread Indirect Branch Predictors), the
> >  By default, Xen will use STIBP when IBRS is in use (IBRS implies STIBP), and
> >  when hardware hints recommend using it as a blanket setting.
> >  
> > -On hardware supporting SSBD (Speculative Store Bypass Disable), the `ssbd=`
> > -option can be used to force or prevent Xen using the feature itself.
> 
> Why would we want to take away this level of control? Shouldn't we turn this
> on while in Xen if so requested? Which would then either mean enabling it on
> VMEXIT if a guest has it off, or running with it turned on using the OR of
> guest and host settings.

Right, but then we need to context switch the value on vm{entry,exit}
which is problematic.  I could move the context switch code code out
of the GIF=0 region, and assume that NMIs executing with the guest
selection of SSBD are OK.

Alternatively setting ssbd= on the command line could be taken as a
value to enforce for the whole system and prevent guest attempts to
change it, not exposing VIRT_SSBD, AMD_SSBD or SSBD (haven't
looked at whether not exposing the SSBD CPUID related to
SPEC_CTRL.SSBD will have impact on other features).

I was under the impression that the command line ssbd option was added
to cope with Xen not exposing the feature to guests. Now that the
feature is exposed guests should be free to make use of it, and hence
there's no need to force a value for Xen.

Thanks, Roger.

Re: [PATCH 4/4] amd/virt_ssbd: add to max HVM policy when SSB_NO is available

[Roger Pau Monné]

On Wed, Oct 12, 2022 at 10:36:57AM +0200, Jan Beulich wrote:
> On 11.10.2022 18:02, Roger Pau Monne wrote:
> > --- a/xen/arch/x86/cpu/amd.c
> > +++ b/xen/arch/x86/cpu/amd.c
> > @@ -814,7 +814,9 @@ void amd_set_ssbd(bool enable)
> >  		wrmsr(MSR_VIRT_SPEC_CTRL, enable ? SPEC_CTRL_SSBD : 0, 0);
> >  	else if ( amd_legacy_ssbd )
> >  		core_set_legacy_ssbd(enable);
> > -	else
> > +	else if ( cpu_has_ssb_no ) {
> 
> Nit: While already an issue in patch 1, it is actually the combination
> of inner blanks and brace placement which made me spot the style issue
> here.

Oh, indeed, extra spaces.

> > +		/* Nothing to do. */
> 
> How is the late placement here in line with ...
> 
> > --- a/xen/arch/x86/cpuid.c
> > +++ b/xen/arch/x86/cpuid.c
> > @@ -558,11 +558,16 @@ static void __init calculate_hvm_max_policy(void)
> >          __clear_bit(X86_FEATURE_IBRSB, hvm_featureset);
> >          __clear_bit(X86_FEATURE_IBRS, hvm_featureset);
> >      }
> > -    else if ( boot_cpu_has(X86_FEATURE_AMD_SSBD) )
> > +    else if ( boot_cpu_has(X86_FEATURE_AMD_SSBD) ||
> > +              boot_cpu_has(X86_FEATURE_SSB_NO) )
> >          /*
> >           * If SPEC_CTRL.SSBD is available VIRT_SPEC_CTRL.SSBD can be exposed
> >           * and implemented using the former. Expose in the max policy only as
> >           * the preference is for guests to use SPEC_CTRL.SSBD if available.
> > +         *
> > +         * Allow VIRT_SSBD in the max policy if SSB_NO is exposed for migration
> > +         * compatibility reasons.  If SSB_NO is present setting
> > +         * VIRT_SPEC_CTRL.SSBD is a no-op.
> >           */
> >          __set_bit(X86_FEATURE_VIRT_SSBD, hvm_featureset);
> 
> ... this comment addition talking about "no-op"?

We need the empty `else if ...` body in order to avoid hitting the
ASSERT, but a guest setting VIRT_SPEC_CTRl.SSBD on a system that has
SSB_NO will not result in any setting being propagated to the
hardware.  I can make that clearer.  In any case I'm unable to test
the patch because there's no hw with the feature that I'm aware of.

Thanks, Roger.

Re: [PATCH for-4.17 3/4] amd/ssbd: remove hypervisor SSBD selection

[Jan Beulich]

On 13.10.2022 15:50, Roger Pau Monné wrote:
> On Wed, Oct 12, 2022 at 10:30:45AM +0200, Jan Beulich wrote:
>> On 11.10.2022 18:02, Roger Pau Monne wrote:
>>> @@ -2365,12 +2365,6 @@ On hardware supporting STIBP (Single Thread Indirect Branch Predictors), the
>>>  By default, Xen will use STIBP when IBRS is in use (IBRS implies STIBP), and
>>>  when hardware hints recommend using it as a blanket setting.
>>>  
>>> -On hardware supporting SSBD (Speculative Store Bypass Disable), the `ssbd=`
>>> -option can be used to force or prevent Xen using the feature itself.
>>
>> Why would we want to take away this level of control? Shouldn't we turn this
>> on while in Xen if so requested? Which would then either mean enabling it on
>> VMEXIT if a guest has it off, or running with it turned on using the OR of
>> guest and host settings.
> 
> Right, but then we need to context switch the value on vm{entry,exit}
> which is problematic.  I could move the context switch code code out
> of the GIF=0 region, and assume that NMIs executing with the guest
> selection of SSBD are OK.
> 
> Alternatively setting ssbd= on the command line could be taken as a
> value to enforce for the whole system and prevent guest attempts to
> change it, not exposing VIRT_SSBD, AMD_SSBD or SSBD (haven't
> looked at whether not exposing the SSBD CPUID related to
> SPEC_CTRL.SSBD will have impact on other features).

That would be my preference (albeit I'm uncertain about the "not exposing"
part, as we don't want to misguide guests into thinking they're unsafe or
can't guarantee safety when requested by user mode code), but ...

> I was under the impression that the command line ssbd option was added
> to cope with Xen not exposing the feature to guests. Now that the
> feature is exposed guests should be free to make use of it, and hence
> there's no need to force a value for Xen.

... me not having had this understanding may have been wrong on my part.
Andrew - any chance you could clarify (original) intentions here?

Jan

Re: [PATCH 4/4] amd/virt_ssbd: add to max HVM policy when SSB_NO is available

[Jan Beulich]

On 13.10.2022 16:06, Roger Pau Monné wrote:
> On Wed, Oct 12, 2022 at 10:36:57AM +0200, Jan Beulich wrote:
>> On 11.10.2022 18:02, Roger Pau Monne wrote:
>>> --- a/xen/arch/x86/cpu/amd.c
>>> +++ b/xen/arch/x86/cpu/amd.c
>>> @@ -814,7 +814,9 @@ void amd_set_ssbd(bool enable)
>>>  		wrmsr(MSR_VIRT_SPEC_CTRL, enable ? SPEC_CTRL_SSBD : 0, 0);
>>>  	else if ( amd_legacy_ssbd )
>>>  		core_set_legacy_ssbd(enable);
>>> -	else
>>> +	else if ( cpu_has_ssb_no ) {
>>
>> Nit: While already an issue in patch 1, it is actually the combination
>> of inner blanks and brace placement which made me spot the style issue
>> here.
> 
> Oh, indeed, extra spaces.
> 
>>> +		/* Nothing to do. */
>>
>> How is the late placement here in line with ...
>>
>>> --- a/xen/arch/x86/cpuid.c
>>> +++ b/xen/arch/x86/cpuid.c
>>> @@ -558,11 +558,16 @@ static void __init calculate_hvm_max_policy(void)
>>>          __clear_bit(X86_FEATURE_IBRSB, hvm_featureset);
>>>          __clear_bit(X86_FEATURE_IBRS, hvm_featureset);
>>>      }
>>> -    else if ( boot_cpu_has(X86_FEATURE_AMD_SSBD) )
>>> +    else if ( boot_cpu_has(X86_FEATURE_AMD_SSBD) ||
>>> +              boot_cpu_has(X86_FEATURE_SSB_NO) )
>>>          /*
>>>           * If SPEC_CTRL.SSBD is available VIRT_SPEC_CTRL.SSBD can be exposed
>>>           * and implemented using the former. Expose in the max policy only as
>>>           * the preference is for guests to use SPEC_CTRL.SSBD if available.
>>> +         *
>>> +         * Allow VIRT_SSBD in the max policy if SSB_NO is exposed for migration
>>> +         * compatibility reasons.  If SSB_NO is present setting
>>> +         * VIRT_SPEC_CTRL.SSBD is a no-op.
>>>           */
>>>          __set_bit(X86_FEATURE_VIRT_SSBD, hvm_featureset);
>>
>> ... this comment addition talking about "no-op"?
> 
> We need the empty `else if ...` body in order to avoid hitting the
> ASSERT, but a guest setting VIRT_SPEC_CTRl.SSBD on a system that has
> SSB_NO will not result in any setting being propagated to the
> hardware.  I can make that clearer.

I guess my question was more towards: Shouldn't that check in
amd_set_ssbd() move ahead?

As an aside I notice you use cpu_has_ssb_no there but not here. I
might guess this is because of the adjacent existing
boot_cpu_has(), but it still strikes me as a little odd (as in:
undue open-coding).

Jan

Re: [PATCH 4/4] amd/virt_ssbd: add to max HVM policy when SSB_NO is available

[Roger Pau Monné]

On Thu, Oct 13, 2022 at 04:43:15PM +0200, Jan Beulich wrote:
> On 13.10.2022 16:06, Roger Pau Monné wrote:
> > On Wed, Oct 12, 2022 at 10:36:57AM +0200, Jan Beulich wrote:
> >> On 11.10.2022 18:02, Roger Pau Monne wrote:
> >>> --- a/xen/arch/x86/cpu/amd.c
> >>> +++ b/xen/arch/x86/cpu/amd.c
> >>> @@ -814,7 +814,9 @@ void amd_set_ssbd(bool enable)
> >>>  		wrmsr(MSR_VIRT_SPEC_CTRL, enable ? SPEC_CTRL_SSBD : 0, 0);
> >>>  	else if ( amd_legacy_ssbd )
> >>>  		core_set_legacy_ssbd(enable);
> >>> -	else
> >>> +	else if ( cpu_has_ssb_no ) {
> >>
> >> Nit: While already an issue in patch 1, it is actually the combination
> >> of inner blanks and brace placement which made me spot the style issue
> >> here.
> > 
> > Oh, indeed, extra spaces.
> > 
> >>> +		/* Nothing to do. */
> >>
> >> How is the late placement here in line with ...
> >>
> >>> --- a/xen/arch/x86/cpuid.c
> >>> +++ b/xen/arch/x86/cpuid.c
> >>> @@ -558,11 +558,16 @@ static void __init calculate_hvm_max_policy(void)
> >>>          __clear_bit(X86_FEATURE_IBRSB, hvm_featureset);
> >>>          __clear_bit(X86_FEATURE_IBRS, hvm_featureset);
> >>>      }
> >>> -    else if ( boot_cpu_has(X86_FEATURE_AMD_SSBD) )
> >>> +    else if ( boot_cpu_has(X86_FEATURE_AMD_SSBD) ||
> >>> +              boot_cpu_has(X86_FEATURE_SSB_NO) )
> >>>          /*
> >>>           * If SPEC_CTRL.SSBD is available VIRT_SPEC_CTRL.SSBD can be exposed
> >>>           * and implemented using the former. Expose in the max policy only as
> >>>           * the preference is for guests to use SPEC_CTRL.SSBD if available.
> >>> +         *
> >>> +         * Allow VIRT_SSBD in the max policy if SSB_NO is exposed for migration
> >>> +         * compatibility reasons.  If SSB_NO is present setting
> >>> +         * VIRT_SPEC_CTRL.SSBD is a no-op.
> >>>           */
> >>>          __set_bit(X86_FEATURE_VIRT_SSBD, hvm_featureset);
> >>
> >> ... this comment addition talking about "no-op"?
> > 
> > We need the empty `else if ...` body in order to avoid hitting the
> > ASSERT, but a guest setting VIRT_SPEC_CTRl.SSBD on a system that has
> > SSB_NO will not result in any setting being propagated to the
> > hardware.  I can make that clearer.
> 
> I guess my question was more towards: Shouldn't that check in
> amd_set_ssbd() move ahead?

Right, I assumed that cpu_has_ssb_no would be exclusive with any other
SSBD mechanism, but that doesn't need to be true.

> As an aside I notice you use cpu_has_ssb_no there but not here. I
> might guess this is because of the adjacent existing
> boot_cpu_has(), but it still strikes me as a little odd (as in:
> undue open-coding).

Indeed, the whole function uses boot_cpu_has() so it seemed clearer to
also use it for SSB_NO.

Thanks, Roger.

Re: [PATCH for-4.17 3/4] amd/ssbd: remove hypervisor SSBD selection

[Roger Pau Monné]

On Thu, Oct 13, 2022 at 04:20:45PM +0200, Jan Beulich wrote:
> On 13.10.2022 15:50, Roger Pau Monné wrote:
> > On Wed, Oct 12, 2022 at 10:30:45AM +0200, Jan Beulich wrote:
> >> On 11.10.2022 18:02, Roger Pau Monne wrote:
> >>> @@ -2365,12 +2365,6 @@ On hardware supporting STIBP (Single Thread Indirect Branch Predictors), the
> >>>  By default, Xen will use STIBP when IBRS is in use (IBRS implies STIBP), and
> >>>  when hardware hints recommend using it as a blanket setting.
> >>>  
> >>> -On hardware supporting SSBD (Speculative Store Bypass Disable), the `ssbd=`
> >>> -option can be used to force or prevent Xen using the feature itself.
> >>
> >> Why would we want to take away this level of control? Shouldn't we turn this
> >> on while in Xen if so requested? Which would then either mean enabling it on
> >> VMEXIT if a guest has it off, or running with it turned on using the OR of
> >> guest and host settings.
> > 
> > Right, but then we need to context switch the value on vm{entry,exit}
> > which is problematic.  I could move the context switch code code out
> > of the GIF=0 region, and assume that NMIs executing with the guest
> > selection of SSBD are OK.
> > 
> > Alternatively setting ssbd= on the command line could be taken as a
> > value to enforce for the whole system and prevent guest attempts to
> > change it, not exposing VIRT_SSBD, AMD_SSBD or SSBD (haven't
> > looked at whether not exposing the SSBD CPUID related to
> > SPEC_CTRL.SSBD will have impact on other features).
> 
> That would be my preference (albeit I'm uncertain about the "not exposing"
> part, as we don't want to misguide guests into thinking they're unsafe or
> can't guarantee safety when requested by user mode code), but ...

For ssbd=1 we could expose the SSBD controls, as the guest trying to
turn it off would have no effect and it would still be protected.

OTOH if the user sets ssbd=0 on the command line then exposing the
SSBD controls to the guest would be misleading, as the guest setting
SSBD will have no effect and thus it won't be protected when it thinks
it is.

Thanks, Roger.

Re: [PATCH for-4.17 3/4] amd/ssbd: remove hypervisor SSBD selection

[Roger Pau Monné]

On Thu, Oct 13, 2022 at 04:20:45PM +0200, Jan Beulich wrote:
> On 13.10.2022 15:50, Roger Pau Monné wrote:
> > I was under the impression that the command line ssbd option was added
> > to cope with Xen not exposing the feature to guests. Now that the
> > feature is exposed guests should be free to make use of it, and hence
> > there's no need to force a value for Xen.
> 
> ... me not having had this understanding may have been wrong on my part.
> Andrew - any chance you could clarify (original) intentions here?

I realized I wasn't taking PV into account, and PV guests on AMD
cannot set ssbd, so the option cannot be removed.

Thanks, Roger.

Re: [PATCH for-4.17 3/4] amd/ssbd: remove hypervisor SSBD selection

[Jan Beulich]

On 14.10.2022 10:17, Roger Pau Monné wrote:
> On Thu, Oct 13, 2022 at 04:20:45PM +0200, Jan Beulich wrote:
>> On 13.10.2022 15:50, Roger Pau Monné wrote:
>>> On Wed, Oct 12, 2022 at 10:30:45AM +0200, Jan Beulich wrote:
>>>> On 11.10.2022 18:02, Roger Pau Monne wrote:
>>>>> @@ -2365,12 +2365,6 @@ On hardware supporting STIBP (Single Thread Indirect Branch Predictors), the
>>>>>  By default, Xen will use STIBP when IBRS is in use (IBRS implies STIBP), and
>>>>>  when hardware hints recommend using it as a blanket setting.
>>>>>  
>>>>> -On hardware supporting SSBD (Speculative Store Bypass Disable), the `ssbd=`
>>>>> -option can be used to force or prevent Xen using the feature itself.
>>>>
>>>> Why would we want to take away this level of control? Shouldn't we turn this
>>>> on while in Xen if so requested? Which would then either mean enabling it on
>>>> VMEXIT if a guest has it off, or running with it turned on using the OR of
>>>> guest and host settings.
>>>
>>> Right, but then we need to context switch the value on vm{entry,exit}
>>> which is problematic.  I could move the context switch code code out
>>> of the GIF=0 region, and assume that NMIs executing with the guest
>>> selection of SSBD are OK.
>>>
>>> Alternatively setting ssbd= on the command line could be taken as a
>>> value to enforce for the whole system and prevent guest attempts to
>>> change it, not exposing VIRT_SSBD, AMD_SSBD or SSBD (haven't
>>> looked at whether not exposing the SSBD CPUID related to
>>> SPEC_CTRL.SSBD will have impact on other features).
>>
>> That would be my preference (albeit I'm uncertain about the "not exposing"
>> part, as we don't want to misguide guests into thinking they're unsafe or
>> can't guarantee safety when requested by user mode code), but ...
> 
> For ssbd=1 we could expose the SSBD controls, as the guest trying to
> turn it off would have no effect and it would still be protected.
> 
> OTOH if the user sets ssbd=0 on the command line then exposing the
> SSBD controls to the guest would be misleading, as the guest setting
> SSBD will have no effect and thus it won't be protected when it thinks
> it is.

Irrespective of your subsequent reply: Unlike "cpuid=no-ssbd",
"spec-ctrl=no-ssbd" ought to affect only Xen itself:

"On hardware supporting SSBD (Speculative Store Bypass Disable), the `ssbd=`
 option can be used to force or prevent Xen using the feature itself."

Jan

Re: [PATCH for-4.17 3/4] amd/ssbd: remove hypervisor SSBD selection

[Roger Pau Monné]

On Fri, Oct 14, 2022 at 11:00:10AM +0200, Jan Beulich wrote:
> On 14.10.2022 10:17, Roger Pau Monné wrote:
> > On Thu, Oct 13, 2022 at 04:20:45PM +0200, Jan Beulich wrote:
> >> On 13.10.2022 15:50, Roger Pau Monné wrote:
> >>> On Wed, Oct 12, 2022 at 10:30:45AM +0200, Jan Beulich wrote:
> >>>> On 11.10.2022 18:02, Roger Pau Monne wrote:
> >>>>> @@ -2365,12 +2365,6 @@ On hardware supporting STIBP (Single Thread Indirect Branch Predictors), the
> >>>>>  By default, Xen will use STIBP when IBRS is in use (IBRS implies STIBP), and
> >>>>>  when hardware hints recommend using it as a blanket setting.
> >>>>>  
> >>>>> -On hardware supporting SSBD (Speculative Store Bypass Disable), the `ssbd=`
> >>>>> -option can be used to force or prevent Xen using the feature itself.
> >>>>
> >>>> Why would we want to take away this level of control? Shouldn't we turn this
> >>>> on while in Xen if so requested? Which would then either mean enabling it on
> >>>> VMEXIT if a guest has it off, or running with it turned on using the OR of
> >>>> guest and host settings.
> >>>
> >>> Right, but then we need to context switch the value on vm{entry,exit}
> >>> which is problematic.  I could move the context switch code code out
> >>> of the GIF=0 region, and assume that NMIs executing with the guest
> >>> selection of SSBD are OK.
> >>>
> >>> Alternatively setting ssbd= on the command line could be taken as a
> >>> value to enforce for the whole system and prevent guest attempts to
> >>> change it, not exposing VIRT_SSBD, AMD_SSBD or SSBD (haven't
> >>> looked at whether not exposing the SSBD CPUID related to
> >>> SPEC_CTRL.SSBD will have impact on other features).
> >>
> >> That would be my preference (albeit I'm uncertain about the "not exposing"
> >> part, as we don't want to misguide guests into thinking they're unsafe or
> >> can't guarantee safety when requested by user mode code), but ...
> > 
> > For ssbd=1 we could expose the SSBD controls, as the guest trying to
> > turn it off would have no effect and it would still be protected.
> > 
> > OTOH if the user sets ssbd=0 on the command line then exposing the
> > SSBD controls to the guest would be misleading, as the guest setting
> > SSBD will have no effect and thus it won't be protected when it thinks
> > it is.
> 
> Irrespective of your subsequent reply: Unlike "cpuid=no-ssbd",
> "spec-ctrl=no-ssbd" ought to affect only Xen itself:
> 
> "On hardware supporting SSBD (Speculative Store Bypass Disable), the `ssbd=`
>  option can be used to force or prevent Xen using the feature itself."

So that brings us back to having to context switch SSBD on guest entry
and exit, and we could only do the SSBD switch at context switch if no
ssbd= option is used.

That would also prevent us from dropping the synthetic feature leaf.

I will wait for Andrews opinion on this one, I would like to make sure
we have reached consensus before I send a new version.

Thanks, Roger.