[PATCH 0/3] sreset debugging improvements

[PATCH 0/3] sreset debugging improvements

[Nicholas Piggin]

Since last time:
- Fixed bug in the fadump patch noticed by Mahesh.
- Removed ppc_md.panic completely.

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

 arch/powerpc/include/asm/machdep.h     |  1 -
 arch/powerpc/include/asm/setup.h       |  1 -
 arch/powerpc/kernel/fadump.c           | 22 ++++++++++++++++
 arch/powerpc/kernel/setup-common.c     | 27 -------------------
 arch/powerpc/kernel/traps.c            | 47 ++++++++++++++++++++++------------
 arch/powerpc/platforms/ps3/setup.c     | 15 -----------
 arch/powerpc/platforms/pseries/ras.c   | 15 +++++++++++
 arch/powerpc/platforms/pseries/setup.c |  1 -
 8 files changed, 68 insertions(+), 61 deletions(-)

-- 
2.11.0

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

[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.

Currently, system reset injection causes the guest to silently
stop.

Stop calling ppc_md.panic in the panic notifier. crash_fadump already
does rtas_os_term() to terminate the guest if fadump is registered.

Remove ppc_md.panic. Move fadump panic notifier into fadump code.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/include/asm/machdep.h     |  1 -
 arch/powerpc/include/asm/setup.h       |  1 -
 arch/powerpc/kernel/fadump.c           | 22 ++++++++++++++++++++++
 arch/powerpc/kernel/setup-common.c     | 27 ---------------------------
 arch/powerpc/platforms/ps3/setup.c     | 15 ---------------
 arch/powerpc/platforms/pseries/setup.c |  1 -
 6 files changed, 22 insertions(+), 45 deletions(-)

diff --git a/arch/powerpc/include/asm/machdep.h b/arch/powerpc/include/asm/machdep.h
index cd2fc1cc1cc7..73b92017b6d7 100644
--- a/arch/powerpc/include/asm/machdep.h
+++ b/arch/powerpc/include/asm/machdep.h
@@ -76,7 +76,6 @@ struct machdep_calls {
 
 	void __noreturn	(*restart)(char *cmd);
 	void __noreturn (*halt)(void);
-	void		(*panic)(char *str);
 	void		(*cpu_die)(void);
 
 	long		(*time_init)(void); /* Optional, may be NULL */
diff --git a/arch/powerpc/include/asm/setup.h b/arch/powerpc/include/asm/setup.h
index 654d64c9f3ac..3a3fb0ca68f5 100644
--- a/arch/powerpc/include/asm/setup.h
+++ b/arch/powerpc/include/asm/setup.h
@@ -23,7 +23,6 @@ extern void reloc_got2(unsigned long);
 
 void check_for_initrd(void);
 void initmem_init(void);
-void setup_panic(void);
 #define ARCH_PANIC_TIMEOUT 180
 
 #ifdef CONFIG_PPC_PSERIES
diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c
index 3079518f2245..8f1a8bd8433c 100644
--- a/arch/powerpc/kernel/fadump.c
+++ b/arch/powerpc/kernel/fadump.c
@@ -1447,6 +1447,25 @@ static void fadump_init_files(void)
 	return;
 }
 
+static int fadump_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. If this returns, then fadump was not registered, so
+	 * go through the rest of the panic path.
+	 */
+	crash_fadump(NULL, ptr);
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block fadump_panic_block = {
+	.notifier_call = fadump_panic_event,
+	.priority = INT_MIN /* may not return; must be done last */
+};
+
 /*
  * Prepare for firmware-assisted dump.
  */
@@ -1479,6 +1498,9 @@ int __init setup_fadump(void)
 		init_fadump_mem_struct(&fdm, fw_dump.reserve_dump_area_start);
 	fadump_init_files();
 
+	atomic_notifier_chain_register(&panic_notifier_list,
+					&fadump_panic_block);
+
 	return 1;
 }
 subsys_initcall(setup_fadump);
diff --git a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c
index 94a948207cd2..b697530d9bdc 100644
--- a/arch/powerpc/kernel/setup-common.c
+++ b/arch/powerpc/kernel/setup-common.c
@@ -704,30 +704,6 @@ int check_legacy_ioport(unsigned long base_port)
 }
 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 */
-	return NOTIFY_DONE;
-}
-
-static struct notifier_block ppc_panic_block = {
-	.notifier_call = ppc_panic_event,
-	.priority = INT_MIN /* may not return; must be done last */
-};
-
-void __init setup_panic(void)
-{
-	if (!ppc_md.panic)
-		return;
-	atomic_notifier_chain_register(&panic_notifier_list, &ppc_panic_block);
-}
-
 #ifdef CONFIG_CHECK_CACHE_COHERENCY
 /*
  * For platforms that have configurable cache-coherency.  This function
@@ -872,9 +848,6 @@ void __init setup_arch(char **cmdline_p)
 	/* Probe the machine type, establish ppc_md. */
 	probe_machine();
 
