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

[Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>]

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