diff mbox series

[48/61] KVM: x86: Do host CPUID at load time to mask KVM cpu caps

Message ID 20200201185218.24473-49-sean.j.christopherson@intel.com
State New, archived
Headers show
Series KVM: x86: Introduce KVM cpu caps | expand

Commit Message

Sean Christopherson Feb. 1, 2020, 6:52 p.m. UTC
Mask kvm_cpu_caps based on host CPUID in preparation for overriding the
CPUID results during KVM_GET_SUPPORTED_CPUID instead of doing the
masking at runtime.

Note, masking may or may not be necessary, e.g. the kernel rarely, if
ever, sets real CPUID bits that are not supported by hardware.  But, the
code is cheap and only runs once at load, so an abundance of caution is
warranted.

No functional change intended.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/cpuid.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Comments

Sean Christopherson Feb. 24, 2020, 11:31 p.m. UTC | #1
On Mon, Feb 24, 2020 at 11:46:09PM +0100, Vitaly Kuznetsov wrote:
> Sean Christopherson <sean.j.christopherson@intel.com> writes:
> 
> > Mask kvm_cpu_caps based on host CPUID in preparation for overriding the
> > CPUID results during KVM_GET_SUPPORTED_CPUID instead of doing the
> > masking at runtime.
> >
> > Note, masking may or may not be necessary, e.g. the kernel rarely, if
> > ever, sets real CPUID bits that are not supported by hardware.  But, the
> > code is cheap and only runs once at load, so an abundance of caution is
> > warranted.
> >
> > No functional change intended.
> >
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > ---
> >  arch/x86/kvm/cpuid.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> >
> > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > index ab2a34337588..4416f2422321 100644
> > --- a/arch/x86/kvm/cpuid.c
> > +++ b/arch/x86/kvm/cpuid.c
> > @@ -272,8 +272,22 @@ static __always_inline void cpuid_entry_mask(struct kvm_cpuid_entry2 *entry,
> >  
> >  static __always_inline void kvm_cpu_cap_mask(enum cpuid_leafs leaf, u32 mask)
> >  {
> > +	const struct cpuid_reg cpuid = x86_feature_cpuid(leaf * 32);
> > +	struct kvm_cpuid_entry2 entry;
> > +
> >  	reverse_cpuid_check(leaf);
> >  	kvm_cpu_caps[leaf] &= mask;
> > +
> > +#ifdef CONFIG_KVM_CPUID_AUDIT
> > +	/* Entry needs to be fully populated when auditing is enabled. */
> > +	entry.function = cpuid.function;
> > +	entry.index = cpuid.index;
> > +#endif
> > +
> > +	cpuid_count(cpuid.function, cpuid.index,
> > +		    &entry.eax, &entry.ebx, &entry.ecx, &entry.edx);
> > +
> > +	kvm_cpu_caps[leaf] &= *__cpuid_entry_get_reg(&entry, &cpuid);
> >  }
> >  
> >  void kvm_set_cpu_caps(void)
> 
> If we don't really believe that masking will actually mask anything,
> maybe we should move it under '#ifdef CONFIG_KVM_CPUID_AUDIT'? And/or 
> add a WARN_ON()?

I'm not opposed to trying that, but I'd definitely want to do it as a
separate patch, or maybe even let it stew separately in kvm/queue for a
few cycles.

> The patch itself looks good, so:
> Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> 
> -- 
> Vitaly
>
Vitaly Kuznetsov Feb. 25, 2020, 1:53 p.m. UTC | #2
Sean Christopherson <sean.j.christopherson@intel.com> writes:

> On Mon, Feb 24, 2020 at 11:46:09PM +0100, Vitaly Kuznetsov wrote:
>> 
>> If we don't really believe that masking will actually mask anything,
>> maybe we should move it under '#ifdef CONFIG_KVM_CPUID_AUDIT'? And/or 
>> add a WARN_ON()?
>
> I'm not opposed to trying that, but I'd definitely want to do it as a
> separate patch, or maybe even let it stew separately in kvm/queue for a
> few cycles.
>

Sounds like a good idea)
Paolo Bonzini Feb. 25, 2020, 3:18 p.m. UTC | #3
On 01/02/20 19:52, Sean Christopherson wrote:
> +#ifdef CONFIG_KVM_CPUID_AUDIT
> +	/* Entry needs to be fully populated when auditing is enabled. */
> +	entry.function = cpuid.function;
> +	entry.index = cpuid.index;
> +#endif

