Add a macro to precisely handle CPUID features that AMD duplicated from
CPUID.0x1.EDX into CPUID.0x8000_0001.EDX.  This will allow adding an
assert that all features passed to kvm_cpu_cap_init() match the word being
processed, e.g. to prevent passing a feature from CPUID 0x7 to CPUID 0x1.

Because the kernel simply reuses the X86_FEATURE_* definitions from
CPUID.0x1.EDX, KVM's use of the aliased features would result in false
positives from such an assert.

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/cpuid.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 5e3b97d06374..f2bd2f5c4ea3 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -88,6 +88,16 @@ u32 xstate_required_size(u64 xstate_bv, bool compacted)
 	F(name);						\
 })
 
+/*
+ * Aliased Features - For features in 0x8000_0001.EDX that are duplicates of
+ * identical 0x1.EDX features, and thus are aliased from 0x1 to 0x8000_0001.
+ */
+#define AF(name)								\
+({										\
+	BUILD_BUG_ON(__feature_leaf(X86_FEATURE_##name) != CPUID_1_EDX);	\
+	feature_bit(name);							\
+})
+
 /*
  * Magic value used by KVM when querying userspace-provided CPUID entries and
  * doesn't care about the CPIUD index because the index of the function in
@@ -758,13 +768,13 @@ void kvm_set_cpu_caps(void)
 	);
 
 	kvm_cpu_cap_init(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) | X86_64_F(GBPAGES) | F(RDTSCP) |
+		AF(FPU) | AF(VME) | AF(DE) | AF(PSE) |
+		AF(TSC) | AF(MSR) | AF(PAE) | AF(MCE) |
+		AF(CX8) | AF(APIC) | 0 /* Reserved */ | F(SYSCALL) |
+		AF(MTRR) | AF(PGE) | AF(MCA) | AF(CMOV) |
+		AF(PAT) | AF(PSE36) | 0 /* Reserved */ |
+		F(NX) | 0 /* Reserved */ | F(MMXEXT) | AF(MMX) |
+		AF(FXSR) | F(FXSR_OPT) | X86_64_F(GBPAGES) | F(RDTSCP) |
 		0 /* Reserved */ | X86_64_F(LM) | F(3DNOWEXT) | F(3DNOW)
 	);
 
-- 
2.45.0.215.g3402c0e53f-goog

On Fri, 2024-05-17 at 10:38 -0700, Sean Christopherson wrote:
> Add a macro to precisely handle CPUID features that AMD duplicated from
> CPUID.0x1.EDX into CPUID.0x8000_0001.EDX.  This will allow adding an
> assert that all features passed to kvm_cpu_cap_init() match the word being
> processed, e.g. to prevent passing a feature from CPUID 0x7 to CPUID 0x1.
> 
> Because the kernel simply reuses the X86_FEATURE_* definitions from
> CPUID.0x1.EDX, KVM's use of the aliased features would result in false
> positives from such an assert.
> 
> No functional change intended.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/cpuid.c | 24 +++++++++++++++++-------
>  1 file changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 5e3b97d06374..f2bd2f5c4ea3 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -88,6 +88,16 @@ u32 xstate_required_size(u64 xstate_bv, bool compacted)
>  	F(name);						\
>  })
>  
> +/*
> + * Aliased Features - For features in 0x8000_0001.EDX that are duplicates of
> + * identical 0x1.EDX features, and thus are aliased from 0x1 to 0x8000_0001.
> + */
> +#define AF(name)								\
> +({										\
> +	BUILD_BUG_ON(__feature_leaf(X86_FEATURE_##name) != CPUID_1_EDX);	\
> +	feature_bit(name);							\
> +})
> +
>  /*
>   * Magic value used by KVM when querying userspace-provided CPUID entries and
>   * doesn't care about the CPIUD index because the index of the function in
> @@ -758,13 +768,13 @@ void kvm_set_cpu_caps(void)
>  	);
>  
>  	kvm_cpu_cap_init(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) | X86_64_F(GBPAGES) | F(RDTSCP) |
> +		AF(FPU) | AF(VME) | AF(DE) | AF(PSE) |
> +		AF(TSC) | AF(MSR) | AF(PAE) | AF(MCE) |
> +		AF(CX8) | AF(APIC) | 0 /* Reserved */ | F(SYSCALL) |
> +		AF(MTRR) | AF(PGE) | AF(MCA) | AF(CMOV) |
> +		AF(PAT) | AF(PSE36) | 0 /* Reserved */ |
> +		F(NX) | 0 /* Reserved */ | F(MMXEXT) | AF(MMX) |
> +		AF(FXSR) | F(FXSR_OPT) | X86_64_F(GBPAGES) | F(RDTSCP) |
>  		0 /* Reserved */ | X86_64_F(LM) | F(3DNOWEXT) | F(3DNOW)
>  	);
>  

Hi,

What if we defined the aliased features instead.
Something like this:

#define __X86_FEATURE_8000_0001_ALIAS(feature) \
	(feature + (CPUID_8000_0001_EDX - CPUID_1_EDX) * 32)

#define KVM_X86_FEATURE_FPU_ALIAS	__X86_FEATURE_8000_0001_ALIAS(KVM_X86_FEATURE_FPU)
#define KVM_X86_FEATURE_VME_ALIAS	__X86_FEATURE_8000_0001_ALIAS(KVM_X86_FEATURE_VME)

And then just use for example the 'F(FPU_ALIAS)' in the CPUID_8000_0001_EDX


Best regards,
	Maxim Levitsky

On Thu, Jul 04, 2024, Maxim Levitsky wrote:
> On Fri, 2024-05-17 at 10:38 -0700, Sean Christopherson wrote:
> > Add a macro to precisely handle CPUID features that AMD duplicated from
> > CPUID.0x1.EDX into CPUID.0x8000_0001.EDX.  This will allow adding an
> > assert that all features passed to kvm_cpu_cap_init() match the word being
> > processed, e.g. to prevent passing a feature from CPUID 0x7 to CPUID 0x1.
> > 
> > Because the kernel simply reuses the X86_FEATURE_* definitions from
> > CPUID.0x1.EDX, KVM's use of the aliased features would result in false
> > positives from such an assert.
> > 
> > No functional change intended.
> > 
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > ---
> >  arch/x86/kvm/cpuid.c | 24 +++++++++++++++++-------
> >  1 file changed, 17 insertions(+), 7 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > index 5e3b97d06374..f2bd2f5c4ea3 100644
> > --- a/arch/x86/kvm/cpuid.c
> > +++ b/arch/x86/kvm/cpuid.c
> > @@ -88,6 +88,16 @@ u32 xstate_required_size(u64 xstate_bv, bool compacted)
> >  	F(name);						\
> >  })
> >  
> > +/*
> > + * Aliased Features - For features in 0x8000_0001.EDX that are duplicates of
> > + * identical 0x1.EDX features, and thus are aliased from 0x1 to 0x8000_0001.
> > + */
> > +#define AF(name)								\
> > +({										\
> > +	BUILD_BUG_ON(__feature_leaf(X86_FEATURE_##name) != CPUID_1_EDX);	\
> > +	feature_bit(name);							\
> > +})
> > +
> >  /*
> >   * Magic value used by KVM when querying userspace-provided CPUID entries and
> >   * doesn't care about the CPIUD index because the index of the function in
> > @@ -758,13 +768,13 @@ void kvm_set_cpu_caps(void)
> >  	);
> >  
> >  	kvm_cpu_cap_init(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) | X86_64_F(GBPAGES) | F(RDTSCP) |
> > +		AF(FPU) | AF(VME) | AF(DE) | AF(PSE) |
> > +		AF(TSC) | AF(MSR) | AF(PAE) | AF(MCE) |
> > +		AF(CX8) | AF(APIC) | 0 /* Reserved */ | F(SYSCALL) |
> > +		AF(MTRR) | AF(PGE) | AF(MCA) | AF(CMOV) |
> > +		AF(PAT) | AF(PSE36) | 0 /* Reserved */ |
> > +		F(NX) | 0 /* Reserved */ | F(MMXEXT) | AF(MMX) |
> > +		AF(FXSR) | F(FXSR_OPT) | X86_64_F(GBPAGES) | F(RDTSCP) |
> >  		0 /* Reserved */ | X86_64_F(LM) | F(3DNOWEXT) | F(3DNOW)
> >  	);
> >  
> 
> Hi,
> 
> What if we defined the aliased features instead.
> Something like this:
> 
> #define __X86_FEATURE_8000_0001_ALIAS(feature) \
> 	(feature + (CPUID_8000_0001_EDX - CPUID_1_EDX) * 32)
> 
> #define KVM_X86_FEATURE_FPU_ALIAS	__X86_FEATURE_8000_0001_ALIAS(KVM_X86_FEATURE_FPU)
> #define KVM_X86_FEATURE_VME_ALIAS	__X86_FEATURE_8000_0001_ALIAS(KVM_X86_FEATURE_VME)
> 
> And then just use for example the 'F(FPU_ALIAS)' in the CPUID_8000_0001_EDX

At first glance, I really liked this idea, but after working through the
ramifications, I think I prefer "converting" the flag when passing it to
kvm_cpu_cap_init().  In-place conversion makes it all but impossible for KVM to
check the alias, e.g. via guest_cpu_cap_has(), especially since the AF() macro
doesn't set the bits in kvm_known_cpu_caps (if/when a non-hacky validation of
usage becomes reality).

