[PATCH 0/3] sreset debugging improvements

[PATCH 0/3] sreset debugging improvements

[Nicholas Piggin]

These are a few independent patches I've accumulated that improve
system reset handling a bit. The first 2 at least should be quite
straight forward and hopefully can be merged for 4.13.

Thanks,
Nick

Nicholas Piggin (3):
  powerpc: do not call ppc_md.panic in panic notifier if fadump not used
  powerpc/pseries/le: work around a firmware quirk
  powerpc: do not send system reset request through the oops path

 arch/powerpc/kernel/setup-common.c   | 15 +++++++-----
 arch/powerpc/kernel/traps.c          | 47 ++++++++++++++++++++++++------------
 arch/powerpc/platforms/pseries/ras.c | 15 ++++++++++++
 3 files changed, 55 insertions(+), 22 deletions(-)

-- 
2.11.0

[PATCH 1/3] powerpc: do not call ppc_md.panic in panic notifier if fadump not used

[Nicholas Piggin]

If fadump is not registered, and no other crash or debug handlers are
registered, the powerpc panic handler stops the guest before the generic
panic code can push out debug information to the console.

Without this patch, system reset injection to a guest causes the guest to
silently stop. Afterwards, we get the expected oops trace.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/kernel/setup-common.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c
index 94a948207cd2..39ba09965b04 100644
--- a/arch/powerpc/kernel/setup-common.c
+++ b/arch/powerpc/kernel/setup-common.c
@@ -707,12 +707,15 @@ EXPORT_SYMBOL(check_legacy_ioport);
 static int ppc_panic_event(struct notifier_block *this,
                              unsigned long event, void *ptr)
 {
-	/*
-	 * If firmware-assisted dump has been registered then trigger
-	 * firmware-assisted dump and let firmware handle everything else.
-	 */
-	crash_fadump(NULL, ptr);
-	ppc_md.panic(ptr);  /* May not return */
+	if (is_fadump_active()) {
+		/*
+		 * If firmware-assisted dump has been registered then trigger
+		 * firmware-assisted dump and let firmware handle everything
+		 * else.
+		 */
+		crash_fadump(NULL, ptr);
+		ppc_md.panic(ptr);  /* May not return */
+	}
 	return NOTIFY_DONE;
 }
 
-- 
2.11.0

Re: [PATCH 1/3] powerpc: do not call ppc_md.panic in panic notifier if fadump not used

[Mahesh Jagannath Salgaonkar]

On 07/04/2017 03:39 PM, Nicholas Piggin wrote:
> If fadump is not registered, and no other crash or debug handlers are
> registered, the powerpc panic handler stops the guest before the generic
> panic code can push out debug information to the console.
> 
> Without this patch, system reset injection to a guest causes the guest to
> silently stop. Afterwards, we get the expected oops trace.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  arch/powerpc/kernel/setup-common.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c
> index 94a948207cd2..39ba09965b04 100644
> --- a/arch/powerpc/kernel/setup-common.c
> +++ b/arch/powerpc/kernel/setup-common.c
> @@ -707,12 +707,15 @@ EXPORT_SYMBOL(check_legacy_ioport);
>  static int ppc_panic_event(struct notifier_block *this,
>                               unsigned long event, void *ptr)
>  {
> -	/*
> -	 * If firmware-assisted dump has been registered then trigger
> -	 * firmware-assisted dump and let firmware handle everything else.
> -	 */
> -	crash_fadump(NULL, ptr);
> -	ppc_md.panic(ptr);  /* May not return */
> +	if (is_fadump_active()) {

Should it be !fw_dump.dump_registered check ? The function crash_dump()
already checks for !fw_dump.dump_registered before proceeding. If fadump
has not been registered then the crash_dump() will return immediately
without doing anything and then fall through next line ppc_md.panic(ptr).

fadump active is always false in the first kernel (production kernel).
Hence is_fadump_active() check here would stop triggering fadump even if
it is registered.

fadump active is true only during second kernel (dump capture kernel)
that boots after fadump crash when firmware exports 'ibm,kernel-dump'
device tree node indicating dump data is available.

Thanks,
-Mahesh.

> +		/*
> +		 * If firmware-assisted dump has been registered then trigger
> +		 * firmware-assisted dump and let firmware handle everything
> +		 * else.
> +		 */
> +		crash_fadump(NULL, ptr);
> +		ppc_md.panic(ptr);  /* May not return */
> +	}
>  	return NOTIFY_DONE;
>  }
> 

Re: [PATCH 1/3] powerpc: do not call ppc_md.panic in panic notifier if fadump not used

[Michael Ellerman]

