[PATCH for-4.17 v4 0/2] amd/virt_ssbd: refactoring and fixes

[PATCH for-4.17 v4 0/2] amd/virt_ssbd: refactoring and fixes

[Roger Pau Monne]

Hello,

Just two patches remaining, and the last one is already Acked.

First patch deals with moving the switching of SSBD from guest
vm{entry,exit} to vCPU context switch, and lets Xen run with the guest
SSBD selection under some circumstances by default.

Andrew has expressed reservations to me privately with patch 2/2, but
I'm still sending it so that comments can be made publicly (or the patch
applied).

Thanks, Roger.

Roger Pau Monne (2):
  amd/virt_ssbd: set SSBD at vCPU context switch
  amd: remove VIRT_SC_MSR_HVM synthetic feature

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

-- 
2.37.3

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

[Roger Pau Monne]

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

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, the documentation is updated to note this behavior.
Also note that then when `ssbd` is selected on the command line guest
SSBD selection will not have an effect, and the hypervisor will run
with SSBD unconditionally enabled when not using SPEC_CTRL itself.

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>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
Changes since v3:
 - Fix commit message order.
 - Remove msr.h comment about context switching virt_spec_ctrl.
 - s/amd_set_ssbd/amd_set_legacy_ssbd/.
 - Adjust comment about clearing SSBD in svm_ctxt_switch_from().

Changes since v2:
 - Fix calling set_reg unconditionally.
 - Fix comment.
 - Call amd_set_ssbd() from guest_wrmsr().

Changes since v1:
 - Just check virt_spec_ctrl value != 0 on context switch.
 - Remove stray asm newline.
 - Use val in svm_set_reg().
 - Fix style in amd.c.
 - Do not clear ssbd
---
 docs/misc/xen-command-line.pandoc | 10 +++---
 xen/arch/x86/cpu/amd.c            | 55 +++++++++++++++++--------------
 xen/arch/x86/hvm/svm/entry.S      |  6 ----
 xen/arch/x86/hvm/svm/svm.c        | 45 ++++++++++---------------
 xen/arch/x86/include/asm/msr.h    |  3 +-
 xen/arch/x86/msr.c                |  9 +++++
 6 files changed, 63 insertions(+), 65 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index 0fbdcb574f..424b12cfb2 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -2372,10 +2372,12 @@ 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.
+option can be used to force or prevent Xen using the feature itself.  The
+feature is virtualised for guests, independently of Xen's choice of setting.
+On AMD hardware, disabling Xen SSBD usage on the command line (`ssbd=0` which
+is the default value) can lead to Xen running with the guest SSBD selection
+depending on hardware support, on the same hardware setting `ssbd=1` will
+result in SSBD always being enabled, regardless of guest choice.
 
 On hardware supporting PSFD (Predictive Store Forwarding Disable), the `psfd=`
 option can be used to force or prevent Xen using the feature itself.  By
diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index 98c52d0686..a332087604 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;
@@ -753,7 +753,7 @@ 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)
+	    boot_cpu_data.x86_num_siblings <= 1 || opt_ssbd)
 		return true;
 
 	/*
@@ -776,46 +776,51 @@ 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++)
+		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_legacy_ssbd(bool enable)
+{
+	if (opt_ssbd)
+		/*
+		 * Ignore attempts to turn off SSBD, it's hardcoded on the
+		 * command line.
+		 */
+		return;
+
+	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..981cd82e7c 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,9 +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
          * to safely resolve any Spectre-v1 concerns in the above logic.
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 1aeaabcb13..fa73257203 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -973,6 +973,16 @@ 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]);
+
+    /*
+     * Possibly clear previous guest selection of SSBD if set.  Note that
+     * SPEC_CTRL.SSBD is already handled by svm_vmexit_spec_ctrl.
+     */
+    if ( v->arch.msrs->virt_spec_ctrl.raw & SPEC_CTRL_SSBD )
+    {
+        ASSERT(v->domain->arch.cpuid->extd.virt_ssbd);
+        amd_set_legacy_ssbd(false);
+    }
 }
 
 static void cf_check svm_ctxt_switch_to(struct vcpu *v)
