[PATCH v3 0/2] SSBD via LS_CFG

[PATCH v3 0/2] SSBD via LS_CFG

[Brian Woods]

This series of patches is for enabling SSBD via the LS_CFG MSR for
family 15h-17h.  The first patch make it so that the information is
correctly displayed on boot.  The last patch is for further enablement
for Xen switching SSBD on or off internally, or for further control of
SSBD for guests via the VIRT_SPEC_CTRL.

Note: Patch 1 is standalone and just improves reporting of SSBD in the
init print statements.  Patch 2 is intended for a later date when
there's a C ALTERNATIVE (which I've talked to Andy about some) and will
most likely get rolled into series of patches for giving HVM guests
control of SSBD when it's only available via LS_CFG.

v1 -> v2:
    - changed some variable types
    - updated ssbd_amd_ls_cfg_set_nonsmt function
    - changed sbd_amd_ls_cfg_init to a presmp_initcall
    - deleted ssbd_amd_ls_cfg_set_init due to ^ and it happening later
      in the boot
    - various smaller cleanups
v2 -> v3:
    - moved the SSBD_AMD_LS_CFG synthetic feature from patch 1 to 2
      and added ssbd_amd_ls_cfg_av for it's use in patch 1
    - in patch 2, only set SSBD_AMD_LS_CFG once everything is known to
      be ready/good
    - since it's later in the boot process due to a v1 change, use
      nr_sockets
    - various smaller changes/cleanups from Jan's feedback

Brian Woods (2):
  x86/spec-ctrl: add AMD SSBD LS_CFG in init print
  x86/spec-ctrl: add support for modifying SSBD via LS_CFG MSR

 xen/arch/x86/cpu/amd.c            |  15 ++--
 xen/arch/x86/smpboot.c            |   3 +
 xen/arch/x86/spec_ctrl.c          | 180 +++++++++++++++++++++++++++++++++++++-
 xen/include/asm-x86/cpufeatures.h |   1 +
 xen/include/asm-x86/spec_ctrl.h   |   5 ++
 5 files changed, 195 insertions(+), 9 deletions(-)

-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v3 1/2] x86/spec-ctrl: add AMD SSBD LS_CFG in init print

[Brian Woods]

Edit the initialization code for AMD's SSBD via the LS_CFG MSR and
enable it to pass the status to the initial spec-ctrl print_details at
boot.

Signed-off-by: Brian Woods <brian.woods@amd.com>
---
 xen/arch/x86/cpu/amd.c          | 12 +++++++++---
 xen/arch/x86/spec_ctrl.c        | 10 ++++++++--
 xen/include/asm-x86/spec_ctrl.h |  3 +++
 3 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index a7afa2fa7a..6e65ae7427 100644
--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -604,7 +604,7 @@ static void init_amd(struct cpuinfo_x86 *c)
 	 * If the user has explicitly chosen to disable Memory Disambiguation
 	 * to mitigiate Speculative Store Bypass, poke the appropriate MSR.
 	 */
