[PATCH 0/3] SSBD AMD via LS CFG Enablement

[PATCH 0/3] 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 two patches are 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

Brian Woods (3):
  x86/spec-ctrl: add AMD SSBD LS_CFG in init print
  x86/spec-ctrl: Add defines and variables for AMD SSBD support
  x86/spec-ctrl: Add support for modifying SSBD AMD VIA LS_CFG MSR

 xen/arch/x86/cpu/amd.c            |   7 +-
 xen/arch/x86/setup.c              |   2 +
 xen/arch/x86/smpboot.c            |   3 +
 xen/arch/x86/spec_ctrl.c          | 218 +++++++++++++++++++++++++++++++++++++-
 xen/include/asm-x86/cpufeatures.h |   1 +
 xen/include/asm-x86/spec_ctrl.h   |   5 +
 6 files changed, 231 insertions(+), 5 deletions(-)

-- 
2.11.0


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

[PATCH 1/3] 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 2/3] x86/spec-ctrl: Add defines and variables for AMD SSBD support

[Brian Woods]

In preparation for adding switchable SSBD, add some defines and
variables.

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

diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
index 62e6519d93..baef907322 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 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)
 {
diff --git a/xen/include/asm-x86/spec_ctrl.h b/xen/include/asm-x86/spec_ctrl.h
index 6aebfa9e4f..c47647771a 100644
--- a/xen/include/asm-x86/spec_ctrl.h
+++ b/xen/include/asm-x86/spec_ctrl.h
@@ -39,6 +39,9 @@ extern uint8_t opt_xpti;
 #define OPT_XPTI_DOMU  0x02
 
 extern uint64_t ssbd_amd_ls_cfg_mask;
+extern bool xen_ssbd_amd_ls_cfg_en;
+extern void ssbd_amd_ls_cfg_init(void);
+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

[PATCH 3/3] x86/spec-ctrl: Add support for modifying SSBD AMD 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   |  10 +--
 xen/arch/x86/setup.c     |   2 +
 xen/arch/x86/smpboot.c   |   3 +
 xen/arch/x86/spec_ctrl.c | 201 ++++++++++++++++++++++++++++++++++++++++++++++-
 4 files changed, 207 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index 06c9e9661b..e7ec0d99a7 100644
--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -607,16 +607,10 @@ static void init_amd(struct cpuinfo_x86 *c)
 		case 0x17: bit = 10; break;
 		}
 
-		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))
+		if (bit >= 0 && !rdmsr_safe(MSR_AMD64_LS_CFG, value)) {
+			ssbd_amd_ls_cfg_mask = 1ull << bit;
 			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/setup.c b/xen/arch/x86/setup.c
index 419b46c033..b551852cbd 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1579,6 +1579,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
 
     arch_init_memory();
 
+    ssbd_amd_ls_cfg_init();
+
     alternative_instructions();
 
     local_irq_enable();
diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index d4478e6132..07760c920d 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 ( 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 baef907322..006e8fb14b 100644
--- a/xen/arch/x86/spec_ctrl.c
+++ b/xen/arch/x86/spec_ctrl.c
@@ -248,7 +248,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-",
            !boot_cpu_has(X86_FEATURE_SSBD_AMD_LS_CFG)? "" :
-           (opt_ssbd && ssbd_amd_ls_cfg_mask)        ? " SSBD+" : " SSBD-",
+           xen_ssbd_amd_ls_cfg_en                    ? " SSBD+" : " SSBD-",
            opt_ibpb                                  ? " IBPB"  : "");
 
     /*
@@ -497,6 +497,201 @@ static __init int parse_xpti(const char *s)
 }
 custom_param("xpti", parse_xpti);
 
+/*
+ * For family 15h and 16h, there are no SMT enabled processors, so there
+ * is no need for locking, just need to set an MSR bit.   For 17h, it
+ * depends if SMT is enabled.  If SMT, are two threads share a single
+ * MSR so there needs to be a lock and a virtual bit for each thread.
+ */
+
+/* used for non SMT mitigations (no shared MSRs) */
+static void ssbd_amd_ls_cfg_set_nonsmt(bool enable_ssbd)
+{
+    unsigned long ls_cfg;
+
+    if ( enable_ssbd )
+    {
+        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
+    {
+        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);
+        }
+    }
+}
+
+/* used for family 17h with SMT enabled (shared MSRs) */
+static void ssbd_amd_ls_cfg_set_smt(bool enable_ssbd)
+{
+    __u32 socket, core, thread;
+    uint64_t enable_mask;
+    unsigned long ls_cfg;
+    struct ssbd_amd_ls_cfg_smt_status *status;
+    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\n");
+        return;
+    }
+
+    if ( ssbd_amd_smt_en )
+        ssbd_amd_ls_cfg_set_smt(enable_ssbd);
+    else
+        ssbd_amd_ls_cfg_set_nonsmt(enable_ssbd);
+}
+
+/*
+ * used to init the boot cpu, we don't need to lock anything because
+ * it's just the boot CPU
+ */
+static __init void ssbd_amd_ls_cfg_set_init(void)
+{
+    __u32 socket, core, thread;
+    unsigned long long ls_cfg;
+    struct ssbd_amd_ls_cfg_smt_status *status;
+    struct cpuinfo_x86  *c =  &boot_cpu_data;
+
+    if ( ssbd_amd_smt_en )
+    {
+        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;
+        status->mask |= (1ull << thread);
+    }
+
+    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);
+    }
+}
+
+__init void ssbd_amd_ls_cfg_init(void)
+{
+    int cores_per_socket, threads_per_core;
+    struct cpuinfo_x86  *c =  &boot_cpu_data;
+    int 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 ( (cores_per_socket < 1) || (threads_per_core < 1) )
+        {
+            dprintk(XENLOG_ERR,
+                    "SSBD AMD LS CFG: error in topology decoding\n");
+            goto ssbd_amd_ls_cfg_init_fail;
+        }
+
+        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 ( xen_ssbd_amd_ls_cfg_en )
+        ssbd_amd_ls_cfg_set_init();
+
+    return;
+
+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);
+    xen_ssbd_amd_ls_cfg_en = false;
+
+    return;
+}
+
 void __init init_speculation_mitigations(void)
 {
     enum ind_thunk thunk = THUNK_DEFAULT;
@@ -599,6 +794,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 )
+         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
-- 
2.11.0


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

