| Message ID | 20240517173926.965351-41-seanjc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: CPUID overhaul, fixes, and caching | expand |
On Fri, 2024-05-17 at 10:39 -0700, Sean Christopherson wrote: > Constrain all guest cpu_caps based on KVM support instead of constraining > only the few features that KVM _currently_ needs to verify are actually > supported by KVM. The intent of cpu_caps is to track what the guest is > actually capable of using, not the raw, unfiltered CPUID values that the > guest sees. > > I.e. KVM should always consult it's only support when making decisions > based on guest CPUID, and the only reason KVM has historically made the > checks opt-in was due to lack of centralized tracking. > > Suggested-by: Maxim Levitsky <mlevitsk@redhat.com> > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/kvm/cpuid.c | 14 +++++++++++++- > arch/x86/kvm/cpuid.h | 7 ------- > arch/x86/kvm/svm/svm.c | 11 ----------- > arch/x86/kvm/vmx/vmx.c | 9 ++------- > 4 files changed, 15 insertions(+), 26 deletions(-) > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > index d1849fe874ab..8ada1cac8fcb 100644 > --- a/arch/x86/kvm/cpuid.c > +++ b/arch/x86/kvm/cpuid.c > @@ -403,6 +403,8 @@ static u32 cpuid_get_reg_unsafe(struct kvm_cpuid_entry2 *entry, u32 reg) > } > } > > +static int cpuid_func_emulated(struct kvm_cpuid_entry2 *entry, u32 func); > + > void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu) > { > struct kvm_lapic *apic = vcpu->arch.apic; > @@ -421,6 +423,7 @@ void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu) > */ > for (i = 0; i < NR_KVM_CPU_CAPS; i++) { > const struct cpuid_reg cpuid = reverse_cpuid[i]; > + struct kvm_cpuid_entry2 emulated; > > if (!cpuid.function) > continue; > @@ -429,7 +432,16 @@ void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu) > if (!entry) > continue; > > - vcpu->arch.cpu_caps[i] = cpuid_get_reg_unsafe(entry, cpuid.reg); > + cpuid_func_emulated(&emulated, cpuid.function); > + > + /* > + * A vCPU has a feature if it's supported by KVM and is enabled > + * in guest CPUID. Note, this includes features that are > + * supported by KVM but aren't advertised to userspace! > + */ > + vcpu->arch.cpu_caps[i] = kvm_cpu_caps[i] | kvm_vmm_cpu_caps[i] | > + cpuid_get_reg_unsafe(&emulated, cpuid.reg); > + vcpu->arch.cpu_caps[i] &= cpuid_get_reg_unsafe(entry, cpuid.reg); Hi, I have an idea. What if we get rid of kvm_vmm_cpu_caps, and instead advertise the MWAIT in KVM_GET_EMULATED_CPUID? MWAIT is sort of emulated as NOP after all, plus features in KVM_GET_EMULATED_CPUID are sort of 'emulated inefficiently' and you can say that NOP is an inefficient emulation of MWAIT sort of. It just feels to me that kvm_vmm_cpu_caps, is somewhat an overkill, and its name is somewhat confusing. Other than that this code looks good. > } > > kvm_update_cpuid_runtime(vcpu); > diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h > index c2c2b8aa347b..60da304db4e4 100644 > --- a/arch/x86/kvm/cpuid.h > +++ b/arch/x86/kvm/cpuid.h > @@ -284,13 +284,6 @@ static __always_inline void guest_cpu_cap_change(struct kvm_vcpu *vcpu, > guest_cpu_cap_clear(vcpu, x86_feature); > } > > -static __always_inline void guest_cpu_cap_constrain(struct kvm_vcpu *vcpu, > - unsigned int x86_feature) > -{ > - if (!kvm_cpu_cap_has(x86_feature)) > - guest_cpu_cap_clear(vcpu, x86_feature); > -} > - > static __always_inline bool guest_cpu_cap_has(struct kvm_vcpu *vcpu, > unsigned int x86_feature) > { > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c > index 1bc431a7e862..946a75771946 100644 > --- a/arch/x86/kvm/svm/svm.c > +++ b/arch/x86/kvm/svm/svm.c > @@ -4344,10 +4344,6 @@ static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu) > boot_cpu_has(X86_FEATURE_XSAVES) && > guest_cpuid_has(vcpu, X86_FEATURE_XSAVE)); > > - guest_cpu_cap_constrain(vcpu, X86_FEATURE_NRIPS); > - guest_cpu_cap_constrain(vcpu, X86_FEATURE_TSCRATEMSR); > - guest_cpu_cap_constrain(vcpu, X86_FEATURE_LBRV); > - > /* > * Intercept VMLOAD if the vCPU mode is Intel in order to emulate that > * VMLOAD drops bits 63:32 of SYSENTER (ignoring the fact that exposing > @@ -4355,13 +4351,6 @@ static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu) > */ > if (guest_cpuid_is_intel(vcpu)) > guest_cpu_cap_clear(vcpu, X86_FEATURE_V_VMSAVE_VMLOAD); > - else > - guest_cpu_cap_constrain(vcpu, X86_FEATURE_V_VMSAVE_VMLOAD); > - > - guest_cpu_cap_constrain(vcpu, X86_FEATURE_PAUSEFILTER); > - guest_cpu_cap_constrain(vcpu, X86_FEATURE_PFTHRESHOLD); > - guest_cpu_cap_constrain(vcpu, X86_FEATURE_VGIF); > - guest_cpu_cap_constrain(vcpu, X86_FEATURE_VNMI); Finally, this code is gone. > > svm_recalc_instruction_intercepts(vcpu, svm); > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index d873386e1473..653c4b68ec7f 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -7836,15 +7836,10 @@ void vmx_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu) > * to the guest. XSAVES depends on CR4.OSXSAVE, and CR4.OSXSAVE can be > * set if and only if XSAVE is supported. > */ > - if (boot_cpu_has(X86_FEATURE_XSAVE) && > - guest_cpuid_has(vcpu, X86_FEATURE_XSAVE)) > - guest_cpu_cap_constrain(vcpu, X86_FEATURE_XSAVES); > - else > + if (!boot_cpu_has(X86_FEATURE_XSAVE) || > + !guest_cpuid_has(vcpu, X86_FEATURE_XSAVE)) > guest_cpu_cap_clear(vcpu, X86_FEATURE_XSAVES); > > - guest_cpu_cap_constrain(vcpu, X86_FEATURE_VMX); > - guest_cpu_cap_constrain(vcpu, X86_FEATURE_LAM); Good riddance! > - > vmx_setup_uret_msrs(vmx); > > if (cpu_has_secondary_exec_ctrls()) Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com> Best regards, Maxim Levitsky
On Thu, Jul 04, 2024, Maxim Levitsky wrote: > On Fri, 2024-05-17 at 10:39 -0700, Sean Christopherson wrote: > > @@ -421,6 +423,7 @@ void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu) > > */ > > for (i = 0; i < NR_KVM_CPU_CAPS; i++) { > > const struct cpuid_reg cpuid = reverse_cpuid[i]; > > + struct kvm_cpuid_entry2 emulated; > > > > if (!cpuid.function) > > continue; > > @@ -429,7 +432,16 @@ void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu) > > if (!entry) > > continue; > > > > - vcpu->arch.cpu_caps[i] = cpuid_get_reg_unsafe(entry, cpuid.reg); > > + cpuid_func_emulated(&emulated, cpuid.function); > > + > > + /* > > + * A vCPU has a feature if it's supported by KVM and is enabled > > + * in guest CPUID. Note, this includes features that are > > + * supported by KVM but aren't advertised to userspace! > > + */ > > + vcpu->arch.cpu_caps[i] = kvm_cpu_caps[i] | kvm_vmm_cpu_caps[i] | > > + cpuid_get_reg_unsafe(&emulated, cpuid.reg); > > + vcpu->arch.cpu_caps[i] &= cpuid_get_reg_unsafe(entry, cpuid.reg); > > Hi, > > I have an idea. What if we get rid of kvm_vmm_cpu_caps, and instead advertise the > MWAIT in KVM_GET_EMULATED_CPUID? > > MWAIT is sort of emulated as NOP after all, plus features in KVM_GET_EMULATED_CPUID are > sort of 'emulated inefficiently' and you can say that NOP is an inefficient emulation > of MWAIT sort of. Heh, sort of indeed. I really don't want to advertise MWAIT to userspace in any capacity beyond KVM_CAP_X86_DISABLE_EXITS, because advertising MWAIT to VMs when MONITOR/MWAIT exiting is enabled is actively harmful, to both host and guest. KVM also doesn't emulate them on #UD, unlike MOVBE, which would make the API even more confusing than it already is. > It just feels to me that kvm_vmm_cpu_caps, is somewhat an overkill, and its name is > somewhat confusing. Yeah, I don't love it either, but trying to handle MWAIT as a one-off was even uglier. One option would be to piggyback cpuid_func_emulated(), but add a param to have it fill MWAIT only for KVM's internal purposes. That'd essentially be the same as a one-off in kvm_vcpu_after_set_cpuid(), but less ugly. I'd say it comes down to whether or not we expect to have more features that KVM "supports", but doesn't advertise to userspace. If we do, then I think adding VMM_F() is the way to go. If we expect MWAIT to be the only feature that gets this treatment, then I'm ok if we bastardize cpuid_func_emulated(). And I think/hope that MWAIT will be a one-off. Emulating it as a nop was a mistake and has since been quirked, and I like to think we (eventually) learn from our mistakes. diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index 0e64a6332052..dbc3f6ce9203 100644 --- a/arch/x86/kvm/cpuid.c +++ b/arch/x86/kvm/cpuid.c @@ -448,7 +448,7 @@ void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu) if (!entry) continue; - cpuid_func_emulated(&emulated, cpuid.function); + cpuid_func_emulated(&emulated, cpuid.function, false); /* * A vCPU has a feature if it's supported by KVM and is enabled @@ -1034,7 +1034,8 @@ static struct kvm_cpuid_entry2 *do_host_cpuid(struct kvm_cpuid_array *array, return entry; } -static int cpuid_func_emulated(struct kvm_cpuid_entry2 *entry, u32 func) +static int cpuid_func_emulated(struct kvm_cpuid_entry2 *entry, u32 func, + bool only_advertised) { memset(entry, 0, sizeof(*entry)); @@ -1048,6 +1049,9 @@ static int cpuid_func_emulated(struct kvm_cpuid_entry2 *entry, u32 func) return 1; case 1: entry->ecx = F(MOVBE); + /* comment goes here. */ + if (!only_advertised) + entry->ecx |= F(MWAIT); return 1; case 7: entry->flags |= KVM_CPUID_FLAG_SIGNIFCANT_INDEX; @@ -1065,7 +1069,7 @@ static int __do_cpuid_func_emulated(struct kvm_cpuid_array *array, u32 func) if (array->nent >= array->maxnent) return -E2BIG; - array->nent += cpuid_func_emulated(&array->entries[array->nent], func); + array->nent += cpuid_func_emulated(&array->entries[array->nent], func, true); return 0; }
On Mon, 2024-07-08 at 17:10 -0700, Sean Christopherson wrote: > On Thu, Jul 04, 2024, Maxim Levitsky wrote: > > On Fri, 2024-05-17 at 10:39 -0700, Sean Christopherson wrote: > > > @@ -421,6 +423,7 @@ void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu) > > > */ > > > for (i = 0; i < NR_KVM_CPU_CAPS; i++) { > > > const struct cpuid_reg cpuid = reverse_cpuid[i]; > > > + struct kvm_cpuid_entry2 emulated; > > > > > > if (!cpuid.function) > > > continue; > > > @@ -429,7 +432,16 @@ void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu) > > > if (!entry) > > > continue; > > > > > > - vcpu->arch.cpu_caps[i] = cpuid_get_reg_unsafe(entry, cpuid.reg); > > > + cpuid_func_emulated(&emulated, cpuid.function); > > > + > > > + /* > > > + * A vCPU has a feature if it's supported by KVM and is enabled > > > + * in guest CPUID. Note, this includes features that are > > > + * supported by KVM but aren't advertised to userspace! > > > + */ > > > + vcpu->arch.cpu_caps[i] = kvm_cpu_caps[i] | kvm_vmm_cpu_caps[i] | > > > + cpuid_get_reg_unsafe(&emulated, cpuid.reg); > > > + vcpu->arch.cpu_caps[i] &= cpuid_get_reg_unsafe(entry, cpuid.reg); > > > > Hi, > > > > I have an idea. What if we get rid of kvm_vmm_cpu_caps, and instead advertise the > > MWAIT in KVM_GET_EMULATED_CPUID? > > > > MWAIT is sort of emulated as NOP after all, plus features in KVM_GET_EMULATED_CPUID are > > sort of 'emulated inefficiently' and you can say that NOP is an inefficient emulation > > of MWAIT sort of. > > Heh, sort of indeed. I really don't want to advertise MWAIT to userspace in any > capacity beyond KVM_CAP_X86_DISABLE_EXITS, because advertising MWAIT to VMs when > MONITOR/MWAIT exiting is enabled is actively harmful, to both host and guest. Assuming that the only purpose of the KVM_GET_EMULATED_CPUID is to allow the guest to use a feature if it really insists, there should be no harm, but yes, I understand your concert here. > > KVM also doesn't emulate them on #UD, unlike MOVBE, which would make the API even > more confusing than it already is. This is even bigger justification for not doing this. > > > It just feels to me that kvm_vmm_cpu_caps, is somewhat an overkill, and its name is > > somewhat confusing. > > Yeah, I don't love it either, but trying to handle MWAIT as a one-off was even > uglier. One option would be to piggyback cpuid_func_emulated(), but add a param > to have it fill MWAIT only for KVM's internal purposes. That'd essentially be > the same as a one-off in kvm_vcpu_after_set_cpuid(), but less ugly. > > I'd say it comes down to whether or not we expect to have more features that KVM > "supports", but doesn't advertise to userspace. If we do, then I think adding > VMM_F() is the way to go. If we expect MWAIT to be the only feature that gets > this treatment, then I'm ok if we bastardize cpuid_func_emulated(). > > And I think/hope that MWAIT will be a one-off. Emulating it as a nop was a > mistake and has since been quirked, and I like to think we (eventually) learn > from our mistakes. > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > index 0e64a6332052..dbc3f6ce9203 100644 > --- a/arch/x86/kvm/cpuid.c > +++ b/arch/x86/kvm/cpuid.c > @@ -448,7 +448,7 @@ void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu) > if (!entry) > continue; > > - cpuid_func_emulated(&emulated, cpuid.function); > + cpuid_func_emulated(&emulated, cpuid.function, false); > > /* > * A vCPU has a feature if it's supported by KVM and is enabled > @@ -1034,7 +1034,8 @@ static struct kvm_cpuid_entry2 *do_host_cpuid(struct kvm_cpuid_array *array, > return entry; > } > > -static int cpuid_func_emulated(struct kvm_cpuid_entry2 *entry, u32 func) > +static int cpuid_func_emulated(struct kvm_cpuid_entry2 *entry, u32 func, > + bool only_advertised) I'll say, lets call this boolean, 'include_partially_emulated', (basically features that kvm emulates but only partially, and thus doesn't advertise, aka mwait) and then it doesn't look that bad, assuming that comes with a comment. > { > memset(entry, 0, sizeof(*entry)); > > @@ -1048,6 +1049,9 @@ static int cpuid_func_emulated(struct kvm_cpuid_entry2 *entry, u32 func) > return 1; > case 1: > entry->ecx = F(MOVBE); > + /* comment goes here. */ > + if (!only_advertised) And here if(include_partially_emulated) ... It sort of even self-documents nature of mwait emulation. > + entry->ecx |= F(MWAIT); > return 1; > case 7: > entry->flags |= KVM_CPUID_FLAG_SIGNIFCANT_INDEX; > @@ -1065,7 +1069,7 @@ static int __do_cpuid_func_emulated(struct kvm_cpuid_array *array, u32 func) > if (array->nent >= array->maxnent) > return -E2BIG; > > - array->nent += cpuid_func_emulated(&array->entries[array->nent], func); > + array->nent += cpuid_func_emulated(&array->entries[array->nent], func, true); > return 0; > } > Best regards, Maxim Levitsky
On Wed, Jul 24, 2024, Maxim Levitsky wrote: > On Mon, 2024-07-08 at 17:10 -0700, Sean Christopherson wrote: > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > > index 0e64a6332052..dbc3f6ce9203 100644 > > --- a/arch/x86/kvm/cpuid.c > > +++ b/arch/x86/kvm/cpuid.c > > @@ -448,7 +448,7 @@ void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu) > > if (!entry) > > continue; > > > > - cpuid_func_emulated(&emulated, cpuid.function); > > + cpuid_func_emulated(&emulated, cpuid.function, false); > > > > /* > > * A vCPU has a feature if it's supported by KVM and is enabled > > @@ -1034,7 +1034,8 @@ static struct kvm_cpuid_entry2 *do_host_cpuid(struct kvm_cpuid_array *array, > > return entry; > > } > > > > -static int cpuid_func_emulated(struct kvm_cpuid_entry2 *entry, u32 func) > > +static int cpuid_func_emulated(struct kvm_cpuid_entry2 *entry, u32 func, > > + bool only_advertised) > > I'll say, lets call this boolean, 'include_partially_emulated', > (basically features that kvm emulates but only partially, > and thus doesn't advertise, aka mwait) > > and then it doesn't look that bad, assuming that comes with a comment. Works for me. I was trying to figure out a way to say "emulated_on_ud", but I can't get the polarity right, at least not without ridiculous verbosity. E.g. include_not_emulated_on_ud is awful.
У пн, 2024-07-29 у 08:34 -0700, Sean Christopherson пише: > > On Wed, Jul 24, 2024, Maxim Levitsky wrote: > > > > On Mon, 2024-07-08 at 17:10 -0700, Sean Christopherson wrote: > > > > > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > > > > > > index 0e64a6332052..dbc3f6ce9203 100644 > > > > > > --- a/arch/x86/kvm/cpuid.c > > > > > > +++ b/arch/x86/kvm/cpuid.c > > > > > > @@ -448,7 +448,7 @@ void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu) > > > > > > if (!entry) > > > > > > continue; > > > > > > > > > > > > - cpuid_func_emulated(&emulated, cpuid.function); > > > > > > + cpuid_func_emulated(&emulated, cpuid.function, false); > > > > > > > > > > > > /* > > > > > > * A vCPU has a feature if it's supported by KVM and is enabled > > > > > > @@ -1034,7 +1034,8 @@ static struct kvm_cpuid_entry2 *do_host_cpuid(struct kvm_cpuid_array *array, > > > > > > return entry; > > > > > > } > > > > > > > > > > > > -static int cpuid_func_emulated(struct kvm_cpuid_entry2 *entry, u32 func) > > > > > > +static int cpuid_func_emulated(struct kvm_cpuid_entry2 *entry, u32 func, > > > > > > + bool only_advertised) > > > > > > > > I'll say, lets call this boolean, 'include_partially_emulated', > > > > (basically features that kvm emulates but only partially, > > > > and thus doesn't advertise, aka mwait) > > > > > > > > and then it doesn't look that bad, assuming that comes with a comment. > > > > Works for me. I was trying to figure out a way to say "emulated_on_ud", but I > > can't get the polarity right, at least not without ridiculous verbosity. E.g. > > include_not_emulated_on_ud is awful. > > Thanks, Best regards, Maxim Levitsky
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index d1849fe874ab..8ada1cac8fcb 100644 --- a/arch/x86/kvm/cpuid.c +++ b/arch/x86/kvm/cpuid.c @@ -403,6 +403,8 @@ static u32 cpuid_get_reg_unsafe(struct kvm_cpuid_entry2 *entry, u32 reg) } } +static int cpuid_func_emulated(struct kvm_cpuid_entry2 *entry, u32 func); + void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu) { struct kvm_lapic *apic = vcpu->arch.apic; @@ -421,6 +423,7 @@ void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu) */ for (i = 0; i < NR_KVM_CPU_CAPS; i++) { const struct cpuid_reg cpuid = reverse_cpuid[i]; + struct kvm_cpuid_entry2 emulated; if (!cpuid.function) continue; @@ -429,7 +432,16 @@ void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu) if (!entry) continue; - vcpu->arch.cpu_caps[i] = cpuid_get_reg_unsafe(entry, cpuid.reg); + cpuid_func_emulated(&emulated, cpuid.function); + + /* + * A vCPU has a feature if it's supported by KVM and is enabled + * in guest CPUID. Note, this includes features that are + * supported by KVM but aren't advertised to userspace! + */ + vcpu->arch.cpu_caps[i] = kvm_cpu_caps[i] | kvm_vmm_cpu_caps[i] | + cpuid_get_reg_unsafe(&emulated, cpuid.reg); + vcpu->arch.cpu_caps[i] &= cpuid_get_reg_unsafe(entry, cpuid.reg); } kvm_update_cpuid_runtime(vcpu); diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h index c2c2b8aa347b..60da304db4e4 100644 --- a/arch/x86/kvm/cpuid.h +++ b/arch/x86/kvm/cpuid.h @@ -284,13 +284,6 @@ static __always_inline void guest_cpu_cap_change(struct kvm_vcpu *vcpu, guest_cpu_cap_clear(vcpu, x86_feature); } -static __always_inline void guest_cpu_cap_constrain(struct kvm_vcpu *vcpu, - unsigned int x86_feature) -{ - if (!kvm_cpu_cap_has(x86_feature)) - guest_cpu_cap_clear(vcpu, x86_feature); -} - static __always_inline bool guest_cpu_cap_has(struct kvm_vcpu *vcpu, unsigned int x86_feature) { diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c index 1bc431a7e862..946a75771946 100644 --- a/arch/x86/kvm/svm/svm.c +++ b/arch/x86/kvm/svm/svm.c @@ -4344,10 +4344,6 @@ static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu) boot_cpu_has(X86_FEATURE_XSAVES) && guest_cpuid_has(vcpu, X86_FEATURE_XSAVE)); - guest_cpu_cap_constrain(vcpu, X86_FEATURE_NRIPS); - guest_cpu_cap_constrain(vcpu, X86_FEATURE_TSCRATEMSR); - guest_cpu_cap_constrain(vcpu, X86_FEATURE_LBRV); - /* * Intercept VMLOAD if the vCPU mode is Intel in order to emulate that * VMLOAD drops bits 63:32 of SYSENTER (ignoring the fact that exposing @@ -4355,13 +4351,6 @@ static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu) */ if (guest_cpuid_is_intel(vcpu)) guest_cpu_cap_clear(vcpu, X86_FEATURE_V_VMSAVE_VMLOAD); - else - guest_cpu_cap_constrain(vcpu, X86_FEATURE_V_VMSAVE_VMLOAD); - - guest_cpu_cap_constrain(vcpu, X86_FEATURE_PAUSEFILTER); - guest_cpu_cap_constrain(vcpu, X86_FEATURE_PFTHRESHOLD); - guest_cpu_cap_constrain(vcpu, X86_FEATURE_VGIF); - guest_cpu_cap_constrain(vcpu, X86_FEATURE_VNMI); svm_recalc_instruction_intercepts(vcpu, svm); diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index d873386e1473..653c4b68ec7f 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -7836,15 +7836,10 @@ void vmx_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu) * to the guest. XSAVES depends on CR4.OSXSAVE, and CR4.OSXSAVE can be * set if and only if XSAVE is supported. */ - if (boot_cpu_has(X86_FEATURE_XSAVE) && - guest_cpuid_has(vcpu, X86_FEATURE_XSAVE)) - guest_cpu_cap_constrain(vcpu, X86_FEATURE_XSAVES); - else + if (!boot_cpu_has(X86_FEATURE_XSAVE) || + !guest_cpuid_has(vcpu, X86_FEATURE_XSAVE)) guest_cpu_cap_clear(vcpu, X86_FEATURE_XSAVES); - guest_cpu_cap_constrain(vcpu, X86_FEATURE_VMX); - guest_cpu_cap_constrain(vcpu, X86_FEATURE_LAM); - vmx_setup_uret_msrs(vmx); if (cpu_has_secondary_exec_ctrls())
Constrain all guest cpu_caps based on KVM support instead of constraining only the few features that KVM _currently_ needs to verify are actually supported by KVM. The intent of cpu_caps is to track what the guest is actually capable of using, not the raw, unfiltered CPUID values that the guest sees. I.e. KVM should always consult it's only support when making decisions based on guest CPUID, and the only reason KVM has historically made the checks opt-in was due to lack of centralized tracking. Suggested-by: Maxim Levitsky <mlevitsk@redhat.com> Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/kvm/cpuid.c | 14 +++++++++++++- arch/x86/kvm/cpuid.h | 7 ------- arch/x86/kvm/svm/svm.c | 11 ----------- arch/x86/kvm/vmx/vmx.c | 9 ++------- 4 files changed, 15 insertions(+), 26 deletions(-)