Re: [RFC] 4xx hardware watchpoint support

2008-07-25 Thread Kumar Gala 


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


On Wed, 2008-07-23 at 11:53 -0400, Josh Boyer wrote:

Shouldn't this (and other places) be:

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

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

josh


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

Like so?

This addresses Christoph's comments as well.


Signed-off-by: Luis Machado [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 asm/firmware.h
#endif
+#include linux/kprobes.h
+#include linux/kdebug.h

extern unsigned long _get_SP(void);

@@ -239,6 +241,35 @@
}
#endif /* CONFIG_SMP */

+void do_dabr(struct pt_regs *regs, unsigned long address,
+   unsigned long error_code)
+{
+   siginfo_t info;
+
+   if (notify_die(DIE_DABR_MATCH, dabr_match, regs, error_code,
+   11, SIGSEGV) == NOTIFY_STOP)
+   return;
+
+   if (debugger_dabr_match(regs))
+   return;
+
+   /* Clear the DAC and struct entries.  One shot trigger */
+#if (defined(CONFIG_44x) || defined(CONFIG_BOOKE))


this is redundant.  CONFIG_BOOKE is sufficient.



+   mtspr(SPRN_DBCR0, mfspr(SPRN_DBCR0)  ~(DBSR_DAC1R | DBSR_DAC1W
+   | DBCR0_IDM));
+#endif
+
+   /* Clear the DABR */
+   set_dabr(0);
+
+   /* Deliver the signal to userspace */
+   info.si_signo = SIGTRAP;
+   info.si_errno = 0;
+   info.si_code = TRAP_HWBKPT;
+   info.si_addr = (void __user *)address;
+   force_sig_info(SIGTRAP, info, current);
+}
+
static DEFINE_PER_CPU(unsigned long, current_dabr);

int set_dabr(unsigned long dabr)
@@ -254,6 +285,11 @@
#if defined(CONFIG_PPC64) || defined(CONFIG_6xx)
mtspr(SPRN_DABR, dabr);
#endif
+
+#if defined(CONFIG_44x) || defined(CONFIG_BOOKE)


ditto.



+   mtspr(SPRN_DAC1, dabr);
+#endif
+
return 0;
}

@@ -337,6 +373,12 @@
if (unlikely(__get_cpu_var(current_dabr) != new-thread.dabr))
set_dabr(new-thread.dabr);

+#if defined(CONFIG_44x) || defined(CONFIG_BOOKE)
+   /* If new thread DAC (HW breakpoint) is the same then leave it */
+   if (new-thread.dabr)
+   set_dabr(new-thread.dabr);
+#endif
+
new_thread = new-thread;
old_thread = current-thread;

@@ -525,6 +567,10 @@
if (current-thread.dabr) {
current-thread.dabr = 0;
set_dabr(0);
+
+#if defined(CONFIG_44x) || defined(CONFIG_BOOKE)
+   current-thread.dbcr0 = ~(DBSR_DAC1R | DBSR_DAC1W);
+#endif
}
}

Index: linux-2.6.26/arch/powerpc/kernel/ptrace.c
===
--- linux-2.6.26.orig/arch/powerpc/kernel/ptrace.c	2008-07-23  
07:44:57.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;
+

Re: [RFC] 4xx hardware watchpoint support

2008-07-25 Thread Kumar Gala 


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


On Wed, 2008-07-23 at 13:10 -0300, Luis Machado wrote:

On Wed, 2008-07-23 at 11:53 -0400, Josh Boyer wrote:

Shouldn't this (and other places) be:

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

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

josh


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

Like so?


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

I'll deal with that specific one for now.


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


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


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

Re: [RFC] 4xx hardware watchpoint support

2008-07-25 Thread Kumar Gala 


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



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


On Wed, 2008-07-23 at 13:10 -0300, Luis Machado wrote:

On Wed, 2008-07-23 at 11:53 -0400, Josh Boyer wrote:

Shouldn't this (and other places) be:

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

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

josh


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

Like so?


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

I'll deal with that specific one for now.


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


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


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

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

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

Re: [RFC] 4xx hardware watchpoint support

2008-07-25 Thread Benjamin Herrenschmidt 

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

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

Cheers,
Ben.


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

Re: [RFC] 4xx hardware watchpoint support

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

Hrm.. too late. I merged it.

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

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

Ben.


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

Re: [RFC] 4xx hardware watchpoint support

2008-07-25 Thread Josh Boyer 
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

2008-07-25 Thread Benjamin Herrenschmidt 
On Fri, 2008-07-25 at 19:08 -0400, Josh Boyer wrote:
 On Sat, 26 Jul 2008 07:38:57 +1000
 Benjamin Herrenschmidt [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

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

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

I'll deal with that specific one for now.

Cheers,
Ben.


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

Re: [RFC] 4xx hardware watchpoint support

2008-07-23 Thread Kumar Gala 


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


Hi,


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

Do you think it's worth to support this facility on 405's  
processors? If

so, i'll gladly work on a solution to it.


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


As the 440 support is ready and the 405 needs additional tweaking  
due to
the use of DBCR1 instead of DBCR0 and due to a different position  
scheme

of the DAC1R/DAC1W flags inside DBCR1, i'd say we should include this
code and handle the 405 case later.

We might have to handle it anyway if we're going to pursue the  
hardware

breakpoint interface work in the future.

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


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


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

Re: [RFC] 4xx hardware watchpoint support

2008-07-23 Thread Luis Machado 
 Some comment, first the above negate conditional
 looks rather ugly, I'd rather do a
 
 #if defined(CONFIG_4xx) || defined(CONFIG_BOOKE)
   dbcr0 case
 #else
   dabr case
 #endif

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

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

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

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

Will do, thanks for reviewing this one.

Regards,
Luis

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

Re: [RFC] 4xx hardware watchpoint support

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

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

Like so?

This addresses Christoph's comments as well.


Signed-off-by: Luis Machado [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 asm/firmware.h
 #endif
+#include linux/kprobes.h
+#include linux/kdebug.h
 
 extern unsigned long _get_SP(void);
 
@@ -239,6 +241,35 @@
 }
 #endif /* CONFIG_SMP */
 
+void do_dabr(struct pt_regs *regs, unsigned long address,
+   unsigned long error_code)
+{
+   siginfo_t info;
+
+   if (notify_die(DIE_DABR_MATCH, dabr_match, regs, error_code,
+   11, SIGSEGV) == NOTIFY_STOP)
+   return;
+
+   if (debugger_dabr_match(regs))
+   return;
+
+   /* Clear the DAC and struct entries.  One shot trigger */
+#if (defined(CONFIG_44x) || defined(CONFIG_BOOKE))
+   mtspr(SPRN_DBCR0, mfspr(SPRN_DBCR0)  ~(DBSR_DAC1R | DBSR_DAC1W
+   | DBCR0_IDM));
+#endif
+
+   /* Clear the DABR */
+   set_dabr(0);
+
+   /* Deliver the signal to userspace */
+   info.si_signo = SIGTRAP;
+   info.si_errno = 0;
+   info.si_code = TRAP_HWBKPT;
+   info.si_addr = (void __user *)address;
+   force_sig_info(SIGTRAP, info, current);
+}
+
 static DEFINE_PER_CPU(unsigned long, current_dabr);
 
 int set_dabr(unsigned long dabr)
@@ -254,6 +285,11 @@
 #if defined(CONFIG_PPC64) || defined(CONFIG_6xx)
mtspr(SPRN_DABR, dabr);
 #endif
+
+#if defined(CONFIG_44x) || defined(CONFIG_BOOKE)
+   mtspr(SPRN_DAC1, dabr);
+#endif
+
return 0;
 }
 
@@ -337,6 +373,12 @@
if (unlikely(__get_cpu_var(current_dabr) != new-thread.dabr))
set_dabr(new-thread.dabr);
 
+#if defined(CONFIG_44x) || defined(CONFIG_BOOKE)
+   /* If new thread DAC (HW breakpoint) is the same then leave it */
+   if (new-thread.dabr)
+   set_dabr(new-thread.dabr);
+#endif
+
new_thread = new-thread;
old_thread = current-thread;
 
@@ -525,6 +567,10 @@
if (current-thread.dabr) {
current-thread.dabr = 0;
set_dabr(0);
+
+#if defined(CONFIG_44x) || defined(CONFIG_BOOKE)
+   current-thread.dbcr0 = ~(DBSR_DAC1R | DBSR_DAC1W);
+#endif
}
 }
 
Index: linux-2.6.26/arch/powerpc/kernel/ptrace.c
===
--- linux-2.6.26.orig/arch/powerpc/kernel/ptrace.c  2008-07-23 
07:44:57.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

Re: [RFC] 4xx hardware watchpoint support

2008-07-23 Thread Kumar Gala 


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

2008-07-22 Thread Luis Machado 
Hi,

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

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

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

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


Signed-off-by: Luis Machado [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 asm/firmware.h
 #endif
+#include linux/kprobes.h
+#include linux/kdebug.h
 
 extern unsigned long _get_SP(void);
 
@@ -239,6 +241,36 @@
 }
 #endif /* CONFIG_SMP */
 
