[PATCH 0/4] x86: RSBA and RRSBA handling

[PATCH 0/4] x86: RSBA and RRSBA handling

[Andrew Cooper]

This series deals with the hanlding of the RSBA and RRSBA bits across all
parts and all mistakes encountered in various microcode versions.

With it in place, here are some examples from various generations of Intel
hardware:

  BDX Raw
  BDX Host
  BDX HVM Max   rsba

  KBL Raw       rsba misc-pkg-ctrl energy-ctrl
  KBL Host      rsba misc-pkg-ctrl energy-ctrl
  KBL HVM Max   rsba

  SKX Raw       rsba misc-pkg-ctrl energy-ctrl
  SKX Host      rsba misc-pkg-ctrl energy-ctrl
  SKX HVM Max   rsba

  CFL Raw       rdcl-no eibrs skip-l1dfl mds-no tsx-ctrl misc-pkg-ctrl energy-ctrl fb-clear
  CFL Host      rdcl-no eibrs rsba skip-l1dfl mds-no tsx-ctrl misc-pkg-ctrl energy-ctrl fb-clear rrsba
  CFL HVM Max   rdcl-no eibrs rsba mds-no fb-clear rrsba

  CLX Raw       rdcl-no eibrs skip-l1dfl mds-no tsx-ctrl misc-pkg-ctrl energy-ctrl sbdr-ssdp-no psdp-no fb-clear rrsba
  CLX Host      rdcl-no eibrs rsba skip-l1dfl mds-no tsx-ctrl misc-pkg-ctrl energy-ctrl sbdr-ssdp-no psdp-no fb-clear rrsba
  CLX HVM Max   rdcl-no eibrs rsba mds-no sbdr-ssdp-no psdp-no fb-clear rrsba

  SPR Raw       rdcl-no eibrs skip-l1dfl mds-no if-pschange-mc-no tsx-ctrl taa-no misc-pkg-ctrl energy-ctrl
  SPR Host      rdcl-no eibrs rsba skip-l1dfl mds-no if-pschange-mc-no tsx-ctrl taa-no misc-pkg-ctrl energy-ctrl rrsba
  SPR HVM Max   rdcl-no eibrs rsba mds-no if-pschange-mc-no taa-no rrsba


Of note:
 * The SPR CPU is pre-release and didn't get the MMIO ucode in the end
   (sbdr-ssdp-no psdp-no fb-clear).
 * SKX/KBL enumerate RSBA following the energy filtering microcode.  Prior to
   that, they don't enumerate MSR_ARCH_CAPS at all.
 * CFL and SPR fails to enumerate both RSBA and RRSBA.  CLX fails to enumerate
   RSBA.  These should be addressed in due course.


Andrew Cooper (4):
  x86/spec-ctrl: Rename retpoline_safe() to retpoline_calculations()
  x86/spec-ctrl: Synthesize missing RSBA/RRSBA bits
  x86/cpu-policy: Rearrange guest_common_default_feature_adjustments()
  x86/cpu-policy: Derive {,R}RSBA for guest policies

 xen/arch/x86/cpu-policy.c             | 59 ++++++++++++++------
 xen/arch/x86/include/asm/cpufeature.h |  1 +
 xen/arch/x86/spec_ctrl.c              | 78 +++++++++++++++++++++------
 xen/tools/gen-cpuid.py                |  5 +-
 4 files changed, 111 insertions(+), 32 deletions(-)

-- 
2.30.2

[PATCH 1/4] x86/spec-ctrl: Rename retpoline_safe() to retpoline_calculations()

[Andrew Cooper]

This is prep work, split out to simply the diff on the following change.

 * Rename to retpoline_calculations(), and call unconditionally.  It is
   shortly going to synthesize missing enumerations required for guest safety.
 * For Broadwell, store the ucode revision calculation in a variable and fall
   out of the bottom of the switch statement.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
---
 xen/arch/x86/spec_ctrl.c | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
index 50d467f74cf8..0774d40627dd 100644
--- a/xen/arch/x86/spec_ctrl.c
+++ b/xen/arch/x86/spec_ctrl.c
@@ -579,9 +579,10 @@ static bool __init check_smt_enabled(void)
 }
 
 /* Calculate whether Retpoline is known-safe on this CPU. */
-static bool __init retpoline_safe(void)
+static bool __init retpoline_calculations(void)
 {
     unsigned int ucode_rev = this_cpu(cpu_sig).rev;
+    bool safe = false;
 
     if ( boot_cpu_data.x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON) )
         return true;
@@ -626,18 +627,18 @@ static bool __init retpoline_safe(void)
          * versions.
          */
     case 0x3d: /* Broadwell */
-        return ucode_rev >= 0x2a;
+        safe = ucode_rev >= 0x2a;      break;
     case 0x47: /* Broadwell H */
-        return ucode_rev >= 0x1d;
+        safe = ucode_rev >= 0x1d;      break;
     case 0x4f: /* Broadwell EP/EX */
-        return ucode_rev >= 0xb000021;
+        safe = ucode_rev >= 0xb000021; break;
     case 0x56: /* Broadwell D */
         switch ( boot_cpu_data.x86_mask )
         {
-        case 2:  return ucode_rev >= 0x15;
-        case 3:  return ucode_rev >= 0x7000012;
-        case 4:  return ucode_rev >= 0xf000011;
-        case 5:  return ucode_rev >= 0xe000009;
+        case 2:  safe = ucode_rev >= 0x15;      break;
+        case 3:  safe = ucode_rev >= 0x7000012; break;
+        case 4:  safe = ucode_rev >= 0xf000011; break;
+        case 5:  safe = ucode_rev >= 0xe000009; break;
         default:
             printk("Unrecognised CPU stepping %#x - assuming not reptpoline safe\n",
                    boot_cpu_data.x86_mask);
@@ -681,6 +682,12 @@ static bool __init retpoline_safe(void)
                boot_cpu_data.x86_model);
         return false;
     }
