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