@@ -1000,6 +1010,13 @@ 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->arch.msrs->virt_spec_ctrl.raw & SPEC_CTRL_SSBD )
+    {
+        ASSERT(v->domain->arch.cpuid->extd.virt_ssbd);
+        amd_set_legacy_ssbd(true);
+    }
 }
 
 static void noreturn cf_check svm_do_resume(void)
@@ -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/msr.h b/xen/arch/x86/include/asm/msr.h
index de18e90b2e..3eef1dfb98 100644
--- a/xen/arch/x86/include/asm/msr.h
+++ b/xen/arch/x86/include/asm/msr.h
@@ -378,8 +378,7 @@ struct vcpu_msrs
     /*
      * 0xc001011f - MSR_VIRT_SPEC_CTRL (if !X86_FEATURE_AMD_SSBD)
      *
-     * AMD only. Guest selected value, context switched on guest VM
-     * entry/exit.
+     * AMD only. Guest selected value.
      */
     struct {
         uint32_t raw;
diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index 95416995a5..cf46b18aa6 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -24,6 +24,7 @@
 #include <xen/nospec.h>
 #include <xen/sched.h>
 
+#include <asm/amd.h>
 #include <asm/debugreg.h>
 #include <asm/hvm/nestedhvm.h>
 #include <asm/hvm/viridian.h>
@@ -697,7 +698,15 @@ 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;
+            if ( v == curr )
+                /*
+                 * Propagate the value to hardware, as it won't be set on guest
+                 * resume path.
+                 */
+                amd_set_legacy_ssbd(val & SPEC_CTRL_SSBD);
+        }
         break;
 
     case MSR_AMD64_DE_CFG:
-- 
2.37.3

[PATCH for-4.17 v4 2/2] 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>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Release-acked-by: Henry Wang <Henry.Wang@arm.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 a332087604..9e3b9094d3 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 6a42f68542..a975d3de26 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_legacy_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

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

[Andrew Cooper]

On 15/11/2022 13:26, 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>
> Release-acked-by: Henry Wang <Henry.Wang@arm.com>

This is definitely not appropriate for 4.17, but it's a performance
regression in general, hence my firm and repeated objection to this
style of patch.

General synthetic bits have existed for several decades longer than
alternatives.  It has never ever been a rule, or even a recommendation,
to aggressively purge the non-alternative bits, because it's a provably
bad thing to do.


You are attempting a micro-optimisation, that won't produce any
improvement at all in centuries, while...

> diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
> index a332087604..9e3b9094d3 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;

... actually expending .rodata with something 8 times less efficiently
packed, and ...

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

... (Specific to this instance) making life harder for the people trying
to make CONFIG_AMD work, and ...

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

... breaking apart a single TEST instruction, which not only adds an
extra conditional merge, but now hits an cold-ish cache line everywhere
it's used.

Count how many synthetic feature bits it will actually take to change
the per-cpu data size, and realise that, when it will take more than 200
years at the current rate of accumulation, any believe that this is an
improvement to be had disappears.

Yes, it is only a micro regression, but you need a far better
justification than "there is a gain of 64 bytes per CPU which will be
non-theoretical in more than 200 years" when traded off vs the actual
512 bytes, plus extra code bloat bloat, plus reduced locality of data
that this "improvement" genuinely costs today.

~Andrew

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

[Jan Beulich]

On 15.11.2022 17:21, Andrew Cooper wrote:
> On 15/11/2022 13:26, 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>
>> Release-acked-by: Henry Wang <Henry.Wang@arm.com>
> 
> This is definitely not appropriate for 4.17, but it's a performance
> regression in general, hence my firm and repeated objection to this
> style of patch.
> 
> General synthetic bits have existed for several decades longer than
> alternatives.  It has never ever been a rule, or even a recommendation,
> to aggressively purge the non-alternative bits, because it's a provably
> bad thing to do.

