| Message ID | 20240517173926.965351-23-seanjc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: CPUID overhaul, fixes, and caching | expand |
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.
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) );
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(-)