Re: [PATCH v12 11/24] KVM: x86: Activate master clock from kvm_arch_init_vm()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] KVM: x86: host-guest clock divergence in
  __get_kvmclock_master_clock()
- [Low] KVM: x86: dead initial TSC generation stores in
  kvm_arch_init_vm()
--

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

KVM: x86: Activate master clock from kvm_arch_init_vm()

This commit initializes the master clock snapshot in kvm_arch_init_vm(),
adds a sanity check against raw clock divergence in
__get_kvmclock_master_clock(), and refreshes the master clock when the
first vCPU is created.

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index a357efbf1d941..4afafe7ed8b8a 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c

[ ... ]

> @@ -1927,13 +1932,34 @@ static bool __get_kvmclock_master_clock(struct kvm *kvm,
>  		u64 tsc_delta = data->host_tsc - ka->master_cycle_now;
>  
>  		tsc_delta = kvm_scale_tsc(tsc_delta, ka->master_tsc_scaling_ratio);
> -		data->clock = hv_clock.system_time +
> -			      pvclock_scale_delta(tsc_delta,
> -						  hv_clock.tsc_to_system_mul,
> -						  hv_clock.tsc_shift);
> +		clock = hv_clock.system_time +
> +			pvclock_scale_delta(tsc_delta,
> +					    hv_clock.tsc_to_system_mul,
> +					    hv_clock.tsc_shift);
>  	} else {
> -		data->clock = __pvclock_read_cycles(&hv_clock, data->host_tsc);
> +		clock = __pvclock_read_cycles(&hv_clock, data->host_tsc);
>  	}
> +
> +	/*
> +	 * Sanity-check the snapshot: if the elapsed time it computes from
> +	 * the host TSC disagrees with the raw clock by more than a second,
> +	 * the TSC was reset (suspend/resume — possibly re-inited by
> +	 * firmware to an arbitrary value, including an underflowing one
> +	 * below master_cycle_now) while nothing refreshed the snapshot.
> +	 * That can happen while the VM has no vCPUs, since both the
> +	 * resume compensation and the clocksource notifier work through
> +	 * vCPUs. Use the fallback clock rather than returning a
> +	 * discontinuity flagged KVM_CLOCK_TSC_STABLE.
> +	 */
> +	if (unlikely(abs((s64)(clock - hv_clock.system_time) -
> +			 (get_kvmclock_base_ns() - ka->master_kernel_ns)) >
> +		     NSEC_PER_SEC))
> +		return false;

[Severity: Medium]
Can this sanity check trigger while vCPUs are active and cause a 1-second
divergence between host-side get_kvmclock() and guest pvclock?

Although the comment describes the no-vCPU window, this check runs even
when vCPUs exist and ka->use_master_clock remains true.  While the master
clock is active, ka->master_cycle_now and ka->master_kernel_ns are not
re-anchored.  Because tk_core.timekeeper.tkr_raw uses a 22- or 23-bit
multiplier from clocks_calc_mult_shift() whereas ka->master_tsc_mul uses a
32-bit multiplier from kvm_get_time_scale() (plus intermediate rounding in
kvm_scale_tsc() when TSC scaling is active), the two elapsed times drift
apart over long uninterrupted uptime until their difference exceeds
NSEC_PER_SEC.

Once that threshold is crossed, __get_kvmclock_master_clock() returns
false, causing get_kvmclock() (and KVM_GET_CLOCK) to jump by at least a
second to get_kvmclock_base_ns() + ka->kvmclock_offset and drop
KVM_CLOCK_TSC_STABLE, KVM_CLOCK_REALTIME, and KVM_CLOCK_HOST_TSC, while
leaving data->host_tsc modified from the earlier
kvm_get_walltime_and_clockread() call.  Meanwhile, kvm_guest_time_update()
continues populating the guest's pvclock from the master clock snapshot
with PVCLOCK_TSC_STABLE_BIT set.

Should this check be gated on having no online/created vCPUs, or should
ka->master_cycle_now and ka->master_kernel_ns be refreshed when the first
vCPU is created so no fallback check is needed in
__get_kvmclock_master_clock()?

[ ... ]

> @@ -10271,14 +10314,46 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
>  	kvm->arch.cur_tsc_scaling_ratio = kvm_caps.default_tsc_scaling_ratio;
>  
>  	raw_spin_lock_irqsave(&kvm->arch.tsc_write_lock, flags);
> +
> +	/*
> +	 * Establish an initial TSC generation so that the master clock
> +	 * snapshot taken below is immediately valid and KVM_GET_CLOCK
> +	 * works before any vCPU exists. The snapshot is refreshed again
> +	 * when the first vCPU is created, in case the host clocksource
> +	 * changed (and changed back) in between: pvclock_gtod_update_fn()
> +	 * does not notify a VM with no vCPUs.
> +	 *
> +	 * This generation is deliberately not matchable: cur_tsc_khz is
> +	 * left at zero, which no vCPU frequency ever equals, so the first
> +	 * vCPU opens its own generation with its TSC starting at zero,
> +	 * exactly as it always has. Letting it match would make the first
> +	 * vCPU's TSC start at the cycles elapsed since KVM_CREATE_VM — a
> +	 * silent guest-visible change over an unbounded window — and
> +	 * would hand it this offset verbatim even when a host suspend
> +	 * reset the TSC in between, since the backwards-TSC compensation
> +	 * in kvm_arch_enable_virtualization_cpu() detects the jump only
> +	 * through existing vCPUs.
> +	 */
> +#ifdef CONFIG_X86_64
> +	if (!kvm_get_time_and_clockread(&kvmclock_ns, &kvmclock_host_tsc))
> +#endif
> +	{
> +		/*
> +		 * If not simultaneous, we want the TSC to be zero slightly
> +		 * *after* the kvmclock is zero.
> +		 */
> +		kvmclock_ns = get_kvmclock_base_ns();
> +		kvmclock_host_tsc = rdtsc();
> +	}
> +	kvm->arch.cur_tsc_write = 0;
> +	kvm->arch.cur_tsc_offset = -(s64)kvmclock_host_tsc;
> +	kvm->arch.cur_tsc_nsec = kvmclock_ns;
> +	kvm->arch.cur_tsc_generation = 1;
> +	kvm->arch.kvmclock_offset = -kvmclock_ns;
> +
>  	pvclock_update_vm_gtod_copy(kvm);
>  	raw_spin_unlock_irqrestore(&kvm->arch.tsc_write_lock, flags);

[Severity: Low]
Are the stores to cur_tsc_write, cur_tsc_offset, cur_tsc_nsec, and
cur_tsc_generation (along with the kvmclock_host_tsc read) dead code?

pvclock_update_vm_gtod_copy() takes its own snapshot into master_kernel_ns
and master_cycle_now and does not read any of the cur_tsc_* fields.
Because cur_tsc_khz is left at 0 and user_set_tsc is false, no caller can
match or consume this initial generation before the first vCPU's
__kvm_synchronize_tsc() call runs with matched == false and overwrites
cur_tsc_generation, cur_tsc_nsec, cur_tsc_write, and cur_tsc_offset.

Also, the commit message states that this patch makes
pvclock_update_vm_gtod_copy() succeed in kvm_arch_init_vm(), but earlier
commits in the series (commit "KVM: x86: Replace nr_vcpus_matched_tsc
count with all_vcpus_matched_tsc bool" and commit "KVM: x86: Allow KVM
master clock mode when TSCs are offset from each other") already
initialized all_vcpus_matched_tsc, cur_tsc_scaling_ratio, and
all_vcpus_matched_freq in kvm_arch_init_vm().

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