This shows that the audit case is prone to bitrot, which is good reason
to enable it by default.

Paolo
Sean Christopherson Feb. 25, 2020, 9:08 p.m. UTC | #4
On Tue, Feb 25, 2020 at 04:18:12PM +0100, Paolo Bonzini wrote:
> On 01/02/20 19:52, Sean Christopherson wrote:
> > +#ifdef CONFIG_KVM_CPUID_AUDIT
> > +	/* Entry needs to be fully populated when auditing is enabled. */
> > +	entry.function = cpuid.function;
> > +	entry.index = cpuid.index;
> > +#endif
> 
> This shows that the audit case is prone to bitrot, which is good reason
> to enable it by default.

I have no argument against that, especially since I missed this case during
development and only caught it when running on a different system that I
had happened to configure with CONFIG_KVM_CPUID_AUDIT=y. :-)
Sean Christopherson Feb. 29, 2020, 6:38 p.m. UTC | #5
On Tue, Feb 25, 2020 at 01:08:43PM -0800, Sean Christopherson wrote:
> On Tue, Feb 25, 2020 at 04:18:12PM +0100, Paolo Bonzini wrote:
> > On 01/02/20 19:52, Sean Christopherson wrote:
> > > +#ifdef CONFIG_KVM_CPUID_AUDIT
> > > +	/* Entry needs to be fully populated when auditing is enabled. */
> > > +	entry.function = cpuid.function;
> > > +	entry.index = cpuid.index;
> > > +#endif
> > 
> > This shows that the audit case is prone to bitrot, which is good reason
> > to enable it by default.
> 
> I have no argument against that, especially since I missed this case during
> development and only caught it when running on a different system that I
> had happened to configure with CONFIG_KVM_CPUID_AUDIT=y. :-)

I ended up dropping the audit code altogether.  The uops overhead wasn't
bad, but the code bloat was pretty rough, ~16 bytes per instance.  The
final nail in the coffin was that the auditing would trigger false
positives if userspace configured CPUID leafs with a non-signficant index
to have a non-zero index, e.g. is_matching_cpuid_entry() ignores the index
if KVM_CPUID_FLAG_SIGNIFCANT_INDEX isn't set.
diff mbox series

Patch

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index ab2a34337588..4416f2422321 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -272,8 +272,22 @@  static __always_inline void cpuid_entry_mask(struct kvm_cpuid_entry2 *entry,
 
 static __always_inline void kvm_cpu_cap_mask(enum cpuid_leafs leaf, u32 mask)
 {
+	const struct cpuid_reg cpuid = x86_feature_cpuid(leaf * 32);
+	struct kvm_cpuid_entry2 entry;
+
 	reverse_cpuid_check(leaf);
 	kvm_cpu_caps[leaf] &= mask;
+
+#ifdef CONFIG_KVM_CPUID_AUDIT
+	/* Entry needs to be fully populated when auditing is enabled. */
+	entry.function = cpuid.function;
+	entry.index = cpuid.index;
+#endif
+
+	cpuid_count(cpuid.function, cpuid.index,
+		    &entry.eax, &entry.ebx, &entry.ecx, &entry.edx);
+
+	kvm_cpu_caps[leaf] &= *__cpuid_entry_get_reg(&entry, &cpuid);
 }
 
 void kvm_set_cpu_caps(void)