Side topic, if it's not already documented somewhere else, kvm/x86/cpuid.rst
should call out that KVM only honors the features in CPUID.0x1, i.e. that setting
aliased bits in CPUID.0x8000_0001 is supported if and only if the bit(s) is also
set in CPUID.0x1.

On Mon, 2024-07-08 at 14:08 -0700, Sean Christopherson wrote:
> On Thu, Jul 04, 2024, Maxim Levitsky wrote:
> > On Fri, 2024-05-17 at 10:38 -0700, Sean Christopherson wrote:
> > > Add a macro to precisely handle CPUID features that AMD duplicated from
> > > CPUID.0x1.EDX into CPUID.0x8000_0001.EDX.  This will allow adding an
> > > assert that all features passed to kvm_cpu_cap_init() match the word being
> > > processed, e.g. to prevent passing a feature from CPUID 0x7 to CPUID 0x1.
> > > 
> > > Because the kernel simply reuses the X86_FEATURE_* definitions from
> > > CPUID.0x1.EDX, KVM's use of the aliased features would result in false
> > > positives from such an assert.
> > > 
> > > No functional change intended.
> > > 
> > > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > > ---
> > >  arch/x86/kvm/cpuid.c | 24 +++++++++++++++++-------
> > >  1 file changed, 17 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > > index 5e3b97d06374..f2bd2f5c4ea3 100644
> > > --- a/arch/x86/kvm/cpuid.c
> > > +++ b/arch/x86/kvm/cpuid.c
> > > @@ -88,6 +88,16 @@ u32 xstate_required_size(u64 xstate_bv, bool compacted)
> > >  	F(name);						\
> > >  })
> > >  
> > > +/*
> > > + * Aliased Features - For features in 0x8000_0001.EDX that are duplicates of
> > > + * identical 0x1.EDX features, and thus are aliased from 0x1 to 0x8000_0001.
> > > + */
> > > +#define AF(name)								\
> > > +({										\
> > > +	BUILD_BUG_ON(__feature_leaf(X86_FEATURE_##name) != CPUID_1_EDX);	\
> > > +	feature_bit(name);							\
> > > +})
> > > +
> > >  /*
> > >   * Magic value used by KVM when querying userspace-provided CPUID entries and
> > >   * doesn't care about the CPIUD index because the index of the function in
> > > @@ -758,13 +768,13 @@ void kvm_set_cpu_caps(void)
> > >  	);
> > >  
> > >  	kvm_cpu_cap_init(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) | X86_64_F(GBPAGES) | F(RDTSCP) |
> > > +		AF(FPU) | AF(VME) | AF(DE) | AF(PSE) |
> > > +		AF(TSC) | AF(MSR) | AF(PAE) | AF(MCE) |
> > > +		AF(CX8) | AF(APIC) | 0 /* Reserved */ | F(SYSCALL) |
> > > +		AF(MTRR) | AF(PGE) | AF(MCA) | AF(CMOV) |
> > > +		AF(PAT) | AF(PSE36) | 0 /* Reserved */ |
> > > +		F(NX) | 0 /* Reserved */ | F(MMXEXT) | AF(MMX) |
> > > +		AF(FXSR) | F(FXSR_OPT) | X86_64_F(GBPAGES) | F(RDTSCP) |
> > >  		0 /* Reserved */ | X86_64_F(LM) | F(3DNOWEXT) | F(3DNOW)
> > >  	);
> > >  
> > 
> > Hi,
> > 
> > What if we defined the aliased features instead.
> > Something like this:
> > 
> > #define __X86_FEATURE_8000_0001_ALIAS(feature) \
> > 	(feature + (CPUID_8000_0001_EDX - CPUID_1_EDX) * 32)
> > 
> > #define KVM_X86_FEATURE_FPU_ALIAS	__X86_FEATURE_8000_0001_ALIAS(KVM_X86_FEATURE_FPU)
> > #define KVM_X86_FEATURE_VME_ALIAS	__X86_FEATURE_8000_0001_ALIAS(KVM_X86_FEATURE_VME)
> > 
> > And then just use for example the 'F(FPU_ALIAS)' in the CPUID_8000_0001_EDX
> 
> At first glance, I really liked this idea, but after working through the
> ramifications, I think I prefer "converting" the flag when passing it to
> kvm_cpu_cap_init().  In-place conversion makes it all but impossible for KVM to
> check the alias, e.g. via guest_cpu_cap_has(), especially since the AF() macro
> doesn't set the bits in kvm_known_cpu_caps (if/when a non-hacky validation of
> usage becomes reality).

