[patch 00/10] Oops path and debugger hooks rework

[patch 00/10] Oops path and debugger hooks rework

[anton]

The following series cleans up our oops printout code and adds some missing
functionality. It also changes xmon over to use the notify_die hooks and
removes the awful debugger* hooks.

[patch 01/10] Add missing oops_enter/oops_exit

[anton]

Add missing oops_enter/oops_exit, makes pause_on_oops boot parameter work.

Signed-off-by: Anton Blanchard <anton@samba.org>
---

Index: linux-2.6/arch/powerpc/kernel/traps.c
===================================================================
--- linux-2.6.orig/arch/powerpc/kernel/traps.c	2007-03-11 10:23:04.000000000 -0500
+++ linux-2.6/arch/powerpc/kernel/traps.c	2007-03-11 10:25:31.000000000 -0500
@@ -99,6 +99,8 @@
 	if (debugger(regs))
 		return 1;
 
+	oops_enter();
+
 	console_verbose();
 	spin_lock_irq(&die_lock);
 	bust_spinlocks(1);
@@ -145,6 +147,7 @@
 	if (panic_on_oops)
 		panic("Fatal exception");
 
+	oops_exit();
 	do_exit(err);
 
 	return 0;

--

[patch 02/10] Clean up pmac_backlight_unblank in oops path

[anton]

Move pmac_backlight_unblank into its own function and only take the
pmac_backlight_mutex when we are on a pmac for that added bit of 
paranoia.

Signed-off-by: Anton Blanchard <anton@samba.org>
---

Index: linux-2.6/arch/powerpc/kernel/traps.c
===================================================================
--- linux-2.6.orig/arch/powerpc/kernel/traps.c	2007-03-11 10:31:29.000000000 -0500
+++ linux-2.6/arch/powerpc/kernel/traps.c	2007-03-11 10:34:44.000000000 -0500
@@ -90,6 +90,24 @@
  * Trap & Exception support
  */
 
+#ifdef CONFIG_PMAC_BACKLIGHT
+static void pmac_backlight_unblank(void)
+{
+	mutex_lock(&pmac_backlight_mutex);
+	if (pmac_backlight) {
+		struct backlight_properties *props;
+
+		props = &pmac_backlight->props;
+		props->brightness = props->max_brightness;
+		props->power = FB_BLANK_UNBLANK;
+		backlight_update_status(pmac_backlight);
+	}
+	mutex_unlock(&pmac_backlight_mutex);
+}
+#else
+static inline void pmac_backlight_unblank(void) { }
+#endif
+
 static DEFINE_SPINLOCK(die_lock);
 
 int die(const char *str, struct pt_regs *regs, long err)
@@ -104,18 +122,9 @@
 	console_verbose();
 	spin_lock_irq(&die_lock);
 	bust_spinlocks(1);
-#ifdef CONFIG_PMAC_BACKLIGHT
-	mutex_lock(&pmac_backlight_mutex);
-	if (machine_is(powermac) && pmac_backlight) {
-		struct backlight_properties *props;
+	if (machine_is(powermac))
+		pmac_backlight_unblank();
 
-		props = &pmac_backlight->props;
-		props->brightness = props->max_brightness;
-		props->power = FB_BLANK_UNBLANK;
-		backlight_update_status(pmac_backlight);
-	}
-	mutex_unlock(&pmac_backlight_mutex);
-#endif
 	printk("Oops: %s, sig: %ld [#%d]\n", str, err, ++die_counter);
 #ifdef CONFIG_PREEMPT
 	printk("PREEMPT ");

--

[patch 03/10] Handle recursive oopses

[anton]

Handle recursive oopses, like on x86. We had a few cases recently where 
we locked up in oops printing and didnt make it into crashdump.

Signed-off-by: Anton Blanchard <anton@samba.org>
---

Index: linux-2.6/arch/powerpc/kernel/traps.c
===================================================================
--- linux-2.6.orig/arch/powerpc/kernel/traps.c	2007-03-11 10:34:44.000000000 -0500
+++ linux-2.6/arch/powerpc/kernel/traps.c	2007-03-11 11:03:11.000000000 -0500
@@ -108,42 +108,62 @@
 static inline void pmac_backlight_unblank(void) { }
 #endif
 
-static DEFINE_SPINLOCK(die_lock);
-
 int die(const char *str, struct pt_regs *regs, long err)
 {
+	static struct {
+		spinlock_t lock;
+		u32 lock_owner;
+		int lock_owner_depth;
+	} die = {
+		.lock =			__SPIN_LOCK_UNLOCKED(die.lock),
+		.lock_owner =		-1,
+		.lock_owner_depth =	0
+	};
 	static int die_counter;
+	unsigned long flags;
 
 	if (debugger(regs))
 		return 1;
 
 	oops_enter();
 
-	console_verbose();
-	spin_lock_irq(&die_lock);
-	bust_spinlocks(1);
-	if (machine_is(powermac))
-		pmac_backlight_unblank();
+	if (die.lock_owner != raw_smp_processor_id()) {
+		console_verbose();
+		spin_lock_irqsave(&die.lock, flags);
+		die.lock_owner = smp_processor_id();
+		die.lock_owner_depth = 0;
+		bust_spinlocks(1);
+		if (machine_is(powermac))
+			pmac_backlight_unblank();
+	} else {
+		local_save_flags(flags);
+	}
 
-	printk("Oops: %s, sig: %ld [#%d]\n", str, err, ++die_counter);
+	if (++die.lock_owner_depth < 3) {
+		printk("Oops: %s, sig: %ld [#%d]\n", str, err, ++die_counter);
 #ifdef CONFIG_PREEMPT
-	printk("PREEMPT ");
+		printk("PREEMPT ");
 #endif
 #ifdef CONFIG_SMP
-	printk("SMP NR_CPUS=%d ", NR_CPUS);
+		printk("SMP NR_CPUS=%d ", NR_CPUS);
 #endif
 #ifdef CONFIG_DEBUG_PAGEALLOC
-	printk("DEBUG_PAGEALLOC ");
+		printk("DEBUG_PAGEALLOC ");
 #endif
 #ifdef CONFIG_NUMA
-	printk("NUMA ");
+		printk("NUMA ");
 #endif
-	printk("%s\n", ppc_md.name ? "" : ppc_md.name);
+		printk("%s\n", ppc_md.name ? "" : ppc_md.name);
+
+		print_modules();
+		show_regs(regs);
+	} else {
+		printk("Recursive die() failure, output suppressed\n");
+	}
 
-	print_modules();
-	show_regs(regs);
 	bust_spinlocks(0);
-	spin_unlock_irq(&die_lock);
+	die.lock_owner = -1;
+	spin_unlock_irqrestore(&die.lock, flags);
 
 	if (kexec_should_crash(current) ||
 		kexec_sr_activated(smp_processor_id()))

--

[patch 04/10] Fix backwards ? : when printing machine type

[anton]

Looks like someone got this backwards, highlighting the perils of the
? : !!! :)

Signed-off-by: Anton Blanchard <anton@samba.org>
---

Index: linux-2.6/arch/powerpc/kernel/traps.c
===================================================================
--- linux-2.6.orig/arch/powerpc/kernel/traps.c	2007-03-11 11:03:11.000000000 -0500
+++ linux-2.6/arch/powerpc/kernel/traps.c	2007-03-11 11:03:20.000000000 -0500
@@ -153,7 +153,7 @@
 #ifdef CONFIG_NUMA
 		printk("NUMA ");
 #endif
-		printk("%s\n", ppc_md.name ? "" : ppc_md.name);
+		printk("%s\n", ppc_md.name ? ppc_md.name : "");
 
 		print_modules();
 		show_regs(regs);

--

[patch 05/10] Use KERN_EMERG everywhere in oops printout

[anton]

Turn the volume up to 11. Distros will silence our oops messages no more.
Use of sunglasses recommended.

Signed-off-by: Anton Blanchard <anton@samba.org>
---

Index: linux-2.6/arch/powerpc/kernel/traps.c
===================================================================
--- linux-2.6.orig/arch/powerpc/kernel/traps.c	2007-03-20 09:53:57.000000000 -0500
+++ linux-2.6/arch/powerpc/kernel/traps.c	2007-03-20 09:53:58.000000000 -0500
@@ -140,7 +140,10 @@
 	}
 
 	if (++die.lock_owner_depth < 3) {
-		printk("Oops: %s, sig: %ld [#%d]\n", str, err, ++die_counter);
+		printk(KERN_EMERG "Oops: %s, sig: %ld [#%d]\n", str, err,
+		       ++die_counter);
+
+		printk(KERN_EMERG);
 #ifdef CONFIG_PREEMPT
 		printk("PREEMPT ");
 #endif
@@ -155,10 +158,11 @@
 #endif
 		printk("%s\n", ppc_md.name ? ppc_md.name : "");
 
+		printk(KERN_EMERG);
 		print_modules();
-		show_regs(regs);
+		show_regs_log_lvl(regs, KERN_EMERG);
 	} else {
-		printk("Recursive die() failure, output suppressed\n");
+		printk(KERN_EMERG "Recursive die() failure, output suppressed\n");
 	}
 
 	bust_spinlocks(0);
Index: linux-2.6/arch/powerpc/kernel/process.c
===================================================================
--- linux-2.6.orig/arch/powerpc/kernel/process.c	2007-03-20 09:53:47.000000000 -0500
+++ linux-2.6/arch/powerpc/kernel/process.c	2007-03-20 09:53:58.000000000 -0500
@@ -49,8 +49,6 @@
 #include <asm/firmware.h>
 #endif
 
-extern unsigned long _get_SP(void);
-
 #ifndef CONFIG_SMP
 struct task_struct *last_task_used_math = NULL;
 struct task_struct *last_task_used_altivec = NULL;
@@ -347,13 +345,13 @@
 	unsigned long pc = regs->nip - (instructions_to_print * 3 / 4 *
 			sizeof(int));
 
-	printk("Instruction dump:");
+	printk(KERN_EMERG "Instruction dump:");
 
 	for (i = 0; i < instructions_to_print; i++) {
 		int instr;
 
 		if (!(i % 8))
-			printk("\n");
+			printk("\n%s", KERN_EMERG);
 
 		/* We use __get_user here *only* to avoid an OOPS on a
 		 * bad address because the pc *should* only be a
@@ -411,21 +409,25 @@
 #define LAST_VOLATILE	12
 #endif
 
-void show_regs(struct pt_regs * regs)
+static void show_stack_log_lvl(struct task_struct *tsk, unsigned long *stack,
+                               char *log_lvl);
+
+void show_regs_log_lvl(struct pt_regs *regs, char *log_lvl)
 {
 	int i, trap;
 
-	printk("NIP: "REG" LR: "REG" CTR: "REG"\n",
-	       regs->nip, regs->link, regs->ctr);
-	printk("REGS: %p TRAP: %04lx   %s  (%s)\n",
+	printk("%sNIP: "REG" LR: "REG" CTR: "REG"\n",
+	       log_lvl, regs->nip, regs->link, regs->ctr);
+	printk("%sREGS: %p TRAP: %04lx   %s  (%s)\n", log_lvl,
 	       regs, regs->trap, print_tainted(), init_utsname()->release);
-	printk("MSR: "REG" ", regs->msr);
+	printk("%sMSR: "REG" ", log_lvl, regs->msr);
 	printbits(regs->msr, msr_bits);
 	printk("  CR: %08lX  XER: %08lX\n", regs->ccr, regs->xer);
 	trap = TRAP(regs);
 	if (trap == 0x300 || trap == 0x600)
-		printk("DAR: "REG", DSISR: "REG"\n", regs->dar, regs->dsisr);
-	printk("TASK = %p[%d] '%s' THREAD: %p",
+		printk("%sDAR: "REG", DSISR: "REG"\n", log_lvl, regs->dar,
+		       regs->dsisr);
+	printk("%sTASK = %p[%d] '%s' THREAD: %p", log_lvl,
 	       current, current->pid, current->comm, task_thread_info(current));
 
 #ifdef CONFIG_SMP
@@ -434,7 +436,7 @@
 
 	for (i = 0;  i < 32;  i++) {
 		if ((i % REGS_PER_LINE) == 0)
-			printk("\n" KERN_INFO "GPR%02d: ", i);
+			printk("\n%sGPR%02d: ", log_lvl, i);
 		printk(REG " ", regs->gpr[i]);
 		if (i == LAST_VOLATILE && !FULL_REGS(regs))
 			break;
@@ -445,16 +447,21 @@
 	 * Lookup NIP late so we have the best change of getting the
 	 * above info out without failing
 	 */
-	printk("NIP ["REG"] ", regs->nip);
+	printk("%sNIP ["REG"] ", log_lvl, regs->nip);
 	print_symbol("%s\n", regs->nip);
-	printk("LR ["REG"] ", regs->link);
+	printk("%sLR ["REG"] ", log_lvl, regs->link);
 	print_symbol("%s\n", regs->link);
 #endif
-	show_stack(current, (unsigned long *) regs->gpr[1]);
+	show_stack_log_lvl(current, (unsigned long *) regs->gpr[1], KERN_EMERG);
 	if (!user_mode(regs))
 		show_instructions(regs);
 }
 
+void show_regs(struct pt_regs *regs)
+{
+	show_regs_log_lvl(regs, "");
+}
+
 void exit_thread(void)
 {
 	discard_lazy_cpu_state();
@@ -884,7 +891,8 @@
 
 static int kstack_depth_to_print = 64;
 
-void show_stack(struct task_struct *tsk, unsigned long *stack)
+static void show_stack_log_lvl(struct task_struct *tsk, unsigned long *stack,
+			       char *log_lvl)
 {
 	unsigned long sp, ip, lr, newsp;
 	int count = 0;
@@ -901,7 +909,7 @@
 	}
 
 	lr = 0;
-	printk("Call Trace:\n");
+	printk("%sCall Trace:\n", log_lvl);
 	do {
 		if (!validate_sp(sp, tsk, MIN_STACK_FRAME))
 			return;
@@ -910,7 +918,7 @@
 		newsp = stack[0];
 		ip = stack[FRAME_LR_SAVE];
 		if (!firstframe || ip != lr) {
-			printk("["REG"] ["REG"] ", sp, ip);
+			printk("%s["REG"] ["REG"] ", log_lvl, sp, ip);
 			print_symbol("%s", ip);
 			if (firstframe)
 				printk(" (unreliable)");
@@ -926,9 +934,10 @@
 		    && stack[FRAME_MARKER] == REGS_MARKER) {
 			struct pt_regs *regs = (struct pt_regs *)
 				(sp + STACK_FRAME_OVERHEAD);
-			printk("--- Exception: %lx", regs->trap);
+			printk("%s--- Exception: %lx", log_lvl, regs->trap);
 			print_symbol(" at %s\n", regs->nip);
 			lr = regs->link;
+			printk(KERN_EMERG);
 			print_symbol("    LR = %s\n", lr);
 			firstframe = 1;
 		}
@@ -937,6 +946,11 @@
 	} while (count++ < kstack_depth_to_print);
 }
 
