| Message ID | 20240517173926.965351-24-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: > Merge kvm_cpu_cap_init() and kvm_cpu_cap_init_kvm_defined() into a single > helper. The only advantage of separating the two was to make it somewhat > obvious that KVM directly initializes the KVM-defined words, whereas using > a common helper will allow for hardening both kernel- and KVM-defined > CPUID words without needing copy+paste. > > No functional change intended. > > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/kvm/cpuid.c | 44 +++++++++++++++----------------------------- > 1 file changed, 15 insertions(+), 29 deletions(-) > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > index f2bd2f5c4ea3..8efffd48cdf1 100644 > --- a/arch/x86/kvm/cpuid.c > +++ b/arch/x86/kvm/cpuid.c > @@ -622,37 +622,23 @@ static __always_inline u32 raw_cpuid_get(struct cpuid_reg cpuid) > return *__cpuid_entry_get_reg(&entry, cpuid.reg); > } > > -/* Mask kvm_cpu_caps for @leaf with the raw CPUID capabilities of this CPU. */ > -static __always_inline void __kvm_cpu_cap_mask(unsigned int leaf) > +static __always_inline void kvm_cpu_cap_init(u32 leaf, u32 mask) > { > const struct cpuid_reg cpuid = x86_feature_cpuid(leaf * 32); > > - reverse_cpuid_check(leaf); > + /* > + * For kernel-defined leafs, mask the boot CPU's pre-populated value. > + * For KVM-defined leafs, explicitly set the leaf, as KVM is the one > + * and only authority. > + */ > + if (leaf < NCAPINTS) > + kvm_cpu_caps[leaf] &= mask; > + else > + kvm_cpu_caps[leaf] = mask; Hi, I have an idea, how about we just initialize the kvm only leafs to 0xFFFFFFFF and then treat them exactly in the same way as kernel regular leafs? Then the user won't have to figure out (assuming that the user doesn't read the comment, who does?) why we use mask as init value. But if you prefer to leave it this way, I won't object either. Best regards, Maxim Levitsky > > kvm_cpu_caps[leaf] &= raw_cpuid_get(cpuid); > } > > -static __always_inline > -void kvm_cpu_cap_init_kvm_defined(enum kvm_only_cpuid_leafs leaf, u32 mask) > -{ > - /* Use kvm_cpu_cap_init for leafs that aren't KVM-only. */ > - BUILD_BUG_ON(leaf < NCAPINTS); > - > - kvm_cpu_caps[leaf] = mask; > - > - __kvm_cpu_cap_mask(leaf); > -} > - > -static __always_inline void kvm_cpu_cap_init(enum cpuid_leafs leaf, u32 mask) > -{ > - /* Use kvm_cpu_cap_init_kvm_defined for KVM-only leafs. */ > - BUILD_BUG_ON(leaf >= NCAPINTS); > - > - kvm_cpu_caps[leaf] &= mask; > - > - __kvm_cpu_cap_mask(leaf); > -} > - > void kvm_set_cpu_caps(void) > { > memset(kvm_cpu_caps, 0, sizeof(kvm_cpu_caps)); > @@ -740,12 +726,12 @@ void kvm_set_cpu_caps(void) > F(AMX_FP16) | F(AVX_IFMA) | F(LAM) > ); > > - kvm_cpu_cap_init_kvm_defined(CPUID_7_1_EDX, > + kvm_cpu_cap_init(CPUID_7_1_EDX, > F(AVX_VNNI_INT8) | F(AVX_NE_CONVERT) | F(PREFETCHITI) | > F(AMX_COMPLEX) > ); > > - kvm_cpu_cap_init_kvm_defined(CPUID_7_2_EDX, > + kvm_cpu_cap_init(CPUID_7_2_EDX, > F(INTEL_PSFD) | F(IPRED_CTRL) | F(RRSBA_CTRL) | F(DDPD_U) | > F(BHI_CTRL) | F(MCDT_NO) > ); > @@ -755,7 +741,7 @@ void kvm_set_cpu_caps(void) > X86_64_F(XFD) > ); > > - kvm_cpu_cap_init_kvm_defined(CPUID_12_EAX, > + kvm_cpu_cap_init(CPUID_12_EAX, > SF(SGX1) | SF(SGX2) | SF(SGX_EDECCSSA) > ); > > @@ -781,7 +767,7 @@ void kvm_set_cpu_caps(void) > if (!tdp_enabled && IS_ENABLED(CONFIG_X86_64)) > kvm_cpu_cap_set(X86_FEATURE_GBPAGES); > > - kvm_cpu_cap_init_kvm_defined(CPUID_8000_0007_EDX, > + kvm_cpu_cap_init(CPUID_8000_0007_EDX, > SF(CONSTANT_TSC) > ); > > @@ -835,7 +821,7 @@ void kvm_set_cpu_caps(void) > kvm_cpu_cap_check_and_set(X86_FEATURE_IBPB_BRTYPE); > kvm_cpu_cap_check_and_set(X86_FEATURE_SRSO_NO); > > - kvm_cpu_cap_init_kvm_defined(CPUID_8000_0022_EAX, > + kvm_cpu_cap_init(CPUID_8000_0022_EAX, > F(PERFMON_V2) > ); >
On Thu, Jul 04, 2024, Maxim Levitsky wrote: > On Fri, 2024-05-17 at 10:39 -0700, Sean Christopherson wrote: > > Merge kvm_cpu_cap_init() and kvm_cpu_cap_init_kvm_defined() into a single > > helper. The only advantage of separating the two was to make it somewhat > > obvious that KVM directly initializes the KVM-defined words, whereas using > > a common helper will allow for hardening both kernel- and KVM-defined > > CPUID words without needing copy+paste. > > > > No functional change intended. > > > > Signed-off-by: Sean Christopherson <seanjc@google.com> > > --- > > arch/x86/kvm/cpuid.c | 44 +++++++++++++++----------------------------- > > 1 file changed, 15 insertions(+), 29 deletions(-) > > > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > > index f2bd2f5c4ea3..8efffd48cdf1 100644 > > --- a/arch/x86/kvm/cpuid.c > > +++ b/arch/x86/kvm/cpuid.c > > @@ -622,37 +622,23 @@ static __always_inline u32 raw_cpuid_get(struct cpuid_reg cpuid) > > return *__cpuid_entry_get_reg(&entry, cpuid.reg); > > } > > > > -/* Mask kvm_cpu_caps for @leaf with the raw CPUID capabilities of this CPU. */ > > -static __always_inline void __kvm_cpu_cap_mask(unsigned int leaf) > > +static __always_inline void kvm_cpu_cap_init(u32 leaf, u32 mask) > > { > > const struct cpuid_reg cpuid = x86_feature_cpuid(leaf * 32); > > > > - reverse_cpuid_check(leaf); > > + /* > > + * For kernel-defined leafs, mask the boot CPU's pre-populated value. > > + * For KVM-defined leafs, explicitly set the leaf, as KVM is the one > > + * and only authority. > > + */ > > + if (leaf < NCAPINTS) > > + kvm_cpu_caps[leaf] &= mask; > > + else > > + kvm_cpu_caps[leaf] = mask; > > Hi, > > I have an idea, how about we just initialize the kvm only leafs to 0xFFFFFFFF > and then treat them exactly in the same way as kernel regular leafs? > > Then the user won't have to figure out (assuming that the user doesn't read > the comment, who does?) why we use mask as init value. > > But if you prefer to leave it this way, I won't object either. Huh, hadn't thought of that. It's a small code change, but I'm leaning towards keeping the current code as we'd still need a comment to explain why KVM sets all bits by default. And in the unlikely case that we royally screw up and fail to call kvm_cpu_cap_init() on a word, starting with 0xff would result in all features in the uninitialized word being treated as supported. For posterity... diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index 18ded0e682f2..6fcfb0fa4bd6 100644 --- a/arch/x86/kvm/cpuid.c +++ b/arch/x86/kvm/cpuid.c @@ -762,11 +762,7 @@ do { \ 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] &= (mask); \ kvm_cpu_caps[leaf] &= (raw_cpuid_get(cpuid) | \ kvm_cpu_cap_synthesized); \ kvm_cpu_caps[leaf] |= kvm_cpu_cap_emulated; \ @@ -780,7 +776,7 @@ do { \ void kvm_set_cpu_caps(void) { - memset(kvm_cpu_caps, 0, sizeof(kvm_cpu_caps)); + memset(kvm_cpu_caps, 0xff, sizeof(kvm_cpu_caps)); BUILD_BUG_ON(sizeof(kvm_cpu_caps) - (NKVMCAPINTS * sizeof(*kvm_cpu_caps)) > sizeof(boot_cpu_data.x86_capability));
On 7/9/2024 5:18 AM, Sean Christopherson wrote: > On Thu, Jul 04, 2024, Maxim Levitsky wrote: >> On Fri, 2024-05-17 at 10:39 -0700, Sean Christopherson wrote: >>> Merge kvm_cpu_cap_init() and kvm_cpu_cap_init_kvm_defined() into a single >>> helper. The only advantage of separating the two was to make it somewhat >>> obvious that KVM directly initializes the KVM-defined words, whereas using >>> a common helper will allow for hardening both kernel- and KVM-defined >>> CPUID words without needing copy+paste. >>> >>> No functional change intended. >>> >>> Signed-off-by: Sean Christopherson <seanjc@google.com> Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com> >>> --- >>> arch/x86/kvm/cpuid.c | 44 +++++++++++++++----------------------------- >>> 1 file changed, 15 insertions(+), 29 deletions(-) >>> >>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c >>> index f2bd2f5c4ea3..8efffd48cdf1 100644 >>> --- a/arch/x86/kvm/cpuid.c >>> +++ b/arch/x86/kvm/cpuid.c >>> @@ -622,37 +622,23 @@ static __always_inline u32 raw_cpuid_get(struct cpuid_reg cpuid) >>> return *__cpuid_entry_get_reg(&entry, cpuid.reg); >>> } >>> >>> -/* Mask kvm_cpu_caps for @leaf with the raw CPUID capabilities of this CPU. */ >>> -static __always_inline void __kvm_cpu_cap_mask(unsigned int leaf) >>> +static __always_inline void kvm_cpu_cap_init(u32 leaf, u32 mask) >>> { >>> const struct cpuid_reg cpuid = x86_feature_cpuid(leaf * 32); >>> >>> - reverse_cpuid_check(leaf); >>> + /* >>> + * For kernel-defined leafs, mask the boot CPU's pre-populated value. >>> + * For KVM-defined leafs, explicitly set the leaf, as KVM is the one >>> + * and only authority. >>> + */ >>> + if (leaf < NCAPINTS) >>> + kvm_cpu_caps[leaf] &= mask; >>> + else >>> + kvm_cpu_caps[leaf] = mask; >> >> Hi, >> >> I have an idea, how about we just initialize the kvm only leafs to 0xFFFFFFFF >> and then treat them exactly in the same way as kernel regular leafs? >> >> Then the user won't have to figure out (assuming that the user doesn't read >> the comment, who does?) why we use mask as init value. >> >> But if you prefer to leave it this way, I won't object either. > > Huh, hadn't thought of that. It's a small code change, but I'm leaning towards > keeping the current code as we'd still need a comment to explain why KVM sets > all bits by default. > And in the unlikely case that we royally screw up and fail > to call kvm_cpu_cap_init() on a word, starting with 0xff would result in all > features in the uninitialized word being treated as supported. +1 > For posterity... > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > index 18ded0e682f2..6fcfb0fa4bd6 100644 > --- a/arch/x86/kvm/cpuid.c > +++ b/arch/x86/kvm/cpuid.c > @@ -762,11 +762,7 @@ do { \ > 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] &= (mask); \ > kvm_cpu_caps[leaf] &= (raw_cpuid_get(cpuid) | \ > kvm_cpu_cap_synthesized); \ > kvm_cpu_caps[leaf] |= kvm_cpu_cap_emulated; \ > @@ -780,7 +776,7 @@ do { \ > > void kvm_set_cpu_caps(void) > { > - memset(kvm_cpu_caps, 0, sizeof(kvm_cpu_caps)); > + memset(kvm_cpu_caps, 0xff, sizeof(kvm_cpu_caps)); > > BUILD_BUG_ON(sizeof(kvm_cpu_caps) - (NKVMCAPINTS * sizeof(*kvm_cpu_caps)) > > sizeof(boot_cpu_data.x86_capability)); >
On Mon, 2024-07-08 at 14:18 -0700, Sean Christopherson wrote: > On Thu, Jul 04, 2024, Maxim Levitsky wrote: > > On Fri, 2024-05-17 at 10:39 -0700, Sean Christopherson wrote: > > > Merge kvm_cpu_cap_init() and kvm_cpu_cap_init_kvm_defined() into a single > > > helper. The only advantage of separating the two was to make it somewhat > > > obvious that KVM directly initializes the KVM-defined words, whereas using > > > a common helper will allow for hardening both kernel- and KVM-defined > > > CPUID words without needing copy+paste. > > > > > > No functional change intended. > > > > > > Signed-off-by: Sean Christopherson <seanjc@google.com> > > > --- > > > arch/x86/kvm/cpuid.c | 44 +++++++++++++++----------------------------- > > > 1 file changed, 15 insertions(+), 29 deletions(-) > > > > > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > > > index f2bd2f5c4ea3..8efffd48cdf1 100644 > > > --- a/arch/x86/kvm/cpuid.c > > > +++ b/arch/x86/kvm/cpuid.c > > > @@ -622,37 +622,23 @@ static __always_inline u32 raw_cpuid_get(struct cpuid_reg cpuid) > > > return *__cpuid_entry_get_reg(&entry, cpuid.reg); > > > } > > > > > > -/* Mask kvm_cpu_caps for @leaf with the raw CPUID capabilities of this CPU. */ > > > -static __always_inline void __kvm_cpu_cap_mask(unsigned int leaf) > > > +static __always_inline void kvm_cpu_cap_init(u32 leaf, u32 mask) > > > { > > > const struct cpuid_reg cpuid = x86_feature_cpuid(leaf * 32); > > > > > > - reverse_cpuid_check(leaf); > > > + /* > > > + * For kernel-defined leafs, mask the boot CPU's pre-populated value. > > > + * For KVM-defined leafs, explicitly set the leaf, as KVM is the one > > > + * and only authority. > > > + */ > > > + if (leaf < NCAPINTS) > > > + kvm_cpu_caps[leaf] &= mask; > > > + else > > > + kvm_cpu_caps[leaf] = mask; > > > > Hi, > > > > I have an idea, how about we just initialize the kvm only leafs to 0xFFFFFFFF > > and then treat them exactly in the same way as kernel regular leafs? > > > > Then the user won't have to figure out (assuming that the user doesn't read > > the comment, who does?) why we use mask as init value. > > > > But if you prefer to leave it this way, I won't object either. > > Huh, hadn't thought of that. It's a small code change, but I'm leaning towards > keeping the current code as we'd still need a comment to explain why KVM sets > all bits by default. I agree that the comment is needed, but the comment in my case is more natural - KVM only leaves don't come from boot_cpu_info, so naturally all features there are '1'. > And in the unlikely case that we royally screw up and fail > to call kvm_cpu_cap_init() on a word, starting with 0xff would result in all > features in the uninitialized word being treated as supported. Yes, but IMHO the chances of this happening are very low. I understand your concerns though, but then IMHO it's better to keep the kvm_cpu_cap_init_kvm_defined, because this way at least the function name cleanly describes the difference instead of the difference being buried in the function itself (the comment helps but still it is less noticeable than a function name). I don't have a very strong opinion on this though, because IMHO the kvm_cpu_cap_init_kvm_defined is also not very user friendly, so if you really think that the new code is more readable, let it be. Best regards, Maxim Levitsky > > For posterity... > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > index 18ded0e682f2..6fcfb0fa4bd6 100644 > --- a/arch/x86/kvm/cpuid.c > +++ b/arch/x86/kvm/cpuid.c > @@ -762,11 +762,7 @@ do { \ > 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] &= (mask); \ > kvm_cpu_caps[leaf] &= (raw_cpuid_get(cpuid) | \ > kvm_cpu_cap_synthesized); \ > kvm_cpu_caps[leaf] |= kvm_cpu_cap_emulated; \ > @@ -780,7 +776,7 @@ do { \ > > void kvm_set_cpu_caps(void) > { > - memset(kvm_cpu_caps, 0, sizeof(kvm_cpu_caps)); > + memset(kvm_cpu_caps, 0xff, sizeof(kvm_cpu_caps)); > > BUILD_BUG_ON(sizeof(kvm_cpu_caps) - (NKVMCAPINTS * sizeof(*kvm_cpu_caps)) > > sizeof(boot_cpu_data.x86_capability)); >
On Wed, Jul 24, 2024, Maxim Levitsky wrote: > On Mon, 2024-07-08 at 14:18 -0700, Sean Christopherson wrote: > > And in the unlikely case that we royally screw up and fail > > to call kvm_cpu_cap_init() on a word, starting with 0xff would result in all > > features in the uninitialized word being treated as supported. > Yes, but IMHO the chances of this happening are very low. > > I understand your concerns though, but then IMHO it's better to keep the > kvm_cpu_cap_init_kvm_defined, because this way at least the function name > cleanly describes the difference instead of the difference being buried in the function > itself (the comment helps but still it is less noticeable than a function name). > > I don't have a very strong opinion on this though, > because IMHO the kvm_cpu_cap_init_kvm_defined is also not very user friendly, > so if you really think that the new code is more readable, let it be. Hmm, the main motiviation of this patch was to avoid duplicate code in later patches, but looking at the end result, I don't think that eliminating the KVM-defined variants is necessary, e.g. ending up with this should work, too. #define __kvm_cpu_cap_init(leaf) \ 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; \ \ kvm_cpu_caps[leaf] &= (raw_cpuid_get(cpuid) | \ kvm_cpu_cap_synthesized); \ kvm_cpu_caps[leaf] |= kvm_cpu_cap_emulated; \ } while (0) /* For kernel-defined leafs, mask the boot CPU's pre-populated value. */ #define kvm_cpu_cap_init(leaf, mask) \ do { BUILD_BUG_ON(leaf >= NCAPINTS); \ kvm_cpu_caps[leaf] &= (mask); \ \ __kvm_cpu_cap_init(leaf); \ } while (0) /* For KVM-defined leafs, explicitly set the leaf, KVM is the sole authority. */ #define kvm_cpu_cap_init_kvm_defined(leaf, mask) \ do { \ BUILD_BUG_ON(leaf < NCAPINTS); \ kvm_cpu_caps[leaf] = (mask); \ \ __kvm_cpu_cap_init(leaf); \ } while (0) That said, unless someone really likes kvm_cpu_cap_init_kvm_defined(), I am leaning toward keeping this patch (but rewriting the changelog). IMO, whether a leaf is KVM-only or known to the kernel is a plumbing detail that really shouldn't affect anything in kvm_set_cpu_caps(). Literally the only difference is whether or not there are kernel capabilities to account for. The "types" of features isn't restricted in any way, e.g. CPUID_12_EAX is KVM-only and contains only scattered features, but CPUID_7_1_EDX is KVM-only and contains only "regular" features. And if a feature changes from KVM-only to kernel-managed, we'd need to update the caller. This is unlikely, but it seems like an unnecessary maintenance burden. Ooh, and thinking more on that and on the argument against initializing the KVM- only leafs to all ones, I think we should remove this: memcpy(&kvm_cpu_caps, &boot_cpu_data.x86_capability, sizeof(kvm_cpu_caps) - (NKVMCAPINTS * sizeof(*kvm_cpu_caps))); and instead explicitly mask the boot_cpu_data.x86_capability[leaf]. It's _way_ more likely that the kernel adds a leaf without updating KVM, in which case copying the kernel capabilities without masking them against KVM's capabilities would over-report the set of supported features. The odds of over-reproring are still low, as KVM limit the max leaf in __do_cpuid_func(), but unless I'm missing something, the memcpy() trick adds no value in the current code base. E.g. diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index dbc3f6ce9203..593de2c1811b 100644 --- a/arch/x86/kvm/cpuid.c +++ b/arch/x86/kvm/cpuid.c @@ -730,18 +730,20 @@ do { \ } while (0) /* - * For kernel-defined leafs, mask the boot CPU's pre-populated value. For KVM- - * defined leafs, explicitly set the leaf, as KVM is the one and only authority. + * For leafs that are managed by the kernel, mask the boot CPU's capabilities, + * which are populated by the kernel. For KVM-only leafs, as KVM is the one + * and only authority. */ #define kvm_cpu_cap_init(leaf, mask) \ do { \ const struct cpuid_reg cpuid = x86_feature_cpuid(leaf * 32); \ const u32 __maybe_unused kvm_cpu_cap_init_in_progress = leaf; \ + const u32 kernel_cpu_caps = boot_cpu_data.x86_capability[leaf]; \ u32 kvm_cpu_cap_emulated = 0; \ u32 kvm_cpu_cap_synthesized = 0; \ \ if (leaf < NCAPINTS) \ - kvm_cpu_caps[leaf] &= (mask); \ + kvm_cpu_caps[leaf] = kernel_cpu_caps & (mask); \ else \ kvm_cpu_caps[leaf] = (mask); \ \ @@ -763,9 +765,6 @@ void kvm_set_cpu_caps(void) BUILD_BUG_ON(sizeof(kvm_cpu_caps) - (NKVMCAPINTS * sizeof(*kvm_cpu_caps)) > sizeof(boot_cpu_data.x86_capability)); - memcpy(&kvm_cpu_caps, &boot_cpu_data.x86_capability, - sizeof(kvm_cpu_caps) - (NKVMCAPINTS * sizeof(*kvm_cpu_caps))); - kvm_cpu_cap_init(CPUID_1_ECX, /* * NOTE: MONITOR (and MWAIT) are emulated as NOP, but *not*
У чт, 2024-07-25 у 12:18 -0700, Sean Christopherson пише: > > On Wed, Jul 24, 2024, Maxim Levitsky wrote: > > > > On Mon, 2024-07-08 at 14:18 -0700, Sean Christopherson wrote: > > > > > > And in the unlikely case that we royally screw up and fail > > > > > > to call kvm_cpu_cap_init() on a word, starting with 0xff would result in all > > > > > > features in the uninitialized word being treated as supported. > > > > Yes, but IMHO the chances of this happening are very low. > > > > > > > > I understand your concerns though, but then IMHO it's better to keep the > > > > kvm_cpu_cap_init_kvm_defined, because this way at least the function name > > > > cleanly describes the difference instead of the difference being buried in the function > > > > itself (the comment helps but still it is less noticeable than a function name). > > > > > > > > I don't have a very strong opinion on this though, > > > > because IMHO the kvm_cpu_cap_init_kvm_defined is also not very user friendly, > > > > so if you really think that the new code is more readable, let it be. > > > > Hmm, the main motiviation of this patch was to avoid duplicate code in later > > patches, but looking at the end result, I don't think that eliminating the > > KVM-defined variants is necessary, e.g. ending up with this should work, too. > > > > #define __kvm_cpu_cap_init(leaf) \ > > 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; \ > > \ > > kvm_cpu_caps[leaf] &= (raw_cpuid_get(cpuid) | \ > > kvm_cpu_cap_synthesized); \ > > kvm_cpu_caps[leaf] |= kvm_cpu_cap_emulated; \ > > } while (0) > > > > /* For kernel-defined leafs, mask the boot CPU's pre-populated value. */ > > #define kvm_cpu_cap_init(leaf, mask) \ > > do { > > BUILD_BUG_ON(leaf >= NCAPINTS); \ > > kvm_cpu_caps[leaf] &= (mask); \ > > \ > > __kvm_cpu_cap_init(leaf); \ > > } while (0) > > > > /* For KVM-defined leafs, explicitly set the leaf, KVM is the sole authority. */ > > #define kvm_cpu_cap_init_kvm_defined(leaf, mask) \ > > do { \ > > BUILD_BUG_ON(leaf < NCAPINTS); \ > > kvm_cpu_caps[leaf] = (mask); \ > > \ > > __kvm_cpu_cap_init(leaf); \ > > } while (0) > > > > That said, unless someone really likes kvm_cpu_cap_init_kvm_defined(), I am > > leaning toward keeping this patch (but rewriting the changelog). IMO, whether a > > leaf is KVM-only or known to the kernel is a plumbing detail that really shouldn't > > affect anything in kvm_set_cpu_caps(). Literally the only difference is whether > > or not there are kernel capabilities to account for. The "types" of features isn't > > restricted in any way, e.g. CPUID_12_EAX is KVM-only and contains only scattered > > features, but CPUID_7_1_EDX is KVM-only and contains only "regular" features. > > > > And if a feature changes from KVM-only to kernel-managed, we'd need to update the > > caller. This is unlikely, but it seems like an unnecessary maintenance burden. > > > > Ooh, and thinking more on that and on the argument against initializing the KVM- > > only leafs to all ones, I think we should remove this: > > > > memcpy(&kvm_cpu_caps, &boot_cpu_data.x86_capability, > > sizeof(kvm_cpu_caps) - (NKVMCAPINTS * sizeof(*kvm_cpu_caps))); > > > > and instead explicitly mask the boot_cpu_data.x86_capability[leaf]. It's _way_ > > more likely that the kernel adds a leaf without updating KVM, in which case > > copying the kernel capabilities without masking them against KVM's capabilities > > would over-report the set of supported features. The odds of over-reproring are > > still low, as KVM limit the max leaf in __do_cpuid_func(), but unless I'm missing > > something, the memcpy() trick adds no value in the current code base. Nothing against this. > > > > E.g. > > > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > > index dbc3f6ce9203..593de2c1811b 100644 > > --- a/arch/x86/kvm/cpuid.c > > +++ b/arch/x86/kvm/cpuid.c > > @@ -730,18 +730,20 @@ do { \ > > } while (0) > > > > /* > > - * For kernel-defined leafs, mask the boot CPU's pre-populated value. For KVM- > > - * defined leafs, explicitly set the leaf, as KVM is the one and only authority. > > + * For leafs that are managed by the kernel, mask the boot CPU's capabilities, > > + * which are populated by the kernel. For KVM-only leafs, as KVM is the one > > + * and only authority. > > */ > > #define kvm_cpu_cap_init(leaf, mask) \ > > do { \ > > const struct cpuid_reg cpuid = x86_feature_cpuid(leaf * 32); \ > > const u32 __maybe_unused kvm_cpu_cap_init_in_progress = leaf; \ > > + const u32 kernel_cpu_caps = boot_cpu_data.x86_capability[leaf]; \ > > u32 kvm_cpu_cap_emulated = 0; \ > > u32 kvm_cpu_cap_synthesized = 0; \ > > \ > > if (leaf < NCAPINTS) \ > > - kvm_cpu_caps[leaf] &= (mask); \ > > + kvm_cpu_caps[leaf] = kernel_cpu_caps & (mask); \ > > else \ > > kvm_cpu_caps[leaf] = (mask); \ I am not going to argue much about this, I still think that assigning directly using a mask is confusing. Using kernel_cpu_caps is indeed better regardless of other issues. Best regards, Maxim Levitsky > > \ > > @@ -763,9 +765,6 @@ void kvm_set_cpu_caps(void) > > BUILD_BUG_ON(sizeof(kvm_cpu_caps) - (NKVMCAPINTS * sizeof(*kvm_cpu_caps)) > > > sizeof(boot_cpu_data.x86_capability)); > > > > - memcpy(&kvm_cpu_caps, &boot_cpu_data.x86_capability, > > - sizeof(kvm_cpu_caps) - (NKVMCAPINTS * sizeof(*kvm_cpu_caps))); > > - > > kvm_cpu_cap_init(CPUID_1_ECX, > > /* > > * NOTE: MONITOR (and MWAIT) are emulated as NOP, but *not* > >
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index f2bd2f5c4ea3..8efffd48cdf1 100644 --- a/arch/x86/kvm/cpuid.c +++ b/arch/x86/kvm/cpuid.c @@ -622,37 +622,23 @@ static __always_inline u32 raw_cpuid_get(struct cpuid_reg cpuid) return *__cpuid_entry_get_reg(&entry, cpuid.reg); } -/* Mask kvm_cpu_caps for @leaf with the raw CPUID capabilities of this CPU. */ -static __always_inline void __kvm_cpu_cap_mask(unsigned int leaf) +static __always_inline void kvm_cpu_cap_init(u32 leaf, u32 mask) { const struct cpuid_reg cpuid = x86_feature_cpuid(leaf * 32); - reverse_cpuid_check(leaf); + /* + * For kernel-defined leafs, mask the boot CPU's pre-populated value. + * For KVM-defined leafs, explicitly set the leaf, as KVM is the one + * and only authority. + */ + if (leaf < NCAPINTS) + kvm_cpu_caps[leaf] &= mask; + else + kvm_cpu_caps[leaf] = mask; kvm_cpu_caps[leaf] &= raw_cpuid_get(cpuid); } -static __always_inline -void kvm_cpu_cap_init_kvm_defined(enum kvm_only_cpuid_leafs leaf, u32 mask) -{ - /* Use kvm_cpu_cap_init for leafs that aren't KVM-only. */ - BUILD_BUG_ON(leaf < NCAPINTS); - - kvm_cpu_caps[leaf] = mask; - - __kvm_cpu_cap_mask(leaf); -} - -static __always_inline void kvm_cpu_cap_init(enum cpuid_leafs leaf, u32 mask) -{ - /* Use kvm_cpu_cap_init_kvm_defined for KVM-only leafs. */ - BUILD_BUG_ON(leaf >= NCAPINTS); - - kvm_cpu_caps[leaf] &= mask; - - __kvm_cpu_cap_mask(leaf); -} - void kvm_set_cpu_caps(void) { memset(kvm_cpu_caps, 0, sizeof(kvm_cpu_caps)); @@ -740,12 +726,12 @@ void kvm_set_cpu_caps(void) F(AMX_FP16) | F(AVX_IFMA) | F(LAM) ); - kvm_cpu_cap_init_kvm_defined(CPUID_7_1_EDX, + kvm_cpu_cap_init(CPUID_7_1_EDX, F(AVX_VNNI_INT8) | F(AVX_NE_CONVERT) | F(PREFETCHITI) | F(AMX_COMPLEX) ); - kvm_cpu_cap_init_kvm_defined(CPUID_7_2_EDX, + kvm_cpu_cap_init(CPUID_7_2_EDX, F(INTEL_PSFD) | F(IPRED_CTRL) | F(RRSBA_CTRL) | F(DDPD_U) | F(BHI_CTRL) | F(MCDT_NO) ); @@ -755,7 +741,7 @@ void kvm_set_cpu_caps(void) X86_64_F(XFD) ); - kvm_cpu_cap_init_kvm_defined(CPUID_12_EAX, + kvm_cpu_cap_init(CPUID_12_EAX, SF(SGX1) | SF(SGX2) | SF(SGX_EDECCSSA) ); @@ -781,7 +767,7 @@ void kvm_set_cpu_caps(void) if (!tdp_enabled && IS_ENABLED(CONFIG_X86_64)) kvm_cpu_cap_set(X86_FEATURE_GBPAGES); - kvm_cpu_cap_init_kvm_defined(CPUID_8000_0007_EDX, + kvm_cpu_cap_init(CPUID_8000_0007_EDX, SF(CONSTANT_TSC) ); @@ -835,7 +821,7 @@ void kvm_set_cpu_caps(void) kvm_cpu_cap_check_and_set(X86_FEATURE_IBPB_BRTYPE); kvm_cpu_cap_check_and_set(X86_FEATURE_SRSO_NO); - kvm_cpu_cap_init_kvm_defined(CPUID_8000_0022_EAX, + kvm_cpu_cap_init(CPUID_8000_0022_EAX, F(PERFMON_V2) );
Merge kvm_cpu_cap_init() and kvm_cpu_cap_init_kvm_defined() into a single helper. The only advantage of separating the two was to make it somewhat obvious that KVM directly initializes the KVM-defined words, whereas using a common helper will allow for hardening both kernel- and KVM-defined CPUID words without needing copy+paste. No functional change intended. Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/kvm/cpuid.c | 44 +++++++++++++++----------------------------- 1 file changed, 15 insertions(+), 29 deletions(-)