+
+    /* Only Broadwell gets here. */
+    if ( safe )
+        return true;
+
+    return false;
 }
 
 /*
@@ -1113,7 +1120,7 @@ void __init init_speculation_mitigations(void)
 {
     enum ind_thunk thunk = THUNK_DEFAULT;
     bool has_spec_ctrl, ibrs = false, hw_smt_enabled;
-    bool cpu_has_bug_taa;
+    bool cpu_has_bug_taa, retpoline_safe;
 
     hw_smt_enabled = check_smt_enabled();
 
@@ -1139,6 +1146,9 @@ void __init init_speculation_mitigations(void)
             thunk = THUNK_JMP;
     }
 
+    /* Determine if retpoline is safe on this CPU. */
+    retpoline_safe = retpoline_calculations();
+
     /*
      * Has the user specified any custom BTI mitigations?  If so, follow their
      * instructions exactly and disable all heuristics.
@@ -1160,7 +1170,7 @@ void __init init_speculation_mitigations(void)
              * On all hardware, we'd like to use retpoline in preference to
              * IBRS, but only if it is safe on this hardware.
              */
-            if ( retpoline_safe() )
+            if ( retpoline_safe )
                 thunk = THUNK_RETPOLINE;
             else if ( has_spec_ctrl )
                 ibrs = true;
-- 
2.30.2

[PATCH 2/4] x86/spec-ctrl: Synthesize missing RSBA/RRSBA bits

[Andrew Cooper]

---
 xen/arch/x86/include/asm/cpufeature.h |  1 +
 xen/arch/x86/spec_ctrl.c              | 50 +++++++++++++++++++++++----
 2 files changed, 44 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/include/asm/cpufeature.h b/xen/arch/x86/include/asm/cpufeature.h
index 50235f098d70..08e3eedd1280 100644
--- a/xen/arch/x86/include/asm/cpufeature.h
+++ b/xen/arch/x86/include/asm/cpufeature.h
@@ -192,6 +192,7 @@ static inline bool boot_cpu_has(unsigned int feat)
 #define cpu_has_tsx_ctrl        boot_cpu_has(X86_FEATURE_TSX_CTRL)
 #define cpu_has_taa_no          boot_cpu_has(X86_FEATURE_TAA_NO)
 #define cpu_has_fb_clear        boot_cpu_has(X86_FEATURE_FB_CLEAR)
+#define cpu_has_rrsba           boot_cpu_has(X86_FEATURE_RRSBA)
 
 /* Synthesized. */
 #define cpu_has_arch_perfmon    boot_cpu_has(X86_FEATURE_ARCH_PERFMON)
diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
index 0774d40627dd..daf77d77e139 100644
--- a/xen/arch/x86/spec_ctrl.c
+++ b/xen/arch/x86/spec_ctrl.c
@@ -578,7 +578,10 @@ static bool __init check_smt_enabled(void)
     return false;
 }
 
-/* Calculate whether Retpoline is known-safe on this CPU. */
+/*
+ * Calculate whether Retpoline is known-safe on this CPU.  Fixes up missing
+ * RSBA/RRSBA enumeration from older microcode versions.
+ */
 static bool __init retpoline_calculations(void)
 {
     unsigned int ucode_rev = this_cpu(cpu_sig).rev;
@@ -592,13 +595,18 @@ static bool __init retpoline_calculations(void)
         return false;
 
     /*
-     * RSBA may be set by a hypervisor to indicate that we may move to a
-     * processor which isn't retpoline-safe.
-     *
      * Processors offering Enhanced IBRS are not guarenteed to be
      * repoline-safe.
      */
-    if ( cpu_has_rsba || cpu_has_eibrs )
+    if ( cpu_has_eibrs )
+        goto unsafe_maybe_fixup_rrsba;
+
+    /*
+     * RSBA is explicitly enumerated in some cases, but may also be set by a
+     * hypervisor to indicate that we may move to a processor which isn't
+     * retpoline-safe.
+     */
+    if ( cpu_has_rsba )
         return false;
 
     switch ( boot_cpu_data.x86_model )
@@ -648,6 +656,8 @@ static bool __init retpoline_calculations(void)
 
         /*
          * Skylake, Kabylake and Cannonlake processors are not retpoline-safe.
+         * Note: the eIBRS-capable steppings are filtered out earlier, so the
+         * remainder here are the ones which suffer only RSBA behaviour.
          */
     case 0x4e: /* Skylake M */
     case 0x55: /* Skylake X */
@@ -656,7 +666,7 @@ static bool __init retpoline_calculations(void)
     case 0x67: /* Cannonlake? */
     case 0x8e: /* Kabylake M */
     case 0x9e: /* Kabylake D */
-        return false;
+        goto unsafe_maybe_fixup_rsba;
 
         /*
          * Atom processors before Goldmont Plus/Gemini Lake are retpoline-safe.
@@ -687,6 +697,32 @@ static bool __init retpoline_calculations(void)
     if ( safe )
         return true;
 
+    /*
+     * The meaning of the RSBA and RRSBA bits have evolved over time.  The
+     * agreed upon meaning at the time of writing (May 2023) is thus:
+     *
+     * - RSBA (RSB Alterantive) means that an RSB may fall back to an
+     *   alternative predictor on underflow.  Skylake uarch and later all have
+     *   this property.  Broadwell too, when running microcode versions prior
+     *   to Jan 2018.
+     *
+     * - All eIBRS-capable processors suffer RSBA, but eIBRS also introduces
+     *   tagging of predictions with the mode in which they were learned.  So
+     *   when eIBRS is active, RSBA becomes RRSBA (Restricted RSBA).
+     *
+     * Some parts (Broadwell) are not expected to ever enumerate this
+     * behaviour directly.  Other parts have differing enumeration with
+     * microcode version.  Fix up Xen's idea, so we can advertise them safely
+     * to guests, and so toolstacks can level a VM safelty for migration.
+     */
+ unsafe_maybe_fixup_rrsba:
+    if ( !cpu_has_rrsba )
+        setup_force_cpu_cap(X86_FEATURE_RRSBA);
+
+ unsafe_maybe_fixup_rsba:
+    if ( !cpu_has_rsba )
+        setup_force_cpu_cap(X86_FEATURE_RSBA);
+
     return false;
 }
 
@@ -1146,7 +1182,7 @@ void __init init_speculation_mitigations(void)
             thunk = THUNK_JMP;
     }
 
-    /* Determine if retpoline is safe on this CPU. */
+    /* Determine if retpoline is safe on this CPU.  Fix up RSBA/RRSBA enumerations. */
     retpoline_safe = retpoline_calculations();
 
     /*
-- 
2.30.2

[PATCH 2/4] x86/spec-ctrl: Synthesize RSBA/RRSBA bits with older microcode

[Andrew Cooper]

In order to level a VM safely for migration, the toolstack needs to know the
RSBA/RRSBA properties of the CPU, whether or not they happen to be enumerated.

Synthesize the bits when missing.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
---
 xen/arch/x86/include/asm/cpufeature.h |  1 +
 xen/arch/x86/spec_ctrl.c              | 50 +++++++++++++++++++++++----
 2 files changed, 44 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/include/asm/cpufeature.h b/xen/arch/x86/include/asm/cpufeature.h
index 50235f098d70..08e3eedd1280 100644
--- a/xen/arch/x86/include/asm/cpufeature.h
+++ b/xen/arch/x86/include/asm/cpufeature.h
@@ -192,6 +192,7 @@ static inline bool boot_cpu_has(unsigned int feat)
 #define cpu_has_tsx_ctrl        boot_cpu_has(X86_FEATURE_TSX_CTRL)
 #define cpu_has_taa_no          boot_cpu_has(X86_FEATURE_TAA_NO)
 #define cpu_has_fb_clear        boot_cpu_has(X86_FEATURE_FB_CLEAR)
+#define cpu_has_rrsba           boot_cpu_has(X86_FEATURE_RRSBA)
 
 /* Synthesized. */
 #define cpu_has_arch_perfmon    boot_cpu_has(X86_FEATURE_ARCH_PERFMON)
diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
index 0774d40627dd..2647784615cc 100644
--- a/xen/arch/x86/spec_ctrl.c
+++ b/xen/arch/x86/spec_ctrl.c
@@ -578,7 +578,10 @@ static bool __init check_smt_enabled(void)
     return false;
 }
 
-/* Calculate whether Retpoline is known-safe on this CPU. */
+/*
+ * Calculate whether Retpoline is known-safe on this CPU.  Synthesize missing
+ * RSBA/RRSBA bits when running with old microcode.
+ */
 static bool __init retpoline_calculations(void)
 {
     unsigned int ucode_rev = this_cpu(cpu_sig).rev;
@@ -592,13 +595,18 @@ static bool __init retpoline_calculations(void)
         return false;
 
     /*
-     * RSBA may be set by a hypervisor to indicate that we may move to a
-     * processor which isn't retpoline-safe.
-     *
      * Processors offering Enhanced IBRS are not guarenteed to be
      * repoline-safe.
      */
-    if ( cpu_has_rsba || cpu_has_eibrs )
+    if ( cpu_has_eibrs )
+        goto unsafe_maybe_fixup_rrsba;
+
+    /*
+     * RSBA is explicitly enumerated in some cases, but may also be set by a
+     * hypervisor to indicate that we may move to a processor which isn't
+     * retpoline-safe.
+     */
+    if ( cpu_has_rsba )
         return false;
 
     switch ( boot_cpu_data.x86_model )
@@ -648,6 +656,8 @@ static bool __init retpoline_calculations(void)
 
         /*
          * Skylake, Kabylake and Cannonlake processors are not retpoline-safe.
+         * Note: the eIBRS-capable steppings are filtered out earlier, so the
+         * remainder here are the ones which suffer only RSBA behaviour.
          */
     case 0x4e: /* Skylake M */
     case 0x55: /* Skylake X */
@@ -656,7 +666,7 @@ static bool __init retpoline_calculations(void)
     case 0x67: /* Cannonlake? */
     case 0x8e: /* Kabylake M */
     case 0x9e: /* Kabylake D */
-        return false;
+        goto unsafe_maybe_fixup_rsba;
 
         /*
          * Atom processors before Goldmont Plus/Gemini Lake are retpoline-safe.
@@ -687,6 +697,32 @@ static bool __init retpoline_calculations(void)
     if ( safe )
         return true;
 
+    /*
+     * The meaning of the RSBA and RRSBA bits have evolved over time.  The
+     * agreed upon meaning at the time of writing (May 2023) is thus:
+     *
+     * - RSBA (RSB Alterantive) means that an RSB may fall back to an
+     *   alternative predictor on underflow.  Skylake uarch and later all have
+     *   this property.  Broadwell too, when running microcode versions prior
+     *   to Jan 2018.
+     *
+     * - All eIBRS-capable processors suffer RSBA, but eIBRS also introduces
+     *   tagging of predictions with the mode in which they were learned.  So
+     *   when eIBRS is active, RSBA becomes RRSBA (Restricted RSBA).
+     *
+     * Some parts (Broadwell) are not expected to ever enumerate this
+     * behaviour directly.  Other parts have differing enumeration with
+     * microcode version.  Fix up Xen's idea, so we can advertise them safely
+     * to guests, and so toolstacks can level a VM safelty for migration.
+     */
+ unsafe_maybe_fixup_rrsba:
+    if ( !cpu_has_rrsba )
+        setup_force_cpu_cap(X86_FEATURE_RRSBA);
+
+ unsafe_maybe_fixup_rsba:
+    if ( !cpu_has_rsba )
+        setup_force_cpu_cap(X86_FEATURE_RSBA);
+
     return false;
 }
 
@@ -1146,7 +1182,7 @@ void __init init_speculation_mitigations(void)
             thunk = THUNK_JMP;
     }
 
