Re: [PATCH v3 27/41] x86/kvmclock: Enable kvmclock on APs during onlining if kvmclock isn't sched_clock

[Sean Christopherson <seanjc@google.com>]

On Fri, May 15, 2026, sashiko-bot@kernel.org 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 <seanjc@google.com>
> 
> 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/20260515191942.1892718-1-seanjc@google.com?part=27