Re: [PATCH 11/11] x86/cpu: Make all all CPUID leaf names consistent

[Dave Hansen <dave.hansen@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 2312 bytes --]

On 10/31/24 03:18, Borislav Petkov wrote:
> On Wed, Oct 30, 2024 at 02:33:29PM -0700, Dave Hansen wrote:
>> diff -puN arch/x86/include/asm/cpuid.h~xsave-leaf-checks-3 arch/x86/include/asm/cpuid.h
>> --- a/arch/x86/include/asm/cpuid.h~xsave-leaf-checks-3	2024-10-30 12:26:59.222216332 -0700
>> +++ b/arch/x86/include/asm/cpuid.h	2024-10-30 12:26:59.238216364 -0700
>> @@ -19,12 +19,12 @@ enum cpuid_regs_idx {
>>  	CPUID_EDX,
>>  };
>>  
>> -#define CPUID_MWAIT_LEAF	0x5
>> -#define CPUID_DCA_LEAF		0x9
>> -#define XSTATE_CPUID		0x0d
>> -#define CPUID_TSC_LEAF		0x15
>> -#define CPUID_FREQ_LEAF		0x16
>> -#define TILE_CPUID		0x1d
>> +#define CPUID_LEAF_MWAIT	0x5
>> +#define CPUID_LEAF_DCA		0x9
>> +#define CPUID_LEAF_XSTATE	0x0d
>> +#define CPUID_LEAF_TSC		0x15
>> +#define CPUID_LEAF_FREQ		0x16
>> +#define CPUID_LEAF_TILE		0x1d
> 
> ... and just to confuse things even more, there's enum cpuid_leafs too which
> start with the "CPUID_" prefix too.
> 
> Pfff.

Yeah, lovely.  'enum cpuid_leafs' does appear misnamed though.  It is a
list of *words*, not actual leaf numbers.  There's also very little
overlap between those leafs and the newly-renamed ones in this series.
I think that's because most of the leaves we dump into the CPU caps have
random feature bits that aren't logically grouped and resist naming.

The one exception to that is the CPUID_D_1_EAX aka. CPUID_LEAF_XSTATE.
We could do something like the attached patch, but I don't think it
really helps much.

> I'd like to unify them and I *think* kvm_cpu_cap_mask() should be able to
> stomach that (or fixed if not)...

Do you mean we could unify the CPUID_8000_0001_EDX enum values and the
CPUID_LEAF_* defines from this series?

I'm not quite sure how that would look.  I think we'd end up doing
something like:

#define CPUID_LEAF_C000_0001 	0xC0000001

and we'd still need some macro magic to munge the word "number" into
there, like:

#define FOO(x,reg) ((x)<<2 | CPUID_##reg)

that could be used this way:

enum cpuid_leafs
{
	...
        FOO(CPUID_LEAF_C000_0001, EDX),

It would let us do stuff like this:

-        case 0xC0000001:
+        case CPUID_LEAF_C000_0001:
                cpuid_entry_override(entry, CPUID_C000_0001_EDX);
                break;

But I'm not sure that makes things all that much more readable or greppable.

[-- Attachment #2: leafd.patch --]
[-- Type: text/x-patch, Size: 550 bytes --]

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index a5f221ea5688..b44dbb952d8c 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1001,8 +1001,8 @@ void get_cpu_cap(struct cpuinfo_x86 *c)
 	}
 
 	/* Extended state features: level 0x0000000d */
-	if (c->cpuid_level >= 0x0000000d) {
-		cpuid_count(0x0000000d, 1, &eax, &ebx, &ecx, &edx);
+	if (c->cpuid_level >= CPUID_LEAF_XSTATE) {
+		cpuid_count(CPUID_LEAF_XSTATE, 1, &eax, &ebx, &ecx, &edx);
 
 		c->x86_capability[CPUID_D_1_EAX] = eax;
 	}