+void show_stack(struct task_struct *tsk, unsigned long *stack)
+{
+	show_stack_log_lvl(tsk, stack, "");
+}
+
 void dump_stack(void)
 {
 	show_stack(current, NULL);
Index: linux-2.6/include/asm-powerpc/system.h
===================================================================
--- linux-2.6.orig/include/asm-powerpc/system.h	2007-03-20 09:53:19.000000000 -0500
+++ linux-2.6/include/asm-powerpc/system.h	2007-03-20 09:53:58.000000000 -0500
@@ -103,6 +103,7 @@
 
 extern int set_dabr(unsigned long dabr);
 extern void print_backtrace(unsigned long *);
+extern void show_regs_log_lvl(struct pt_regs * regs, char * log_lvl);
 extern void show_regs(struct pt_regs * regs);
 extern void flush_instruction_cache(void);
 extern void hard_reset_now(void);
Index: linux-2.6/arch/powerpc/mm/fault.c
===================================================================
--- linux-2.6.orig/arch/powerpc/mm/fault.c	2007-03-20 09:53:19.000000000 -0500
+++ linux-2.6/arch/powerpc/mm/fault.c	2007-03-20 09:53:58.000000000 -0500
@@ -429,20 +429,20 @@
 	switch (regs->trap) {
 	case 0x300:
 	case 0x380:
-		printk(KERN_ALERT "Unable to handle kernel paging request for "
+		printk(KERN_EMERG "Unable to handle kernel paging request for "
 			"data at address 0x%08lx\n", regs->dar);
 		break;
 	case 0x400:
 	case 0x480:
-		printk(KERN_ALERT "Unable to handle kernel paging request for "
+		printk(KERN_EMERG "Unable to handle kernel paging request for "
 			"instruction fetch\n");
 		break;
 	default:
-		printk(KERN_ALERT "Unable to handle kernel paging request for "
+		printk(KERN_EMERG "Unable to handle kernel paging request for "
 			"unknown fault\n");
 		break;
 	}
-	printk(KERN_ALERT "Faulting instruction address: 0x%08lx\n",
+	printk(KERN_EMERG "Faulting instruction address: 0x%08lx\n",
 		regs->nip);
 
 	die("Kernel access of bad area", regs, sig);

--

[patch 06/10] Add notify die hooks and remove some redundant debugger hooks

[anton]

Add a DIE_OOPS and DIE_MACHINE_CHECK notify_die hook and remove some
redundant debugger* hooks.

Signed-off-by: Anton Blanchard <anton@samba.org>
---

Index: linux-2.6/arch/powerpc/kernel/traps.c
===================================================================
--- linux-2.6.orig/arch/powerpc/kernel/traps.c	2007-03-11 12:11:13.000000000 -0500
+++ linux-2.6/arch/powerpc/kernel/traps.c	2007-03-11 12:40:41.000000000 -0500
@@ -122,6 +122,10 @@
 	static int die_counter;
 	unsigned long flags;
 
+	if (notify_die(DIE_OOPS, str, regs, err, regs->trap, SIGSEGV) ==
+		       NOTIFY_STOP)
+		return 1;
+
 	if (debugger(regs))
 		return 1;
 
@@ -374,6 +378,11 @@
 	return;
 #endif
 
+        if (notify_die(DIE_MACHINE_CHECK, "machine_check", regs, 7, 7,
+		       SIGBUS) == NOTIFY_STOP) {
+		return;
+	}
+
 	if (debugger_fault_handler(regs)) {
 		regs->msr |= MSR_RI;
 		return;
@@ -505,8 +514,6 @@
 	 */
 	platform_machine_check(regs);
 
-	if (debugger_fault_handler(regs))
-		return;
 	die("Machine check", regs, SIGBUS);
 
 	/* Must die if the interrupt is not recoverable */
@@ -890,7 +897,6 @@
 {
 	printk(KERN_ERR "Non-recoverable exception at PC=%lx MSR=%lx\n",
 	       regs->nip, regs->msr);
-	debugger(regs);
 	die("nonrecoverable exception", regs, SIGKILL);
 }
 
@@ -936,10 +942,8 @@
 
 	CHECK_FULL_REGS(regs);
 
-	if (!user_mode(regs)) {
-		debugger(regs);
+	if (!user_mode(regs))
 		die("Kernel Mode Software FPU Emulation", regs, SIGFPE);
-	}
 
 #ifdef CONFIG_MATH_EMULATION
 	errcode = do_mathemu(regs);
@@ -992,6 +996,9 @@
 			mtspr(SPRN_DBCR0, mfspr(SPRN_DBCR0) & ~DBCR0_IC);
 			/* Clear the instruction completion event */
 			mtspr(SPRN_DBSR, DBSR_IC);
+			if (notify_die(DIE_SSTEP, "single_step", regs, 5, 5,
+				       SIGTRAP) == NOTIFY_STOP)
+				return;
 			if (debugger_sstep(regs))
 				return;
 		}
Index: linux-2.6/include/asm-powerpc/kdebug.h
===================================================================
--- linux-2.6.orig/include/asm-powerpc/kdebug.h	2007-03-11 12:36:52.000000000 -0500
+++ linux-2.6/include/asm-powerpc/kdebug.h	2007-03-11 12:37:05.000000000 -0500
@@ -30,6 +30,7 @@
 	DIE_BPT,
 	DIE_SSTEP,
 	DIE_PAGE_FAULT,
+	DIE_MACHINE_CHECK,
 };
 
 static inline int notify_die(enum die_val val,char *str,struct pt_regs *regs,long err,int trap, int sig)

--

[patch 07/10] Page fault handler should not depend on CONFIG_KPROBES

[anton]

Like x86-64, change the page fault notifier code to not depend on
CONFIG_KPROBES and export the handlers.

Signed-off-by: Anton Blanchard <anton@samba.org>
---

Index: linux-2.6/arch/powerpc/mm/fault.c
===================================================================
--- linux-2.6.orig/arch/powerpc/mm/fault.c	2007-03-11 12:34:34.000000000 -0500
+++ linux-2.6/arch/powerpc/mm/fault.c	2007-03-11 12:40:52.000000000 -0500
@@ -39,7 +39,6 @@
 #include <asm/kdebug.h>
 #include <asm/siginfo.h>
 
-#ifdef CONFIG_KPROBES
 ATOMIC_NOTIFIER_HEAD(notify_page_fault_chain);
 
 /* Hook to register for page fault notifications */
@@ -47,11 +46,13 @@
 {
 	return atomic_notifier_chain_register(&notify_page_fault_chain, nb);
 }
+EXPORT_SYMBOL_GPL(register_page_fault_notifier);
 
 int unregister_page_fault_notifier(struct notifier_block *nb)
 {
 	return atomic_notifier_chain_unregister(&notify_page_fault_chain, nb);
 }
+EXPORT_SYMBOL_GPL(unregister_page_fault_notifier);
 
 static inline int notify_page_fault(enum die_val val, const char *str,
 			struct pt_regs *regs, long err, int trap, int sig)
@@ -65,13 +66,6 @@
 	};
 	return atomic_notifier_call_chain(&notify_page_fault_chain, val, &args);
 }
-#else
-static inline int notify_page_fault(enum die_val val, const char *str,
-			struct pt_regs *regs, long err, int trap, int sig)
-{
-	return NOTIFY_DONE;
-}
-#endif
 
 /*
  * Check whether the instruction at regs->nip is a store using
Index: linux-2.6/include/asm-powerpc/kdebug.h
===================================================================
--- linux-2.6.orig/include/asm-powerpc/kdebug.h	2007-03-11 12:42:23.000000000 -0500
+++ linux-2.6/include/asm-powerpc/kdebug.h	2007-03-11 12:42:49.000000000 -0500
@@ -33,7 +33,8 @@
 	DIE_MACHINE_CHECK,
 };
 
-static inline int notify_die(enum die_val val,char *str,struct pt_regs *regs,long err,int trap, int sig)
+static inline int notify_die(enum die_val val, const char *str,
+			     struct pt_regs *regs, long err, int trap, int sig)
 {
 	struct die_args args = { .regs=regs, .str=str, .err=err, .trapnr=trap,.signr=sig };
 	return atomic_notifier_call_chain(&powerpc_die_chain, val, &args);

--

[patch 08/10] Use notifier hooks for xmon

[anton]

Use notifier hooks for xmon and remove all the debugger* junk. Based on
a patch by Ananth N Mavinakayanahalli <ananth@in.ibm.com>

Signed-off-by: Anton Blanchard <anton@samba.org>
---

Index: linux-2.6/arch/powerpc/xmon/xmon.c
===================================================================
--- linux-2.6.orig/arch/powerpc/xmon/xmon.c	2007-03-20 09:53:45.000000000 -0500
+++ linux-2.6/arch/powerpc/xmon/xmon.c	2007-03-20 09:54:03.000000000 -0500
@@ -40,6 +40,7 @@
 #include <asm/spu.h>
 #include <asm/spu_priv1.h>
 #include <asm/firmware.h>
+#include <asm/kdebug.h>
 
 #ifdef CONFIG_PPC64
 #include <asm/hvcall.h>
@@ -2578,38 +2579,91 @@
 }
 #endif
 
-void xmon_init(int enable)
+static int xmon_exceptions_notify(struct notifier_block *self,
+				  unsigned long val, void *data)
+{
+	struct die_args *args = (struct die_args *)data;
+
+	switch (val) {
+		case DIE_OOPS:
+		case DIE_DEBUGGER:
+			if (xmon(args->regs))
+				return NOTIFY_STOP;
+			break;
+		case DIE_IABR_MATCH:
+			if (xmon_iabr_match(args->regs))
+				return NOTIFY_STOP;
+			break;
+		case DIE_DABR_MATCH:
+			if (xmon_dabr_match(args->regs))
+				return NOTIFY_STOP;
+			break;
+		case DIE_BPT:
+			if (xmon_bpt(args->regs))
+				return NOTIFY_STOP;
+			break;
+		case DIE_SSTEP:
+			if (xmon_sstep(args->regs))
+				return NOTIFY_STOP;
+			break;
+		case DIE_PAGE_FAULT:
+			if ((TRAP(args->regs) == 0x300 ||
+			    TRAP(args->regs) == 0x400) &&
+			    xmon_fault_handler(args->regs))
+				return NOTIFY_STOP;
+			break;
+		case DIE_MACHINE_CHECK:
+			if (xmon_fault_handler(args->regs)) {
+				/* XXX doesnt look safe to me */
+				args->regs->msr |= MSR_RI;
+				return NOTIFY_STOP;
+			}
+			break;
+		case DIE_IPI:
+			if (!xmon_ipi(args->regs))
+				return NOTIFY_STOP;
+			break;
+		default:
+			break;
+	}
+
+	return NOTIFY_DONE;
+}
+
+/* XXX What priority should we set? */
+static struct notifier_block xmon_exceptions_nb = {
+	.notifier_call = xmon_exceptions_notify,
+};
+
+static struct notifier_block xmon_page_fault_nb = {
+	.notifier_call = xmon_exceptions_notify,
+};
+
+void xmon_enable(void)
 {
-#ifdef CONFIG_PPC_ISERIES
 	if (firmware_has_feature(FW_FEATURE_ISERIES))
 		return;
-#endif
-	if (enable) {
-		__debugger = xmon;
-		__debugger_ipi = xmon_ipi;
-		__debugger_bpt = xmon_bpt;
-		__debugger_sstep = xmon_sstep;
-		__debugger_iabr_match = xmon_iabr_match;
-		__debugger_dabr_match = xmon_dabr_match;
-		__debugger_fault_handler = xmon_fault_handler;
-	} else {
-		__debugger = NULL;
-		__debugger_ipi = NULL;
-		__debugger_bpt = NULL;
-		__debugger_sstep = NULL;
-		__debugger_iabr_match = NULL;
-		__debugger_dabr_match = NULL;
-		__debugger_fault_handler = NULL;
-	}
+
+	register_die_notifier(&xmon_exceptions_nb);
+	register_page_fault_notifier(&xmon_page_fault_nb);
 	xmon_map_scc();
 }
 
