| Message ID | 20240517173926.965351-19-seanjc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: CPUID overhaul, fixes, and caching | expand |
On 5/18/2024 1:38 AM, Sean Christopherson wrote: [...] > /* 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) > { > const struct cpuid_reg cpuid = x86_feature_cpuid(leaf * 32); > - struct kvm_cpuid_entry2 entry; > > reverse_cpuid_check(leaf); IIUC, this reverse_cpuid_check() is redundant since it's already enforced in x86_feature_cpuid() via __feature_leaf() as previous patch(17) shows. > > - cpuid_count(cpuid.function, cpuid.index, > - &entry.eax, &entry.ebx, &entry.ecx, &entry.edx); > - > - kvm_cpu_caps[leaf] &= *__cpuid_entry_get_reg(&entry, cpuid.reg); > + kvm_cpu_caps[leaf] &= raw_cpuid_get(cpuid); > } > > static __always_inline
On 6/19/2024 2:17 PM, Yang, Weijiang wrote: > On 5/18/2024 1:38 AM, Sean Christopherson wrote: > > [...] > >> /* 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) >> { >> const struct cpuid_reg cpuid = x86_feature_cpuid(leaf * 32); >> - struct kvm_cpuid_entry2 entry; >> reverse_cpuid_check(leaf); > > IIUC, this reverse_cpuid_check() is redundant since it's already enforced in x86_feature_cpuid() via __feature_leaf() as previous patch(17) shows. Aha, I saw the function is removed in patch(23). Sorry for the noise. >> - cpuid_count(cpuid.function, cpuid.index, >> - &entry.eax, &entry.ebx, &entry.ecx, &entry.edx); >> - >> - kvm_cpu_caps[leaf] &= *__cpuid_entry_get_reg(&entry, cpuid.reg); >> + kvm_cpu_caps[leaf] &= raw_cpuid_get(cpuid); >> } >> static __always_inline > >
On Fri, 2024-05-17 at 10:38 -0700, Sean Christopherson wrote: > Explicitly zero out the feature word in kvm_cpu_caps if the word's > associated CPUID function is greater than the max leaf supported by the > CPU. For such unsupported functions, Intel CPUs return the output from > the last supported leaf, not all zeros. > > Practically speaking, this is likely a benign bug, as KVM uses the raw > host CPUID to mask the kernel's computed capabilities, and the kernel does > perform max leaf checks when populating boot_cpu_data. The only way KVM's > goof could be problematic is if the kernel force-set a feature in a leaf > that is completely unsupported, _and_ the max supported leaf happened to > return a value with '1' the same bit position. Which is theoretically > possible, but extremely unlikely. And even if that did happen, it's > entirely possible that KVM would still provide the correct functionality; > the kernel did set the capability after all. > > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/kvm/cpuid.c | 29 ++++++++++++++++++++++++----- > 1 file changed, 24 insertions(+), 5 deletions(-) > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > index a51e48663f53..77625a5477b1 100644 > --- a/arch/x86/kvm/cpuid.c > +++ b/arch/x86/kvm/cpuid.c > @@ -571,18 +571,37 @@ int kvm_vcpu_ioctl_get_cpuid2(struct kvm_vcpu *vcpu, > return 0; > } > > +static __always_inline u32 raw_cpuid_get(struct cpuid_reg cpuid) > +{ > + struct kvm_cpuid_entry2 entry; > + u32 base; > + > + /* > + * KVM only supports features defined by Intel (0x0), AMD (0x80000000), > + * and Centaur (0xc0000000). WARN if a feature for new vendor base is > + * defined, as this and other code would need to be updated. > + */ > + base = cpuid.function & 0xffff0000; > + if (WARN_ON_ONCE(base && base != 0x80000000 && base != 0xc0000000)) > + return 0; > + > + if (cpuid_eax(base) < cpuid.function) > + return 0; > + > + cpuid_count(cpuid.function, cpuid.index, > + &entry.eax, &entry.ebx, &entry.ecx, &entry.edx); > + > + 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) > { > const struct cpuid_reg cpuid = x86_feature_cpuid(leaf * 32); > - struct kvm_cpuid_entry2 entry; > > reverse_cpuid_check(leaf); > > - cpuid_count(cpuid.function, cpuid.index, > - &entry.eax, &entry.ebx, &entry.ecx, &entry.edx); > - > - kvm_cpu_caps[leaf] &= *__cpuid_entry_get_reg(&entry, cpuid.reg); > + kvm_cpu_caps[leaf] &= raw_cpuid_get(cpuid); > } > > static __always_inline Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com> Best regards, Maxim Levitsky
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index a51e48663f53..77625a5477b1 100644 --- a/arch/x86/kvm/cpuid.c +++ b/arch/x86/kvm/cpuid.c @@ -571,18 +571,37 @@ int kvm_vcpu_ioctl_get_cpuid2(struct kvm_vcpu *vcpu, return 0; } +static __always_inline u32 raw_cpuid_get(struct cpuid_reg cpuid) +{ + struct kvm_cpuid_entry2 entry; + u32 base; + + /* + * KVM only supports features defined by Intel (0x0), AMD (0x80000000), + * and Centaur (0xc0000000). WARN if a feature for new vendor base is + * defined, as this and other code would need to be updated. + */ + base = cpuid.function & 0xffff0000; + if (WARN_ON_ONCE(base && base != 0x80000000 && base != 0xc0000000)) + return 0; + + if (cpuid_eax(base) < cpuid.function) + return 0; + + cpuid_count(cpuid.function, cpuid.index, + &entry.eax, &entry.ebx, &entry.ecx, &entry.edx); + + 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) { const struct cpuid_reg cpuid = x86_feature_cpuid(leaf * 32); - struct kvm_cpuid_entry2 entry; reverse_cpuid_check(leaf); - cpuid_count(cpuid.function, cpuid.index, - &entry.eax, &entry.ebx, &entry.ecx, &entry.edx); - - kvm_cpu_caps[leaf] &= *__cpuid_entry_get_reg(&entry, cpuid.reg); + kvm_cpu_caps[leaf] &= raw_cpuid_get(cpuid); } static __always_inline
Explicitly zero out the feature word in kvm_cpu_caps if the word's associated CPUID function is greater than the max leaf supported by the CPU. For such unsupported functions, Intel CPUs return the output from the last supported leaf, not all zeros. Practically speaking, this is likely a benign bug, as KVM uses the raw host CPUID to mask the kernel's computed capabilities, and the kernel does perform max leaf checks when populating boot_cpu_data. The only way KVM's goof could be problematic is if the kernel force-set a feature in a leaf that is completely unsupported, _and_ the max supported leaf happened to return a value with '1' the same bit position. Which is theoretically possible, but extremely unlikely. And even if that did happen, it's entirely possible that KVM would still provide the correct functionality; the kernel did set the capability after all. Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/kvm/cpuid.c | 29 ++++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-)