Message-ID: <04EAB7311EE43145B2D3536183D1A84454CBBABC@GSjpTKYDCembx31.service.hitachi.net>
Date:	Mon, 15 Aug 2016 11:22:08 +0000
From:	河合英宏 / KAWAI，HIDEHIRO 
	<hidehiro.kawai.ez@...achi.com>
To:	"'Dave Young'" <dyoung@...hat.com>
CC:	Andrew Morton <akpm@...ux-foundation.org>,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	Baoquan He <bhe@...hat.com>,
	Ralf Baechle <ralf@...ux-mips.org>,
	"linux-mips@...ux-mips.org" <linux-mips@...ux-mips.org>,
	"xen-devel@...ts.xenproject.org" <xen-devel@...ts.xenproject.org>,
	Toshi Kani <toshi.kani@....com>,
	"Xunlei Pang" <xpang@...hat.com>,
	"x86@...nel.org" <x86@...nel.org>,
	"kexec@...ts.infradead.org" <kexec@...ts.infradead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"HATAYAMA Daisuke" <d.hatayama@...fujitsu.com>,
	Ingo Molnar <mingo@...hat.com>,
	"David Vrabel" <david.vrabel@...rix.com>,
	"H. Peter Anvin" <hpa@...or.com>,
	"Daniel Walker" <dwalker@...o99.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	"Borislav Petkov" <bp@...e.de>, Vivek Goyal <vgoyal@...hat.com>,
	Masami Hiramatsu <mhiramat@...nel.org>
Subject: RE: Re: [V4 PATCH 1/2] x86/panic: Replace smp_send_stop() with
 kdump friendly version in panic path

Hi Dave,

Thank you for the review.

