On Fri, May 15, 2026, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s)
> to consider:
> > diff --git a/arch/x86/xen/time.c b/arch/x86/xen/time.c
> > index 3d3165eef8214..21d366d01985e 100644
> > --- a/arch/x86/xen/time.c
> > +++ b/arch/x86/xen/time.c
> > @@ -568,6 +568,12 @@ static void __init xen_init_time_common(void)
> > xen_sched_clock_offset = xen_clocksource_read();
> > static_call_update(pv_steal_clock, xen_steal_clock);
> > paravirt_set_sched_clock(xen_sched_clock);
> > + /*
> > + * Xen has paravirtualized suspend/resume and so doesn't use the common
> > + * x86 sched_clock save/restore hooks.
> > + */
> > + x86_platform.save_sched_clock_state = NULL;
> > + x86_platform.restore_sched_clock_state = NULL;
>
> Does setting these hooks to NULL lead to an unconditional pointer dereference
> regression during suspend, hibernation, or kexec operations in Xen guests?
>
> Looking at save_processor_state() and __restore_processor_state() in
> arch/x86/power/cpu.c, the hooks are called directly without being checked
> for NULL first:
>
> arch/x86/power/cpu.c:save_processor_state() {
> __save_processor_state(&saved_context);
> x86_platform.save_sched_clock_state();
> }
>
> arch/x86/power/cpu.c:__restore_processor_state() {
> ...
> do_fpu_end();
> tsc_verify_tsc_adjust(true);
> x86_platform.restore_sched_clock_state();
> ...
> }
>
> Will operations that trigger a generic CPU state save or restore on a Xen
> guest execute a call on these NULL pointers, causing a kernel panic
> regression? Should these instead be assigned to an empty stub function to
> safely bypass them?
/facepalm
Yes. I suspect past me carried over the Xen changes before I understood why
it's safe on Xen (Xen uses a unique PV suspend/resume flow).
Playing nice with NULL x86_platform.{save,restore}_sched_clock_state pointers
is the obvious fix. The other option would be to wire up nop callbacks, but I
don't see any value in doing so. I really don't want to leave the callbacks
wired up to tsc_{save,restore}_sched_clock_state() (unless it turns out VMware
actually needs them).
Regardless, this definitely highlights that VMware guests need to be tested. :-/