+void xmon_disable(void)
+{
+	if (firmware_has_feature(FW_FEATURE_ISERIES))
+		return;
+
+	unregister_die_notifier(&xmon_exceptions_nb);
+	unregister_page_fault_notifier(&xmon_page_fault_nb);
+}
+
 #ifdef CONFIG_MAGIC_SYSRQ
 static void sysrq_handle_xmon(int key, struct tty_struct *tty) 
 {
 	/* ensure xmon is enabled */
-	xmon_init(1);
-	debugger(get_irq_regs());
+	xmon_enable();
+	xmon(get_irq_regs());
 }
 
 static struct sysrq_key_op sysrq_xmon_op = 
@@ -2621,10 +2675,8 @@
 
 static int __init setup_xmon_sysrq(void)
 {
-#ifdef CONFIG_PPC_ISERIES
 	if (firmware_has_feature(FW_FEATURE_ISERIES))
 		return 0;
-#endif
 	register_sysrq_key('x', &sysrq_xmon_op);
 	return 0;
 }
@@ -2637,10 +2689,10 @@
 {
 	if (!p || strncmp(p, "early", 5) == 0) {
 		/* just "xmon" is equivalent to "xmon=early" */
-		xmon_init(1);
+		xmon_enable();
 		xmon_early = 1;
 	} else if (strncmp(p, "on", 2) == 0)
-		xmon_init(1);
+		xmon_enable();
 	else if (strncmp(p, "off", 3) == 0)
 		xmon_off = 1;
 	else if (strncmp(p, "nobt", 4) == 0)
@@ -2656,10 +2708,10 @@
 {
 #ifdef CONFIG_XMON_DEFAULT
 	if (!xmon_off)
-		xmon_init(1);
+		xmon_enable();
 #endif
 	if (xmon_early)
-		debugger(NULL);
+		xmon(NULL);
 }
 
 #ifdef CONFIG_SPU_BASE
Index: linux-2.6/include/asm-powerpc/system.h
===================================================================
--- linux-2.6.orig/include/asm-powerpc/system.h	2007-03-20 09:53:58.000000000 -0500
+++ linux-2.6/include/asm-powerpc/system.h	2007-03-20 09:54:03.000000000 -0500
@@ -65,42 +65,6 @@
 struct task_struct;
 struct pt_regs;
 
-#ifdef CONFIG_DEBUGGER
-
-extern int (*__debugger)(struct pt_regs *regs);
-extern int (*__debugger_ipi)(struct pt_regs *regs);
-extern int (*__debugger_bpt)(struct pt_regs *regs);
-extern int (*__debugger_sstep)(struct pt_regs *regs);
-extern int (*__debugger_iabr_match)(struct pt_regs *regs);
-extern int (*__debugger_dabr_match)(struct pt_regs *regs);
-extern int (*__debugger_fault_handler)(struct pt_regs *regs);
-
-#define DEBUGGER_BOILERPLATE(__NAME) \
-static inline int __NAME(struct pt_regs *regs) \
-{ \
-	if (unlikely(__ ## __NAME)) \
-		return __ ## __NAME(regs); \
-	return 0; \
-}
-
-DEBUGGER_BOILERPLATE(debugger)
-DEBUGGER_BOILERPLATE(debugger_ipi)
-DEBUGGER_BOILERPLATE(debugger_bpt)
-DEBUGGER_BOILERPLATE(debugger_sstep)
-DEBUGGER_BOILERPLATE(debugger_iabr_match)
-DEBUGGER_BOILERPLATE(debugger_dabr_match)
-DEBUGGER_BOILERPLATE(debugger_fault_handler)
-
-#else
-static inline int debugger(struct pt_regs *regs) { return 0; }
-static inline int debugger_ipi(struct pt_regs *regs) { return 0; }
-static inline int debugger_bpt(struct pt_regs *regs) { return 0; }
-static inline int debugger_sstep(struct pt_regs *regs) { return 0; }
-static inline int debugger_iabr_match(struct pt_regs *regs) { return 0; }
-static inline int debugger_dabr_match(struct pt_regs *regs) { return 0; }
-static inline int debugger_fault_handler(struct pt_regs *regs) { return 0; }
-#endif
-
 extern int set_dabr(unsigned long dabr);
 extern void print_backtrace(unsigned long *);
 extern void show_regs_log_lvl(struct pt_regs * regs, char * log_lvl);
Index: linux-2.6/arch/powerpc/kernel/traps.c
===================================================================
--- linux-2.6.orig/arch/powerpc/kernel/traps.c	2007-03-20 09:54:01.000000000 -0500
+++ linux-2.6/arch/powerpc/kernel/traps.c	2007-03-20 09:54:03.000000000 -0500
@@ -54,24 +54,6 @@
 #endif
 #include <asm/kexec.h>
 
-#ifdef CONFIG_DEBUGGER
-int (*__debugger)(struct pt_regs *regs);
-int (*__debugger_ipi)(struct pt_regs *regs);
-int (*__debugger_bpt)(struct pt_regs *regs);
-int (*__debugger_sstep)(struct pt_regs *regs);
-int (*__debugger_iabr_match)(struct pt_regs *regs);
-int (*__debugger_dabr_match)(struct pt_regs *regs);
-int (*__debugger_fault_handler)(struct pt_regs *regs);
-
-EXPORT_SYMBOL(__debugger);
-EXPORT_SYMBOL(__debugger_ipi);
-EXPORT_SYMBOL(__debugger_bpt);
-EXPORT_SYMBOL(__debugger_sstep);
-EXPORT_SYMBOL(__debugger_iabr_match);
-EXPORT_SYMBOL(__debugger_dabr_match);
-EXPORT_SYMBOL(__debugger_fault_handler);
-#endif
-
 ATOMIC_NOTIFIER_HEAD(powerpc_die_chain);
 
 int register_die_notifier(struct notifier_block *nb)
@@ -126,9 +108,6 @@
 		       NOTIFY_STOP)
 		return 1;
 
-	if (debugger(regs))
-		return 1;
-
 	oops_enter();
 
 	if (die.lock_owner != raw_smp_processor_id()) {
@@ -383,11 +362,6 @@
 		return;
 	}
 
-	if (debugger_fault_handler(regs)) {
-		regs->msr |= MSR_RI;
-		return;
-	}
-
 	if (check_io_access(regs))
 		return;
 
@@ -539,8 +513,6 @@
 	if (notify_die(DIE_IABR_MATCH, "iabr_match", regs, 5,
 					5, SIGTRAP) == NOTIFY_STOP)
 		return;
-	if (debugger_iabr_match(regs))
-		return;
 	_exception(SIGTRAP, regs, TRAP_BRKPT, regs->nip);
 }
 
@@ -556,8 +528,6 @@
 	if (notify_die(DIE_SSTEP, "single_step", regs, 5,
 					5, SIGTRAP) == NOTIFY_STOP)
 		return;
-	if (debugger_sstep(regs))
-		return;
 
 	_exception(SIGTRAP, regs, TRAP_TRACE, regs->nip);
 }
