On 22/01/2025 17:16, Vitaly Kuznetsov wrote:
Fred Griffoul <fgriffo@xxxxxxxxxxxx> writes:
Previous commit ee3a5f9e3d9b ("KVM: x86: Do runtime CPUID update before
updating vcpu->arch.cpuid_entries") implemented CPUID data mangling in
KVM_SET_CPUID2 support before verifying that no changes occur on running
vCPUs. However, it overlooked the CPUID leaves that are modified by
KVM's Xen emulation.
Fix this by calling a Xen update function when mangling CPUID data.
Fixes: ee3a5f9e3d9b ("KVM: x86: Do runtime CPUID update before
updating vcpu->arch.cpuid_entries")
Well, kvm_xen_update_tsc_info() was added with
commit f422f853af0369be27d2a9f1b20079f2bc3d1ca2
Author: Paul Durrant <pdurrant@xxxxxxxxxx>
Date: Fri Jan 6 10:36:00 2023 +0000
KVM: x86/xen: update Xen CPUID Leaf 4 (tsc info) sub-leaves, if present
and the commit you mention in 'Fixes' is older:
commit ee3a5f9e3d9bf94159f3cc80da542fbe83502dd8
Author: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>
Date: Mon Jan 17 16:05:39 2022 +0100
KVM: x86: Do runtime CPUID update before updating vcpu->arch.cpuid_entries
so I guess we should be 'Fixing' f422f853af03 instead :-)
Signed-off-by: Fred Griffoul <fgriffo@xxxxxxxxxxxx>
---
arch/x86/kvm/cpuid.c | 1 +
arch/x86/kvm/xen.c | 5 +++++
arch/x86/kvm/xen.h | 5 +++++
3 files changed, 11 insertions(+)
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index edef30359c19..432d8e9e1bab 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -212,6 +212,7 @@ static int kvm_cpuid_check_equal(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2
*/
kvm_update_cpuid_runtime(vcpu);
kvm_apply_cpuid_pv_features_quirk(vcpu);
+ kvm_xen_update_cpuid_runtime(vcpu);
This one is weird as we update it in runtime (kvm_guest_time_update())
and values may change when we e.g. migrate the guest. First, I do not
understand how the guest is supposed to notice the change as CPUID data
is normally considered static. Second, I do not see how the VMM is
supposed to track it as if it tries to supply some different data for
these Xen leaves, kvm_cpuid_check_equal() will still fail.
Would it make more sense to just ignore these Xen CPUID leaves with TSC
information when we do the comparison?
What is the purpose of the comparison anyway? IIUC we want to ensure
that a VMM does not change its mind after KVM_RUN so should we not be
stashing what was set by the VMM and comparing against that *before*
mangling any values?
if (nent != vcpu->arch.cpuid_nent)
return -EINVAL;
diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
index a909b817b9c0..219f9a9a92be 100644
--- a/arch/x86/kvm/xen.c
+++ b/arch/x86/kvm/xen.c
@@ -2270,6 +2270,11 @@ void kvm_xen_update_tsc_info(struct kvm_vcpu *vcpu)
entry->eax = vcpu->arch.hw_tsc_khz;
}
+void kvm_xen_update_cpuid_runtime(struct kvm_vcpu *vcpu)
+{
+ kvm_xen_update_tsc_info(vcpu);
+}
+
void kvm_xen_init_vm(struct kvm *kvm)
{
mutex_init(&kvm->arch.xen.xen_lock);
diff --git a/arch/x86/kvm/xen.h b/arch/x86/kvm/xen.h
index f5841d9000ae..d3182b0ab7e3 100644
--- a/arch/x86/kvm/xen.h
+++ b/arch/x86/kvm/xen.h
@@ -36,6 +36,7 @@ int kvm_xen_setup_evtchn(struct kvm *kvm,
struct kvm_kernel_irq_routing_entry *e,
const struct kvm_irq_routing_entry *ue);
void kvm_xen_update_tsc_info(struct kvm_vcpu *vcpu);
+void kvm_xen_update_cpuid_runtime(struct kvm_vcpu *vcpu);
static inline void kvm_xen_sw_enable_lapic(struct kvm_vcpu *vcpu)
{
@@ -160,6 +161,10 @@ static inline bool kvm_xen_timer_enabled(struct kvm_vcpu *vcpu)
static inline void kvm_xen_update_tsc_info(struct kvm_vcpu *vcpu)
{
}
+
+static inline void kvm_xen_update_cpuid_runtime(struct kvm_vcpu *vcpu)
+{
+}
#endif
int kvm_xen_hypercall(struct kvm_vcpu *vcpu);