There we are again - you state something as bad without really saying
why it is bad. My view is that synthetic bits were wrong to introduce
when they don't stand a chance of being used in an alternative.

I agree though that there's no strong need for this to make 4.17. It
may end up making backports slightly easier, as no such bit existed
in 4.16.

> You are attempting a micro-optimisation, that won't produce any
> improvement at all in centuries, while...
> 
>> diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
>> index a332087604..9e3b9094d3 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;
> 
> ... actually expending .rodata with something 8 times less efficiently
> packed, and ...

... as long as you're talking of just a single CPU. The break-even is
at 8 CPUs (8 bits used either way).

>> --- 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 ||
> 
> ... breaking apart a single TEST instruction, which not only adds an
> extra conditional merge, but now hits an cold-ish cache line everywhere
> it's used.
> 
> Count how many synthetic feature bits it will actually take to change
> the per-cpu data size, and realise that, when it will take more than 200
> years at the current rate of accumulation, any believe that this is an
> improvement to be had disappears.
> 
> Yes, it is only a micro regression, but you need a far better
> justification than "there is a gain of 64 bytes per CPU which will be
> non-theoretical in more than 200 years" when traded off vs the actual
> 512 bytes, plus extra code bloat bloat, plus reduced locality of data
> that this "improvement" genuinely costs today.

I don't see Roger stating anything like this.

I think we need to settle on at least halfway firm rules on when to use
synthetic feature bits and when to use plain global booleans. Without
that the tastes of the three of us are going to collide again every once
in a while.

Jan

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

[Roger Pau Monné]

On Tue, Nov 15, 2022 at 04:21:07PM +0000, Andrew Cooper wrote:
> On 15/11/2022 13:26, 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>
> > Release-acked-by: Henry Wang <Henry.Wang@arm.com>
> 
> This is definitely not appropriate for 4.17, but it's a performance
> regression in general, hence my firm and repeated objection to this
> style of patch.

While I don't have any objections in deferring this past 4.17, none of
the modified paths are performance sensitive AFAICT.

> General synthetic bits have existed for several decades longer than
> alternatives.  It has never ever been a rule, or even a recommendation,
> to aggressively purge the non-alternative bits, because it's a provably
> bad thing to do.
> 
> 
> You are attempting a micro-optimisation, that won't produce any
> improvement at all in centuries, while...

Oh, I wasn't attempting any micro-optimizations TBH, just didn't see
the need to keep this as a synthetic feature, and generally consider
better to use a global variable because it's IMO easier to follow.

> > diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
> > index a332087604..9e3b9094d3 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;
> 
> ... actually expending .rodata with something 8 times less efficiently
> packed, and ...
> 
> >  
> >  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>
> 
> ... (Specific to this instance) making life harder for the people trying
> to make CONFIG_AMD work, and ...

