Re: [PATCH v2 22/49] KVM: x86: Add a macro to precisely handle aliased 0x1.EDX CPUID features

[Sean Christopherson <seanjc@google.com>]

On Thu, Nov 21, 2024, Maxim Levitsky wrote:
> On Wed, 2024-09-11 at 08:37 -0700, Sean Christopherson wrote:
> > On Tue, Sep 10, 2024, Maxim Levitsky wrote:
> > > On Mon, 2024-08-05 at 15:00 -0700, Sean Christopherson wrote:
> > > > At that point, I'm ok with defining each alias, though I honestly still don't
> > > > understand the motivation for defining single-use macros.
> > > > 
> > > 
> > > The idea is that nobody will need to look at these macros
> > > (e.g__X86_FEATURE_8000_0001_ALIAS() and its usages), because it's clear what
> > > they do, they just define few extra CPUID features that nobody really cares
> > > about.
> > > 
> > > ALIASED_F() on the other hand is yet another _F macro() and we will need,
> > > once again and again to figure out why it is there, what it does, etc.
> > 
> > That seems easily solved by naming the macro ALIASED_8000_0001_F().  I don't see
> > how that's any less clear than __X86_FEATURE_8000_0001_ALIAS(), and as above,
> > there are several advantages to defining the alias in the context of the leaf
> > builder.
> > 
> 
> Hi!
> 
> I am stating my point again: Treating 8000_0001 leaf aliases as regular CPUID
> features means that we don't need common code to deal with this, and thus
> when someone reads the common code (and this is the thing I care about the
> most) that someone won't need to dig up the info about what these aliases
> are. 

Ah, this is where we disagree, I think.  I feel quite strongly that oddities such
as aliased/duplicate CPUID feature bits need to be made as visible as possible,
and well documented.  Hiding architectural quirks might save some readers a few
seconds of their time, but it can also confuse others, and more importantly, makes
it more difficult for new readers/developers to learn about the quirks.

This code _looks_ wrong, as there's no indication that CPUID_8000_0001_EDX is
unique.  I too wasn't aware of the aliases until this series, and I was very
confused by KVM's code.  The only clue that I was given was the "Don't duplicate
feature flags which are redundant with Intel!" comment in cpufeatures.h; I still
ended up digging through the APM to understand what was going on.

	kvm_cpu_cap_mask(CPUID_1_EDX,
		F(FPU) | F(VME) | F(DE) | F(PSE) |
		F(TSC) | F(MSR) | F(PAE) | F(MCE) |
		F(CX8) | F(APIC) | 0 /* Reserved */ | F(SEP) |
		F(MTRR) | F(PGE) | F(MCA) | F(CMOV) |
		F(PAT) | F(PSE36) | 0 /* PSN */ | F(CLFLUSH) |
		0 /* Reserved, DS, ACPI */ | F(MMX) |
		F(FXSR) | F(XMM) | F(XMM2) | F(SELFSNOOP) |
		0 /* HTT, TM, Reserved, PBE */
	);

	kvm_cpu_cap_mask(CPUID_8000_0001_EDX,
		F(FPU) | F(VME) | F(DE) | F(PSE) |
		F(TSC) | F(MSR) | F(PAE) | F(MCE) |
		F(CX8) | F(APIC) | 0 /* Reserved */ | F(SYSCALL) |
		F(MTRR) | F(PGE) | F(MCA) | F(CMOV) |
		F(PAT) | F(PSE36) | 0 /* Reserved */ |
		F(NX) | 0 /* Reserved */ | F(MMXEXT) | F(MMX) |
		F(FXSR) | F(FXSR_OPT) | f_gbpages | F(RDTSCP) |
		0 /* Reserved */ | f_lm | F(3DNOWEXT) | F(3DNOW)
	);

Versus this code, which hopefully elicits a "huh!?" and prompts curious readers
to go look at the definition of ALIASED_1_EDX_F() to understand why KVM is being
weird.  And if readers can't figure things out purely from ALIASED_1_EDX_F()'s
comment, then that's effectively a KVM documentation issue and should be fixed.

In other words, I want to make things like this stick out so that more developers
are aware of such quirks, i.e. to to minimize the probability of such knowledge
being lost.  I don't want the next generation of KVM developers to have to
re-discover things that can be solved by a moderately verbose comment.

	kvm_cpu_cap_init(CPUID_1_EDX,
		F(FPU),
		F(VME),
		F(DE),
		F(PSE),
		F(TSC),
		F(MSR),
		F(PAE),
		F(MCE),
		F(CX8),
		F(APIC),
		...
	);

	kvm_cpu_cap_init(CPUID_8000_0001_EDX,
		ALIASED_1_EDX_F(FPU),
		ALIASED_1_EDX_F(VME),
		ALIASED_1_EDX_F(DE),
		ALIASED_1_EDX_F(PSE),
		ALIASED_1_EDX_F(TSC),
		ALIASED_1_EDX_F(MSR),
		ALIASED_1_EDX_F(PAE),
		ALIASED_1_EDX_F(MCE),
		ALIASED_1_EDX_F(CX8),
		ALIASED_1_EDX_F(APIC),
		...
	);

> I for example didn't knew about them because these aliases are basically a
> result of AMD redoing some things in the spec their way when they just
> released first 64-bit extensions.  I didn't follow the x86 ISA closely back
> then (I only had 32 bit systems to play with).
> 
> Best regards,
> 	Maxim Levitsky
> 
>