[PATCH v2 0/2] SSBD AMD via LS CFG Enablement

[PATCH v2 0/2] SSBD AMD via LS CFG Enablement

[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: these should be applied after Jan's patch:
x86/AMD: distinguish compute units from hyper-threads

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

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            |  12 ++-
 xen/arch/x86/setup.c              |   2 +
 xen/arch/x86/smpboot.c            |   3 +
 xen/arch/x86/spec_ctrl.c          | 202 +++++++++++++++++++++++++++++++++++++-
 xen/include/asm-x86/cpufeatures.h |   1 +
 xen/include/asm-x86/spec_ctrl.h   |   5 +
 6 files changed, 218 insertions(+), 7 deletions(-)

-- 
2.11.0


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            |  12 +--
 xen/arch/x86/smpboot.c            |   3 +
 xen/arch/x86/spec_ctrl.c          | 179 +++++++++++++++++++++++++++++++++++++-
 xen/include/asm-x86/cpufeatures.h |   1 +
 xen/include/asm-x86/spec_ctrl.h   |   4 +
 5 files changed, 192 insertions(+), 7 deletions(-)

-- 
2.11.0


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

[PATCH v2 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            | 13 ++++++++++---
 xen/arch/x86/spec_ctrl.c          |  9 +++++++--
 xen/include/asm-x86/cpufeatures.h |  1 +
 xen/include/asm-x86/spec_ctrl.h   |  2 ++
 4 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index bad5b43628..06c9e9661b 100644
--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -598,7 +598,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) {
@@ -607,8 +607,15 @@ 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)) {
+		if (!boot_cpu_has(X86_FEATURE_SSBD_AMD_LS_CFG))
+			setup_force_cpu_cap(X86_FEATURE_SSBD_AMD_LS_CFG);
+		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 08e6784c4c..62e6519d93 100644
--- a/xen/arch/x86/spec_ctrl.c
+++ b/xen/arch/x86/spec_ctrl.c
@@ -50,6 +50,8 @@ bool __initdata bsp_delay_spec_ctrl;
 uint8_t __read_mostly default_xen_spec_ctrl;
 uint8_t __read_mostly default_spec_ctrl_flags;
 
+uint64_t __read_mostly ssbd_amd_ls_cfg_mask = 0ull;
+
 static int __init parse_bti(const char *s)
 {
     const char *ss;
@@ -210,10 +212,11 @@ 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\n",
+    printk("  Hardware features:%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_SSBD))  ? " SSBD"      : "",
+           boot_cpu_has(X86_FEATURE_SSBD_AMD_LS_CFG)? " SSBD"      : "",
            (e8b  & cpufeat_mask(X86_FEATURE_IBPB))  ? " IBPB"      : "",
            (caps & ARCH_CAPABILITIES_IBRS_ALL)      ? " IBRS_ALL"  : "",
            (caps & ARCH_CAPABILITIES_RDCL_NO)       ? " RDCL_NO"   : "",
@@ -225,7 +228,7 @@ static void __init print_details(enum ind_thunk thunk, uint64_t caps)
         printk("  Compiled-in support: INDIRECT_THUNK\n");
 
     /* Settings for Xen's protection, irrespective of guests. */
-    printk("  Xen settings: BTI-Thunk %s, SPEC_CTRL: %s%s, Other:%s\n",
+    printk("  Xen settings: BTI-Thunk %s, SPEC_CTRL: %s%s%s, Other:%s\n",
            thunk == THUNK_NONE      ? "N/A" :
            thunk == THUNK_RETPOLINE ? "RETPOLINE" :
            thunk == THUNK_LFENCE    ? "LFENCE" :
@@ -234,6 +237,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-",
+           !boot_cpu_has(X86_FEATURE_SSBD_AMD_LS_CFG)? "" :
+           (opt_ssbd && ssbd_amd_ls_cfg_mask)        ? " SSBD+" : " SSBD-",
            opt_ibpb                                  ? " IBPB"  : "");
 
     /*
diff --git a/xen/include/asm-x86/cpufeatures.h b/xen/include/asm-x86/cpufeatures.h
index b90aa2d046..9383d4058b 100644
--- a/xen/include/asm-x86/cpufeatures.h
+++ b/xen/include/asm-x86/cpufeatures.h
@@ -32,3 +32,4 @@ XEN_CPUFEATURE(SC_RSB_PV,       (FSCAPINTS+0)*32+18) /* RSB overwrite needed for
 XEN_CPUFEATURE(SC_RSB_HVM,      (FSCAPINTS+0)*32+19) /* RSB overwrite needed for HVM */
 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(SSBD_AMD_LS_CFG, (FSCAPINTS+0)*32+22) /* 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 5b40afbab0..6aebfa9e4f 100644
--- a/xen/include/asm-x86/spec_ctrl.h
+++ b/xen/include/asm-x86/spec_ctrl.h
@@ -38,6 +38,8 @@ extern uint8_t opt_xpti;
 #define OPT_XPTI_DOM0  0x01
 #define OPT_XPTI_DOMU  0x02
 
+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 v2 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          |   7 +-
 xen/arch/x86/smpboot.c          |   3 +
 xen/arch/x86/spec_ctrl.c        | 174 +++++++++++++++++++++++++++++++++++++++-
 xen/include/asm-x86/spec_ctrl.h |   2 +
 4 files changed, 178 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index 06c9e9661b..6a5fbcae22 100644
--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -611,14 +611,9 @@ static void init_amd(struct cpuinfo_x86 *c)
 			ssbd_amd_ls_cfg_mask = 1ull << bit;
 	}
 
-	if (ssbd_amd_ls_cfg_mask && !rdmsr_safe(MSR_AMD64_LS_CFG, value)) {
+	if (ssbd_amd_ls_cfg_mask && !rdmsr_safe(MSR_AMD64_LS_CFG, value))
 		if (!boot_cpu_has(X86_FEATURE_SSBD_AMD_LS_CFG))
 			setup_force_cpu_cap(X86_FEATURE_SSBD_AMD_LS_CFG);
-		if (opt_ssbd) {
-			value |= ssbd_amd_ls_cfg_mask;
-			wrmsr_safe(MSR_AMD64_LS_CFG, value);
-		}
-	}
 
 	/* MFENCE stops RDTSC speculation */
 	if (!cpu_has_lfence_dispatch)
diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index d4478e6132..af881ba30a 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -366,6 +366,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 62e6519d93..ff14b8f985 100644
--- a/xen/arch/x86/spec_ctrl.c
+++ b/xen/arch/x86/spec_ctrl.c
@@ -19,6 +19,7 @@
 #include <xen/errno.h>
 #include <xen/init.h>
 #include <xen/lib.h>
+#include <xen/spinlock.h>
 
 #include <asm/microcode.h>
 #include <asm/msr.h>
@@ -50,7 +51,16 @@ bool __initdata bsp_delay_spec_ctrl;
 uint8_t __read_mostly default_xen_spec_ctrl;
 uint8_t __read_mostly default_spec_ctrl_flags;
 
+/* for SSBD support for AMD via LS_CFG */
+#define SSBD_AMD_MAX_SOCKET 2
+struct ssbd_amd_ls_cfg_smt_status {
+    spinlock_t lock;
+    uint32_t mask;
+} __attribute__ ((aligned (64)));
+bool __read_mostly ssbd_amd_smt_en = false;
+bool __read_mostly default_xen_ssbd_amd_ls_cfg_en = false;
 uint64_t __read_mostly ssbd_amd_ls_cfg_mask = 0ull;
+struct ssbd_amd_ls_cfg_smt_status *ssbd_amd_smt_status[SSBD_AMD_MAX_SOCKET] = {NULL};
 
 static int __init parse_bti(const char *s)
 {
@@ -237,8 +247,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-",
-           !boot_cpu_has(X86_FEATURE_SSBD_AMD_LS_CFG)? "" :
-           (opt_ssbd && ssbd_amd_ls_cfg_mask)        ? " SSBD+" : " SSBD-",
+           !boot_cpu_has(X86_FEATURE_SSBD_AMD_LS_CFG) ? "" :
+           default_xen_ssbd_amd_ls_cfg_en            ? " SSBD+" : " SSBD-",
            opt_ibpb                                  ? " IBPB"  : "");
 
     /*
@@ -487,6 +497,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)
+{
+    uint32_t 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 ( !ssbd_amd_ls_cfg_mask ||
+         !boot_cpu_has(X86_FEATURE_SSBD_AMD_LS_CFG) ) {
+        dprintk(XENLOG_ERR, "SSBD AMD LS CFG: invalid mask or missing feature\n");
+        return;
+    }
+
+    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)
+{
+    uint32_t cores_per_socket, threads_per_core;
+    const struct cpuinfo_x86  *c =  &boot_cpu_data;
+    uint32_t core, socket;
+
+    if ( !ssbd_amd_ls_cfg_mask ||
+         !boot_cpu_has(X86_FEATURE_SSBD_AMD_LS_CFG) )
+        goto ssbd_amd_ls_cfg_init_fail;
+
+    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 < SSBD_AMD_MAX_SOCKET; 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 < SSBD_AMD_MAX_SOCKET; 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;
+    }
+
+    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++ )
+        if ( ssbd_amd_smt_status[socket] != NULL )
+           xfree(ssbd_amd_smt_status[socket]);
+
+    setup_clear_cpu_cap(X86_FEATURE_SSBD_AMD_LS_CFG);
+    default_xen_ssbd_amd_ls_cfg_en = false;
+
+    dprintk(XENLOG_ERR, "SSBD AMD LS CFG: disalbing SSBD due to errors\n");
+
+    return 1;
+}
+presmp_initcall(ssbd_amd_ls_cfg_init);
+
 void __init init_speculation_mitigations(void)
 {
     enum ind_thunk thunk = THUNK_DEFAULT;
@@ -589,6 +755,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 ( boot_cpu_has(X86_FEATURE_SSBD_AMD_LS_CFG) && 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/spec_ctrl.h b/xen/include/asm-x86/spec_ctrl.h
index 6aebfa9e4f..e48c76ac4f 100644
--- a/xen/include/asm-x86/spec_ctrl.h
+++ b/xen/include/asm-x86/spec_ctrl.h
@@ -39,6 +39,8 @@ extern uint8_t opt_xpti;
 #define OPT_XPTI_DOMU  0x02
 
 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 v2 2/2] x86/spec-ctrl: add support for modifying SSBD VIA LS_CFG MSR

[Brian Woods]

On Thu, Aug 09, 2018 at 02:42:13PM -0500, Brian Woods wrote:
> @@ -237,8 +247,8 @@ static void __init print_details(enum ind_thunk thunk, uint64_t caps)
> -           !boot_cpu_has(X86_FEATURE_SSBD_AMD_LS_CFG)? "" :
> -           (opt_ssbd && ssbd_amd_ls_cfg_mask)        ? " SSBD+" : " SSBD-",
> +           !boot_cpu_has(X86_FEATURE_SSBD_AMD_LS_CFG) ? "" :
> +           default_xen_ssbd_amd_ls_cfg_en            ? " SSBD+" : " SSBD-",

This will change in the next version.  I just noticed I only changed it
in patch 2 and not 1.  Sorry about that.

-- 
Brian Woods

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

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

[Jan Beulich]

>>> On 09.08.18 at 21:42, <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>
> ---

Please have a brief revision log here.

> --- a/xen/arch/x86/cpu/amd.c
> +++ b/xen/arch/x86/cpu/amd.c
> @@ -598,7 +598,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) {
> @@ -607,8 +607,15 @@ 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;
> +	}

I think you want to truly restrict this code to only run on the BSP.
Keying it to !ssbd_amd_ls_cfg_mask will lead to it getting re-run
on APs if the BSP didn't set a non-zero value.

> +	if (ssbd_amd_ls_cfg_mask && !rdmsr_safe(MSR_AMD64_LS_CFG, value)) {
> +		if (!boot_cpu_has(X86_FEATURE_SSBD_AMD_LS_CFG))
> +			setup_force_cpu_cap(X86_FEATURE_SSBD_AMD_LS_CFG);

Why the if()?

> +		if (opt_ssbd) {
> +			value |= ssbd_amd_ls_cfg_mask;
>  			wrmsr_safe(MSR_AMD64_LS_CFG, value);
>  		}
>  	}

Wouldn't you better set ssbd_amd_ls_cfg_mask to zero again if
the rdmsr_safe() failed (unexpectedly)?

> --- a/xen/arch/x86/spec_ctrl.c
> +++ b/xen/arch/x86/spec_ctrl.c
> @@ -50,6 +50,8 @@ bool __initdata bsp_delay_spec_ctrl;
>  uint8_t __read_mostly default_xen_spec_ctrl;
>  uint8_t __read_mostly default_spec_ctrl_flags;
>  
> +uint64_t __read_mostly ssbd_amd_ls_cfg_mask = 0ull;

I think I had pointed out already that the initializer is pointless.
See the other variables visible in context.

> @@ -210,10 +212,11 @@ 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\n",
> +    printk("  Hardware features:%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_SSBD))  ? " SSBD"      : "",
> +           boot_cpu_has(X86_FEATURE_SSBD_AMD_LS_CFG)? " SSBD"      : "",

I'm still not really happy about the double " SSBD" string literals
(and also the redundant ones further down), but I'll let Andrew
break ties here. I am however sure I had already pointed out
that there's a blank missing ahead of the ? in any event (and
also again further down).

> --- a/xen/include/asm-x86/cpufeatures.h
> +++ b/xen/include/asm-x86/cpufeatures.h
> @@ -32,3 +32,4 @@ XEN_CPUFEATURE(SC_RSB_PV,       (FSCAPINTS+0)*32+18) /* RSB overwrite needed for
>  XEN_CPUFEATURE(SC_RSB_HVM,      (FSCAPINTS+0)*32+19) /* RSB overwrite needed for HVM */
>  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(SSBD_AMD_LS_CFG, (FSCAPINTS+0)*32+22) /* if SSBD support is enabled via LS_CGF MSR on AMD hardware */

As also said before - please no synthetic features unless there's
going to be a use for alternatives patching. A simple boolean
variable is quite sufficient for all other cases.

Jan



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

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

[Jan Beulich]

>>> On 09.08.18 at 21:42, <brian.woods@amd.com> wrote:
> --- a/xen/arch/x86/cpu/amd.c
> +++ b/xen/arch/x86/cpu/amd.c
> @@ -611,14 +611,9 @@ static void init_amd(struct cpuinfo_x86 *c)
>  			ssbd_amd_ls_cfg_mask = 1ull << bit;
>  	}
>  
> -	if (ssbd_amd_ls_cfg_mask && !rdmsr_safe(MSR_AMD64_LS_CFG, value)) {
> +	if (ssbd_amd_ls_cfg_mask && !rdmsr_safe(MSR_AMD64_LS_CFG, value))
>  		if (!boot_cpu_has(X86_FEATURE_SSBD_AMD_LS_CFG))
>  			setup_force_cpu_cap(X86_FEATURE_SSBD_AMD_LS_CFG);

If the inner if() was not to go away altogether in patch 1, please
fold two successive if()-s like these.

> --- a/xen/arch/x86/spec_ctrl.c
> +++ b/xen/arch/x86/spec_ctrl.c

First of all - I'm not convinced some of the AMD specific code here
wouldn't better live in cpu/amd.c.

> @@ -50,7 +51,16 @@ bool __initdata bsp_delay_spec_ctrl;
>  uint8_t __read_mostly default_xen_spec_ctrl;
>  uint8_t __read_mostly default_spec_ctrl_flags;
>  
> +/* for SSBD support for AMD via LS_CFG */
> +#define SSBD_AMD_MAX_SOCKET 2
> +struct ssbd_amd_ls_cfg_smt_status {
> +    spinlock_t lock;
> +    uint32_t mask;
> +} __attribute__ ((aligned (64)));

Where's this literal 64 coming from? Do you perhaps mean
SMP_CACHE_BYTES? And if this is really needed (as said before,
I think the array would better be dynamically allocated than having
compile time determined fixed size), then please don't open-code
__aligned().

> +bool __read_mostly ssbd_amd_smt_en = false;
> +bool __read_mostly default_xen_ssbd_amd_ls_cfg_en = false;
>  uint64_t __read_mostly ssbd_amd_ls_cfg_mask = 0ull;
> +struct ssbd_amd_ls_cfg_smt_status *ssbd_amd_smt_status[SSBD_AMD_MAX_SOCKET] = {NULL};

Several further pointless initializers.

> +static void ssbd_amd_ls_cfg_set_smt(bool enable_ssbd)
> +{
> +    uint32_t 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) )

So with ->mask being uint32_t, why does enable_mask need to be
uint64_t? Even uint32_t seems way more than needed, but there's
certainly no point going below this. Just that, as expressed before,
you should please use "unsigned int" in favor of uint32_t everywhere,
unless you really need something that's exactly 32-bits wide.

> +        {
> +            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) )

Please prefer the shorter ! over " == 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 ( !ssbd_amd_ls_cfg_mask ||
> +         !boot_cpu_has(X86_FEATURE_SSBD_AMD_LS_CFG) ) {
> +        dprintk(XENLOG_ERR, "SSBD AMD LS CFG: invalid mask or missing feature\n");

If the plan is for the function to also be called at runtime eventually,
this dprintk() needs to go away.

> +        return;
> +    }
> +
> +    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)
> +{
> +    uint32_t cores_per_socket, threads_per_core;
> +    const struct cpuinfo_x86  *c =  &boot_cpu_data;
> +    uint32_t core, socket;
> +
> +    if ( !ssbd_amd_ls_cfg_mask ||
> +         !boot_cpu_has(X86_FEATURE_SSBD_AMD_LS_CFG) )
> +        goto ssbd_amd_ls_cfg_init_fail;

Why not simply "return"?

> +    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 < SSBD_AMD_MAX_SOCKET; 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);

