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 <[email protected]> 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/[email protected]?part=2
