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

["Jan Beulich" <JBeulich@suse.com>]

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