Re: [PATCH v7 14/36] KVM: x86: Improve synchronization in kvm_synchronize_tsc()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Incorrect TSC calculation during synchronization mixes different time bases (generation base vs. relative elapsed time), causing the guest TSC to drift backwards for 3rd and subsequent vCPUs.
- [Medium] The commit message claims to reset {data, ns} to previous values, but the code actively advances the reference point instead.
--

--- Patch [14]: [PATCH v7 14/36] KVM: x86: Improve synchronization in kvm_synchronize_tsc() ---
commit 19537539a4f15d4647e8e8b2f7465c1412098ea0
Author: David Woodhouse <dwmw@amazon.co.uk>

KVM: x86: Improve synchronization in kvm_synchronize_tsc()

This commit addresses issues with how last_tsc_write and last_tsc_nsec were
recorded, which caused subsequent writes to fail synchronization. It also
unifies the TSC reads used for calculating current time by using
kvm_get_time_and_clockread() where possible.

> Fix that by resetting {data, ns} to the previous values before passing
> them to __kvm_synchronize_tsc() when synchronization is detected.
> Except in the case where the TSC is unstable and *has* to be synthesised
> from the host clock, in which case attempt to create a nsec/tsc pair
> which is on the correct line.

[Severity: Medium]
The commit message states that it fixes the issue by resetting data and ns
to the previous values, but the code appears to actively advance the
reference point instead.

Could the commit message be updated to more accurately reflect the implemented
logic?

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 083ab2ca6282f..a1265d1f5c3fe 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c

[ ... ]

> @@ -1347,13 +1360,18 @@ void kvm_synchronize_tsc(struct kvm_vcpu *vcpu, u64 *user_value)
>           */
>  	if (synchronizing &&
>  	    vcpu->arch.virtual_tsc_khz == kvm->arch.last_tsc_khz) {
> -		if (!kvm_check_tsc_unstable()) {
> +		/*
> +		 * If synchronizing, advance the reference point to "now"
> +		 * so the matching window slides forward with each vCPU.
> +		 */
> +		u64 delta = nsec_to_cycles(vcpu, elapsed);
> +
> +		data = kvm->arch.cur_tsc_write + delta;

[Severity: High]
Does this calculate an incorrect TSC value by mixing different time bases?

It appears that elapsed is computed as the time since the previous vCPU,
but delta is added to kvm->arch.cur_tsc_write, which is the absolute base
TSC of the first vCPU in the generation.

If a VM is created with 3 or more vCPUs, wouldn't the time elapsed between
the 1st and 2nd vCPUs be lost when calculating for the 3rd vCPU? This seems
like it could cause the guest TSC to drift backwards across vCPUs.

> +
> +		if (!kvm_check_tsc_unstable())
>  			offset = kvm->arch.cur_tsc_offset;
> -		} else {
> -			u64 delta = nsec_to_cycles(vcpu, elapsed);
> -			data += delta;
> -			offset = kvm_compute_l1_tsc_offset(vcpu, rdtsc(), data);
> -		}
> +		else
> +			offset = kvm_compute_l1_tsc_offset(vcpu, host_tsc, data);
>  		matched = true;
>  	}

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