Re: [PATCH v5 18/34] KVM: x86: Improve synchronization in kvm_synchronize_tsc()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The patch breaks the 1-second TSC matching interval extension, causing severe TSC synchronization failures when restoring large VMs.
--

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

x86/kvm: Fix misrecorded TSC synchronization values

This commit modifies kvm_synchronize_tsc() to reset the written data and
timestamp to previous values when a synchronization match is detected. It
also unifies the TSC reads used for calculating the current time.

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index bce4c7a6a6fe..c8c0633263fb 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c

[ ... ]

> @@ -2904,12 +2916,25 @@ static void kvm_synchronize_tsc(struct kvm_vcpu *vcpu, u64 *user_value)
>           */
>  	if (synchronizing &&
>  	    vcpu->arch.virtual_tsc_khz == kvm->arch.last_tsc_khz) {
> +		/*
> +		 * If synchronizing, the "last written" TSC value/time
> +		 * recorded by __kvm_synchronize_tsc() should not change
> +		 * (i.e. should be precisely the same as the existing
> +		 * generation).
> +		 */
> +		data = kvm->arch.last_tsc_write;
> +
>  		if (!kvm_check_tsc_unstable()) {
>  			offset = kvm->arch.cur_tsc_offset;
> +			ns = kvm->arch.cur_tsc_nsec;

[Severity: High]
Does forcing ns and data back to the start of the generation break the
1-second TSC matching interval extension for large VMs?

During VM restore, userspace sequentially writes the exact same saved TSC
value to all vCPUs. KVM's legacy behavior updated the tracking anchor to the
time and value of each matched write, creating a sliding window that allowed
the restoration process to exceed 1 second in total, as long as each
individual vCPU was restored within 1 second of the previous one.

By freezing the anchor to the start of the generation (T0) here, this sliding
window appears to be lost. If a large VM takes more than 1 second to restore,
a vCPU restored at T_N (where T_N - T0 > 1s) will compute an elapsed time
greater than 1s.

Since userspace writes the same static saved TSC value, the expected TSC
will be more than 1s ahead of the written value. Would this cause the
synchronization check to fail, creating a new TSC generation with a
misaligned offset compared to the rest of the guest?

>  		} else {
> +			/*
> +			 * ...unless the TSC is unstable and has to be
> +			 * synthesised from the host clock in nanoseconds.
> +			 */
>  			u64 delta = nsec_to_cycles(vcpu, elapsed);
>  			data += delta;
> -			offset = kvm_compute_l1_tsc_offset(vcpu, rdtsc(), data);
> +			offset = kvm_compute_l1_tsc_offset(vcpu, host_tsc, data);
>  		}
>  		matched = true;
>  	}

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