-	if (opt_ssbd) {
+	if (!ssbd_amd_ls_cfg_mask) {
 		int bit = -1;
 
 		switch (c->x86) {
@@ -613,8 +613,14 @@ static void init_amd(struct cpuinfo_x86 *c)
 		case 0x17: bit = 10; break;
 		}
 
-		if (bit >= 0 && !rdmsr_safe(MSR_AMD64_LS_CFG, value)) {
-			value |= 1ull << bit;
+		if (bit >= 0)
+			ssbd_amd_ls_cfg_mask = 1ull << bit;
+	}
+
+	if (ssbd_amd_ls_cfg_mask && !rdmsr_safe(MSR_AMD64_LS_CFG, value)) {
+		ssbd_amd_ls_cfg_av = true;
+		if (opt_ssbd) {
+			value |= ssbd_amd_ls_cfg_mask;
 			wrmsr_safe(MSR_AMD64_LS_CFG, value);
 		}
 	}
diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
index c430b25b84..b32c12c6c0 100644
--- a/xen/arch/x86/spec_ctrl.c
+++ b/xen/arch/x86/spec_ctrl.c
@@ -58,6 +58,9 @@ paddr_t __read_mostly l1tf_addr_mask, __read_mostly l1tf_safe_maddr;
 static bool __initdata cpu_has_bug_l1tf;
 static unsigned int __initdata l1d_maxphysaddr;
 
+bool ssbd_amd_ls_cfg_av;
+uint64_t __read_mostly ssbd_amd_ls_cfg_mask;
+
 static int __init parse_bti(const char *s)
 {
     const char *ss;
@@ -281,11 +284,12 @@ static void __init print_details(enum ind_thunk thunk, uint64_t caps)
     printk("Speculative mitigation facilities:\n");
 
     /* Hardware features which pertain to speculative mitigations. */
-    printk("  Hardware features:%s%s%s%s%s%s%s%s%s%s\n",
+    printk("  Hardware features:%s%s%s%s%s%s%s%s%s%s%s\n",
            (_7d0 & cpufeat_mask(X86_FEATURE_IBRSB)) ? " IBRS/IBPB" : "",
            (_7d0 & cpufeat_mask(X86_FEATURE_STIBP)) ? " STIBP"     : "",
            (_7d0 & cpufeat_mask(X86_FEATURE_L1D_FLUSH)) ? " L1D_FLUSH" : "",
            (_7d0 & cpufeat_mask(X86_FEATURE_SSBD))  ? " SSBD"      : "",
+           ssbd_amd_ls_cfg_av                       ? " LS_CFG_SSBD" : "",
            (e8b  & cpufeat_mask(X86_FEATURE_IBPB))  ? " IBPB"      : "",
            (caps & ARCH_CAPABILITIES_IBRS_ALL)      ? " IBRS_ALL"  : "",
            (caps & ARCH_CAPABILITIES_RDCL_NO)       ? " RDCL_NO"   : "",
@@ -305,7 +309,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, Other:%s%s\n",
+    printk("  Xen settings: BTI-Thunk %s, SPEC_CTRL: %s%s%s, Other:%s%s\n",
            thunk == THUNK_NONE      ? "N/A" :
            thunk == THUNK_RETPOLINE ? "RETPOLINE" :
            thunk == THUNK_LFENCE    ? "LFENCE" :
@@ -314,6 +318,8 @@ static void __init print_details(enum ind_thunk thunk, uint64_t caps)
            (default_xen_spec_ctrl & SPEC_CTRL_IBRS)  ? "IBRS+" :  "IBRS-",
            !boot_cpu_has(X86_FEATURE_SSBD)           ? "" :
            (default_xen_spec_ctrl & SPEC_CTRL_SSBD)  ? " SSBD+" : " SSBD-",
+           !ssbd_amd_ls_cfg_av                       ? "" :
+           opt_ssbd                                  ? " LS_CFG_SSBD+" : " LS_CFG_SSBD-",
            opt_ibpb                                  ? " IBPB"  : "",
            opt_l1d_flush                             ? " L1D_FLUSH" : "");
 
diff --git a/xen/include/asm-x86/spec_ctrl.h b/xen/include/asm-x86/spec_ctrl.h
index 8f8aad40bb..1b9101a988 100644
--- a/xen/include/asm-x86/spec_ctrl.h
+++ b/xen/include/asm-x86/spec_ctrl.h
@@ -50,6 +50,9 @@ extern int8_t opt_pv_l1tf;
  */
 extern paddr_t l1tf_addr_mask, l1tf_safe_maddr;
 
+extern bool ssbd_amd_ls_cfg_av;
+extern uint64_t ssbd_amd_ls_cfg_mask;
+
 static inline void init_shadow_spec_ctrl_state(void)
 {
     struct cpu_info *info = get_cpu_info();
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v3 2/2] x86/spec-ctrl: add support for modifying SSBD via LS_CFG MSR

[Brian Woods]

Adds support for modifying the LS_CFG MSR to enable SSBD on supporting
AMD CPUs.  There needs to be locking logic for family 17h with SMT
enabled since both threads share the same MSR.  Otherwise, a core just
needs to write to the LS_CFG MSR.  For more information see:
https://developer.amd.com/wp-content/resources/124441_AMD64_SpeculativeStoreBypassDisable_Whitepaper_final.pdf

Signed-off-by: Brian Woods <brian.woods@amd.com>
---
 xen/arch/x86/cpu/amd.c            |  13 +--
 xen/arch/x86/smpboot.c            |   3 +
 xen/arch/x86/spec_ctrl.c          | 172 +++++++++++++++++++++++++++++++++++++-
 xen/include/asm-x86/cpufeatures.h |   1 +
 xen/include/asm-x86/spec_ctrl.h   |   2 +
 5 files changed, 181 insertions(+), 10 deletions(-)

diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index 6e65ae7427..e96f14268e 100644
--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -601,8 +601,8 @@ static void init_amd(struct cpuinfo_x86 *c)
 	}
 
 	/*
-	 * If the user has explicitly chosen to disable Memory Disambiguation
-	 * to mitigiate Speculative Store Bypass, poke the appropriate MSR.
+	 * Poke the LS_CFG MSR to see if the mitigation for Speculative
+	 * Store Bypass is available.
 	 */
 	if (!ssbd_amd_ls_cfg_mask) {
 		int bit = -1;
@@ -615,14 +615,9 @@ static void init_amd(struct cpuinfo_x86 *c)
 
 		if (bit >= 0)
 			ssbd_amd_ls_cfg_mask = 1ull << bit;
-	}
 
-	if (ssbd_amd_ls_cfg_mask && !rdmsr_safe(MSR_AMD64_LS_CFG, value)) {
-		ssbd_amd_ls_cfg_av = true;
-		if (opt_ssbd) {
-			value |= ssbd_amd_ls_cfg_mask;
-			wrmsr_safe(MSR_AMD64_LS_CFG, value);
-		}
+		if (ssbd_amd_ls_cfg_mask && !rdmsr_safe(MSR_AMD64_LS_CFG, value))
+			ssbd_amd_ls_cfg_av = true;
 	}
 
 	/* MFENCE stops RDTSC speculation */
diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index 7e76cc3d68..b103b46dee 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -376,6 +376,9 @@ void start_secondary(void *unused)
     if ( boot_cpu_has(X86_FEATURE_IBRSB) )
         wrmsrl(MSR_SPEC_CTRL, default_xen_spec_ctrl);
 
+    if ( default_xen_ssbd_amd_ls_cfg_en )
+        ssbd_amd_ls_cfg_set(true);
+
     if ( xen_guest )
         hypervisor_ap_setup();
 
diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
index b32c12c6c0..89cc444f56 100644
--- a/xen/arch/x86/spec_ctrl.c
+++ b/xen/arch/x86/spec_ctrl.c
@@ -20,6 +20,7 @@
 #include <xen/init.h>
 #include <xen/lib.h>
 #include <xen/warning.h>
+#include <xen/spinlock.h>
 
 #include <asm/microcode.h>
 #include <asm/msr.h>
@@ -58,8 +59,17 @@ paddr_t __read_mostly l1tf_addr_mask, __read_mostly l1tf_safe_maddr;
 static bool __initdata cpu_has_bug_l1tf;
 static unsigned int __initdata l1d_maxphysaddr;
 
+/* for SSBD support for AMD via LS_CFG */
+#define SSBD_AMD_MAX_SOCKET 4
+struct ssbd_amd_ls_cfg_smt_status {
+    spinlock_t lock;
+    uint32_t mask;
+} __attribute__ ((aligned (64)));
+bool __read_mostly ssbd_amd_smt_en;
+bool __read_mostly default_xen_ssbd_amd_ls_cfg_en;
 bool ssbd_amd_ls_cfg_av;
 uint64_t __read_mostly ssbd_amd_ls_cfg_mask;
+struct ssbd_amd_ls_cfg_smt_status *ssbd_amd_smt_status[SSBD_AMD_MAX_SOCKET];
 
 static int __init parse_bti(const char *s)
 {
@@ -319,7 +329,7 @@ static void __init print_details(enum ind_thunk thunk, uint64_t caps)
            !boot_cpu_has(X86_FEATURE_SSBD)           ? "" :
            (default_xen_spec_ctrl & SPEC_CTRL_SSBD)  ? " SSBD+" : " SSBD-",
            !ssbd_amd_ls_cfg_av                       ? "" :
-           opt_ssbd                                  ? " LS_CFG_SSBD+" : " LS_CFG_SSBD-",
+           default_xen_ssbd_amd_ls_cfg_en            ? " LS_CFG_SSBD+" : " LS_CFG_SSBD-",
            opt_ibpb                                  ? " IBPB"  : "",
            opt_l1d_flush                             ? " L1D_FLUSH" : "");
 
@@ -725,6 +735,162 @@ static __init int parse_xpti(const char *s)
 }
 custom_param("xpti", parse_xpti);
 
+/*
+ * Enabling SSBD on AMD processers via the LS_CFG MSR
+ *
+ * For family 15h and 16h, there are no SMT enabled processors, so there
+ * is no need for locking, just setting an MSR bit.  For 17h, it depends
+ * if SMT is enabled.  If SMT, are two threads that share a single MSR
+ * so there needs to be a lock and a virtual bit for each thread,
+ * otherwise it's the same as family 15h/16h.
+ */
+
+static void ssbd_amd_ls_cfg_set_nonsmt(bool enable_ssbd)
+{
+    uint64_t ls_cfg, new_ls_cfg;
+
+    rdmsrl(MSR_AMD64_LS_CFG, ls_cfg);
+
+    if ( enable_ssbd )
+        new_ls_cfg = ls_cfg | ssbd_amd_ls_cfg_mask;
+    else
+        new_ls_cfg = ls_cfg & ~ssbd_amd_ls_cfg_mask;
+
+    if ( new_ls_cfg != ls_cfg )
+        wrmsrl(MSR_AMD64_LS_CFG, new_ls_cfg);
+}
+
+static void ssbd_amd_ls_cfg_set_smt(bool enable_ssbd)
+{
+    unsigned socket, core, thread;
+    uint64_t enable_mask;
+    uint64_t ls_cfg;
+    struct ssbd_amd_ls_cfg_smt_status *status;
+    const struct cpuinfo_x86  *c =  &current_cpu_data;
+
+    socket = c->phys_proc_id;
+    core   = c->cpu_core_id;
+    thread = c->apicid & (c->x86_num_siblings - 1);
+    status = ssbd_amd_smt_status[socket] + core;
+    enable_mask = (1ull << thread);
+
+    spin_lock(&status->lock);
+
+    if ( enable_ssbd )
+    {
+        if ( !(status->mask & enable_mask) )
+        {
+            status->mask |= enable_mask;
+            rdmsrl(MSR_AMD64_LS_CFG, ls_cfg);
+            if ( !(ls_cfg & ssbd_amd_ls_cfg_mask) )
+            {
+                ls_cfg |= ssbd_amd_ls_cfg_mask;
+                wrmsrl(MSR_AMD64_LS_CFG, ls_cfg);
+            }
+        }
+    }
+    else
+    {
+        if ( status->mask & enable_mask )
+        {
+            status->mask &= ~enable_mask;
+            rdmsrl(MSR_AMD64_LS_CFG, ls_cfg);
+            if ( (ls_cfg & ssbd_amd_ls_cfg_mask) && (status->mask == 0) )
+            {
+                ls_cfg &= ~ssbd_amd_ls_cfg_mask;
+                wrmsrl(MSR_AMD64_LS_CFG, ls_cfg);
+            }
+        }
+    }
+
+    spin_unlock(&status->lock);
+}
+
+void ssbd_amd_ls_cfg_set(bool enable_ssbd)
+{
+    if ( !boot_cpu_has(X86_FEATURE_SSBD_AMD_LS_CFG) )
+        return;
+
+    ASSERT(ssbd_amd_ls_cfg_mask);
+
+    if ( ssbd_amd_smt_en )
+        ssbd_amd_ls_cfg_set_smt(enable_ssbd);
+    else
+        ssbd_amd_ls_cfg_set_nonsmt(enable_ssbd);
+}
+
+static int __init ssbd_amd_ls_cfg_init(void)
+{
+    unsigned cores_per_socket, threads_per_core;
+    unsigned core, socket;
+    const struct cpuinfo_x86  *c =  &boot_cpu_data;
+
+    if ( !ssbd_amd_ls_cfg_av )
+        return 0;
+
+    ASSERT(ssbd_amd_ls_cfg_mask);
+
+    switch ( c->x86 )
+    {
+    case 0x15:
+    case 0x16:
+        break;
+
+    case 0x17:
+        cores_per_socket = c->x86_max_cores;
+        threads_per_core = c->x86_num_siblings;
+
+        if ( threads_per_core > 1 )
+        {
+            ssbd_amd_smt_en = true;
+            for ( socket = 0; socket < nr_sockets; socket++ )
+            {
+                ssbd_amd_smt_status[socket] =
+                  (struct ssbd_amd_ls_cfg_smt_status *)
+                  xmalloc_array(struct ssbd_amd_ls_cfg_smt_status,
+                                cores_per_socket);
+                if ( ssbd_amd_smt_status[socket] == NULL )
+                {
+                    dprintk(XENLOG_ERR,
+                            "SSBD AMD LS CFG: error in status allocing\n");
+                    goto ssbd_amd_ls_cfg_init_fail;
+                }
+            }
+
+            for ( socket = 0; socket < nr_sockets; socket++ )
+            {
+                for ( core = 0; core < cores_per_socket; core++ )
+                {
+                    spin_lock_init(&ssbd_amd_smt_status[socket][core].lock);
+                    ssbd_amd_smt_status[socket][core].mask = 0;
+                }
+            }
+        }
+        break;
+
+    default:
+        goto ssbd_amd_ls_cfg_init_fail;
+    }
+
+    setup_force_cpu_cap(X86_FEATURE_SSBD_AMD_LS_CFG);
+
+    if ( default_xen_ssbd_amd_ls_cfg_en )
+        ssbd_amd_ls_cfg_set(true);
+
+    return 0;
+
+ ssbd_amd_ls_cfg_init_fail:
+    for ( socket = 0; socket < SSBD_AMD_MAX_SOCKET; socket++ )
+        xfree(ssbd_amd_smt_status[socket]);
+
+    default_xen_ssbd_amd_ls_cfg_en = false;
+
+    dprintk(XENLOG_ERR, "SSBD AMD LS CFG: disalbing SSBD due to errors\n");
+
+    return 0;
+}
+presmp_initcall(ssbd_amd_ls_cfg_init);
+
 void __init init_speculation_mitigations(void)
 {
     enum ind_thunk thunk = THUNK_DEFAULT;
@@ -827,6 +993,10 @@ void __init init_speculation_mitigations(void)
     if ( boot_cpu_has(X86_FEATURE_SSBD) && opt_ssbd )
         default_xen_spec_ctrl |= SPEC_CTRL_SSBD;
 
+    /* if we have SSBD LS_CFG available, see whether we should use it. */
+    if ( ssbd_amd_ls_cfg_av && opt_ssbd )
+         default_xen_ssbd_amd_ls_cfg_en = true;
+
     /*
      * PV guests can poison the RSB to any virtual address from which
      * they can execute a call instruction.  This is necessarily outside
diff --git a/xen/include/asm-x86/cpufeatures.h b/xen/include/asm-x86/cpufeatures.h
index 249fa6e531..8927704ef3 100644
--- a/xen/include/asm-x86/cpufeatures.h
+++ b/xen/include/asm-x86/cpufeatures.h
@@ -33,3 +33,4 @@ XEN_CPUFEATURE(SC_RSB_HVM,      (FSCAPINTS+0)*32+19) /* RSB overwrite needed for
 XEN_CPUFEATURE(NO_XPTI,         (FSCAPINTS+0)*32+20) /* XPTI mitigation not in use */
 XEN_CPUFEATURE(SC_MSR_IDLE,     (FSCAPINTS+0)*32+21) /* (SC_MSR_PV || SC_MSR_HVM) && default_xen_spec_ctrl */
 XEN_CPUFEATURE(XEN_LBR,         (FSCAPINTS+0)*32+22) /* Xen uses MSR_DEBUGCTL.LBR */
+XEN_CPUFEATURE(SSBD_AMD_LS_CFG, (FSCAPINTS+0)*32+23) /* if SSBD support is enabled via LS    _CGF MSR on AMD hardware */
diff --git a/xen/include/asm-x86/spec_ctrl.h b/xen/include/asm-x86/spec_ctrl.h
index 1b9101a988..b142cf09d7 100644
--- a/xen/include/asm-x86/spec_ctrl.h
+++ b/xen/include/asm-x86/spec_ctrl.h
@@ -52,6 +52,8 @@ extern paddr_t l1tf_addr_mask, l1tf_safe_maddr;
 
 extern bool ssbd_amd_ls_cfg_av;
 extern uint64_t ssbd_amd_ls_cfg_mask;
+extern bool default_xen_ssbd_amd_ls_cfg_en;
+extern void ssbd_amd_ls_cfg_set(bool enable_ssbd);
 
 static inline void init_shadow_spec_ctrl_state(void)
 {
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 1/2] x86/spec-ctrl: add AMD SSBD LS_CFG in init print

[Jan Beulich]

>>> On 27.08.18 at 18:54, <brian.woods@amd.com> wrote:
> Edit the initialization code for AMD's SSBD via the LS_CFG MSR and
> enable it to pass the status to the initial spec-ctrl print_details at
> boot.
> 
> Signed-off-by: Brian Woods <brian.woods@amd.com>
> ---

I think I had indicated so before: A brief revision log is expected here,
in particular to aid people having looked at prior versions of a patch.
Whether you also put such a log in the cover letter is up to you, but
there it's not normally going to be patch specific.

> --- a/xen/arch/x86/cpu/amd.c
> +++ b/xen/arch/x86/cpu/amd.c
> @@ -604,7 +604,7 @@ static void init_amd(struct cpuinfo_x86 *c)
>  	 * If the user has explicitly chosen to disable Memory Disambiguation
>  	 * to mitigiate Speculative Store Bypass, poke the appropriate MSR.
>  	 */
> -	if (opt_ssbd) {
> +	if (!ssbd_amd_ls_cfg_mask) {
>  		int bit = -1;
>  
>  		switch (c->x86) {
> @@ -613,8 +613,14 @@ static void init_amd(struct cpuinfo_x86 *c)
>  		case 0x17: bit = 10; break;
>  		}
>  
> -		if (bit >= 0 && !rdmsr_safe(MSR_AMD64_LS_CFG, value)) {
> -			value |= 1ull << bit;
> +		if (bit >= 0)
> +			ssbd_amd_ls_cfg_mask = 1ull << bit;
> +	}
> +
> +	if (ssbd_amd_ls_cfg_mask && !rdmsr_safe(MSR_AMD64_LS_CFG, value)) {
> +		ssbd_amd_ls_cfg_av = true;

"av" is standing for "avail" I guess? If so, I'd prefer if you spelled it out.

Also two of the three comments given on v2 still apply here. Please
address _all_ comments either verbally or by code changes before
sending a new version.

> --- a/xen/arch/x86/spec_ctrl.c
> +++ b/xen/arch/x86/spec_ctrl.c
> @@ -58,6 +58,9 @@ paddr_t __read_mostly l1tf_addr_mask, __read_mostly l1tf_safe_maddr;
>  static bool __initdata cpu_has_bug_l1tf;
>  static unsigned int __initdata l1d_maxphysaddr;
>  
> +bool ssbd_amd_ls_cfg_av;

__read_mostly (afaict)

Also the variable seems pointless - this ...

> +uint64_t __read_mostly ssbd_amd_ls_cfg_mask;

... variable being (non-)zero should suffice as indication.
Otherwise the definition of this variable in this file looks
wrong, as there's no reference to it in here.

> @@ -281,11 +284,12 @@ static void __init print_details(enum ind_thunk thunk, 
> uint64_t caps)
>      printk("Speculative mitigation facilities:\n");
>  
>      /* Hardware features which pertain to speculative mitigations. */
> -    printk("  Hardware features:%s%s%s%s%s%s%s%s%s%s\n",
> +    printk("  Hardware features:%s%s%s%s%s%s%s%s%s%s%s\n",
>             (_7d0 & cpufeat_mask(X86_FEATURE_IBRSB)) ? " IBRS/IBPB" : "",
>             (_7d0 & cpufeat_mask(X86_FEATURE_STIBP)) ? " STIBP"     : "",
>             (_7d0 & cpufeat_mask(X86_FEATURE_L1D_FLUSH)) ? " L1D_FLUSH" : "",
>             (_7d0 & cpufeat_mask(X86_FEATURE_SSBD))  ? " SSBD"      : "",
> +           ssbd_amd_ls_cfg_av                       ? " LS_CFG_SSBD" : "",
>             (e8b  & cpufeat_mask(X86_FEATURE_IBPB))  ? " IBPB"      : "",
>             (caps & ARCH_CAPABILITIES_IBRS_ALL)      ? " IBRS_ALL"  : "",
>             (caps & ARCH_CAPABILITIES_RDCL_NO)       ? " RDCL_NO"   : "",
> @@ -305,7 +309,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, Other:%s%s\n",
> +    printk("  Xen settings: BTI-Thunk %s, SPEC_CTRL: %s%s%s, Other:%s%s\n",
>             thunk == THUNK_NONE      ? "N/A" :
>             thunk == THUNK_RETPOLINE ? "RETPOLINE" :
>             thunk == THUNK_LFENCE    ? "LFENCE" :
> @@ -314,6 +318,8 @@ static void __init print_details(enum ind_thunk thunk, 
> uint64_t caps)
>             (default_xen_spec_ctrl & SPEC_CTRL_IBRS)  ? "IBRS+" :  "IBRS-",
>             !boot_cpu_has(X86_FEATURE_SSBD)           ? "" :
>             (default_xen_spec_ctrl & SPEC_CTRL_SSBD)  ? " SSBD+" : " SSBD-",
> +           !ssbd_amd_ls_cfg_av                       ? "" :
> +           opt_ssbd                                  ? " LS_CFG_SSBD+" : " 
> LS_CFG_SSBD-",
>             opt_ibpb                                  ? " IBPB"  : "",
>             opt_l1d_flush                             ? " L1D_FLUSH" : "");
>  
> diff --git a/xen/include/asm-x86/spec_ctrl.h b/xen/include/asm-x86/spec_ctrl.h
> index 8f8aad40bb..1b9101a988 100644
> --- a/xen/include/asm-x86/spec_ctrl.h
> +++ b/xen/include/asm-x86/spec_ctrl.h
> @@ -50,6 +50,9 @@ extern int8_t opt_pv_l1tf;
>   */
>  extern paddr_t l1tf_addr_mask, l1tf_safe_maddr;
>  
> +extern bool ssbd_amd_ls_cfg_av;
> +extern uint64_t ssbd_amd_ls_cfg_mask;
> +
>  static inline void init_shadow_spec_ctrl_state(void)
>  {
>      struct cpu_info *info = get_cpu_info();
> -- 
> 2.11.0




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 2/2] x86/spec-ctrl: add support for modifying SSBD via LS_CFG MSR

[Jan Beulich]

>>> On 27.08.18 at 18:55, <brian.woods@amd.com> wrote:
> Adds support for modifying the LS_CFG MSR to enable SSBD on supporting
> AMD CPUs.  There needs to be locking logic for family 17h with SMT
> enabled since both threads share the same MSR.  Otherwise, a core just
> needs to write to the LS_CFG MSR.  For more information see:
> https://developer.amd.com/wp-content/resources/124441_AMD64_SpeculativeStoreB 
> ypassDisable_Whitepaper_final.pdf
> 
> Signed-off-by: Brian Woods <brian.woods@amd.com>
> ---
>  xen/arch/x86/cpu/amd.c            |  13 +--
>  xen/arch/x86/smpboot.c            |   3 +
>  xen/arch/x86/spec_ctrl.c          | 172 
> +++++++++++++++++++++++++++++++++++++-
>  xen/include/asm-x86/cpufeatures.h |   1 +
>  xen/include/asm-x86/spec_ctrl.h   |   2 +
>  5 files changed, 181 insertions(+), 10 deletions(-)
> 
> diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
> index 6e65ae7427..e96f14268e 100644
> --- a/xen/arch/x86/cpu/amd.c
> +++ b/xen/arch/x86/cpu/amd.c
> @@ -601,8 +601,8 @@ static void init_amd(struct cpuinfo_x86 *c)
>  	}
>  
>  	/*
> -	 * If the user has explicitly chosen to disable Memory Disambiguation
> -	 * to mitigiate Speculative Store Bypass, poke the appropriate MSR.
> +	 * Poke the LS_CFG MSR to see if the mitigation for Speculative
> +	 * Store Bypass is available.
>  	 */
>  	if (!ssbd_amd_ls_cfg_mask) {
>  		int bit = -1;
> @@ -615,14 +615,9 @@ static void init_amd(struct cpuinfo_x86 *c)
>  
>  		if (bit >= 0)
>  			ssbd_amd_ls_cfg_mask = 1ull << bit;
> -	}
>  
> -	if (ssbd_amd_ls_cfg_mask && !rdmsr_safe(MSR_AMD64_LS_CFG, value)) {
> -		ssbd_amd_ls_cfg_av = true;
> -		if (opt_ssbd) {
> -			value |= ssbd_amd_ls_cfg_mask;
> -			wrmsr_safe(MSR_AMD64_LS_CFG, value);
> -		}
> +		if (ssbd_amd_ls_cfg_mask && !rdmsr_safe(MSR_AMD64_LS_CFG, value))
> +			ssbd_amd_ls_cfg_av = true;
>  	}
>  
>  	/* MFENCE stops RDTSC speculation */
> diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
> index 7e76cc3d68..b103b46dee 100644
> --- a/xen/arch/x86/smpboot.c
> +++ b/xen/arch/x86/smpboot.c
> @@ -376,6 +376,9 @@ void start_secondary(void *unused)
>      if ( boot_cpu_has(X86_FEATURE_IBRSB) )
>          wrmsrl(MSR_SPEC_CTRL, default_xen_spec_ctrl);
>  
> +    if ( default_xen_ssbd_amd_ls_cfg_en )
> +        ssbd_amd_ls_cfg_set(true);
> +
>      if ( xen_guest )
>          hypervisor_ap_setup();
>  
> diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
> index b32c12c6c0..89cc444f56 100644
> --- a/xen/arch/x86/spec_ctrl.c
> +++ b/xen/arch/x86/spec_ctrl.c
> @@ -20,6 +20,7 @@
>  #include <xen/init.h>
>  #include <xen/lib.h>
>  #include <xen/warning.h>
> +#include <xen/spinlock.h>
>  
>  #include <asm/microcode.h>
>  #include <asm/msr.h>
> @@ -58,8 +59,17 @@ paddr_t __read_mostly l1tf_addr_mask, __read_mostly l1tf_safe_maddr;
>  static bool __initdata cpu_has_bug_l1tf;
>  static unsigned int __initdata l1d_maxphysaddr;
>  
> +/* for SSBD support for AMD via LS_CFG */
> +#define SSBD_AMD_MAX_SOCKET 4

Oh, went up from 2 to 4? But still not a dynamic upper bound?

> +struct ssbd_amd_ls_cfg_smt_status {
> +    spinlock_t lock;
> +    uint32_t mask;
> +} __attribute__ ((aligned (64)));

Ehem. See the respective comment on patch 1. To put it bluntly,
I'm not willing to look at patches where prior comments were not
addressed, the more that you had verbally agreed to use
SMP_CACHE_BYTES here.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 2/2] x86/spec-ctrl: add support for modifying SSBD via LS_CFG MSR

[Brian Woods]

On Thu, Aug 30, 2018 at 09:49:58AM -0600, Jan Beulich wrote:
> >>> On 27.08.18 at 18:55, <brian.woods@amd.com> wrote:
> > Adds support for modifying the LS_CFG MSR to enable SSBD on supporting
> > AMD CPUs.  There needs to be locking logic for family 17h with SMT
> > enabled since both threads share the same MSR.  Otherwise, a core just
> > needs to write to the LS_CFG MSR.  For more information see:
> > https://developer.amd.com/wp-content/resources/124441_AMD64_SpeculativeStoreB 
> > ypassDisable_Whitepaper_final.pdf
> > 
> > Signed-off-by: Brian Woods <brian.woods@amd.com>
> > ---
> >  xen/arch/x86/cpu/amd.c            |  13 +--
> >  xen/arch/x86/smpboot.c            |   3 +
> >  xen/arch/x86/spec_ctrl.c          | 172 
> > +++++++++++++++++++++++++++++++++++++-
> >  xen/include/asm-x86/cpufeatures.h |   1 +
> >  xen/include/asm-x86/spec_ctrl.h   |   2 +
> >  5 files changed, 181 insertions(+), 10 deletions(-)
> > 
> > diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
> > index 6e65ae7427..e96f14268e 100644
> > --- a/xen/arch/x86/cpu/amd.c
> > +++ b/xen/arch/x86/cpu/amd.c
> > @@ -601,8 +601,8 @@ static void init_amd(struct cpuinfo_x86 *c)
> >  	}
> >  
> >  	/*
> > -	 * If the user has explicitly chosen to disable Memory Disambiguation
> > -	 * to mitigiate Speculative Store Bypass, poke the appropriate MSR.
> > +	 * Poke the LS_CFG MSR to see if the mitigation for Speculative
> > +	 * Store Bypass is available.
> >  	 */
> >  	if (!ssbd_amd_ls_cfg_mask) {
> >  		int bit = -1;
> > @@ -615,14 +615,9 @@ static void init_amd(struct cpuinfo_x86 *c)
> >  
> >  		if (bit >= 0)
> >  			ssbd_amd_ls_cfg_mask = 1ull << bit;
> > -	}
> >  
> > -	if (ssbd_amd_ls_cfg_mask && !rdmsr_safe(MSR_AMD64_LS_CFG, value)) {
> > -		ssbd_amd_ls_cfg_av = true;
> > -		if (opt_ssbd) {
> > -			value |= ssbd_amd_ls_cfg_mask;
> > -			wrmsr_safe(MSR_AMD64_LS_CFG, value);
> > -		}
> > +		if (ssbd_amd_ls_cfg_mask && !rdmsr_safe(MSR_AMD64_LS_CFG, value))
> > +			ssbd_amd_ls_cfg_av = true;
> >  	}
> >  
> >  	/* MFENCE stops RDTSC speculation */
> > diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
> > index 7e76cc3d68..b103b46dee 100644
> > --- a/xen/arch/x86/smpboot.c
> > +++ b/xen/arch/x86/smpboot.c
> > @@ -376,6 +376,9 @@ void start_secondary(void *unused)
> >      if ( boot_cpu_has(X86_FEATURE_IBRSB) )
> >          wrmsrl(MSR_SPEC_CTRL, default_xen_spec_ctrl);
> >  
> > +    if ( default_xen_ssbd_amd_ls_cfg_en )
> > +        ssbd_amd_ls_cfg_set(true);
> > +
> >      if ( xen_guest )
> >          hypervisor_ap_setup();
> >  
> > diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
> > index b32c12c6c0..89cc444f56 100644
> > --- a/xen/arch/x86/spec_ctrl.c
> > +++ b/xen/arch/x86/spec_ctrl.c
> > @@ -20,6 +20,7 @@
> >  #include <xen/init.h>
> >  #include <xen/lib.h>
> >  #include <xen/warning.h>
> > +#include <xen/spinlock.h>
> >  
> >  #include <asm/microcode.h>
> >  #include <asm/msr.h>
> > @@ -58,8 +59,17 @@ paddr_t __read_mostly l1tf_addr_mask, __read_mostly l1tf_safe_maddr;
> >  static bool __initdata cpu_has_bug_l1tf;
> >  static unsigned int __initdata l1d_maxphysaddr;
> >  
> > +/* for SSBD support for AMD via LS_CFG */
> > +#define SSBD_AMD_MAX_SOCKET 4
> 
> Oh, went up from 2 to 4? But still not a dynamic upper bound?
> 

Well, 4 was just to be safe.  2 is more than reasonable IMO.  Having it
dynamic seems like a lot of work to save 8 bytes (or after the bump to
4, 24 bytes) when it doesn't get used.

> > +struct ssbd_amd_ls_cfg_smt_status {
> > +    spinlock_t lock;
> > +    uint32_t mask;
> > +} __attribute__ ((aligned (64)));
> 
> Ehem. See the respective comment on patch 1. To put it bluntly,
> I'm not willing to look at patches where prior comments were not
> addressed, the more that you had verbally agreed to use
> SMP_CACHE_BYTES here.
> 
> Jan
> 

Well, a rebasing failed  and I had to regenerate the patches from a
diff of the v3 patches combined, and I missed fixing that one part.
I'm terrible sorry I missed that one fix.

-- 
Brian Woods

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 2/2] x86/spec-ctrl: add support for modifying SSBD via LS_CFG MSR

[Jan Beulich]

>>> On 30.08.18 at 20:09, <brian.woods@amd.com> wrote:
> On Thu, Aug 30, 2018 at 09:49:58AM -0600, Jan Beulich wrote:
>> >>> On 27.08.18 at 18:55, <brian.woods@amd.com> wrote:
>> > --- a/xen/arch/x86/spec_ctrl.c
>> > +++ b/xen/arch/x86/spec_ctrl.c
>> > @@ -20,6 +20,7 @@
>> >  #include <xen/init.h>
>> >  #include <xen/lib.h>
>> >  #include <xen/warning.h>
>> > +#include <xen/spinlock.h>
>> >  
>> >  #include <asm/microcode.h>
>> >  #include <asm/msr.h>
>> > @@ -58,8 +59,17 @@ paddr_t __read_mostly l1tf_addr_mask, __read_mostly l1tf_safe_maddr;
>> >  static bool __initdata cpu_has_bug_l1tf;
>> >  static unsigned int __initdata l1d_maxphysaddr;
>> >  
>> > +/* for SSBD support for AMD via LS_CFG */
>> > +#define SSBD_AMD_MAX_SOCKET 4
>> 
>> Oh, went up from 2 to 4? But still not a dynamic upper bound?
>> 
> 
> Well, 4 was just to be safe.  2 is more than reasonable IMO.  Having it
> dynamic seems like a lot of work to save 8 bytes (or after the bump to
> 4, 24 bytes) when it doesn't get used.

Well, am I misremembering that I saw you say somewhere that you'd
switch to using nr_sockets?

>> > +struct ssbd_amd_ls_cfg_smt_status {
>> > +    spinlock_t lock;
>> > +    uint32_t mask;
>> > +} __attribute__ ((aligned (64)));
>> 
>> Ehem. See the respective comment on patch 1. To put it bluntly,
>> I'm not willing to look at patches where prior comments were not
>> addressed, the more that you had verbally agreed to use
>> SMP_CACHE_BYTES here.
> 
> Well, a rebasing failed  and I had to regenerate the patches from a
> diff of the v3 patches combined, and I missed fixing that one part.
> I'm terrible sorry I missed that one fix.

If it was just that one fix, I wouldn't have stopped review at that
point. But together with patch 1 it makes at least three missed
fixes, and that made me then consider it too much.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel