У чт, 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*
> >