-	/* Setup panic notifier if requested by the platform. */
-	setup_panic();
-
 	/*
 	 * Configure ppc_md.power_save (ppc32 only, 64-bit machines do
 	 * it from their respective probe() function.
diff --git a/arch/powerpc/platforms/ps3/setup.c b/arch/powerpc/platforms/ps3/setup.c
index 6244bc849469..9dabea6e1443 100644
--- a/arch/powerpc/platforms/ps3/setup.c
+++ b/arch/powerpc/platforms/ps3/setup.c
@@ -104,20 +104,6 @@ static void __noreturn ps3_halt(void)
 	ps3_sys_manager_halt(); /* never returns */
 }
 
-static void ps3_panic(char *str)
-{
-	DBG("%s:%d %s\n", __func__, __LINE__, str);
-
-	smp_send_stop();
-	printk("\n");
-	printk("   System does not reboot automatically.\n");
-	printk("   Please press POWER button.\n");
-	printk("\n");
-
-	while(1)
-		lv1_pause(1);
-}
-
 #if defined(CONFIG_FB_PS3) || defined(CONFIG_FB_PS3_MODULE) || \
     defined(CONFIG_PS3_FLASH) || defined(CONFIG_PS3_FLASH_MODULE)
 static void __init prealloc(struct ps3_prealloc *p)
@@ -269,7 +255,6 @@ define_machine(ps3) {
 	.probe				= ps3_probe,
 	.setup_arch			= ps3_setup_arch,
 	.init_IRQ			= ps3_init_IRQ,
-	.panic				= ps3_panic,
 	.get_boot_time			= ps3_get_boot_time,
 	.set_dabr			= ps3_set_dabr,
 	.calibrate_decr			= ps3_calibrate_decr,
diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c
index b5d86426e97b..b5b650910cf5 100644
--- a/arch/powerpc/platforms/pseries/setup.c
+++ b/arch/powerpc/platforms/pseries/setup.c
@@ -722,7 +722,6 @@ define_machine(pseries) {
 	.pcibios_fixup		= pSeries_final_fixup,
 	.restart		= rtas_restart,
 	.halt			= rtas_halt,
-	.panic			= rtas_os_term,
 	.get_boot_time		= rtas_get_boot_time,
 	.get_rtc_time		= rtas_get_rtc_time,
 	.set_rtc_time		= rtas_set_rtc_time,
-- 
2.11.0

[PATCH v2 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 v2 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

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

[Mahesh Jagannath Salgaonkar]

On 07/05/2017 09:26 AM, 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.
> 
> Currently, system reset injection causes the guest to silently
> stop.
> 
> Stop calling ppc_md.panic in the panic notifier. crash_fadump already
> does rtas_os_term() to terminate the guest if fadump is registered.
> 
> Remove ppc_md.panic. Move fadump panic notifier into fadump code.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

Reviewed-by: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>

Thanks,
-Mahesh.

> ---
>  arch/powerpc/include/asm/machdep.h     |  1 -
>  arch/powerpc/include/asm/setup.h       |  1 -
>  arch/powerpc/kernel/fadump.c           | 22 ++++++++++++++++++++++
>  arch/powerpc/kernel/setup-common.c     | 27 ---------------------------
>  arch/powerpc/platforms/ps3/setup.c     | 15 ---------------
>  arch/powerpc/platforms/pseries/setup.c |  1 -
>  6 files changed, 22 insertions(+), 45 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/machdep.h b/arch/powerpc/include/asm/machdep.h
> index cd2fc1cc1cc7..73b92017b6d7 100644
> --- a/arch/powerpc/include/asm/machdep.h
> +++ b/arch/powerpc/include/asm/machdep.h
> @@ -76,7 +76,6 @@ struct machdep_calls {
> 
>  	void __noreturn	(*restart)(char *cmd);
>  	void __noreturn (*halt)(void);
> -	void		(*panic)(char *str);
>  	void		(*cpu_die)(void);
> 
>  	long		(*time_init)(void); /* Optional, may be NULL */
> diff --git a/arch/powerpc/include/asm/setup.h b/arch/powerpc/include/asm/setup.h
> index 654d64c9f3ac..3a3fb0ca68f5 100644
> --- a/arch/powerpc/include/asm/setup.h
> +++ b/arch/powerpc/include/asm/setup.h
> @@ -23,7 +23,6 @@ extern void reloc_got2(unsigned long);
> 
>  void check_for_initrd(void);
>  void initmem_init(void);
> -void setup_panic(void);
>  #define ARCH_PANIC_TIMEOUT 180
> 
>  #ifdef CONFIG_PPC_PSERIES
> diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c
> index 3079518f2245..8f1a8bd8433c 100644
> --- a/arch/powerpc/kernel/fadump.c
> +++ b/arch/powerpc/kernel/fadump.c
> @@ -1447,6 +1447,25 @@ static void fadump_init_files(void)
>  	return;
>  }
> 
> +static int fadump_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. If this returns, then fadump was not registered, so
> +	 * go through the rest of the panic path.
> +	 */
> +	crash_fadump(NULL, ptr);
> +
> +	return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block fadump_panic_block = {
> +	.notifier_call = fadump_panic_event,
> +	.priority = INT_MIN /* may not return; must be done last */
> +};
> +
>  /*
>   * Prepare for firmware-assisted dump.
>   */
> @@ -1479,6 +1498,9 @@ int __init setup_fadump(void)
>  		init_fadump_mem_struct(&fdm, fw_dump.reserve_dump_area_start);
>  	fadump_init_files();
> 
> +	atomic_notifier_chain_register(&panic_notifier_list,
> +					&fadump_panic_block);
> +
>  	return 1;
>  }
>  subsys_initcall(setup_fadump);
> diff --git a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c
> index 94a948207cd2..b697530d9bdc 100644
> --- a/arch/powerpc/kernel/setup-common.c
> +++ b/arch/powerpc/kernel/setup-common.c
> @@ -704,30 +704,6 @@ int check_legacy_ioport(unsigned long base_port)
>  }
>  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 */
> -	return NOTIFY_DONE;
> -}
> -
> -static struct notifier_block ppc_panic_block = {
> -	.notifier_call = ppc_panic_event,
> -	.priority = INT_MIN /* may not return; must be done last */
> -};
> -
> -void __init setup_panic(void)
> -{
> -	if (!ppc_md.panic)
> -		return;
> -	atomic_notifier_chain_register(&panic_notifier_list, &ppc_panic_block);
> -}
> -
>  #ifdef CONFIG_CHECK_CACHE_COHERENCY
>  /*
>   * For platforms that have configurable cache-coherency.  This function
> @@ -872,9 +848,6 @@ void __init setup_arch(char **cmdline_p)
>  	/* Probe the machine type, establish ppc_md. */
>  	probe_machine();
> 
> -	/* Setup panic notifier if requested by the platform. */
> -	setup_panic();
> -
>  	/*
>  	 * Configure ppc_md.power_save (ppc32 only, 64-bit machines do
>  	 * it from their respective probe() function.
> diff --git a/arch/powerpc/platforms/ps3/setup.c b/arch/powerpc/platforms/ps3/setup.c
> index 6244bc849469..9dabea6e1443 100644
> --- a/arch/powerpc/platforms/ps3/setup.c
> +++ b/arch/powerpc/platforms/ps3/setup.c
> @@ -104,20 +104,6 @@ static void __noreturn ps3_halt(void)
>  	ps3_sys_manager_halt(); /* never returns */
>  }
> 
> -static void ps3_panic(char *str)
> -{
> -	DBG("%s:%d %s\n", __func__, __LINE__, str);
> -
> -	smp_send_stop();
> -	printk("\n");
> -	printk("   System does not reboot automatically.\n");
> -	printk("   Please press POWER button.\n");
> -	printk("\n");
> -
> -	while(1)
> -		lv1_pause(1);
> -}
> -
>  #if defined(CONFIG_FB_PS3) || defined(CONFIG_FB_PS3_MODULE) || \
>      defined(CONFIG_PS3_FLASH) || defined(CONFIG_PS3_FLASH_MODULE)
>  static void __init prealloc(struct ps3_prealloc *p)
> @@ -269,7 +255,6 @@ define_machine(ps3) {
>  	.probe				= ps3_probe,
>  	.setup_arch			= ps3_setup_arch,
>  	.init_IRQ			= ps3_init_IRQ,
> -	.panic				= ps3_panic,
>  	.get_boot_time			= ps3_get_boot_time,
>  	.set_dabr			= ps3_set_dabr,
>  	.calibrate_decr			= ps3_calibrate_decr,
> diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c
> index b5d86426e97b..b5b650910cf5 100644
> --- a/arch/powerpc/platforms/pseries/setup.c
> +++ b/arch/powerpc/platforms/pseries/setup.c
> @@ -722,7 +722,6 @@ define_machine(pseries) {
>  	.pcibios_fixup		= pSeries_final_fixup,
>  	.restart		= rtas_restart,
>  	.halt			= rtas_halt,
> -	.panic			= rtas_os_term,
>  	.get_boot_time		= rtas_get_boot_time,
>  	.get_rtc_time		= rtas_get_rtc_time,
>  	.set_rtc_time		= rtas_set_rtc_time,
> 

Re: [v2, 1/3] powerpc: do not call ppc_md.panic in fadump panic notifier

[Michael Ellerman]

On Wed, 2017-07-05 at 03:56:25 UTC, 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.
> 
> Currently, system reset injection causes the guest to silently
> stop.
> 
> Stop calling ppc_md.panic in the panic notifier. crash_fadump already
> does rtas_os_term() to terminate the guest if fadump is registered.
> 
> Remove ppc_md.panic. Move fadump panic notifier into fadump code.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> Reviewed-by: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>

Series applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/a3b2cb30f252b21a6f962e0dd107c8

cheers