| Message ID | 20240517173926.965351-30-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 only searches for KVM's PV CPUID base when userspace sets > guest CPUID, drop the cache and simply do the search every time. > > Practically speaking, this is a nop except for situations where userspace > sets CPUID _after_ running the vCPU, which is anything but a hot path, > e.g. QEMU does so only when hotplugging a vCPU. And on the flip side, > caching guest CPUID information, especially information that is used to > query/modify _other_ CPUID state, is inherently dangerous as it's all too > easy to use stale information, i.e. KVM should only cache CPUID state when > the performance and/or programming benefits justify it. > > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/include/asm/kvm_host.h | 1 - > arch/x86/kvm/cpuid.c | 34 +++++++-------------------------- > 2 files changed, 7 insertions(+), 28 deletions(-) > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index aabf1648a56a..3003e99155e7 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -858,7 +858,6 @@ struct kvm_vcpu_arch { > > int cpuid_nent; > struct kvm_cpuid_entry2 *cpuid_entries; > - struct kvm_hypervisor_cpuid kvm_cpuid; > bool is_amd_compatible; > > /* > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > index 93a7399dc0db..7290f91c422c 100644 > --- a/arch/x86/kvm/cpuid.c > +++ b/arch/x86/kvm/cpuid.c > @@ -269,28 +269,16 @@ static struct kvm_hypervisor_cpuid kvm_get_hypervisor_cpuid(struct kvm_vcpu *vcp > vcpu->arch.cpuid_nent, sig); > } > > -static struct kvm_cpuid_entry2 *__kvm_find_kvm_cpuid_features(struct kvm_cpuid_entry2 *entries, > - int nent, u32 kvm_cpuid_base) > -{ > - return cpuid_entry2_find(entries, nent, kvm_cpuid_base | KVM_CPUID_FEATURES, > - KVM_CPUID_INDEX_NOT_SIGNIFICANT); > -} > - > -static struct kvm_cpuid_entry2 *kvm_find_kvm_cpuid_features(struct kvm_vcpu *vcpu) > -{ > - u32 base = vcpu->arch.kvm_cpuid.base; > - > - if (!base) > - return NULL; > - > - return __kvm_find_kvm_cpuid_features(vcpu->arch.cpuid_entries, > - vcpu->arch.cpuid_nent, base); > -} > - > static u32 kvm_apply_cpuid_pv_features_quirk(struct kvm_vcpu *vcpu) > { > - struct kvm_cpuid_entry2 *best = kvm_find_kvm_cpuid_features(vcpu); > + struct kvm_hypervisor_cpuid kvm_cpuid; > + struct kvm_cpuid_entry2 *best; > > + kvm_cpuid = kvm_get_hypervisor_cpuid(vcpu, KVM_SIGNATURE); > + if (!kvm_cpuid.base) > + return 0; > + > + best = kvm_find_cpuid_entry(vcpu, kvm_cpuid.base | KVM_CPUID_FEATURES); > if (!best) > return 0; > > @@ -491,13 +479,6 @@ static int kvm_set_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *e2, > * whether the supplied CPUID data is equal to what's already set. > */ > if (kvm_vcpu_has_run(vcpu)) { > - /* > - * Note, runtime CPUID updates may consume other CPUID-driven > - * vCPU state, e.g. KVM or Xen CPUID bases. Updating runtime > - * state before full CPUID processing is functionally correct > - * only because any change in CPUID is disallowed, i.e. using > - * stale data is ok because KVM will reject the change. > - */ Hi, Any reason why this comment was removed? As I said earlier in the review. It might make sense to replace this comment with a comment reflecting on why we need to call kvm_update_cpuid_runtime, that is solely to allow old == new compare to succeed. > kvm_update_cpuid_runtime(vcpu); > kvm_apply_cpuid_pv_features_quirk(vcpu); > > @@ -519,7 +500,6 @@ static int kvm_set_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *e2, > if (r) > goto err; > > - vcpu->arch.kvm_cpuid = kvm_get_hypervisor_cpuid(vcpu, KVM_SIGNATURE); > #ifdef CONFIG_KVM_XEN > vcpu->arch.xen.cpuid = kvm_get_hypervisor_cpuid(vcpu, XEN_SIGNATURE); > #endif Best regards, Maxim Levitsky
On Thu, Jul 04, 2024, Maxim Levitsky wrote: > On Fri, 2024-05-17 at 10:39 -0700, Sean Christopherson wrote: > > Now that KVM only searches for KVM's PV CPUID base when userspace sets > > guest CPUID, drop the cache and simply do the search every time. > > > > Practically speaking, this is a nop except for situations where userspace > > sets CPUID _after_ running the vCPU, which is anything but a hot path, > > e.g. QEMU does so only when hotplugging a vCPU. And on the flip side, > > caching guest CPUID information, especially information that is used to > > query/modify _other_ CPUID state, is inherently dangerous as it's all too > > easy to use stale information, i.e. KVM should only cache CPUID state when > > the performance and/or programming benefits justify it. > > > > Signed-off-by: Sean Christopherson <seanjc@google.com> > > --- ... > > @@ -491,13 +479,6 @@ static int kvm_set_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *e2, > > * whether the supplied CPUID data is equal to what's already set. > > */ > > if (kvm_vcpu_has_run(vcpu)) { > > - /* > > - * Note, runtime CPUID updates may consume other CPUID-driven > > - * vCPU state, e.g. KVM or Xen CPUID bases. Updating runtime > > - * state before full CPUID processing is functionally correct > > - * only because any change in CPUID is disallowed, i.e. using > > - * stale data is ok because KVM will reject the change. > > - */ > Hi, > > Any reason why this comment was removed? Because after this patch, runtime CPUID updates no longer consume other vCPU state that is derived from guest CPUID. > As I said earlier in the review. It might make sense to replace this comment > with a comment reflecting on why we need to call kvm_update_cpuid_runtime, > that is solely to allow old == new compare to succeed. Ya, I'll figure out a location and patch to document why KVM applies runtime and quirks to the CPUID before checking. > > > kvm_update_cpuid_runtime(vcpu); > > kvm_apply_cpuid_pv_features_quirk(vcpu);
On Tue, 2024-07-09 at 12:00 -0700, Sean Christopherson wrote: > On Thu, Jul 04, 2024, Maxim Levitsky wrote: > > On Fri, 2024-05-17 at 10:39 -0700, Sean Christopherson wrote: > > > Now that KVM only searches for KVM's PV CPUID base when userspace sets > > > guest CPUID, drop the cache and simply do the search every time. > > > > > > Practically speaking, this is a nop except for situations where userspace > > > sets CPUID _after_ running the vCPU, which is anything but a hot path, > > > e.g. QEMU does so only when hotplugging a vCPU. And on the flip side, > > > caching guest CPUID information, especially information that is used to > > > query/modify _other_ CPUID state, is inherently dangerous as it's all too > > > easy to use stale information, i.e. KVM should only cache CPUID state when > > > the performance and/or programming benefits justify it. > > > > > > Signed-off-by: Sean Christopherson <seanjc@google.com> > > > --- > > ... > > > > @@ -491,13 +479,6 @@ static int kvm_set_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *e2, > > > * whether the supplied CPUID data is equal to what's already set. > > > */ > > > if (kvm_vcpu_has_run(vcpu)) { > > > - /* > > > - * Note, runtime CPUID updates may consume other CPUID-driven > > > - * vCPU state, e.g. KVM or Xen CPUID bases. Updating runtime > > > - * state before full CPUID processing is functionally correct > > > - * only because any change in CPUID is disallowed, i.e. using > > > - * stale data is ok because KVM will reject the change. > > > - */ > > Hi, > > > > Any reason why this comment was removed? > > Because after this patch, runtime CPUID updates no longer consume other vCPU > state that is derived from guest CPUID. > > > As I said earlier in the review. It might make sense to replace this comment > > with a comment reflecting on why we need to call kvm_update_cpuid_runtime, > > that is solely to allow old == new compare to succeed. > > Ya, I'll figure out a location and patch to document why KVM applies runtime > and quirks to the CPUID before checking. > > > > kvm_update_cpuid_runtime(vcpu); > > > kvm_apply_cpuid_pv_features_quirk(vcpu); Makes sense, thanks! Best regards, Maxim Levitsky
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index aabf1648a56a..3003e99155e7 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -858,7 +858,6 @@ struct kvm_vcpu_arch { int cpuid_nent; struct kvm_cpuid_entry2 *cpuid_entries; - struct kvm_hypervisor_cpuid kvm_cpuid; bool is_amd_compatible; /* diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index 93a7399dc0db..7290f91c422c 100644 --- a/arch/x86/kvm/cpuid.c +++ b/arch/x86/kvm/cpuid.c @@ -269,28 +269,16 @@ static struct kvm_hypervisor_cpuid kvm_get_hypervisor_cpuid(struct kvm_vcpu *vcp vcpu->arch.cpuid_nent, sig); } -static struct kvm_cpuid_entry2 *__kvm_find_kvm_cpuid_features(struct kvm_cpuid_entry2 *entries, - int nent, u32 kvm_cpuid_base) -{ - return cpuid_entry2_find(entries, nent, kvm_cpuid_base | KVM_CPUID_FEATURES, - KVM_CPUID_INDEX_NOT_SIGNIFICANT); -} - -static struct kvm_cpuid_entry2 *kvm_find_kvm_cpuid_features(struct kvm_vcpu *vcpu) -{ - u32 base = vcpu->arch.kvm_cpuid.base; - - if (!base) - return NULL; - - return __kvm_find_kvm_cpuid_features(vcpu->arch.cpuid_entries, - vcpu->arch.cpuid_nent, base); -} - static u32 kvm_apply_cpuid_pv_features_quirk(struct kvm_vcpu *vcpu) { - struct kvm_cpuid_entry2 *best = kvm_find_kvm_cpuid_features(vcpu); + struct kvm_hypervisor_cpuid kvm_cpuid; + struct kvm_cpuid_entry2 *best; + kvm_cpuid = kvm_get_hypervisor_cpuid(vcpu, KVM_SIGNATURE); + if (!kvm_cpuid.base) + return 0; + + best = kvm_find_cpuid_entry(vcpu, kvm_cpuid.base | KVM_CPUID_FEATURES); if (!best) return 0; @@ -491,13 +479,6 @@ static int kvm_set_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *e2, * whether the supplied CPUID data is equal to what's already set. */ if (kvm_vcpu_has_run(vcpu)) { - /* - * Note, runtime CPUID updates may consume other CPUID-driven - * vCPU state, e.g. KVM or Xen CPUID bases. Updating runtime - * state before full CPUID processing is functionally correct - * only because any change in CPUID is disallowed, i.e. using - * stale data is ok because KVM will reject the change. - */ kvm_update_cpuid_runtime(vcpu); kvm_apply_cpuid_pv_features_quirk(vcpu); @@ -519,7 +500,6 @@ static int kvm_set_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *e2, if (r) goto err; - vcpu->arch.kvm_cpuid = kvm_get_hypervisor_cpuid(vcpu, KVM_SIGNATURE); #ifdef CONFIG_KVM_XEN vcpu->arch.xen.cpuid = kvm_get_hypervisor_cpuid(vcpu, XEN_SIGNATURE); #endif
Now that KVM only searches for KVM's PV CPUID base when userspace sets guest CPUID, drop the cache and simply do the search every time. Practically speaking, this is a nop except for situations where userspace sets CPUID _after_ running the vCPU, which is anything but a hot path, e.g. QEMU does so only when hotplugging a vCPU. And on the flip side, caching guest CPUID information, especially information that is used to query/modify _other_ CPUID state, is inherently dangerous as it's all too easy to use stale information, i.e. KVM should only cache CPUID state when the performance and/or programming benefits justify it. Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/include/asm/kvm_host.h | 1 - arch/x86/kvm/cpuid.c | 34 +++++++-------------------------- 2 files changed, 7 insertions(+), 28 deletions(-)