-    /* Determine if retpoline is safe on this CPU. */
+    /* Determine if retpoline is safe on this CPU.  Fix up RSBA/RRSBA enumerations. */
     retpoline_safe = retpoline_calculations();
 
     /*
-- 
2.30.2

[PATCH 3/4] x86/cpu-policy: Rearrange guest_common_default_feature_adjustments()

[Andrew Cooper]

This is prep work, split out to simply the diff on the following change.

 * Split the INTEL check out of the IvyBridge RDRAND check, as the former will
   be reused.
 * Use asm/intel-family.h to remove a raw 0x3a model number.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
---
 xen/arch/x86/cpu-policy.c | 34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

diff --git a/xen/arch/x86/cpu-policy.c b/xen/arch/x86/cpu-policy.c
index 74266d30b551..bdbc5660acd4 100644
--- a/xen/arch/x86/cpu-policy.c
+++ b/xen/arch/x86/cpu-policy.c
@@ -10,6 +10,7 @@
 #include <asm/cpu-policy.h>
 #include <asm/hvm/nestedhvm.h>
 #include <asm/hvm/svm/svm.h>
+#include <asm/intel-family.h>
 #include <asm/msr-index.h>
 #include <asm/paging.h>
 #include <asm/setup.h>
@@ -429,21 +430,24 @@ static void __init guest_common_max_feature_adjustments(uint32_t *fs)
 
 static void __init guest_common_default_feature_adjustments(uint32_t *fs)
 {
-    /*
-     * IvyBridge client parts suffer from leakage of RDRAND data due to SRBDS
-     * (XSA-320 / CVE-2020-0543), and won't be receiving microcode to
-     * compensate.
-     *
-     * Mitigate by hiding RDRAND from guests by default, unless explicitly
-     * overridden on the Xen command line (cpuid=rdrand).  Irrespective of the
-     * default setting, guests can use RDRAND if explicitly enabled
-     * (cpuid="host,rdrand=1") in the VM's config file, and VMs which were
-     * previously using RDRAND can migrate in.
-     */
-    if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL &&
-         boot_cpu_data.x86 == 6 && boot_cpu_data.x86_model == 0x3a &&
-         cpu_has_rdrand && !is_forced_cpu_cap(X86_FEATURE_RDRAND) )
-        __clear_bit(X86_FEATURE_RDRAND, fs);
+    if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL )
+    {
+        /*
+         * IvyBridge client parts suffer from leakage of RDRAND data due to SRBDS
+         * (XSA-320 / CVE-2020-0543), and won't be receiving microcode to
+         * compensate.
+         *
+         * Mitigate by hiding RDRAND from guests by default, unless explicitly
+         * overridden on the Xen command line (cpuid=rdrand).  Irrespective of the
+         * default setting, guests can use RDRAND if explicitly enabled
+         * (cpuid="host,rdrand=1") in the VM's config file, and VMs which were
+         * previously using RDRAND can migrate in.
+         */
+        if ( boot_cpu_data.x86 == 6 &&
+             boot_cpu_data.x86_model == INTEL_FAM6_IVYBRIDGE &&
+             cpu_has_rdrand && !is_forced_cpu_cap(X86_FEATURE_RDRAND) )
+            __clear_bit(X86_FEATURE_RDRAND, fs);
+    }
 
     /*
      * On certain hardware, speculative or errata workarounds can result in
-- 
2.30.2

[PATCH 4/4] x86/cpu-policy: Derive {,R}RSBA for guest policies

[Andrew Cooper]

The RSBA bit, "RSB Alternative", means that the RSB may use alternative
predictors when empty.  From a practical point of view, this mean "Retpoline
not safe".

Enhanced IBRS (officially IBRS_ALL in Intel's docs, previously IBRS_ATT) is a
statement that IBRS is implemented in hardware (as opposed to the form
retrofitted to existing CPUs in microcode).

The RRSBA bit, "Restricted-RSBA", is a combination of RSBA, and the eIBRS
property that predictions are tagged with the mode in which they were learnt.
Therefore, it means "when eIBRS is active, the RSB may fall back to
alternative predictors but restricted to the current prediction mode".  As
such, it's stronger statement than RSBA, but still means "Retpoline not safe".

Add feature dependencies for EIBRS and RRSBA.  While technically they're not
linked, absolutely nothing good can of letting the guest see RRSBA without
EIBRS.  Furthermore, we use this dependency to simplify the max/default
derivation logic.

The max policies gets RSBA and RRSBA unconditionally set (with the EIBRS
dependency maybe hiding RRSBA).  We can run any VM, even if it has been told
"somewhere else, Retpoline isn't safe".

The default policies inherit the host settings, because the guest wants to run
with as few (anti)features as it can safely get away with.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
---
 xen/arch/x86/cpu-policy.c | 25 +++++++++++++++++++++++++
 xen/tools/gen-cpuid.py    |  5 ++++-
 2 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/cpu-policy.c b/xen/arch/x86/cpu-policy.c
index bdbc5660acd4..eb1ecb75f593 100644
--- a/xen/arch/x86/cpu-policy.c
+++ b/xen/arch/x86/cpu-policy.c
@@ -423,8 +423,14 @@ static void __init guest_common_max_feature_adjustments(uint32_t *fs)
          * Retpoline not safe)", so these need to be visible to a guest in all
          * cases, even when it's only some other server in the pool which
          * suffers the identified behaviour.
+         *
+         * We can always run any VM which has previously (or will
+         * subsequently) run on hardware where Retpoline is not safe.  Note:
+         * The dependency logic may hide RRSBA for other reasons.
          */
         __set_bit(X86_FEATURE_ARCH_CAPS, fs);
+        __set_bit(X86_FEATURE_RSBA, fs);
+        __set_bit(X86_FEATURE_RRSBA, fs);
     }
 }
 
