diff mbox series

x86/spec-ctrl: Knobs for STIBP and PSFD, and follow hardware STIBP hint

Message ID 20220316140035.32057-1-andrew.cooper3@citrix.com (mailing list archive)
State New, archived
Headers show
Series x86/spec-ctrl: Knobs for STIBP and PSFD, and follow hardware STIBP hint | expand

Commit Message

Andrew Cooper March 16, 2022, 2 p.m. UTC
STIBP and PSFD are slightly weird bits, because they're both implied by other
bits in MSR_SPEC_CTRL.  Add fine grain controls for them, and take the
implications into account when setting IBRS/SSBD.

Rearrange the IBPB text/variables/logic to keep all the MSR_SPEC_CTRL bits
together, for consistency.

However, AMD have a hardware hint CPUID bit recommending that STIBP be set
uniaterally.  This is advertised on Zen3, so follow the recommendation.  This
is the only default 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>
---
 docs/misc/xen-command-line.pandoc | 20 +++++++++----
 xen/arch/x86/spec_ctrl.c          | 59 ++++++++++++++++++++++++++++++++++++---
 2 files changed, 70 insertions(+), 9 deletions(-)

Comments

Jan Beulich March 17, 2022, 9:58 a.m. UTC | #1
On 16.03.2022 15:00, Andrew Cooper wrote:
> STIBP and PSFD are slightly weird bits, because they're both implied by other
> bits in MSR_SPEC_CTRL.  Add fine grain controls for them, and take the
> implications into account when setting IBRS/SSBD.
> 
> Rearrange the IBPB text/variables/logic to keep all the MSR_SPEC_CTRL bits
> together, for consistency.
> 
> However, AMD have a hardware hint CPUID bit recommending that STIBP be set
> uniaterally.  This is advertised on Zen3, so follow the recommendation.  This
> is the only default change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

In principle
Reviewed-by: Jan Beulich <jbeulich@suse.com>
but I have two comments:

> @@ -170,12 +174,18 @@ static int __init cf_check parse_spec_ctrl(const char *s)
>              else
>                  rc = -EINVAL;
>          }
> +
>          else if ( (val = parse_boolean("ibrs", s, ss)) >= 0 )
>              opt_ibrs = val;
> -        else if ( (val = parse_boolean("ibpb", s, ss)) >= 0 )
> -            opt_ibpb = 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;
> +
> +        else if ( (val = parse_boolean("ibpb", s, ss)) >= 0 )
> +            opt_ibpb = val;
>          else if ( (val = parse_boolean("eager-fpu", s, ss)) >= 0 )
>              opt_eager_fpu = val;
>          else if ( (val = parse_boolean("l1d-flush", s, ss)) >= 0 )

Personally I find blank lines ahead of "else if" misleading. Could I
talk you into moving ibrs+stibp and ssbd+psfd close to the end of this
(immediately ahead of "else"), prefixing each pair with a comment about
one feature implying the other (and hence the comments replacing the
blank lines)?

Otoh I notice that we already have blank lines elsewhere in the middle
if this block of code, but at least there they're accompanied by a
comment making more obvious why there is such separation. Which means
as an intermediate approach I'd be okay with no re-ordering, but with
comments added.

> @@ -1070,12 +1083,50 @@ void __init init_speculation_mitigations(void)
>  
>      /* If we have IBRS available, see whether we should use it. */
>      if ( has_spec_ctrl && ibrs )
> +    {
> +        /* IBRS implies STIBP.  */
> +        if ( opt_stibp == -1 )
> +            opt_stibp = 1;
> +
>          default_xen_spec_ctrl |= SPEC_CTRL_IBRS;
> +    }
> +
> +    /* Use STIBP by default if the hardware hint is set. */
> +    if ( opt_stibp == -1 && boot_cpu_has(X86_FEATURE_STIBP_ALWAYS) )
> +        opt_stibp = 1;
> +
> +    /*
> +     * Otherwise, don't use STIBP by default.  It has some severe performance
> +     * implications on older hardware.
> +     */
> +    if ( opt_stibp == -1 )
> +        opt_stibp = 0;

I'd find this easier to read if written along the lines of

    if ( opt_stibp == -1 )
    {
        /*
         * Use STIBP by default if the hardware hint is set.  Otherwise,
         * don't use STIBP by default.  It has some severe performance
         * implications on older hardware.
         */
        opt_stibp = !!boot_cpu_has(X86_FEATURE_STIBP_ALWAYS);
    }

FTAOD I'm not going to insist on either adjustment.

Jan
diff mbox series

Patch

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index 1dc7e1ca0706..6fa1fe97aeeb 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -2254,8 +2254,8 @@  By default SSBD will be mitigated at runtime (i.e `ssbd=runtime`).
 
 ### spec-ctrl (x86)
 > `= List of [ <bool>, xen=<bool>, {pv,hvm,msr-sc,rsb,md-clear}=<bool>,
->              bti-thunk=retpoline|lfence|jmp, {ibrs,ibpb,ssbd,eager-fpu,
->              l1d-flush,branch-harden,srb-lock}=<bool> ]`
+>              bti-thunk=retpoline|lfence|jmp, {ibrs,stibp,ssbd,psfd,ibpb,
+>              eager-fpu,l1d-flush,branch-harden,srb-lock}=<bool> ]`
 
 Controls for speculative execution sidechannel mitigations.  By default, Xen
 will pick the most appropriate mitigations based on compiled in support,
@@ -2306,9 +2306,10 @@  On hardware supporting IBRS (Indirect Branch Restricted Speculation), the
 If Xen is not using IBRS itself, functionality is still set up so IBRS can be
 virtualised for guests.
 
-On hardware supporting IBPB (Indirect Branch Prediction Barrier), the `ibpb=`
-option can be used to force (the default) or prevent Xen from issuing branch
-prediction barriers on vcpu context switches.
+On hardware supporting STIBP (Single Thread Indirect Branch Predictors), the
+`stibp=` option can be used to force or prevent Xen using the feature itself.
+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
@@ -2316,6 +2317,15 @@  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
+default.
+
+On hardware supporting IBPB (Indirect Branch Prediction Barrier), the `ibpb=`
+option can be used to force (the default) or prevent Xen from issuing branch
+prediction barriers on vcpu context switches.
+
 On all hardware, the `eager-fpu=` option can be used to force or prevent Xen
 from using fully eager FPU context switches.  This is currently implemented as
 a global control.  By default, Xen will choose to use fully eager context
diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
index 1408e4c7abd0..446b62486447 100644
--- a/xen/arch/x86/spec_ctrl.c
+++ b/xen/arch/x86/spec_ctrl.c
@@ -48,9 +48,13 @@  static enum ind_thunk {
     THUNK_LFENCE,
     THUNK_JMP,
 } opt_thunk __initdata = THUNK_DEFAULT;
+
 static int8_t __initdata opt_ibrs = -1;
+int8_t __initdata opt_stibp = -1;
+bool __read_mostly opt_ssbd;
+int8_t __initdata opt_psfd = -1;
+
 bool __read_mostly opt_ibpb = true;
-bool __read_mostly opt_ssbd = false;
 int8_t __read_mostly opt_eager_fpu = -1;
 int8_t __read_mostly opt_l1d_flush = -1;
 static bool __initdata opt_branch_harden = true;
@@ -170,12 +174,18 @@  static int __init cf_check parse_spec_ctrl(const char *s)
             else
                 rc = -EINVAL;
         }
+
         else if ( (val = parse_boolean("ibrs", s, ss)) >= 0 )
             opt_ibrs = val;
-        else if ( (val = parse_boolean("ibpb", s, ss)) >= 0 )
-            opt_ibpb = 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;
+
+        else if ( (val = parse_boolean("ibpb", s, ss)) >= 0 )
+            opt_ibpb = val;
         else if ( (val = parse_boolean("eager-fpu", s, ss)) >= 0 )
             opt_eager_fpu = val;
         else if ( (val = parse_boolean("l1d-flush", s, ss)) >= 0 )
@@ -367,7 +377,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, Other:%s%s%s%s%s\n",
+    printk("  Xen settings: BTI-Thunk %s, SPEC_CTRL: %s%s%s%s%s, Other:%s%s%s%s%s\n",
            thunk == THUNK_NONE      ? "N/A" :
            thunk == THUNK_RETPOLINE ? "RETPOLINE" :
            thunk == THUNK_LFENCE    ? "LFENCE" :
@@ -381,6 +391,9 @@  static void __init print_details(enum ind_thunk thunk, uint64_t caps)
            (!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-",
            !(caps & ARCH_CAPS_TSX_CTRL)              ? "" :
            (opt_tsx & 1)                             ? " TSX+" : " TSX-",
            !cpu_has_srbds_ctrl                       ? "" :
@@ -1070,12 +1083,50 @@  void __init init_speculation_mitigations(void)
 
     /* If we have IBRS available, see whether we should use it. */
     if ( has_spec_ctrl && ibrs )
+    {
+        /* IBRS implies STIBP.  */
+        if ( opt_stibp == -1 )
+            opt_stibp = 1;
+
         default_xen_spec_ctrl |= SPEC_CTRL_IBRS;
+    }
+
+    /* Use STIBP by default if the hardware hint is set. */
+    if ( opt_stibp == -1 && boot_cpu_has(X86_FEATURE_STIBP_ALWAYS) )
+        opt_stibp = 1;
+
+    /*
+     * Otherwise, don't use STIBP by default.  It has some severe performance
+     * implications on older hardware.
+     */
+    if ( opt_stibp == -1 )
+        opt_stibp = 0;
+
+    if ( opt_stibp && (boot_cpu_has(X86_FEATURE_STIBP) ||
+                       boot_cpu_has(X86_FEATURE_AMD_STIBP)) )
+        default_xen_spec_ctrl |= SPEC_CTRL_STIBP;
 
     /* If we have SSBD available, see whether we should use it. */
     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 off by default.
+     */
+    if ( opt_psfd == -1 )
+        opt_psfd = 0;
+
+    if ( opt_psfd && (boot_cpu_has(X86_FEATURE_PSFD) ||
+                      boot_cpu_has(X86_FEATURE_INTEL_PSFD)) )
+        default_xen_spec_ctrl |= SPEC_CTRL_PSFD;
 
     /*
      * PV guests can create RSB entries for any linear address they control,