Re: [RFC] 4xx hardware watchpoint support
On Fri, 2008-07-25 at 19:08 -0400, Josh Boyer wrote: > On Sat, 26 Jul 2008 07:38:57 +1000 > Benjamin Herrenschmidt <[EMAIL PROTECTED]> 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. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] 4xx hardware watchpoint support
On Sat, 26 Jul 2008 07:38:57 +1000 Benjamin Herrenschmidt <[EMAIL PROTECTED]> 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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] 4xx hardware watchpoint support
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. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] 4xx hardware watchpoint support
> 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. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] 4xx hardware watchpoint support
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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] 4xx hardware watchpoint support
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 <[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.0 -0700
+++ linux-2.6.26/arch/powerpc/kernel/entry_32.S 2008-07-23
07:50:31.0 -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,[EMAIL PROTECTED]
+ 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,[EMAIL PROTECTED]
+ 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,[EMAIL PROTECTED]
+ 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.0 -0700
+++ linux-2.6.26/arch/powerpc/kernel/process.c 2008-07-23
07:50:31.0 -0700
@@ -47,6 +47,8 @@
#ifdef CONFIG_PPC64
#include
#endif
+#include
+#include
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 = ¤t->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.0 -0700
+++ linux-2.6.26/arch/powerpc/kernel/ptrace.c 2008-07-23
07:53:45.0 -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;
Re: [RFC] 4xx hardware watchpoint support
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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] 4xx hardware watchpoint support
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. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] 4xx hardware watchpoint support
On Jul 23, 2008, at 10:53 AM, Josh Boyer wrote:
On Tue, 22 Jul 2008 22:47:58 -0300
Luis Machado <[EMAIL PROTECTED]> 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.0 -0700
+++ linux-2.6.26/arch/powerpc/kernel/signal.c 2008-07-22
16:47:22.0 -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
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] 4xx hardware watchpoint support
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 <[EMAIL PROTECTED]>
Index: linux-2.6.26/arch/powerpc/kernel/entry_32.S
===
--- linux-2.6.26.orig/arch/powerpc/kernel/entry_32.S2008-07-23
07:44:57.0 -0700
+++ linux-2.6.26/arch/powerpc/kernel/entry_32.S 2008-07-23 07:50:31.0
-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,[EMAIL PROTECTED]
+ 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,[EMAIL PROTECTED]
+ 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,[EMAIL PROTECTED]
+ 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.0 -0700
+++ linux-2.6.26/arch/powerpc/kernel/process.c 2008-07-23 07:50:31.0
-0700
@@ -47,6 +47,8 @@
#ifdef CONFIG_PPC64
#include
#endif
+#include
+#include
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 = ¤t->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.0 -0700
+++ linux-2.6.26/arch/powerpc/kernel/ptrace.c 2008-07-23 07:53:45.0
-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->
Re: [RFC] 4xx hardware watchpoint support
On Tue, 22 Jul 2008 22:47:58 -0300
Luis Machado <[EMAIL PROTECTED]> 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.c2008-07-20
> 16:56:57.0 -0700
> +++ linux-2.6.26/arch/powerpc/kernel/signal.c 2008-07-22 16:47:22.0
> -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
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] 4xx hardware watchpoint support
> 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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] 4xx hardware watchpoint support
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.
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] 4xx hardware watchpoint support
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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] 4xx hardware watchpoint support
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 <[EMAIL PROTECTED]>
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.0 -0700
+++ linux-2.6.26/arch/powerpc/kernel/process.c 2008-07-22 16:46:36.0
-0700
@@ -47,6 +47,8 @@
#ifdef CONFIG_PPC64
#include
#endif
+#include
+#include
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 = ¤t->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.0 -0700
+++ linux-2.6.26/arch/powerpc/kernel/ptrace.c 2008-07-22 16:41:24.0
-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
Re: [RFC] 4xx hardware watchpoint support
On Mon, 21 Jul 2008 13:36:33 -0300 Luis Machado <[EMAIL PROTECTED]> 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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] 4xx hardware watchpoint support
> 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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] 4xx hardware watchpoint support
On Fri, 20 Jun 2008 17:14:53 -0300
Luis Machado <[EMAIL PROTECTED]> 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.0 -0700
> +++ linux-2.6.25-oldpatch/arch/powerpc/kernel/process.c 2008-06-20
> 02:51:16.0 -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.
> 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.0 -0700
> +++ linux-2.6.25-oldpatch/arch/powerpc/kernel/signal.c2008-06-20
> 02:51:16.0 -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.c2008-06-20
> 02:48:19.0 -0700
> +++ linux-2.6.25-oldpatch/arch/powerpc/kernel/traps.c 2008-06-20
> 02:54:37.0 -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
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] 4xx hardware watchpoint support
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.0 -0700
> +++ linux-2.6.25-oldpatch/arch/powerpc/kernel/process.c 2008-06-20
> 02:51:16.0 -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);
Re: [RFC] 4xx hardware watchpoint support
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.c2008-06-20
02:48:19.0 -0700
+++ linux-2.6.25-oldpatch/arch/powerpc/kernel/process.c 2008-06-20
02:51:16.0 -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.
Re: [RFC] 4xx hardware watchpoint support
> 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? [EMAIL PROTECTED] 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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] 4xx hardware watchpoint support
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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] 4xx hardware watchpoint support
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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] 4xx hardware watchpoint support
> 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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] 4xx hardware watchpoint support
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. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] 4xx hardware watchpoint support
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.0 -0700
+++ linux-2.6.25.1/arch/powerpc/kernel/process.c2008-05-21
07:26:55.0 -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 = ¤t->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.c2008-05-21
07:25:45.0 -0700
+++ linux-2.6.25.1/arch/powerpc/kernel/ptrace.c 2008-05-21 08:23:34.0
-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.
Re: [RFC] 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 :) - k ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
[RFC] 4xx hardware watchpoint support
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
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.0 -0700
+++ linux-2.6.25.1/arch/powerpc/kernel/process.c 2008-05-21 07:26:55.0 -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 = ¤t->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.0 -0700
+++ linux-2.6.25.1/arch/powerpc/kernel/ptrace.c 2008-05-21 08:23:34.0 -0700
@@ -629,9 +629,15 @@
{
struct pt_regs *r