Nicholas Piggin <npiggin@gmail.com> writes:
> diff --git a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c
> index 94a948207cd2..39ba09965b04 100644
> --- a/arch/powerpc/kernel/setup-common.c
> +++ b/arch/powerpc/kernel/setup-common.c
> @@ -707,12 +707,15 @@ EXPORT_SYMBOL(check_legacy_ioport);
>  static int ppc_panic_event(struct notifier_block *this,
>                               unsigned long event, void *ptr)
>  {
> -	/*
> -	 * If firmware-assisted dump has been registered then trigger
> -	 * firmware-assisted dump and let firmware handle everything else.
> -	 */
> -	crash_fadump(NULL, ptr);
> -	ppc_md.panic(ptr);  /* May not return */
> +	if (is_fadump_active()) {
> +		/*
> +		 * If firmware-assisted dump has been registered then trigger
> +		 * firmware-assisted dump and let firmware handle everything
> +		 * else.
> +		 */
> +		crash_fadump(NULL, ptr);

As Mahesh pointed out the check for fadump active is not correct.

> +		ppc_md.panic(ptr);  /* May not return */

But I wonder if this is the real problem?

AFAICS we only have two users of ppc_md.panic(), ps3 and pseries.

At least on ps3 it has nothing to do with fadump, so skipping it like
you've done is not right.

On pseries it uses rtas_os_term() which tells the HV that we "terminated
normal operation", unless you're doing fadump it's supposed to return
and shouldn't stop the regular panic path. Though maybe that's not true
in practice.

It sounds like what we need to do is have a look at the panic flow and
decide whether ppc_md.panic is useful at all, and if so is it called in
the right place.

cheers

Re: [PATCH 1/3] powerpc: do not call ppc_md.panic in panic notifier if fadump not used

[Nicholas Piggin]

On Wed, 05 Jul 2017 09:42:46 +1000
Michael Ellerman <mpe@ellerman.id.au> wrote:

> Nicholas Piggin <npiggin@gmail.com> writes:
> > diff --git a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c
> > index 94a948207cd2..39ba09965b04 100644
> > --- a/arch/powerpc/kernel/setup-common.c
> > +++ b/arch/powerpc/kernel/setup-common.c
> > @@ -707,12 +707,15 @@ EXPORT_SYMBOL(check_legacy_ioport);
> >  static int ppc_panic_event(struct notifier_block *this,
> >                               unsigned long event, void *ptr)
> >  {
> > -	/*
> > -	 * If firmware-assisted dump has been registered then trigger
> > -	 * firmware-assisted dump and let firmware handle everything else.
> > -	 */
> > -	crash_fadump(NULL, ptr);
> > -	ppc_md.panic(ptr);  /* May not return */
> > +	if (is_fadump_active()) {
> > +		/*
> > +		 * If firmware-assisted dump has been registered then trigger
> > +		 * firmware-assisted dump and let firmware handle everything
> > +		 * else.
> > +		 */
> > +		crash_fadump(NULL, ptr);  
> 
> As Mahesh pointed out the check for fadump active is not correct.

Yep, I misread that code.

> 
> > +		ppc_md.panic(ptr);  /* May not return */  
> 
> But I wonder if this is the real problem?
> 
> AFAICS we only have two users of ppc_md.panic(), ps3 and pseries.
> 
> At least on ps3 it has nothing to do with fadump, so skipping it like
> you've done is not right.

But the call has to do with fadump -- at least that's what the comment
implies. ps3 does not want to panic here (it's .panic is just a hang).
It wants to go via the normal panic path and flush printk buffers etc.

> On pseries it uses rtas_os_term() which tells the HV that we "terminated
> normal operation", unless you're doing fadump it's supposed to return
> and shouldn't stop the regular panic path. Though maybe that's not true
> in practice.
> 
> It sounds like what we need to do is have a look at the panic flow and
> decide whether ppc_md.panic is useful at all, and if so is it called in
> the right place.

Yeah I would say you're probably right. The panic handling could possibly
go into the fadump code itself too, if it's relatively common across all
platforms that use it. It can register its own panic handler, no need for
this generic one.

Thanks,
Nick

[PATCH 2/3] powerpc/pseries/le: work around a firmware quirk

[Nicholas Piggin]

Some PowerVM firmware when delivering a system reset interrupt to a
little endian OS will mess up SRR registers. They are byteswapped, and
SRR1 is incorrect. An example from a crash:

  NIP: 14dd0900000000c0
  MSR: 1000000200000080

It's possible to detect this pattern in SRR1 (that would never happen in
normal operation), and at least fix the NIP. After this patch, the same
interrupt reports NIP properly:

  NIP [c00000000009dd14] plpar_hcall_norets+0x1c/0x28

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/platforms/pseries/ras.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/arch/powerpc/platforms/pseries/ras.c b/arch/powerpc/platforms/pseries/ras.c
index bb70b26334f0..4923ffe230cf 100644
--- a/arch/powerpc/platforms/pseries/ras.c
+++ b/arch/powerpc/platforms/pseries/ras.c
@@ -379,6 +379,21 @@ static void fwnmi_release_errinfo(void)
 
 int pSeries_system_reset_exception(struct pt_regs *regs)
 {
+#ifdef __LITTLE_ENDIAN__
+	/*
+	 * Some firmware byteswaps SRR registers and gives incorrect SRR1. Try
+	 * to detect the bad SRR1 pattern here. Flip the NIP back to correct
+	 * endian for reporting purposes. Unfortunately the MSR can't be fixed,
+	 * so clear it. It will be missing MSR_RI so we won't try to recover.
+	 */
+	if ((be64_to_cpu(regs->msr) &
+			(MSR_LE|MSR_RI|MSR_DR|MSR_IR|MSR_ME|MSR_PR|
+			 MSR_ILE|MSR_HV|MSR_SF)) == (MSR_DR|MSR_SF)) {
+		regs->nip = be64_to_cpu((__be64)regs->nip);
+		regs->msr = 0;
+	}
+#endif
+
 	if (fwnmi_active) {
 		struct rtas_error_log *errhdr = fwnmi_get_errinfo(regs);
 		if (errhdr) {
-- 
2.11.0

[PATCH 3/3] powerpc: do not send system reset request through the oops path

[Nicholas Piggin]

A system reset is a request to crash / debug the system rather than
necessarily caused by encountering a BUG. So there is no need to
serialize all CPUs behind the die lock, adding taints to all subsequent
traces beyond the first, breaking console locks, etc.

The system reset is NMI context which has its own printk buffers to
prevent output being interleaved. Then it's better to have all
secondaries print out their debug as quickly as possible and the primary
will flush out all printk buffers during panic().

So remove the 0x100 path from die, and move it into system_reset. Name
the crash/dump reasons "System Reset".

This gives "not tained" traces when crashing an untainted kernel. It
also gives the panic reason as "System Reset" as opposed to "Fatal
exception in interrupt" (or "die oops" for fadump).

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/kernel/traps.c | 47 ++++++++++++++++++++++++++++++---------------
 1 file changed, 31 insertions(+), 16 deletions(-)

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index bfcfd9ef09f2..574e949f8db9 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -162,21 +162,9 @@ static void oops_end(unsigned long flags, struct pt_regs *regs,
 
 	crash_fadump(regs, "die oops");
 
-	/*
-	 * A system reset (0x100) is a request to dump, so we always send
-	 * it through the crashdump code.
-	 */
-	if (kexec_should_crash(current) || (TRAP(regs) == 0x100)) {
+	if (kexec_should_crash(current))
 		crash_kexec(regs);
 
-		/*
-		 * We aren't the primary crash CPU. We need to send it
-		 * to a holding pattern to avoid it ending up in the panic
-		 * code.
-		 */
-		crash_kexec_secondary(regs);
-	}
-
 	if (!signr)
 		return;
 
@@ -294,17 +282,44 @@ void system_reset_exception(struct pt_regs *regs)
 			goto out;
 	}
 
-	die("System Reset", regs, SIGABRT);
+	if (debugger(regs))
+		goto out;
+
+	/*
+	 * A system reset is a request to dump, so we always send
+	 * it through the crashdump code (if fadump or kdump are
+	 * registered).
+	 */
+	crash_fadump(regs, "System Reset");
+
+	crash_kexec(regs);
+
+	/*
+	 * We aren't the primary crash CPU. We need to send it
+	 * to a holding pattern to avoid it ending up in the panic
+	 * code.
+	 */
+	crash_kexec_secondary(regs);
+
+	/*
+	 * No debugger or crash dump registered, print logs then
+	 * panic.
+	 */
+	__die("System Reset", regs, SIGABRT);
+
+	mdelay(2*MSEC_PER_SEC); /* Wait a little while for others to print */
+	add_taint(TAINT_DIE, LOCKDEP_NOW_UNRELIABLE);
+	nmi_panic(regs, "System Reset");
 
 out:
 #ifdef CONFIG_PPC_BOOK3S_64
 	BUG_ON(get_paca()->in_nmi == 0);
 	if (get_paca()->in_nmi > 1)
-		panic("Unrecoverable nested System Reset");
+		nmi_panic(regs, "Unrecoverable nested System Reset");
 #endif
 	/* Must die if the interrupt is not recoverable */
 	if (!(regs->msr & MSR_RI))
-		panic("Unrecoverable System Reset");
+		nmi_panic(regs, "Unrecoverable System Reset");
 
 	if (!nested)
 		nmi_exit();
-- 
2.11.0