| Message ID | 20231013131846.175702-1-alejandro.vallejo@cloud.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [for-4.18,v2] x86/amd: Address AMD erratum #1485 | expand |
On 13/10/2023 9:18 pm, Alejandro Vallejo wrote: > diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c > index 4f27187f92..085c4772d7 100644 > --- a/xen/arch/x86/cpu/amd.c > +++ b/xen/arch/x86/cpu/amd.c > @@ -1167,6 +1167,14 @@ static void cf_check init_amd(struct cpuinfo_x86 *c) > if (c->x86 == 0x10) > __clear_bit(X86_FEATURE_MONITOR, c->x86_capability); > > + /* Fix for AMD erratum #1485 */ > + if (!cpu_has_hypervisor && c->x86 == 0x19 && is_zen4_uarch()) { > + rdmsrl(MSR_AMD64_BP_CFG, value); > + #define ZEN4_BP_CFG_SHARED_BTB_FIX (1ull << 5) > + wrmsrl(MSR_AMD64_BP_CFG, > + value | ZEN4_BP_CFG_SHARED_BTB_FIX); A #define indented like that is weird. I tend to either opencode it directly in the "value |" expression, or have a local variable called chickenbit. This will surely be a core scope MSR rather than thread scope, at which point it the write ought to be conditional on seeing the chickenbit clear (hence needing to refer to the value at least twice, so use a local variable). It probably also wants a note about non-atomic RMW, and how it's safe in practice. (See the Zenbleed comment). Otherwise, LGTM. As this is just cribbing from an existing example, I'm happy to adjust on commit, but it's probably better to double check in the PPR and retest. ~Andrew
On Fri, Oct 13, 2023 at 09:40:52PM +0800, Andrew Cooper wrote: > On 13/10/2023 9:18 pm, Alejandro Vallejo wrote: > > diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c > > index 4f27187f92..085c4772d7 100644 > > --- a/xen/arch/x86/cpu/amd.c > > +++ b/xen/arch/x86/cpu/amd.c > > @@ -1167,6 +1167,14 @@ static void cf_check init_amd(struct cpuinfo_x86 *c) > > if (c->x86 == 0x10) > > __clear_bit(X86_FEATURE_MONITOR, c->x86_capability); > > > > + /* Fix for AMD erratum #1485 */ > > + if (!cpu_has_hypervisor && c->x86 == 0x19 && is_zen4_uarch()) { > > + rdmsrl(MSR_AMD64_BP_CFG, value); > > + #define ZEN4_BP_CFG_SHARED_BTB_FIX (1ull << 5) > > + wrmsrl(MSR_AMD64_BP_CFG, > > + value | ZEN4_BP_CFG_SHARED_BTB_FIX); > > A #define indented like that is weird. I tend to either opencode it > directly in the "value |" expression, or have a local variable called > chickenbit. Ok, I don't mind either way. I'll just go with the chickenbit. > > This will surely be a core scope MSR rather than thread scope, It is, though I doubt it matters a whole lot. The writes are consistent anyway. > at which > point the write ought to be conditional on seeing the chickenbit > clear (hence needing to refer to the value at least twice, so use a > local variable). I have serious doubts such a conditional would do much for boot times, but sure. > > It probably also wants a note about non-atomic RMW, and how it's safe in > practice. (See the Zenbleed comment). Fair enough. > > Otherwise, LGTM. > > As this is just cribbing from an existing example, I'm happy to adjust > on commit, but it's probably better to double check in the PPR and retest. > > ~Andrew Let me send a v3 after re-testing with the conditional in place Thanks, Alejandro
Hi Alejandro, The original e-mail didn't yet reach my inbox. So answering here as there are enough context. On 13/10/2023 15:33, Alejandro Vallejo wrote: > On Fri, Oct 13, 2023 at 09:40:52PM +0800, Andrew Cooper wrote: >> On 13/10/2023 9:18 pm, Alejandro Vallejo wrote: >>> diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c >>> index 4f27187f92..085c4772d7 100644 >>> --- a/xen/arch/x86/cpu/amd.c >>> +++ b/xen/arch/x86/cpu/amd.c >>> @@ -1167,6 +1167,14 @@ static void cf_check init_amd(struct cpuinfo_x86 *c) >>> if (c->x86 == 0x10) >>> __clear_bit(X86_FEATURE_MONITOR, c->x86_capability); >>> >>> + /* Fix for AMD erratum #1485 */ >>> + if (!cpu_has_hypervisor && c->x86 == 0x19 && is_zen4_uarch()) { >>> + rdmsrl(MSR_AMD64_BP_CFG, value); >>> + #define ZEN4_BP_CFG_SHARED_BTB_FIX (1ull << 5) You are introducing a violation to MISRA Rule 7.3 [1] which was accepted recently. You want to use 1ULL rather than the lower case version. [1] https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_07_03.c Cheers,
Hi, On Fri, Oct 13, 2023 at 03:41:49PM +0100, Julien Grall wrote: > Hi Alejandro, > > The original e-mail didn't yet reach my inbox. So answering here as there > are enough context. > > On 13/10/2023 15:33, Alejandro Vallejo wrote: > > On Fri, Oct 13, 2023 at 09:40:52PM +0800, Andrew Cooper wrote: > > > On 13/10/2023 9:18 pm, Alejandro Vallejo wrote: > > > > diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c > > > > index 4f27187f92..085c4772d7 100644 > > > > --- a/xen/arch/x86/cpu/amd.c > > > > +++ b/xen/arch/x86/cpu/amd.c > > > > @@ -1167,6 +1167,14 @@ static void cf_check init_amd(struct cpuinfo_x86 *c) > > > > if (c->x86 == 0x10) > > > > __clear_bit(X86_FEATURE_MONITOR, c->x86_capability); > > > > + /* Fix for AMD erratum #1485 */ > > > > + if (!cpu_has_hypervisor && c->x86 == 0x19 && is_zen4_uarch()) { > > > > + rdmsrl(MSR_AMD64_BP_CFG, value); > > > > + #define ZEN4_BP_CFG_SHARED_BTB_FIX (1ull << 5) > > You are introducing a violation to MISRA Rule 7.3 [1] which was accepted > recently. You want to use 1ULL rather than the lower case version. > > [1] https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_07_03.c > > Cheers, > > -- > Julien Grall Oh, I didn't know about that one. Fair enough. Thanks, Alejandro
On 13/10/2023 10:33 pm, Alejandro Vallejo wrote: > On Fri, Oct 13, 2023 at 09:40:52PM +0800, Andrew Cooper wrote: >> On 13/10/2023 9:18 pm, Alejandro Vallejo wrote: >> This will surely be a core scope MSR rather than thread scope, > It is, though I doubt it matters a whole lot. The writes are consistent > anyway. This happens to be true because you introduced the first use of the MSR. It ceases to be true for the next chickenbit in this MSR, which is why ... >> at which >> point the write ought to be conditional on seeing the chickenbit >> clear (hence needing to refer to the value at least twice, so use a >> local variable). > I have serious doubts such a conditional would do much for boot times, but > sure. ... this is not about boot time. It's about avoiding an unnecessary non-atomic action. (TBH, when the second chickenbit comes, it's all suspect anyway and probably needs to end up like the pre-SSBD handling, which has horrifying complexity.) ~Andrew
diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c index 4f27187f92..085c4772d7 100644 --- a/xen/arch/x86/cpu/amd.c +++ b/xen/arch/x86/cpu/amd.c @@ -1167,6 +1167,14 @@ static void cf_check init_amd(struct cpuinfo_x86 *c) if (c->x86 == 0x10) __clear_bit(X86_FEATURE_MONITOR, c->x86_capability); + /* Fix for AMD erratum #1485 */ + if (!cpu_has_hypervisor && c->x86 == 0x19 && is_zen4_uarch()) { + rdmsrl(MSR_AMD64_BP_CFG, value); + #define ZEN4_BP_CFG_SHARED_BTB_FIX (1ull << 5) + wrmsrl(MSR_AMD64_BP_CFG, + value | ZEN4_BP_CFG_SHARED_BTB_FIX); + } + if (!cpu_has_amd_erratum(c, AMD_ERRATUM_121)) opt_allow_unsafe = 1; else if (opt_allow_unsafe < 0) diff --git a/xen/arch/x86/include/asm/amd.h b/xen/arch/x86/include/asm/amd.h index d862cb7972..0700827561 100644 --- a/xen/arch/x86/include/asm/amd.h +++ b/xen/arch/x86/include/asm/amd.h @@ -145,11 +145,16 @@ * Hygon (Fam18h) but without simple model number rules. Instead, use STIBP * as a heuristic that distinguishes the two. * + * For Zen3 and Zen4 (Fam19h) the heuristic is the presence of AutoIBRS, as + * it's Zen4-specific. + * * The caller is required to perform the appropriate vendor/family checks * first. */ #define is_zen1_uarch() (!boot_cpu_has(X86_FEATURE_AMD_STIBP)) #define is_zen2_uarch() boot_cpu_has(X86_FEATURE_AMD_STIBP) +#define is_zen3_uarch() (!boot_cpu_has(X86_FEATURE_AUTO_IBRS)) +#define is_zen4_uarch() boot_cpu_has(X86_FEATURE_AUTO_IBRS) struct cpuinfo_x86; int cpu_has_amd_erratum(const struct cpuinfo_x86 *, int, ...); diff --git a/xen/arch/x86/include/asm/msr-index.h b/xen/arch/x86/include/asm/msr-index.h index 11ffed543a..7b3490bfb1 100644 --- a/xen/arch/x86/include/asm/msr-index.h +++ b/xen/arch/x86/include/asm/msr-index.h @@ -403,6 +403,7 @@ #define MSR_AMD64_DE_CFG 0xc0011029 #define AMD64_DE_CFG_LFENCE_SERIALISE (_AC(1, ULL) << 1) #define MSR_AMD64_EX_CFG 0xc001102c +#define MSR_AMD64_BP_CFG 0xc001102e #define MSR_AMD64_DE_CFG2 0xc00110e3 #define MSR_AMD64_DR0_ADDRESS_MASK 0xc0011027
Fix adapted off Linux's mailing list: https://lore.kernel.org/lkml/D99589F4-BC5D-430B-87B2-72C20370CF57@exactcode.com/T/#u Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com> Release-acked-by: Henry Wang <Henry.Wang@arm.com> --- v2: * Removed v1/patch1, as it proved to be contentious * Also changed is_zen[34]_uarch() into a heuristic check only * Streamlined comments and commit message --- xen/arch/x86/cpu/amd.c | 8 ++++++++ xen/arch/x86/include/asm/amd.h | 5 +++++ xen/arch/x86/include/asm/msr-index.h | 1 + 3 files changed, 14 insertions(+)