Could you elaborate on this as well?

My suggestion was that we can just treat aliases as completely independent and dummy features,
say KVM_X86_FEATURE_FPU_ALIAS, and pass them as is to the guest, which means that
if an alias is present in host cpuid, it appears in kvm caps, and thus qemu can then
set it in guest cpuid.

I don't think that we need any special treatment for them if you look at it this way.
If you don't agree, can you give me an example?


> 
> Side topic, if it's not already documented somewhere else, kvm/x86/cpuid.rst
> should call out that KVM only honors the features in CPUID.0x1, i.e. that setting
> aliased bits in CPUID.0x8000_0001 is supported if and only if the bit(s) is also
> set in CPUID.0x1.

To be honest if KVM enforces this, such enforcement can be removed IMHO:

KVM already allows all kinds of totally invalid
CPUIDs to be set by the guest, for example a CPUID in which AVX3 is set, and AVX and/or XSAVE is not set.

So having a guest given cpuid where aliased feature is set, and regular feature is not set,
should not pose any problem to KVM itself, as long as KVM itself uses only the non-aliased
features as the ground truth.

Since such configuration is an error anyway, allowing it won't break any existing users IMHO.

What do you think about this? If you don't agree, can you provide an example of a breakage?


Best regards,
	Maxim Levitsky

>

On Wed, Jul 24, 2024, Maxim Levitsky wrote:
> On Mon, 2024-07-08 at 14:08 -0700, Sean Christopherson wrote:
> > On Thu, Jul 04, 2024, Maxim Levitsky wrote:
> > > On Fri, 2024-05-17 at 10:38 -0700, Sean Christopherson wrote:
> > > > Add a macro to precisely handle CPUID features that AMD duplicated from
> > > > CPUID.0x1.EDX into CPUID.0x8000_0001.EDX.  This will allow adding an
> > > > assert that all features passed to kvm_cpu_cap_init() match the word being
> > > > processed, e.g. to prevent passing a feature from CPUID 0x7 to CPUID 0x1.
> > > > 
> > > > Because the kernel simply reuses the X86_FEATURE_* definitions from
> > > > CPUID.0x1.EDX, KVM's use of the aliased features would result in false
> > > > positives from such an assert.
> > > > 
> > > > No functional change intended.
> > > > 
> > > > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > > > ---
> > > >  arch/x86/kvm/cpuid.c | 24 +++++++++++++++++-------
> > > >  1 file changed, 17 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > > > index 5e3b97d06374..f2bd2f5c4ea3 100644
> > > > --- a/arch/x86/kvm/cpuid.c
> > > > +++ b/arch/x86/kvm/cpuid.c
> > > > @@ -88,6 +88,16 @@ u32 xstate_required_size(u64 xstate_bv, bool compacted)
> > > >  	F(name);						\
> > > >  })
> > > >  
> > > > +/*
> > > > + * Aliased Features - For features in 0x8000_0001.EDX that are duplicates of
> > > > + * identical 0x1.EDX features, and thus are aliased from 0x1 to 0x8000_0001.
> > > > + */
> > > > +#define AF(name)								\
> > > > +({										\
> > > > +	BUILD_BUG_ON(__feature_leaf(X86_FEATURE_##name) != CPUID_1_EDX);	\
> > > > +	feature_bit(name);							\
> > > > +})
> > > > +
> > > >  /*
> > > >   * Magic value used by KVM when querying userspace-provided CPUID entries and
> > > >   * doesn't care about the CPIUD index because the index of the function in
> > > > @@ -758,13 +768,13 @@ void kvm_set_cpu_caps(void)
> > > >  	);
> > > >  
> > > >  	kvm_cpu_cap_init(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) | X86_64_F(GBPAGES) | F(RDTSCP) |
> > > > +		AF(FPU) | AF(VME) | AF(DE) | AF(PSE) |
> > > > +		AF(TSC) | AF(MSR) | AF(PAE) | AF(MCE) |
> > > > +		AF(CX8) | AF(APIC) | 0 /* Reserved */ | F(SYSCALL) |
> > > > +		AF(MTRR) | AF(PGE) | AF(MCA) | AF(CMOV) |
> > > > +		AF(PAT) | AF(PSE36) | 0 /* Reserved */ |
> > > > +		F(NX) | 0 /* Reserved */ | F(MMXEXT) | AF(MMX) |
> > > > +		AF(FXSR) | F(FXSR_OPT) | X86_64_F(GBPAGES) | F(RDTSCP) |
> > > >  		0 /* Reserved */ | X86_64_F(LM) | F(3DNOWEXT) | F(3DNOW)
> > > >  	);
> > > >  
> > > 
> > > Hi,
> > > 
> > > What if we defined the aliased features instead.
> > > Something like this:
> > > 
> > > #define __X86_FEATURE_8000_0001_ALIAS(feature) \
> > > 	(feature + (CPUID_8000_0001_EDX - CPUID_1_EDX) * 32)
> > > 
> > > #define KVM_X86_FEATURE_FPU_ALIAS	__X86_FEATURE_8000_0001_ALIAS(KVM_X86_FEATURE_FPU)
> > > #define KVM_X86_FEATURE_VME_ALIAS	__X86_FEATURE_8000_0001_ALIAS(KVM_X86_FEATURE_VME)
> > > 
> > > And then just use for example the 'F(FPU_ALIAS)' in the CPUID_8000_0001_EDX
> > 
> > At first glance, I really liked this idea, but after working through the
> > ramifications, I think I prefer "converting" the flag when passing it to
> > kvm_cpu_cap_init().  In-place conversion makes it all but impossible for KVM to
> > check the alias, e.g. via guest_cpu_cap_has(), especially since the AF() macro
> > doesn't set the bits in kvm_known_cpu_caps (if/when a non-hacky validation of
> > usage becomes reality).
> 
> Could you elaborate on this as well?
> 
> My suggestion was that we can just treat aliases as completely independent
> and dummy features, say KVM_X86_FEATURE_FPU_ALIAS, and pass them as is to the
> guest, which means that if an alias is present in host cpuid, it appears in
> kvm caps, and thus qemu can then set it in guest cpuid.
> 
> I don't think that we need any special treatment for them if you look at it
> this way.  If you don't agree, can you give me an example?

KVM doesn't honor the aliases beyond telling userspace they can be set (see below
for all the aliased features that KVM _should_ be checking).  The APM clearly
states that the features are the same as their CPUID.0x1 counterparts, but Intel
CPUs don't support the aliases.  So, as you also note below, I think we could
unequivocally say that enumerating the aliases but not the "real" features is a
bogus CPUID model, but we can't say the opposite, i.e. the real features can
exists without the aliases.

And that means that KVM must never query the aliases, e.g. should never do
guest_cpu_cap_has(KVM_X86_FEATURE_FPU_ALIAS), because the result is essentially
meaningless.  It's a small thing, but if KVM_X86_FEATURE_FPU_ALIAS simply doesn't
exist, i.e. we do in-place conversion, then it's impossible to feed the aliases
into things like guest_cpu_cap_has().

Heh, on a related topic, __cr4_reserved_bits() fails to account for any of the
aliased features.  Unless I'm missing something, VME, DE, TSC, PSE, PAE, PGE and
MCE, all need to be handled in __cr4_reserved_bits().  Amusingly, 
nested_vmx_cr_fixed1_bits_update() handles the aliased legacy features.  I don't
see any reason for nested_vmx_cr_fixed1_bits_update() to manually query guest
CPUID, it should be able to use cr4_guest_rsvd_bits verbatim.

> > Side topic, if it's not already documented somewhere else, kvm/x86/cpuid.rst
> > should call out that KVM only honors the features in CPUID.0x1, i.e. that setting
> > aliased bits in CPUID.0x8000_0001 is supported if and only if the bit(s) is also
> > set in CPUID.0x1.
> 
> To be honest if KVM enforces this, such enforcement can be removed IMHO:

There's no enforcement, and as above I agree that this would be a bogus CPUID
model.  I was thinking that it could be helpful to document that KVM never checks
the aliases, but on second though, it's probably unnecessary because the APM does
say

  Same as CPUID Fn0000_0001_EDX[...]

for all the bits, i.e. setting the aliases without the real bits is an
architectural violation.

У чт, 2024-07-25 у 11:39 -0700, Sean Christopherson пише:
> > On Wed, Jul 24, 2024, Maxim Levitsky wrote:
> > > > On Mon, 2024-07-08 at 14:08 -0700, Sean Christopherson wrote:
> > > > > > On Thu, Jul 04, 2024, Maxim Levitsky wrote:
> > > > > > > > On Fri, 2024-05-17 at 10:38 -0700, Sean Christopherson wrote:
> > > > > > > > > > Add a macro to precisely handle CPUID features that AMD duplicated from
> > > > > > > > > > CPUID.0x1.EDX into CPUID.0x8000_0001.EDX.  This will allow adding an
> > > > > > > > > > assert that all features passed to kvm_cpu_cap_init() match the word being
> > > > > > > > > > processed, e.g. to prevent passing a feature from CPUID 0x7 to CPUID 0x1.
> > > > > > > > > > 
> > > > > > > > > > Because the kernel simply reuses the X86_FEATURE_* definitions from
> > > > > > > > > > CPUID.0x1.EDX, KVM's use of the aliased features would result in false
> > > > > > > > > > positives from such an assert.
> > > > > > > > > > 
> > > > > > > > > > No functional change intended.
> > > > > > > > > > 
> > > > > > > > > > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > > > > > > > > > ---
> > > > > > > > > >  arch/x86/kvm/cpuid.c | 24 +++++++++++++++++-------
> > > > > > > > > >  1 file changed, 17 insertions(+), 7 deletions(-)
> > > > > > > > > > 
> > > > > > > > > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > > > > > > > > > index 5e3b97d06374..f2bd2f5c4ea3 100644
> > > > > > > > > > --- a/arch/x86/kvm/cpuid.c
> > > > > > > > > > +++ b/arch/x86/kvm/cpuid.c
> > > > > > > > > > @@ -88,6 +88,16 @@ u32 xstate_required_size(u64 xstate_bv, bool compacted)
> > > > > > > > > >         F(name);                                                \
> > > > > > > > > >  })
> > > > > > > > > >  
> > > > > > > > > > +/*
> > > > > > > > > > + * Aliased Features - For features in 0x8000_0001.EDX that are duplicates of
> > > > > > > > > > + * identical 0x1.EDX features, and thus are aliased from 0x1 to 0x8000_0001.
> > > > > > > > > > + */
> > > > > > > > > > +#define AF(name)                                                               \
> > > > > > > > > > +({                                                                             \
> > > > > > > > > > +       BUILD_BUG_ON(__feature_leaf(X86_FEATURE_##name) != CPUID_1_EDX);        \
> > > > > > > > > > +       feature_bit(name);                                                      \
> > > > > > > > > > +})
> > > > > > > > > > +
> > > > > > > > > >  /*
> > > > > > > > > >   * Magic value used by KVM when querying userspace-provided CPUID entries and
> > > > > > > > > >   * doesn't care about the CPIUD index because the index of the function in
> > > > > > > > > > @@ -758,13 +768,13 @@ void kvm_set_cpu_caps(void)
> > > > > > > > > >         );
> > > > > > > > > >  
> > > > > > > > > >         kvm_cpu_cap_init(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) | X86_64_F(GBPAGES) | F(RDTSCP) |
> > > > > > > > > > +               AF(FPU) | AF(VME) | AF(DE) | AF(PSE) |
> > > > > > > > > > +               AF(TSC) | AF(MSR) | AF(PAE) | AF(MCE) |
> > > > > > > > > > +               AF(CX8) | AF(APIC) | 0 /* Reserved */ | F(SYSCALL) |
> > > > > > > > > > +               AF(MTRR) | AF(PGE) | AF(MCA) | AF(CMOV) |
> > > > > > > > > > +               AF(PAT) | AF(PSE36) | 0 /* Reserved */ |
> > > > > > > > > > +               F(NX) | 0 /* Reserved */ | F(MMXEXT) | AF(MMX) |
> > > > > > > > > > +               AF(FXSR) | F(FXSR_OPT) | X86_64_F(GBPAGES) | F(RDTSCP) |
> > > > > > > > > >                 0 /* Reserved */ | X86_64_F(LM) | F(3DNOWEXT) | F(3DNOW)
> > > > > > > > > >         );
> > > > > > > > > >  
> > > > > > > > 
> > > > > > > > Hi,
> > > > > > > > 
> > > > > > > > What if we defined the aliased features instead.
> > > > > > > > Something like this:
> > > > > > > > 
> > > > > > > > #define __X86_FEATURE_8000_0001_ALIAS(feature) \
> > > > > > > >         (feature + (CPUID_8000_0001_EDX - CPUID_1_EDX) * 32)
> > > > > > > > 
> > > > > > > > #define KVM_X86_FEATURE_FPU_ALIAS       __X86_FEATURE_8000_0001_ALIAS(KVM_X86_FEATURE_FPU)
> > > > > > > > #define KVM_X86_FEATURE_VME_ALIAS       __X86_FEATURE_8000_0001_ALIAS(KVM_X86_FEATURE_VME)
> > > > > > > > 
> > > > > > > > And then just use for example the 'F(FPU_ALIAS)' in the CPUID_8000_0001_EDX
> > > > > > 
> > > > > > At first glance, I really liked this idea, but after working through the
> > > > > > ramifications, I think I prefer "converting" the flag when passing it to
> > > > > > kvm_cpu_cap_init().  In-place conversion makes it all but impossible for KVM to
> > > > > > check the alias, e.g. via guest_cpu_cap_has(), especially since the AF() macro
> > > > > > doesn't set the bits in kvm_known_cpu_caps (if/when a non-hacky validation of
> > > > > > usage becomes reality).
> > > > 
> > > > Could you elaborate on this as well?
> > > > 
> > > > My suggestion was that we can just treat aliases as completely independent
> > > > and dummy features, say KVM_X86_FEATURE_FPU_ALIAS, and pass them as is to the
> > > > guest, which means that if an alias is present in host cpuid, it appears in
> > > > kvm caps, and thus qemu can then set it in guest cpuid.
> > > > 
> > > > I don't think that we need any special treatment for them if you look at it
> > > > this way.  If you don't agree, can you give me an example?
> > 
> > KVM doesn't honor the aliases beyond telling userspace they can be set (see below
> > for all the aliased features that KVM _should_ be checking).  The APM clearly
> > states that the features are the same as their CPUID.0x1 counterparts, but Intel
> > CPUs don't support the aliases.  So, as you also note below, I think we could
> > unequivocally say that enumerating the aliases but not the "real" features is a
> > bogus CPUID model, but we can't say the opposite, i.e. the real features can
> > exists without the aliases.
> > 
> > And that means that KVM must never query the aliases, e.g. should never do
> > guest_cpu_cap_has(KVM_X86_FEATURE_FPU_ALIAS), because the result is essentially
> > meaningless.  It's a small thing, but if KVM_X86_FEATURE_FPU_ALIAS simply doesn't
> > exist, i.e. we do in-place conversion, then it's impossible to feed the aliases
> > into things like guest_cpu_cap_has().

This only makes my case stronger - treating the aliases as just features will
allow us to avoid adding more logic to code which is already too complex IMHO.

If your concern is that features could be queried by guest_cpu_cap_has()
that is easy to fix, we can (and should) put them into a separate file and
#include them only in cpuid.c.

We can even #undef the __X86_FEATURE_8000_0001_ALIAS macro after the kvm_set_cpu_caps,
then if I understand the macro pre-processor correctly, any use of feature alias
macros will not fully evaluate and cause a compile error.



> > 
> > Heh, on a related topic, __cr4_reserved_bits() fails to account for any of the
> > aliased features.  Unless I'm missing something, VME, DE, TSC, PSE, PAE, PGE and
> > MCE, all need to be handled in __cr4_reserved_bits(). 
> >  Amusingly, 
> > nested_vmx_cr_fixed1_bits_update() handles the aliased legacy features.  I don't
> > see any reason for nested_vmx_cr_fixed1_bits_update() to manually query guest
> > CPUID, it should be able to use cr4_guest_rsvd_bits verbatim.

Yep, this should be fixed - this patch series is about to grow even more I guess,
or rather let me suggest that you split it into several patch series, which
can be merged and discussed separately.


> > 
> > > > > > Side topic, if it's not already documented somewhere else, kvm/x86/cpuid.rst
> > > > > > should call out that KVM only honors the features in CPUID.0x1, i.e. that setting
> > > > > > aliased bits in CPUID.0x8000_0001 is supported if and only if the bit(s) is also
> > > > > > set in CPUID.0x1.
> > > > 
> > > > To be honest if KVM enforces this, such enforcement can be removed IMHO:
> > 
> > There's no enforcement, and as above I agree that this would be a bogus CPUID
> > model.  I was thinking that it could be helpful to document that KVM never checks
> > the aliases, but on second though, it's probably unnecessary because the APM does
> > say
> > 
> >   Same as CPUID Fn0000_0001_EDX[...]
> > 
> > for all the bits, i.e. setting the aliases without the real bits is an
> > architectural violation.

Regardless if this is an architectural violation or not, KVM should allow this
because it allows many architectural violations, like AVX3 with no XSAVE, and such.

IMHO being consistent is more important than being right in only some cases,
and I don't think we want to start enforcing all the CPUID dependencies
(I actually won't object to this).

Best regards,
	Maxim Levitsky


> > 

On Mon, Aug 05, 2024, mlevitsk@redhat.com wrote:
> У чт, 2024-07-25 у 11:39 -0700, Sean Christopherson пише:
> > > On Wed, Jul 24, 2024, Maxim Levitsky wrote:
> > > > > On Mon, 2024-07-08 at 14:08 -0700, Sean Christopherson wrote:
> > > > > > > On Thu, Jul 04, 2024, Maxim Levitsky wrote:
> > > > > > > > > What if we defined the aliased features instead.
> > > > > > > > > Something like this:
> > > > > > > > > 
> > > > > > > > > #define __X86_FEATURE_8000_0001_ALIAS(feature) \
> > > > > > > > >         (feature + (CPUID_8000_0001_EDX - CPUID_1_EDX) * 32)
> > > > > > > > > 
> > > > > > > > > #define KVM_X86_FEATURE_FPU_ALIAS       __X86_FEATURE_8000_0001_ALIAS(KVM_X86_FEATURE_FPU)
> > > > > > > > > #define KVM_X86_FEATURE_VME_ALIAS       __X86_FEATURE_8000_0001_ALIAS(KVM_X86_FEATURE_VME)
> > > > > > > > > 
> > > > > > > > > And then just use for example the 'F(FPU_ALIAS)' in the CPUID_8000_0001_EDX
> > > > > > > 
> > > > > > > At first glance, I really liked this idea, but after working through the
> > > > > > > ramifications, I think I prefer "converting" the flag when passing it to
> > > > > > > kvm_cpu_cap_init().  In-place conversion makes it all but impossible for KVM to
> > > > > > > check the alias, e.g. via guest_cpu_cap_has(), especially since the AF() macro
> > > > > > > doesn't set the bits in kvm_known_cpu_caps (if/when a non-hacky validation of
> > > > > > > usage becomes reality).
> > > > > 
> > > > > Could you elaborate on this as well?
> > > > > 
> > > > > My suggestion was that we can just treat aliases as completely independent
> > > > > and dummy features, say KVM_X86_FEATURE_FPU_ALIAS, and pass them as is to the
> > > > > guest, which means that if an alias is present in host cpuid, it appears in
> > > > > kvm caps, and thus qemu can then set it in guest cpuid.
> > > > > 
> > > > > I don't think that we need any special treatment for them if you look at it
> > > > > this way.  If you don't agree, can you give me an example?
> > > 
> > > KVM doesn't honor the aliases beyond telling userspace they can be set (see below
> > > for all the aliased features that KVM _should_ be checking).  The APM clearly
> > > states that the features are the same as their CPUID.0x1 counterparts, but Intel
> > > CPUs don't support the aliases.  So, as you also note below, I think we could
> > > unequivocally say that enumerating the aliases but not the "real" features is a
> > > bogus CPUID model, but we can't say the opposite, i.e. the real features can
> > > exists without the aliases.
> > > 
> > > And that means that KVM must never query the aliases, e.g. should never do
> > > guest_cpu_cap_has(KVM_X86_FEATURE_FPU_ALIAS), because the result is essentially
> > > meaningless.  It's a small thing, but if KVM_X86_FEATURE_FPU_ALIAS simply doesn't
> > > exist, i.e. we do in-place conversion, then it's impossible to feed the aliases
> > > into things like guest_cpu_cap_has().
> 
> This only makes my case stronger - treating the aliases as just features will
> allow us to avoid adding more logic to code which is already too complex IMHO.
> 
> If your concern is that features could be queried by guest_cpu_cap_has()
> that is easy to fix, we can (and should) put them into a separate file and
> #include them only in cpuid.c.
> 
> We can even #undef the __X86_FEATURE_8000_0001_ALIAS macro after the kvm_set_cpu_caps,
> then if I understand the macro pre-processor correctly, any use of feature alias
> macros will not fully evaluate and cause a compile error.

