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

[Sean Christopherson]

On Tue, Sep 10, 2024, Maxim Levitsky wrote:
> On Mon, 2024-08-05 at 15:00 -0700, Sean Christopherson wrote:
> > If we go with ALIASED_F() (or ALIASED_8000_0001_F()), then that macro is all that
> > is needed, and it's bulletproof.  E.g. there is no KVM_X86_FEATURE_FPU_ALIAS that
> > can be queried, and thus no need to be ensure it's defined in cpuid.c and #undef'd
> > after its use.
> > 
> > Hmm, I supposed we could harden the aliased feature usage in the same way as the
> > ALIASED_F(), e.g.
> > 
> >   #define __X86_FEATURE_8000_0001_ALIAS(feature)				\
> >   ({										\
> > 	BUILD_BUG_ON(__feature_leaf(X86_FEATURE_##name) != CPUID_1_EDX);	\
> > 	BUILD_BUG_ON(kvm_cpu_cap_init_in_progress != CPUID_8000_0001_EDX);	\
> > 	(feature + (CPUID_8000_0001_EDX - CPUID_1_EDX) * 32);			\
> >   })
> > 
> > If something tries to use an X86_FEATURE_*_ALIAS outside if kvm_cpu_cap_init(),
> > it would need to define and set kvm_cpu_cap_init_in_progress, i.e. would really
> > have to try to mess up.
> > 
> > Effectively the only differences are that KVM would have ~10 or so more lines of
> > code to define the X86_FEATURE_*_ALIAS macros, and that the usage would look like:
> > 
> > 	VIRTUALIZED_F(FPU_ALIAS)
> > 
> > versus
> > 
> > 	ALIASED_F(FPU)
> 
> 
> This is exactly my point. I want to avoid profiliation of the _F macros, because
> later, we will need to figure out what each of them (e.g ALIASED_F) does.
> 
> A whole leaf alias, is once in x86 arch life misfeature, and it is very likely that
> Intel/AMD won't add more such aliases.
> 
> Why VIRTUALIZED_F though, it wasn't in the patch series? Normal F() should be enough
> IMHO.

I'm a-ok with F(), I simply thought there was a desire for more verbosity across
the board.

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