+void do_dabr(struct pt_regs *regs, unsigned long address,
+   unsigned long error_code)
+{
+   siginfo_t info;
+
+#if !(defined(CONFIG_4xx) || defined(CONFIG_BOOKE))
+   if (notify_die(DIE_DABR_MATCH, dabr_match, regs, error_code,
+   11, SIGSEGV) == NOTIFY_STOP)
+   return;
+
+   if (debugger_dabr_match(regs))
+   return;
+
+   /* Clear the DAC and struct entries.  One shot trigger.  */
+#else /* (defined(CONFIG_4xx) || defined(CONFIG_BOOKE)) */
+   mtspr(SPRN_DBCR0, mfspr(SPRN_DBCR0)  ~(DBSR_DAC1R | DBSR_DAC1W
+   | DBCR0_IDM));
+#endif
+
+   /* Clear the DABR */
+   set_dabr(0);
+
+   /* Deliver the signal to userspace */
+   info.si_signo = SIGTRAP;
+   info.si_errno = 0;
+   info.si_code = TRAP_HWBKPT;
+   info.si_addr = (void __user *)address;
+   force_sig_info(SIGTRAP, info, current);
+}
+
 static DEFINE_PER_CPU(unsigned long, current_dabr);
 
 int set_dabr(unsigned long dabr)
@@ -254,6 +286,11 @@
 #if defined(CONFIG_PPC64) || defined(CONFIG_6xx)
mtspr(SPRN_DABR, dabr);
 #endif
+
+#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
+   mtspr(SPRN_DAC1, dabr);
+#endif
+
return 0;
 }
 
@@ -337,6 +374,12 @@
if (unlikely(__get_cpu_var(current_dabr) != new-thread.dabr))
set_dabr(new-thread.dabr);
 
+#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
+   /* If new thread DAC (HW breakpoint) is the same then leave it.  */
+   if (new-thread.dabr)
+   set_dabr(new-thread.dabr);
+#endif
+
new_thread = new-thread;
old_thread = current-thread;
 
@@ -525,6 +568,10 @@
if (current-thread.dabr) {
current-thread.dabr = 0;
set_dabr(0);
+
+#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
+   current-thread.dbcr0 = ~(DBSR_DAC1R | DBSR_DAC1W);
+#endif
}
 }
 
Index: linux-2.6.26/arch/powerpc/kernel/ptrace.c
===
--- linux-2.6.26.orig/arch/powerpc/kernel/ptrace.c  2008-07-20 
16:56:57.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

Re: [RFC] 4xx hardware watchpoint support

2008-07-21 Thread Luis Machado 

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

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

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

Regards,
Luis

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

Re: [RFC] 4xx hardware watchpoint support

2008-07-21 Thread Josh Boyer 
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

2008-07-19 Thread Josh Boyer 
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.

snip

 Index: linux-2.6.25-oldpatch/arch/powerpc/kernel/signal.c
 ===
 --- linux-2.6.25-oldpatch.orig/arch/powerpc/kernel/signal.c   2008-06-20 
 02:48:19.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

2008-06-30 Thread Luis Machado 
Hi guys,

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

Thanks!

Regards,
Luis

On Fri, 2008-06-20 at 17:14 -0300, Luis Machado wrote:
 On Thu, 2008-05-22 at 13:51 +1000, Paul Mackerras wrote:
  Luis Machado writes:
  
   This is a patch that has been sitting idle for quite some time. I
   decided to move it further because it is something useful. It was
   originally written by Michel Darneille, based off of 2.6.16.
   
   The original patch, though, was not compatible with the current DABR
   logic. DABR's are used to implement hardware watchpoint support for
   ppc64 processors (i.e. 970, Power5 etc). 4xx's have a different
   debugging register layout and needs to be handled differently (they two
   registers: DAC and DBCR0).
  
  Yes, they are different, but they do essentially the same thing, so I
  think we should try and unify the handling of the two.  Maybe you
  could rename set_dabr() to set_hw_watchpoint() or something and make
  it set DABR on 64-bit and classic 32-bit processors, and DAC on
  4xx/Book E processors.
  
  Likewise, I don't think we need both a dabr field and a dac field
  in the thread_struct - one should do.  Rename dabr to something else
  if you feel that the dabr name is too ppc64-specific.  And I don't
  think we need both __debugger_dabr_match and __debugger_dac_match.
  
   5) This is something i'm worried about for future features. We currently
   have a way to support only Hardware Watchpoints, but not Hardware
   Breakpoints (on 64-bit processors that have a IABR register or 32-bit
   processors carrying the IAC register). Looking at the code, we don't
   differentiate a watchpoint from a breakpoint request. A ptrace call has
   currently 3 arguments: REQUEST, ADDR and DATA. We use REQUEST and DATA
   to set a hardware watchpoint. Maybe we could use the ADDR parameter to
   set a hardware breakpoint? Or use it to tell the kernel whether we want
   a hardware watchpoint or hardware breakpoint and then pass the address
   of the instruction/data through the DATA parameter? What do you think?
  
  I would think there would be a different REQUEST value to mean set a
  hardware breakpoint.  Roland McGrath (cc'd) might be able to tell us
  what other architectures do.
  
  Paul.
 
 Paul,
 
 Follows a re-worked patch that unifies the handling of hardware
 watchpoint structures for DABR-based and DAC-based processors.
 
 The flow is exactly the same for DABR-based processors.
 
 As for the DAC-based code, i've eliminated the dac field from the
 thread structure, since now we use the dabr field to represent DAC1 of
 the DAC-based processors. As a consequence, i removed all the
 occurrences of dac throughout the code, using dabr in those cases.
 
 The function set_dabr sets the correct register (DABR OR DAC) for each
 specific processor now, instead of only setting the value for DABR-based
 processors.
 
 do_dac was merged with do_dabr as they were mostly equivalent pieces
 of code. I've moved do_dabr to arch/powerpc/kernel/process.c so it
 is visible for DAC-based code as well.
 
 Tested on a Taishan 440GX with success.
 
 Is it OK to leave it as dabr for both DABR and DAC? What do you think
 of the patch overall?
 
 Thanks,
 
 Best regards,
 Luis
 
 
 Index: linux-2.6.25-oldpatch/arch/powerpc/kernel/process.c
 ===
 --- linux-2.6.25-oldpatch.orig/arch/powerpc/kernel/process.c  2008-06-20 
 02:48:19.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) !=

Re: [RFC] 4xx hardware watchpoint support

2008-06-20 Thread Luis Machado 

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

Paul,

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

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

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

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

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

Tested on a Taishan 440GX with success.

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

Thanks,

Best regards,
Luis


Index: linux-2.6.25-oldpatch/arch/powerpc/kernel/process.c
===
--- linux-2.6.25-oldpatch.orig/arch/powerpc/kernel/process.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.  */
+   if (new-thread.dabr)
+   set_dabr(new-thread.dabr);
+#endif
+

Re: [RFC] 4xx hardware watchpoint support

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

[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

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

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

Best regards,
Luis

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

Re: [RFC] 4xx hardware watchpoint support

2008-05-23 Thread Luis Machado 

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

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

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

Best Regards,
Luis

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

Re: [RFC] 4xx hardware watchpoint support

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

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

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


Thanks,
Roland
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

[RFC] 4xx hardware watchpoint support

2008-05-21 Thread Luis Machado 
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 = current-thread;
 
@@ -515,6 +544,16 @@
 
 	discard_lazy_cpu_state();
 
+#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
+	if (current-thread.dac) {
+		current-thread.dac = 0;
+		/* Clear debug control.  */
+		current-thread.dbcr0 = ~(DBSR_DAC1R | DBSR_DAC1W);
+
+		set_dac(0);
+	}
+#endif
+
 	if (current-thread.dabr) {
 		current-thread.dabr = 0;
 		set_dabr(0);
Index: linux-2.6.25.1/arch/powerpc/kernel/ptrace.c
===
--- linux-2.6.25.1.orig/arch/powerpc/kernel/ptrace.c	2008-05-21 07:25:45.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 =

Re: [RFC] 4xx hardware watchpoint support

2008-05-21 Thread Kumar Gala 

Two real quick notes.

Take a look at:

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

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


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

Re: [RFC] 4xx hardware watchpoint support

2008-05-21 Thread Luis Machado 
Thanks for the inlining tip. It should be now. :-)

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

Regards,
Luis

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

===
--- linux-2.6.25.1.orig/arch/powerpc/kernel/process.c   2008-05-21 
07:25:45.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 = current-thread;
 
@@ -515,6 +544,16 @@
 
discard_lazy_cpu_state();
 
+#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
+   if (current-thread.dac) {
+   current-thread.dac = 0;
+   /* Clear debug control.  */
+   current-thread.dbcr0 = ~(DBSR_DAC1R | DBSR_DAC1W);
+
+   set_dac(0);
+   }
+#endif
+
if (current-thread.dabr) {
current-thread.dabr = 0;
set_dabr(0);
Index: linux-2.6.25.1/arch/powerpc/kernel/ptrace.c
===
--- linux-2.6.25.1.orig/arch/powerpc/kernel/ptrace.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.  */
+   if (!(data  0x3UL))
+

Re: [RFC] 4xx hardware watchpoint support

2008-05-21 Thread Paul Mackerras 
Luis Machado writes:

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

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

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

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

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

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