Re: [PATCH v5 29/34] KVM: x86: Re-synchronize TSC after KVM_SET_TSC_KHZ

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Calling `kvm_read_l1_tsc` after `set_tsc_khz` calculates the current TSC using the newly set scaling ratio but the old offset, causing a massive discontinuity in the guest's observed TSC instead of preserving continuity.
--

commit c224c306d9d055ced6e04bed3638e47c8d31fb69
Author: David Woodhouse <dwmw@amazon.co.uk>

KVM_SET_TSC_KHZ changes the vCPU's TSC scaling ratio but does not update

This commit fixes an issue where get_kvmclock() uses a stale (1:1) ratio
when computing the KVM clock. It calls kvm_synchronize_tsc() after
changing the TSC frequency to ensure cur_tsc_scaling_ratio is updated
and the TSC value remains continuous across the frequency change.

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 54d4b1b3cfe4..96250264d403 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c

[ ... ]

> @@ -2611,7 +2612,20 @@ static int kvm_set_tsc_khz(struct kvm_vcpu *vcpu, u32 user_tsc_khz)
>  			 user_tsc_khz, thresh_lo, thresh_hi);
>  		use_scaling = 1;
>  	}
> -	return set_tsc_khz(vcpu, user_tsc_khz, use_scaling);
> +	if (set_tsc_khz(vcpu, user_tsc_khz, use_scaling))
> +		return -1;
> +
> +	/*
> +	 * Re-synchronize the TSC after changing frequency. This ensures
> +	 * cur_tsc_scaling_ratio is updated (used by get_kvmclock) and
> +	 * the TSC value is continuous across the frequency change.
> +	 */
> +	{
> +		u64 tsc = kvm_read_l1_tsc(vcpu, rdtsc());
> +
> +		kvm_synchronize_tsc(vcpu, &tsc);

[Severity: High]
Will this cause a discontinuity in the guest's observed TSC instead of
preserving continuity?

If kvm_read_l1_tsc() is called after set_tsc_khz() has already updated the
scaling ratio, the current guest TSC is calculated using the newly set
scaling ratio combined with the old offset:

tsc = old_offset + rdtsc() * new_ratio

When kvm_synchronize_tsc() is then called to compute the new offset:

new_offset = tsc - rdtsc() * new_ratio

This algebraically simplifies back to the old offset. Because the offset
remains essentially unchanged while the scaling ratio has changed, the
guest's observed TSC would immediately jump by the difference between the
old and new ratios multiplied by rdtsc().

Since rdtsc() represents the host's uptime in cycles, could this cause a
massive time jump that breaks guest timekeeping or leads to lockups?

Would it be safer to evaluate kvm_read_l1_tsc(vcpu, rdtsc()) before
calling set_tsc_khz(), so the old scaling ratio is used to establish the
baseline for continuity?

> +	}
> +	return 0;
>  }

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