That's indeed a point, albeit I think adding a `#define
amd_virt_spec_ctrl false` won't be the bigger of the problems when
dealing with CONFIG_AMD, and will need to be done for other AMD
specific variables anyway.

> > 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 ||
> 
> ... breaking apart a single TEST instruction, which not only adds an
> extra conditional merge, but now hits an cold-ish cache line everywhere
> it's used.

Why does performance matter here?  It's an init function that prints
the speculation related settings to the screen, so that's likely to be
many times slower that accessing a cold cache line.

> Count how many synthetic feature bits it will actually take to change
> the per-cpu data size, and realise that, when it will take more than 200
> years at the current rate of accumulation, any believe that this is an
> improvement to be had disappears.
> 
> Yes, it is only a micro regression, but you need a far better
> justification than "there is a gain of 64 bytes per CPU which will be
> non-theoretical in more than 200 years" when traded off vs the actual
> 512 bytes, plus extra code bloat bloat, plus reduced locality of data
> that this "improvement" genuinely costs today.

I wasn't considering any of the above when proposing the change, my
only motivation was that global variables are clearer to use than
synthetic features, and I didn't see a need for a synthetic feature in
this case.  If we agree the above possible performance regressions
are worth it I'm fine keeping it as-is.

Now that I realize it amd_virt_spec_ctrl could even be plain __init.

Thanks, Roger.

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

[Andrew Cooper]

On 15/11/2022 16:44, Jan Beulich wrote:
> On 15.11.2022 17:21, Andrew Cooper wrote:
>> On 15/11/2022 13:26, 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>
>>> Release-acked-by: Henry Wang <Henry.Wang@arm.com>
>> This is definitely not appropriate for 4.17, but it's a performance
>> regression in general, hence my firm and repeated objection to this
>> style of patch.
>>
>> General synthetic bits have existed for several decades longer than
>> alternatives.  It has never ever been a rule, or even a recommendation,
>> to aggressively purge the non-alternative bits, because it's a provably
>> bad thing to do.
> There we are again - you state something as bad without really saying
> why it is bad.

You may not agree with the reasoning, but you are lying to yourself, if
no-one else, by claiming that no justification was presented.


> My view is that synthetic bits were wrong to introduce
> when they don't stand a chance of being used in an alternative.

Your view is incompatible with a linear interpretation of history, as
has been pointed repeatedly before by the fact that 1/3 of Xen's
synthetic features full predate the introduction of alternatives.

"I don't like using synthetic bits in this way" is a point of view, but
is not something that counters technical reasoning about the tradeoff in
question.

>
> I agree though that there's no strong need for this to make 4.17. It
> may end up making backports slightly easier, as no such bit existed
> in 4.16.

*This* is a good justification to take the change.

Equally, Roger's subsequent observation that it can actually live in
__initdata.

>> You are attempting a micro-optimisation, that won't produce any
>> improvement at all in centuries, while...
>>
>>> diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
>>> index a332087604..9e3b9094d3 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;
>> ... actually expending .rodata with something 8 times less efficiently
>> packed, and ...
> ... as long as you're talking of just a single CPU. The break-even is
> at 8 CPUs (8 bits used either way).

And still irrelevant when the size of the per-cpu data area doesn't
change for several centuries in the argued case.

> I think we need to settle on at least halfway firm rules on when to use
> synthetic feature bits and when to use plain global booleans. Without
> that the tastes of the three of us are going to collide again every once
> in a while.

Its very easy.  All other things being equal, synthetic features are the
most efficient option.

In most cases, things aren't all equal, and literally any
technically-credible justification will do.


If a tradeoff doesn't plausibly work within a decade, then it's probably
a waste of time raising, and definitely not a point to legitimately
object with.  Especially as in the past, I've already given you an
alternative course of action where the synthetic features aren't per-cpu...

~Andrew

Re: [PATCH for-4.17 v4 0/2] amd/virt_ssbd: refactoring and fixes

[Andrew Cooper]

On 15/11/2022 13:26, Roger Pau Monne wrote:
> Hello,
>
> Just two patches remaining, and the last one is already Acked.
>
> First patch deals with moving the switching of SSBD from guest
> vm{entry,exit} to vCPU context switch, and lets Xen run with the guest
> SSBD selection under some circumstances by default.
>
> Andrew has expressed reservations to me privately with patch 2/2, but
> I'm still sending it so that comments can be made publicly (or the patch
> applied).
>
> Thanks, Roger.

To avoid a further round trip, I've committed these patches with some
minor non-functional adjustments.  In patch 1, an expanded context
switching comment, and in patch 2, an adjusted commit message and
movement into __initdata.

I've also closed out #90, which closes one of the release blockers.

All other bugs can wait until 4.18, including the totally bizarre one
where using cpuid="host:virt-ssbd=1" in the xl.cfg file causes IBRS to
become hidden.  I confirmed this was pre-existing behaviour, but haven't
investigated further.

~Andrew

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

[Jan Beulich]

On 16.11.2022 00:54, Andrew Cooper wrote:
> On 15/11/2022 16:44, Jan Beulich wrote:
>> I think we need to settle on at least halfway firm rules on when to use
>> synthetic feature bits and when to use plain global booleans. Without
>> that the tastes of the three of us are going to collide again every once
>> in a while.
> 
> Its very easy.  All other things being equal, synthetic features are the
> most efficient option.

See Roger's better wording of "why use a more complicated construct when
a simple one will do". Yes, generated code may be better in certain cases,
but no, we don't always judge by that aspect alone. Source simplicity is
an important criteria, which at other times I recall you also weighing
higher than the performance of resulting code (especially when dealing
with performance aspects when they don't really matter at most/all use
sites of whichever construct).

Jan

Re: [PATCH for-4.17 v4 0/2] amd/virt_ssbd: refactoring and fixes

[Roger Pau Monné]

On Wed, Nov 16, 2022 at 12:27:29AM +0000, Andrew Cooper wrote:
> On 15/11/2022 13:26, Roger Pau Monne wrote:
> > Hello,
> >
> > Just two patches remaining, and the last one is already Acked.
> >
> > First patch deals with moving the switching of SSBD from guest
> > vm{entry,exit} to vCPU context switch, and lets Xen run with the guest
> > SSBD selection under some circumstances by default.
> >
> > Andrew has expressed reservations to me privately with patch 2/2, but
> > I'm still sending it so that comments can be made publicly (or the patch
> > applied).
> >
> > Thanks, Roger.
> 
> To avoid a further round trip, I've committed these patches with some
> minor non-functional adjustments.  In patch 1, an expanded context
> switching comment, and in patch 2, an adjusted commit message and
> movement into __initdata.

FWIW, I didn't expand the comment in patch 1 because I though it was
likely to get out of sync.

> I've also closed out #90, which closes one of the release blockers.
> 
> All other bugs can wait until 4.18, including the totally bizarre one
> where using cpuid="host:virt-ssbd=1" in the xl.cfg file causes IBRS to
> become hidden.  I confirmed this was pre-existing behaviour, but haven't
> investigated further.

Hm, I don't seem to be able to use cpuid="host:virt-ssbd=1", I get:

# xl create -c tests/virt-spec/test-hvm32-virt-spec.cfg
Parsing config from tests/virt-spec/test-hvm32-virt-spec.cfg
while parsing CPUID string: "host:virt-ssbd=1":
  error: first word must be "host"

However using
cpuid=["0x80000008:ebx=xxxxxx1xxxxxxxxxxxxxxxxxxxxxxxxx"] does work
and I can see both virt-ssbd and ibrs present in the guest CPUID on an
EPYC 7543P (Milan).

Regards, Roger.

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

[Roger Pau Monné]

On Wed, Nov 16, 2022 at 08:41:06AM +0100, Jan Beulich wrote:
> On 16.11.2022 00:54, Andrew Cooper wrote:
> > On 15/11/2022 16:44, Jan Beulich wrote:
> >> I think we need to settle on at least halfway firm rules on when to use
> >> synthetic feature bits and when to use plain global booleans. Without
> >> that the tastes of the three of us are going to collide again every once
> >> in a while.
> > 
> > Its very easy.  All other things being equal, synthetic features are the
> > most efficient option.
> 
> See Roger's better wording of "why use a more complicated construct when
> a simple one will do". Yes, generated code may be better in certain cases,
> but no, we don't always judge by that aspect alone. Source simplicity is
> an important criteria, which at other times I recall you also weighing
> higher than the performance of resulting code (especially when dealing
> with performance aspects when they don't really matter at most/all use
> sites of whichever construct).

I think it would be easier if we can discuss this in one of our x86
related meetings.  It's still unclear to me why a synthetic feature
would be preferred rather than a global variable in most cases (like
the one here, even if the variable didn't end up having the __init
attribute).

Thanks, Roger.