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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.08.18 at 18:54, <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>
> ---

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

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

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

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

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

__read_mostly (afaict)

Also the variable seems pointless - this ...

> +uint64_t __read_mostly ssbd_amd_ls_cfg_mask;

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

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




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