Pointless cast.

> +                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 < SSBD_AMD_MAX_SOCKET; 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;
> +    }
> +
> +    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++ )
> +        if ( ssbd_amd_smt_status[socket] != NULL )
> +           xfree(ssbd_amd_smt_status[socket]);

There's no need for the if() here.

> +    setup_clear_cpu_cap(X86_FEATURE_SSBD_AMD_LS_CFG);

The same feature must not first be forced, and the cleared. Please
take a look at the implementation of the functions.

> +    default_xen_ssbd_amd_ls_cfg_en = false;
> +
> +    dprintk(XENLOG_ERR, "SSBD AMD LS CFG: disalbing SSBD due to errors\n");
> +
> +    return 1;

If the function returns 0 and 1 only, it looks like you've meant to
use bool, false, and true respectively.

Jan


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

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

[Brian Woods]

On Wed, Aug 15, 2018 at 10:00:48AM -0600, Jan Beulich wrote:
> >>> On 09.08.18 at 21:42, <brian.woods@amd.com> wrote:
> > --- a/xen/arch/x86/cpu/amd.c
> > +++ b/xen/arch/x86/cpu/amd.c
> > @@ -611,14 +611,9 @@ static void init_amd(struct cpuinfo_x86 *c)
> >  			ssbd_amd_ls_cfg_mask = 1ull << bit;
> >  	}
> >  
> > -	if (ssbd_amd_ls_cfg_mask && !rdmsr_safe(MSR_AMD64_LS_CFG, value)) {
> > +	if (ssbd_amd_ls_cfg_mask && !rdmsr_safe(MSR_AMD64_LS_CFG, value))
> >  		if (!boot_cpu_has(X86_FEATURE_SSBD_AMD_LS_CFG))
> >  			setup_force_cpu_cap(X86_FEATURE_SSBD_AMD_LS_CFG);
> 
> If the inner if() was not to go away altogether in patch 1, please
> fold two successive if()-s like these.
> 
> > --- a/xen/arch/x86/spec_ctrl.c
> > +++ b/xen/arch/x86/spec_ctrl.c
> 
> First of all - I'm not convinced some of the AMD specific code here
> wouldn't better live in cpu/amd.c.

Well, by that logic, a lot of the other logic could go in cpu/intel.c.
It has to do with SSBD and when AMD's processors support it via the
SPEC_CTRL MSR, the support for SSBD will get merged together in
spec_ctrl.c and if that's the case, it makes sense to have all the SSBD
code together. IMO

> > @@ -50,7 +51,16 @@ bool __initdata bsp_delay_spec_ctrl;
> >  uint8_t __read_mostly default_xen_spec_ctrl;
> >  uint8_t __read_mostly default_spec_ctrl_flags;
> >  
> > +/* for SSBD support for AMD via LS_CFG */
> > +#define SSBD_AMD_MAX_SOCKET 2
> > +struct ssbd_amd_ls_cfg_smt_status {
> > +    spinlock_t lock;
> > +    uint32_t mask;
> > +} __attribute__ ((aligned (64)));
> 
> Where's this literal 64 coming from? Do you perhaps mean
> SMP_CACHE_BYTES? And if this is really needed (as said before,
> I think the array would better be dynamically allocated than having
> compile time determined fixed size), then please don't open-code
> __aligned().

It's the cache coherency size.  The SMP_CACHE_BYTES is 128 bytes IIRC.
I was trying to save space since the struct is so small it would double
the size.  That can be changed though.

> > +bool __read_mostly ssbd_amd_smt_en = false;
> > +bool __read_mostly default_xen_ssbd_amd_ls_cfg_en = false;
> >  uint64_t __read_mostly ssbd_amd_ls_cfg_mask = 0ull;
> > +struct ssbd_amd_ls_cfg_smt_status *ssbd_amd_smt_status[SSBD_AMD_MAX_SOCKET] = {NULL};
> 
> Several further pointless initializers.

ssbd_amd_ls_cfg_mask -> needs to be initialized, due to how it gets set
ssbd_amd_ls_cfg_smt_status -> needs to be initialized, unless xalloc
                              sets it as NULL on failure to alloc
default_xen_ssbd_amd_ls_cfg_en -> needs to be initialized of an else
                                  added to an if statement
ssbd_amd_smt_en -> like the above

If you want default_xen_ssbd_amd_ls_cfg_en and ssbd_amd_smt_en to be
not be initialized, I can add code to do that.

> > +static void ssbd_amd_ls_cfg_set_smt(bool enable_ssbd)
> > +{
> > +    uint32_t 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) )
> 
> So with ->mask being uint32_t, why does enable_mask need to be
> uint64_t? Even uint32_t seems way more than needed, but there's
> certainly no point going below this. Just that, as expressed before,
> you should please use "unsigned int" in favor of uint32_t everywhere,
> unless you really need something that's exactly 32-bits wide.

Because when changing the variable types from __32 etc, I confused it
with the enable mask for the LS_CFG reg.  I'll change them.

> > +        {
> > +            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) )
> 
> Please prefer the shorter ! over " == 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 ( !ssbd_amd_ls_cfg_mask ||
> > +         !boot_cpu_has(X86_FEATURE_SSBD_AMD_LS_CFG) ) {
> > +        dprintk(XENLOG_ERR, "SSBD AMD LS CFG: invalid mask or missing feature\n");
> 
> If the plan is for the function to also be called at runtime eventually,
> this dprintk() needs to go away.
> 
> > +        return;
> > +    }
> > +
> > +    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)
> > +{
> > +    uint32_t cores_per_socket, threads_per_core;
> > +    const struct cpuinfo_x86  *c =  &boot_cpu_data;
> > +    uint32_t core, socket;
> > +
> > +    if ( !ssbd_amd_ls_cfg_mask ||
> > +         !boot_cpu_has(X86_FEATURE_SSBD_AMD_LS_CFG) )
> > +        goto ssbd_amd_ls_cfg_init_fail;
> 
> Why not simply "return"?

Because it forces all the failed returns down one code path.  I can
change it if you wish.

> > +    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 < SSBD_AMD_MAX_SOCKET; 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);
> 
> Pointless cast.
> 
> > +                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 < SSBD_AMD_MAX_SOCKET; 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;
> > +    }
> > +
> > +    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++ )
> > +        if ( ssbd_amd_smt_status[socket] != NULL )
> > +           xfree(ssbd_amd_smt_status[socket]);
> 
> There's no need for the if() here.
> 
> > +    setup_clear_cpu_cap(X86_FEATURE_SSBD_AMD_LS_CFG);
> 
> The same feature must not first be forced, and the cleared. Please
> take a look at the implementation of the functions.

