Re: [PATCH v11 02/23] KVM: x86: Kill last_tsc_{nsec,write,offset} fields

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] KVM: x86: cur_tsc_nsec diverges from cur_tsc_offset after backwards TSC
--

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

KVM: x86: Kill last_tsc_{nsec,write,offset} fields

This commit removes the redundant last_tsc_{nsec,write,offset} fields from
struct kvm_arch and renames last_tsc_{khz,scaling_ratio} to
cur_tsc_{khz,scaling_ratio}. It also updates kvm->arch.cur_tsc_offset during
backwards-TSC compensation on resume and clears pending per-vCPU
tsc_offset_adjustment during TSC synchronization.

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 7a2b0b99373fe..90f9b07ef7491 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c

[ ... ]

> @@ -1395,7 +1400,7 @@ void kvm_synchronize_tsc(struct kvm_vcpu *vcpu, u64 *user_value)
>  	}
>  
>  	offset = kvm_compute_l1_tsc_offset(vcpu, host_tsc, data);
> -	elapsed = ns - kvm->arch.last_tsc_nsec;
> +	elapsed = ns - kvm->arch.cur_tsc_nsec;
>  
>  	if (vcpu->arch.virtual_tsc_khz) {
>  		if (data == 0) {
> @@ -1405,7 +1410,7 @@ void kvm_synchronize_tsc(struct kvm_vcpu *vcpu, u64 *user_value)
>  			 */
>  			synchronizing = true;
>  		} else if (kvm->arch.user_set_tsc) {
> -			u64 tsc_exp = kvm->arch.last_tsc_write +
> +			u64 tsc_exp = kvm->arch.cur_tsc_write +
>  						nsec_to_cycles(vcpu, elapsed);
>  			u64 tsc_hz = vcpu->arch.virtual_tsc_khz * 1000LL;
>  			/*
> @@ -1436,15 +1441,12 @@ void kvm_synchronize_tsc(struct kvm_vcpu *vcpu, u64 *user_value)
>  	 * it's better to try to match offsets from the beginning.
>           */
>  	if (synchronizing &&
> -	    vcpu->arch.virtual_tsc_khz == kvm->arch.last_tsc_khz) {
> +	    vcpu->arch.virtual_tsc_khz == kvm->arch.cur_tsc_khz) {
>  		/*
>  		 * If synchronizing, advance the reference point to "now"
>  		 * so the matching window slides forward with each vCPU.
> -		 * Extrapolate from the generation's own origin: 'elapsed'
> -		 * is measured from the most recent write, which lies on
> -		 * the generation's line only for the first matched write.
>  		 */
> -		u64 delta = nsec_to_cycles(vcpu, ns - kvm->arch.cur_tsc_nsec);
> +		u64 delta = nsec_to_cycles(vcpu, elapsed);
>  
>  		data = kvm->arch.cur_tsc_write + delta;

[ ... ]

> @@ -9874,6 +9876,24 @@ int kvm_arch_enable_virtualization_cpu(void)
>  	if (backwards_tsc) {
>  		u64 delta_cyc = max_tsc - local_tsc;
>  		list_for_each_entry(kvm, &vm_list, vm_list) {
> +			/*
> +			 * Hold tsc_write_lock across the per-vCPU adjustments
> +			 * AND the generation reference offset adjustment, so
> +			 * that a concurrent TSC synchronization cannot observe
> +			 * one without the other (it would compute an offset
> +			 * with the delta applied twice, or not at all).
> +			 *
> +			 * The generation's reference offset is adjusted by the
> +			 * same (guest-scaled, as adjust_tsc_offset_host()
> +			 * does for each vCPU) delta, so that future
> +			 * KVM_SET_TSC / vCPU creation still matches against
> +			 * the adjusted TSC timeline. The recorded
> +			 * {cur_tsc_nsec, cur_tsc_write} pair needs no
> +			 * adjustment: the guest TSC is kept continuous
> +			 * across the host TSC jump, and that pair lives in
> +			 * the wall-clock/guest-cycle domain.
> +			 */
> +			raw_spin_lock_irqsave(&kvm->arch.tsc_write_lock, flags);
>  			kvm->arch.backwards_tsc_observed = true;
>  			kvm_for_each_vcpu(i, vcpu, kvm) {
>  				vcpu->arch.tsc_offset_adjustment += delta_cyc;
> @@ -9881,14 +9901,10 @@ int kvm_arch_enable_virtualization_cpu(void)
>  				kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu);
>  			}
>  
> -			/*
> -			 * We have to disable TSC offset matching.. if you were
> -			 * booting a VM while issuing an S4 host suspend....
> -			 * you may have some problem.  Solving this issue is
> -			 * left as an exercise to the reader.
> -			 */
> -			kvm->arch.last_tsc_nsec = 0;
> -			kvm->arch.last_tsc_write = 0;
> +			kvm->arch.cur_tsc_offset +=
> +				kvm_scale_tsc(delta_cyc,
> +					      kvm->arch.cur_tsc_scaling_ratio);
> +			raw_spin_unlock_irqrestore(&kvm->arch.tsc_write_lock, flags);

[Severity: Medium]
Does leaving kvm->arch.cur_tsc_nsec and kvm->arch.cur_tsc_write unadjusted
here cause them to diverge from kvm->arch.cur_tsc_offset by the host sleep
duration after a suspend or hibernate?

In kvm_arch_enable_virtualization_cpu(), delta_cyc = max_tsc - local_tsc only
compensates for the host TSC resetting from max_tsc (recorded at the last
vcpu_put before suspend) to local_tsc. As the earlier comment in
kvm_arch_enable_virtualization_cpu() explains, it does not advance the guest
TSCs by the real time elapsed while the host was suspended. Adjusting
kvm->arch.cur_tsc_offset by the scaled delta_cyc therefore keeps the guest
TSC timeline continuous with its pre-suspend value, excluding the sleep
duration.

However, kvm_synchronize_tsc() obtains ns via kvm_get_time_and_clockread() or
get_kvmclock_base_ns():

arch/x86/kvm/x86.c:get_kvmclock_base_ns() {
	return ktime_to_ns(ktime_mono_to_any(ktime_get_raw(), TK_OFFS_BOOT));
}

During resume, timekeeping injects the suspend/hibernate sleep time into
TK_OFFS_BOOT. As a result, elapsed = ns - kvm->arch.cur_tsc_nsec in
kvm_synchronize_tsc() includes the entire host sleep duration, whereas
kvm->arch.cur_tsc_offset and the running guest TSCs do not.

After a suspend or hibernate longer than 1 second where backwards_tsc is
triggered, won't tsc_exp = kvm->arch.cur_tsc_write + nsec_to_cycles(vcpu,
elapsed) be ahead of the guest TSC timeline by the sleep duration, causing
kvm_synchronize_tsc() to mismatch userspace writes that match the post-resume
guest TSC (or falsely match writes that include the sleep duration and snap
their offset back to the unadvanced kvm->arch.cur_tsc_offset), and causing
synchronizing vCPUs (such as data == 0 on vCPU creation) to record
vcpu->arch.last_guest_tsc ahead of the actual guest TSC by the sleep
duration?

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