@@ -798,8 +768,6 @@
 		if (notify_die(DIE_BPT, "breakpoint", regs, 5, 5, SIGTRAP)
 				== NOTIFY_STOP)
 			return;
-		if (debugger_bpt(regs))
-			return;
 
 		if (!(regs->msr & MSR_PR) &&  /* not user-mode */
 		    report_bug(regs->nip) == BUG_TRAP_TYPE_WARN) {
@@ -888,7 +856,7 @@
 {
 	printk(KERN_CRIT "Kernel stack overflow in process %p, r1=%lx\n",
 	       current, regs->gpr[1]);
-	debugger(regs);
+	notify_die(DIE_DEBUGGER, "die", regs, 5, 5, SIGSEGV);
 	show_regs(regs);
 	panic("kernel stack overflow");
 }
@@ -999,8 +967,6 @@
 			if (notify_die(DIE_SSTEP, "single_step", regs, 5, 5,
 				       SIGTRAP) == NOTIFY_STOP)
 				return;
-			if (debugger_sstep(regs))
-				return;
 		}
 		_exception(SIGTRAP, regs, TRAP_TRACE, 0);
 	}
Index: linux-2.6/arch/powerpc/mm/fault.c
===================================================================
--- linux-2.6.orig/arch/powerpc/mm/fault.c	2007-03-20 09:54:02.000000000 -0500
+++ linux-2.6/arch/powerpc/mm/fault.c	2007-03-20 09:54:03.000000000 -0500
@@ -115,9 +115,6 @@
 			11, SIGSEGV) == NOTIFY_STOP)
 		return;
 
-	if (debugger_dabr_match(regs))
-		return;
-
 	/* Clear the DABR */
 	set_dabr(0);
 
@@ -173,11 +170,6 @@
 				11, SIGSEGV) == NOTIFY_STOP)
 		return 0;
 
-	if (trap == 0x300) {
-		if (debugger_fault_handler(regs))
-			return 0;
-	}
-
 	/* On a kernel SLB miss we can only check for a valid exception entry */
 	if (!user_mode(regs) && (address >= TASK_SIZE))
 		return SIGSEGV;
Index: linux-2.6/arch/powerpc/kernel/smp.c
===================================================================
--- linux-2.6.orig/arch/powerpc/kernel/smp.c	2007-03-20 09:53:19.000000000 -0500
+++ linux-2.6/arch/powerpc/kernel/smp.c	2007-03-20 09:54:03.000000000 -0500
@@ -45,6 +45,7 @@
 #include <asm/system.h>
 #include <asm/mpic.h>
 #include <asm/vdso_datapage.h>
+#include <asm/kdebug.h>
 #ifdef CONFIG_PPC64
 #include <asm/paca.h>
 #endif
@@ -108,10 +109,9 @@
 			crash_ipi_function_ptr(get_irq_regs());
 			break;
 		}