I don't see how that's less code.  Either way, KVM needs a macro to handle aliases,
e.g. either we end up with ALIAS_F() or __X86_FEATURE_8000_0001_ALIAS().  For the
macros themselves, IMO they carry the same amount of complexity.

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)

At that point, I'm ok with defining each alias, though I honestly still don't
understand the motivation for defining single-use macros.

On Mon, 2024-08-05 at 15:00 -0700, Sean Christopherson wrote:
> On Mon, Aug 05, 2024, mlevitsk@redhat.com wrote:
> > У чт, 2024-07-25 у 11:39 -0700, Sean Christopherson пише:
> > > > On Wed, Jul 24, 2024, Maxim Levitsky wrote:
> > > > > > On Mon, 2024-07-08 at 14:08 -0700, Sean Christopherson wrote:
> > > > > > > > On Thu, Jul 04, 2024, Maxim Levitsky wrote:
> > > > > > > > > > What if we defined the aliased features instead.
> > > > > > > > > > Something like this:
> > > > > > > > > > 
> > > > > > > > > > #define __X86_FEATURE_8000_0001_ALIAS(feature) \
> > > > > > > > > >         (feature + (CPUID_8000_0001_EDX - CPUID_1_EDX) * 32)
> > > > > > > > > > 
> > > > > > > > > > #define KVM_X86_FEATURE_FPU_ALIAS       __X86_FEATURE_8000_0001_ALIAS(KVM_X86_FEATURE_FPU)
> > > > > > > > > > #define KVM_X86_FEATURE_VME_ALIAS       __X86_FEATURE_8000_0001_ALIAS(KVM_X86_FEATURE_VME)
> > > > > > > > > > 
> > > > > > > > > > And then just use for example the 'F(FPU_ALIAS)' in the CPUID_8000_0001_EDX
> > > > > > > > 
> > > > > > > > At first glance, I really liked this idea, but after working through the
> > > > > > > > ramifications, I think I prefer "converting" the flag when passing it to
> > > > > > > > kvm_cpu_cap_init().  In-place conversion makes it all but impossible for KVM to
> > > > > > > > check the alias, e.g. via guest_cpu_cap_has(), especially since the AF() macro
> > > > > > > > doesn't set the bits in kvm_known_cpu_caps (if/when a non-hacky validation of
> > > > > > > > usage becomes reality).
> > > > > > 
> > > > > > Could you elaborate on this as well?
> > > > > > 
> > > > > > My suggestion was that we can just treat aliases as completely independent
> > > > > > and dummy features, say KVM_X86_FEATURE_FPU_ALIAS, and pass them as is to the
> > > > > > guest, which means that if an alias is present in host cpuid, it appears in
> > > > > > kvm caps, and thus qemu can then set it in guest cpuid.
> > > > > > 
> > > > > > I don't think that we need any special treatment for them if you look at it
> > > > > > this way.  If you don't agree, can you give me an example?
> > > > 
> > > > KVM doesn't honor the aliases beyond telling userspace they can be set (see below
> > > > for all the aliased features that KVM _should_ be checking).  The APM clearly
> > > > states that the features are the same as their CPUID.0x1 counterparts, but Intel
> > > > CPUs don't support the aliases.  So, as you also note below, I think we could
> > > > unequivocally say that enumerating the aliases but not the "real" features is a
> > > > bogus CPUID model, but we can't say the opposite, i.e. the real features can
> > > > exists without the aliases.
> > > > 
> > > > And that means that KVM must never query the aliases, e.g. should never do
> > > > guest_cpu_cap_has(KVM_X86_FEATURE_FPU_ALIAS), because the result is essentially
> > > > meaningless.  It's a small thing, but if KVM_X86_FEATURE_FPU_ALIAS simply doesn't
> > > > exist, i.e. we do in-place conversion, then it's impossible to feed the aliases
> > > > into things like guest_cpu_cap_has().
> > 
> > This only makes my case stronger - treating the aliases as just features will
> > allow us to avoid adding more logic to code which is already too complex IMHO.
> > 
> > If your concern is that features could be queried by guest_cpu_cap_has()
> > that is easy to fix, we can (and should) put them into a separate file and
> > #include them only in cpuid.c.
> > 
> > We can even #undef the __X86_FEATURE_8000_0001_ALIAS macro after the kvm_set_cpu_caps,
> > then if I understand the macro pre-processor correctly, any use of feature alias
> > macros will not fully evaluate and cause a compile error.
> 
> I don't see how that's less code.  Either way, KVM needs a macro to handle aliases,
> e.g. either we end up with ALIAS_F() or __X86_FEATURE_8000_0001_ALIAS().  For the
> macros themselves, IMO they carry the same amount of complexity.
> 
> 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.


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

Best regards,
	Maxim Levitsky

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.

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

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. 

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

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