4xx hardware watchpoint support

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

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

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.

Quoted 3 lines

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?

Quoted 9 lines · Luis Machado

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.

Quoted 9 lines · Luis Machado

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.

Quoted 10 lines

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.

Quoted 34 lines · Paul Mackerras

Quoted 9 lines · Luis Machado

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.

Quoted 10 lines

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?

Quoted 5 lines

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
===================================================================

arch/powerpc/kernel/process.c +44 -0

@@ -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
===================================================================

arch/powerpc/kernel/ptrace.c +70 -6

@@ -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
===================================================================

arch/powerpc/kernel/signal.c +5 -1

@@ -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
===================================================================

arch/powerpc/kernel/traps.c +15 -0

@@ -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
===================================================================

arch/powerpc/kernel/entry_32.S +3 -3

@@ -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
===================================================================

arch/powerpc/mm/fault.c +0 -25

@@ -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
===================================================================

include/asm-powerpc/system.h +2 -0

@@ -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
[email protected]
https://ozlabs.org/mailman/listinfo/linuxppc-dev

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?

Index: linux-2.6.25-oldpatch/arch/powerpc/kernel/process.c
===================================================================

arch/powerpc/kernel/process.c +6 -0

@@ -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))

Index: linux-2.6.25-oldpatch/arch/powerpc/kernel/signal.c
===================================================================

arch/powerpc/kernel/signal.c +1 -0

+	}
 
 	if (is32) {
         	if (ka.sa.sa_flags & SA_SIGINFO)

Index: linux-2.6.25-oldpatch/arch/powerpc/kernel/traps.c
===================================================================

arch/powerpc/kernel/traps.c +2 -0

@@ -1045,6 +1045,21 @@
 				return;
 		}
 		_exception(SIGTRAP, regs, TRAP_TRACE, 0);
+	} else if (debug_status & (DBSR_DAC1R | DBSR_DAC1W)) {
+		regs->msr &= ~MSR_DE;

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.

Quoted 6 lines

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.

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

Quoted 2 lines

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.

Hi,

Quoted 9 lines

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

Quoted 2 lines

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?

arch/powerpc/kernel/process.c +8 -0

+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)

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.

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.

Hi,

Quoted 6 lines

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

Quoted 2 lines

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.

Quoted 3 lines

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.

Index: linux-2.6.26/arch/powerpc/kernel/signal.c
===================================================================

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

Quoted 7 lines · Josh Boyer

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.

Quoted 1 line

josh

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

Like so?

Quoted 13 lines · Luis Machado

Quoted 8 lines · Josh Boyer

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.

Quoted 1 line

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.

Quoted 20 lines · Benjamin Herrenschmidt

Quoted 8 lines · Luis Machado

Quoted 7 lines · Josh Boyer

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.

Quoted 6 lines

Quoted 1 line

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.

Quoted 1 line

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.

Quoted 4 lines

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.

Quoted 5 lines · On Sat, 26 Jul 2008 07:38:57 +1000 Benjamin Herrenschmidt…

Quoted 4 lines

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.

Quoted 1 line

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.

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.

Quoted 8 lines · Josh Boyer

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 <[email protected]>

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

arch/powerpc/kernel/entry_32.S +2 -2

 	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

arch/powerpc/kernel/process.c +6 -0

@@ -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;

arch/powerpc/kernel/process.c +15 -0

+	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)

arch/powerpc/kernel/process.c +13 -0

+	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

arch/powerpc/kernel/ptrace.c +11 -4

@@ -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).

arch/powerpc/kernel/ptrace.c +3 -0

+	 *  For embedded processors we support one DAC and no IAC's at the
+	 *  moment.
+	 */

	if (addr > 0)
		return -EINVAL;

(quoted diff) +2 -0

+
+	/* For processors using DABR (i.e. 970), the bottom 3 bits are  

flags.

arch/powerpc/kernel/ptrace.c +12 -0

+	 *  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;

arch/powerpc/kernel/ptrace.c +1 -0

+	/* Move contents to the DABR register */

	task->thread.dabr = data;

(quoted diff) +2 -0

+
+	/* As described above, it was assumed 3 bits were passed with the  

data

arch/powerpc/kernel/ptrace.c +31 -0

+	 *  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

arch/powerpc/kernel/signal.c +3 -0

+		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

arch/powerpc/kernel/traps.c +16 -0

@@ -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

arch/powerpc/mm/fault.c +0 -25

@@ -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

include/asm-powerpc/system.h +2 -0

@@ -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
[email protected]
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Quoted 8 lines · On Tue, 22 Jul 2008 22:47:58 -0300 Luis Machado <[email protected]>…

Hi,

Quoted 6 lines

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

Quoted 2 lines

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.

Quoted 8 lines

Quoted 3 lines

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.

Quoted 7 lines

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

arch/powerpc/kernel/signal.c line 152

Quoted 9 lines

arch/powerpc/kernel/signal.c +4 -1

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