@@ -432,6 +438,25 @@ static void __init guest_common_default_feature_adjustments(uint32_t *fs)
 {
     if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL )
     {
+        /*
+         * The {,R}RSBA bits under virt mean "you might migrate somewhere
+         * where retpoline is not safe".  In particular, a VM's settings may
+         * not be applicable to the current host.
+         *
+         * Discard the settings inherited from the max policy, and and feed in
+         * the host values.  The ideal case for a VM is for neither of these
+         * bits to be set, but toolstacks must accumuate them across anywhere
+         * the VM might migrate to, in case any possible destination happens
+         * to be unsafe.
+         *
+         * Note: The dependency logic might hide RRSBA for other reasons.
+         */
+        fs[FEATURESET_m10Al] &= ~(cpufeat_mask(X86_FEATURE_RSBA) |
+                                  cpufeat_mask(X86_FEATURE_RRSBA));
+        fs[FEATURESET_m10Al] |=
+            host_cpu_policy.arch_caps.lo & (cpufeat_mask(X86_FEATURE_RSBA) |
+                                            cpufeat_mask(X86_FEATURE_RRSBA));
+
         /*
          * IvyBridge client parts suffer from leakage of RDRAND data due to SRBDS
          * (XSA-320 / CVE-2020-0543), and won't be receiving microcode to
diff --git a/xen/tools/gen-cpuid.py b/xen/tools/gen-cpuid.py
index f28ff708a2fc..22294a26adc0 100755
--- a/xen/tools/gen-cpuid.py
+++ b/xen/tools/gen-cpuid.py
@@ -318,7 +318,7 @@ def crunch_numbers(state):
         # IBRSB/IBRS, and we pass this MSR directly to guests.  Treating them
         # as dependent features simplifies Xen's logic, and prevents the guest
         # from seeing implausible configurations.
-        IBRSB: [STIBP, SSBD, INTEL_PSFD],
+        IBRSB: [STIBP, SSBD, INTEL_PSFD, EIBRS],
         IBRS: [AMD_STIBP, AMD_SSBD, PSFD,
                IBRS_ALWAYS, IBRS_FAST, IBRS_SAME_MODE],
         AMD_STIBP: [STIBP_ALWAYS],
@@ -328,6 +328,9 @@ def crunch_numbers(state):
 
         # The ARCH_CAPS CPUID bit enumerates the availability of the whole register.
         ARCH_CAPS: list(range(RDCL_NO, RDCL_NO + 64)),
+
+        # The behaviour described by RRSBA depend on eIBRS being active.
+        EIBRS: [RRSBA],
     }
 
     deep_features = tuple(sorted(deps.keys()))
-- 
2.30.2

Re: [PATCH 2/4] x86/spec-ctrl: Synthesize missing RSBA/RRSBA bits

[Andrew Cooper]

On 26/05/2023 12:06 pm, Andrew Cooper wrote:
> ---
>  xen/arch/x86/include/asm/cpufeature.h |  1 +
>  xen/arch/x86/spec_ctrl.c              | 50 +++++++++++++++++++++++----
>  2 files changed, 44 insertions(+), 7 deletions(-)

Sorry, please ignore this patch and look at the other patch 2, which has
a commit message.

~Andrew

Re: [PATCH 1/4] x86/spec-ctrl: Rename retpoline_safe() to retpoline_calculations()

[Jan Beulich]

On 26.05.2023 13:06, Andrew Cooper wrote:
> This is prep work, split out to simply the diff on the following change.
> 
>  * Rename to retpoline_calculations(), and call unconditionally.  It is
>    shortly going to synthesize missing enumerations required for guest safety.
>  * For Broadwell, store the ucode revision calculation in a variable and fall
>    out of the bottom of the switch statement.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

I guess subsequent patches will teach me why ...

> @@ -681,6 +682,12 @@ static bool __init retpoline_safe(void)
>                 boot_cpu_data.x86_model);
>          return false;
>      }
> +
> +    /* Only Broadwell gets here. */
> +    if ( safe )
> +        return true;
> +
> +    return false;

... this isn't just "return safe;".

Jan

Re: [PATCH 1/4] x86/spec-ctrl: Rename retpoline_safe() to retpoline_calculations()

[Andrew Cooper]

On 30/05/2023 10:07 am, Jan Beulich wrote:
> On 26.05.2023 13:06, Andrew Cooper wrote:
>> This is prep work, split out to simply the diff on the following change.
>>
>>  * Rename to retpoline_calculations(), and call unconditionally.  It is
>>    shortly going to synthesize missing enumerations required for guest safety.
>>  * For Broadwell, store the ucode revision calculation in a variable and fall
>>    out of the bottom of the switch statement.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

>
> I guess subsequent patches will teach me why ...
>
>> @@ -681,6 +682,12 @@ static bool __init retpoline_safe(void)
>>                 boot_cpu_data.x86_model);
>>          return false;
>>      }
>> +
>> +    /* Only Broadwell gets here. */
>> +    if ( safe )
>> +        return true;
>> +
>> +    return false;
> ... this isn't just "return safe;".

Indeed they will.

~Andrew

Re: [PATCH 2/4] x86/spec-ctrl: Synthesize RSBA/RRSBA bits with older microcode

[Jan Beulich]

On 26.05.2023 13:06, Andrew Cooper wrote:
> @@ -687,6 +697,32 @@ static bool __init retpoline_calculations(void)
>      if ( safe )
>          return true;
>  
> +    /*
> +     * The meaning of the RSBA and RRSBA bits have evolved over time.  The
> +     * agreed upon meaning at the time of writing (May 2023) is thus:
> +     *
> +     * - RSBA (RSB Alterantive) means that an RSB may fall back to an
> +     *   alternative predictor on underflow.  Skylake uarch and later all have
> +     *   this property.  Broadwell too, when running microcode versions prior
> +     *   to Jan 2018.
> +     *
> +     * - All eIBRS-capable processors suffer RSBA, but eIBRS also introduces
> +     *   tagging of predictions with the mode in which they were learned.  So
> +     *   when eIBRS is active, RSBA becomes RRSBA (Restricted RSBA).
> +     *
> +     * Some parts (Broadwell) are not expected to ever enumerate this
> +     * behaviour directly.  Other parts have differing enumeration with
> +     * microcode version.  Fix up Xen's idea, so we can advertise them safely
> +     * to guests, and so toolstacks can level a VM safelty for migration.
> +     */

