Re: [PATCH v12 08/24] KVM: x86: Re-synchronize TSC after KVM_SET_TSC_KHZ

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [Medium] KVM: x86: unreachable 1ms sync window in
  kvm_synchronize_tsc_at()
- [Medium] KVM: x86: unsigned elapsed underflow in
  kvm_synchronize_tsc_at()
- [Medium] KVM: x86: kvm_set_tsc_khz() sync breaks per-vCPU init and
  restore
- [Medium] KVM: x86: TSC sync in kvm_set_tsc_khz() skews across vCPUs by
  (F1 - F2) * dt
--

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

KVM: x86: Re-synchronize TSC after KVM_SET_TSC_KHZ

This commit re-synchronizes the vCPU's TSC in kvm_set_tsc_khz() across
KVM_SET_TSC_KHZ frequency changes, preserving the current guest TSC value
when the vCPU has already run or had its TSC written by userspace.

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 290a3c3bb44fc..b47c36265c2f2 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
[ ... ]
> @@ -1091,7 +1110,67 @@ static int kvm_set_tsc_khz(struct kvm_vcpu *vcpu, u32 user_tsc_khz)
[ ... ]
> +	if (init || !freq_changed)
> +		return 0;
> +
> +	/*
> +	 * Changing the frequency changes the vCPU's position on the
> +	 * generation's cycle timeline, so re-synchronize. For a vCPU
> +	 * which has run, preserve the guest TSC value read above across
> +	 * the change; likewise if userspace has written a TSC on this
> +	 * vCPU — a zero write would discard it. Otherwise the vCPU holds
> +	 * only its creation-time synchronization, so synchronize as for
> +	 * a zero write. When the generation is already at the new
> +	 * frequency this places the vCPU on its timeline rather than
> +	 * opening a gratuitous new generation and orphaning the matched
> +	 * state of its siblings; the first vCPU to adopt a new frequency
> +	 * necessarily opens a new generation, starting at zero — mixed
> +	 * frequencies within a VM forfeit the master clock anyway.
> +	 */
> +	if (vcpu->arch.last_vmentry_cpu != -1 || vcpu->arch.user_set_tsc)
> +		kvm_synchronize_tsc_at(vcpu, &tsc, host_tsc, ns, false);
> +	else
> +		kvm_synchronize_tsc_at(vcpu, NULL, host_tsc, ns, false);
> +	return 0;
>  }

[Severity: Medium]
Does re-synchronizing in kvm_set_tsc_khz() break frequency and TSC
matching when userspace creates and configures vCPUs one at a time?

When a VMM runs KVM_CREATE_VCPU followed by KVM_SET_TSC_KHZ(F_new) for
vCPU 0, kvm->arch.cur_tsc_khz becomes F_new.  When vCPU 1 is then created,
kvm_arch_vcpu_postcreate() synchronizes it at default_tsc_khz, which sees
virtual_tsc_khz != cur_tsc_khz, bumps cur_tsc_freq_generation and
cur_tsc_generation, resets nr_vcpus_matched_freq and nr_vcpus_matched_tsc,
and sets cur_tsc_khz back to default_tsc_khz.  When KVM_SET_TSC_KHZ(F_new)
then runs on vCPU 1, cur_tsc_khz is no longer F_new, so vCPU 1 opens
another new generation and orphans vCPU 0; the same ping-pong happens when
restoring vCPUs one by one in MSR_IA32_TSC-then-KVM_SET_TSC_KHZ order.
After all vCPUs are configured to F_new, all_vcpus_matched_freq and
all_vcpus_matched_tsc remain false (disabling masterclock and
PVCLOCK_TSC_STABLE_BIT), and repeating KVM_SET_TSC_KHZ on earlier vCPUs
cannot recover because !freq_changed returns early.

[ ... ]

> @@ -1506,6 +1586,7 @@ void kvm_synchronize_tsc(struct kvm_vcpu *vcpu, u64 *user_value)
>  			u64 tsc_exp = kvm->arch.cur_tsc_write +
>  						nsec_to_cycles(vcpu, elapsed);
>  			u64 tsc_hz = vcpu->arch.virtual_tsc_khz * 1000LL;
> +			u64 window;
>  			/*
>  			 * Here lies UAPI baggage: when a user-initiated TSC write has
>  			 * a small delta (1 second) of virtual cycle time against the
> @@ -1520,9 +1601,18 @@ void kvm_synchronize_tsc(struct kvm_vcpu *vcpu, u64 *user_value)
>  			 * to this 'correction' to make it sync up with values that only
>  			 * come from the kernel's default vCPU creation. Make the 1-second
>  			 * slop hack only trigger if the user_set_tsc flag is already set.
> +			 *
> +			 * A kernel-internal write (a frequency change preserving the
> +			 * guest TSC) carries an exact value sampled with its own
> +			 * reference point, not a racy userspace one; a vCPU on the
> +			 * generation's line still matches it to within arithmetic
> +			 * rounding, but one which genuinely diverged (e.g. by a guest
> +			 * TSC_ADJUST write) must not be snapped back by up to a
> +			 * second. Use a 1ms window for those.
>  			 */
> -			synchronizing = data < tsc_exp + tsc_hz &&
> -					data + tsc_hz > tsc_exp;
> +			window = user_initiated ? tsc_hz : tsc_hz / 1000;
> +			synchronizing = data < tsc_exp + window &&
> +					data + window > tsc_exp;
>  		}
>  	}

