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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.07.18 at 16:57, <brian.woods@xxxxxxx> 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@xxxxxxx>
> ---
>  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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel