| Message ID | 20240517173926.965351-49-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:39 -0700, Sean Christopherson wrote: > Add yet another CPUID macro, this time for features that the host kernel > synthesizes into boot_cpu_data, i.e. that the kernel force sets even in > situations where the feature isn't reported by CPUID. Thanks to the > macro shenanigans of kvm_cpu_cap_init(), such features can now be handled > in the core CPUID framework, i.e. don't need to be handled out-of-band and > thus without as many guardrails. > > Adding a dedicated macro also helps document what's going on, e.g. the > calls to kvm_cpu_cap_check_and_set() are very confusing unless the reader > knows exactly how kvm_cpu_cap_init() generates kvm_cpu_caps (and even > then, it's far from obvious). > > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/kvm/cpuid.c | 22 ++++++++++++++++------ > 1 file changed, 16 insertions(+), 6 deletions(-) > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > index 0130e0677387..0e64a6332052 100644 > --- a/arch/x86/kvm/cpuid.c > +++ b/arch/x86/kvm/cpuid.c > @@ -106,6 +106,17 @@ u32 xstate_required_size(u64 xstate_bv, bool compacted) > F(name); \ > }) > > +/* > + * Synthesized Feature - For features that are synthesized into boot_cpu_data, > + * i.e. may not be present in the raw CPUID, but can still be advertised to > + * userspace. Primarily used for mitigation related feature flags. > + */ > +#define SYN_F(name) \ > +({ \ > + kvm_cpu_cap_synthesized |= F(name); \ > + 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. > @@ -727,13 +738,15 @@ do { \ > const struct cpuid_reg cpuid = x86_feature_cpuid(leaf * 32); \ > const u32 __maybe_unused kvm_cpu_cap_init_in_progress = leaf; \ > u32 kvm_cpu_cap_emulated = 0; \ > + u32 kvm_cpu_cap_synthesized = 0; \ > \ > if (leaf < NCAPINTS) \ > kvm_cpu_caps[leaf] &= (mask); \ > else \ > kvm_cpu_caps[leaf] = (mask); \ > \ > - kvm_cpu_caps[leaf] &= raw_cpuid_get(cpuid); \ > + kvm_cpu_caps[leaf] &= (raw_cpuid_get(cpuid) | \ > + kvm_cpu_cap_synthesized); \ > kvm_cpu_caps[leaf] |= kvm_cpu_cap_emulated; \ > } while (0) > > @@ -913,13 +926,10 @@ void kvm_set_cpu_caps(void) > kvm_cpu_cap_init(CPUID_8000_0021_EAX, > F(NO_NESTED_DATA_BP) | F(LFENCE_RDTSC) | 0 /* SmmPgCfgLock */ | > F(NULL_SEL_CLR_BASE) | F(AUTOIBRS) | 0 /* PrefetchCtlMsr */ | > - F(WRMSR_XX_BASE_NS) > + F(WRMSR_XX_BASE_NS) | SYN_F(SBPB) | SYN_F(IBPB_BRTYPE) | > + SYN_F(SRSO_NO) > ); > > - kvm_cpu_cap_check_and_set(X86_FEATURE_SBPB); > - kvm_cpu_cap_check_and_set(X86_FEATURE_IBPB_BRTYPE); > - kvm_cpu_cap_check_and_set(X86_FEATURE_SRSO_NO); > - > kvm_cpu_cap_init(CPUID_8000_0022_EAX, > F(PERFMON_V2) > ); Hi, Now that you added the final F_* macro, let's list all of them: #define F(name) \ /* Scattered Flag - For features that are scattered by cpufeatures.h. */ #define SF(name) \ /* Features that KVM supports only on 64-bit kernels. */ #define X86_64_F(name) \ /* * Raw Feature - For features that KVM supports based purely on raw host CPUID, * i.e. that KVM virtualizes even if the host kernel doesn't use the feature. * Simply force set the feature in KVM's capabilities, raw CPUID support will * be factored in by __kvm_cpu_cap_mask(). */ #define RAW_F(name) \ /* * Emulated Feature - For features that KVM emulates in software irrespective * of host CPU/kernel support. */ #define EMUL_F(name) \ /* * Synthesized Feature - For features that are synthesized into boot_cpu_data, * i.e. may not be present in the raw CPUID, but can still be advertised to * userspace. Primarily used for mitigation related feature flags. */ #define SYN_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) \ /* * VMM Features - For features that KVM "supports" in some capacity, i.e. that * KVM may query, but that are never advertised to userspace. E.g. KVM allows * userspace to enumerate MONITOR+MWAIT support to the guest, but the MWAIT * feature flag is never advertised to userspace because MONITOR+MWAIT aren't * virtualized by hardware, can't be faithfully emulated in software (KVM * emulates them as NOPs), and allowing the guest to execute them natively * requires enabling a per-VM capability. */ #define VMM_F(name) \ Honestly, I already somewhat lost in what each of those macros means even when reading the comments, which might indicate that a future reader might also have a hard time understanding those. I now support even more the case of setting each feature bit in a separate statement as I explained in an earlier patch. What do you think? Best regards, Maxim Levitsky
On Thu, Jul 04, 2024, Maxim Levitsky wrote: > On Fri, 2024-05-17 at 10:39 -0700, Sean Christopherson wrote: > > Add yet another CPUID macro, this time for features that the host kernel > > synthesizes into boot_cpu_data, i.e. that the kernel force sets even in > > situations where the feature isn't reported by CPUID. Thanks to the > > macro shenanigans of kvm_cpu_cap_init(), such features can now be handled > > in the core CPUID framework, i.e. don't need to be handled out-of-band and > > thus without as many guardrails. > > > > Adding a dedicated macro also helps document what's going on, e.g. the > > calls to kvm_cpu_cap_check_and_set() are very confusing unless the reader > > knows exactly how kvm_cpu_cap_init() generates kvm_cpu_caps (and even > > then, it's far from obvious). > > > > Signed-off-by: Sean Christopherson <seanjc@google.com> > > --- ... > Now that you added the final F_* macro, let's list all of them: > > #define F(name) \ > > /* Scattered Flag - For features that are scattered by cpufeatures.h. */ > #define SF(name) \ > > /* Features that KVM supports only on 64-bit kernels. */ > #define X86_64_F(name) \ > > /* > * Raw Feature - For features that KVM supports based purely on raw host CPUID, > * i.e. that KVM virtualizes even if the host kernel doesn't use the feature. > * Simply force set the feature in KVM's capabilities, raw CPUID support will > * be factored in by __kvm_cpu_cap_mask(). > */ > #define RAW_F(name) \ > > /* > * Emulated Feature - For features that KVM emulates in software irrespective > * of host CPU/kernel support. > */ > #define EMUL_F(name) \ > > /* > * Synthesized Feature - For features that are synthesized into boot_cpu_data, > * i.e. may not be present in the raw CPUID, but can still be advertised to > * userspace. Primarily used for mitigation related feature flags. > */ > #define SYN_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) \ > > /* > * VMM Features - For features that KVM "supports" in some capacity, i.e. that > * KVM may query, but that are never advertised to userspace. E.g. KVM allows > * userspace to enumerate MONITOR+MWAIT support to the guest, but the MWAIT > * feature flag is never advertised to userspace because MONITOR+MWAIT aren't > * virtualized by hardware, can't be faithfully emulated in software (KVM > * emulates them as NOPs), and allowing the guest to execute them natively > * requires enabling a per-VM capability. > */ > #define VMM_F(name) \ > > > Honestly, I already somewhat lost in what each of those macros means even > when reading the comments, which might indicate that a future reader might > also have a hard time understanding those. > > I now support even more the case of setting each feature bit in a separate > statement as I explained in an earlier patch. > > What do you think? I completely agree that there are an absurd number of flavors of features, but I don't see how using separate statement eliminates any of that complexity. The complexity comes from the fact that KVM actually has that many different ways and combinations for advertising and enumerating CPUID-based features. Ignoring for the moment that "vmm" and "aliased" could be avoided for any approach, if we go with statements, we'll still have kvm_cpu_cap_init{,passthrough,emulated,synthesized,aliased,vmm,only64}() or if the flavor is an input/enum, enum kvm_cpuid_feature_type { NORMAL, PASSTHROUGH, EMULATED, SYNTHESIZED, ALIASED, VMM, ONLY_64, } I.e. we'll still need the same functionality and comments, it would simply be dressed up differently. If the underlying concern is that the macro names are too terse, and/or getting one feature per line is desirable, then I'm definitely open to exploring alternative formatting options. But that's largely orthogonal to using macros instead of individual function calls.
On Tue, 2024-07-09 at 14:13 -0700, Sean Christopherson wrote: > On Thu, Jul 04, 2024, Maxim Levitsky wrote: > > On Fri, 2024-05-17 at 10:39 -0700, Sean Christopherson wrote: > > > Add yet another CPUID macro, this time for features that the host kernel > > > synthesizes into boot_cpu_data, i.e. that the kernel force sets even in > > > situations where the feature isn't reported by CPUID. Thanks to the > > > macro shenanigans of kvm_cpu_cap_init(), such features can now be handled > > > in the core CPUID framework, i.e. don't need to be handled out-of-band and > > > thus without as many guardrails. > > > > > > Adding a dedicated macro also helps document what's going on, e.g. the > > > calls to kvm_cpu_cap_check_and_set() are very confusing unless the reader > > > knows exactly how kvm_cpu_cap_init() generates kvm_cpu_caps (and even > > > then, it's far from obvious). > > > > > > Signed-off-by: Sean Christopherson <seanjc@google.com> > > > --- > > ... > > > Now that you added the final F_* macro, let's list all of them: > > > > #define F(name) \ > > > > /* Scattered Flag - For features that are scattered by cpufeatures.h. */ > > #define SF(name) \ > > > > /* Features that KVM supports only on 64-bit kernels. */ > > #define X86_64_F(name) \ > > > > /* > > * Raw Feature - For features that KVM supports based purely on raw host CPUID, > > * i.e. that KVM virtualizes even if the host kernel doesn't use the feature. > > * Simply force set the feature in KVM's capabilities, raw CPUID support will > > * be factored in by __kvm_cpu_cap_mask(). > > */ > > #define RAW_F(name) \ > > > > /* > > * Emulated Feature - For features that KVM emulates in software irrespective > > * of host CPU/kernel support. > > */ > > #define EMUL_F(name) \ > > > > /* > > * Synthesized Feature - For features that are synthesized into boot_cpu_data, > > * i.e. may not be present in the raw CPUID, but can still be advertised to > > * userspace. Primarily used for mitigation related feature flags. > > */ > > #define SYN_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) \ > > > > /* > > * VMM Features - For features that KVM "supports" in some capacity, i.e. that > > * KVM may query, but that are never advertised to userspace. E.g. KVM allows > > * userspace to enumerate MONITOR+MWAIT support to the guest, but the MWAIT > > * feature flag is never advertised to userspace because MONITOR+MWAIT aren't > > * virtualized by hardware, can't be faithfully emulated in software (KVM > > * emulates them as NOPs), and allowing the guest to execute them natively > > * requires enabling a per-VM capability. > > */ > > #define VMM_F(name) \ > > > > > > Honestly, I already somewhat lost in what each of those macros means even > > when reading the comments, which might indicate that a future reader might > > also have a hard time understanding those. > > > > I now support even more the case of setting each feature bit in a separate > > statement as I explained in an earlier patch. > > > > What do you think? > > I completely agree that there are an absurd number of flavors of features, but > I don't see how using separate statement eliminates any of that complexity. The > complexity comes from the fact that KVM actually has that many different ways and > combinations for advertising and enumerating CPUID-based features. > > Ignoring for the moment that "vmm" and "aliased" could be avoided for any approach, > if we go with statements, we'll still have > > kvm_cpu_cap_init{,passthrough,emulated,synthesized,aliased,vmm,only64}() > > or if the flavor is an input/enum, > > enum kvm_cpuid_feature_type { > NORMAL, > PASSTHROUGH, > EMULATED, > SYNTHESIZED, > ALIASED, > VMM, > ONLY_64, > } It doesn't have to be like that - something more compact can be done, plus bitmask of various flags can be used. > > I.e. we'll still need the same functionality and comments, it would simply be > dressed up differently. > > If the underlying concern is that the macro names are too terse, and/or getting > one feature per line is desirable, I indeed have these concerns and more: These are my concerns 1. Macro names are indeed too terse, and hard to figure out, even after looking at the macro source. This wasn't a problem before this patch series. 2. One feature per line would be very nice, it is much more readable, especially when features have various 'modifiers'. This wasn't such a problem before this patch series, because we just had features 'or'ed, but having one feature per line would be a good thing to have even before this patch series. 3. Feature bitmap 'or'ing of macro's output after this patch series became very confusing, now that macros do various side things. In fact VMM_F confuses the user even more, because it doesn't even contribute to the feature mask at all. It was OK before the patch series. Technically of course I am not opposed to have the 'kvm_cpu_cap_init' or whatever we name it, to remain a macro, it is probably even desirable to have it as a macro, but it is OK, as long as it is just a macro which doesn't evaluate to anything and thus looks like a function call. Best regards, Maxim Levitsky > then I'm definitely open to exploring alternative > formatting options. But that's largely orthogonal to using macros instead of > individual function calls. >
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index 0130e0677387..0e64a6332052 100644 --- a/arch/x86/kvm/cpuid.c +++ b/arch/x86/kvm/cpuid.c @@ -106,6 +106,17 @@ u32 xstate_required_size(u64 xstate_bv, bool compacted) F(name); \ }) +/* + * Synthesized Feature - For features that are synthesized into boot_cpu_data, + * i.e. may not be present in the raw CPUID, but can still be advertised to + * userspace. Primarily used for mitigation related feature flags. + */ +#define SYN_F(name) \ +({ \ + kvm_cpu_cap_synthesized |= F(name); \ + 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. @@ -727,13 +738,15 @@ do { \ const struct cpuid_reg cpuid = x86_feature_cpuid(leaf * 32); \ const u32 __maybe_unused kvm_cpu_cap_init_in_progress = leaf; \ u32 kvm_cpu_cap_emulated = 0; \ + u32 kvm_cpu_cap_synthesized = 0; \ \ if (leaf < NCAPINTS) \ kvm_cpu_caps[leaf] &= (mask); \ else \ kvm_cpu_caps[leaf] = (mask); \ \ - kvm_cpu_caps[leaf] &= raw_cpuid_get(cpuid); \ + kvm_cpu_caps[leaf] &= (raw_cpuid_get(cpuid) | \ + kvm_cpu_cap_synthesized); \ kvm_cpu_caps[leaf] |= kvm_cpu_cap_emulated; \ } while (0) @@ -913,13 +926,10 @@ void kvm_set_cpu_caps(void) kvm_cpu_cap_init(CPUID_8000_0021_EAX, F(NO_NESTED_DATA_BP) | F(LFENCE_RDTSC) | 0 /* SmmPgCfgLock */ | F(NULL_SEL_CLR_BASE) | F(AUTOIBRS) | 0 /* PrefetchCtlMsr */ | - F(WRMSR_XX_BASE_NS) + F(WRMSR_XX_BASE_NS) | SYN_F(SBPB) | SYN_F(IBPB_BRTYPE) | + SYN_F(SRSO_NO) ); - kvm_cpu_cap_check_and_set(X86_FEATURE_SBPB); - kvm_cpu_cap_check_and_set(X86_FEATURE_IBPB_BRTYPE); - kvm_cpu_cap_check_and_set(X86_FEATURE_SRSO_NO); - kvm_cpu_cap_init(CPUID_8000_0022_EAX, F(PERFMON_V2) );
Add yet another CPUID macro, this time for features that the host kernel synthesizes into boot_cpu_data, i.e. that the kernel force sets even in situations where the feature isn't reported by CPUID. Thanks to the macro shenanigans of kvm_cpu_cap_init(), such features can now be handled in the core CPUID framework, i.e. don't need to be handled out-of-band and thus without as many guardrails. Adding a dedicated macro also helps document what's going on, e.g. the calls to kvm_cpu_cap_check_and_set() are very confusing unless the reader knows exactly how kvm_cpu_cap_init() generates kvm_cpu_caps (and even then, it's far from obvious). Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/kvm/cpuid.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-)