From: Jork Loeser <[email protected]> Sent: Tuesday, April 7, 2026 6:37 PM > > Move hv_stimer_global_cleanup() from vmbus's hv_kexec_handler() to > hv_machine_shutdown() in the platform code. This ensures stimer cleanup > happens before the vmbus unload, which is required for root partition > kexec to work correctly.
I know I'm late to the party in commenting on this patch set. But I'll add my $.02 anyway on this patch and a couple others in the set. I had difficulty understanding this patch's intent. The commit message says the intent is to make sure stimer cleanup happens before VMBus unload. But at first glance, the code in this patch doesn't change the ordering. It appears to be immaterial whether hv_stimer_global_cleanup() is called just before invoking hv_kexec_handler(), or as the first step of hv_kexec_handler(). But then I realized that hv_kexec_handler isn't set unless the VMBus driver is loaded and initialized, and that doesn't happen in the root partition. In the old code, stimer cleanup is dependent on the VMBus driver being loaded, and that's a wrong dependency, as stimer operation is independent of VMBus (almost -- there's the old non-direct mode stimer path that depends on the VMBus interrupt handler, which muddies the conceptual separation). So the patch does the right thing, but the commit message doesn't make the real reasoning clear. My feedback would be to improve the commit message. Michael > > Co-developed-by: Anirudh Rayabharam <[email protected]> > Signed-off-by: Anirudh Rayabharam <[email protected]> > Signed-off-by: Jork Loeser <[email protected]> > --- > arch/x86/kernel/cpu/mshyperv.c | 8 ++++++-- > drivers/hv/vmbus_drv.c | 1 - > 2 files changed, 6 insertions(+), 3 deletions(-) > > diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c > index a7dfc29d3470..e498b6b2ef19 100644 > --- a/arch/x86/kernel/cpu/mshyperv.c > +++ b/arch/x86/kernel/cpu/mshyperv.c > @@ -237,8 +237,12 @@ void hv_remove_crash_handler(void) > #ifdef CONFIG_KEXEC_CORE > static void hv_machine_shutdown(void) > { > - if (kexec_in_progress && hv_kexec_handler) > - hv_kexec_handler(); > + if (kexec_in_progress) { > + hv_stimer_global_cleanup(); > + > + if (hv_kexec_handler) > + hv_kexec_handler(); > + } Immediately following your code change is this code: if (kexec_in_progress) cpuhp_remove_state(CPUHP_AP_HYPERV_ONLINE); So with this change there are two "if (kexec_in_progress)" statements in a row. It's not wrong, but it looks like the patch wasn't fully integrated into the existing code. The cpuhp_remove_state() and the comment should be moved under the newly added "if (kexec_in_progress)" and the duplicate "if (kexec_in_progress)" can be dropped. Michael > > /* > * Call hv_cpu_die() on all the CPUs, otherwise later the hypervisor > diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c > index 5e7a6839c933..c5dfe9f3b206 100644 > --- a/drivers/hv/vmbus_drv.c > +++ b/drivers/hv/vmbus_drv.c > @@ -2891,7 +2891,6 @@ static struct platform_driver vmbus_platform_driver = { > > static void hv_kexec_handler(void) > { > - hv_stimer_global_cleanup(); > vmbus_initiate_unload(false); > /* Make sure conn_state is set as hv_synic_cleanup checks for it */ > mb(); > -- > 2.43.0 >
