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

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

[Fred Griffoul]

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(+)

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

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

[Vitaly Kuznetsov]

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

-- 
Vitaly

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

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

[Vitaly Kuznetsov]

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.

-- 
Vitaly

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

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 701 bytes --]

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?

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5069 bytes --]

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

[Sean Christopherson]

On Wed, Jan 22, 2025, David Woodhouse wrote:
> On Wed, 2025-01-22 at 18:44 +0100, Vitaly Kuznetsov wrote:
> > > What is the purpose of the comparison anyway?

To avoid scenarios where KVM has configured state for a set of features X, and
doesn't correctly handle vCPU features suddenly become Y.  Or more commonly,
where correctly handling such transitions (if there's even a "correct" option)
is a complete waste of time and complexity because no sane setup will ever add
and/or remove features from a running VM.

> > > 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.

More importantly, userspace is allowed to set the CPUID returned by KVM_GET_CPUID2.
E.g. selftests do KVM_GET_CPUID2 specifically to read the bits that are managed
by KVM.

Disallowing that would likely break userspace, and would create a weird ABI where
the output of KVM_GET_CPUID2 is rejected by KVM_SET_CPUID2.

> If they're supposed to be entirely unchanged, would it suffice just to
> keep a hash of them?

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

[Vitaly Kuznetsov]

Sean Christopherson <seanjc@google.com> writes:

> On Wed, Jan 22, 2025, David Woodhouse wrote:
>> On Wed, 2025-01-22 at 18:44 +0100, Vitaly Kuznetsov wrote:
>> > > What is the purpose of the comparison anyway?
>
> To avoid scenarios where KVM has configured state for a set of features X, and
> doesn't correctly handle vCPU features suddenly become Y.  Or more commonly,
> where correctly handling such transitions (if there's even a "correct" option)
> is a complete waste of time and complexity because no sane setup will ever add
> and/or remove features from a running VM.
>
>> > > 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.
>
> More importantly, userspace is allowed to set the CPUID returned by KVM_GET_CPUID2.
> E.g. selftests do KVM_GET_CPUID2 specifically to read the bits that are managed
> by KVM.
>
> Disallowing that would likely break userspace, and would create a weird ABI where
> the output of KVM_GET_CPUID2 is rejected by KVM_SET_CPUID2.
>
>> If they're supposed to be entirely unchanged, would it suffice just to
>> keep a hash of them?

In case we want to support both cases:
- VMM calls KVM_SET_CPUID2 at some point in vCPU's lifetime with the
same data it used initially;
- VMM does KVM_GET_CPUID2 and feeds this directly into KVM_SET_CPUID2
we can't use a hash as the later contains entries mangled by
KVM. Currently, we kind of support both but we expect the result of the
mangling done by KVM to always be the same.

I guess we can change the logic the following: when KVM_SET_CPUID2 is
called on a vCPU again we check that all entries which KVM did not touch
match. For that, we will need to keep a list of mangled entries so we
can introduce a kvm_mangle_cpuid_entry() helper to avoid the need to
keep a static list. Personally, I'm not sure this is not an overkill
though.

-- 
Vitaly

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

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 836 bytes --]

On Thu, 2025-01-23 at 13:35 +0100, Vitaly Kuznetsov wrote:
> 
> I guess we can change the logic the following: when KVM_SET_CPUID2 is
> called on a vCPU again we check that all entries which KVM did not touch
> match. For that, we will need to keep a list of mangled entries so we
> can introduce a kvm_mangle_cpuid_entry() helper to avoid the need to
> keep a static list. Personally, I'm not sure this is not an overkill
> though.

Putting that another way, KVM would exempt the dynamic entries that KVM
is going to overwrite by *itself*, from the comparison. By keeping a
list of the entries that it's going to overwrite.

I'm not sure I'd call that overkill. I think I prefer it to the option
of mangling the CPUID at *runtime* in kvm_cpuid() while the entries in
the array differ from what the guest actually sees.

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5069 bytes --]

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

[Sean Christopherson]

On Wed, Jan 22, 2025, Vitaly Kuznetsov wrote:
> > 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.

I don't think it does.  Linux-as-a-guest reads the info once during boot (see
xen_tsc_safe_clocksource()), and if and only if the TSC is constant and non-stop,
i.e. iff the values won't change.  

>  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?

Another alternative would be to modify the register output in kvm_cpuid().  Given
that Linux reads the info once during boot, and presumably other guests do the
same, runtime "patching" wouldn't incur meaningful overhead.  And there are no
feature bits that KVM cares about, i.e. no reason KVM's view needs to be correct.

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

[Vitaly Kuznetsov]

Sean Christopherson <seanjc@google.com> writes:

> On Wed, Jan 22, 2025, Vitaly Kuznetsov wrote:
>> > 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.
>
> I don't think it does.  Linux-as-a-guest reads the info once during boot (see
> xen_tsc_safe_clocksource()), and if and only if the TSC is constant and non-stop,
> i.e. iff the values won't change.  

Right, the values shouldn't change on the same host. What I was thinking
is what happens when we migrate the guest to another
host. kvm_guest_time_update() is going to be called and we will get
something different (maybe just slightly different, but still) in Xen
TSC CPUIDs. The guest, however, is likely not going to notice at all.

>
>>  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?
>
> Another alternative would be to modify the register output in kvm_cpuid().  Given
> that Linux reads the info once during boot, and presumably other guests do the
> same, runtime "patching" wouldn't incur meaningful overhead.  And there are no
> feature bits that KVM cares about, i.e. no reason KVM's view needs to be correct.

True, CPUID reading time should not be performance critical.

-- 
Vitaly

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

[Griffoul, Fred]

Thanks for your comments: I will post a new patch following Sean's idea to fix the CPUID registers directly in kvm_cpuid().

Br,

--
Fred

________________________________________
From: Vitaly Kuznetsov <vkuznets@redhat.com>
Sent: Thursday, January 23, 2025 1:24:12 PM
To: Sean Christopherson
Cc: Griffoul, Fred; kvm@vger.kernel.org; Paolo Bonzini; Thomas Gleixner; Ingo Molnar; Borislav Petkov; Dave Hansen; x86@kernel.org; H. Peter Anvin; David Woodhouse; Paul Durrant; linux-kernel@vger.kernel.org
Subject: RE: [EXTERNAL] [PATCH] KVM: x86: Update Xen-specific CPUID leaves during mangling


Sean Christopherson <seanjc@google.com> writes:

> On Wed, Jan 22, 2025, Vitaly Kuznetsov wrote:
>> > 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.
>
> I don't think it does.  Linux-as-a-guest reads the info once during boot (see
> xen_tsc_safe_clocksource()), and if and only if the TSC is constant and non-stop,
> i.e. iff the values won't change.

Right, the values shouldn't change on the same host. What I was thinking
is what happens when we migrate the guest to another
host. kvm_guest_time_update() is going to be called and we will get
something different (maybe just slightly different, but still) in Xen
TSC CPUIDs. The guest, however, is likely not going to notice at all.

>
>>  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?
>
> Another alternative would be to modify the register output in kvm_cpuid().  Given
> that Linux reads the info once during boot, and presumably other guests do the
> same, runtime "patching" wouldn't incur meaningful overhead.  And there are no
> feature bits that KVM cares about, i.e. no reason KVM's view needs to be correct.

True, CPUID reading time should not be performance critical.

--
Vitaly