[Severity: Medium]
Is the 1ms window for !user_initiated unreachable when userspace has never
written the TSC?

The window check sits inside:

arch/x86/kvm/x86.c:kvm_synchronize_tsc_at() {
	if (vcpu->arch.virtual_tsc_khz) {
		if (data == 0) {
			synchronizing = true;
		} else if (kvm->arch.user_set_tsc) {
			...
			window = user_initiated ? tsc_hz : tsc_hz / 1000;

When KVM_SET_TSC_KHZ is called across vCPUs that have already run
(last_vmentry_cpu != -1) without userspace ever writing MSR_IA32_TSC or
KVM_VCPU_TSC_OFFSET, kvm_set_tsc_khz() passes a non-zero tsc and
user_initiated = false.  Because __kvm_synchronize_tsc() only sets
kvm->arch.user_set_tsc when user_initiated is true, kvm->arch.user_set_tsc
stays false and every subsequent vCPU skips the else-if branch, fails to
match the generation, and clears all_vcpus_matched_tsc.  Should the
condition be "else if (kvm->arch.user_set_tsc || !user_initiated)"?

[Severity: Medium]
Even when that branch is reached, doesn't comparing data against tsc_exp
within a 1ms window fail whenever the delay dt between vCPU 0's and a
sibling vCPU's KVM_SET_TSC_KHZ ioctls exceeds 1ms * F2 / |F1 - F2|?

When changing multiple vCPUs from frequency F1 to F2 while preserving
their TSCs, vCPU 0 anchors cur_tsc_nsec and cur_tsc_write at t0.  Over the
elapsed time dt before a sibling vCPU's KVM_SET_TSC_KHZ ioctl runs, that
sibling is still scaled at F1, so its sampled data = kvm_read_l1_tsc()
has advanced by F1 * dt cycles.  However, kvm_set_tsc_khz() updates
virtual_tsc_mult and virtual_tsc_shift to F2 before calling
kvm_synchronize_tsc_at(), so nsec_to_cycles(vcpu, elapsed) advances
tsc_exp by F2 * dt cycles.  The difference |data - tsc_exp| is therefore
|F1 - F2| * dt rather than arithmetic rounding, which exceeds the 1ms
window (tsc_hz / 1000) once dt >= 1ms * F2 / |F1 - F2| (for example 1ms
when halving the frequency), causing sibling vCPUs to miss the generation
and lose all_vcpus_matched_tsc.

> @@ -1552,10 +1642,26 @@ void kvm_synchronize_tsc(struct kvm_vcpu *vcpu, u64 *user_value)
>  		matched = true;
>  	}
>  
> -	__kvm_synchronize_tsc(vcpu, offset, data, ns, matched, !!user_value);
> +	__kvm_synchronize_tsc(vcpu, offset, data, ns, matched, user_initiated);
>  	raw_spin_unlock_irqrestore(&kvm->arch.tsc_write_lock, flags);
>  }
>  
> +void kvm_synchronize_tsc(struct kvm_vcpu *vcpu, u64 *value, bool user_initiated)
> +{
> +	u64 host_tsc;
> +	s64 ns;
> +
> +#ifdef CONFIG_X86_64
> +	if (!kvm_get_time_and_clockread(&ns, &host_tsc))
> +#endif
> +	{
> +		host_tsc = rdtsc();
> +		ns = get_kvmclock_base_ns();
> +	}
> +
> +	kvm_synchronize_tsc_at(vcpu, value, host_tsc, ns, user_initiated);
> +}

[Severity: Medium]
Can sampling {ns, host_tsc} outside kvm->arch.tsc_write_lock in
kvm_set_tsc_khz() and kvm_synchronize_tsc() underflow elapsed in
kvm_synchronize_tsc_at()?

CPU0                              CPU1
kvm_set_tsc_khz(vcpu0)
  sample ns = 100
  <preempted>
                                  kvm_set_tsc_khz(vcpu1)
                                    sample ns = 200
                                    kvm_synchronize_tsc_at()
                                      lock(&kvm->arch.tsc_write_lock)
                                      kvm->arch.cur_tsc_nsec = 200
                                      unlock(&kvm->arch.tsc_write_lock)
  kvm_synchronize_tsc_at()
    lock(&kvm->arch.tsc_write_lock)
    elapsed = ns - cur_tsc_nsec   <-- wraps u64
    nsec_to_cycles(vcpu, elapsed)

In kvm_synchronize_tsc_at(), elapsed is a u64:

arch/x86/kvm/x86.c:kvm_synchronize_tsc_at() {
	offset = kvm_compute_l1_tsc_offset(vcpu, host_tsc, data);
	elapsed = ns - kvm->arch.cur_tsc_nsec;

When CPU0 acquires tsc_write_lock after CPU1 has already recorded a newer
cur_tsc_nsec, elapsed wraps around near U64_MAX and is passed to unsigned
nsec_to_cycles() (pvclock_scale_delta()).  For non-zero writes this
corrupts tsc_exp so CPU0 fails to match and overwrites cur_tsc_nsec with
the older timestamp; for zero-value syncs (synchronizing == true) it
corrupts delta, data, vcpu->arch.last_guest_tsc, and (when
kvm_check_tsc_unstable() is true) offset.  In addition, the fallback
rdtsc() + get_kvmclock_base_ns() path now runs with preemption and
interrupts enabled, so preemption between the two reads can skew the
{host_tsc, ns} pair.

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