| Message ID | 20250122161612.20981-1-fgriffo@amazon.co.uk (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | KVM: x86: Update Xen-specific CPUID leaves during mangling | expand |
Fred Griffoul <fgriffo@amazon.co.uk> 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@amazon.com> 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@redhat.com> 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@amazon.co.uk> > --- > 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? > > 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);
On 22/01/2025 17:16, Vitaly Kuznetsov wrote: > Fred Griffoul <fgriffo@amazon.co.uk> 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@amazon.com> > 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@redhat.com> > 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@amazon.co.uk> >> --- >> 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); >
Paul Durrant <xadimgnik@gmail.com> writes: > On 22/01/2025 17:16, Vitaly Kuznetsov wrote: >> Fred Griffoul <fgriffo@amazon.co.uk> 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@amazon.com> >> 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@redhat.com> >> 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@amazon.co.uk> >>> --- >>> 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? > I guess it can be done this way but we will need to keep these 'original' unmangled values for the lifetime of the vCPU with very little gain (IMO): KVM_SET_CPUID{,2} either fails (if the data is different) or does (almost) nothing when the data is the same.
On Wed, 2025-01-22 at 18:44 +0100, Vitaly Kuznetsov wrote: > > > > > 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? > > > > I guess it can be done this way but we will need to keep these 'original' > unmangled values for the lifetime of the vCPU with very little gain (IMO): > KVM_SET_CPUID{,2} either fails (if the data is different) or does (almost) > nothing when the data is the same. If they're supposed to be entirely unchanged, would it suffice just to keep a hash of them?
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); 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);
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") Signed-off-by: Fred Griffoul <fgriffo@amazon.co.uk> --- arch/x86/kvm/cpuid.c | 1 + arch/x86/kvm/xen.c | 5 +++++ arch/x86/kvm/xen.h | 5 +++++ 3 files changed, 11 insertions(+)