Re: [PATCH 2/3] x86/spec-ctrl: Add defines and variables for AMD SSBD support

[Jan Beulich]

>>> On 20.07.18 at 16:57, <brian.woods@amd.com> wrote:
> In preparation for adding switchable SSBD, add some defines and
> variables.
> 
> Signed-off-by: Brian Woods <brian.woods@amd.com>

Whether these additions fit the purpose can only be told when
seeing their use. Please fold this into the patch using these.

Jan



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

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

[Jan Beulich]

>>> On 20.07.18 at 16:57, <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>
> ---
>  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;

Pointless initializer.

> @@ -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"      : "",

Why log the same string twice? Simply OR together both conditions.
Also please don't omit the blank before the ? operator. Both remarks
apply as well 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 

I've peeked ahead into the following patches, and I can't see
why this needs to be a synthetic feature flag. We use such flags
for the purpose of alternative instruction patching, but you don't
do anything like that.

Jan



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

Re: [PATCH 3/3] x86/spec-ctrl: Add support for modifying SSBD AMD VIA LS_CFG MSR

[Jan Beulich]

>>> On 20.07.18 at 16:57, <brian.woods@amd.com> wrote:
> --- a/xen/arch/x86/cpu/amd.c
> +++ b/xen/arch/x86/cpu/amd.c
> @@ -607,16 +607,10 @@ static void init_amd(struct cpuinfo_x86 *c)
>  		case 0x17: bit = 10; break;
>  		}
>  
> -		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))
> +		if (bit >= 0 && !rdmsr_safe(MSR_AMD64_LS_CFG, value)) {
> +			ssbd_amd_ls_cfg_mask = 1ull << bit;
>  			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);
>  		}
>  	}

Code structure wise this looks to undo a fair part of what patch
1 has done. It would be nice to limit code churn.

> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1579,6 +1579,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>  
>      arch_init_memory();
>  
> +    ssbd_amd_ls_cfg_init();

Why can't this be called from init_speculation_mitigations()?

