Re: [PATCH] x86/amd: extend CPU errata #1474 affected models

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 20/12/2023 3:10 pm, Roger Pau Monné wrote:
> On Wed, Dec 20, 2023 at 02:46:43PM +0000, Andrew Cooper wrote:
>> On 20/12/2023 2:22 pm, Roger Pau Monne wrote:
>>> Errata #1474 does also affect models from family 17h ranges 00-2Fh, so the
>>> errata now covers all the models released under Family 17h (Zen, Zen+ and
>>> Zen2).
>> Perhaps "has now been extended to cover models from ..." ?
>>
>>> Additionally extend the workaround to Family 18h (Hygon), since it's based 
>>> on
>>> the Zen architecture and very likely affected.
>>>
>>> Rename all the zen2 related symbols to plain zen, since the errata doesn't
>>> exclusively affect Zen2 anymore.
>>>
>>> Reported-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>> Thanks for doing this.
>>
>> Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>
>> I was going to suggest linking to an example revision guide but I see
>> the AMD website is still broken.
>>
>>> diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
>>> index 0f305312ff2a..099646dc3994 100644
>>> --- a/xen/arch/x86/cpu/amd.c
>>> +++ b/xen/arch/x86/cpu/amd.c
>>> @@ -54,7 +54,7 @@ bool __read_mostly amd_acpi_c1e_quirk;
>>>  bool __ro_after_init amd_legacy_ssbd;
>>>  bool __initdata amd_virt_spec_ctrl;
>>>  
>>> -static bool __read_mostly zen2_c6_disabled;
>>> +static bool __read_mostly zen_c6_disabled;
>> amd_1474_c6_disable ?
> Maybe just fam17h_c6_disabled, since the main usage of that variable
> is to force calling fam17_disable_c6().
>
>> That's about as general as I can make it, without losing precision.
>>
>>
>>>  
>>>  static inline int rdmsr_amd_safe(unsigned int msr, unsigned int *lo,
>>>                              unsigned int *hi)
>>> @@ -978,24 +978,24 @@ void amd_check_zenbleed(void)
>>>                    val & chickenbit ? "chickenbit" : "microcode");
>>>  }
>>>  
>>> -static void cf_check zen2_disable_c6(void *arg)
>>> +static void cf_check zen_disable_c6(void *arg)
>> fam17_disable_c6() ?  I know Hygon is 0x18 but it's also reasonably well
>> know to be the same uarch.
>>
>> This particular algorithm is good for all Fam17 uarches, irrespective of
>> #1474, even if they happen to be the same set of CPUs in practice.
> Yeah, I was about to use fam17h prefix, but that wouldn't cover Hygon.
> I we are fine with it I can send an adjusted v2 using fam17h prefix.

I think we're fine calling it fam17.  Happy to do that consistently.

~Andrew