[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 1/2] x86/spec-ctrl: add AMD SSBD LS_CFG in init print
>>> On 09.08.18 at 21:42, <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> > --- 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@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |