[RFC] 4xx hardware watchpoint support

[RFC] 4xx hardware watchpoint support

[Luis Machado]

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

Hi,

This is a patch that has been sitting idle for quite some time. I
decided to move it further because it is something useful. It was
originally written by Michel Darneille, based off of 2.6.16.

The original patch, though, was not compatible with the current DABR
logic. DABR's are used to implement hardware watchpoint support for
ppc64 processors (i.e. 970, Power5 etc). 4xx's have a different
debugging register layout and needs to be handled differently (they two
registers: DAC and DBCR0).

I've refreshed the patch to a recent stable release (2.6.25.1, still
apllies cleanly on 2.6.25.4), made the patch compatible with both 4xx
and ppc64 processor designs, fixed some masks that didn't seem correct
(the ones setting hw watch read/write modes) and refactored some of the
code.

Though, i'm still not happy enough with the patch as i think we could
improve it a bit further. Some points i consider worth of attention:

1) There is a do_dac(...) implementation inside
arch/powerpc/kernel/traps.c. I don't feel this is correct. I see that
the 64-bit counterpart, do_dabr(...), is implemented inside
arch/powerpc/mm/fault.c. Due to do_dac(...) being implemented inside
traps.c, we need to externalize the declaration for "get_dac(...)" on
"include/asm-[powerpc|ppc]/system.h" so it's made visible to that scope.
We could use mfspr(...) to get the register's contents directly, but
then i wouldn't make sense to have get_dac(...) in the first place.
Maybe moving the do_dac(...) code to arch/powerpc/mm/fault.c would make
more sense since we seem to have the address already, and won't need to
call get_dac(...) to get it.

2) The change to make set_debugreg(...) and get_debugreg(...)
transparent for both DAC-driven and DABR-driven processors is OK. But
that shouldn't require us to externalize the declaration of
set_debugreg(...) in order to use it in arch/powerpc/kernel/traps.c
right? Maybe this has some relationship with the above point.

3) Maybe it would be better to come up with a way to merge both DABR and
DAC/DBCR0 logic in order to get rid of dozens of processor-specific
#ifdef's? This could be a bit more complex since it would require
re-writing good portions of code.

4) Should i use CONFIG_40x ou CONFIG_4xx instead? Would CONFIG_4xx
automatically include 40x's? I'm mainly targetting 4xx's here, though
40x's should be similar except for 403.

5) This is something i'm worried about for future features. We currently
have a way to support only Hardware Watchpoints, but not Hardware
Breakpoints (on 64-bit processors that have a IABR register or 32-bit
processors carrying the IAC register). Looking at the code, we don't
differentiate a watchpoint from a breakpoint request. A ptrace call has
currently 3 arguments: REQUEST, ADDR and DATA. We use REQUEST and DATA
to set a hardware watchpoint. Maybe we could use the ADDR parameter to
set a hardware breakpoint? Or use it to tell the kernel whether we want
a hardware watchpoint or hardware breakpoint and then pass the address
of the instruction/data through the DATA parameter? What do you think?

I appreciate any comments about these items and the patch itself.

Best regards.
Luis

[-- Attachment #2: 4xx-hw-watch.diff --]
[-- Type: text/x-patch, Size: 12498 bytes --]

Index: linux-2.6.25.1/arch/powerpc/kernel/process.c
===================================================================
--- linux-2.6.25.1.orig/arch/powerpc/kernel/process.c	2008-05-21 07:25:45.000000000 -0700
+++ linux-2.6.25.1/arch/powerpc/kernel/process.c	2008-05-21 07:26:55.000000000 -0700
@@ -219,6 +219,28 @@
 }
 #endif /* CONFIG_SPE */
 
+#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
+
+/* Add support for HW Debug breakpoint.  Use DAC register.  */
+int set_dac(unsigned long dac)
+{
+	mtspr(SPRN_DAC1, dac);
+
+	return 0;
+}
+unsigned int get_dac()
+{
+	return mfspr(SPRN_DAC1);
+}
+int set_dbcr0(unsigned long dbcr)
+{
+	mtspr(SPRN_DBCR0, dbcr);
+
+	return 0;
+}
+
+#endif
+
 #ifndef CONFIG_SMP
 /*
  * If we are doing lazy switching of CPU state (FP, altivec or SPE),
@@ -330,6 +352,13 @@
 	if (unlikely(__get_cpu_var(current_dabr) != new->thread.dabr))
 		set_dabr(new->thread.dabr);
 
+
+#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
+	/* If new thread DAC (HW breakpoint) is the same then leave it.  */
+	if (new->thread.dac)
+		set_dac(new->thread.dac);
+#endif
+
 	new_thread = &new->thread;
 	old_thread = &current->thread;
 
@@ -515,6 +544,16 @@
 
 	discard_lazy_cpu_state();
 
+#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
+	if (current->thread.dac) {
+		current->thread.dac = 0;
+		/* Clear debug control.  */
+		current->thread.dbcr0 &= ~(DBSR_DAC1R | DBSR_DAC1W);
+
+		set_dac(0);
+	}
+#endif
+
 	if (current->thread.dabr) {
 		current->thread.dabr = 0;
 		set_dabr(0);
Index: linux-2.6.25.1/arch/powerpc/kernel/ptrace.c
===================================================================
--- linux-2.6.25.1.orig/arch/powerpc/kernel/ptrace.c	2008-05-21 07:25:45.000000000 -0700
+++ linux-2.6.25.1/arch/powerpc/kernel/ptrace.c	2008-05-21 08:23:34.000000000 -0700
@@ -629,9 +629,15 @@
 {
 	struct pt_regs *regs = task->thread.regs;
 
+#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
+	/* If DAC then do not single step, skip.  */
+	if (task->thread.dac)
+		return;
+#endif
+
 	if (regs != NULL) {
 #if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
-		task->thread.dbcr0 = 0;
+		task->thread.dbcr0 &= ~(DBCR0_IC | DBCR0_IDM);
 		regs->msr &= ~MSR_DE;
 #else
 		regs->msr &= ~MSR_SE;
@@ -640,22 +646,83 @@
 	clear_tsk_thread_flag(task, TIF_SINGLESTEP);
 }
 
-static int ptrace_set_debugreg(struct task_struct *task, unsigned long addr,
+static int ptrace_get_debugreg(struct task_struct *task, unsigned long data)
+{
+	int ret;
+
+	#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
+		ret = put_user(task->thread.dac,
+				(unsigned long __user *)data);
+	#else
+		ret = put_user(task->thread.dabr,
+				(unsigned long __user *)data);
+	#endif
+
+	return ret;
+}
+
+int ptrace_set_debugreg(struct task_struct *task, unsigned long addr,
 			       unsigned long data)
 {
-	/* We only support one DABR and no IABRS at the moment */
+	/* For ppc64 we support one DABR and no IABR's at the moment (ppc64).
+	   For embedded processors we support one DAC and no IAC's
+	   at the moment.  */
 	if (addr > 0)
 		return -EINVAL;
 
-	/* The bottom 3 bits are flags */
 	if ((data & ~0x7UL) >= TASK_SIZE)
 		return -EIO;
 
-	/* Ensure translation is on */
+#ifdef CONFIG_PPC64
+
+	/* For processors using DABR (i.e. 970), the bottom 3 bits are flags.
+	   It was assumed, on previous implementations, that 3 bits were
+	   passed together with the data address, fitting the design of the
+	   DABR register, as follows:
+
+	   bit 0: Read flag
+	   bit 1: Write flag
+	   bit 2: Breakpoint translation
+
+	   Thus, we use them here as so.  */
+
+	/* Ensure breakpoint translation bit is set.  */
 	if (data && !(data & DABR_TRANSLATION))
 		return -EIO;
 
+	/* Move contents to the DABR register.  */
 	task->thread.dabr = data;
+
+#endif
+#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
+
+	/* Read or Write bits must be set.  */
+	if (!(data & 0x3UL))
+		return -EINVAL;
+
+	/* As described above, we assume 3 bits are passed with the data
+	   address, so mask them so we can set the DAC register with the
+	   correct address.  */
+	task->thread.dac = data & ~0x7UL;
+
+	if (task->thread.dac == 0) {
+		task->thread.dbcr0 &= ~(DBSR_DAC1R | DBSR_DAC1W | DBCR0_IDM);
+		task->thread.regs->msr &= ~MSR_DE;
+		return 0;
+	}
+
+	/* Set the Internal Debugging flag (IDM bit 1) for the DBCR0
+	   register.  */
+	task->thread.dbcr0 = DBCR0_IDM;
+
+	/* Check for write and read flags and set DBCR0 accordingly.  */
+	if (data & 1)
+		task->thread.dbcr0 |= DBSR_DAC1R;
+	if (data & 2)
+		task->thread.dbcr0 |= DBSR_DAC1W;
+
+	task->thread.regs->msr |= MSR_DE;
+#endif
 	return 0;
 }
 
@@ -763,11 +830,10 @@
 
 	case PTRACE_GET_DEBUGREG: {
 		ret = -EINVAL;
-		/* We only support one DABR and no IABRS at the moment */
+		/* We only support one DAC or DABR at the moment.  */
 		if (addr > 0)
 			break;
-		ret = put_user(child->thread.dabr,
-			       (unsigned long __user *)data);
+		ret = ptrace_get_debugreg(child, data);
 		break;
 	}
 
Index: linux-2.6.25.1/arch/powerpc/kernel/signal.c
===================================================================
--- linux-2.6.25.1.orig/arch/powerpc/kernel/signal.c	2008-05-21 07:25:45.000000000 -0700
+++ linux-2.6.25.1/arch/powerpc/kernel/signal.c	2008-05-21 07:26:55.000000000 -0700
@@ -148,6 +148,17 @@
 		set_dabr(current->thread.dabr);
 
 	if (is32) {
+#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
+		/*
+		* Reenable the DAC before delivering the signal to
+		* user space. The DAC will have been cleared if it
+		* triggered inside the kernel.
+		*/
+		if (current->thread.dac) {
+			set_dac(current->thread.dac);
+			set_dbcr0(current->thread.dbcr0);
+		}
+#endif
         	if (ka.sa.sa_flags & SA_SIGINFO)
 			ret = handle_rt_signal32(signr, &ka, &info, oldset,
 					regs);
Index: linux-2.6.25.1/arch/powerpc/kernel/traps.c
===================================================================
--- linux-2.6.25.1.orig/arch/powerpc/kernel/traps.c	2008-05-21 07:25:45.000000000 -0700
+++ linux-2.6.25.1/arch/powerpc/kernel/traps.c	2008-05-21 09:08:49.000000000 -0700
@@ -54,6 +54,9 @@
 #endif
 #include <asm/kexec.h>
 
+extern int ptrace_set_debugreg(struct task_struct *task, unsigned long addr,
+						      unsigned long data);
+
 #if defined(CONFIG_DEBUGGER) || defined(CONFIG_KEXEC)
 int (*__debugger)(struct pt_regs *regs);
 int (*__debugger_ipi)(struct pt_regs *regs);
@@ -61,6 +64,7 @@
 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_dac_match)(struct pt_regs *regs);
 int (*__debugger_fault_handler)(struct pt_regs *regs);
 
 EXPORT_SYMBOL(__debugger);
@@ -69,6 +73,7 @@
 EXPORT_SYMBOL(__debugger_sstep);
 EXPORT_SYMBOL(__debugger_iabr_match);
 EXPORT_SYMBOL(__debugger_dabr_match);
+EXPORT_SYMBOL(__debugger_dac_match);
 EXPORT_SYMBOL(__debugger_fault_handler);
 #endif
 
@@ -253,6 +258,27 @@
 }
 #endif
 
+#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
+
+static void do_dac(struct pt_regs *regs, unsigned long address,
+						unsigned long error_code)
+{
+	siginfo_t info;
+
+	/* Clear the DAC and struct entries.  One shot trigger.  */
+	mtspr(SPRN_DBCR0, mfspr(SPRN_DBCR0) & ~(DBSR_DAC1R | DBSR_DAC1W
+							| DBCR0_IDM));
+	set_dac(0);
+	ptrace_set_debugreg(current, 0, 0);
+
+	/* Deliver signal to userspace.  */
+	info.si_signo = SIGTRAP;
+	info.si_errno = 0;
+	info.si_code = TRAP_HWBKPT;
+	info.si_addr = (void __user *)address;
+	force_sig_info(SIGTRAP, &info, current);
+}
+#endif
 /*
  * I/O accesses can cause machine checks on powermacs.
  * Check if the NIP corresponds to the address of a sync
@@ -1045,6 +1071,20 @@
 				return;
 		}
 		_exception(SIGTRAP, regs, TRAP_TRACE, 0);
+	} else if (debug_status & (DBSR_DAC1R | DBSR_DAC1W)) {
+		regs->msr &= ~MSR_DE;
+		if (user_mode(regs)) {
+			current->thread.dbcr0 &= ~(DBSR_DAC1R | DBSR_DAC1W |
+								DBCR0_IDM);
+		} else {
+			/* Disable DAC interupts.  */
+			mtspr(SPRN_DBCR0, mfspr(SPRN_DBCR0) & ~(DBSR_DAC1R |
+							DBSR_DAC1W | DBCR0_IDM));
+			/* Clear the DAC event.  */
+			mtspr(SPRN_DBSR, (DBSR_DAC1R | DBSR_DAC1W));
+		}
+		/* Setup and send the trap to the handler.  */
+		do_dac(regs, get_dac(), debug_status);
 	}
 }
 #endif /* CONFIG_4xx || CONFIG_BOOKE */
Index: linux-2.6.25.1/include/asm-powerpc/processor.h
===================================================================
--- linux-2.6.25.1.orig/include/asm-powerpc/processor.h	2008-05-21 07:25:45.000000000 -0700
+++ linux-2.6.25.1/include/asm-powerpc/processor.h	2008-05-21 07:26:55.000000000 -0700
@@ -149,6 +149,7 @@
 #if defined(CONFIG_4xx) || defined (CONFIG_BOOKE)
 	unsigned long	dbcr0;		/* debug control register values */
 	unsigned long	dbcr1;
+	unsigned long   dac;		/* Data Address Compare register */
 #endif
 	double		fpr[32];	/* Complete floating point set */
 	struct {			/* fpr ... fpscr must be contiguous */
Index: linux-2.6.25.1/include/asm-powerpc/system.h
===================================================================
--- linux-2.6.25.1.orig/include/asm-powerpc/system.h	2008-05-21 07:25:45.000000000 -0700
+++ linux-2.6.25.1/include/asm-powerpc/system.h	2008-05-21 08:26:10.000000000 -0700
@@ -73,6 +73,7 @@
 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_dac_match)(struct pt_regs *regs);
 extern int (*__debugger_fault_handler)(struct pt_regs *regs);
 
 #define DEBUGGER_BOILERPLATE(__NAME) \
@@ -89,6 +90,7 @@
 DEBUGGER_BOILERPLATE(debugger_sstep)
 DEBUGGER_BOILERPLATE(debugger_iabr_match)
 DEBUGGER_BOILERPLATE(debugger_dabr_match)
+DEBUGGER_BOILERPLATE(debugger_dac_match)
 DEBUGGER_BOILERPLATE(debugger_fault_handler)
 
 #else
@@ -98,10 +100,17 @@
 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_dac_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);
+
+/* These are 4XX-specific functions for debugging register manipulations.  */
+extern int set_dac(unsigned long dac);
+extern unsigned int get_dac(void);
+extern int set_dbcr0(unsigned long dbcr);
+
 extern void print_backtrace(unsigned long *);
 extern void show_regs(struct pt_regs * regs);
 extern void flush_instruction_cache(void);
Index: linux-2.6.25.1/arch/powerpc/kernel/entry_32.S
===================================================================
--- linux-2.6.25.1.orig/arch/powerpc/kernel/entry_32.S	2008-05-21 07:25:45.000000000 -0700
+++ linux-2.6.25.1/arch/powerpc/kernel/entry_32.S	2008-05-21 07:26:55.000000000 -0700
@@ -112,7 +112,7 @@
 	/* Check to see if the dbcr0 register is set up to debug.  Use the
 	   single-step bit to do this. */
 	lwz	r12,THREAD_DBCR0(r12)
-	andis.	r12,r12,DBCR0_IC@h
+	andis.	r12,r12,(DBCR0_IC | DBSR_DAC1R | DBSR_DAC1W)@h
 	beq+	3f
 	/* From user and task is ptraced - load up global dbcr0 */
 	li	r12,-1			/* clear all pending debug events */
@@ -241,7 +241,7 @@
 	/* If the process has its own DBCR0 value, load it up.  The single
 	   step bit tells us that dbcr0 should be loaded. */
 	lwz	r0,THREAD+THREAD_DBCR0(r2)
-	andis.	r10,r0,DBCR0_IC@h
+	andis.	r10,r0,(DBCR0_IC | DBSR_DAC1R | DBSR_DAC1W)@h
 	bnel-	load_dbcr0
 #endif
 #ifdef CONFIG_44x
@@ -669,7 +669,7 @@
 	/* Check whether this process has its own DBCR0 value.  The single
 	   step bit tells us that dbcr0 should be loaded. */
 	lwz	r0,THREAD+THREAD_DBCR0(r2)
-	andis.	r10,r0,DBCR0_IC@h
+	andis.	r10,r0,(DBCR0_IC | DBSR_DAC1R | DBSR_DAC1W)@h
 	bnel-	load_dbcr0
 #endif
 
Index: linux-2.6.25.1/include/asm-ppc/system.h
===================================================================
--- linux-2.6.25.1.orig/include/asm-ppc/system.h	2008-05-01 14:45:25.000000000 -0700
+++ linux-2.6.25.1/include/asm-ppc/system.h	2008-05-21 08:26:28.000000000 -0700
@@ -56,6 +56,12 @@
 extern void hard_reset_now(void);
 extern void poweroff_now(void);
 extern int set_dabr(unsigned long dabr);
+
+/* These are 4XX-specific functions for debugging register manipulations.  */
+extern int set_dac(unsigned long dac);
+extern unsigned int get_dac(void);
+extern int set_dbcr0(unsigned long dbcr);
+
 #ifdef CONFIG_6xx
 extern long _get_L2CR(void);
 extern long _get_L3CR(void);

Re: [RFC] 4xx hardware watchpoint support

[Kumar Gala]

Two real quick notes.

Take a look at:

http://ozlabs.org/pipermail/linuxppc-dev/2008-May/055745.html

and can you try and post the patch inline next time.  Hard to provide  
review comments on it :)

- k

Re: [RFC] 4xx hardware watchpoint support

[Luis Machado]

Thanks for the inlining tip. It should be now. :-)

So, basically we are looking at a cleaner and much better interface to
set such hardware features? That's something that would greatly improve
the communication from, say, GDB to the kernel regarding these
facilities.

Regards,
Luis

On Wed, 2008-05-21 at 16:16 -0500, Kumar Gala wrote:
> Two real quick notes.
> 
> Take a look at:
> 
> http://ozlabs.org/pipermail/linuxppc-dev/2008-May/055745.html
> 
> and can you try and post the patch inline next time.  Hard to provide  
> review comments on it :)
> 
> - kIndex: linux-2.6.25.1/arch/powerpc/kernel/process.c

===================================================================
--- linux-2.6.25.1.orig/arch/powerpc/kernel/process.c	2008-05-21 07:25:45.000000000 -0700
+++ linux-2.6.25.1/arch/powerpc/kernel/process.c	2008-05-21 07:26:55.000000000 -0700
@@ -219,6 +219,28 @@
 }
 #endif /* CONFIG_SPE */
 
+#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
+
+/* Add support for HW Debug breakpoint.  Use DAC register.  */
+int set_dac(unsigned long dac)
+{
+	mtspr(SPRN_DAC1, dac);
+
+	return 0;
+}
+unsigned int get_dac()
+{
+	return mfspr(SPRN_DAC1);
+}
+int set_dbcr0(unsigned long dbcr)
+{
+	mtspr(SPRN_DBCR0, dbcr);
+
+	return 0;
+}
+
+#endif
+
 #ifndef CONFIG_SMP
 /*
  * If we are doing lazy switching of CPU state (FP, altivec or SPE),
@@ -330,6 +352,13 @@
 	if (unlikely(__get_cpu_var(current_dabr) != new->thread.dabr))
 		set_dabr(new->thread.dabr);
 
+
+#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
+	/* If new thread DAC (HW breakpoint) is the same then leave it.  */
+	if (new->thread.dac)
+		set_dac(new->thread.dac);
+#endif
+
 	new_thread = &new->thread;
 	old_thread = &current->thread;
 
@@ -515,6 +544,16 @@
 
 	discard_lazy_cpu_state();
 
+#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
+	if (current->thread.dac) {
+		current->thread.dac = 0;
+		/* Clear debug control.  */
+		current->thread.dbcr0 &= ~(DBSR_DAC1R | DBSR_DAC1W);
+
+		set_dac(0);
+	}
+#endif
+
 	if (current->thread.dabr) {
 		current->thread.dabr = 0;
 		set_dabr(0);
Index: linux-2.6.25.1/arch/powerpc/kernel/ptrace.c
===================================================================
--- linux-2.6.25.1.orig/arch/powerpc/kernel/ptrace.c	2008-05-21 07:25:45.000000000 -0700
+++ linux-2.6.25.1/arch/powerpc/kernel/ptrace.c	2008-05-21 08:23:34.000000000 -0700
@@ -629,9 +629,15 @@
 {
 	struct pt_regs *regs = task->thread.regs;
 
+#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
+	/* If DAC then do not single step, skip.  */
+	if (task->thread.dac)
+		return;
+#endif
+
 	if (regs != NULL) {
 #if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
-		task->thread.dbcr0 = 0;
+		task->thread.dbcr0 &= ~(DBCR0_IC | DBCR0_IDM);
 		regs->msr &= ~MSR_DE;
 #else
 		regs->msr &= ~MSR_SE;
@@ -640,22 +646,83 @@
 	clear_tsk_thread_flag(task, TIF_SINGLESTEP);
 }
 
-static int ptrace_set_debugreg(struct task_struct *task, unsigned long addr,
+static int ptrace_get_debugreg(struct task_struct *task, unsigned long data)
+{
+	int ret;
+
+	#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
+		ret = put_user(task->thread.dac,
+				(unsigned long __user *)data);
+	#else
+		ret = put_user(task->thread.dabr,
+				(unsigned long __user *)data);
+	#endif
+
+	return ret;
+}
+
+int ptrace_set_debugreg(struct task_struct *task, unsigned long addr,
 			       unsigned long data)
 {
-	/* We only support one DABR and no IABRS at the moment */
+	/* For ppc64 we support one DABR and no IABR's at the moment (ppc64).
+	   For embedded processors we support one DAC and no IAC's
+	   at the moment.  */
 	if (addr > 0)
 		return -EINVAL;
 
-	/* The bottom 3 bits are flags */
 	if ((data & ~0x7UL) >= TASK_SIZE)
 		return -EIO;
 
-	/* Ensure translation is on */
+#ifdef CONFIG_PPC64
+
+	/* For processors using DABR (i.e. 970), the bottom 3 bits are flags.
+	   It was assumed, on previous implementations, that 3 bits were
+	   passed together with the data address, fitting the design of the
+	   DABR register, as follows:
+
+	   bit 0: Read flag
+	   bit 1: Write flag
+	   bit 2: Breakpoint translation
+
+	   Thus, we use them here as so.  */
+
+	/* Ensure breakpoint translation bit is set.  */
 	if (data && !(data & DABR_TRANSLATION))
 		return -EIO;
 
+	/* Move contents to the DABR register.  */
 	task->thread.dabr = data;
+
+#endif
+#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
+
+	/* Read or Write bits must be set.  */
+	if (!(data & 0x3UL))
+		return -EINVAL;
+
+	/* As described above, we assume 3 bits are passed with the data
+	   address, so mask them so we can set the DAC register with the
+	   correct address.  */
+	task->thread.dac = data & ~0x7UL;
+
+	if (task->thread.dac == 0) {
+		task->thread.dbcr0 &= ~(DBSR_DAC1R | DBSR_DAC1W | DBCR0_IDM);
+		task->thread.regs->msr &= ~MSR_DE;
+		return 0;
+	}
+
+	/* Set the Internal Debugging flag (IDM bit 1) for the DBCR0
+	   register.  */
+	task->thread.dbcr0 = DBCR0_IDM;
+
+	/* Check for write and read flags and set DBCR0 accordingly.  */
+	if (data & 1)
+		task->thread.dbcr0 |= DBSR_DAC1R;
+	if (data & 2)
+		task->thread.dbcr0 |= DBSR_DAC1W;
+
+	task->thread.regs->msr |= MSR_DE;
+#endif
 	return 0;
 }
 
@@ -763,11 +830,10 @@
 
 	case PTRACE_GET_DEBUGREG: {
 		ret = -EINVAL;
-		/* We only support one DABR and no IABRS at the moment */
+		/* We only support one DAC or DABR at the moment.  */
 		if (addr > 0)
 			break;
-		ret = put_user(child->thread.dabr,
-			       (unsigned long __user *)data);
+		ret = ptrace_get_debugreg(child, data);
 		break;
 	}
 
Index: linux-2.6.25.1/arch/powerpc/kernel/signal.c
===================================================================
--- linux-2.6.25.1.orig/arch/powerpc/kernel/signal.c	2008-05-21 07:25:45.000000000 -0700
+++ linux-2.6.25.1/arch/powerpc/kernel/signal.c	2008-05-21 07:26:55.000000000 -0700
@@ -148,6 +148,17 @@
 		set_dabr(current->thread.dabr);
 
 	if (is32) {
+#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
+		/*
+		* Reenable the DAC before delivering the signal to
+		* user space. The DAC will have been cleared if it
+		* triggered inside the kernel.
+		*/
+		if (current->thread.dac) {
+			set_dac(current->thread.dac);
+			set_dbcr0(current->thread.dbcr0);
+		}
+#endif
         	if (ka.sa.sa_flags & SA_SIGINFO)
 			ret = handle_rt_signal32(signr, &ka, &info, oldset,
 					regs);
Index: linux-2.6.25.1/arch/powerpc/kernel/traps.c
===================================================================
--- linux-2.6.25.1.orig/arch/powerpc/kernel/traps.c	2008-05-21 07:25:45.000000000 -0700
+++ linux-2.6.25.1/arch/powerpc/kernel/traps.c	2008-05-21 09:08:49.000000000 -0700
@@ -54,6 +54,9 @@
 #endif
 #include <asm/kexec.h>
 
+extern int ptrace_set_debugreg(struct task_struct *task, unsigned long addr,
+						      unsigned long data);
+
 #if defined(CONFIG_DEBUGGER) || defined(CONFIG_KEXEC)
 int (*__debugger)(struct pt_regs *regs);
 int (*__debugger_ipi)(struct pt_regs *regs);
@@ -61,6 +64,7 @@
 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_dac_match)(struct pt_regs *regs);
 int (*__debugger_fault_handler)(struct pt_regs *regs);
 
 EXPORT_SYMBOL(__debugger);
@@ -69,6 +73,7 @@
 EXPORT_SYMBOL(__debugger_sstep);
 EXPORT_SYMBOL(__debugger_iabr_match);
 EXPORT_SYMBOL(__debugger_dabr_match);
+EXPORT_SYMBOL(__debugger_dac_match);
 EXPORT_SYMBOL(__debugger_fault_handler);
 #endif
 
@@ -253,6 +258,27 @@
 }
 #endif
 
+#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
+
+static void do_dac(struct pt_regs *regs, unsigned long address,
+						unsigned long error_code)
+{
+	siginfo_t info;
+
+	/* Clear the DAC and struct entries.  One shot trigger.  */
+	mtspr(SPRN_DBCR0, mfspr(SPRN_DBCR0) & ~(DBSR_DAC1R | DBSR_DAC1W
+							| DBCR0_IDM));
+	set_dac(0);
+	ptrace_set_debugreg(current, 0, 0);
+
+	/* Deliver signal to userspace.  */
+	info.si_signo = SIGTRAP;
+	info.si_errno = 0;
+	info.si_code = TRAP_HWBKPT;
+	info.si_addr = (void __user *)address;
+	force_sig_info(SIGTRAP, &info, current);
+}
+#endif
 /*
  * I/O accesses can cause machine checks on powermacs.
  * Check if the NIP corresponds to the address of a sync
@@ -1045,6 +1071,20 @@
 				return;
 		}
 		_exception(SIGTRAP, regs, TRAP_TRACE, 0);
+	} else if (debug_status & (DBSR_DAC1R | DBSR_DAC1W)) {
+		regs->msr &= ~MSR_DE;
+		if (user_mode(regs)) {
+			current->thread.dbcr0 &= ~(DBSR_DAC1R | DBSR_DAC1W |
+								DBCR0_IDM);
+		} else {
+			/* Disable DAC interupts.  */
+			mtspr(SPRN_DBCR0, mfspr(SPRN_DBCR0) & ~(DBSR_DAC1R |
+							DBSR_DAC1W | DBCR0_IDM));
+			/* Clear the DAC event.  */
+			mtspr(SPRN_DBSR, (DBSR_DAC1R | DBSR_DAC1W));
+		}
+		/* Setup and send the trap to the handler.  */
+		do_dac(regs, get_dac(), debug_status);
 	}
 }
 #endif /* CONFIG_4xx || CONFIG_BOOKE */
Index: linux-2.6.25.1/include/asm-powerpc/processor.h
===================================================================
--- linux-2.6.25.1.orig/include/asm-powerpc/processor.h	2008-05-21 07:25:45.000000000 -0700
+++ linux-2.6.25.1/include/asm-powerpc/processor.h	2008-05-21 07:26:55.000000000 -0700
@@ -149,6 +149,7 @@
 #if defined(CONFIG_4xx) || defined (CONFIG_BOOKE)
 	unsigned long	dbcr0;		/* debug control register values */
 	unsigned long	dbcr1;
+	unsigned long   dac;		/* Data Address Compare register */
 #endif
 	double		fpr[32];	/* Complete floating point set */
 	struct {			/* fpr ... fpscr must be contiguous */
Index: linux-2.6.25.1/include/asm-powerpc/system.h
===================================================================
--- linux-2.6.25.1.orig/include/asm-powerpc/system.h	2008-05-21 07:25:45.000000000 -0700
+++ linux-2.6.25.1/include/asm-powerpc/system.h	2008-05-21 08:26:10.000000000 -0700
@@ -73,6 +73,7 @@
 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_dac_match)(struct pt_regs *regs);
 extern int (*__debugger_fault_handler)(struct pt_regs *regs);
 
 #define DEBUGGER_BOILERPLATE(__NAME) \
@@ -89,6 +90,7 @@
 DEBUGGER_BOILERPLATE(debugger_sstep)
 DEBUGGER_BOILERPLATE(debugger_iabr_match)
 DEBUGGER_BOILERPLATE(debugger_dabr_match)
+DEBUGGER_BOILERPLATE(debugger_dac_match)
 DEBUGGER_BOILERPLATE(debugger_fault_handler)
 
 #else
@@ -98,10 +100,17 @@
 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_dac_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);
+
+/* These are 4XX-specific functions for debugging register manipulations.  */
+extern int set_dac(unsigned long dac);
+extern unsigned int get_dac(void);
+extern int set_dbcr0(unsigned long dbcr);
+
 extern void print_backtrace(unsigned long *);
 extern void show_regs(struct pt_regs * regs);
 extern void flush_instruction_cache(void);
Index: linux-2.6.25.1/arch/powerpc/kernel/entry_32.S
===================================================================
--- linux-2.6.25.1.orig/arch/powerpc/kernel/entry_32.S	2008-05-21 07:25:45.000000000 -0700
+++ linux-2.6.25.1/arch/powerpc/kernel/entry_32.S	2008-05-21 07:26:55.000000000 -0700
@@ -112,7 +112,7 @@
 	/* Check to see if the dbcr0 register is set up to debug.  Use the
 	   single-step bit to do this. */
 	lwz	r12,THREAD_DBCR0(r12)
-	andis.	r12,r12,DBCR0_IC@h
+	andis.	r12,r12,(DBCR0_IC | DBSR_DAC1R | DBSR_DAC1W)@h
 	beq+	3f
 	/* From user and task is ptraced - load up global dbcr0 */
 	li	r12,-1			/* clear all pending debug events */
@@ -241,7 +241,7 @@
 	/* If the process has its own DBCR0 value, load it up.  The single
 	   step bit tells us that dbcr0 should be loaded. */
 	lwz	r0,THREAD+THREAD_DBCR0(r2)
-	andis.	r10,r0,DBCR0_IC@h
+	andis.	r10,r0,(DBCR0_IC | DBSR_DAC1R | DBSR_DAC1W)@h
 	bnel-	load_dbcr0
 #endif
 #ifdef CONFIG_44x
@@ -669,7 +669,7 @@
 	/* Check whether this process has its own DBCR0 value.  The single
 	   step bit tells us that dbcr0 should be loaded. */
 	lwz	r0,THREAD+THREAD_DBCR0(r2)
-	andis.	r10,r0,DBCR0_IC@h
+	andis.	r10,r0,(DBCR0_IC | DBSR_DAC1R | DBSR_DAC1W)@h
 	bnel-	load_dbcr0
 #endif
 
Index: linux-2.6.25.1/include/asm-ppc/system.h
===================================================================
--- linux-2.6.25.1.orig/include/asm-ppc/system.h	2008-05-01 14:45:25.000000000 -0700
+++ linux-2.6.25.1/include/asm-ppc/system.h	2008-05-21 08:26:28.000000000 -0700
@@ -56,6 +56,12 @@
 extern void hard_reset_now(void);
 extern void poweroff_now(void);
 extern int set_dabr(unsigned long dabr);
+
+/* These are 4XX-specific functions for debugging register manipulations.  */
+extern int set_dac(unsigned long dac);
+extern unsigned int get_dac(void);
+extern int set_dbcr0(unsigned long dbcr);
+
 #ifdef CONFIG_6xx
 extern long _get_L2CR(void);
 extern long _get_L3CR(void);

Re: [RFC] 4xx hardware watchpoint support

[Paul Mackerras]

Luis Machado writes:

> This is a patch that has been sitting idle for quite some time. I
> decided to move it further because it is something useful. It was
> originally written by Michel Darneille, based off of 2.6.16.
> 
> The original patch, though, was not compatible with the current DABR
> logic. DABR's are used to implement hardware watchpoint support for
> ppc64 processors (i.e. 970, Power5 etc). 4xx's have a different
> debugging register layout and needs to be handled differently (they two
> registers: DAC and DBCR0).

Yes, they are different, but they do essentially the same thing, so I
think we should try and unify the handling of the two.  Maybe you
could rename set_dabr() to set_hw_watchpoint() or something and make
it set DABR on 64-bit and "classic" 32-bit processors, and DAC on
4xx/Book E processors.

Likewise, I don't think we need both a "dabr" field and a "dac" field
in the thread_struct - one should do.  Rename "dabr" to something else
if you feel that the "dabr" name is too ppc64-specific.  And I don't
think we need both __debugger_dabr_match and __debugger_dac_match.

> 5) This is something i'm worried about for future features. We currently
> have a way to support only Hardware Watchpoints, but not Hardware
> Breakpoints (on 64-bit processors that have a IABR register or 32-bit
> processors carrying the IAC register). Looking at the code, we don't
> differentiate a watchpoint from a breakpoint request. A ptrace call has
> currently 3 arguments: REQUEST, ADDR and DATA. We use REQUEST and DATA
> to set a hardware watchpoint. Maybe we could use the ADDR parameter to
> set a hardware breakpoint? Or use it to tell the kernel whether we want
> a hardware watchpoint or hardware breakpoint and then pass the address
> of the instruction/data through the DATA parameter? What do you think?

I would think there would be a different REQUEST value to mean "set a
hardware breakpoint".  Roland McGrath (cc'd) might be able to tell us
what other architectures do.

Paul.

Re: [RFC] 4xx hardware watchpoint support

[Roland McGrath]

> I would think there would be a different REQUEST value to mean "set a
> hardware breakpoint".  Roland McGrath (cc'd) might be able to tell us
> what other architectures do.

Other architectures don't give a good model to follow.  (If anything,
they just trivally virtualize their own idiosyncratic hardware.)

What I want to see done for this in the future is reviving and
finishing the hw_breakpoint work begun by Alan Stern, and porting
that to each arch's particular hardware features.  On that we'd
build any new interfaces in abstract machine-independent terms,
just describing the constraints of what the hardware can do,
rather than having the user interface involve mimicking hardware
encodings.  (The existing hardware-idiosyncratic ptrace interfaces
would tie into hw_breakpoint for backward compatibility.)


Thanks,
Roland

Re: [RFC] 4xx hardware watchpoint support

[Luis Machado]

On Thu, 2008-05-22 at 13:51 +1000, Paul Mackerras wrote:
> Luis Machado writes:
> 
> > This is a patch that has been sitting idle for quite some time. I
> > decided to move it further because it is something useful. It was
> > originally written by Michel Darneille, based off of 2.6.16.
> > 
> > The original patch, though, was not compatible with the current DABR
> > logic. DABR's are used to implement hardware watchpoint support for
> > ppc64 processors (i.e. 970, Power5 etc). 4xx's have a different
> > debugging register layout and needs to be handled differently (they two
> > registers: DAC and DBCR0).
> 
> Yes, they are different, but they do essentially the same thing, so I
> think we should try and unify the handling of the two.  Maybe you
> could rename set_dabr() to set_hw_watchpoint() or something and make
> it set DABR on 64-bit and "classic" 32-bit processors, and DAC on
> 4xx/Book E processors.
> 
> Likewise, I don't think we need both a "dabr" field and a "dac" field
> in the thread_struct - one should do.  Rename "dabr" to something else
> if you feel that the "dabr" name is too ppc64-specific.  And I don't
> think we need both __debugger_dabr_match and __debugger_dac_match.
> 

Thanks for the feedback Paul. I'll try consolidating those mechanisms
into a single, more general scheme. 

Best regards,
Luis

Re: [RFC] 4xx hardware watchpoint support

[Luis Machado]

On Wed, 2008-05-21 at 23:46 -0700, Roland McGrath wrote:
> > I would think there would be a different REQUEST value to mean "set a
> > hardware breakpoint".  Roland McGrath (cc'd) might be able to tell us
> > what other architectures do.
> 
> Other architectures don't give a good model to follow.  (If anything,
> they just trivally virtualize their own idiosyncratic hardware.)
> 
> What I want to see done for this in the future is reviving and
> finishing the hw_breakpoint work begun by Alan Stern, and porting
> that to each arch's particular hardware features.  On that we'd
> build any new interfaces in abstract machine-independent terms,
> just describing the constraints of what the hardware can do,
> rather than having the user interface involve mimicking hardware
> encodings.  (The existing hardware-idiosyncratic ptrace interfaces
> would tie into hw_breakpoint for backward compatibility.)
> 
> 

Kumar was just mentioning this post a few messages ago:
http://ozlabs.org/pipermail/linuxppc-dev/2008-May/055745.html

That is a very interesting approach to handle all the differences
between each processor's architecture, and a much cleaner way to set the
facilities we want than the current interface we have. Do you know what
is the status of this work? Did it move any further?

Best Regards,
Luis

Re: [RFC] 4xx hardware watchpoint support

[Roland McGrath]

> Kumar was just mentioning this post a few messages ago:
> http://ozlabs.org/pipermail/linuxppc-dev/2008-May/055745.html
> 
> That is a very interesting approach to handle all the differences
> between each processor's architecture, and a much cleaner way to set the
> facilities we want than the current interface we have. Do you know what
> is the status of this work? Did it move any further?

prasad@linux.vnet.ibm.com was going to look into this.  I don't think there
has been much progress yet, but I hope we can get it started up again.


Thanks,
Roland

Re: [RFC] 4xx hardware watchpoint support

[Luis Machado]

On Thu, 2008-05-22 at 13:51 +1000, Paul Mackerras wrote:
> Luis Machado writes:
> 
> > This is a patch that has been sitting idle for quite some time. I
> > decided to move it further because it is something useful. It was
> > originally written by Michel Darneille, based off of 2.6.16.
> > 
> > The original patch, though, was not compatible with the current DABR
> > logic. DABR's are used to implement hardware watchpoint support for
> > ppc64 processors (i.e. 970, Power5 etc). 4xx's have a different
> > debugging register layout and needs to be handled differently (they two
> > registers: DAC and DBCR0).
> 
> Yes, they are different, but they do essentially the same thing, so I
> think we should try and unify the handling of the two.  Maybe you
> could rename set_dabr() to set_hw_watchpoint() or something and make
> it set DABR on 64-bit and "classic" 32-bit processors, and DAC on
> 4xx/Book E processors.
> 
> Likewise, I don't think we need both a "dabr" field and a "dac" field
> in the thread_struct - one should do.  Rename "dabr" to something else
> if you feel that the "dabr" name is too ppc64-specific.  And I don't
> think we need both __debugger_dabr_match and __debugger_dac_match.
> 
> > 5) This is something i'm worried about for future features. We currently
> > have a way to support only Hardware Watchpoints, but not Hardware
> > Breakpoints (on 64-bit processors that have a IABR register or 32-bit
> > processors carrying the IAC register). Looking at the code, we don't
> > differentiate a watchpoint from a breakpoint request. A ptrace call has
> > currently 3 arguments: REQUEST, ADDR and DATA. We use REQUEST and DATA
> > to set a hardware watchpoint. Maybe we could use the ADDR parameter to
> > set a hardware breakpoint? Or use it to tell the kernel whether we want
> > a hardware watchpoint or hardware breakpoint and then pass the address
> > of the instruction/data through the DATA parameter? What do you think?
> 
> I would think there would be a different REQUEST value to mean "set a
> hardware breakpoint".  Roland McGrath (cc'd) might be able to tell us
> what other architectures do.
> 
> Paul.

Paul,

Follows a re-worked patch that unifies the handling of hardware
watchpoint structures for DABR-based and DAC-based processors.

The flow is exactly the same for DABR-based processors.

As for the DAC-based code, i've eliminated the "dac" field from the
thread structure, since now we use the "dabr" field to represent DAC1 of
the DAC-based processors. As a consequence, i removed all the
occurrences of "dac" throughout the code, using "dabr" in those cases.

The function "set_dabr" sets the correct register (DABR OR DAC) for each
specific processor now, instead of only setting the value for DABR-based
processors.

"do_dac" was merged with "do_dabr" as they were mostly equivalent pieces
of code. I've moved "do_dabr" to "arch/powerpc/kernel/process.c" so it
is visible for DAC-based code as well.

Tested on a Taishan 440GX with success.

Is it OK to leave it as "dabr" for both DABR and DAC? What do you think
of the patch overall?

Thanks,

Best regards,
Luis


Index: linux-2.6.25-oldpatch/arch/powerpc/kernel/process.c
===================================================================
--- linux-2.6.25-oldpatch.orig/arch/powerpc/kernel/process.c	2008-06-20 02:48:19.000000000 -0700
+++ linux-2.6.25-oldpatch/arch/powerpc/kernel/process.c	2008-06-20 02:51:16.000000000 -0700
@@ -241,6 +241,35 @@
 }
 #endif /* CONFIG_SMP */
 
+void do_dabr(struct pt_regs *regs, unsigned long address,
+		    unsigned long error_code)
+{
+	siginfo_t info;
+
+#if !(defined(CONFIG_4xx) || defined(CONFIG_BOOKE))
+	if (notify_die(DIE_DABR_MATCH, "dabr_match", regs, error_code,
+			11, SIGSEGV) == NOTIFY_STOP)
+		return;
+
+	if (debugger_dabr_match(regs))
+		return;
+#else
+        /* Clear the DAC and struct entries.  One shot trigger.  */
+        mtspr(SPRN_DBCR0, mfspr(SPRN_DBCR0) & ~(DBSR_DAC1R | DBSR_DAC1W
+                                                        | DBCR0_IDM));
+#endif
+
+	/* Clear the DABR */
+	set_dabr(0);
+
+	/* Deliver the signal to userspace */
+	info.si_signo = SIGTRAP;
+	info.si_errno = 0;
+	info.si_code = TRAP_HWBKPT;
+	info.si_addr = (void __user *)address;
+	force_sig_info(SIGTRAP, &info, current);
+}
+
 static DEFINE_PER_CPU(unsigned long, current_dabr);
 
 int set_dabr(unsigned long dabr)
@@ -256,6 +285,11 @@
 #if defined(CONFIG_PPC64) || defined(CONFIG_6xx)
 	mtspr(SPRN_DABR, dabr);
 #endif
+
+#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
+	mtspr(SPRN_DAC1, dabr);
+#endif
+
 	return 0;
 }
 
@@ -330,6 +364,12 @@
 	if (unlikely(__get_cpu_var(current_dabr) != new->thread.dabr))
 		set_dabr(new->thread.dabr);
 
+#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
+	/* If new thread DAC (HW breakpoint) is the same then leave it.  */
+	if (new->thread.dabr)
+		set_dabr(new->thread.dabr);
+#endif
+
 	new_thread = &new->thread;
 	old_thread = &current->thread;
 
@@ -518,6 +558,10 @@
 	if (current->thread.dabr) {
 		current->thread.dabr = 0;
 		set_dabr(0);
+
+#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
+		current->thread.dbcr0 &= ~(DBSR_DAC1R | DBSR_DAC1W);
+#endif
 	}
 }
 
Index: linux-2.6.25-oldpatch/arch/powerpc/kernel/ptrace.c
===================================================================
--- linux-2.6.25-oldpatch.orig/arch/powerpc/kernel/ptrace.c	2008-06-20 02:48:19.000000000 -0700
+++ linux-2.6.25-oldpatch/arch/powerpc/kernel/ptrace.c	2008-06-20 05:10:59.000000000 -0700
@@ -616,7 +616,7 @@
 
 	if (regs != NULL) {
 #if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
-		task->thread.dbcr0 = DBCR0_IDM | DBCR0_IC;
+		task->thread.dbcr0 |= DBCR0_IDM | DBCR0_IC;
 		regs->msr |= MSR_DE;
 #else
 		regs->msr |= MSR_SE;
@@ -629,9 +629,16 @@
 {
 	struct pt_regs *regs = task->thread.regs;
 
+
+#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
+	/* If DAC then do not single step, skip.  */
+	if (task->thread.dabr)
+		return;
+#endif
+
 	if (regs != NULL) {
 #if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
-		task->thread.dbcr0 = 0;
+		task->thread.dbcr0 &= ~(DBCR0_IC | DBCR0_IDM);
 		regs->msr &= ~MSR_DE;
 #else
 		regs->msr &= ~MSR_SE;
@@ -640,22 +647,79 @@
 	clear_tsk_thread_flag(task, TIF_SINGLESTEP);
 }
 
-static int ptrace_set_debugreg(struct task_struct *task, unsigned long addr,
+static int ptrace_get_debugreg(struct task_struct *task, unsigned long data)
+{
+	int ret;
+
+	ret = put_user(task->thread.dabr,
+			(unsigned long __user *)data);
+	return ret;
+}
+
+int ptrace_set_debugreg(struct task_struct *task, unsigned long addr,
 			       unsigned long data)
 {
-	/* We only support one DABR and no IABRS at the moment */
+	/* For ppc64 we support one DABR and no IABR's at the moment (ppc64).
+	   For embedded processors we support one DAC and no IAC's
+	   at the moment.  */
 	if (addr > 0)
 		return -EINVAL;
 
-	/* The bottom 3 bits are flags */
 	if ((data & ~0x7UL) >= TASK_SIZE)
 		return -EIO;
 
-	/* Ensure translation is on */
+#ifdef CONFIG_PPC64
+
+	/* For processors using DABR (i.e. 970), the bottom 3 bits are flags.
+	   It was assumed, on previous implementations, that 3 bits were
+	   passed together with the data address, fitting the design of the
+	   DABR register, as follows:
+
+	   bit 0: Read flag
+	   bit 1: Write flag
+	   bit 2: Breakpoint translation
+
+	   Thus, we use them here as so.  */
+
+	/* Ensure breakpoint translation bit is set.  */
 	if (data && !(data & DABR_TRANSLATION))
 		return -EIO;
 
+	/* Move contents to the DABR register.  */
 	task->thread.dabr = data;
+
+#endif
+#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
+
+	/* As described above, we assume 3 bits are passed with the data
+	   address, so mask them so we can set the DAC register with the
+	   correct address.  */
+	/* DAC's hold the whole address without any mode flags.  */
+	task->thread.dabr = data & ~0x3UL;
+
+	if (task->thread.dabr == 0) {
+		task->thread.dbcr0 &= ~(DBSR_DAC1R | DBSR_DAC1W | DBCR0_IDM);
+		task->thread.regs->msr &= ~MSR_DE;
+		return 0;
+	}
+
+        /* Read or Write bits must be set.  */
+
+        if (!(data & 0x3UL))
+                return -EINVAL;
+
+	/* Set the Internal Debugging flag (IDM bit 1) for the DBCR0
+	   register.  */
+	task->thread.dbcr0 = DBCR0_IDM;
+
+	/* Check for write and read flags and set DBCR0 accordingly.  */
+	if (data & 0x1UL)
+		task->thread.dbcr0 |= DBSR_DAC1R;
+	if (data & 0x2UL)
+		task->thread.dbcr0 |= DBSR_DAC1W;
+
+	task->thread.regs->msr |= MSR_DE;
+#endif
 	return 0;
 }
 
Index: linux-2.6.25-oldpatch/arch/powerpc/kernel/signal.c
===================================================================
--- linux-2.6.25-oldpatch.orig/arch/powerpc/kernel/signal.c	2008-06-20 02:48:19.000000000 -0700
+++ linux-2.6.25-oldpatch/arch/powerpc/kernel/signal.c	2008-06-20 02:51:16.000000000 -0700
@@ -144,8 +144,12 @@
 	 * user space. The DABR will have been cleared if it
 	 * triggered inside the kernel.
 	 */
-	if (current->thread.dabr)
+	if (current->thread.dabr) {
 		set_dabr(current->thread.dabr);
+#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
+		mtspr(SPRN_DBCR0, current->thread.dbcr0);
+#endif
+	}
 
 	if (is32) {
         	if (ka.sa.sa_flags & SA_SIGINFO)
Index: linux-2.6.25-oldpatch/arch/powerpc/kernel/traps.c
===================================================================
--- linux-2.6.25-oldpatch.orig/arch/powerpc/kernel/traps.c	2008-06-20 02:48:19.000000000 -0700
+++ linux-2.6.25-oldpatch/arch/powerpc/kernel/traps.c	2008-06-20 02:54:37.000000000 -0700
@@ -1045,6 +1045,21 @@
 				return;
 		}
 		_exception(SIGTRAP, regs, TRAP_TRACE, 0);
+	} else if (debug_status & (DBSR_DAC1R | DBSR_DAC1W)) {
+		regs->msr &= ~MSR_DE;
+	        printk("\nWatchpoint Triggered\n");
+		if (user_mode(regs)) {
+			current->thread.dbcr0 &= ~(DBSR_DAC1R | DBSR_DAC1W |
+								DBCR0_IDM);
+		} else {
+			/* Disable DAC interupts.  */
+			mtspr(SPRN_DBCR0, mfspr(SPRN_DBCR0) & ~(DBSR_DAC1R |
+							DBSR_DAC1W | DBCR0_IDM));
+			/* Clear the DAC event.  */
+			mtspr(SPRN_DBSR, (DBSR_DAC1R | DBSR_DAC1W));
+		}
+		/* Setup and send the trap to the handler.  */
+		do_dabr(regs, mfspr(SPRN_DAC1), debug_status);
 	}
 }
 #endif /* CONFIG_4xx || CONFIG_BOOKE */
Index: linux-2.6.25-oldpatch/arch/powerpc/kernel/entry_32.S
===================================================================
--- linux-2.6.25-oldpatch.orig/arch/powerpc/kernel/entry_32.S	2008-06-20 02:48:19.000000000 -0700
+++ linux-2.6.25-oldpatch/arch/powerpc/kernel/entry_32.S	2008-06-20 02:51:16.000000000 -0700
@@ -112,7 +112,7 @@
 	/* Check to see if the dbcr0 register is set up to debug.  Use the
 	   internal debug mode bit to do this. */
 	lwz	r12,THREAD_DBCR0(r12)
-	andis.	r12,r12,DBCR0_IDM@h
+	andis.	r12,r12,(DBCR0_IDM  | DBSR_DAC1R | DBSR_DAC1W)@h
 	beq+	3f
 	/* From user and task is ptraced - load up global dbcr0 */
 	li	r12,-1			/* clear all pending debug events */
@@ -248,7 +248,7 @@
 	/* If the process has its own DBCR0 value, load it up.  The internal
 	   debug mode bit tells us that dbcr0 should be loaded. */
 	lwz	r0,THREAD+THREAD_DBCR0(r2)
-	andis.	r10,r0,DBCR0_IDM@h
+	andis.	r10,r0,(DBCR0_IDM  | DBSR_DAC1R | DBSR_DAC1W)@h
 	bnel-	load_dbcr0
 #endif
 #ifdef CONFIG_44x
@@ -676,7 +676,7 @@
 	/* Check whether this process has its own DBCR0 value.  The internal
 	   debug mode bit tells us that dbcr0 should be loaded. */
 	lwz	r0,THREAD+THREAD_DBCR0(r2)
-	andis.	r10,r0,DBCR0_IDM@h
+	andis.	r10,r0,(DBCR0_IDM  | DBSR_DAC1R | DBSR_DAC1W)@h
 	bnel-	load_dbcr0
 #endif
 
Index: linux-2.6.25-oldpatch/arch/powerpc/mm/fault.c
===================================================================
--- linux-2.6.25-oldpatch.orig/arch/powerpc/mm/fault.c	2008-06-20 02:48:19.000000000 -0700
+++ linux-2.6.25-oldpatch/arch/powerpc/mm/fault.c	2008-06-20 02:51:16.000000000 -0700
@@ -100,31 +100,6 @@
 	return 0;
 }
 
-#if !(defined(CONFIG_4xx) || defined(CONFIG_BOOKE))
-static void do_dabr(struct pt_regs *regs, unsigned long address,
-		    unsigned long error_code)
-{
-	siginfo_t info;
-
-	if (notify_die(DIE_DABR_MATCH, "dabr_match", regs, error_code,
-			11, SIGSEGV) == NOTIFY_STOP)
-		return;
-
-	if (debugger_dabr_match(regs))
-		return;
-
-	/* Clear the DABR */
-	set_dabr(0);
-
-	/* Deliver the signal to userspace */
-	info.si_signo = SIGTRAP;
-	info.si_errno = 0;
-	info.si_code = TRAP_HWBKPT;
-	info.si_addr = (void __user *)address;
-	force_sig_info(SIGTRAP, &info, current);
-}
-#endif /* !(CONFIG_4xx || CONFIG_BOOKE)*/
-
 /*
  * For 600- and 800-family processors, the error_code parameter is DSISR
  * for a data fault, SRR1 for an instruction fault. For 400-family processors
Index: linux-2.6.25-oldpatch/include/asm-powerpc/system.h
===================================================================
--- linux-2.6.25-oldpatch.orig/include/asm-powerpc/system.h	2008-06-20 02:48:19.000000000 -0700
+++ linux-2.6.25-oldpatch/include/asm-powerpc/system.h	2008-06-20 02:51:16.000000000 -0700
@@ -103,6 +103,8 @@
 #endif
 
 extern int set_dabr(unsigned long dabr);
+extern void do_dabr(struct pt_regs *regs, unsigned long address,
+                    unsigned long error_code);
 extern void print_backtrace(unsigned long *);
 extern void show_regs(struct pt_regs * regs);
 extern void flush_instruction_cache(void);

Re: [RFC] 4xx hardware watchpoint support

[Luis Machado]

Hi guys,

Did anyone have a chance to go over this patch? Looking forward to
receive feedbacks on it.

Thanks!

Regards,
Luis

On Fri, 2008-06-20 at 17:14 -0300, Luis Machado wrote:
> On Thu, 2008-05-22 at 13:51 +1000, Paul Mackerras wrote:
> > Luis Machado writes:
> > 
> > > This is a patch that has been sitting idle for quite some time. I
> > > decided to move it further because it is something useful. It was
> > > originally written by Michel Darneille, based off of 2.6.16.
> > > 
> > > The original patch, though, was not compatible with the current DABR
> > > logic. DABR's are used to implement hardware watchpoint support for
> > > ppc64 processors (i.e. 970, Power5 etc). 4xx's have a different
> > > debugging register layout and needs to be handled differently (they two
> > > registers: DAC and DBCR0).
> > 
> > Yes, they are different, but they do essentially the same thing, so I
> > think we should try and unify the handling of the two.  Maybe you
> > could rename set_dabr() to set_hw_watchpoint() or something and make
> > it set DABR on 64-bit and "classic" 32-bit processors, and DAC on
> > 4xx/Book E processors.
> > 
> > Likewise, I don't think we need both a "dabr" field and a "dac" field
> > in the thread_struct - one should do.  Rename "dabr" to something else
> > if you feel that the "dabr" name is too ppc64-specific.  And I don't
> > think we need both __debugger_dabr_match and __debugger_dac_match.
> > 
> > > 5) This is something i'm worried about for future features. We currently
> > > have a way to support only Hardware Watchpoints, but not Hardware
> > > Breakpoints (on 64-bit processors that have a IABR register or 32-bit
> > > processors carrying the IAC register). Looking at the code, we don't
> > > differentiate a watchpoint from a breakpoint request. A ptrace call has
> > > currently 3 arguments: REQUEST, ADDR and DATA. We use REQUEST and DATA
> > > to set a hardware watchpoint. Maybe we could use the ADDR parameter to
> > > set a hardware breakpoint? Or use it to tell the kernel whether we want
> > > a hardware watchpoint or hardware breakpoint and then pass the address
> > > of the instruction/data through the DATA parameter? What do you think?
> > 
> > I would think there would be a different REQUEST value to mean "set a
> > hardware breakpoint".  Roland McGrath (cc'd) might be able to tell us
> > what other architectures do.
> > 
> > Paul.
> 
> Paul,
> 
> Follows a re-worked patch that unifies the handling of hardware
> watchpoint structures for DABR-based and DAC-based processors.
> 
> The flow is exactly the same for DABR-based processors.
> 
> As for the DAC-based code, i've eliminated the "dac" field from the
> thread structure, since now we use the "dabr" field to represent DAC1 of
> the DAC-based processors. As a consequence, i removed all the
> occurrences of "dac" throughout the code, using "dabr" in those cases.
> 
> The function "set_dabr" sets the correct register (DABR OR DAC) for each
> specific processor now, instead of only setting the value for DABR-based
> processors.
> 
> "do_dac" was merged with "do_dabr" as they were mostly equivalent pieces
> of code. I've moved "do_dabr" to "arch/powerpc/kernel/process.c" so it
> is visible for DAC-based code as well.
> 
> Tested on a Taishan 440GX with success.
> 
> Is it OK to leave it as "dabr" for both DABR and DAC? What do you think
> of the patch overall?
> 
> Thanks,
> 
> Best regards,
> Luis
> 
> 
> Index: linux-2.6.25-oldpatch/arch/powerpc/kernel/process.c
> ===================================================================
> --- linux-2.6.25-oldpatch.orig/arch/powerpc/kernel/process.c	2008-06-20 02:48:19.000000000 -0700
> +++ linux-2.6.25-oldpatch/arch/powerpc/kernel/process.c	2008-06-20 02:51:16.000000000 -0700
> @@ -241,6 +241,35 @@
>  }
>  #endif /* CONFIG_SMP */
> 
> +void do_dabr(struct pt_regs *regs, unsigned long address,
> +		    unsigned long error_code)
> +{
> +	siginfo_t info;
> +
> +#if !(defined(CONFIG_4xx) || defined(CONFIG_BOOKE))
> +	if (notify_die(DIE_DABR_MATCH, "dabr_match", regs, error_code,
> +			11, SIGSEGV) == NOTIFY_STOP)
> +		return;
> +
> +	if (debugger_dabr_match(regs))
> +		return;
> +#else
> +        /* Clear the DAC and struct entries.  One shot trigger.  */
> +        mtspr(SPRN_DBCR0, mfspr(SPRN_DBCR0) & ~(DBSR_DAC1R | DBSR_DAC1W
> +                                                        | DBCR0_IDM));
> +#endif
> +
> +	/* Clear the DABR */
> +	set_dabr(0);
> +
> +	/* Deliver the signal to userspace */
> +	info.si_signo = SIGTRAP;
> +	info.si_errno = 0;
> +	info.si_code = TRAP_HWBKPT;
> +	info.si_addr = (void __user *)address;
> +	force_sig_info(SIGTRAP, &info, current);
> +}
> +
>  static DEFINE_PER_CPU(unsigned long, current_dabr);
> 
>  int set_dabr(unsigned long dabr)
> @@ -256,6 +285,11 @@
>  #if defined(CONFIG_PPC64) || defined(CONFIG_6xx)
>  	mtspr(SPRN_DABR, dabr);
>  #endif
> +
> +#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
> +	mtspr(SPRN_DAC1, dabr);
> +#endif
> +
>  	return 0;
>  }
> 
> @@ -330,6 +364,12 @@
>  	if (unlikely(__get_cpu_var(current_dabr) != new->thread.dabr))
>  		set_dabr(new->thread.dabr);
> 
> +#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
> +	/* If new thread DAC (HW breakpoint) is the same then leave it.  */
> +	if (new->thread.dabr)
> +		set_dabr(new->thread.dabr);
> +#endif
> +
>  	new_thread = &new->thread;
>  	old_thread = &current->thread;
> 
> @@ -518,6 +558,10 @@
>  	if (current->thread.dabr) {
>  		current->thread.dabr = 0;
>  		set_dabr(0);
> +
> +#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
> +		current->thread.dbcr0 &= ~(DBSR_DAC1R | DBSR_DAC1W);
> +#endif
>  	}
>  }
> 
> Index: linux-2.6.25-oldpatch/arch/powerpc/kernel/ptrace.c
> ===================================================================
> --- linux-2.6.25-oldpatch.orig/arch/powerpc/kernel/ptrace.c	2008-06-20 02:48:19.000000000 -0700
> +++ linux-2.6.25-oldpatch/arch/powerpc/kernel/ptrace.c	2008-06-20 05:10:59.000000000 -0700
> @@ -616,7 +616,7 @@
> 
>  	if (regs != NULL) {
>  #if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
> -		task->thread.dbcr0 = DBCR0_IDM | DBCR0_IC;
> +		task->thread.dbcr0 |= DBCR0_IDM | DBCR0_IC;
>  		regs->msr |= MSR_DE;
>  #else
>  		regs->msr |= MSR_SE;
> @@ -629,9 +629,16 @@
>  {
>  	struct pt_regs *regs = task->thread.regs;
> 
> +
> +#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
> +	/* If DAC then do not single step, skip.  */
> +	if (task->thread.dabr)
> +		return;
> +#endif
> +
>  	if (regs != NULL) {
>  #if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
> -		task->thread.dbcr0 = 0;
> +		task->thread.dbcr0 &= ~(DBCR0_IC | DBCR0_IDM);
>  		regs->msr &= ~MSR_DE;
>  #else
>  		regs->msr &= ~MSR_SE;
> @@ -640,22 +647,79 @@
>  	clear_tsk_thread_flag(task, TIF_SINGLESTEP);
>  }
> 
> -static int ptrace_set_debugreg(struct task_struct *task, unsigned long addr,
> +static int ptrace_get_debugreg(struct task_struct *task, unsigned long data)
> +{
> +	int ret;
> +
> +	ret = put_user(task->thread.dabr,
> +			(unsigned long __user *)data);
> +	return ret;
> +}
> +
> +int ptrace_set_debugreg(struct task_struct *task, unsigned long addr,
>  			       unsigned long data)
>  {
> -	/* We only support one DABR and no IABRS at the moment */
> +	/* For ppc64 we support one DABR and no IABR's at the moment (ppc64).
> +	   For embedded processors we support one DAC and no IAC's
> +	   at the moment.  */
>  	if (addr > 0)
>  		return -EINVAL;
> 
> -	/* The bottom 3 bits are flags */
>  	if ((data & ~0x7UL) >= TASK_SIZE)
>  		return -EIO;
> 
> -	/* Ensure translation is on */
> +#ifdef CONFIG_PPC64
> +
> +	/* For processors using DABR (i.e. 970), the bottom 3 bits are flags.
> +	   It was assumed, on previous implementations, that 3 bits were
> +	   passed together with the data address, fitting the design of the
> +	   DABR register, as follows:
> +
> +	   bit 0: Read flag
> +	   bit 1: Write flag
> +	   bit 2: Breakpoint translation
> +
> +	   Thus, we use them here as so.  */
> +
> +	/* Ensure breakpoint translation bit is set.  */
>  	if (data && !(data & DABR_TRANSLATION))
>  		return -EIO;
> 
> +	/* Move contents to the DABR register.  */
>  	task->thread.dabr = data;
> +
> +#endif
> +#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
> +
> +	/* As described above, we assume 3 bits are passed with the data
> +	   address, so mask them so we can set the DAC register with the
> +	   correct address.  */
> +	/* DAC's hold the whole address without any mode flags.  */
> +	task->thread.dabr = data & ~0x3UL;
> +
> +	if (task->thread.dabr == 0) {
> +		task->thread.dbcr0 &= ~(DBSR_DAC1R | DBSR_DAC1W | DBCR0_IDM);
> +		task->thread.regs->msr &= ~MSR_DE;
> +		return 0;
> +	}
> +
> +        /* Read or Write bits must be set.  */
> +
> +        if (!(data & 0x3UL))
> +                return -EINVAL;
> +
> +	/* Set the Internal Debugging flag (IDM bit 1) for the DBCR0
> +	   register.  */
> +	task->thread.dbcr0 = DBCR0_IDM;
> +
> +	/* Check for write and read flags and set DBCR0 accordingly.  */
> +	if (data & 0x1UL)
> +		task->thread.dbcr0 |= DBSR_DAC1R;
> +	if (data & 0x2UL)
> +		task->thread.dbcr0 |= DBSR_DAC1W;
> +
> +	task->thread.regs->msr |= MSR_DE;
> +#endif
>  	return 0;
>  }
> 
> Index: linux-2.6.25-oldpatch/arch/powerpc/kernel/signal.c
> ===================================================================
> --- linux-2.6.25-oldpatch.orig/arch/powerpc/kernel/signal.c	2008-06-20 02:48:19.000000000 -0700
> +++ linux-2.6.25-oldpatch/arch/powerpc/kernel/signal.c	2008-06-20 02:51:16.000000000 -0700
> @@ -144,8 +144,12 @@
>  	 * user space. The DABR will have been cleared if it
>  	 * triggered inside the kernel.
>  	 */
> -	if (current->thread.dabr)
> +	if (current->thread.dabr) {
>  		set_dabr(current->thread.dabr);
> +#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
> +		mtspr(SPRN_DBCR0, current->thread.dbcr0);
> +#endif
> +	}
> 
>  	if (is32) {
>          	if (ka.sa.sa_flags & SA_SIGINFO)
> Index: linux-2.6.25-oldpatch/arch/powerpc/kernel/traps.c
> ===================================================================
> --- linux-2.6.25-oldpatch.orig/arch/powerpc/kernel/traps.c	2008-06-20 02:48:19.000000000 -0700
> +++ linux-2.6.25-oldpatch/arch/powerpc/kernel/traps.c	2008-06-20 02:54:37.000000000 -0700
> @@ -1045,6 +1045,21 @@
>  				return;
>  		}
>  		_exception(SIGTRAP, regs, TRAP_TRACE, 0);
> +	} else if (debug_status & (DBSR_DAC1R | DBSR_DAC1W)) {
> +		regs->msr &= ~MSR_DE;
> +	        printk("\nWatchpoint Triggered\n");
> +		if (user_mode(regs)) {
> +			current->thread.dbcr0 &= ~(DBSR_DAC1R | DBSR_DAC1W |
> +								DBCR0_IDM);
> +		} else {
> +			/* Disable DAC interupts.  */
> +			mtspr(SPRN_DBCR0, mfspr(SPRN_DBCR0) & ~(DBSR_DAC1R |
> +							DBSR_DAC1W | DBCR0_IDM));
> +			/* Clear the DAC event.  */
> +			mtspr(SPRN_DBSR, (DBSR_DAC1R | DBSR_DAC1W));
> +		}
> +		/* Setup and send the trap to the handler.  */
> +		do_dabr(regs, mfspr(SPRN_DAC1), debug_status);
>  	}
>  }
>  #endif /* CONFIG_4xx || CONFIG_BOOKE */
> Index: linux-2.6.25-oldpatch/arch/powerpc/kernel/entry_32.S
> ===================================================================
> --- linux-2.6.25-oldpatch.orig/arch/powerpc/kernel/entry_32.S	2008-06-20 02:48:19.000000000 -0700
> +++ linux-2.6.25-oldpatch/arch/powerpc/kernel/entry_32.S	2008-06-20 02:51:16.000000000 -0700
> @@ -112,7 +112,7 @@
>  	/* Check to see if the dbcr0 register is set up to debug.  Use the
>  	   internal debug mode bit to do this. */
>  	lwz	r12,THREAD_DBCR0(r12)
> -	andis.	r12,r12,DBCR0_IDM@h
> +	andis.	r12,r12,(DBCR0_IDM  | DBSR_DAC1R | DBSR_DAC1W)@h
>  	beq+	3f
>  	/* From user and task is ptraced - load up global dbcr0 */
>  	li	r12,-1			/* clear all pending debug events */
> @@ -248,7 +248,7 @@
>  	/* If the process has its own DBCR0 value, load it up.  The internal
>  	   debug mode bit tells us that dbcr0 should be loaded. */
>  	lwz	r0,THREAD+THREAD_DBCR0(r2)
> -	andis.	r10,r0,DBCR0_IDM@h
> +	andis.	r10,r0,(DBCR0_IDM  | DBSR_DAC1R | DBSR_DAC1W)@h
>  	bnel-	load_dbcr0
>  #endif
>  #ifdef CONFIG_44x
> @@ -676,7 +676,7 @@
>  	/* Check whether this process has its own DBCR0 value.  The internal
>  	   debug mode bit tells us that dbcr0 should be loaded. */
>  	lwz	r0,THREAD+THREAD_DBCR0(r2)
> -	andis.	r10,r0,DBCR0_IDM@h
> +	andis.	r10,r0,(DBCR0_IDM  | DBSR_DAC1R | DBSR_DAC1W)@h
>  	bnel-	load_dbcr0
>  #endif
> 
> Index: linux-2.6.25-oldpatch/arch/powerpc/mm/fault.c
> ===================================================================
> --- linux-2.6.25-oldpatch.orig/arch/powerpc/mm/fault.c	2008-06-20 02:48:19.000000000 -0700
> +++ linux-2.6.25-oldpatch/arch/powerpc/mm/fault.c	2008-06-20 02:51:16.000000000 -0700
> @@ -100,31 +100,6 @@
>  	return 0;
>  }
> 
> -#if !(defined(CONFIG_4xx) || defined(CONFIG_BOOKE))
> -static void do_dabr(struct pt_regs *regs, unsigned long address,
> -		    unsigned long error_code)
> -{
> -	siginfo_t info;
> -
> -	if (notify_die(DIE_DABR_MATCH, "dabr_match", regs, error_code,
> -			11, SIGSEGV) == NOTIFY_STOP)
> -		return;
> -
> -	if (debugger_dabr_match(regs))
> -		return;
> -
> -	/* Clear the DABR */
> -	set_dabr(0);
> -
> -	/* Deliver the signal to userspace */
> -	info.si_signo = SIGTRAP;
> -	info.si_errno = 0;
> -	info.si_code = TRAP_HWBKPT;
> -	info.si_addr = (void __user *)address;
> -	force_sig_info(SIGTRAP, &info, current);
> -}
> -#endif /* !(CONFIG_4xx || CONFIG_BOOKE)*/
> -
>  /*
>   * For 600- and 800-family processors, the error_code parameter is DSISR
>   * for a data fault, SRR1 for an instruction fault. For 400-family processors
> Index: linux-2.6.25-oldpatch/include/asm-powerpc/system.h
> ===================================================================
> --- linux-2.6.25-oldpatch.orig/include/asm-powerpc/system.h	2008-06-20 02:48:19.000000000 -0700
> +++ linux-2.6.25-oldpatch/include/asm-powerpc/system.h	2008-06-20 02:51:16.000000000 -0700
> @@ -103,6 +103,8 @@
>  #endif
> 
>  extern int set_dabr(unsigned long dabr);
> +extern void do_dabr(struct pt_regs *regs, unsigned long address,
> +                    unsigned long error_code);
>  extern void print_backtrace(unsigned long *);
>  extern void show_regs(struct pt_regs * regs);
>  extern void flush_instruction_cache(void);
> 
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [RFC] 4xx hardware watchpoint support

[Josh Boyer]

On Fri, 20 Jun 2008 17:14:53 -0300
Luis Machado <luisgpm@linux.vnet.ibm.com> wrote:
> 
> Follows a re-worked patch that unifies the handling of hardware
> watchpoint structures for DABR-based and DAC-based processors.
> 
> The flow is exactly the same for DABR-based processors.
> 
> As for the DAC-based code, i've eliminated the "dac" field from the
> thread structure, since now we use the "dabr" field to represent DAC1 of
> the DAC-based processors. As a consequence, i removed all the
> occurrences of "dac" throughout the code, using "dabr" in those cases.
> 
> The function "set_dabr" sets the correct register (DABR OR DAC) for each
> specific processor now, instead of only setting the value for DABR-based
> processors.
> 
> "do_dac" was merged with "do_dabr" as they were mostly equivalent pieces
> of code. I've moved "do_dabr" to "arch/powerpc/kernel/process.c" so it
> is visible for DAC-based code as well.
> 
> Tested on a Taishan 440GX with success.
> 
> Is it OK to leave it as "dabr" for both DABR and DAC? What do you think
> of the patch overall?

Aside from the comment I mention below (which really does need fixing),
the overall look of the patch seems good.

> Index: linux-2.6.25-oldpatch/arch/powerpc/kernel/process.c
> ===================================================================
> --- linux-2.6.25-oldpatch.orig/arch/powerpc/kernel/process.c	2008-06-20 02:48:19.000000000 -0700
> +++ linux-2.6.25-oldpatch/arch/powerpc/kernel/process.c	2008-06-20 02:51:16.000000000 -0700
> @@ -241,6 +241,35 @@
>  }
>  #endif /* CONFIG_SMP */
> 
> +void do_dabr(struct pt_regs *regs, unsigned long address,
> +		    unsigned long error_code)
> +{
> +	siginfo_t info;
> +
> +#if !(defined(CONFIG_4xx) || defined(CONFIG_BOOKE))
> +	if (notify_die(DIE_DABR_MATCH, "dabr_match", regs, error_code,
> +			11, SIGSEGV) == NOTIFY_STOP)
> +		return;
> +
> +	if (debugger_dabr_match(regs))
> +		return;
> +#else
> +        /* Clear the DAC and struct entries.  One shot trigger.  */
> +        mtspr(SPRN_DBCR0, mfspr(SPRN_DBCR0) & ~(DBSR_DAC1R | DBSR_DAC1W
> +                                                        | DBCR0_IDM));

This doesn't look right for how it's coded.  This would be the
CONFIG_4xx || CONFIG_BOOKE case, but CONFIG_4xx includes PowerPC 405.
That has a different bit layout among the DBCR registers.  Namely, on
405 you would be clearing the TDE and IAC1 events because the DAC
events are in DBCR1, not DBCR0.

<snip>

> Index: linux-2.6.25-oldpatch/arch/powerpc/kernel/signal.c
> ===================================================================
> --- linux-2.6.25-oldpatch.orig/arch/powerpc/kernel/signal.c	2008-06-20 02:48:19.000000000 -0700
> +++ linux-2.6.25-oldpatch/arch/powerpc/kernel/signal.c	2008-06-20 02:51:16.000000000 -0700
> @@ -144,8 +144,12 @@
>  	 * user space. The DABR will have been cleared if it
>  	 * triggered inside the kernel.
>  	 */
> -	if (current->thread.dabr)
> +	if (current->thread.dabr) {
>  		set_dabr(current->thread.dabr);
> +#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
> +		mtspr(SPRN_DBCR0, current->thread.dbcr0);
> +#endif

This has the same problem I mention above, as the 44x bit layout is
stored in dbcr0 throughout the code.

> +	}
> 
>  	if (is32) {
>          	if (ka.sa.sa_flags & SA_SIGINFO)
> Index: linux-2.6.25-oldpatch/arch/powerpc/kernel/traps.c
> ===================================================================
> --- linux-2.6.25-oldpatch.orig/arch/powerpc/kernel/traps.c	2008-06-20 02:48:19.000000000 -0700
> +++ linux-2.6.25-oldpatch/arch/powerpc/kernel/traps.c	2008-06-20 02:54:37.000000000 -0700
> @@ -1045,6 +1045,21 @@
>  				return;
>  		}
>  		_exception(SIGTRAP, regs, TRAP_TRACE, 0);
> +	} else if (debug_status & (DBSR_DAC1R | DBSR_DAC1W)) {
> +		regs->msr &= ~MSR_DE;
> +	        printk("\nWatchpoint Triggered\n");
> +		if (user_mode(regs)) {
> +			current->thread.dbcr0 &= ~(DBSR_DAC1R | DBSR_DAC1W |
> +								DBCR0_IDM);
> +		} else {
> +			/* Disable DAC interupts.  */
> +			mtspr(SPRN_DBCR0, mfspr(SPRN_DBCR0) & ~(DBSR_DAC1R |
> +							DBSR_DAC1W | DBCR0_IDM));
> +			/* Clear the DAC event.  */
> +			mtspr(SPRN_DBSR, (DBSR_DAC1R | DBSR_DAC1W));

And again here.

josh

Re: [RFC] 4xx hardware watchpoint support

[Luis Machado]

> This doesn't look right for how it's coded.  This would be the
> CONFIG_4xx || CONFIG_BOOKE case, but CONFIG_4xx includes PowerPC 405.
> That has a different bit layout among the DBCR registers.  Namely, on
> 405 you would be clearing the TDE and IAC1 events because the DAC
> events are in DBCR1, not DBCR0.

Maybe guarding the 405-specific parts in a separate "#if
defined(CONFIG_40x)" block will do?

Do you think it's worth to support this facility on 405's processors? If
so, i'll gladly work on a solution to it.

Regards,
Luis

Re: [RFC] 4xx hardware watchpoint support

[Josh Boyer]

On Mon, 21 Jul 2008 13:36:33 -0300
Luis Machado <luisgpm@linux.vnet.ibm.com> wrote:

> 
> > This doesn't look right for how it's coded.  This would be the
> > CONFIG_4xx || CONFIG_BOOKE case, but CONFIG_4xx includes PowerPC 405.
> > That has a different bit layout among the DBCR registers.  Namely, on
> > 405 you would be clearing the TDE and IAC1 events because the DAC
> > events are in DBCR1, not DBCR0.
> 
> Maybe guarding the 405-specific parts in a separate "#if
> defined(CONFIG_40x)" block will do?

That, or adding a small function to move the bits to the appropriate
registers (set_dbcr or set_dac_events).

> Do you think it's worth to support this facility on 405's processors? If
> so, i'll gladly work on a solution to it.

I would think so.  There's really no difference from a userspace
perspective, so gdb watchpoints could be valuable there too.  I'll
leave it up to you though.

josh

Re: [RFC] 4xx hardware watchpoint support

[Luis Machado]

Hi,

> That, or adding a small function to move the bits to the appropriate
> registers (set_dbcr or set_dac_events).
> 
> > Do you think it's worth to support this facility on 405's processors? If
> > so, i'll gladly work on a solution to it.
> 
> I would think so.  There's really no difference from a userspace
> perspective, so gdb watchpoints could be valuable there too.  I'll
> leave it up to you though.

As the 440 support is ready and the 405 needs additional tweaking due to
the use of DBCR1 instead of DBCR0 and due to a different position scheme
of the DAC1R/DAC1W flags inside DBCR1, i'd say we should include this
code and handle the 405 case later. 

We might have to handle it anyway if we're going to pursue the hardware
breakpoint interface work in the future.

I've fixed some formatting problems. Tested on a 440 Taishan board and
on a PPC970. Both worked as they should. Ok?


Signed-off-by: Luis Machado <luisgpm@br.ibm.com>

Index: linux-2.6.26/arch/powerpc/kernel/process.c
===================================================================
--- linux-2.6.26.orig/arch/powerpc/kernel/process.c	2008-07-20 16:56:57.000000000 -0700
+++ linux-2.6.26/arch/powerpc/kernel/process.c	2008-07-22 16:46:36.000000000 -0700
@@ -47,6 +47,8 @@
 #ifdef CONFIG_PPC64
 #include <asm/firmware.h>
 #endif
+#include <linux/kprobes.h>
+#include <linux/kdebug.h>
 
 extern unsigned long _get_SP(void);
 
@@ -239,6 +241,36 @@
 }
 #endif /* CONFIG_SMP */
 
+void do_dabr(struct pt_regs *regs, unsigned long address,
+		    unsigned long error_code)
+{
+	siginfo_t info;
+
+#if !(defined(CONFIG_4xx) || defined(CONFIG_BOOKE))
+	if (notify_die(DIE_DABR_MATCH, "dabr_match", regs, error_code,
+			11, SIGSEGV) == NOTIFY_STOP)
+		return;
+
+	if (debugger_dabr_match(regs))
+		return;
+
+	/* Clear the DAC and struct entries.  One shot trigger.  */
+#else /* (defined(CONFIG_4xx) || defined(CONFIG_BOOKE)) */
+	mtspr(SPRN_DBCR0, mfspr(SPRN_DBCR0) & ~(DBSR_DAC1R | DBSR_DAC1W
+							| DBCR0_IDM));
+#endif
+
+	/* Clear the DABR */
+	set_dabr(0);
+
+	/* Deliver the signal to userspace */
+	info.si_signo = SIGTRAP;
+	info.si_errno = 0;
+	info.si_code = TRAP_HWBKPT;
+	info.si_addr = (void __user *)address;
+	force_sig_info(SIGTRAP, &info, current);
+}
+
 static DEFINE_PER_CPU(unsigned long, current_dabr);
 
 int set_dabr(unsigned long dabr)
@@ -254,6 +286,11 @@
 #if defined(CONFIG_PPC64) || defined(CONFIG_6xx)
 	mtspr(SPRN_DABR, dabr);
 #endif
+
+#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
+	mtspr(SPRN_DAC1, dabr);
+#endif
+
 	return 0;
 }
 
@@ -337,6 +374,12 @@
 	if (unlikely(__get_cpu_var(current_dabr) != new->thread.dabr))
 		set_dabr(new->thread.dabr);
 
+#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
+	/* If new thread DAC (HW breakpoint) is the same then leave it.  */
+	if (new->thread.dabr)
+		set_dabr(new->thread.dabr);
+#endif
+
 	new_thread = &new->thread;
 	old_thread = &current->thread;
 
@@ -525,6 +568,10 @@
 	if (current->thread.dabr) {
 		current->thread.dabr = 0;
 		set_dabr(0);
+
+#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
+		current->thread.dbcr0 &= ~(DBSR_DAC1R | DBSR_DAC1W);
+#endif
 	}
 }
 
Index: linux-2.6.26/arch/powerpc/kernel/ptrace.c
===================================================================
--- linux-2.6.26.orig/arch/powerpc/kernel/ptrace.c	2008-07-20 16:56:57.000000000 -0700
+++ linux-2.6.26/arch/powerpc/kernel/ptrace.c	2008-07-22 16:41:24.000000000 -0700
@@ -703,7 +703,7 @@
 
 	if (regs != NULL) {
 #if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
-		task->thread.dbcr0 = DBCR0_IDM | DBCR0_IC;
+		task->thread.dbcr0 |= DBCR0_IDM | DBCR0_IC;
 		regs->msr |= MSR_DE;
 #else
 		regs->msr |= MSR_SE;
@@ -716,9 +716,16 @@
 {
 	struct pt_regs *regs = task->thread.regs;
 
+
+#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
+	/* If DAC then do not single step, skip.  */
+	if (task->thread.dabr)
+		return;
+#endif
+
 	if (regs != NULL) {
 #if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
-		task->thread.dbcr0 = 0;
+		task->thread.dbcr0 &= ~(DBCR0_IC | DBCR0_IDM);
 		regs->msr &= ~MSR_DE;
 #else
 		regs->msr &= ~MSR_SE;
@@ -727,22 +734,71 @@
 	clear_tsk_thread_flag(task, TIF_SINGLESTEP);
 }
 
-static int ptrace_set_debugreg(struct task_struct *task, unsigned long addr,
+int ptrace_set_debugreg(struct task_struct *task, unsigned long addr,
 			       unsigned long data)
 {
-	/* We only support one DABR and no IABRS at the moment */
+	/* For ppc64 we support one DABR and no IABR's at the moment (ppc64).
+	   For embedded processors we support one DAC and no IAC's
+	   at the moment.  */
 	if (addr > 0)
 		return -EINVAL;
 
-	/* The bottom 3 bits are flags */
 	if ((data & ~0x7UL) >= TASK_SIZE)
 		return -EIO;
 
-	/* Ensure translation is on */
+#ifdef CONFIG_PPC64
+
+	/* For processors using DABR (i.e. 970), the bottom 3 bits are flags.
+	   It was assumed, on previous implementations, that 3 bits were
+	   passed together with the data address, fitting the design of the
+	   DABR register, as follows:
+
+	   bit 0: Read flag
+	   bit 1: Write flag
+	   bit 2: Breakpoint translation
+
+	   Thus, we use them here as so.  */
+
+	/* Ensure breakpoint translation bit is set.  */
 	if (data && !(data & DABR_TRANSLATION))
 		return -EIO;
 
+	/* Move contents to the DABR register.  */
 	task->thread.dabr = data;
+
+#endif
+#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
+
+	/* As described above, it was assumed 3 bits were passed with the data
+	   address, but we will assume only the mode bits will be passed
+	   as to not cause alignment restrictions for DAC-based processors.  */
+	/* DAC's hold the whole address without any mode flags.  */
+	task->thread.dabr = data & ~0x3UL;
+
+	if (task->thread.dabr == 0) {
+		task->thread.dbcr0 &= ~(DBSR_DAC1R | DBSR_DAC1W | DBCR0_IDM);
+		task->thread.regs->msr &= ~MSR_DE;
+		return 0;
+	}
+
+	/* Read or Write bits must be set.  */
+
+	if (!(data & 0x3UL))
+		return -EINVAL;
+
+	/* Set the Internal Debugging flag (IDM bit 1) for the DBCR0
+	   register.  */
+	task->thread.dbcr0 = DBCR0_IDM;
+
+	/* Check for write and read flags and set DBCR0
+	   accordingly.  */
+	if (data & 0x1UL)
+		task->thread.dbcr0 |= DBSR_DAC1R;
+	if (data & 0x2UL)
+		task->thread.dbcr0 |= DBSR_DAC1W;
+
+	task->thread.regs->msr |= MSR_DE;
+#endif
 	return 0;
 }
 
Index: linux-2.6.26/arch/powerpc/kernel/signal.c
===================================================================
--- linux-2.6.26.orig/arch/powerpc/kernel/signal.c	2008-07-20 16:56:57.000000000 -0700
+++ linux-2.6.26/arch/powerpc/kernel/signal.c	2008-07-22 16:47:22.000000000 -0700
@@ -145,8 +145,12 @@
 	 * user space. The DABR will have been cleared if it
 	 * triggered inside the kernel.
 	 */
-	if (current->thread.dabr)
+	if (current->thread.dabr) {
 		set_dabr(current->thread.dabr);
+#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
+		mtspr(SPRN_DBCR0, current->thread.dbcr0);
+#endif
+	}
 
 	if (is32) {
         	if (ka.sa.sa_flags & SA_SIGINFO)
Index: linux-2.6.26/arch/powerpc/kernel/traps.c
===================================================================
--- linux-2.6.26.orig/arch/powerpc/kernel/traps.c	2008-07-20 16:56:57.000000000 -0700
+++ linux-2.6.26/arch/powerpc/kernel/traps.c	2008-07-22 16:49:28.000000000 -0700
@@ -1067,6 +1067,22 @@
 		}
 
 		_exception(SIGTRAP, regs, TRAP_TRACE, regs->nip);
+	} else if (debug_status & (DBSR_DAC1R | DBSR_DAC1W)) {
+		regs->msr &= ~MSR_DE;
+
+		if (user_mode(regs)) {
+			current->thread.dbcr0 &= ~(DBSR_DAC1R | DBSR_DAC1W |
+								DBCR0_IDM);
+		} else {
+			/* Disable DAC interupts.  */
+			mtspr(SPRN_DBCR0, mfspr(SPRN_DBCR0) & ~(DBSR_DAC1R |
+						DBSR_DAC1W | DBCR0_IDM));
+
+			/* Clear the DAC event.  */
+			mtspr(SPRN_DBSR, (DBSR_DAC1R | DBSR_DAC1W));
+		}
+		/* Setup and send the trap to the handler.  */
+		do_dabr(regs, mfspr(SPRN_DAC1), debug_status);
 	}
 }
 #endif /* CONFIG_4xx || CONFIG_BOOKE */
Index: linux-2.6.26/arch/powerpc/kernel/entry_32.S
===================================================================
--- linux-2.6.26.orig/arch/powerpc/kernel/entry_32.S	2008-07-20 16:56:57.000000000 -0700
+++ linux-2.6.26/arch/powerpc/kernel/entry_32.S	2008-07-20 16:58:54.000000000 -0700
@@ -148,7 +148,7 @@
 	/* Check to see if the dbcr0 register is set up to debug.  Use the
 	   internal debug mode bit to do this. */
 	lwz	r12,THREAD_DBCR0(r12)
-	andis.	r12,r12,DBCR0_IDM@h
+	andis.	r12,r12,(DBCR0_IDM  | DBSR_DAC1R | DBSR_DAC1W)@h
 	beq+	3f
 	/* From user and task is ptraced - load up global dbcr0 */
 	li	r12,-1			/* clear all pending debug events */
@@ -292,7 +292,7 @@
 	/* If the process has its own DBCR0 value, load it up.  The internal
 	   debug mode bit tells us that dbcr0 should be loaded. */
 	lwz	r0,THREAD+THREAD_DBCR0(r2)
-	andis.	r10,r0,DBCR0_IDM@h
+	andis.	r10,r0,(DBCR0_IDM  | DBSR_DAC1R | DBSR_DAC1W)@h
 	bnel-	load_dbcr0
 #endif
 #ifdef CONFIG_44x
@@ -720,7 +720,7 @@
 	/* Check whether this process has its own DBCR0 value.  The internal
 	   debug mode bit tells us that dbcr0 should be loaded. */
 	lwz	r0,THREAD+THREAD_DBCR0(r2)
-	andis.	r10,r0,DBCR0_IDM@h
+	andis.	r10,r0,(DBCR0_IDM  | DBSR_DAC1R | DBSR_DAC1W)@h
 	bnel-	load_dbcr0
 #endif
 
Index: linux-2.6.26/arch/powerpc/mm/fault.c
===================================================================
--- linux-2.6.26.orig/arch/powerpc/mm/fault.c	2008-07-20 16:56:57.000000000 -0700
+++ linux-2.6.26/arch/powerpc/mm/fault.c	2008-07-20 16:58:54.000000000 -0700
@@ -100,31 +100,6 @@
 	return 0;
 }
 
-#if !(defined(CONFIG_4xx) || defined(CONFIG_BOOKE))
-static void do_dabr(struct pt_regs *regs, unsigned long address,
-		    unsigned long error_code)
-{
-	siginfo_t info;
-
-	if (notify_die(DIE_DABR_MATCH, "dabr_match", regs, error_code,
-			11, SIGSEGV) == NOTIFY_STOP)
-		return;
-
-	if (debugger_dabr_match(regs))
-		return;
-
-	/* Clear the DABR */
-	set_dabr(0);
-
-	/* Deliver the signal to userspace */
-	info.si_signo = SIGTRAP;
-	info.si_errno = 0;
-	info.si_code = TRAP_HWBKPT;
-	info.si_addr = (void __user *)address;
-	force_sig_info(SIGTRAP, &info, current);
-}
-#endif /* !(CONFIG_4xx || CONFIG_BOOKE)*/
-
 /*
  * For 600- and 800-family processors, the error_code parameter is DSISR
  * for a data fault, SRR1 for an instruction fault. For 400-family processors
Index: linux-2.6.26/include/asm-powerpc/system.h
===================================================================
--- linux-2.6.26.orig/include/asm-powerpc/system.h	2008-07-20 16:57:09.000000000 -0700
+++ linux-2.6.26/include/asm-powerpc/system.h	2008-07-20 23:00:36.000000000 -0700
@@ -110,6 +110,8 @@
 #endif
 
 extern int set_dabr(unsigned long dabr);
+extern void do_dabr(struct pt_regs *regs, unsigned long address,
+		    unsigned long error_code);
 extern void print_backtrace(unsigned long *);
 extern void show_regs(struct pt_regs * regs);
 extern void flush_instruction_cache(void);

Re: [RFC] 4xx hardware watchpoint support

[Kumar Gala]

On Jul 22, 2008, at 8:47 PM, Luis Machado wrote:

> Hi,
>
>> That, or adding a small function to move the bits to the appropriate
>> registers (set_dbcr or set_dac_events).
>>
>>> Do you think it's worth to support this facility on 405's  
>>> processors? If
>>> so, i'll gladly work on a solution to it.
>>
>> I would think so.  There's really no difference from a userspace
>> perspective, so gdb watchpoints could be valuable there too.  I'll
>> leave it up to you though.
>
> As the 440 support is ready and the 405 needs additional tweaking  
> due to
> the use of DBCR1 instead of DBCR0 and due to a different position  
> scheme
> of the DAC1R/DAC1W flags inside DBCR1, i'd say we should include this
> code and handle the 405 case later.
>
> We might have to handle it anyway if we're going to pursue the  
> hardware
> breakpoint interface work in the future.
>
> I've fixed some formatting problems. Tested on a 440 Taishan board and
> on a PPC970. Both worked as they should. Ok?

I'd like to test this on some Freescale Book-e parts.  Is there a gdb  
patch or some user space ptrace test you have?

- k

Re: [RFC] 4xx hardware watchpoint support

[Christoph Hellwig]

On Tue, Jul 22, 2008 at 10:47:58PM -0300, Luis Machado wrote:
> +void do_dabr(struct pt_regs *regs, unsigned long address,
> +		    unsigned long error_code)
> +{
> +	siginfo_t info;
> +
> +#if !(defined(CONFIG_4xx) || defined(CONFIG_BOOKE))
> +	if (notify_die(DIE_DABR_MATCH, "dabr_match", regs, error_code,
> +			11, SIGSEGV) == NOTIFY_STOP)
> +		return;
> +
> +	if (debugger_dabr_match(regs))
> +		return;
> +
> +	/* Clear the DAC and struct entries.  One shot trigger.  */
> +#else /* (defined(CONFIG_4xx) || defined(CONFIG_BOOKE)) */
> +	mtspr(SPRN_DBCR0, mfspr(SPRN_DBCR0) & ~(DBSR_DAC1R | DBSR_DAC1W
> +							| DBCR0_IDM));
> +#endif

Some comment, first the above negate conditional
looks rather ugly, I'd rather do a

#if defined(CONFIG_4xx) || defined(CONFIG_BOOKE)
	dbcr0 case
#else
	dabr case
#endif

second I wonder why we have the notify_die only for one case, that seems
rather odd.  Looking further the notify_die is even more odd because
DIE_DABR_MATCH doesn't actually appear anywhere else in the kernel.
I'd suggest simply removing it.

> +	/* For processors using DABR (i.e. 970), the bottom 3 bits are flags.
> +	   It was assumed, on previous implementations, that 3 bits were
> +	   passed together with the data address, fitting the design of the
> +	   DABR register, as follows:
> +
> +	   bit 0: Read flag
> +	   bit 1: Write flag
> +	   bit 2: Breakpoint translation
> +
> +	   Thus, we use them here as so.  */

Can you redo this in the normal Linux comment style, ala:

	/*
	 * For processors using DABR (i.e. 970), the bottom 3 bits are flags.
	 *  It was assumed, on previous implementations, that 3 bits were
	 *  passed together with the data address, fitting the design of the
	 *  DABR register, as follows:
	 *
	 *  bit 0: Read flag
	 *  bit 1: Write flag
	 *  bit 2: Breakpoint translation
	 *
	 *  Thus, we use them here as so.
	 */

and similar in few other places.

Re: [RFC] 4xx hardware watchpoint support

[Luis Machado]

> Some comment, first the above negate conditional
> looks rather ugly, I'd rather do a
> 
> #if defined(CONFIG_4xx) || defined(CONFIG_BOOKE)
> 	dbcr0 case
> #else
> 	dabr case
> #endif

Yes, it makes sense. I'll switch it around.

> second I wonder why we have the notify_die only for one case, that seems
> rather odd.  Looking further the notify_die is even more odd because
> DIE_DABR_MATCH doesn't actually appear anywhere else in the kernel.
> I'd suggest simply removing it.

DIE_DABR_MATCH doesn't appear anywhere else because there is only a
single function responsible for handling the DABR/DAC events on powerPC
with this modification. It would make sense to call this to both the
DAC/DABR cases though (i.e. taking it out of the #ifdef), what do you
think?

> Can you redo this in the normal Linux comment style, ala:
> 
> 	/*
> 	 * For processors using DABR (i.e. 970), the bottom 3 bits are flags.
> 	 *  It was assumed, on previous implementations, that 3 bits were
> 	 *  passed together with the data address, fitting the design of the
> 	 *  DABR register, as follows:
> 	 *
> 	 *  bit 0: Read flag
> 	 *  bit 1: Write flag
> 	 *  bit 2: Breakpoint translation
> 	 *
> 	 *  Thus, we use them here as so.
> 	 */
> 
> and similar in few other places.

Will do, thanks for reviewing this one.

Regards,
Luis

Re: [RFC] 4xx hardware watchpoint support

[Josh Boyer]

On Tue, 22 Jul 2008 22:47:58 -0300
Luis Machado <luisgpm@linux.vnet.ibm.com> wrote:

> Hi,
> 
> > That, or adding a small function to move the bits to the appropriate
> > registers (set_dbcr or set_dac_events).
> > 
> > > Do you think it's worth to support this facility on 405's processors? If
> > > so, i'll gladly work on a solution to it.
> > 
> > I would think so.  There's really no difference from a userspace
> > perspective, so gdb watchpoints could be valuable there too.  I'll
> > leave it up to you though.
> 
> As the 440 support is ready and the 405 needs additional tweaking due to
> the use of DBCR1 instead of DBCR0 and due to a different position scheme
> of the DAC1R/DAC1W flags inside DBCR1, i'd say we should include this
> code and handle the 405 case later. 

That's fine with me, but I have one question below then.

> Index: linux-2.6.26/arch/powerpc/kernel/signal.c
> ===================================================================
> --- linux-2.6.26.orig/arch/powerpc/kernel/signal.c	2008-07-20 16:56:57.000000000 -0700
> +++ linux-2.6.26/arch/powerpc/kernel/signal.c	2008-07-22 16:47:22.000000000 -0700
> @@ -145,8 +145,12 @@
>  	 * user space. The DABR will have been cleared if it
>  	 * triggered inside the kernel.
>  	 */
> -	if (current->thread.dabr)
> +	if (current->thread.dabr) {
>  		set_dabr(current->thread.dabr);
> +#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
> +		mtspr(SPRN_DBCR0, current->thread.dbcr0);
> +#endif

Shouldn't this (and other places) be:

#if defined(CONFIG_44x) || defined(CONFIG_BOOKE)

if you are going to exclude 40x for now?  Otherwise this is still
enabled on 405 and setting the wrong register.

josh

Re: [RFC] 4xx hardware watchpoint support

[Luis Machado]

On Wed, 2008-07-23 at 11:53 -0400, Josh Boyer wrote:
> Shouldn't this (and other places) be:
> 
> #if defined(CONFIG_44x) || defined(CONFIG_BOOKE)
> 
> if you are going to exclude 40x for now?  Otherwise this is still
> enabled on 405 and setting the wrong register.
> 
> josh

Yes, sorry. I wasn't aware of this specific define value. It makes
things easier to support 405's later.

Like so?

This addresses Christoph's comments as well.


Signed-off-by: Luis Machado <luisgpm@br.ibm.com>

Index: linux-2.6.26/arch/powerpc/kernel/entry_32.S
===================================================================
--- linux-2.6.26.orig/arch/powerpc/kernel/entry_32.S	2008-07-23 07:44:57.000000000 -0700
+++ linux-2.6.26/arch/powerpc/kernel/entry_32.S	2008-07-23 07:50:31.000000000 -0700
@@ -148,7 +148,7 @@
 	/* Check to see if the dbcr0 register is set up to debug.  Use the
 	   internal debug mode bit to do this. */
 	lwz	r12,THREAD_DBCR0(r12)
-	andis.	r12,r12,DBCR0_IDM@h
+	andis.	r12,r12,(DBCR0_IDM  | DBSR_DAC1R | DBSR_DAC1W)@h
 	beq+	3f
 	/* From user and task is ptraced - load up global dbcr0 */
 	li	r12,-1			/* clear all pending debug events */
@@ -292,7 +292,7 @@
 	/* If the process has its own DBCR0 value, load it up.  The internal
 	   debug mode bit tells us that dbcr0 should be loaded. */
 	lwz	r0,THREAD+THREAD_DBCR0(r2)
-	andis.	r10,r0,DBCR0_IDM@h
+	andis.	r10,r0,(DBCR0_IDM  | DBSR_DAC1R | DBSR_DAC1W)@h
 	bnel-	load_dbcr0
 #endif
 #ifdef CONFIG_44x
@@ -720,7 +720,7 @@
 	/* Check whether this process has its own DBCR0 value.  The internal
 	   debug mode bit tells us that dbcr0 should be loaded. */
 	lwz	r0,THREAD+THREAD_DBCR0(r2)
-	andis.	r10,r0,DBCR0_IDM@h
+	andis.	r10,r0,(DBCR0_IDM  | DBSR_DAC1R | DBSR_DAC1W)@h
 	bnel-	load_dbcr0
 #endif
 
Index: linux-2.6.26/arch/powerpc/kernel/process.c
===================================================================
--- linux-2.6.26.orig/arch/powerpc/kernel/process.c	2008-07-23 07:44:57.000000000 -0700
+++ linux-2.6.26/arch/powerpc/kernel/process.c	2008-07-23 07:50:31.000000000 -0700
@@ -47,6 +47,8 @@
 #ifdef CONFIG_PPC64
 #include <asm/firmware.h>
 #endif
+#include <linux/kprobes.h>
+#include <linux/kdebug.h>
 
 extern unsigned long _get_SP(void);
 
@@ -239,6 +241,35 @@
 }
 #endif /* CONFIG_SMP */
 
+void do_dabr(struct pt_regs *regs, unsigned long address,
+		    unsigned long error_code)
+{
+	siginfo_t info;
+
+	if (notify_die(DIE_DABR_MATCH, "dabr_match", regs, error_code,
+			11, SIGSEGV) == NOTIFY_STOP)
+		return;
+
+	if (debugger_dabr_match(regs))
+		return;
+
+	/* Clear the DAC and struct entries.  One shot trigger */
+#if (defined(CONFIG_44x) || defined(CONFIG_BOOKE))
+	mtspr(SPRN_DBCR0, mfspr(SPRN_DBCR0) & ~(DBSR_DAC1R | DBSR_DAC1W
+							| DBCR0_IDM));
+#endif
+
+	/* Clear the DABR */
+	set_dabr(0);
+
+	/* Deliver the signal to userspace */
+	info.si_signo = SIGTRAP;
+	info.si_errno = 0;
+	info.si_code = TRAP_HWBKPT;
+	info.si_addr = (void __user *)address;
+	force_sig_info(SIGTRAP, &info, current);
+}
+
 static DEFINE_PER_CPU(unsigned long, current_dabr);
 
 int set_dabr(unsigned long dabr)
@@ -254,6 +285,11 @@
 #if defined(CONFIG_PPC64) || defined(CONFIG_6xx)
 	mtspr(SPRN_DABR, dabr);
 #endif
+
+#if defined(CONFIG_44x) || defined(CONFIG_BOOKE)
+	mtspr(SPRN_DAC1, dabr);
+#endif
+
 	return 0;
 }
 
@@ -337,6 +373,12 @@
 	if (unlikely(__get_cpu_var(current_dabr) != new->thread.dabr))
 		set_dabr(new->thread.dabr);
 
+#if defined(CONFIG_44x) || defined(CONFIG_BOOKE)
+	/* If new thread DAC (HW breakpoint) is the same then leave it */
+	if (new->thread.dabr)
+		set_dabr(new->thread.dabr);
+#endif
+
 	new_thread = &new->thread;
 	old_thread = &current->thread;
 
@@ -525,6 +567,10 @@
 	if (current->thread.dabr) {
 		current->thread.dabr = 0;
 		set_dabr(0);
+
+#if defined(CONFIG_44x) || defined(CONFIG_BOOKE)
+		current->thread.dbcr0 &= ~(DBSR_DAC1R | DBSR_DAC1W);
+#endif
 	}
 }
 
Index: linux-2.6.26/arch/powerpc/kernel/ptrace.c
===================================================================
--- linux-2.6.26.orig/arch/powerpc/kernel/ptrace.c	2008-07-23 07:44:57.000000000 -0700
+++ linux-2.6.26/arch/powerpc/kernel/ptrace.c	2008-07-23 07:53:45.000000000 -0700
@@ -703,7 +703,7 @@
 
 	if (regs != NULL) {
 #if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
-		task->thread.dbcr0 = DBCR0_IDM | DBCR0_IC;
+		task->thread.dbcr0 |= DBCR0_IDM | DBCR0_IC;
 		regs->msr |= MSR_DE;
 #else
 		regs->msr |= MSR_SE;
@@ -716,9 +716,16 @@
 {
 	struct pt_regs *regs = task->thread.regs;
 
+
+#if defined(CONFIG_44x) || defined(CONFIG_BOOKE)
+	/* If DAC then do not single step, skip */
+	if (task->thread.dabr)
+		return;
+#endif
+
 	if (regs != NULL) {
 #if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
-		task->thread.dbcr0 = 0;
+		task->thread.dbcr0 &= ~(DBCR0_IC | DBCR0_IDM);
 		regs->msr &= ~MSR_DE;
 #else
 		regs->msr &= ~MSR_SE;
@@ -727,22 +734,75 @@
 	clear_tsk_thread_flag(task, TIF_SINGLESTEP);
 }
 
-static int ptrace_set_debugreg(struct task_struct *task, unsigned long addr,
+int ptrace_set_debugreg(struct task_struct *task, unsigned long addr,
 			       unsigned long data)
 {
-	/* We only support one DABR and no IABRS at the moment */
+	/* For ppc64 we support one DABR and no IABR's at the moment (ppc64).
+	 *  For embedded processors we support one DAC and no IAC's at the
+	 *  moment.
+	 */
 	if (addr > 0)
 		return -EINVAL;
 
-	/* The bottom 3 bits are flags */
 	if ((data & ~0x7UL) >= TASK_SIZE)
 		return -EIO;
 
-	/* Ensure translation is on */
+#ifdef CONFIG_PPC64
+
+	/* For processors using DABR (i.e. 970), the bottom 3 bits are flags.
+	 *  It was assumed, on previous implementations, that 3 bits were
+	 *  passed together with the data address, fitting the design of the
+	 *  DABR register, as follows:
+	 *
+	 *  bit 0: Read flag
+	 *  bit 1: Write flag
+	 *  bit 2: Breakpoint translation
+	 *
+	 *  Thus, we use them here as so.
+	 */
+
+	/* Ensure breakpoint translation bit is set */
 	if (data && !(data & DABR_TRANSLATION))
 		return -EIO;
 
+	/* Move contents to the DABR register */
 	task->thread.dabr = data;
+
+#endif
+#if defined(CONFIG_44x) || defined(CONFIG_BOOKE)
+
+	/* As described above, it was assumed 3 bits were passed with the data
+	 *  address, but we will assume only the mode bits will be passed
+	 *  as to not cause alignment restrictions for DAC-based processors.
+	 */
+
+	/* DAC's hold the whole address without any mode flags */
+	task->thread.dabr = data & ~0x3UL;
+
+	if (task->thread.dabr == 0) {
+		task->thread.dbcr0 &= ~(DBSR_DAC1R | DBSR_DAC1W | DBCR0_IDM);
+		task->thread.regs->msr &= ~MSR_DE;
+		return 0;
+	}
+
+	/* Read or Write bits must be set */
+
+	if (!(data & 0x3UL))
+		return -EINVAL;
+
+	/* Set the Internal Debugging flag (IDM bit 1) for the DBCR0
+	   register */
+	task->thread.dbcr0 = DBCR0_IDM;
+
+	/* Check for write and read flags and set DBCR0
+	   accordingly */
+	if (data & 0x1UL)
+		task->thread.dbcr0 |= DBSR_DAC1R;
+	if (data & 0x2UL)
+		task->thread.dbcr0 |= DBSR_DAC1W;
+
+	task->thread.regs->msr |= MSR_DE;
+#endif
 	return 0;
 }
 
Index: linux-2.6.26/arch/powerpc/kernel/signal.c
===================================================================
--- linux-2.6.26.orig/arch/powerpc/kernel/signal.c	2008-07-23 07:44:57.000000000 -0700
+++ linux-2.6.26/arch/powerpc/kernel/signal.c	2008-07-23 07:50:31.000000000 -0700
@@ -145,8 +145,12 @@
 	 * user space. The DABR will have been cleared if it
 	 * triggered inside the kernel.
 	 */
-	if (current->thread.dabr)
+	if (current->thread.dabr) {
 		set_dabr(current->thread.dabr);
+#if defined(CONFIG_44x) || defined(CONFIG_BOOKE)
+		mtspr(SPRN_DBCR0, current->thread.dbcr0);
+#endif
+	}
 
 	if (is32) {
         	if (ka.sa.sa_flags & SA_SIGINFO)
Index: linux-2.6.26/arch/powerpc/kernel/traps.c
===================================================================
--- linux-2.6.26.orig/arch/powerpc/kernel/traps.c	2008-07-23 07:44:57.000000000 -0700
+++ linux-2.6.26/arch/powerpc/kernel/traps.c	2008-07-23 07:50:31.000000000 -0700
@@ -1067,6 +1067,22 @@
 		}
 
 		_exception(SIGTRAP, regs, TRAP_TRACE, regs->nip);
+	} else if (debug_status & (DBSR_DAC1R | DBSR_DAC1W)) {
+		regs->msr &= ~MSR_DE;
+
+		if (user_mode(regs)) {
+			current->thread.dbcr0 &= ~(DBSR_DAC1R | DBSR_DAC1W |
+								DBCR0_IDM);
+		} else {
+			/* Disable DAC interupts */
+			mtspr(SPRN_DBCR0, mfspr(SPRN_DBCR0) & ~(DBSR_DAC1R |
+						DBSR_DAC1W | DBCR0_IDM));
+
+			/* Clear the DAC event */
+			mtspr(SPRN_DBSR, (DBSR_DAC1R | DBSR_DAC1W));
+		}
+		/* Setup and send the trap to the handler */
+		do_dabr(regs, mfspr(SPRN_DAC1), debug_status);
 	}
 }
 #endif /* CONFIG_4xx || CONFIG_BOOKE */
Index: linux-2.6.26/arch/powerpc/mm/fault.c
===================================================================
--- linux-2.6.26.orig/arch/powerpc/mm/fault.c	2008-07-23 07:44:57.000000000 -0700
+++ linux-2.6.26/arch/powerpc/mm/fault.c	2008-07-23 07:50:31.000000000 -0700
@@ -100,31 +100,6 @@
 	return 0;
 }
 
-#if !(defined(CONFIG_4xx) || defined(CONFIG_BOOKE))
-static void do_dabr(struct pt_regs *regs, unsigned long address,
-		    unsigned long error_code)
-{
-	siginfo_t info;
-
-	if (notify_die(DIE_DABR_MATCH, "dabr_match", regs, error_code,
-			11, SIGSEGV) == NOTIFY_STOP)
-		return;
-
-	if (debugger_dabr_match(regs))
-		return;
-
-	/* Clear the DABR */
-	set_dabr(0);
-
-	/* Deliver the signal to userspace */
-	info.si_signo = SIGTRAP;
-	info.si_errno = 0;
-	info.si_code = TRAP_HWBKPT;
-	info.si_addr = (void __user *)address;
-	force_sig_info(SIGTRAP, &info, current);
-}
-#endif /* !(CONFIG_4xx || CONFIG_BOOKE)*/
-
 /*
  * For 600- and 800-family processors, the error_code parameter is DSISR
  * for a data fault, SRR1 for an instruction fault. For 400-family processors
Index: linux-2.6.26/include/asm-powerpc/system.h
===================================================================
--- linux-2.6.26.orig/include/asm-powerpc/system.h	2008-07-23 07:44:57.000000000 -0700
+++ linux-2.6.26/include/asm-powerpc/system.h	2008-07-23 07:50:31.000000000 -0700
@@ -110,6 +110,8 @@
 #endif
 
 extern int set_dabr(unsigned long dabr);
+extern void do_dabr(struct pt_regs *regs, unsigned long address,
+		    unsigned long error_code);
 extern void print_backtrace(unsigned long *);
 extern void show_regs(struct pt_regs * regs);
 extern void flush_instruction_cache(void);

Re: [RFC] 4xx hardware watchpoint support

[Kumar Gala]

On Jul 23, 2008, at 10:53 AM, Josh Boyer wrote:

> On Tue, 22 Jul 2008 22:47:58 -0300
> Luis Machado <luisgpm@linux.vnet.ibm.com> wrote:
>
>> Hi,
>>
>>> That, or adding a small function to move the bits to the appropriate
>>> registers (set_dbcr or set_dac_events).
>>>
>>>> Do you think it's worth to support this facility on 405's  
>>>> processors? If
>>>> so, i'll gladly work on a solution to it.
>>>
>>> I would think so.  There's really no difference from a userspace
>>> perspective, so gdb watchpoints could be valuable there too.  I'll
>>> leave it up to you though.
>>
>> As the 440 support is ready and the 405 needs additional tweaking  
>> due to
>> the use of DBCR1 instead of DBCR0 and due to a different position  
>> scheme
>> of the DAC1R/DAC1W flags inside DBCR1, i'd say we should include this
>> code and handle the 405 case later.
>
> That's fine with me, but I have one question below then.
>
>> Index: linux-2.6.26/arch/powerpc/kernel/signal.c
>> ===================================================================
>> --- linux-2.6.26.orig/arch/powerpc/kernel/signal.c	2008-07-20  
>> 16:56:57.000000000 -0700
>> +++ linux-2.6.26/arch/powerpc/kernel/signal.c	2008-07-22  
>> 16:47:22.000000000 -0700
>> @@ -145,8 +145,12 @@
>> 	 * user space. The DABR will have been cleared if it
>> 	 * triggered inside the kernel.
>> 	 */
>> -	if (current->thread.dabr)
>> +	if (current->thread.dabr) {
>> 		set_dabr(current->thread.dabr);
>> +#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
>> +		mtspr(SPRN_DBCR0, current->thread.dbcr0);
>> +#endif
>
> Shouldn't this (and other places) be:
>
> #if defined(CONFIG_44x) || defined(CONFIG_BOOKE)
>
> if you are going to exclude 40x for now?  Otherwise this is still
> enabled on 405 and setting the wrong register.

if we are ignoring 40x this can just be CONFIG_BOOKE.  CONFIG_44x sets  
CONFIG_BOOKE.

- k

Re: [RFC] 4xx hardware watchpoint support

[Benjamin Herrenschmidt]

On Wed, 2008-07-23 at 13:10 -0300, Luis Machado wrote:
> On Wed, 2008-07-23 at 11:53 -0400, Josh Boyer wrote:
> > Shouldn't this (and other places) be:
> > 
> > #if defined(CONFIG_44x) || defined(CONFIG_BOOKE)
> > 
> > if you are going to exclude 40x for now?  Otherwise this is still
> > enabled on 405 and setting the wrong register.
> > 
> > josh
> 
> Yes, sorry. I wasn't aware of this specific define value. It makes
> things easier to support 405's later.
> 
> Like so?

Please, next time, when you re-send a patch like that, re-include
the full subject and patch description. You can add then blurbs after
the --- line if you want to add things that won't make it to git.

I'll deal with that specific one for now.

Cheers,
Ben.

Re: [RFC] 4xx hardware watchpoint support

[Kumar Gala]

On Jul 23, 2008, at 11:10 AM, Luis Machado wrote:

> On Wed, 2008-07-23 at 11:53 -0400, Josh Boyer wrote:
>> Shouldn't this (and other places) be:
>>
>> #if defined(CONFIG_44x) || defined(CONFIG_BOOKE)
>>
>> if you are going to exclude 40x for now?  Otherwise this is still
>> enabled on 405 and setting the wrong register.
>>
>> josh
>
> Yes, sorry. I wasn't aware of this specific define value. It makes
> things easier to support 405's later.
>
> Like so?
>
> This addresses Christoph's comments as well.
>
>
> Signed-off-by: Luis Machado <luisgpm@br.ibm.com>
>
> Index: linux-2.6.26/arch/powerpc/kernel/entry_32.S
> ===================================================================
> --- linux-2.6.26.orig/arch/powerpc/kernel/entry_32.S	2008-07-23  
> 07:44:57.000000000 -0700
> +++ linux-2.6.26/arch/powerpc/kernel/entry_32.S	2008-07-23  
> 07:50:31.000000000 -0700
> @@ -148,7 +148,7 @@
> 	/* Check to see if the dbcr0 register is set up to debug.  Use the
> 	   internal debug mode bit to do this. */
> 	lwz	r12,THREAD_DBCR0(r12)
> -	andis.	r12,r12,DBCR0_IDM@h
> +	andis.	r12,r12,(DBCR0_IDM  | DBSR_DAC1R | DBSR_DAC1W)@h

why the change here?  Is IDM not always on in this case for 44x?
>
> 	beq+	3f
> 	/* From user and task is ptraced - load up global dbcr0 */
> 	li	r12,-1			/* clear all pending debug events */
> @@ -292,7 +292,7 @@
> 	/* If the process has its own DBCR0 value, load it up.  The internal
> 	   debug mode bit tells us that dbcr0 should be loaded. */
> 	lwz	r0,THREAD+THREAD_DBCR0(r2)
> -	andis.	r10,r0,DBCR0_IDM@h
> +	andis.	r10,r0,(DBCR0_IDM  | DBSR_DAC1R | DBSR_DAC1W)@h
> 	bnel-	load_dbcr0
> #endif
> #ifdef CONFIG_44x
> @@ -720,7 +720,7 @@
> 	/* Check whether this process has its own DBCR0 value.  The internal
> 	   debug mode bit tells us that dbcr0 should be loaded. */
> 	lwz	r0,THREAD+THREAD_DBCR0(r2)
> -	andis.	r10,r0,DBCR0_IDM@h
> +	andis.	r10,r0,(DBCR0_IDM  | DBSR_DAC1R | DBSR_DAC1W)@h
> 	bnel-	load_dbcr0
> #endif
>
> Index: linux-2.6.26/arch/powerpc/kernel/process.c
> ===================================================================
> --- linux-2.6.26.orig/arch/powerpc/kernel/process.c	2008-07-23  
> 07:44:57.000000000 -0700
> +++ linux-2.6.26/arch/powerpc/kernel/process.c	2008-07-23  
> 07:50:31.000000000 -0700
> @@ -47,6 +47,8 @@
> #ifdef CONFIG_PPC64
> #include <asm/firmware.h>
> #endif
> +#include <linux/kprobes.h>
> +#include <linux/kdebug.h>
>
> extern unsigned long _get_SP(void);
>
> @@ -239,6 +241,35 @@
> }
> #endif /* CONFIG_SMP */
>
> +void do_dabr(struct pt_regs *regs, unsigned long address,
> +		    unsigned long error_code)
> +{
> +	siginfo_t info;
> +
> +	if (notify_die(DIE_DABR_MATCH, "dabr_match", regs, error_code,
> +			11, SIGSEGV) == NOTIFY_STOP)
> +		return;
> +
> +	if (debugger_dabr_match(regs))
> +		return;
> +
> +	/* Clear the DAC and struct entries.  One shot trigger */
> +#if (defined(CONFIG_44x) || defined(CONFIG_BOOKE))

this is redundant.  CONFIG_BOOKE is sufficient.

>
> +	mtspr(SPRN_DBCR0, mfspr(SPRN_DBCR0) & ~(DBSR_DAC1R | DBSR_DAC1W
> +							| DBCR0_IDM));
> +#endif
> +
> +	/* Clear the DABR */
> +	set_dabr(0);
> +
> +	/* Deliver the signal to userspace */
> +	info.si_signo = SIGTRAP;
> +	info.si_errno = 0;
> +	info.si_code = TRAP_HWBKPT;
> +	info.si_addr = (void __user *)address;
> +	force_sig_info(SIGTRAP, &info, current);
> +}
> +
> static DEFINE_PER_CPU(unsigned long, current_dabr);
>
> int set_dabr(unsigned long dabr)
> @@ -254,6 +285,11 @@
> #if defined(CONFIG_PPC64) || defined(CONFIG_6xx)
> 	mtspr(SPRN_DABR, dabr);
> #endif
> +
> +#if defined(CONFIG_44x) || defined(CONFIG_BOOKE)

ditto.

>
> +	mtspr(SPRN_DAC1, dabr);
> +#endif
> +
> 	return 0;
> }
>
> @@ -337,6 +373,12 @@
> 	if (unlikely(__get_cpu_var(current_dabr) != new->thread.dabr))
> 		set_dabr(new->thread.dabr);
>
> +#if defined(CONFIG_44x) || defined(CONFIG_BOOKE)
> +	/* If new thread DAC (HW breakpoint) is the same then leave it */
> +	if (new->thread.dabr)
> +		set_dabr(new->thread.dabr);
> +#endif
> +
> 	new_thread = &new->thread;
> 	old_thread = &current->thread;
>
> @@ -525,6 +567,10 @@
> 	if (current->thread.dabr) {
> 		current->thread.dabr = 0;
> 		set_dabr(0);
> +
> +#if defined(CONFIG_44x) || defined(CONFIG_BOOKE)
> +		current->thread.dbcr0 &= ~(DBSR_DAC1R | DBSR_DAC1W);
> +#endif
> 	}
> }
>
> Index: linux-2.6.26/arch/powerpc/kernel/ptrace.c
> ===================================================================
> --- linux-2.6.26.orig/arch/powerpc/kernel/ptrace.c	2008-07-23  
> 07:44:57.000000000 -0700
> +++ linux-2.6.26/arch/powerpc/kernel/ptrace.c	2008-07-23  
> 07:53:45.000000000 -0700
> @@ -703,7 +703,7 @@
>
> 	if (regs != NULL) {
> #if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
> -		task->thread.dbcr0 = DBCR0_IDM | DBCR0_IC;
> +		task->thread.dbcr0 |= DBCR0_IDM | DBCR0_IC;
> 		regs->msr |= MSR_DE;
> #else
> 		regs->msr |= MSR_SE;
> @@ -716,9 +716,16 @@
> {
> 	struct pt_regs *regs = task->thread.regs;
>
> +
> +#if defined(CONFIG_44x) || defined(CONFIG_BOOKE)
> +	/* If DAC then do not single step, skip */
> +	if (task->thread.dabr)
> +		return;
> +#endif
> +
> 	if (regs != NULL) {
> #if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
> -		task->thread.dbcr0 = 0;
> +		task->thread.dbcr0 &= ~(DBCR0_IC | DBCR0_IDM);
> 		regs->msr &= ~MSR_DE;
> #else
> 		regs->msr &= ~MSR_SE;
> @@ -727,22 +734,75 @@
> 	clear_tsk_thread_flag(task, TIF_SINGLESTEP);
> }
>
> -static int ptrace_set_debugreg(struct task_struct *task, unsigned  
> long addr,
> +int ptrace_set_debugreg(struct task_struct *task, unsigned long addr,
> 			       unsigned long data)
> {
> -	/* We only support one DABR and no IABRS at the moment */
> +	/* For ppc64 we support one DABR and no IABR's at the moment  
> (ppc64).
> +	 *  For embedded processors we support one DAC and no IAC's at the
> +	 *  moment.
> +	 */
> 	if (addr > 0)
> 		return -EINVAL;
>
> -	/* The bottom 3 bits are flags */

fix the comment, don't remove it.

>
> 	if ((data & ~0x7UL) >= TASK_SIZE)
> 		return -EIO;
>
> -	/* Ensure translation is on */
> +#ifdef CONFIG_PPC64

make this ifndef CONFIG_BOOKE, its not PPC64 specific.

>
> +
> +	/* For processors using DABR (i.e. 970), the bottom 3 bits are  
> flags.
> +	 *  It was assumed, on previous implementations, that 3 bits were
> +	 *  passed together with the data address, fitting the design of the
> +	 *  DABR register, as follows:
> +	 *
> +	 *  bit 0: Read flag
> +	 *  bit 1: Write flag
> +	 *  bit 2: Breakpoint translation
> +	 *
> +	 *  Thus, we use them here as so.
> +	 */
> +
> +	/* Ensure breakpoint translation bit is set */
> 	if (data && !(data & DABR_TRANSLATION))
> 		return -EIO;
>
> +	/* Move contents to the DABR register */
> 	task->thread.dabr = data;
> +
> +#endif
> +#if defined(CONFIG_44x) || defined(CONFIG_BOOKE)

see comment above.
>
> +
> +	/* As described above, it was assumed 3 bits were passed with the  
> data
> +	 *  address, but we will assume only the mode bits will be passed
> +	 *  as to not cause alignment restrictions for DAC-based processors.
> +	 */
> +
> +	/* DAC's hold the whole address without any mode flags */
> +	task->thread.dabr = data & ~0x3UL;
> +
> +	if (task->thread.dabr == 0) {
> +		task->thread.dbcr0 &= ~(DBSR_DAC1R | DBSR_DAC1W | DBCR0_IDM);
> +		task->thread.regs->msr &= ~MSR_DE;
> +		return 0;
> +	}
> +
> +	/* Read or Write bits must be set */
> +
> +	if (!(data & 0x3UL))
> +		return -EINVAL;
> +
> +	/* Set the Internal Debugging flag (IDM bit 1) for the DBCR0
> +	   register */
> +	task->thread.dbcr0 = DBCR0_IDM;
> +
> +	/* Check for write and read flags and set DBCR0
> +	   accordingly */
> +	if (data & 0x1UL)
> +		task->thread.dbcr0 |= DBSR_DAC1R;
> +	if (data & 0x2UL)
> +		task->thread.dbcr0 |= DBSR_DAC1W;
> +
> +	task->thread.regs->msr |= MSR_DE;
> +#endif
> 	return 0;
> }
>
> Index: linux-2.6.26/arch/powerpc/kernel/signal.c
> ===================================================================
> --- linux-2.6.26.orig/arch/powerpc/kernel/signal.c	2008-07-23  
> 07:44:57.000000000 -0700
> +++ linux-2.6.26/arch/powerpc/kernel/signal.c	2008-07-23  
> 07:50:31.000000000 -0700
> @@ -145,8 +145,12 @@
> 	 * user space. The DABR will have been cleared if it
> 	 * triggered inside the kernel.
> 	 */
> -	if (current->thread.dabr)
> +	if (current->thread.dabr) {
> 		set_dabr(current->thread.dabr);
> +#if defined(CONFIG_44x) || defined(CONFIG_BOOKE)

see comment above
>
> +		mtspr(SPRN_DBCR0, current->thread.dbcr0);
> +#endif
> +	}
>
> 	if (is32) {
>         	if (ka.sa.sa_flags & SA_SIGINFO)
> Index: linux-2.6.26/arch/powerpc/kernel/traps.c
> ===================================================================
> --- linux-2.6.26.orig/arch/powerpc/kernel/traps.c	2008-07-23  
> 07:44:57.000000000 -0700
> +++ linux-2.6.26/arch/powerpc/kernel/traps.c	2008-07-23  
> 07:50:31.000000000 -0700
> @@ -1067,6 +1067,22 @@
> 		}
>
> 		_exception(SIGTRAP, regs, TRAP_TRACE, regs->nip);
> +	} else if (debug_status & (DBSR_DAC1R | DBSR_DAC1W)) {
> +		regs->msr &= ~MSR_DE;
> +
> +		if (user_mode(regs)) {
> +			current->thread.dbcr0 &= ~(DBSR_DAC1R | DBSR_DAC1W |
> +								DBCR0_IDM);
> +		} else {
> +			/* Disable DAC interupts */
> +			mtspr(SPRN_DBCR0, mfspr(SPRN_DBCR0) & ~(DBSR_DAC1R |
> +						DBSR_DAC1W | DBCR0_IDM));
> +
> +			/* Clear the DAC event */
> +			mtspr(SPRN_DBSR, (DBSR_DAC1R | DBSR_DAC1W));
> +		}
> +		/* Setup and send the trap to the handler */
> +		do_dabr(regs, mfspr(SPRN_DAC1), debug_status);
> 	}
> }
> #endif /* CONFIG_4xx || CONFIG_BOOKE */
> Index: linux-2.6.26/arch/powerpc/mm/fault.c
> ===================================================================
> --- linux-2.6.26.orig/arch/powerpc/mm/fault.c	2008-07-23  
> 07:44:57.000000000 -0700
> +++ linux-2.6.26/arch/powerpc/mm/fault.c	2008-07-23  
> 07:50:31.000000000 -0700
> @@ -100,31 +100,6 @@
> 	return 0;
> }
>
> -#if !(defined(CONFIG_4xx) || defined(CONFIG_BOOKE))
> -static void do_dabr(struct pt_regs *regs, unsigned long address,
> -		    unsigned long error_code)
> -{
> -	siginfo_t info;
> -
> -	if (notify_die(DIE_DABR_MATCH, "dabr_match", regs, error_code,
> -			11, SIGSEGV) == NOTIFY_STOP)
> -		return;
> -
> -	if (debugger_dabr_match(regs))
> -		return;
> -
> -	/* Clear the DABR */
> -	set_dabr(0);
> -
> -	/* Deliver the signal to userspace */
> -	info.si_signo = SIGTRAP;
> -	info.si_errno = 0;
> -	info.si_code = TRAP_HWBKPT;
> -	info.si_addr = (void __user *)address;
> -	force_sig_info(SIGTRAP, &info, current);
> -}
> -#endif /* !(CONFIG_4xx || CONFIG_BOOKE)*/
> -
> /*
>  * For 600- and 800-family processors, the error_code parameter is  
> DSISR
>  * for a data fault, SRR1 for an instruction fault. For 400-family  
> processors
> Index: linux-2.6.26/include/asm-powerpc/system.h
> ===================================================================
> --- linux-2.6.26.orig/include/asm-powerpc/system.h	2008-07-23  
> 07:44:57.000000000 -0700
> +++ linux-2.6.26/include/asm-powerpc/system.h	2008-07-23  
> 07:50:31.000000000 -0700
> @@ -110,6 +110,8 @@
> #endif
>
> extern int set_dabr(unsigned long dabr);
> +extern void do_dabr(struct pt_regs *regs, unsigned long address,
> +		    unsigned long error_code);
> extern void print_backtrace(unsigned long *);
> extern void show_regs(struct pt_regs * regs);
> extern void flush_instruction_cache(void);
>
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [RFC] 4xx hardware watchpoint support

[Kumar Gala]

On Jul 24, 2008, at 11:00 PM, Benjamin Herrenschmidt wrote:

> On Wed, 2008-07-23 at 13:10 -0300, Luis Machado wrote:
>> On Wed, 2008-07-23 at 11:53 -0400, Josh Boyer wrote:
>>> Shouldn't this (and other places) be:
>>>
>>> #if defined(CONFIG_44x) || defined(CONFIG_BOOKE)
>>>
>>> if you are going to exclude 40x for now?  Otherwise this is still
>>> enabled on 405 and setting the wrong register.
>>>
>>> josh
>>
>> Yes, sorry. I wasn't aware of this specific define value. It makes
>> things easier to support 405's later.
>>
>> Like so?
>
> Please, next time, when you re-send a patch like that, re-include
> the full subject and patch description. You can add then blurbs after
> the --- line if you want to add things that won't make it to git.
>
> I'll deal with that specific one for now.

Ben,  I want to make sure this works on FSL Book-E before it gets into  
tree and I need to think about what SMP issues it might have.

I talked to Josh about this at OLS and if you are ok I can deal with  
acceptance of this patch via my tree.

- k

Re: [RFC] 4xx hardware watchpoint support

[Kumar Gala]

On Jul 25, 2008, at 10:23 AM, Kumar Gala wrote:

>
> On Jul 24, 2008, at 11:00 PM, Benjamin Herrenschmidt wrote:
>
>> On Wed, 2008-07-23 at 13:10 -0300, Luis Machado wrote:
>>> On Wed, 2008-07-23 at 11:53 -0400, Josh Boyer wrote:
>>>> Shouldn't this (and other places) be:
>>>>
>>>> #if defined(CONFIG_44x) || defined(CONFIG_BOOKE)
>>>>
>>>> if you are going to exclude 40x for now?  Otherwise this is still
>>>> enabled on 405 and setting the wrong register.
>>>>
>>>> josh
>>>
>>> Yes, sorry. I wasn't aware of this specific define value. It makes
>>> things easier to support 405's later.
>>>
>>> Like so?
>>
>> Please, next time, when you re-send a patch like that, re-include
>> the full subject and patch description. You can add then blurbs after
>> the --- line if you want to add things that won't make it to git.
>>
>> I'll deal with that specific one for now.
>
> Ben,  I want to make sure this works on FSL Book-E before it gets  
> into tree and I need to think about what SMP issues it might have.
>
> I talked to Josh about this at OLS and if you are ok I can deal with  
> acceptance of this patch via my tree.

Josh pointed out that you went ahead and merged this.  Curse you :)

I've got a patch in my tree to address my initial concerns.

- k

Re: [RFC] 4xx hardware watchpoint support

[Benjamin Herrenschmidt]

On Fri, 2008-07-25 at 10:23 -0500, Kumar Gala wrote:
> 
> Ben,  I want to make sure this works on FSL Book-E before it gets into  
> tree and I need to think about what SMP issues it might have.

Hrm.. too late. I merged it.

> I talked to Josh about this at OLS and if you are ok I can deal with  
> acceptance of this patch via my tree.

I don't see the patch causing issues as-is, unless I missed something.
Currently none of the platform it affects supports SMP in the tree
either. Can you verify about FSL and if there's a problem we can fixup
the #ifdefs.

Ben.

Re: [RFC] 4xx hardware watchpoint support

[Benjamin Herrenschmidt]

> Josh pointed out that you went ahead and merged this.  Curse you :)
> 
> I've got a patch in my tree to address my initial concerns.

Well, I asked Josh on IRC and he was fine, I got your email too late.

Cheers,
Ben.

Re: [RFC] 4xx hardware watchpoint support

[Josh Boyer]

On Sat, 26 Jul 2008 07:38:57 +1000
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> 
> > Josh pointed out that you went ahead and merged this.  Curse you :)
> > 
> > I've got a patch in my tree to address my initial concerns.
> 
> Well, I asked Josh on IRC and he was fine, I got your email too late.

I was (and still am) OK with it.  Kumar's comments are valid but not
major for 44x.  Some cleanup could be done, but I was more focused on
getting it in during this merge window.

I think we just had a small case of bad timing.  I blame conferences
and late nights of beer^H^H^H^Hcoding.

josh

Re: [RFC] 4xx hardware watchpoint support

[Benjamin Herrenschmidt]

On Fri, 2008-07-25 at 19:08 -0400, Josh Boyer wrote:
> On Sat, 26 Jul 2008 07:38:57 +1000
> Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> 
> > 
> > > Josh pointed out that you went ahead and merged this.  Curse you :)
> > > 
> > > I've got a patch in my tree to address my initial concerns.
> > 
> > Well, I asked Josh on IRC and he was fine, I got your email too late.
> 
> I was (and still am) OK with it.  Kumar's comments are valid but not
> major for 44x.  Some cleanup could be done, but I was more focused on
> getting it in during this merge window.
> 
> I think we just had a small case of bad timing.  I blame conferences
> and late nights of beer^H^H^H^Hcoding.

Yeah, as I said, the patch is fine (ie shouldn't break anything) and has
had plenty of review so I felt it was good to go, we can do cleanups
later. I wanted that feature in the merge window, next week would have
been too late :-)

Cheers,
Ben.