Re: [PATCH] KVM: x86: Update Xen-specific CPUID leaves during mangling

[Paul Durrant]

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);