Re: [PATCH v7 22/36] KVM: x86: Remove runtime Xen TSC frequency CPUID update

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Unconditional removal of runtime Xen TSC CPUID updates breaks existing userspace (e.g., older QEMU versions) that relies on KVM to populate these fields.
- [High] The commit relies on non-existent IOCTLs (`KVM_GET_CLOCK_GUEST` and `KVM_VCPU_TSC_SCALE`) for userspace to populate the Xen TSC CPUID leaves.
--

--- Patch [22]: [PATCH v7 22/36] KVM: x86: Remove runtime Xen TSC frequency CPUID update ---
commit 127a72b774b2e5b2be60192682a2508bb1a3e37c
Author: David Woodhouse <dwmw@amazon.co.uk>

KVM: x86: Remove runtime Xen TSC frequency CPUID update

This commit removes the dynamic updating of the Xen TSC info CPUID leaf in
KVM, assuming that userspace can populate these fields using specific IOCTLs
at vCPU setup time. It also removes the associated helper functions in xen.h.

>   - KVM_GET_CLOCK_GUEST returns the pvclock_vcpu_time_info structure
>     containing tsc_to_system_mul and tsc_shift (Xen leaf index 1)
>   - KVM_VCPU_TSC_SCALE returns the effective TSC and bus
>     frequencies in kHz (Xen leaf index 2, and 0x40000010)
>   - KVM_VCPU_TSC_SCALE returns the raw hardware scaling ratio for
>     precise arithmetic (VMClock)

[Severity: High]
Are KVM_GET_CLOCK_GUEST and KVM_VCPU_TSC_SCALE actually present in the tree?

I cannot find these IOCTLs, and it doesn't appear that internal variables like
vcpu->arch.pvclock_tsc_mul are exported to userspace by any other mechanism.
Could the commit message be updated, or are there missing patches that
introduce these interfaces?

> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 8eabd68fa0a15..cd495bedf6c8e 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c

[ ... ]

> @@ -2119,22 +2118,6 @@ bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx,
>  		} else if (function == 0x80000007) {
>  			if (kvm_hv_invtsc_suppressed(vcpu))
>  				*edx &= ~feature_bit(CONSTANT_TSC);
> -		} else if (IS_ENABLED(CONFIG_KVM_XEN) &&
> -			   kvm_xen_is_tsc_leaf(vcpu, function)) {
> -			/*
> -			 * Update guest TSC frequency information if necessary.
> -			 * Ignore failures, there is no sane value that can be
> -			 * provided if KVM can't get the TSC frequency.
> -			 */

[Severity: Critical]
Does unconditionally removing this dynamic Xen TSC CPUID update break older
userspace binaries (like older QEMU versions) that expect KVM to handle
populating these leaves?

Since older VMMs do not populate these leaves themselves, removing this could
result in Xen guests receiving zeroes or uninitialized data for TSC info,
leading to incorrect timekeeping or crashes.

Should this ABI change be gated behind a new capability (e.g.,
KVM_CAP_XEN_TSC_CPUID) so that legacy VMMs continue functioning while new VMMs
can opt-in to the new behavior?

> -			if (kvm_check_request(KVM_REQ_CLOCK_UPDATE, vcpu))
> -				kvm_guest_time_update(vcpu);
> -
> -			if (index == 1) {
> -				*ecx = vcpu->arch.pvclock_tsc_mul;
> -				*edx = vcpu->arch.pvclock_tsc_shift;
> -			} else if (index == 2) {
> -				*eax = div_u64(vcpu->arch.hw_tsc_hz, HZ_PER_KHZ);
> -			}
>  		}
>  	} else {
>  		*eax = *ebx = *ecx = *edx = 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728144954.355376-1-dwmw2@infradead.org?part=22