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

[sashiko-bot@kernel.org]

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.

> +	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