-#ifdef CONFIG_DEBUGGER
-		debugger_ipi(get_irq_regs());
-		break;
-#endif /* CONFIG_DEBUGGER */
+		if (notify_die(DIE_IPI, "smp_call_function",
+			       get_irq_regs(), 5, 5, SIGSEGV) == NOTIFY_STOP)
+			break;
 		/* FALLTHROUGH */
 	default:
 		printk("SMP %d: smp_message_recv(): unknown msg %d\n",
@@ -233,7 +233,8 @@
 			printk("smp_call_function on cpu %d: other cpus not "
 			       "responding (%d)\n", smp_processor_id(),
 			       atomic_read(&data.started));
-			debugger(NULL);
+			notify_die(DIE_DEBUGGER, "smp_call_function",
+				   NULL, 5, 5, SIGSEGV);
 			goto out;
 		}
 	}
@@ -247,7 +248,8 @@
 				       smp_processor_id(),
 				       atomic_read(&data.finished),
 				       atomic_read(&data.started));
-				debugger(NULL);
+				notify_die(DIE_DEBUGGER, "smp_call_function",
+					   NULL, 5, 5, SIGSEGV);
 				goto out;
 			}
 		}
Index: linux-2.6/include/asm-powerpc/kdebug.h
===================================================================
--- linux-2.6.orig/include/asm-powerpc/kdebug.h	2007-03-20 09:54:02.000000000 -0500
+++ linux-2.6/include/asm-powerpc/kdebug.h	2007-03-20 09:54:03.000000000 -0500
@@ -31,6 +31,8 @@
 	DIE_SSTEP,
 	DIE_PAGE_FAULT,
 	DIE_MACHINE_CHECK,
+	DIE_IPI,
+	DIE_DEBUGGER,
 };
 
 static inline int notify_die(enum die_val val, const char *str,

--

[patch 09/10] Use lowercase for hex printouts in oops messages.

[anton]

Use lowercase for hex printouts in oops messages. The number of times I have
tried to copy and paste from an oops into an objdump search...

Signed-off-by: Anton Blanchard <anton@samba.org>
---

Index: linux-2.6/arch/powerpc/kernel/process.c
===================================================================
--- linux-2.6.orig/arch/powerpc/kernel/process.c	2007-03-11 14:40:22.000000000 -0500
+++ linux-2.6/arch/powerpc/kernel/process.c	2007-03-11 14:40:58.000000000 -0500
@@ -400,11 +400,11 @@
 }
 
 #ifdef CONFIG_PPC64
-#define REG		"%016lX"
+#define REG		"%016lx"
 #define REGS_PER_LINE	4
 #define LAST_VOLATILE	13
 #else
-#define REG		"%08lX"
+#define REG		"%08lx"
 #define REGS_PER_LINE	8
 #define LAST_VOLATILE	12
 #endif
@@ -422,7 +422,7 @@
 	       regs, regs->trap, print_tainted(), init_utsname()->release);
 	printk("%sMSR: "REG" ", log_lvl, regs->msr);
 	printbits(regs->msr, msr_bits);
-	printk("  CR: %08lX  XER: %08lX\n", regs->ccr, regs->xer);
+	printk("  CR: %08lx  XER: %08lx\n", regs->ccr, regs->xer);
 	trap = TRAP(regs);
 	if (trap == 0x300 || trap == 0x600)
 		printk("%sDAR: "REG", DSISR: "REG"\n", log_lvl, regs->dar,

--

[patch 10/10] Make sure we only enable xmon once

[anton]

Now we use notifier hooks we have to be careful not to enable xmon multiple
times.

Signed-off-by: Anton Blanchard <anton@samba.org>
---

Index: linux-2.6/arch/powerpc/xmon/xmon.c
===================================================================
--- linux-2.6.orig/arch/powerpc/xmon/xmon.c	2007-03-11 14:52:45.000000000 -0500
+++ linux-2.6/arch/powerpc/xmon/xmon.c	2007-03-11 14:56:37.000000000 -0500
@@ -2658,11 +2658,20 @@
 	unregister_page_fault_notifier(&xmon_page_fault_nb);
 }
 
+#ifdef CONFIG_XMON_DEFAULT
+int __initdata xmon_on = 1;
+#else
+int __initdata xmon_on = 0;
+#endif
+
 #ifdef CONFIG_MAGIC_SYSRQ
 static void sysrq_handle_xmon(int key, struct tty_struct *tty) 
 {
 	/* ensure xmon is enabled */
-	xmon_enable();
+	if (!xmon_on) {
+		xmon_on = 1;
+		xmon_enable();
+	}
 	xmon(get_irq_regs());
 }
 
@@ -2683,18 +2692,18 @@
 __initcall(setup_xmon_sysrq);
 #endif /* CONFIG_MAGIC_SYSRQ */
 
-int __initdata xmon_early, xmon_off;
+int __initdata xmon_early;
 
 static int __init early_parse_xmon(char *p)
 {
 	if (!p || strncmp(p, "early", 5) == 0) {
 		/* just "xmon" is equivalent to "xmon=early" */
-		xmon_enable();
+		xmon_on = 1;
 		xmon_early = 1;
 	} else if (strncmp(p, "on", 2) == 0)
-		xmon_enable();
+		xmon_on = 1;
 	else if (strncmp(p, "off", 3) == 0)
-		xmon_off = 1;
+		xmon_on = 0;
 	else if (strncmp(p, "nobt", 4) == 0)
 		xmon_no_auto_backtrace = 1;
 	else
@@ -2706,10 +2715,9 @@
 
 void __init xmon_setup(void)
 {
-#ifdef CONFIG_XMON_DEFAULT
-	if (!xmon_off)
+	if (xmon_on)
 		xmon_enable();
-#endif
+
 	if (xmon_early)
 		xmon(NULL);
 }

--

Re: [patch 10/10] Make sure we only enable xmon once

[Anton Blanchard]

[Resent with __initdata fix from Olof]

Now we use notifier hooks we have to be careful not to enable xmon multiple
times.

Signed-off-by: Anton Blanchard <anton@samba.org>
---

Index: linux-2.6/arch/powerpc/xmon/xmon.c
===================================================================
--- linux-2.6.orig/arch/powerpc/xmon/xmon.c	2007-03-11 14:52:45.000000000 -0500
+++ linux-2.6/arch/powerpc/xmon/xmon.c	2007-03-11 14:56:37.000000000 -0500
@@ -2658,11 +2658,20 @@
 	unregister_page_fault_notifier(&xmon_page_fault_nb);
 }
 
+#ifdef CONFIG_XMON_DEFAULT
+int xmon_on = 1;
+#else
+int xmon_on = 0;
+#endif
+
 #ifdef CONFIG_MAGIC_SYSRQ
 static void sysrq_handle_xmon(int key, struct tty_struct *tty) 
 {
 	/* ensure xmon is enabled */
-	xmon_enable();
+	if (!xmon_on) {
+		xmon_on = 1;
+		xmon_enable();
+	}
 	xmon(get_irq_regs());
 }
 
@@ -2683,18 +2692,18 @@
 __initcall(setup_xmon_sysrq);
 #endif /* CONFIG_MAGIC_SYSRQ */
 
-int __initdata xmon_early, xmon_off;
+int __initdata xmon_early;
 
 static int __init early_parse_xmon(char *p)
 {
 	if (!p || strncmp(p, "early", 5) == 0) {
 		/* just "xmon" is equivalent to "xmon=early" */
-		xmon_enable();
+		xmon_on = 1;
 		xmon_early = 1;
 	} else if (strncmp(p, "on", 2) == 0)
-		xmon_enable();
+		xmon_on = 1;
 	else if (strncmp(p, "off", 3) == 0)
-		xmon_off = 1;
+		xmon_on = 0;
 	else if (strncmp(p, "nobt", 4) == 0)
 		xmon_no_auto_backtrace = 1;
 	else
@@ -2706,10 +2715,9 @@
 
 void __init xmon_setup(void)
 {
-#ifdef CONFIG_XMON_DEFAULT
-	if (!xmon_off)
+	if (xmon_on)
 		xmon_enable();
-#endif
+
 	if (xmon_early)
 		xmon(NULL);
 }

--

_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [patch 10/10] Make sure we only enable xmon once

[Anton Blanchard]

> > +#ifdef CONFIG_XMON_DEFAULT
> > +int __initdata xmon_on = 1;
> > +#else
> > +int __initdata xmon_on = 0;
> > +#endif
> 
> This shouldn't be __initdata, should it? It's used at runtime below.

Thanks for spotting! New patch on the way.

Anton

Re: [patch 10/10] Make sure we only enable xmon once

[Olof Johansson]

On Tue, Mar 20, 2007 at 08:38:20PM -0500, anton@samba.org wrote:
> Now we use notifier hooks we have to be careful not to enable xmon multiple
> times.
> 
> Signed-off-by: Anton Blanchard <anton@samba.org>
> ---
> 
> Index: linux-2.6/arch/powerpc/xmon/xmon.c
> ===================================================================
> --- linux-2.6.orig/arch/powerpc/xmon/xmon.c	2007-03-11 14:52:45.000000000 -0500
> +++ linux-2.6/arch/powerpc/xmon/xmon.c	2007-03-11 14:56:37.000000000 -0500
> @@ -2658,11 +2658,20 @@
>  	unregister_page_fault_notifier(&xmon_page_fault_nb);
>  }
>  
> +#ifdef CONFIG_XMON_DEFAULT
> +int __initdata xmon_on = 1;
> +#else
> +int __initdata xmon_on = 0;
> +#endif

This shouldn't be __initdata, should it? It's used at runtime below.

> +
>  #ifdef CONFIG_MAGIC_SYSRQ
>  static void sysrq_handle_xmon(int key, struct tty_struct *tty) 
>  {
>  	/* ensure xmon is enabled */
> -	xmon_enable();
> +	if (!xmon_on) {
> +		xmon_on = 1;
> +		xmon_enable();
> +	}
>  	xmon(get_irq_regs());
>  }
[...]



-Olof

Re: [patch 09/10] Use lowercase for hex printouts in oops messages.

[Olof Johansson]

On Tue, Mar 20, 2007 at 08:38:19PM -0500, anton@samba.org wrote:
> Use lowercase for hex printouts in oops messages. The number of times I have
> tried to copy and paste from an oops into an objdump search...

YES! Finally someone bothered to fix this. Thank you!

> Signed-off-by: Anton Blanchard <anton@samba.org>

Acked-by: Olof Johansson <olof@lixom.net>


-Olof

Re: [patch 05/10] Use KERN_EMERG everywhere in oops printout

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 254 bytes --]

On Tue, 20 Mar 2007 20:38:15 -0500 anton@samba.org wrote:
>
> -			printk("\n");
> +			printk("\n%s", KERN_EMERG);

Why not
			printk("\n" KERN_EMERG);

--
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [patch 09/10] Use lowercase for hex printouts in oops messages.

[Geert Uytterhoeven]

On Tue, 20 Mar 2007, Olof Johansson wrote:
> On Tue, Mar 20, 2007 at 08:38:19PM -0500, anton@samba.org wrote:
> > Use lowercase for hex printouts in oops messages. The number of times I have
> > tried to copy and paste from an oops into an objdump search...
> 
> YES! Finally someone bothered to fix this. Thank you!

What about people pasting them into dc? ;-)

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

Re: [patch 07/10] Page fault handler should not depend on CONFIG_KPROBES

[Christoph Hellwig]

On Tue, Mar 20, 2007 at 08:38:17PM -0500, anton@samba.org wrote:
> Like x86-64, change the page fault notifier code to not depend on
> CONFIG_KPROBES and export the handlers.

NACK until the performace implication a fixed.  Martin measured this
costs 300 cycles on s390 even without any notifier activce, and while
ppc might be slightly bad that's still too much to add to the pagefault
handler unconditionally.  Please add a light weight method to check
whether there is any activce user of the notifications.  Also please
move the code into kernel/ at the same time instead of having the exact
same thing duplicated all over, as in my notify_die patches.

> +EXPORT_SYMBOL_GPL(register_page_fault_notifier);

> +EXPORT_SYMBOL_GPL(unregister_page_fault_notifier);

Please drop these exports.  There's not modular users of it, and
the target users (kprobes and debuggers) can't be modular either.

Re: [patch 06/10] Add notify die hooks and remove some redundant debugger hooks

[Christoph Hellwig]

> Index: linux-2.6/include/asm-powerpc/kdebug.h
> ===================================================================
> --- linux-2.6.orig/include/asm-powerpc/kdebug.h	2007-03-11 12:36:52.000000000 -0500
> +++ linux-2.6/include/asm-powerpc/kdebug.h	2007-03-11 12:37:05.000000000 -0500
> @@ -30,6 +30,7 @@
>  	DIE_BPT,
>  	DIE_SSTEP,
>  	DIE_PAGE_FAULT,
> +	DIE_MACHINE_CHECK,
>  };
>  
>  static inline int notify_die(enum die_val val,char *str,struct pt_regs *regs,long err,int trap, int sig)

this is not around anymore after my patch to move the die notifier to
common code and will need some trivial rediffing.

Re: [patch 09/10] Use lowercase for hex printouts in oops messages.

[Segher Boessenkool]

>>> Use lowercase for hex printouts in oops messages. The number of 
>>> times I have
>>> tried to copy and paste from an oops into an objdump search...
>>
>> YES! Finally someone bothered to fix this. Thank you!

Bis.

> What about people pasting them into dc? ;-)

So _that's_ how it works in dc!  :-)


Segher

Re: [patch 05/10] Use KERN_EMERG everywhere in oops printout

[Paul Mackerras]

anton@samba.org writes:

