Re: [PATCH v3 41/41] x86/kvmclock: Get CPU base frequency from CPUID when it's available

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Fallback path in `kvm_get_cpu_khz()` bypasses `kvm_para_tsc_khz()`, causing a precision regression when CPUID.0x16 is missing.
- [Medium] Core x86 timekeeping logic unconditionally overwrites `cpu_khz` if it diverges from `tsc_khz` by >10%, nullifying the commit's intent.
--

commit 244283a64d7a3809c004f242b3973386bb521665
Author: Sean Christopherson <seanjc@google.com>

    x86/kvmclock: Get CPU base frequency from CPUID when it's available

This commit updates kvmclock initialization to prefer fetching the base CPU
frequency from CPUID.0x16 instead of assuming the virtual CPU runs at the
same frequency as the TSC. It introduces kvm_get_cpu_khz() which falls back
to the PV clock TSC calculation if CPUID is unavailable.

> diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
> index 62c8ea2e67691..7607920ae386e 100644
> --- a/arch/x86/kernel/kvmclock.c
> +++ b/arch/x86/kernel/kvmclock.c
> @@ -190,6 +190,20 @@ void kvmclock_cpu_action(enum kvm_guest_cpu_action action)
>  	}
>  }
>  
> +static unsigned long kvm_get_cpu_khz(void)
> +{
> +	unsigned int cpu_khz;
> +
> +	/*
> +	 * Prefer CPUID over kvmclock when possible, as the base CPU frequency
> +	 * isn't necessarily the same as the kvmlock "TSC" frequency.
> +	 */
> +	if (!cpuid_get_cpu_freq(&cpu_khz))
> +		return cpu_khz;
> +
> +	return pvclock_tsc_khz(this_cpu_pvti());

Before this patch, both calibrate_cpu and calibrate_tsc used
kvm_get_tsc_khz(), which checks kvm_para_tsc_khz() to get the exact TSC
frequency from KVM's PV CPUID timing leaf, and only falls back to
pvclock_tsc_khz() if it is unavailable.

Does the fallback path in kvm_get_cpu_khz() bypass kvm_para_tsc_khz()
entirely? For VMs that expose KVM's PV CPUID timing leaf but not
CPUID.0x16, it looks like cpu_khz will now be reverse-calculated from
pvclock multipliers instead of using the exact hypervisor-provided value.

Could this introduce a precision regression due to truncation loss, causing
cpu_khz and tsc_khz to needlessly diverge on the fallback path?

> +}
> +
>  /*
>   * If we don't do that, there is the possibility that the guest
>   * will calibrate under heavy load - thus, getting a lower lpj -

[ ... ]

> @@ -434,7 +448,7 @@ void __init kvmclock_init(void)
>  		kvm_sched_clock_init(stable);
>  	}
>  
> -	tsc_register_calibration_routines(kvm_get_tsc_khz, kvm_get_tsc_khz,
> +	tsc_register_calibration_routines(kvm_get_tsc_khz, kvm_get_cpu_khz,
>  					  tsc_properties);

The commit message notes that treating the TSC and CPU frequencies as one
and the same is nonsensical when the hypervisor explicitly enumerates the
CPU frequency.

However, looking at the core x86 initialization in
determine_cpu_tsc_frequencies(), it unconditionally overwrites cpu_khz if it
diverges from tsc_khz by more than 10%:

arch/x86/kernel/tsc.c:determine_cpu_tsc_frequencies() {
	...
	if (tsc_khz == 0)
		tsc_khz = cpu_khz;
	else if (abs(cpu_khz - tsc_khz) * 10 > tsc_khz)
		cpu_khz = tsc_khz;
	...
}

If the hypervisor explicitly enumerates a CPU base frequency via CPUID.0x16
that genuinely differs from the TSC frequency by more than 10%, will this
core logic silently discard the explicitly enumerated CPU frequency and
overwrite it with the TSC frequency? 

Does this check thwart the patch's goal of decoupling the two values
exactly in the scenarios where they substantially differ?

>  
>  	x86_platform.get_wallclock = kvm_get_wallclock;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515191942.1892718-1-seanjc@google.com?part=41