Re: [RFC PATCH 3/4] x86/cpu: amd: Define processor families

[Punit Agrawal]

Hi Boris,

Borislav Petkov <bp@xxxxxxxxx> writes:

> On Wed, Nov 25, 2020 at 11:48:46PM +0900, Punit Agrawal wrote:
>> So far, the AMD processor identifier (family, models, stepping) are
>> referred to by raw values making it easy to make mistakes. It is also
>> harder to read and maintain. Additionally, these values are also being
>> used in subsystems outside the arch code where not everybody maybe be
>> as familiar with the processor identifiers.
>> 
>> As a first step towards improving the status quo, add macros for the
>> AMD processor families and propagate them through the existing
>> cpu_device_id.h header used for this purpose.
>> 
>> Signed-off-by: Punit Agrawal <punitagrawal@xxxxxxxxx>
>> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
>> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
>> Cc: Borislav Petkov <bp@xxxxxxxxx>
>> Cc: x86@xxxxxxxxxx
>> ---
>>  arch/x86/include/asm/amd-family.h    | 18 ++++++++++++++++++
>>  arch/x86/include/asm/cpu_device_id.h |  2 ++
>>  2 files changed, 20 insertions(+)
>>  create mode 100644 arch/x86/include/asm/amd-family.h
>> 
>> diff --git a/arch/x86/include/asm/amd-family.h b/arch/x86/include/asm/amd-family.h
>> new file mode 100644
>> index 000000000000..dff4d13b8e74
>> --- /dev/null
>> +++ b/arch/x86/include/asm/amd-family.h
>> @@ -0,0 +1,18 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef _ASM_X86_AMD_FAMILY_H
>> +#define _ASM_X86_AMD_FAMILY_H
>> +
>> +#define AMD_FAM_K5			0x04
>> +#define AMD_FAM_K6			0x05
>> +#define AMD_FAM_K7			0x06
>> +#define AMD_FAM_K8			0x0F
>> +#define AMD_FAM_K10			0x10
>
> Fam 0x10 is Greyhound and a lot more core names. I'd let the AMD folks
> on Cc say what they wanna call it but I don't think K10 was used
> anywhere except outside of AMD. :)

Didn't realize the core was internal only. There are a couple of
instances where the family does shows up arch/x86/kernel/cpu/amd.c so
atleast some of the patches did make it upstream.

>> +#define AMD_FAM_K8_K10_HYBRID		0x11
>
> That was called Griffin so AMD_FAM_GRIFFIN I guess. If not used anywhere
> in the tree, no need to add the define.

I haven't looked to deeply but there's one instance in
arch/x86/kernel/cpu/amd.c - though I wonder if that could be re-written
to rely on a hardware / firmware interface instead.

> Same holds true for the rest of the defines - add them only when
> they're used.

Makes sense - I will follow your suggestion in the next version.

>> +#define AMD_FAM_LLANO			0x12
>> +#define AMD_FAM_BOBCAT			0x14
>> +#define AMD_FAM_BULLDOZER		0x15
>> +#define AMD_FAM_JAGUAR			0x16
>> +#define AMD_FAM_ZEN			0x17
>
> ZEN2 is also 0x17 but different models so this is where the family
> matching scheme doesn't work - you'd need the models too.

Yes, I wasn't sure the best way to handle this so went with the earlier
generation name. I think for such cases, something that looks at the
cpuinfo_x86 structure and decides the family / generation is probably
the way to go.

> 0x18 is HYGON

I missed this one. I'll add it to the list.

Thanks for the review and your comments. I'll wait a bit to see if
there's any further feedback.

Cheers,
Punit

[...]