Re: [PATCH v7 11/36] KVM: x86: Restructure kvm_guest_time_update() for TSC upscaling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Uninitialized kernel_ns and host_tsc variables are used on 32-bit builds if use_master_clock is true.
- [High] The fallback clock path reads rdtsc() after preemption is re-enabled, causing mismatched TSC frequency calculations and guest clock corruption.
--

--- Patch [11]: [PATCH v7 11/36] KVM: x86: Restructure kvm_guest_time_update() for TSC upscaling ---
commit a9ced800a4cffa309e689174fd612acf127799a3
Author: David Woodhouse <dwmw@amazon.co.uk>

KVM: x86: Restructure kvm_guest_time_update() for TSC upscaling

This commit restructures kvm_guest_time_update to simplify the TSC catchup
adjustment by ensuring kernel_ns and host_tsc are always now during catchup,
then swapping in the master clock reference values afterward.

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 27f2f02bd5f40..faeb2d7b19a48 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1776,45 +1776,60 @@ static void kvm_setup_guest_pvclock(struct pvclock_vcpu_time_info *ref_hv_clock,
>  int kvm_guest_time_update(struct kvm_vcpu *v)
>  {
[ ... ]
> -	kernel_ns = 0;
> -	host_tsc = 0;
> -
>  	/*
>  	 * If the host uses TSC clock, then passthrough TSC as stable
>  	 * to the guest.
>  	 */
>  	do {
>  		seq = read_seqcount_begin(&ka->pvclock_sc);
> +
>  		use_master_clock = ka->use_master_clock;
> +
> +		/*
> +		 * The TSC read and the call to get_cpu_tsc_khz() must happen
> +		 * on the same CPU.
> +		 */
> +		get_cpu();
> +
> +		tgt_tsc_hz = (u64)get_cpu_tsc_khz() * HZ_PER_KHZ;
> +
> +#ifdef CONFIG_X86_64
> +		if (use_master_clock &&
> +		    !kvm_get_time_and_clockread(&kernel_ns, &host_tsc) &&
> +		    !read_seqcount_retry(&ka->pvclock_sc, seq))
> +			use_master_clock = false;
> +#endif
> +
> +		put_cpu();
> +
> +		if (use_master_clock) {
> -			host_tsc = ka->master_cycle_now;
> -			kernel_ns = ka->master_kernel_ns;
> +			master_host_tsc = ka->master_cycle_now;
> +			master_kernel_ns = ka->master_kernel_ns;
> +		} else {
> +			local_irq_disable();
> +			host_tsc = rdtsc();

[Severity: High]
Because put_cpu() re-enables preemption, could the vCPU thread be preempted
and migrated to another physical CPU before local_irq_disable() and rdtsc()
are called in this fallback path?

If migration occurs here, host_tsc might be read on a different CPU than
where tgt_tsc_hz was calculated.

> +			kernel_ns = get_kvmclock_base_ns();
> +			local_irq_enable();
>  		}

[ ... ]

> @@ -1824,17 +1839,32 @@ int kvm_guest_time_update(struct kvm_vcpu *v)
>  	 *      entry to avoid unknown leaps of TSC even when running
>  	 *      again on the same CPU.  This may cause apparent elapsed
>  	 *      time to disappear, and the guest to stand still or run
> -	 *	very slowly.
> +	 *      very slowly.
>  	 */
>  	if (vcpu->tsc_catchup) {
> -		u64 tsc = compute_guest_tsc(v, kernel_ns);
> -		if (tsc > tsc_timestamp) {
> -			adjust_tsc_offset_guest(v, tsc - tsc_timestamp);
> -			tsc_timestamp = tsc;
> -		}
> +		s64 adjustment;
> +
> +		/*
> +		 * Calculate the delta between what the guest TSC *should* be
> +		 * and what it actually is according to kvm_read_l1_tsc().
> +		 */
> +		adjustment = compute_guest_tsc(v, kernel_ns) -
> +			     kvm_read_l1_tsc(v, host_tsc);

[Severity: Critical]
On 32-bit builds, will kernel_ns and host_tsc remain uninitialized when
use_master_clock is true?

The original 0-initializations for kernel_ns and host_tsc were removed by
this patch. Their new initialization via kvm_get_time_and_clockread() is
guarded by CONFIG_X86_64.

When vcpu->tsc_catchup is set on 32-bit builds, it seems this would pass
uninitialized stack variables to compute_guest_tsc() and kvm_read_l1_tsc()
for the adjustment calculation.

> +		if (adjustment > 0)
> +			adjust_tsc_offset_guest(v, adjustment);
>  	}

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