Will do.

> > +    default_xen_ssbd_amd_ls_cfg_en = false;
> > +
> > +    dprintk(XENLOG_ERR, "SSBD AMD LS CFG: disalbing SSBD due to errors\n");
> > +
> > +    return 1;
> 
> If the function returns 0 and 1 only, it looks like you've meant to
> use bool, false, and true respectively.
> 
> Jan
> 

Because it's more of an error code than boolean logic value.  I can
switch it over to bool if you want.

Noted about the things I didn't directly reply to.

-- 
Brian Woods

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

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

[Jan Beulich]

>>> On 16.08.18 at 22:02, <brian.woods@amd.com> wrote:
> On Wed, Aug 15, 2018 at 10:00:48AM -0600, Jan Beulich wrote:
>> >>> On 09.08.18 at 21:42, <brian.woods@amd.com> wrote:
>> > --- a/xen/arch/x86/spec_ctrl.c
>> > +++ b/xen/arch/x86/spec_ctrl.c
>> 
>> First of all - I'm not convinced some of the AMD specific code here
>> wouldn't better live in cpu/amd.c.
> 
> Well, by that logic, a lot of the other logic could go in cpu/intel.c.
> It has to do with SSBD and when AMD's processors support it via the
> SPEC_CTRL MSR, the support for SSBD will get merged together in
> spec_ctrl.c and if that's the case, it makes sense to have all the SSBD
> code together. IMO

