| Message ID | 20240517173926.965351-29-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: > Now that KVM disallows disabling HLT-exiting after vCPUs have been created, > i.e. now that it's impossible for kvm_hlt_in_guest() to change while vCPUs > are running, apply KVM's PV_UNHALT quirk only when userspace is setting > guest CPUID. > > Opportunistically rename the helper to make it clear that KVM's behavior > is a quirk that should never have been added. KVM's documentation > explicitly states that userspace should not advertise PV_UNHALT if > HLT-exiting is disabled, but for unknown reasons, commit caa057a2cad6 > ("KVM: X86: Provide a capability to disable HLT intercepts") didn't stop > at documenting the requirement and also massaged the incoming guest CPUID. > > Unfortunately, it's quite likely that userspace has come to rely on KVM's > behavior, i.e. the code can't simply be deleted. The only reason KVM > doesn't have an "official" quirk is that there is no known use case where > disabling the quirk would make sense, i.e. letting userspace disable the > quirk would further increase KVM's burden without any benefit. Makes sense overall. > > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/kvm/cpuid.c | 26 +++++++++----------------- > 1 file changed, 9 insertions(+), 17 deletions(-) > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > index 4ad01867cb8d..93a7399dc0db 100644 > --- a/arch/x86/kvm/cpuid.c > +++ b/arch/x86/kvm/cpuid.c > @@ -287,18 +287,17 @@ static struct kvm_cpuid_entry2 *kvm_find_kvm_cpuid_features(struct kvm_vcpu *vcp > vcpu->arch.cpuid_nent, base); > } > > -static void kvm_update_pv_runtime(struct kvm_vcpu *vcpu) > +static u32 kvm_apply_cpuid_pv_features_quirk(struct kvm_vcpu *vcpu) > { > struct kvm_cpuid_entry2 *best = kvm_find_kvm_cpuid_features(vcpu); > > - vcpu->arch.pv_cpuid.features = 0; > + if (!best) > + return 0; > > - /* > - * save the feature bitmap to avoid cpuid lookup for every PV > - * operation > - */ > - if (best) > - vcpu->arch.pv_cpuid.features = best->eax; > + if (kvm_hlt_in_guest(vcpu->kvm)) > + best->eax &= ~(1 << KVM_FEATURE_PV_UNHALT); > + > + return best->eax; > } > > /* > @@ -320,7 +319,6 @@ static void __kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu, struct kvm_cpuid_e > int nent) > { > struct kvm_cpuid_entry2 *best; > - struct kvm_hypervisor_cpuid kvm_cpuid; > > best = cpuid_entry2_find(entries, nent, 1, KVM_CPUID_INDEX_NOT_SIGNIFICANT); > if (best) { > @@ -347,13 +345,6 @@ static void __kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu, struct kvm_cpuid_e > cpuid_entry_has(best, X86_FEATURE_XSAVEC))) > best->ebx = xstate_required_size(vcpu->arch.xcr0, true); > > - kvm_cpuid = __kvm_get_hypervisor_cpuid(entries, nent, KVM_SIGNATURE); > - if (kvm_cpuid.base) { > - best = __kvm_find_kvm_cpuid_features(entries, nent, kvm_cpuid.base); > - if (kvm_hlt_in_guest(vcpu->kvm) && best) > - best->eax &= ~(1 << KVM_FEATURE_PV_UNHALT); > - } > - > if (!kvm_check_has_quirk(vcpu->kvm, KVM_X86_QUIRK_MISC_ENABLE_NO_MWAIT)) { > best = cpuid_entry2_find(entries, nent, 0x1, KVM_CPUID_INDEX_NOT_SIGNIFICANT); > if (best) > @@ -425,7 +416,7 @@ void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu) > vcpu->arch.guest_supported_xcr0 = > cpuid_get_supported_xcr0(vcpu->arch.cpuid_entries, vcpu->arch.cpuid_nent); > > - kvm_update_pv_runtime(vcpu); > + vcpu->arch.pv_cpuid.features = kvm_apply_cpuid_pv_features_quirk(vcpu); > > vcpu->arch.is_amd_compatible = guest_cpuid_is_amd_or_hygon(vcpu); > vcpu->arch.maxphyaddr = cpuid_query_maxphyaddr(vcpu); > @@ -508,6 +499,7 @@ static int kvm_set_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *e2, > * stale data is ok because KVM will reject the change. > */ > kvm_update_cpuid_runtime(vcpu); > + kvm_apply_cpuid_pv_features_quirk(vcpu); > > r = kvm_cpuid_check_equal(vcpu, e2, nent); > if (r) 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 4ad01867cb8d..93a7399dc0db 100644 --- a/arch/x86/kvm/cpuid.c +++ b/arch/x86/kvm/cpuid.c @@ -287,18 +287,17 @@ static struct kvm_cpuid_entry2 *kvm_find_kvm_cpuid_features(struct kvm_vcpu *vcp vcpu->arch.cpuid_nent, base); } -static void kvm_update_pv_runtime(struct kvm_vcpu *vcpu) +static u32 kvm_apply_cpuid_pv_features_quirk(struct kvm_vcpu *vcpu) { struct kvm_cpuid_entry2 *best = kvm_find_kvm_cpuid_features(vcpu); - vcpu->arch.pv_cpuid.features = 0; + if (!best) + return 0; - /* - * save the feature bitmap to avoid cpuid lookup for every PV - * operation - */ - if (best) - vcpu->arch.pv_cpuid.features = best->eax; + if (kvm_hlt_in_guest(vcpu->kvm)) + best->eax &= ~(1 << KVM_FEATURE_PV_UNHALT); + + return best->eax; } /* @@ -320,7 +319,6 @@ static void __kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu, struct kvm_cpuid_e int nent) { struct kvm_cpuid_entry2 *best; - struct kvm_hypervisor_cpuid kvm_cpuid; best = cpuid_entry2_find(entries, nent, 1, KVM_CPUID_INDEX_NOT_SIGNIFICANT); if (best) { @@ -347,13 +345,6 @@ static void __kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu, struct kvm_cpuid_e cpuid_entry_has(best, X86_FEATURE_XSAVEC))) best->ebx = xstate_required_size(vcpu->arch.xcr0, true); - kvm_cpuid = __kvm_get_hypervisor_cpuid(entries, nent, KVM_SIGNATURE); - if (kvm_cpuid.base) { - best = __kvm_find_kvm_cpuid_features(entries, nent, kvm_cpuid.base); - if (kvm_hlt_in_guest(vcpu->kvm) && best) - best->eax &= ~(1 << KVM_FEATURE_PV_UNHALT); - } - if (!kvm_check_has_quirk(vcpu->kvm, KVM_X86_QUIRK_MISC_ENABLE_NO_MWAIT)) { best = cpuid_entry2_find(entries, nent, 0x1, KVM_CPUID_INDEX_NOT_SIGNIFICANT); if (best) @@ -425,7 +416,7 @@ void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu) vcpu->arch.guest_supported_xcr0 = cpuid_get_supported_xcr0(vcpu->arch.cpuid_entries, vcpu->arch.cpuid_nent); - kvm_update_pv_runtime(vcpu); + vcpu->arch.pv_cpuid.features = kvm_apply_cpuid_pv_features_quirk(vcpu); vcpu->arch.is_amd_compatible = guest_cpuid_is_amd_or_hygon(vcpu); vcpu->arch.maxphyaddr = cpuid_query_maxphyaddr(vcpu); @@ -508,6 +499,7 @@ static int kvm_set_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *e2, * stale data is ok because KVM will reject the change. */ kvm_update_cpuid_runtime(vcpu); + kvm_apply_cpuid_pv_features_quirk(vcpu); r = kvm_cpuid_check_equal(vcpu, e2, nent); if (r)
Now that KVM disallows disabling HLT-exiting after vCPUs have been created, i.e. now that it's impossible for kvm_hlt_in_guest() to change while vCPUs are running, apply KVM's PV_UNHALT quirk only when userspace is setting guest CPUID. Opportunistically rename the helper to make it clear that KVM's behavior is a quirk that should never have been added. KVM's documentation explicitly states that userspace should not advertise PV_UNHALT if HLT-exiting is disabled, but for unknown reasons, commit caa057a2cad6 ("KVM: X86: Provide a capability to disable HLT intercepts") didn't stop at documenting the requirement and also massaged the incoming guest CPUID. Unfortunately, it's quite likely that userspace has come to rely on KVM's behavior, i.e. the code can't simply be deleted. The only reason KVM doesn't have an "official" quirk is that there is no known use case where disabling the quirk would make sense, i.e. letting userspace disable the quirk would further increase KVM's burden without any benefit. Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/kvm/cpuid.c | 26 +++++++++----------------- 1 file changed, 9 insertions(+), 17 deletions(-)