On Fri, May 15, 2026, [email protected] wrote: > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) > to consider: > - [High] The logic to defer AP kvmclock registration to the > CPUHP_AP_ONLINE_DYN state when it is not used as sched_clock is > architecturally unsafe and will break timekeeping. > -- > > commit 88625772445f01c4397e3dfd4ee43222efd758cf > Author: Sean Christopherson <[email protected]> > > x86/kvmclock: Enable kvmclock on APs during onlining if kvmclock isn't > sched_clock > > This commit prepares for making early_percpu_clock_init() a dedicated > sched_clock hook. It adds logic to enable kvmclock on APs during CPU online > if kvmclock is not used as sched_clock, and includes WARNs in the suspend > and resume paths. > > > diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c > > index e7250d21c6721..d3bb281c0805d 100644 > > --- a/arch/x86/kernel/kvmclock.c > > +++ b/arch/x86/kernel/kvmclock.c > [ ... ] > > @@ -141,25 +142,51 @@ static void kvm_restore_sched_clock_state(void) > [ ... ] > > void kvmclock_cpu_action(enum kvm_guest_cpu_action action) > > { > > - /* > > - * Don't disable kvmclock on the BSP during suspend. If kvmclock is > > - * being used for sched_clock, then it needs to be kept alive until the > > - * last minute, and restored as quickly as possible after resume. > > - */ > > - if (action != KVM_GUEST_BSP_SUSPEND) > > + switch (action) { > [ ... ] > > + case KVM_GUEST_AP_ONLINE: > > + /* > > + * Secondary CPUs use dedicated sched_clock hooks to enable > > + * kvmclock early during bringup, there's nothing to be done > > + * when during CPU online. > > + */ > > + if (kvmclock_is_sched_clock) > > + break; > > + kvm_register_clock("secondary cpu, online"); > > + break; > > Will deferring AP kvmclock registration to the CPU online phase break > timekeeping if kvmclock is still being used as the system clocksource? > > If the user boots with clocksource=kvm-clock or if kvmclock is selected as a > fallback, it will be used as the clocksource despite not being sched_clock. > The kvmclock clocksource read function accesses per-CPU memory that the > hypervisor only populates after the AP registers its clock. > > If early registration in start_secondary() is skipped, as anticipated here, > an AP will boot, enable local interrupts, and process timer ticks before > reaching the late CPUHP_AP_ONLINE_DYN hotplug state. > > Could a call to ktime_get() during this window read uninitialized per-CPU > memory and return 0? It seems this might cause the timekeeper to compute > massive forward time jumps, leading to immediate timer expirations and > watchdogs firing.
Hrm, yes, that seems plausible. Even if it's not actually possible in practice, replacing x86_cpuinit.early_percpu_clock_init() with a dedicated paravirt_sched_clock_start_secondary() isn't obviously better, and so definitely isn't worth risking breakage, at least not in this series. I'll scrap most of this, but still plumb in AP_ONLINE and BSP_RESUME for documentation purposes. > > + case KVM_GUEST_AP_OFFLINE: > > + case KVM_GUEST_SHUTDOWN: > > kvmclock_disable(); > > + break; > > + default: > > + WARN_ON_ONCE(1); > > + break; > > + } > > } > > -- > Sashiko AI review ยท > https://sashiko.dev/#/patchset/[email protected]?part=27