Maybe, though I'd say retpoline_safe(), should_use_eager_fpu(),
l1tf_calculations(), and xpti_init_default() are all written in a way
that they could be extended to other CPU vendors should it
become known that they're also affected. I don't think we really
know for sure whether VIA and/or Shanghai are affected. Otoh
the code you add is not just AMD-specific, but even model-specific
within AMD's palette.

I'd be curious to know whether Andrew has an opinion here.

>> > @@ -50,7 +51,16 @@ bool __initdata bsp_delay_spec_ctrl;
>> >  uint8_t __read_mostly default_xen_spec_ctrl;
>> >  uint8_t __read_mostly default_spec_ctrl_flags;
>> >  
>> > +/* for SSBD support for AMD via LS_CFG */
>> > +#define SSBD_AMD_MAX_SOCKET 2
>> > +struct ssbd_amd_ls_cfg_smt_status {
>> > +    spinlock_t lock;
>> > +    uint32_t mask;
>> > +} __attribute__ ((aligned (64)));
>> 
>> Where's this literal 64 coming from? Do you perhaps mean
>> SMP_CACHE_BYTES? And if this is really needed (as said before,
>> I think the array would better be dynamically allocated than having
>> compile time determined fixed size), then please don't open-code
>> __aligned().
> 
> It's the cache coherency size.  The SMP_CACHE_BYTES is 128 bytes IIRC.
> I was trying to save space since the struct is so small it would double
> the size.  That can be changed though.

