| Message ID | 20200201185218.24473-49-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: Introduce KVM cpu caps | expand |
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 >
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)
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
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. :-)
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 --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)
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(+)