Message-ID: <5a0858597867948f271fa5676fb2e3361f35d920.camel@amazon.com>
Date: Mon, 18 Dec 2023 12:41:34 +0000
From: "Gowans, James" <jgowans@...zon.com>
To: "ebiederm@...ssion.com" <ebiederm@...ssion.com>
CC: "kexec@...ts.infradead.org" <kexec@...ts.infradead.org>, "maz@...nel.org"
	<maz@...nel.org>, "tony.luck@...el.com" <tony.luck@...el.com>,
	"sre@...nel.org" <sre@...nel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "seanjc@...gle.com" <seanjc@...gle.com>,
	"mingo@...hat.com" <mingo@...hat.com>, "pbonzini@...hat.com"
	<pbonzini@...hat.com>, "tglx@...utronix.de" <tglx@...utronix.de>,
	"arnd@...db.de" <arnd@...db.de>, "wens@...e.org" <wens@...e.org>, "Graf
 (AWS), Alexander" <graf@...zon.de>, Schönherr, Jan H.
	<jschoenh@...zon.de>, "orsonzhai@...il.com" <orsonzhai@...il.com>,
	"bp@...en8.de" <bp@...en8.de>, "samuel@...lland.org" <samuel@...lland.org>,
	"pavel@....cz" <pavel@....cz>, "jernej.skrabec@...il.com"
	<jernej.skrabec@...il.com>
Subject: Re: [PATCH] kexec: do syscore_shutdown() in kernel_kexec

Hi Eric,

On Wed, 2023-12-13 at 10:39 -0600, Eric W. Biederman wrote:
> 
> James Gowans <jgowans@...zon.com> writes:
> 
> > syscore_shutdown() runs driver and module callbacks to get the system
> > into a state where it can be correctly shut down. In commit
> > 6f389a8f1dd2 ("PM / reboot: call syscore_shutdown() after disable_nonboot_cpus()")
> > syscore_shutdown() was removed from kernel_restart_prepare() and hence
> > got (incorrectly?) removed from the kexec flow. This was innocuous until
> > commit 6735150b6997 ("KVM: Use syscore_ops instead of reboot_notifier to hook restart/shutdown")
> > changed the way that KVM registered its shutdown callbacks, switching from
> > reboot notifiers to syscore_ops.shutdown. As syscore_shutdown() is
> > missing from kexec, KVM's shutdown hook is not run and virtualisation is
> > left enabled on the boot CPU which results in triple faults when
> > switching to the new kernel on Intel x86 VT-x with VMXE enabled.
> > 
> > Fix this by adding syscore_shutdown() to the kexec sequence. In terms of
> > where to add it, it is being added after migrating the kexec task to the
> > boot CPU, but before APs are shut down. It is not totally clear if this
> > is the best place: in commit 6f389a8f1dd2 ("PM / reboot: call syscore_shutdown() after disable_nonboot_cpus()")
> > it is stated that "syscore_ops operations should be carried with one
> > CPU on-line and interrupts disabled." APs are only offlined later in
> > machine_shutdown(), so this syscore_shutdown() is being run while APs
> > are still online. This seems to be the correct place as it matches where
> > syscore_shutdown() is run in the reboot and halt flows - they also run
> > it before APs are shut down. The assumption is that the commit message
> > in commit 6f389a8f1dd2 ("PM / reboot: call syscore_shutdown() after disable_nonboot_cpus()")
> > is no longer valid.
> > 
> > KVM has been discussed here as it is what broke loudly by not having
> > syscore_shutdown() in kexec, but this change impacts more than just KVM;
> > all drivers/modules which register a syscore_ops.shutdown callback will
> > now be invoked in the kexec flow. Looking at some of them like x86 MCE
> > it is probably more correct to also shut these down during kexec.
> > Maintainers of all drivers which use syscore_ops.shutdown are added on
> > CC for visibility. They are:
> > 
> > arch/powerpc/platforms/cell/spu_base.c  .shutdown = spu_shutdown,
> > arch/x86/kernel/cpu/mce/core.c                .shutdown = mce_syscore_shutdown,
> > arch/x86/kernel/i8259.c                 .shutdown = i8259A_shutdown,
> > drivers/irqchip/irq-i8259.c           .shutdown = i8259A_shutdown,
> > drivers/irqchip/irq-sun6i-r.c         .shutdown = sun6i_r_intc_shutdown,
> > drivers/leds/trigger/ledtrig-cpu.c    .shutdown = ledtrig_cpu_syscore_shutdown,
> > drivers/power/reset/sc27xx-poweroff.c .shutdown = sc27xx_poweroff_shutdown,
> > kernel/irq/generic-chip.c             .shutdown = irq_gc_shutdown,
> > virt/kvm/kvm_main.c                   .shutdown = kvm_shutdown,
> > 
> > This has been tested by doing a kexec on x86_64 and aarch64.
> 
> From the 10,000 foot perspective:
> Acked-by: "Eric W. Biederman" <ebiederm@...ssion.com>

Thanks for the ACK!
What's the next step to get this into the kexec tree?

JG

> 
> 
> Eric
> 
> > Fixes: 6735150b6997 ("KVM: Use syscore_ops instead of reboot_notifier to hook restart/shutdown")
> > 
> > Signed-off-by: James Gowans <jgowans@...zon.com>
> > Cc: Eric Biederman <ebiederm@...ssion.com>
> > Cc: Paolo Bonzini <pbonzini@...hat.com>
> > Cc: Sean Christopherson <seanjc@...gle.com>
> > Cc: Marc Zyngier <maz@...nel.org>
> > Cc: Arnd Bergmann <arnd@...db.de>
> > Cc: Tony Luck <tony.luck@...el.com>
> > Cc: Borislav Petkov <bp@...en8.de>
> > Cc: Thomas Gleixner <tglx@...utronix.de>
> > Cc: Ingo Molnar <mingo@...hat.com>
> > Cc: Chen-Yu Tsai <wens@...e.org>
> > Cc: Jernej Skrabec <jernej.skrabec@...il.com>
> > Cc: Samuel Holland <samuel@...lland.org>
> > Cc: Pavel Machek <pavel@....cz>
> > Cc: Sebastian Reichel <sre@...nel.org>
> > Cc: Orson Zhai <orsonzhai@...il.com>
> > Cc: Alexander Graf <graf@...zon.de>
> > Cc: Jan H. Schoenherr <jschoenh@...zon.de>
> > ---
> >  kernel/kexec_core.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> > index be5642a4ec49..b926c4db8a91 100644
> > --- a/kernel/kexec_core.c
> > +++ b/kernel/kexec_core.c
> > @@ -1254,6 +1254,7 @@ int kernel_kexec(void)
> >               kexec_in_progress = true;
> >               kernel_restart_prepare("kexec reboot");
> >               migrate_to_reboot_cpu();
> > +             syscore_shutdown();
> > 
> >               /*
> >                * migrate_to_reboot_cpu() disables CPU hotplug assuming that

Powered by blists - more mailing lists