> From: Dave Young [mailto:dyoung@...hat.com]
> Sent: Friday, August 12, 2016 12:17 PM
> 
> Thanks for the update.
> On 08/10/16 at 05:09pm, Hidehiro Kawai wrote:
> > Daniel Walker reported problems which happens when
> > crash_kexec_post_notifiers kernel option is enabled
> > (https://lkml.org/lkml/2015/6/24/44).
> >
> > In that case, smp_send_stop() is called before entering kdump routines
> > which assume other CPUs are still online.  As the result, for x86,
> > kdump routines fail to save other CPUs' registers  and disable
> > virtualization extensions.
> 
> Seems you simplified the changelog, but I think a little more details
> will be helpful to understand the patch. You know sometimes lkml.org
> does not work well.

So, I'll try another archives when I post patch set next time.

> > To fix this problem, call a new kdump friendly function,
> > crash_smp_send_stop(), instead of the smp_send_stop() when
> > crash_kexec_post_notifiers is enabled.  crash_smp_send_stop() is a
> > weak function, and it just call smp_send_stop().  Architecture
> > codes should override it so that kdump can work appropriately.
> > This patch only provides x86-specific version.
> >
> > For Xen's PV kernel, just keep the current behavior.
> 
> Could you explain a bit about above Xen PV kernel behavior?
> 
> BTW, this version looks better,  I think I'm fine with this version
> besides of the questions about changelog.

As for Dom0 kernel, it doesn't use crash_kexec routines, and
it relies on panic notifier chain.  At the end of the chain,
xen_panic_event is called, and it issues a hypercall which
requests Hypervisor to execute kdump.  This means whether
crash_kexec_panic_notifiers is set or not, panic notifiers
are called after smp_send_stop.  Even if we save registers
in Dom0 kernel, they seem to be ignored (Hypervisor is responsible
for that).  This is why I kept the current behavior for Xen.

For PV DomU kernel, kdump is not supported.  For PV HVM
DomU, I'm not sure what will happen on panic because I
couldn't boot PV HVM DomU and test it.  But I think it will
work similarly to baremetal kernels with extra cleanups
for Hypervisor.

Best regards,

Hidehiro Kawai

> > Changes in V4:
> > - Keep to use smp_send_stop if crash_kexec_post_notifiers is not set
> > - Rename panic_smp_send_stop to crash_smp_send_stop
> > - Don't change the behavior for Xen's PV kernel
> >
> > Changes in V3:
> > - Revise comments, description, and symbol names
> >
> > Changes in V2:
> > - Replace smp_send_stop() call with crash_kexec version which
> >   saves cpu states and cleans up VMX/SVM
> > - Drop a fix for Problem 1 at this moment
> >
> > Reported-by: Daniel Walker <dwalker@...o99.com>
> > Fixes: f06e5153f4ae (kernel/panic.c: add "crash_kexec_post_notifiers" option)
> > Signed-off-by: Hidehiro Kawai <hidehiro.kawai.ez@...achi.com>
> > Cc: Dave Young <dyoung@...hat.com>
> > Cc: Baoquan He <bhe@...hat.com>
> > Cc: Vivek Goyal <vgoyal@...hat.com>
> > Cc: Eric Biederman <ebiederm@...ssion.com>
> > Cc: Masami Hiramatsu <mhiramat@...nel.org>
> > Cc: Daniel Walker <dwalker@...o99.com>
> > Cc: Xunlei Pang <xpang@...hat.com>
> > Cc: Thomas Gleixner <tglx@...utronix.de>
> > Cc: Ingo Molnar <mingo@...hat.com>
> > Cc: "H. Peter Anvin" <hpa@...or.com>
> > Cc: Borislav Petkov <bp@...e.de>
> > Cc: David Vrabel <david.vrabel@...rix.com>
> > Cc: Toshi Kani <toshi.kani@....com>
> > Cc: Andrew Morton <akpm@...ux-foundation.org>
> > ---
> >  arch/x86/include/asm/kexec.h |    1 +
> >  arch/x86/include/asm/smp.h   |    1 +
> >  arch/x86/kernel/crash.c      |   22 +++++++++++++++++---
> >  arch/x86/kernel/smp.c        |    5 ++++
> >  kernel/panic.c               |   47 ++++++++++++++++++++++++++++++++++++------
> >  5 files changed, 66 insertions(+), 10 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/kexec.h b/arch/x86/include/asm/kexec.h
> > index d2434c1..282630e 100644
> > --- a/arch/x86/include/asm/kexec.h
> > +++ b/arch/x86/include/asm/kexec.h
> > @@ -210,6 +210,7 @@ struct kexec_entry64_regs {
> >
> >  typedef void crash_vmclear_fn(void);
> >  extern crash_vmclear_fn __rcu *crash_vmclear_loaded_vmcss;
> > +extern void kdump_nmi_shootdown_cpus(void);
> >
> >  #endif /* __ASSEMBLY__ */
> >
> > diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
> > index ebd0c16..f70989c 100644
> > --- a/arch/x86/include/asm/smp.h
> > +++ b/arch/x86/include/asm/smp.h
> > @@ -50,6 +50,7 @@ struct smp_ops {
> >  	void (*smp_cpus_done)(unsigned max_cpus);
> >
> >  	void (*stop_other_cpus)(int wait);
> > +	void (*crash_stop_other_cpus)(void);
> >  	void (*smp_send_reschedule)(int cpu);
> >
> >  	int (*cpu_up)(unsigned cpu, struct task_struct *tidle);
> > diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
> > index 9616cf7..650830e 100644
> > --- a/arch/x86/kernel/crash.c
> > +++ b/arch/x86/kernel/crash.c
> > @@ -133,15 +133,31 @@ static void kdump_nmi_callback(int cpu, struct pt_regs *regs)
> >  	disable_local_APIC();
> >  }
> >
> > -static void kdump_nmi_shootdown_cpus(void)
> > +void kdump_nmi_shootdown_cpus(void)
> >  {
> >  	nmi_shootdown_cpus(kdump_nmi_callback);
> >
> >  	disable_local_APIC();
> >  }
> >
> > +/* Override the weak function in kernel/panic.c */
> > +void crash_smp_send_stop(void)
> > +{
> > +	static int cpus_stopped;
> > +
> > +	if (cpus_stopped)
> > +		return;
> > +
> > +	if (smp_ops.crash_stop_other_cpus)
> > +		smp_ops.crash_stop_other_cpus();
> > +	else
> > +		smp_send_stop();
> > +
> > +	cpus_stopped = 1;
> > +}
> > +
> >  #else
> > -static void kdump_nmi_shootdown_cpus(void)
> > +void crash_smp_send_stop(void)
> >  {
> >  	/* There are no cpus to shootdown */
> >  }
> > @@ -160,7 +176,7 @@ void native_machine_crash_shutdown(struct pt_regs *regs)
> >  	/* The kernel is broken so disable interrupts */
> >  	local_irq_disable();
> >
> > -	kdump_nmi_shootdown_cpus();
> > +	crash_smp_send_stop();
> >
> >  	/*
> >  	 * VMCLEAR VMCSs loaded on this cpu if needed.
> > diff --git a/arch/x86/kernel/smp.c b/arch/x86/kernel/smp.c
> > index 658777c..68f8cc2 100644
> > --- a/arch/x86/kernel/smp.c
> > +++ b/arch/x86/kernel/smp.c
> > @@ -32,6 +32,8 @@
> >  #include <asm/nmi.h>
> >  #include <asm/mce.h>
> >  #include <asm/trace/irq_vectors.h>
> > +#include <asm/kexec.h>
> > +
> >  /*
> >   *	Some notes on x86 processor bugs affecting SMP operation:
> >   *
> > @@ -342,6 +344,9 @@ struct smp_ops smp_ops = {
> >  	.smp_cpus_done		= native_smp_cpus_done,
> >
> >  	.stop_other_cpus	= native_stop_other_cpus,
> > +#if defined(CONFIG_KEXEC_CORE)
> > +	.crash_stop_other_cpus	= kdump_nmi_shootdown_cpus,
> > +#endif
> >  	.smp_send_reschedule	= native_smp_send_reschedule,
> >
> >  	.cpu_up			= native_cpu_up,
> > diff --git a/kernel/panic.c b/kernel/panic.c
> > index ca8cea1..e6480e2 100644
> > --- a/kernel/panic.c
> > +++ b/kernel/panic.c
> > @@ -71,6 +71,32 @@ void __weak nmi_panic_self_stop(struct pt_regs *regs)
> >  	panic_smp_self_stop();
> >  }
> >
> > +/*
> > + * Stop other CPUs in panic.  Architecture dependent code may override this
> > + * with more suitable version.  For example, if the architecture supports
> > + * crash dump, it should save registers of each stopped CPU and disable
> > + * per-CPU features such as virtualization extensions.
> > + */
> > +void __weak crash_smp_send_stop(void)
> > +{
> > +	static int cpus_stopped;
> > +
> > +	/*
> > +	 * This function can be called twice in panic path, but obviously
> > +	 * we execute this only once.
> > +	 */
> > +	if (cpus_stopped)
> > +		return;
> > +
> > +	/*
> > +	 * Note smp_send_stop is the usual smp shutdown function, which
> > +	 * unfortunately means it may not be hardened to work in a panic
> > +	 * situation.
> > +	 */
> > +	smp_send_stop();
> > +	cpus_stopped = 1;
> > +}
> > +
> >  atomic_t panic_cpu = ATOMIC_INIT(PANIC_CPU_INVALID);
> >
> >  /*
> > @@ -164,14 +190,21 @@ void panic(const char *fmt, ...)
> >  	if (!_crash_kexec_post_notifiers) {
> >  		printk_nmi_flush_on_panic();
> >  		__crash_kexec(NULL);
> > -	}
> >
> > -	/*
> > -	 * Note smp_send_stop is the usual smp shutdown function, which
> > -	 * unfortunately means it may not be hardened to work in a panic
> > -	 * situation.
> > -	 */
> > -	smp_send_stop();
> > +		/*
> > +		 * Note smp_send_stop is the usual smp shutdown function, which
> > +		 * unfortunately means it may not be hardened to work in a
> > +		 * panic situation.
> > +		 */
> > +		smp_send_stop();
> > +	} else {
> > +		/*
> > +		 * If we want to do crash dump after notifier calls and
> > +		 * kmsg_dump, we will need architecture dependent extra
> > +		 * works in addition to stopping other CPUs.
> > +		 */
> > +		crash_smp_send_stop();
> > +	}
> >
> >  	/*
> >  	 * Run any panic handlers, including those that might need to
> >
> >

Powered by blists - more mailing lists