If SMP_CACHE_BYTES doesn't fit your need, the literal number used
needs either an explaining comment or a suitably named #define.

>> > +bool __read_mostly ssbd_amd_smt_en = false;
>> > +bool __read_mostly default_xen_ssbd_amd_ls_cfg_en = false;
>> >  uint64_t __read_mostly ssbd_amd_ls_cfg_mask = 0ull;
>> > +struct ssbd_amd_ls_cfg_smt_status *ssbd_amd_smt_status[SSBD_AMD_MAX_SOCKET] = {NULL};
>> 
>> Several further pointless initializers.
> 
> ssbd_amd_ls_cfg_mask -> needs to be initialized, due to how it gets set
> ssbd_amd_ls_cfg_smt_status -> needs to be initialized, unless xalloc
>                               sets it as NULL on failure to alloc
> default_xen_ssbd_amd_ls_cfg_en -> needs to be initialized of an else
>                                   added to an if statement
> ssbd_amd_smt_en -> like the above
> 
> If you want default_xen_ssbd_amd_ls_cfg_en and ssbd_amd_smt_en to be
> not be initialized, I can add code to do that.

I don't understand: Add code? The initializers here are all pointless
because the values you supply are what the variables would be
initialized with anyway if you omitted the initializers. That's what
the C standard specifies.

>> > +static int __init ssbd_amd_ls_cfg_init(void)
>> > +{
>> > +    uint32_t cores_per_socket, threads_per_core;
>> > +    const struct cpuinfo_x86  *c =  &boot_cpu_data;
>> > +    uint32_t core, socket;
>> > +
>> > +    if ( !ssbd_amd_ls_cfg_mask ||
>> > +         !boot_cpu_has(X86_FEATURE_SSBD_AMD_LS_CFG) )
>> > +        goto ssbd_amd_ls_cfg_init_fail;
>> 
>> Why not simply "return"?
> 
> Because it forces all the failed returns down one code path.  I can
> change it if you wish.

If any cleanup is to be done, using "goto" for this purpose is
generally accepted (although personally I dislike "goto" in
general). Here, however, nothing has been done yet and the
cleanup path is non-trivial. It took me extra time to verify
that nothing bad would happen from going that path despite
nothing having been done before. It is far more clear to the
reader imo if there is just "return" here.

>> > +    default_xen_ssbd_amd_ls_cfg_en = false;
>> > +
>> > +    dprintk(XENLOG_ERR, "SSBD AMD LS CFG: disalbing SSBD due to errors\n");
>> > +
>> > +    return 1;
>> 
>> If the function returns 0 and 1 only, it looks like you've meant to
>> use bool, false, and true respectively.
> 
> Because it's more of an error code than boolean logic value.  I can
> switch it over to bool if you want.

Error code style returns would please use (negative) errno-style
numbers. But if the function really just means to say "success"
or "failure", without particular error codes, then boolean would
imo still be preferable.

Jan


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

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

[Brian Woods]

On Fri, Aug 17, 2018 at 12:59:28AM -0600, Jan Beulich wrote:
> >>> On 16.08.18 at 22:02, <brian.woods@amd.com> wrote:
> > On Wed, Aug 15, 2018 at 10:00:48AM -0600, Jan Beulich wrote:
> >> >>> On 09.08.18 at 21:42, <brian.woods@amd.com> wrote:
> >> > --- a/xen/arch/x86/spec_ctrl.c
> >> > +++ b/xen/arch/x86/spec_ctrl.c
> >> 
> >> First of all - I'm not convinced some of the AMD specific code here
> >> wouldn't better live in cpu/amd.c.
> > 
> > Well, by that logic, a lot of the other logic could go in cpu/intel.c.
> > It has to do with SSBD and when AMD's processors support it via the
> > SPEC_CTRL MSR, the support for SSBD will get merged together in
> > spec_ctrl.c and if that's the case, it makes sense to have all the SSBD
> > code together. IMO
> 
> Maybe, though I'd say retpoline_safe(), should_use_eager_fpu(),
> l1tf_calculations(), and xpti_init_default() are all written in a way
> that they could be extended to other CPU vendors should it
> become known that they're also affected. I don't think we really
> know for sure whether VIA and/or Shanghai are affected. Otoh
> the code you add is not just AMD-specific, but even model-specific
> within AMD's palette.
> 
> I'd be curious to know whether Andrew has an opinion here.

Since most of the spec_ctrl code is his, his opinion here would be the
most important.

> >> > @@ -50,7 +51,16 @@ bool __initdata bsp_delay_spec_ctrl;
> >> >  uint8_t __read_mostly default_xen_spec_ctrl;
> >> >  uint8_t __read_mostly default_spec_ctrl_flags;
> >> >  
> >> > +/* for SSBD support for AMD via LS_CFG */
> >> > +#define SSBD_AMD_MAX_SOCKET 2
> >> > +struct ssbd_amd_ls_cfg_smt_status {
> >> > +    spinlock_t lock;
> >> > +    uint32_t mask;
> >> > +} __attribute__ ((aligned (64)));
> >> 
> >> Where's this literal 64 coming from? Do you perhaps mean
> >> SMP_CACHE_BYTES? And if this is really needed (as said before,
> >> I think the array would better be dynamically allocated than having
> >> compile time determined fixed size), then please don't open-code
> >> __aligned().
> > 
> > It's the cache coherency size.  The SMP_CACHE_BYTES is 128 bytes IIRC.
> > I was trying to save space since the struct is so small it would double
> > the size.  That can be changed though.
> 
> If SMP_CACHE_BYTES doesn't fit your need, the literal number used
> needs either an explaining comment or a suitably named #define.

I'll just use SMP_CACHE_BYTES and not worry about the extra space.

> >> > +bool __read_mostly ssbd_amd_smt_en = false;
> >> > +bool __read_mostly default_xen_ssbd_amd_ls_cfg_en = false;
> >> >  uint64_t __read_mostly ssbd_amd_ls_cfg_mask = 0ull;
> >> > +struct ssbd_amd_ls_cfg_smt_status *ssbd_amd_smt_status[SSBD_AMD_MAX_SOCKET] = {NULL};
> >> 
> >> Several further pointless initializers.
> > 
> > ssbd_amd_ls_cfg_mask -> needs to be initialized, due to how it gets set
> > ssbd_amd_ls_cfg_smt_status -> needs to be initialized, unless xalloc
> >                               sets it as NULL on failure to alloc
> > default_xen_ssbd_amd_ls_cfg_en -> needs to be initialized of an else
> >                                   added to an if statement
> > ssbd_amd_smt_en -> like the above
> > 
> > If you want default_xen_ssbd_amd_ls_cfg_en and ssbd_amd_smt_en to be
> > not be initialized, I can add code to do that.
> 
> I don't understand: Add code? The initializers here are all pointless
> because the values you supply are what the variables would be
> initialized with anyway if you omitted the initializers. That's what
> the C standard specifies.

Leaving values which determine the behavior of the hypervisor to
defaults of the compiler's implementation of the standard seems like it
would be a possible source of subtle bugs when the compiler doesn't do
something just right.  I'd much rather have an initialized value or
have it set in the code before use.

If you strongly feel that they shouldn't be initialized or set in code,
I'll change them though.

> >> > +static int __init ssbd_amd_ls_cfg_init(void)
> >> > +{
> >> > +    uint32_t cores_per_socket, threads_per_core;
> >> > +    const struct cpuinfo_x86  *c =  &boot_cpu_data;
> >> > +    uint32_t core, socket;
> >> > +
> >> > +    if ( !ssbd_amd_ls_cfg_mask ||
> >> > +         !boot_cpu_has(X86_FEATURE_SSBD_AMD_LS_CFG) )
> >> > +        goto ssbd_amd_ls_cfg_init_fail;
> >> 
> >> Why not simply "return"?
> > 
> > Because it forces all the failed returns down one code path.  I can
> > change it if you wish.
> 
> If any cleanup is to be done, using "goto" for this purpose is
> generally accepted (although personally I dislike "goto" in
> general). Here, however, nothing has been done yet and the
> cleanup path is non-trivial. It took me extra time to verify
> that nothing bad would happen from going that path despite
> nothing having been done before. It is far more clear to the
> reader imo if there is just "return" here.

Well, it's just a difference of opinion at this point.  I'll change it.

> >> > +    default_xen_ssbd_amd_ls_cfg_en = false;
> >> > +
> >> > +    dprintk(XENLOG_ERR, "SSBD AMD LS CFG: disalbing SSBD due to errors\n");
> >> > +
> >> > +    return 1;
> >> 
> >> If the function returns 0 and 1 only, it looks like you've meant to
> >> use bool, false, and true respectively.
> > 
> > Because it's more of an error code than boolean logic value.  I can
> > switch it over to bool if you want.
> 
> Error code style returns would please use (negative) errno-style
> numbers. But if the function really just means to say "success"
> or "failure", without particular error codes, then boolean would
> imo still be preferable.
> 
> Jan
> 

Sounds fair to change it to bool.  I'll do that.

-- 
Brian Woods

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

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

[Jan Beulich]

>>> On 17.08.18 at 20:45, <brian.woods@amd.com> wrote:
> On Fri, Aug 17, 2018 at 12:59:28AM -0600, Jan Beulich wrote:
>> >>> On 16.08.18 at 22:02, <brian.woods@amd.com> wrote:
>> > On Wed, Aug 15, 2018 at 10:00:48AM -0600, Jan Beulich wrote:
>> >> > +bool __read_mostly ssbd_amd_smt_en = false;
>> >> > +bool __read_mostly default_xen_ssbd_amd_ls_cfg_en = false;
>> >> >  uint64_t __read_mostly ssbd_amd_ls_cfg_mask = 0ull;
>> >> > +struct ssbd_amd_ls_cfg_smt_status *ssbd_amd_smt_status[SSBD_AMD_MAX_SOCKET] = {NULL};
>> >> 
>> >> Several further pointless initializers.
>> > 
>> > ssbd_amd_ls_cfg_mask -> needs to be initialized, due to how it gets set
>> > ssbd_amd_ls_cfg_smt_status -> needs to be initialized, unless xalloc
>> >                               sets it as NULL on failure to alloc
>> > default_xen_ssbd_amd_ls_cfg_en -> needs to be initialized of an else
>> >                                   added to an if statement
>> > ssbd_amd_smt_en -> like the above
>> > 
>> > If you want default_xen_ssbd_amd_ls_cfg_en and ssbd_amd_smt_en to be
>> > not be initialized, I can add code to do that.
>> 
>> I don't understand: Add code? The initializers here are all pointless
>> because the values you supply are what the variables would be
>> initialized with anyway if you omitted the initializers. That's what
>> the C standard specifies.
> 
> Leaving values which determine the behavior of the hypervisor to
> defaults of the compiler's implementation of the standard seems like it
> would be a possible source of subtle bugs when the compiler doesn't do
> something just right.  I'd much rather have an initialized value or
> have it set in the code before use.
> 
> If you strongly feel that they shouldn't be initialized or set in code,
> I'll change them though.

Yes, I do feel strongly about this: We omit pointless initializers
elsewhere, and the comment I've given here is a pretty common one
to be made in reviews. If we were to write code being prepared for
compiler bugs (which standards-non-conformance is), we could as
well stop writing any code in the first place. Workarounds are
acceptable only for _known_ compiler defects.

Jan



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