Re: [PATCH v2 2/3] x86/kvmclock: Implement read_snapshot() for kvmclock clocksource

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] kvm_clock_get_cycles_snapshot() unconditionally returns raw TSC cycles and bypasses the global last_value monotonicity clamp, breaking timekeeping on hosts with unstable TSCs.
--

commit 6dc8b64e567331ab0edc9710a62a9a13cd0b8811
Author: David Woodhouse <dwmw@amazon.co.uk>

x86/kvmclock: Implement read_snapshot() for kvmclock clocksource

This commit implements the read_snapshot() callback for the kvmclock
clocksource. This allows returning both the kvmclock nanosecond value
and the raw TSC value used to compute it.

> diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
> index b5991d53fc0ea..cb3d0ca1fa225 100644
> --- a/arch/x86/kernel/kvmclock.c
> +++ b/arch/x86/kernel/kvmclock.c

[ ... ]

> @@ -87,6 +87,27 @@ static u64 kvm_clock_get_cycles(struct clocksource *cs)
>  	return kvm_clock_read();
>  }
>  
> +static u64 kvm_clock_get_cycles_snapshot(struct clocksource *cs,
> +					 struct clocksource_hw_snapshot *chs)
> +{
> +	struct pvclock_vcpu_time_info *src;
> +	unsigned version;
> +	u64 ret, tsc;
> +
> +	preempt_disable_notrace();
> +	src = this_cpu_pvti();
> +	do {
> +		version = pvclock_read_begin(src);
> +		tsc = rdtsc_ordered();
> +		ret = __pvclock_read_cycles(src, tsc);
> +	} while (pvclock_read_retry(src, version));
> +	preempt_enable_notrace();
> +
> +	chs->hw_cycles = tsc;
> +	chs->hw_csid = CSID_X86_TSC;

[Severity: High]
Does this open-coded read loop bypass the fallback monotonic clamping used in
the standard kvm-clock read path?

The standard path (__pvclock_clocksource_read) enforces clock monotonicity
by clamping to a globally shared last_value accumulator when
PVCLOCK_TSC_STABLE_BIT is not set.

Without this check and clamping logic, if a vCPU migrates to a host with an
unsynchronized or slower TSC, could the snapshot return a derived time that
goes backwards relative to the timekeeper's cycle_last?

Also, by unconditionally returning CSID_X86_TSC and the raw hardware cycles
even when the host marks the TSC as unstable, does this incorrectly trick
cross-timestamping consumers into assuming they have a reliable, synchronized
hardware timestamp?

> +	return ret;
> +}

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