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 > >