> @@ -497,6 +497,201 @@ static __init int parse_xpti(const char *s)
>  }
>  custom_param("xpti", parse_xpti);
>  
> +/*
> + * For family 15h and 16h, there are no SMT enabled processors, so there
> + * is no need for locking, just need to set an MSR bit.   For 17h, it
> + * depends if SMT is enabled.  If SMT, are two threads share a single
> + * MSR so there needs to be a lock and a virtual bit for each thread.
> + */
> +
> +/* used for non SMT mitigations (no shared MSRs) */
> +static void ssbd_amd_ls_cfg_set_nonsmt(bool enable_ssbd)
> +{
> +    unsigned long ls_cfg;

Please be consistent with types: ssbd_amd_ls_cfg_mask is
uint64_t, in which case variables like the one here should be too.

> +    if ( enable_ssbd )
> +    {
> +        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
> +    {
> +        rdmsrl(MSR_AMD64_LS_CFG, ls_cfg);
> +        if (ls_cfg & ssbd_amd_ls_cfg_mask)

Missing blanks.

> +        {
> +            ls_cfg &= ~ssbd_amd_ls_cfg_mask;
> +            wrmsrl(MSR_AMD64_LS_CFG, ls_cfg);
> +        }
> +    }
> +}

Please simplify this to have just a single rdmsrl() and wrmsrl()
each in the function.

> +/* used for family 17h with SMT enabled (shared MSRs) */
> +static void ssbd_amd_ls_cfg_set_smt(bool enable_ssbd)
> +{
> +    __u32 socket, core, thread;
> +    uint64_t enable_mask;

You really should notice such anomalies / inconsistencies yourself:
You properly use uint64_t here, so why not also uint32_t on the
preceding line? That said - why a fixed width type anyway for
those variables - unsigned int looks to be just fine there.

> +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\n");
> +        return;
> +    }
> +
> +    if ( ssbd_amd_smt_en )
> +        ssbd_amd_ls_cfg_set_smt(enable_ssbd);
> +    else
> +        ssbd_amd_ls_cfg_set_nonsmt(enable_ssbd);
> +}

This function is called from exactly one place, with the
parameter set to true. Makes me wonder what the actual
purpose of this patch is.

> +/*
> + * used to init the boot cpu, we don't need to lock anything because
> + * it's just the boot CPU
> + */

Still I wonder whether less code duplication wouldn't be better.

> +static __init void ssbd_amd_ls_cfg_set_init(void)
> +{
> +    __u32 socket, core, thread;
> +    unsigned long long ls_cfg;
> +    struct ssbd_amd_ls_cfg_smt_status *status;
> +    struct cpuinfo_x86  *c =  &boot_cpu_data;
> +
> +    if ( ssbd_amd_smt_en )
> +    {
> +        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;
> +        status->mask |= (1ull << thread);

This hides very well an assumption of there only ever being two
threads. Please don't. I'm also having a hard time seeing why
c->apicid being (non-)zero matters. Or wait - do you mean &
instead of && above (then also in ssbd_amd_ls_cfg_set_smt())?

> +    }
> +
> +    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);
> +    }
> +}
> +
> +__init void ssbd_amd_ls_cfg_init(void)

Most noticeable here, but applicable elsewhere: The canonical
placement is

void __init ssbd_amd_ls_cfg_init(void)

> +{
> +    int cores_per_socket, threads_per_core;
> +    struct cpuinfo_x86  *c =  &boot_cpu_data;
> +    int core,socket;

unsigned int please for anything that can't go negative. And a
blank is missing after the comma here, while there's one too
many on the line before.

You also don't look to be altering the data c points to - please
make this a pointer to const (and check whether there are
other places wanting such a transformation).

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

Blank lines between case blocks please.

> +        cores_per_socket = c->x86_max_cores;
> +        threads_per_core = c->x86_num_siblings;
> +
> +        if ( (cores_per_socket < 1) || (threads_per_core < 1) )
> +        {
> +            dprintk(XENLOG_ERR,
> +                    "SSBD AMD LS CFG: error in topology decoding\n");
> +            goto ssbd_amd_ls_cfg_init_fail;
> +        }
> +
> +        if ( threads_per_core > 1 )
> +        {
> +            ssbd_amd_smt_en = true;
> +            for ( socket = 0; socket < SSBD_AMD_MAX_SOCKET; socket++ )

I find such a hard-coded upper bound quite concerning. Is nr_sockets
not correct in the AMD case? If so, that would want fixing, such that
you can use the variable here.

> +            {
> +                ssbd_amd_smt_status[socket] =
> +                  (struct ssbd_amd_ls_cfg_smt_status*)
> +                  xmalloc_array (struct ssbd_amd_ls_cfg_smt_status,
> +                                 cores_per_socket);

Style: Blank before * and no blank before (.

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

Perhaps better

                    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 ( xen_ssbd_amd_ls_cfg_en )
> +        ssbd_amd_ls_cfg_set_init();
> +
> +    return;
> +
> +ssbd_amd_ls_cfg_init_fail:

Labels indented by at least one blank please.

> +    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);
> +    xen_ssbd_amd_ls_cfg_en = false;

Btw - why the xen_ prefix for the variable?

> +    return;
> +}

Pointless "return" at end of function.

Jan


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

Re: [PATCH 2/3] x86/spec-ctrl: Add defines and variables for AMD SSBD support

[Brian Woods]

On Tue, Jul 31, 2018 at 04:44:57AM -0600, Jan Beulich wrote:
> Whether these additions fit the purpose can only be told when
> seeing their use. Please fold this into the patch using these.
> 
> Jan

I was trying to split up patch 3 but I'll combine 2 and 3.

-- 
Brian Woods

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

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

[Brian Woods]

On Tue, Jul 31, 2018 at 04:47:48AM -0600, Jan Beulich wrote:
> Pointless initializer.

Noted

> Why log the same string twice? Simply OR together both conditions.
> Also please don't omit the blank before the ? operator. Both remarks
> apply as well further down.

Because they're completely different implementation at this point.  I
thought about naming it something else but couldn't think of something.
It also reads easier with having them split up where there isn't a ton
of logic in a single ternary operator.  There can be easily changed.

> I've peeked ahead into the following patches, and I can't see
> why this needs to be a synthetic feature flag. We use such flags
> for the purpose of alternative instruction patching, but you don't
> do anything like that.
> 
> Jan

I was talking Andy earily about a SSBD (via VIRT_SPEC_CTRL)
implementation and he mentioned something about ALTERNATIVE C funcs
which would allow to allow HVM guests to be able control SSBD.  In that
case having a synthetic feature flag is useful for helping to
differiate between SSBD via the LS_CFG MSR vs SPEC_CTRL in the future.


-- 
Brian Woods

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

Re: [PATCH 3/3] x86/spec-ctrl: Add support for modifying SSBD AMD VIA LS_CFG MSR

[Brian Woods]

On Tue, Jul 31, 2018 at 05:25:27AM -0600, Jan Beulich wrote:
> Code structure wise this looks to undo a fair part of what patch
> 1 has done. It would be nice to limit code churn.

Patch 1 stand alone just to improve reporting the capabilities of the
processor.  Currently Xen doesn't mention anything if SSBD is actually
enable on AMD processors.  Patch 2-3 add it where Xen can it
dynamically.

> Why can't this be called from init_speculation_mitigations()?

IIRC I was having memory init problems with.  It was moved to later in
the boot so that xmalloc would work correctly.  I haven't touched this
code in over month.  If you want a 100% tested answer I can retest
putting it in init_speculation_mitigations().

> Please be consistent with types: ssbd_amd_ls_cfg_mask is
> uint64_t, in which case variables like the one here should be too.

I think that was left over from something in init_amd, but yes.

> Missing blanks.

Noted

> Please simplify this to have just a single rdmsrl() and wrmsrl()
> each in the function.

Will do.

> You really should notice such anomalies / inconsistencies yourself:
> You properly use uint64_t here, so why not also uint32_t on the
> preceding line? That said - why a fixed width type anyway for
> those variables - unsigned int looks to be just fine there.

IIRC they're __32 in struct I'm reading from so I decided to use that.
I can change them though, that's easily.


> This function is called from exactly one place, with the
> parameter set to true. Makes me wonder what the actual
> purpose of this patch is.

See earlier in this email.

> Still I wonder whether less code duplication wouldn't be better.

current_cpu_data isn't available when ssbd_amd_ls_cfg_init is called.

> This hides very well an assumption of there only ever being two
> threads. Please don't. I'm also having a hard time seeing why
> c->apicid being (non-)zero matters. Or wait - do you mean &
> instead of && above (then also in ssbd_amd_ls_cfg_set_smt())?

Yes... That was meant to be a &.  Thanks for catching that.

> Most noticeable here, but applicable elsewhere: The canonical
> placement is
> 
> void __init ssbd_amd_ls_cfg_init(void)

> unsigned int please for anything that can't go negative. And a
> blank is missing after the comma here, while there's one too
> many on the line before.
> 
> You also don't look to be altering the data c points to - please
> make this a pointer to const (and check whether there are
> other places wanting such a transformation).

> Blank lines between case blocks please.

Noted about the above.

> I find such a hard-coded upper bound quite concerning. Is nr_sockets
> not correct in the AMD case? If so, that would want fixing, such that
> you can use the variable here.

It's been a while since I wrote this but IIRC, it has to do with
nr_sockets either being off or not available when the code is run.
For Family 17h which the patches are for, there's a max of two sockets.

> Style: Blank before * and no blank before (.

> Perhaps better
>                     spin_lock_init(&ssbd_amd_smt_status[socket][core].lock);
>                     ssbd_amd_smt_status[socket][core].mask = 0;
> ?

> Labels indented by at least one blank please.

Noted about the above.

> Btw - why the xen_ prefix for the variable?

See the first reply, but basically it's for if Xen has SSBD turned on
or not via using the LS_CFG MSR.

> Pointless "return" at end of function.
> 
> Jan

Noted.


Thanks for all the feedback.  I'll try and get v2 out in a couple of
days or so.

-- 
Brian Woods

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

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

[Jan Beulich]

>>> On 01.08.18 at 23:38, <brian.woods@amd.com> wrote:
> On Tue, Jul 31, 2018 at 04:47:48AM -0600, Jan Beulich wrote:
>> Why log the same string twice? Simply OR together both conditions.
>> Also please don't omit the blank before the ? operator. Both remarks
>> apply as well further down.
> 
> Because they're completely different implementation at this point.  I
> thought about naming it something else but couldn't think of something.
> It also reads easier with having them split up where there isn't a ton
> of logic in a single ternary operator.  There can be easily changed.

Well, I'm not going to insist, but I'd prefer things to be as suggested,
to be in line with other similar code. Let's see what Andrew thinks.

>> I've peeked ahead into the following patches, and I can't see
>> why this needs to be a synthetic feature flag. We use such flags
>> for the purpose of alternative instruction patching, but you don't
>> do anything like that.
> 
> I was talking Andy earily about a SSBD (via VIRT_SPEC_CTRL)
> implementation and he mentioned something about ALTERNATIVE C funcs
> which would allow to allow HVM guests to be able control SSBD.  In that
> case having a synthetic feature flag is useful for helping to
> differiate between SSBD via the LS_CFG MSR vs SPEC_CTRL in the future.

I'd much appreciate if such would be introduced once actually
needed, not early on just in case.

Jan



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

Re: [PATCH 3/3] x86/spec-ctrl: Add support for modifying SSBD AMD VIA LS_CFG MSR

[Jan Beulich]

>>> On 02.08.18 at 00:20, <brian.woods@amd.com> wrote:
> On Tue, Jul 31, 2018 at 05:25:27AM -0600, Jan Beulich wrote:
>> Code structure wise this looks to undo a fair part of what patch
>> 1 has done. It would be nice to limit code churn.
> 
> Patch 1 stand alone just to improve reporting the capabilities of the
> processor.  Currently Xen doesn't mention anything if SSBD is actually
> enable on AMD processors.  Patch 2-3 add it where Xen can it
> dynamically.

Which is all fine, but couldn't patch 1 be written in a way that the
next one(s) don't have to turn code structure all over again.

>> Why can't this be called from init_speculation_mitigations()?
> 
> IIRC I was having memory init problems with.  It was moved to later in
> the boot so that xmalloc would work correctly.  I haven't touched this
> code in over month.  If you want a 100% tested answer I can retest
> putting it in init_speculation_mitigations().

Would be nice if that could be arranged for, as the rather specialized
call looks pretty odd in (iirc __start_xen(); iirc because you've dropped
too much of the context in your reply, and I'm too lazy to dig out the
original patch again).

>> You really should notice such anomalies / inconsistencies yourself:
>> You properly use uint64_t here, so why not also uint32_t on the
>> preceding line? That said - why a fixed width type anyway for
>> those variables - unsigned int looks to be just fine there.
> 
> IIRC they're __32 in struct I'm reading from so I decided to use that.
> I can change them though, that's easily.

Thanks - we prefer to avoid u<N> and s<N> in favor of uint<N>_t
and int<N>_t in new code, and __u<N> as well as __s<N> should
go away rather sooner than later anyway, as representing name
space violations (__s8, for example, already looks to be unused).

>> This function is called from exactly one place, with the
>> parameter set to true. Makes me wonder what the actual
>> purpose of this patch is.
> 
> See earlier in this email.

Where? I can't find anything as to the purpose. Your response to
patch 1's comments on mine might have been a hint, but then again
the function parameter here seems contrary to the alternatives
patching plans, at least at the first glance. If you want to keep
the parameter (rather than introducing it when needed), please at
least outline the plans in the description.

>> Still I wonder whether less code duplication wouldn't be better.
> 
> current_cpu_data isn't available when ssbd_amd_ls_cfg_init is called.

By "isn't available" you mean "hasn't been populated yet"? Else
I don't understand.

>> I find such a hard-coded upper bound quite concerning. Is nr_sockets
>> not correct in the AMD case? If so, that would want fixing, such that
>> you can use the variable here.
> 
> It's been a while since I wrote this but IIRC, it has to do with
> nr_sockets either being off or not available when the code is run.
> For Family 17h which the patches are for, there's a max of two sockets.

I've implied that from how you wrote the patches, but such hard coding
will only make for more code churn later on. Try to be as generic as
possible.

>> Btw - why the xen_ prefix for the variable?
> 
> See the first reply, but basically it's for if Xen has SSBD turned on
> or not via using the LS_CFG MSR.

Personally I'd prefer if the xen_-prefixed sub-name-space was left to
the public interface. Make it an infix if you want to express what you've
explained?

Jan



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

Re: [PATCH 3/3] x86/spec-ctrl: Add support for modifying SSBD AMD VIA LS_CFG MSR

[Brian Woods]

On Thu, Aug 02, 2018 at 01:09:06AM -0600, Jan Beulich wrote:
> >>> On 02.08.18 at 00:20, <brian.woods@amd.com> wrote:
> > On Tue, Jul 31, 2018 at 05:25:27AM -0600, Jan Beulich wrote:
> >> Code structure wise this looks to undo a fair part of what patch
> >> 1 has done. It would be nice to limit code churn.
> > 
> > Patch 1 stand alone just to improve reporting the capabilities of the
> > processor.  Currently Xen doesn't mention anything if SSBD is actually
> > enable on AMD processors.  Patch 2-3 add it where Xen can it
> > dynamically.
> 
> Which is all fine, but couldn't patch 1 be written in a way that the
> next one(s) don't have to turn code structure all over again.

Rather than change 1, I changed patch 3 (well now patch 2).

> >> Why can't this be called from init_speculation_mitigations()?
> > 
> > IIRC I was having memory init problems with.  It was moved to later in
> > the boot so that xmalloc would work correctly.  I haven't touched this
> > code in over month.  If you want a 100% tested answer I can retest
> > putting it in init_speculation_mitigations().
> 
> Would be nice if that could be arranged for, as the rather specialized
> call looks pretty odd in (iirc __start_xen(); iirc because you've dropped
> too much of the context in your reply, and I'm too lazy to dig out the
> original patch again).

It was __start_xen().  Well, IIRC xalloc didn't work (well writing to
the address given) until after arch_init_memory().  For it to work in
init_speculation_mitigations(), I'd assume you'd need to call
arch_init_memory() before init_speculation_mitigations().

> >> You really should notice such anomalies / inconsistencies yourself:
> >> You properly use uint64_t here, so why not also uint32_t on the
> >> preceding line? That said - why a fixed width type anyway for
> >> those variables - unsigned int looks to be just fine there.
> > 
> > IIRC they're __32 in struct I'm reading from so I decided to use that.
> > I can change them though, that's easily.
> 
> Thanks - we prefer to avoid u<N> and s<N> in favor of uint<N>_t
> and int<N>_t in new code, and __u<N> as well as __s<N> should
> go away rather sooner than later anyway, as representing name
> space violations (__s8, for example, already looks to be unused).

Noted, I've changed all of them.

> >> This function is called from exactly one place, with the
> >> parameter set to true. Makes me wonder what the actual
> >> purpose of this patch is.
> > 
> > See earlier in this email.
> 
> Where? I can't find anything as to the purpose. Your response to
> patch 1's comments on mine might have been a hint, but then again
> the function parameter here seems contrary to the alternatives
> patching plans, at least at the first glance. If you want to keep
> the parameter (rather than introducing it when needed), please at
> least outline the plans in the description.

To be honest, I assumed that only patch 1 would get accepted and then
once there was the infrastructure (alternative c funcs), I'd rework the
rest into a patch set that would introduce allowing HVM guests to
change SSBD via MSR interception (since there is some interest in
that).  I guess I should have been clearer up front about it.

> >> Still I wonder whether less code duplication wouldn't be better.
> > 
> > current_cpu_data isn't available when ssbd_amd_ls_cfg_init is called.
> 
> By "isn't available" you mean "hasn't been populated yet"? Else
> I don't understand.

It hasn't been populated yet.

> >> I find such a hard-coded upper bound quite concerning. Is nr_sockets
> >> not correct in the AMD case? If so, that would want fixing, such that
> >> you can use the variable here.
> > 
> > It's been a while since I wrote this but IIRC, it has to do with
> > nr_sockets either being off or not available when the code is run.
> > For Family 17h which the patches are for, there's a max of two sockets.
> 
> I've implied that from how you wrote the patches, but such hard coding
> will only make for more code churn later on. Try to be as generic as
> possible.

Well, nr_sockets gets set in smp_prepare_cpus, which gets called after
init_speculation_mitigations() and ssbd_amd_ls_cfg_init().  It could be
put later on in the boot, but it needs to be run at least before other
cpus are brought online.  I'm welcome to any suggestions about how to
restructure things though.

Also, settings SSBD via the LS_CFG MSR is not a permanent method.  Once
hardware supports using SPEC_CTRL for it, it won't be needed.  For
multisocket systems, there should only be 2 sockets.  I agree with
making things generic, but this early in the boot there's limited
information.

> >> Btw - why the xen_ prefix for the variable?
> > 
> > See the first reply, but basically it's for if Xen has SSBD turned on
> > or not via using the LS_CFG MSR.
> 
> Personally I'd prefer if the xen_-prefixed sub-name-space was left to
> the public interface. Make it an infix if you want to express what you've
> explained?
> 
> Jan

Noted.  I'll change that.

Thanks again for the feedback.

-- 
Brian Woods

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

Re: [PATCH 3/3] x86/spec-ctrl: Add support for modifying SSBD AMD VIA LS_CFG MSR

[Jan Beulich]

>>> On 06.08.18 at 21:07, <brian.woods@amd.com> wrote:
> On Thu, Aug 02, 2018 at 01:09:06AM -0600, Jan Beulich wrote:
>> >>> On 02.08.18 at 00:20, <brian.woods@amd.com> wrote:
>> > On Tue, Jul 31, 2018 at 05:25:27AM -0600, Jan Beulich wrote:
>> >> Code structure wise this looks to undo a fair part of what patch
>> >> 1 has done. It would be nice to limit code churn.
>> > 
>> > Patch 1 stand alone just to improve reporting the capabilities of the
>> > processor.  Currently Xen doesn't mention anything if SSBD is actually
>> > enable on AMD processors.  Patch 2-3 add it where Xen can it
>> > dynamically.
>> 
>> Which is all fine, but couldn't patch 1 be written in a way that the
>> next one(s) don't have to turn code structure all over again.
> 
> Rather than change 1, I changed patch 3 (well now patch 2).
> 
>> >> Why can't this be called from init_speculation_mitigations()?
>> > 
>> > IIRC I was having memory init problems with.  It was moved to later in
>> > the boot so that xmalloc would work correctly.  I haven't touched this
>> > code in over month.  If you want a 100% tested answer I can retest
>> > putting it in init_speculation_mitigations().
>> 
>> Would be nice if that could be arranged for, as the rather specialized
>> call looks pretty odd in (iirc __start_xen(); iirc because you've dropped
>> too much of the context in your reply, and I'm too lazy to dig out the
>> original patch again).
> 
> It was __start_xen().  Well, IIRC xalloc didn't work (well writing to
> the address given) until after arch_init_memory().  For it to work in
> init_speculation_mitigations(), I'd assume you'd need to call
> arch_init_memory() before init_speculation_mitigations().

I don't think that's a viable option, but it could certainly be explored.
I wonder though whether you can't get away without allocation at
this early point.

>> >> Still I wonder whether less code duplication wouldn't be better.
>> > 
>> > current_cpu_data isn't available when ssbd_amd_ls_cfg_init is called.
>> 
>> By "isn't available" you mean "hasn't been populated yet"? Else
>> I don't understand.
> 
> It hasn't been populated yet.

Not even boot_cpu_data?

>> >> I find such a hard-coded upper bound quite concerning. Is nr_sockets
>> >> not correct in the AMD case? If so, that would want fixing, such that
>> >> you can use the variable here.
>> > 
>> > It's been a while since I wrote this but IIRC, it has to do with
>> > nr_sockets either being off or not available when the code is run.
>> > For Family 17h which the patches are for, there's a max of two sockets.
>> 
>> I've implied that from how you wrote the patches, but such hard coding
>> will only make for more code churn later on. Try to be as generic as
>> possible.
> 
> Well, nr_sockets gets set in smp_prepare_cpus, which gets called after
> init_speculation_mitigations() and ssbd_amd_ls_cfg_init().  It could be
> put later on in the boot, but it needs to be run at least before other
> cpus are brought online.  I'm welcome to any suggestions about how to
> restructure things though.

Did you consider using a presmp-initcall?

Jan



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

Re: [PATCH 3/3] x86/spec-ctrl: Add support for modifying SSBD AMD VIA LS_CFG MSR

[Brian Woods]

On Tue, Aug 07, 2018 at 01:51:07AM -0600, Jan Beulich wrote:
> >>> On 06.08.18 at 21:07, <brian.woods@amd.com> wrote:
> > On Thu, Aug 02, 2018 at 01:09:06AM -0600, Jan Beulich wrote:
> >> >>> On 02.08.18 at 00:20, <brian.woods@amd.com> wrote:
> >> > On Tue, Jul 31, 2018 at 05:25:27AM -0600, Jan Beulich wrote:
> >> >> Code structure wise this looks to undo a fair part of what patch
> >> >> 1 has done. It would be nice to limit code churn.
> >> > 
> >> > Patch 1 stand alone just to improve reporting the capabilities of the
> >> > processor.  Currently Xen doesn't mention anything if SSBD is actually
> >> > enable on AMD processors.  Patch 2-3 add it where Xen can it
> >> > dynamically.
> >> 
> >> Which is all fine, but couldn't patch 1 be written in a way that the
> >> next one(s) don't have to turn code structure all over again.
> > 
> > Rather than change 1, I changed patch 3 (well now patch 2).
> > 
> >> >> Why can't this be called from init_speculation_mitigations()?
> >> > 
> >> > IIRC I was having memory init problems with.  It was moved to later in
> >> > the boot so that xmalloc would work correctly.  I haven't touched this
> >> > code in over month.  If you want a 100% tested answer I can retest
> >> > putting it in init_speculation_mitigations().
> >> 
> >> Would be nice if that could be arranged for, as the rather specialized
> >> call looks pretty odd in (iirc __start_xen(); iirc because you've dropped
> >> too much of the context in your reply, and I'm too lazy to dig out the
> >> original patch again).
> > 
> > It was __start_xen().  Well, IIRC xalloc didn't work (well writing to
> > the address given) until after arch_init_memory().  For it to work in
> > init_speculation_mitigations(), I'd assume you'd need to call
> > arch_init_memory() before init_speculation_mitigations().
> 
> I don't think that's a viable option, but it could certainly be explored.
> I wonder though whether you can't get away without allocation at
> this early point.

Well, the thing is that you could use DECLARE_PER_CPU but then you
have to initialize it during each cpu start up, but two logical cpus
share a lock and only one needs to touch it etc.  I found it better to
just initialize everything on the boot cpu before others are brought
up, but the whole thing is a little messy.  (See the last comment.)

> >> >> Still I wonder whether less code duplication wouldn't be better.
> >> > 
> >> > current_cpu_data isn't available when ssbd_amd_ls_cfg_init is called.
> >> 
> >> By "isn't available" you mean "hasn't been populated yet"? Else
> >> I don't understand.
> > 
> > It hasn't been populated yet.
> 
> Not even boot_cpu_data?

boot_cpu_data is, but current_cpu_data and nr_sockets is not.

> >> >> I find such a hard-coded upper bound quite concerning. Is nr_sockets
> >> >> not correct in the AMD case? If so, that would want fixing, such that
> >> >> you can use the variable here.
> >> > 
> >> > It's been a while since I wrote this but IIRC, it has to do with
> >> > nr_sockets either being off or not available when the code is run.
> >> > For Family 17h which the patches are for, there's a max of two sockets.
> >> 
> >> I've implied that from how you wrote the patches, but such hard coding
> >> will only make for more code churn later on. Try to be as generic as
> >> possible.
> > 
> > Well, nr_sockets gets set in smp_prepare_cpus, which gets called after
> > init_speculation_mitigations() and ssbd_amd_ls_cfg_init().  It could be
> > put later on in the boot, but it needs to be run at least before other
> > cpus are brought online.  I'm welcome to any suggestions about how to
> > restructure things though.
> 
> Did you consider using a presmp-initcall?
> 
> Jan

