Re: [PATCH v2 23/49] KVM: x86: Handle kernel- and KVM-defined CPUID words in a single helper

[Sean Christopherson]

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@xxxxxxxxxx>
> > ---
> >  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));