[v2] x86/amd: Address AMD erratum #1485

[for-4.18][PATCH v2] x86/amd: Address AMD erratum #1485

Posted by Alejandro Vallejo 6 months, 3 weeks ago

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(+)

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

Re: [for-4.18][PATCH v2] x86/amd: Address AMD erratum #1485

Posted by Andrew Cooper 6 months, 3 weeks ago

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

Re: [for-4.18][PATCH v2] x86/amd: Address AMD erratum #1485

Posted by Alejandro Vallejo 6 months, 3 weeks ago

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

Re: [for-4.18][PATCH v2] x86/amd: Address AMD erratum #1485

Posted by Andrew Cooper 6 months, 3 weeks ago

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

Re: [for-4.18][PATCH v2] x86/amd: Address AMD erratum #1485

Posted by Julien Grall 6 months, 3 weeks ago

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

Re: [for-4.18][PATCH v2] x86/amd: Address AMD erratum #1485

Posted by Alejandro Vallejo 6 months, 3 weeks ago

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