I've been looking at it, seems like that could work.  You'd still need
something in start_secondary but it would take the init call out of
__start_xen().  I'll test that.


-- 
Brian Woods

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

Re: [PATCH 3/3] x86/spec-ctrl: Add support for modifying SSBD AMD VIA LS_CFG MSR

[Jan Beulich]

>>> On 08.08.18 at 18:43, <brian.woods@amd.com> wrote:
> On Tue, Aug 07, 2018 at 01:51:07AM -0600, Jan Beulich wrote:
>> >>> On 06.08.18 at 21:07, <brian.woods@amd.com> wrote:
>> > On Thu, Aug 02, 2018 at 01:09:06AM -0600, Jan Beulich wrote:
>> >> >>> On 02.08.18 at 00:20, <brian.woods@amd.com> wrote:
>> >> > current_cpu_data isn't available when ssbd_amd_ls_cfg_init is called.
>> >> 
>> >> By "isn't available" you mean "hasn't been populated yet"? Else
>> >> I don't understand.
>> > 
>> > It hasn't been populated yet.
>> 
>> Not even boot_cpu_data?
> 
> boot_cpu_data is, but current_cpu_data and nr_sockets is not.

Then any reason not to use it (on the BSP)? I agree nr_sockets
is a problem, should it really be needed this early.

Jan



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