| Message ID | 20210817143006.2821-4-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/amd: Hardware speculative controls | expand |
On 17.08.2021 16:30, Andrew Cooper wrote: > The opencoded legacy Memory Disambiguation logic in init_amd() neglected > Fam19h for the Zen3 microarchitecture. > > In practice, all Zen2 based system (AMD Fam17h Model >= 0x30 and Hygon Fam18h > Model >= 0x4) have the architectural MSR_SPEC_CTRL and the SSBD bit within it. > > Implement the algorithm given in AMD's SSBD whitepaper, and leave a > printk_once() behind in the case that no controls can be found. > > This now means that a user choosing `spec-ctrl=no-ssb` will actually turn off > Memory Disambiguation on Fam19h/Zen3 systems. Aiui you mean `spec-ctrl=no-ssbd` here? And the effect would then be to turn _on_ Memory Disambiguation, unless the original comment was the wrong way round? I'm also concerned by this behavioral change: I think opt_ssbd would want to become a tristate, such that not specifying the option at all will not also result in turning the bit off even if it was on for some reason (firmware?). Similarly "spec-ctrl=no" and "spec-ctrl=no-xen" imo shouldn't have this effect. > --- a/xen/arch/x86/cpu/amd.c > +++ b/xen/arch/x86/cpu/amd.c > @@ -681,6 +681,56 @@ void amd_init_lfence(struct cpuinfo_x86 *c) > c->x86_capability); > } > > +/* > + * Refer to the AMD Speculative Store Bypass whitepaper: > + * https://developer.amd.com/wp-content/resources/124441_AMD64_SpeculativeStoreBypassDisable_Whitepaper_final.pdf > + */ > +void amd_init_ssbd(const struct cpuinfo_x86 *c) > +{ > + int bit = -1; > + > + if (cpu_has_ssb_no) > + return; > + > + if (cpu_has_amd_ssbd) { > + wrmsrl(MSR_SPEC_CTRL, opt_ssbd ? SPEC_CTRL_SSBD : 0); > + return; > + } > + > + if (cpu_has_virt_ssbd) { > + wrmsrl(MSR_VIRT_SPEC_CTRL, opt_ssbd ? SPEC_CTRL_SSBD : 0); > + return; > + } > + > + switch (c->x86) { > + case 0x15: bit = 54; break; > + case 0x16: bit = 33; break; > + case 0x17: > + case 0x18: bit = 10; break; > + } > + > + if (bit >= 0) { > + uint64_t val, mask = 1ull << bit; > + > + if (rdmsr_safe(MSR_AMD64_LS_CFG, val) || > + ({ > + val &= ~mask; > + if ( opt_ssbd ) Nit: No spaces inside the parentheses here. Jan
On 19/08/2021 15:59, Jan Beulich wrote: > On 17.08.2021 16:30, Andrew Cooper wrote: >> The opencoded legacy Memory Disambiguation logic in init_amd() neglected >> Fam19h for the Zen3 microarchitecture. >> >> In practice, all Zen2 based system (AMD Fam17h Model >= 0x30 and Hygon Fam18h >> Model >= 0x4) have the architectural MSR_SPEC_CTRL and the SSBD bit within it. >> >> Implement the algorithm given in AMD's SSBD whitepaper, and leave a >> printk_once() behind in the case that no controls can be found. >> >> This now means that a user choosing `spec-ctrl=no-ssb` will actually turn off >> Memory Disambiguation on Fam19h/Zen3 systems. > Aiui you mean `spec-ctrl=no-ssbd` here? And the effect would then be > to turn _on_ Memory Disambiguation, unless the original comment was > the wrong way round? I'm also concerned by this behavioral change: > I think opt_ssbd would want to become a tristate, such that not > specifying the option at all will not also result in turning the bit > off even if it was on for some reason (firmware?). Similarly > "spec-ctrl=no" and "spec-ctrl=no-xen" imo shouldn't have this effect. I messed that bit of the description up. I means `spec-ctrl=ssb`, i.e. the non-default value. We do not disable Memory Disambiguation (the speculative feature which causes the Speculative Store Bypass vulnerability) by default (due to the perf hit), but if the user explicitly asks for it using the available command line option, nothing currently happens on Fam19h. ~Andrew
On 24.08.2021 15:39, Andrew Cooper wrote: > On 19/08/2021 15:59, Jan Beulich wrote: >> On 17.08.2021 16:30, Andrew Cooper wrote: >>> The opencoded legacy Memory Disambiguation logic in init_amd() neglected >>> Fam19h for the Zen3 microarchitecture. >>> >>> In practice, all Zen2 based system (AMD Fam17h Model >= 0x30 and Hygon Fam18h >>> Model >= 0x4) have the architectural MSR_SPEC_CTRL and the SSBD bit within it. >>> >>> Implement the algorithm given in AMD's SSBD whitepaper, and leave a >>> printk_once() behind in the case that no controls can be found. >>> >>> This now means that a user choosing `spec-ctrl=no-ssb` will actually turn off >>> Memory Disambiguation on Fam19h/Zen3 systems. >> Aiui you mean `spec-ctrl=no-ssbd` here? And the effect would then be >> to turn _on_ Memory Disambiguation, unless the original comment was >> the wrong way round? I'm also concerned by this behavioral change: >> I think opt_ssbd would want to become a tristate, such that not >> specifying the option at all will not also result in turning the bit >> off even if it was on for some reason (firmware?). Similarly >> "spec-ctrl=no" and "spec-ctrl=no-xen" imo shouldn't have this effect. > > I messed that bit of the description up. I means `spec-ctrl=ssb`, i.e. > the non-default value. > > We do not disable Memory Disambiguation (the speculative feature which > causes the Speculative Store Bypass vulnerability) by default (due to > the perf hit), but if the user explicitly asks for it using the > available command line option, nothing currently happens on Fam19h. Oh, I see. Yet (nit) then still "spec-ctrl=ssbd". Jan
diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c index 2260eef3aab5..567565199373 100644 --- a/xen/arch/x86/cpu/amd.c +++ b/xen/arch/x86/cpu/amd.c @@ -681,6 +681,56 @@ void amd_init_lfence(struct cpuinfo_x86 *c) c->x86_capability); } +/* + * Refer to the AMD Speculative Store Bypass whitepaper: + * https://developer.amd.com/wp-content/resources/124441_AMD64_SpeculativeStoreBypassDisable_Whitepaper_final.pdf + */ +void amd_init_ssbd(const struct cpuinfo_x86 *c) +{ + int bit = -1; + + if (cpu_has_ssb_no) + return; + + if (cpu_has_amd_ssbd) { + wrmsrl(MSR_SPEC_CTRL, opt_ssbd ? SPEC_CTRL_SSBD : 0); + return; + } + + if (cpu_has_virt_ssbd) { + wrmsrl(MSR_VIRT_SPEC_CTRL, opt_ssbd ? SPEC_CTRL_SSBD : 0); + return; + } + + switch (c->x86) { + case 0x15: bit = 54; break; + case 0x16: bit = 33; break; + case 0x17: + case 0x18: bit = 10; break; + } + + if (bit >= 0) { + uint64_t val, mask = 1ull << bit; + + if (rdmsr_safe(MSR_AMD64_LS_CFG, val) || + ({ + val &= ~mask; + if ( opt_ssbd ) + val |= mask; + false; + }) || + wrmsr_safe(MSR_AMD64_LS_CFG, val) || + ({ + rdmsrl(MSR_AMD64_LS_CFG, val); + (val & mask) != (opt_ssbd * mask); + })) + bit = -1; + } + + if (bit < 0) + printk_once(XENLOG_ERR "No SSBD controls available\n"); +} + static void init_amd(struct cpuinfo_x86 *c) { u32 l, h; @@ -731,24 +781,7 @@ static void init_amd(struct cpuinfo_x86 *c) else /* Implicily "== 0x10 || >= 0x12" by being 64bit. */ amd_init_lfence(c); - /* - * If the user has explicitly chosen to disable Memory Disambiguation - * to mitigiate Speculative Store Bypass, poke the appropriate MSR. - */ - if (opt_ssbd) { - int bit = -1; - - switch (c->x86) { - case 0x15: bit = 54; break; - case 0x16: bit = 33; break; - case 0x17: bit = 10; break; - } - - if (bit >= 0 && !rdmsr_safe(MSR_AMD64_LS_CFG, value)) { - value |= 1ull << bit; - wrmsr_safe(MSR_AMD64_LS_CFG, value); - } - } + amd_init_ssbd(c); /* MFENCE stops RDTSC speculation */ if (!cpu_has_lfence_dispatch) diff --git a/xen/arch/x86/cpu/cpu.h b/xen/arch/x86/cpu/cpu.h index 1ac3b2867a04..1a5b3918b37e 100644 --- a/xen/arch/x86/cpu/cpu.h +++ b/xen/arch/x86/cpu/cpu.h @@ -21,3 +21,4 @@ extern bool detect_extended_topology(struct cpuinfo_x86 *c); void early_init_amd(struct cpuinfo_x86 *c); void amd_log_freq(const struct cpuinfo_x86 *c); void amd_init_lfence(struct cpuinfo_x86 *c); +void amd_init_ssbd(const struct cpuinfo_x86 *c); diff --git a/xen/arch/x86/cpu/hygon.c b/xen/arch/x86/cpu/hygon.c index 67e23c5df9e3..56792146739e 100644 --- a/xen/arch/x86/cpu/hygon.c +++ b/xen/arch/x86/cpu/hygon.c @@ -33,15 +33,7 @@ static void init_hygon(struct cpuinfo_x86 *c) unsigned long long value; amd_init_lfence(c); - - /* - * If the user has explicitly chosen to disable Memory Disambiguation - * to mitigiate Speculative Store Bypass, poke the appropriate MSR. - */ - if (opt_ssbd && !rdmsr_safe(MSR_AMD64_LS_CFG, value)) { - value |= 1ull << 10; - wrmsr_safe(MSR_AMD64_LS_CFG, value); - } + amd_init_ssbd(c); /* MFENCE stops RDTSC speculation */ if (!cpu_has_lfence_dispatch) diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c index 9bf0fbf99813..0850afc09358 100644 --- a/xen/arch/x86/spec_ctrl.c +++ b/xen/arch/x86/spec_ctrl.c @@ -326,20 +326,23 @@ static void __init print_details(enum ind_thunk thunk, uint64_t caps) (caps & ARCH_CAPS_IBRS_ALL) ? " IBRS_ALL" : "", (caps & ARCH_CAPS_RSBA) ? " RSBA" : "", (caps & ARCH_CAPS_SKIP_L1DFL) ? " SKIP_L1DFL" : "", + (e8b & cpufeat_mask(X86_FEATURE_SSB_NO)) || (caps & ARCH_CAPS_SSB_NO) ? " SSB_NO" : "", (caps & ARCH_CAPS_MDS_NO) ? " MDS_NO" : "", (caps & ARCH_CAPS_TAA_NO) ? " TAA_NO" : ""); /* Hardware features which need driving to mitigate issues. */ - 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", (e8b & cpufeat_mask(X86_FEATURE_IBPB)) || (_7d0 & cpufeat_mask(X86_FEATURE_IBRSB)) ? " IBPB" : "", (_7d0 & cpufeat_mask(X86_FEATURE_IBRSB)) ? " IBRS" : "", (_7d0 & cpufeat_mask(X86_FEATURE_STIBP)) ? " STIBP" : "", + (e8b & cpufeat_mask(X86_FEATURE_AMD_SSBD)) || (_7d0 & cpufeat_mask(X86_FEATURE_SSBD)) ? " SSBD" : "", (_7d0 & cpufeat_mask(X86_FEATURE_L1D_FLUSH)) ? " L1D_FLUSH" : "", (_7d0 & cpufeat_mask(X86_FEATURE_MD_CLEAR)) ? " MD_CLEAR" : "", (_7d0 & cpufeat_mask(X86_FEATURE_SRBDS_CTRL)) ? " SRBDS_CTRL" : "", + (e8b & cpufeat_mask(X86_FEATURE_VIRT_SSBD)) ? " VIRT_SSBD" : "", (caps & ARCH_CAPS_TSX_CTRL) ? " TSX_CTRL" : ""); /* Compiled-in support which pertains to mitigations. */
The opencoded legacy Memory Disambiguation logic in init_amd() neglected Fam19h for the Zen3 microarchitecture. In practice, all Zen2 based system (AMD Fam17h Model >= 0x30 and Hygon Fam18h Model >= 0x4) have the architectural MSR_SPEC_CTRL and the SSBD bit within it. Implement the algorithm given in AMD's SSBD whitepaper, and leave a printk_once() behind in the case that no controls can be found. This now means that a user choosing `spec-ctrl=no-ssb` will actually turn off Memory Disambiguation on Fam19h/Zen3 systems. This still remains a single system-wide setting (for now), and is not context switched between vCPUs. As such, it doesn't interact with Intel's use of MSR_SPEC_CTRL and default_xen_spec_ctrl (yet). Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Wei Liu <wei.liu2@citrix.com> CC: Roger Pau Monné <roger.pau@citrix.com> --- xen/arch/x86/cpu/amd.c | 69 +++++++++++++++++++++++++++++++++++------------- xen/arch/x86/cpu/cpu.h | 1 + xen/arch/x86/cpu/hygon.c | 10 +------ xen/arch/x86/spec_ctrl.c | 5 +++- 4 files changed, 57 insertions(+), 28 deletions(-)