If the difference between the two is whether eIBRS is active (as you did
word it yet more explicitly in e.g. [1]), then ...

> + unsafe_maybe_fixup_rrsba:
> +    if ( !cpu_has_rrsba )
> +        setup_force_cpu_cap(X86_FEATURE_RRSBA);
> +
> + unsafe_maybe_fixup_rsba:
> +    if ( !cpu_has_rsba )
> +        setup_force_cpu_cap(X86_FEATURE_RSBA);
> +
>      return false;
>  }

... can both actually be active at the same time? IOW is there a "return
false" missing ahead of the 2nd label?

Not having looked at further patches yet it also strikes me as odd that
each of the two labels is used exactly once only. Leaving the shared
comment aside, imo this would then better avoid "goto".

Finally, what use are the two if()s? There's nothing wrong with forcing
a feature which is already available.

Jan

[1] https://lore.kernel.org/lkml/f43c3c33-f8b9-e764-709d-b3864d2bd9f8@citrix.com/

Re: [PATCH 3/4] x86/cpu-policy: Rearrange guest_common_default_feature_adjustments()

[Jan Beulich]

On 26.05.2023 13:06, Andrew Cooper wrote:
> This is prep work, split out to simply the diff on the following change.
> 
>  * Split the INTEL check out of the IvyBridge RDRAND check, as the former will
>    be reused.
>  * Use asm/intel-family.h to remove a raw 0x3a model number.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH 4/4] x86/cpu-policy: Derive {,R}RSBA for guest policies

[Jan Beulich]

On 26.05.2023 13:06, Andrew Cooper wrote:
> The RSBA bit, "RSB Alternative", means that the RSB may use alternative
> predictors when empty.  From a practical point of view, this mean "Retpoline
> not safe".
> 
> Enhanced IBRS (officially IBRS_ALL in Intel's docs, previously IBRS_ATT) is a
> statement that IBRS is implemented in hardware (as opposed to the form
> retrofitted to existing CPUs in microcode).
> 
> The RRSBA bit, "Restricted-RSBA", is a combination of RSBA, and the eIBRS
> property that predictions are tagged with the mode in which they were learnt.
> Therefore, it means "when eIBRS is active, the RSB may fall back to
> alternative predictors but restricted to the current prediction mode".  As
> such, it's stronger statement than RSBA, but still means "Retpoline not safe".

Just for my own understanding: Whether retpoline is safe with RRSBA does
depend on the level of control a less privileged entity has over a more
privileged entity's alternative predictor state? If so, maybe add
"probably" to the quoted statement?

> --- a/xen/arch/x86/cpu-policy.c
> +++ b/xen/arch/x86/cpu-policy.c
> @@ -423,8 +423,14 @@ static void __init guest_common_max_feature_adjustments(uint32_t *fs)
>           * Retpoline not safe)", so these need to be visible to a guest in all
>           * cases, even when it's only some other server in the pool which
>           * suffers the identified behaviour.
> +         *
> +         * We can always run any VM which has previously (or will
> +         * subsequently) run on hardware where Retpoline is not safe.  Note:
> +         * The dependency logic may hide RRSBA for other reasons.
>           */
>          __set_bit(X86_FEATURE_ARCH_CAPS, fs);
> +        __set_bit(X86_FEATURE_RSBA, fs);
> +        __set_bit(X86_FEATURE_RRSBA, fs);
>      }
>  }

Similar question to what I raised before: Can't this lead to both bits being
set, which according to descriptions earlier in the series and elsewhere
ought to not be possible?

> --- a/xen/tools/gen-cpuid.py
> +++ b/xen/tools/gen-cpuid.py
> @@ -318,7 +318,7 @@ def crunch_numbers(state):
>          # IBRSB/IBRS, and we pass this MSR directly to guests.  Treating them
>          # as dependent features simplifies Xen's logic, and prevents the guest
>          # from seeing implausible configurations.
> -        IBRSB: [STIBP, SSBD, INTEL_PSFD],
> +        IBRSB: [STIBP, SSBD, INTEL_PSFD, EIBRS],

Is this really an architecturally established dependency? From an abstract
pov having just eIBRS ought to be enough of an indicator? And hence it would
be wrong to hide it just because IBRSB isn't also set. Plus aiui ...

> @@ -328,6 +328,9 @@ def crunch_numbers(state):
>  
>          # The ARCH_CAPS CPUID bit enumerates the availability of the whole register.
>          ARCH_CAPS: list(range(RDCL_NO, RDCL_NO + 64)),
> +
> +        # The behaviour described by RRSBA depend on eIBRS being active.
> +        EIBRS: [RRSBA],

... for the purpose of the change here this dependency is all you need.

Jan

Re: [PATCH 2/4] x86/spec-ctrl: Synthesize RSBA/RRSBA bits with older microcode

[Andrew Cooper]

On 30/05/2023 10:18 am, Jan Beulich wrote:
> On 26.05.2023 13:06, Andrew Cooper wrote:
>> @@ -687,6 +697,32 @@ static bool __init retpoline_calculations(void)
>>      if ( safe )
>>          return true;
>>  
>> +    /*
>> +     * The meaning of the RSBA and RRSBA bits have evolved over time.  The
>> +     * agreed upon meaning at the time of writing (May 2023) is thus:
>> +     *
>> +     * - RSBA (RSB Alterantive) means that an RSB may fall back to an
>> +     *   alternative predictor on underflow.  Skylake uarch and later all have
>> +     *   this property.  Broadwell too, when running microcode versions prior
>> +     *   to Jan 2018.
>> +     *
>> +     * - All eIBRS-capable processors suffer RSBA, but eIBRS also introduces
>> +     *   tagging of predictions with the mode in which they were learned.  So
>> +     *   when eIBRS is active, RSBA becomes RRSBA (Restricted RSBA).
>> +     *
>> +     * Some parts (Broadwell) are not expected to ever enumerate this
>> +     * behaviour directly.  Other parts have differing enumeration with
>> +     * microcode version.  Fix up Xen's idea, so we can advertise them safely
>> +     * to guests, and so toolstacks can level a VM safelty for migration.
>> +     */
> If the difference between the two is whether eIBRS is active (as you did
> word it yet more explicitly in e.g. [1]), then ...
>
>> + unsafe_maybe_fixup_rrsba:
>> +    if ( !cpu_has_rrsba )
>> +        setup_force_cpu_cap(X86_FEATURE_RRSBA);
>> +
>> + unsafe_maybe_fixup_rsba:
>> +    if ( !cpu_has_rsba )
>> +        setup_force_cpu_cap(X86_FEATURE_RSBA);
>> +
>>      return false;
>>  }
> ... can both actually be active at the same time? IOW is there a "return
> false" missing ahead of the 2nd label?

I've already got a question out to Intel to this effect.  (I didn't say
the enumeration made much sense...)

> Not having looked at further patches yet it also strikes me as odd that
> each of the two labels is used exactly once only. Leaving the shared
> comment aside, imo this would then better avoid "goto".

They're both used twice, not once.  You asked why it wasn't "return
safe;" in the previous patch?  Well this is why.

> Finally, what use are the two if()s? There's nothing wrong with forcing
> a feature which is already available.

It breaks is_forced_cpu_cap().

Also, I considered having a printk() here.  I've still got it around in
a debug patch, but I decided against it.

~Andrew

Re: [PATCH 2/4] x86/spec-ctrl: Synthesize RSBA/RRSBA bits with older microcode

[Jan Beulich]

On 30.05.2023 12:00, Andrew Cooper wrote:
> On 30/05/2023 10:18 am, Jan Beulich wrote:
>> On 26.05.2023 13:06, Andrew Cooper wrote:
>>> @@ -687,6 +697,32 @@ static bool __init retpoline_calculations(void)
>>>      if ( safe )
>>>          return true;
>>>  
>>> +    /*
>>> +     * The meaning of the RSBA and RRSBA bits have evolved over time.  The
>>> +     * agreed upon meaning at the time of writing (May 2023) is thus:
>>> +     *
>>> +     * - RSBA (RSB Alterantive) means that an RSB may fall back to an
>>> +     *   alternative predictor on underflow.  Skylake uarch and later all have
>>> +     *   this property.  Broadwell too, when running microcode versions prior
>>> +     *   to Jan 2018.
>>> +     *
>>> +     * - All eIBRS-capable processors suffer RSBA, but eIBRS also introduces
>>> +     *   tagging of predictions with the mode in which they were learned.  So
>>> +     *   when eIBRS is active, RSBA becomes RRSBA (Restricted RSBA).
>>> +     *
>>> +     * Some parts (Broadwell) are not expected to ever enumerate this
>>> +     * behaviour directly.  Other parts have differing enumeration with
>>> +     * microcode version.  Fix up Xen's idea, so we can advertise them safely
>>> +     * to guests, and so toolstacks can level a VM safelty for migration.
>>> +     */
>> If the difference between the two is whether eIBRS is active (as you did
>> word it yet more explicitly in e.g. [1]), then ...
>>
>>> + unsafe_maybe_fixup_rrsba:
>>> +    if ( !cpu_has_rrsba )
>>> +        setup_force_cpu_cap(X86_FEATURE_RRSBA);
>>> +
>>> + unsafe_maybe_fixup_rsba:
>>> +    if ( !cpu_has_rsba )
>>> +        setup_force_cpu_cap(X86_FEATURE_RSBA);
>>> +
>>>      return false;
>>>  }
>> ... can both actually be active at the same time? IOW is there a "return
>> false" missing ahead of the 2nd label?
> 
> I've already got a question out to Intel to this effect.  (I didn't say
> the enumeration made much sense...)
> 
>> Not having looked at further patches yet it also strikes me as odd that
>> each of the two labels is used exactly once only. Leaving the shared
>> comment aside, imo this would then better avoid "goto".
> 
> They're both used twice, not once.  You asked why it wasn't "return
> safe;" in the previous patch?  Well this is why.

Ouch, yes. The labels themselves are used just once, but there's
important fall-through from above here.

>> Finally, what use are the two if()s? There's nothing wrong with forcing
>> a feature which is already available.
> 
> It breaks is_forced_cpu_cap().

Hmm, yes, but is that important here? (If you decide to keep the if()s,
which I'm not opposed to, would you mind adding half a sentence to the
description or maybe a brief code comment?)

Jan

> Also, I considered having a printk() here.  I've still got it around in
> a debug patch, but I decided against it.
> 
> ~Andrew

Re: [PATCH 4/4] x86/cpu-policy: Derive {,R}RSBA for guest policies

[Andrew Cooper]

On 30/05/2023 10:40 am, Jan Beulich wrote:
> On 26.05.2023 13:06, Andrew Cooper wrote:
>> The RSBA bit, "RSB Alternative", means that the RSB may use alternative
>> predictors when empty.  From a practical point of view, this mean "Retpoline
>> not safe".
>>
>> Enhanced IBRS (officially IBRS_ALL in Intel's docs, previously IBRS_ATT) is a
>> statement that IBRS is implemented in hardware (as opposed to the form
>> retrofitted to existing CPUs in microcode).
>>
>> The RRSBA bit, "Restricted-RSBA", is a combination of RSBA, and the eIBRS
>> property that predictions are tagged with the mode in which they were learnt.
>> Therefore, it means "when eIBRS is active, the RSB may fall back to
>> alternative predictors but restricted to the current prediction mode".  As
>> such, it's stronger statement than RSBA, but still means "Retpoline not safe".
> Just for my own understanding: Whether retpoline is safe with RRSBA does
> depend on the level of control a less privileged entity has over a more
> privileged entity's alternative predictor state?

Correct, but...

> If so, maybe add "probably" to the quoted statement?

... Spectre-BHI proved it was exploitable and could leak data.

"Don't do JIT in the kernel" was a very unsatisfactory resolution, and
in particular I think there is room to replicate the exploit with array
style sys/hypercalls.

As far as I'm concerned it's a matter of when, not if, a researcher
breaks this boundary again.

Concern in this area is why Intel added
MSR_SPEC_CTRL.{RRSBA,BHI}_DIS_{U,S} controls in ADL/SPR, which hobbles
this behaviour.  (And yes, we need to support these in guests too, but
that involves rearranging Xen's MSR_SPEC_CTRL handling which is why I
haven't gotten around to it yet.)

>> --- a/xen/arch/x86/cpu-policy.c
>> +++ b/xen/arch/x86/cpu-policy.c
>> @@ -423,8 +423,14 @@ static void __init guest_common_max_feature_adjustments(uint32_t *fs)
>>           * Retpoline not safe)", so these need to be visible to a guest in all
>>           * cases, even when it's only some other server in the pool which
>>           * suffers the identified behaviour.
>> +         *
>> +         * We can always run any VM which has previously (or will
>> +         * subsequently) run on hardware where Retpoline is not safe.  Note:
>> +         * The dependency logic may hide RRSBA for other reasons.
>>           */
>>          __set_bit(X86_FEATURE_ARCH_CAPS, fs);
>> +        __set_bit(X86_FEATURE_RSBA, fs);
>> +        __set_bit(X86_FEATURE_RRSBA, fs);
>>      }
>>  }
> Similar question to what I raised before: Can't this lead to both bits being
> set, which according to descriptions earlier in the series and elsewhere
> ought to not be possible?

In this case, no, because the max values are fully discarded when
establishing the default policy.

Remember this value is used to decide whether an incoming VM can be
accepted.  It doesn't reflect a sensible configuration to run.

Whether or not both values ought to be visible is the subject of the
outstanding question.

>
>> --- a/xen/tools/gen-cpuid.py
>> +++ b/xen/tools/gen-cpuid.py
>> @@ -318,7 +318,7 @@ def crunch_numbers(state):
>>          # IBRSB/IBRS, and we pass this MSR directly to guests.  Treating them
>>          # as dependent features simplifies Xen's logic, and prevents the guest
>>          # from seeing implausible configurations.
>> -        IBRSB: [STIBP, SSBD, INTEL_PSFD],
>> +        IBRSB: [STIBP, SSBD, INTEL_PSFD, EIBRS],
> Is this really an architecturally established dependency? From an abstract
> pov having just eIBRS ought to be enough of an indicator?

This is the same as asking "can we hide AVX512F but expose AVX512_*"...

> And hence it would
> be wrong to hide it just because IBRSB isn't also set.

EIBRS means "you should set MSR_SPEC_CTRL.IBRS once at the start of day
and leave it set", which to me firmly states a dependency.


>  Plus aiui ...
>
>> @@ -328,6 +328,9 @@ def crunch_numbers(state):
>>  
>>          # The ARCH_CAPS CPUID bit enumerates the availability of the whole register.
>>          ARCH_CAPS: list(range(RDCL_NO, RDCL_NO + 64)),
>> +
>> +        # The behaviour described by RRSBA depend on eIBRS being active.
>> +        EIBRS: [RRSBA],
> ... for the purpose of the change here this dependency is all you need.

This change is to make the values sane for a guest, which includes "you
don't get RRSBA, or EIBRS if you have to level IBRS out".

~Andrew

Re: [PATCH 4/4] x86/cpu-policy: Derive {,R}RSBA for guest policies

[Jan Beulich]

On 30.05.2023 15:25, Andrew Cooper wrote:
> On 30/05/2023 10:40 am, Jan Beulich wrote:
>> On 26.05.2023 13:06, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/cpu-policy.c
>>> +++ b/xen/arch/x86/cpu-policy.c
>>> @@ -423,8 +423,14 @@ static void __init guest_common_max_feature_adjustments(uint32_t *fs)
>>>           * Retpoline not safe)", so these need to be visible to a guest in all
>>>           * cases, even when it's only some other server in the pool which
>>>           * suffers the identified behaviour.
>>> +         *
>>> +         * We can always run any VM which has previously (or will
>>> +         * subsequently) run on hardware where Retpoline is not safe.  Note:
>>> +         * The dependency logic may hide RRSBA for other reasons.
>>>           */
>>>          __set_bit(X86_FEATURE_ARCH_CAPS, fs);
>>> +        __set_bit(X86_FEATURE_RSBA, fs);
>>> +        __set_bit(X86_FEATURE_RRSBA, fs);
>>>      }
>>>  }
>> Similar question to what I raised before: Can't this lead to both bits being
>> set, which according to descriptions earlier in the series and elsewhere
>> ought to not be possible?
> 
> In this case, no, because the max values are fully discarded when
> establishing the default policy.
> 
> Remember this value is used to decide whether an incoming VM can be
> accepted.  It doesn't reflect a sensible configuration to run.

Right. I should have dropped the question when seeing the "default"
counterpart's behavior.

> Whether or not both values ought to be visible is the subject of the
> outstanding question.

Pending the answer there (and whichever easy adjustment)
Reviewed-by: Jan Beulich <jbeulich@suse.com>

>>> --- a/xen/tools/gen-cpuid.py
>>> +++ b/xen/tools/gen-cpuid.py
>>> @@ -318,7 +318,7 @@ def crunch_numbers(state):
>>>          # IBRSB/IBRS, and we pass this MSR directly to guests.  Treating them
>>>          # as dependent features simplifies Xen's logic, and prevents the guest
>>>          # from seeing implausible configurations.
>>> -        IBRSB: [STIBP, SSBD, INTEL_PSFD],
>>> +        IBRSB: [STIBP, SSBD, INTEL_PSFD, EIBRS],
>> Is this really an architecturally established dependency? From an abstract
>> pov having just eIBRS ought to be enough of an indicator?
> 
> This is the same as asking "can we hide AVX512F but expose AVX512_*"...
> 
>> And hence it would
>> be wrong to hide it just because IBRSB isn't also set.
> 
> EIBRS means "you should set MSR_SPEC_CTRL.IBRS once at the start of day
> and leave it set", which to me firmly states a dependency.

Hmm, yes, now that you put it that way, I agree.

Jan