> @@ -926,9 +934,10 @@
>  		    && stack[FRAME_MARKER] == REGS_MARKER) {
>  			struct pt_regs *regs = (struct pt_regs *)
>  				(sp + STACK_FRAME_OVERHEAD);
> -			printk("--- Exception: %lx", regs->trap);
> +			printk("%s--- Exception: %lx", log_lvl, regs->trap);
>  			print_symbol(" at %s\n", regs->nip);
>  			lr = regs->link;
> +			printk(KERN_EMERG);

Why is this one KERN_EMERG rather than log_lvl?  Just an oversight?

Paul.

Re: [patch 06/10] Add notify die hooks and remove some redundant debugger hooks

[Paul Mackerras]

anton@samba.org writes:

> Add a DIE_OOPS and DIE_MACHINE_CHECK notify_die hook and remove some
> redundant debugger* hooks.

As far as I can see, at the moment there is just one thing that gets
registered to be called by die_notify, and that is
kprobe_exceptions_notify.  It doesn't do anything with DIE_OOPS or
DIE_MACHINE_CHECK.

I would rather get rid of die_notify and have a kprobe_notify instead
that just directly calls kprobe_exceptions_notify.  In fact having
separate kprobe routines for the different events would be even
better.

It's not like our cpus are particularly good at indirect functions
calls... ;(

Paul.

Re: [patch 06/10] Add notify die hooks and remove some redundant debugger hooks

[Christoph Hellwig]

On Fri, Mar 23, 2007 at 10:14:50PM +1100, Paul Mackerras wrote:
> anton@samba.org writes:
> 
> > Add a DIE_OOPS and DIE_MACHINE_CHECK notify_die hook and remove some
> > redundant debugger* hooks.
> 
> As far as I can see, at the moment there is just one thing that gets
> registered to be called by die_notify, and that is
> kprobe_exceptions_notify.  It doesn't do anything with DIE_OOPS or
> DIE_MACHINE_CHECK.
> 
> I would rather get rid of die_notify and have a kprobe_notify instead
> that just directly calls kprobe_exceptions_notify.  In fact having
> separate kprobe routines for the different events would be even
> better.
> 
> It's not like our cpus are particularly good at indirect functions
> calls... ;(

See the patch I posted a few days ago for the pagefault path..

Re: [patch 06/10] Add notify die hooks and remove some redundant debugger hooks

[Ananth N Mavinakayanahalli]

On Fri, Mar 23, 2007 at 10:14:50PM +1100, Paul Mackerras wrote:
> anton@samba.org writes:
> 
> > Add a DIE_OOPS and DIE_MACHINE_CHECK notify_die hook and remove some
> > redundant debugger* hooks.
> 
> As far as I can see, at the moment there is just one thing that gets
> registered to be called by die_notify, and that is
> kprobe_exceptions_notify.  It doesn't do anything with DIE_OOPS or
> DIE_MACHINE_CHECK.

Make that two: patch [8/10] of this series converts xmon to use
die_notify and these two were added to facilitate that. In any case,
kprobes just returns NOTIFY_DONE for these callbacks.

> I would rather get rid of die_notify and have a kprobe_notify instead
> that just directly calls kprobe_exceptions_notify.  In fact having
> separate kprobe routines for the different events would be even
> better.

Well, the die_notify hooks for DIE_SSTEP, DIE_BPT (and when kwatch is
implemented for powerpc, even DIE_DABR_MATCH), are shared between
kprobes and xmon, after the xmon conversion to use notifiers.

Ananth

Re: [patch 06/10] Add notify die hooks and remove some redundant debugger hooks

[Anton Blanchard]

Hi,

> As far as I can see, at the moment there is just one thing that gets
> registered to be called by die_notify, and that is
> kprobe_exceptions_notify.  It doesn't do anything with DIE_OOPS or
> DIE_MACHINE_CHECK.

Keep reading :) Later on I remove all the debugger() indirect branches.

> I would rather get rid of die_notify and have a kprobe_notify instead
> that just directly calls kprobe_exceptions_notify.  In fact having
> separate kprobe routines for the different events would be even
> better.
> 
> It's not like our cpus are particularly good at indirect functions
> calls... ;(

True and we are removing a set of indirect branches that exist already
(debugger). Id prefer not to have 2 sets of duplicate hooks for all this
stuff.

Keep in mind with xmon and kprobes disabled these calls are fast. I
verified it in sim after Christophs 300 cycle rant, a pessimistic
estimate is 20 cycles on power5, including function call overhead.

Anton

Re: [patch 07/10] Page fault handler should not depend on CONFIG_KPROBES

[Anton Blanchard]

 
> NACK until the performace implication a fixed.  Martin measured this
> costs 300 cycles on s390 even without any notifier activce, and while
> ppc might be slightly bad that's still too much to add to the pagefault
> handler unconditionally.  Please add a light weight method to check
> whether there is any activce user of the notifications.  Also please
> move the code into kernel/ at the same time instead of having the exact
> same thing duplicated all over, as in my notify_die patches.

Im struggling to understand how they could take 300 cycles to execute
this code sequence. We have seen similar questions on the ia64 side,
they both must have some pretty fundamental branch prediction issues.

I measured this in sim and the sequence takes under 20 cycles on a
power5 class machine, and thats a pessimistic estimate.

Anton

Re: [patch 06/10] Add notify die hooks and remove some redundant debugger hooks

[Paul Mackerras]

Anton Blanchard writes:

> Keep reading :) Later on I remove all the debugger() indirect branches.

Well, you replace one indirect branch with another.  I'm not sure how
that is any better.

> Keep in mind with xmon and kprobes disabled these calls are fast. I
> verified it in sim after Christophs 300 cycle rant, a pessimistic
> estimate is 20 cycles on power5, including function call overhead.

With xmon disabled the debugger() call is already fast.  Generally I
see notifiers as being useful when you have some event, or some group
of events, where you can imagine lots of different software modules
(e.g. random device drivers) being interested in it.  I don't see that
that is the case with any of the kprobes notifiers.

Also I don't see any real point to forcing these disparate things to
go through the one notify_die() interface, with the consequent
demultiplexing that you then have to do in the receiving function.

Paul.

Re: [patch 06/10] Add notify die hooks and remove some redundant debugger hooks

[Paul Mackerras]

Anton Blanchard writes:

> True and we are removing a set of indirect branches that exist already
> (debugger). Id prefer not to have 2 sets of duplicate hooks for all this
> stuff.

There are two concerns I have here.  First, using notifiers tends to
obscure what's going on since the notify call goes off and calls some
random set of functions somewhere, and it's not obvious when reading
the code what that set of functions is.  That's fine when there is a
reason why the set of kernel code modules that want to know about the
event needs to be able to be extended at will without impacting the
generator of the event, but that isn't the case with single-step,
breakpoint or page fault.  If there are only two functions to be
called, I'd rather have a couple of explicit calls than use a
notifier.

The other thing is that we seem to be throwing all sorts of unrelated
events into the die_notifier.  The symptom of that is that you have
one set of handlers that are only interested in the debug events, and
another set that are only interested in the events where the machine
really is dying (oops, etc.).  That is, you have two sets of handlers
that are interested in disjoint sets of events.  That says to me that
we shouldn't be using a single notifier for all the events.
Particularly for page faults I really don't see the point of calling
handlers that are only interested in oopses.

Paul.

Re: [patch 06/10] Add notify die hooks and remove some redundant debugger hooks

[Ananth N Mavinakayanahalli]

On Mon, Apr 09, 2007 at 08:21:25PM +1000, Paul Mackerras wrote:
> 
> The other thing is that we seem to be throwing all sorts of unrelated
> events into the die_notifier.  The symptom of that is that you have
> one set of handlers that are only interested in the debug events, and
> another set that are only interested in the events where the machine
> really is dying (oops, etc.).  That is, you have two sets of handlers
> that are interested in disjoint sets of events.  That says to me that
> we shouldn't be using a single notifier for all the events.
> Particularly for page faults I really don't see the point of calling
> handlers that are only interested in oopses.

The page fault no longer uses the die_notifier. In recent kernels, we
have done away with registering the page fault notifier unconditionally.
In the kprobe case, its active only if there are any active probes.

As to the debug events vs. dying events both using the die_notifier, we
did it that way just to maintain consistency across platforms. i386,
x86_64 and sparc64 were using similar stuff before we put in the powerpc
portions.

Aside, I was told that some folks use the page fault notifiers to
determine application behaviour or somesuch and have used it effectively
to solve some of their performance issues. I don't have the details
though.

Ananth