Re: [PATCH 1/2] [hw-breakpoint] Use generic hw-breakpoint interfaces for new PPC ptrace flags

2011-12-20 Thread David Gibson 
On Thu, Dec 08, 2011 at 04:49:48PM +0530, K.Prasad wrote:
 PPC_PTRACE_GETHWDBGINFO, PPC_PTRACE_SETHWDEBUG and PPC_PTRACE_DELHWDEBUG are
 PowerPC specific ptrace flags that use the watchpoint register. While they are
 targeted primarily towards BookE users, user-space applications such as GDB
 have started using them for BookS too. This patch enables the use of generic
 hardware breakpoint interfaces for these new flags.
 
 Apart from the usual benefits of using generic hw-breakpoint interfaces, these
 changes allow debuggers (such as GDB) to use a common set of ptrace flags for
 their watchpoint needs and allow more precise breakpoint specification (length
 of the variable can be specified).
 
 Signed-off-by: K.Prasad pra...@linux.vnet.ibm.com

Ok, I think it's finally ready to go.

Acked-by: David Gibson da...@gibson.dropbear.id.au

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson

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

Re: [PATCH 1/2] [hw-breakpoint] Use generic hw-breakpoint interfaces for new PPC ptrace flags

2011-12-08 Thread K.Prasad 
On Wed, Dec 07, 2011 at 05:01:57PM -0200, Thiago Jung Bauermann wrote:
 On Thu, 2011-12-01 at 15:50 +0530, K.Prasad wrote:
  On Mon, Nov 28, 2011 at 02:11:11PM +1100, David Gibson wrote:
   [snip]
   On Wed, Oct 12, 2011 at 11:09:48PM +0530, K.Prasad wrote:
diff --git a/Documentation/powerpc/ptrace.txt 
b/Documentation/powerpc/ptrace.txt
index f4a5499..f2a7a39 100644
--- a/Documentation/powerpc/ptrace.txt
+++ b/Documentation/powerpc/ptrace.txt
@@ -127,6 +127,22 @@ Some examples of using the structure to:
   p.addr2   = (uint64_t) end_range;
   p.condition_value = 0;
 
+- set a watchpoint in server processors (BookS)
+
+  p.version = 1;
+  p.trigger_type= PPC_BREAKPOINT_TRIGGER_RW;
+  p.addr_mode   = PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE;
+  or
+  p.addr_mode   = PPC_BREAKPOINT_MODE_EXACT;
+
+  p.condition_mode  = PPC_BREAKPOINT_CONDITION_NONE;
+  p.addr= (uint64_t) begin_range;
   
   You should probably document the alignment constraint on the address
   here, too.
   
  
  Alignment constraints will be learnt by the user-space during runtime.
  We provide that as part of 'struct ppc_debug_info' in
  'data_bp_alignment' field.
  
  While the alignment is always 8-bytes for BookS, I think userspace
  should be left to learn it through PTRACE_PPC_GETHWDEBUGINFO.
 
 Right. In particular, BookE doesn't have alignment constraints.


Okay.
 
+   attr.bp_len = len;
+   ret =  modify_user_hw_breakpoint(bp, attr);
+   if (ret) {
+   ptrace_put_breakpoints(child);
+   return ret;
+   }
   
   If a bp already exists, you're modifying it.  I thought the semantics
   of the new interface meant that you shoul return ENOSPC in this case,
   and a DEL would be necessary before adding another breakpoint.
   
  
  I'm not too sure what would be the desired behaviour for this interface,
  either way is fine with me. I'd like to hear from the GDB folks (copied
  in this email) to know what would please them.
 
 ENOSPC should be returned. The interface doesn't have provisions for
 modifying breakpoints. The client should delete/create instead of trying
 to modify.
 
 Since PTRACE_PPC_GETHWDEBUGINFO returns the number of available
 breakpoint registers, the client shouldn't (and GDB doesn't) try to set
 more breakpoints than possible.
 

Okay, I will modify the code accordingly.

@@ -1426,10 +1488,24 @@ static long ppc_del_hwdebug(struct task_struct 
*child, long addr, long data)
 #else
if (data != 1)
return -EINVAL;
+
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+   if (ptrace_get_breakpoints(child)  0)
+   return -ESRCH;
+
+   bp = thread-ptrace_bps[0];
+   if (bp) {
+   unregister_hw_breakpoint(bp);
+   thread-ptrace_bps[0] = NULL;
+   }
+   ptrace_put_breakpoints(child);
+   return 0;
   
   Shouldn't DEL return an error if there is no existing bp.
  
  
  Same comment as above. We'd like to know what behaviour would help the
  GDB use this interface better as there's no right or wrong way here.
 
 GDB expects DEL to return ENOENT is there's no existing bp.


Fine, here too. We'll return a -ENOENT here.

Thanks for your comments.

-- K.Prasad
 

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

[PATCH 1/2] [hw-breakpoint] Use generic hw-breakpoint interfaces for new PPC ptrace flags

2011-12-08 Thread K.Prasad 
PPC_PTRACE_GETHWDBGINFO, PPC_PTRACE_SETHWDEBUG and PPC_PTRACE_DELHWDEBUG are
PowerPC specific ptrace flags that use the watchpoint register. While they are
targeted primarily towards BookE users, user-space applications such as GDB
have started using them for BookS too. This patch enables the use of generic
hardware breakpoint interfaces for these new flags.

Apart from the usual benefits of using generic hw-breakpoint interfaces, these
changes allow debuggers (such as GDB) to use a common set of ptrace flags for
their watchpoint needs and allow more precise breakpoint specification (length
of the variable can be specified).

Signed-off-by: K.Prasad pra...@linux.vnet.ibm.com
---
 Documentation/powerpc/ptrace.txt |   16 
 arch/powerpc/kernel/ptrace.c |   77 +++---
 2 files changed, 87 insertions(+), 6 deletions(-)

diff --git a/Documentation/powerpc/ptrace.txt b/Documentation/powerpc/ptrace.txt
index f4a5499..f2a7a39 100644
--- a/Documentation/powerpc/ptrace.txt
+++ b/Documentation/powerpc/ptrace.txt
@@ -127,6 +127,22 @@ Some examples of using the structure to:
   p.addr2   = (uint64_t) end_range;
   p.condition_value = 0;
 
+- set a watchpoint in server processors (BookS)
+
+  p.version = 1;
+  p.trigger_type= PPC_BREAKPOINT_TRIGGER_RW;
+  p.addr_mode   = PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE;
+  or
+  p.addr_mode   = PPC_BREAKPOINT_MODE_EXACT;
+
+  p.condition_mode  = PPC_BREAKPOINT_CONDITION_NONE;
+  p.addr= (uint64_t) begin_range;
+  /* For PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE addr2 needs to be specified, where
+   * addr2 - addr = 8 Bytes.
+   */
+  p.addr2   = (uint64_t) end_range;
+  p.condition_value = 0;
+
 3. PTRACE_DELHWDEBUG
 
 Takes an integer which identifies an existing breakpoint or watchpoint
diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index 05b7dd2..cd41c78 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -1339,6 +1339,12 @@ static int set_dac_range(struct task_struct *child,
 static long ppc_set_hwdebug(struct task_struct *child,
 struct ppc_hw_breakpoint *bp_info)
 {
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+   int ret, len = 0;
+   struct thread_struct *thread = (child-thread);
+   struct perf_event *bp;
+   struct perf_event_attr attr;
+#endif /* CONFIG_HAVE_HW_BREAKPOINT */
 #ifndef CONFIG_PPC_ADV_DEBUG_REGS
unsigned long dabr;
 #endif
@@ -1382,13 +1388,9 @@ static long ppc_set_hwdebug(struct task_struct *child,
 */
if ((bp_info-trigger_type  PPC_BREAKPOINT_TRIGGER_RW) == 0 ||
(bp_info-trigger_type  ~PPC_BREAKPOINT_TRIGGER_RW) != 0 ||
-   bp_info-addr_mode != PPC_BREAKPOINT_MODE_EXACT ||
bp_info-condition_mode != PPC_BREAKPOINT_CONDITION_NONE)
return -EINVAL;
 
-   if (child-thread.dabr)
-   return -ENOSPC;
-
if ((unsigned long)bp_info-addr = TASK_SIZE)
return -EIO;
 
@@ -1398,15 +1400,63 @@ static long ppc_set_hwdebug(struct task_struct *child,
dabr |= DABR_DATA_READ;
if (bp_info-trigger_type  PPC_BREAKPOINT_TRIGGER_WRITE)
dabr |= DABR_DATA_WRITE;
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+   if (ptrace_get_breakpoints(child)  0)
+   return -ESRCH;
 
-   child-thread.dabr = dabr;
+   /*
+* Check if the request is for 'range' breakpoints. We can
+* support it if range  8 bytes.
+*/
+   if (bp_info-addr_mode == PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE) {
+   len = bp_info-addr2 - bp_info-addr;
+   } else if (bp_info-addr_mode != PPC_BREAKPOINT_MODE_EXACT) {
+   ptrace_put_breakpoints(child);
+   return -EINVAL;
+   }
+   bp = thread-ptrace_bps[0];
+   if (bp) {
+   ptrace_put_breakpoints(child);
+   return -ENOSPC;
+   }
+
+   /* Create a new breakpoint request if one doesn't exist already */
+   hw_breakpoint_init(attr);
+   attr.bp_addr = (unsigned long)bp_info-addr  ~HW_BREAKPOINT_ALIGN;
+   attr.bp_len = len;
+   arch_bp_generic_fields(dabr  (DABR_DATA_WRITE | DABR_DATA_READ),
+   attr.bp_type);
+
+   thread-ptrace_bps[0] = bp = register_user_hw_breakpoint(attr,
+  ptrace_triggered, NULL, child);
+   if (IS_ERR(bp)) {
+   thread-ptrace_bps[0] = NULL;
+   ptrace_put_breakpoints(child);
+   return PTR_ERR(bp);
+   }
 
+   ptrace_put_breakpoints(child);
+   return 1;
+#endif /* CONFIG_HAVE_HW_BREAKPOINT */
+
+   if (bp_info-addr_mode != PPC_BREAKPOINT_MODE_EXACT)
+   return -EINVAL;
+
+   if (child-thread.dabr)
+   return -ENOSPC;
+
+   child-thread.dabr = dabr;
return 1;
 #endif /* !CONFIG_PPC_ADV_DEBUG_DVCS */
 }

Re: [PATCH 1/2] [hw-breakpoint] Use generic hw-breakpoint interfaces for new PPC ptrace flags

2011-12-07 Thread Thiago Jung Bauermann 
On Thu, 2011-12-01 at 15:50 +0530, K.Prasad wrote:
 On Mon, Nov 28, 2011 at 02:11:11PM +1100, David Gibson wrote:
  [snip]
  On Wed, Oct 12, 2011 at 11:09:48PM +0530, K.Prasad wrote:
   diff --git a/Documentation/powerpc/ptrace.txt 
   b/Documentation/powerpc/ptrace.txt
   index f4a5499..f2a7a39 100644
   --- a/Documentation/powerpc/ptrace.txt
   +++ b/Documentation/powerpc/ptrace.txt
   @@ -127,6 +127,22 @@ Some examples of using the structure to:
  p.addr2   = (uint64_t) end_range;
  p.condition_value = 0;

   +- set a watchpoint in server processors (BookS)
   +
   +  p.version = 1;
   +  p.trigger_type= PPC_BREAKPOINT_TRIGGER_RW;
   +  p.addr_mode   = PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE;
   +  or
   +  p.addr_mode   = PPC_BREAKPOINT_MODE_EXACT;
   +
   +  p.condition_mode  = PPC_BREAKPOINT_CONDITION_NONE;
   +  p.addr= (uint64_t) begin_range;
  
  You should probably document the alignment constraint on the address
  here, too.
  
 
 Alignment constraints will be learnt by the user-space during runtime.
 We provide that as part of 'struct ppc_debug_info' in
 'data_bp_alignment' field.
 
 While the alignment is always 8-bytes for BookS, I think userspace
 should be left to learn it through PTRACE_PPC_GETHWDEBUGINFO.

Right. In particular, BookE doesn't have alignment constraints.

   + attr.bp_len = len;
   + ret =  modify_user_hw_breakpoint(bp, attr);
   + if (ret) {
   + ptrace_put_breakpoints(child);
   + return ret;
   + }
  
  If a bp already exists, you're modifying it.  I thought the semantics
  of the new interface meant that you shoul return ENOSPC in this case,
  and a DEL would be necessary before adding another breakpoint.
  
 
 I'm not too sure what would be the desired behaviour for this interface,
 either way is fine with me. I'd like to hear from the GDB folks (copied
 in this email) to know what would please them.

ENOSPC should be returned. The interface doesn't have provisions for
modifying breakpoints. The client should delete/create instead of trying
to modify.

Since PTRACE_PPC_GETHWDEBUGINFO returns the number of available
breakpoint registers, the client shouldn't (and GDB doesn't) try to set
more breakpoints than possible.
 
   @@ -1426,10 +1488,24 @@ static long ppc_del_hwdebug(struct task_struct 
   *child, long addr, long data)
#else
 if (data != 1)
 return -EINVAL;
   +
   +#ifdef CONFIG_HAVE_HW_BREAKPOINT
   + if (ptrace_get_breakpoints(child)  0)
   + return -ESRCH;
   +
   + bp = thread-ptrace_bps[0];
   + if (bp) {
   + unregister_hw_breakpoint(bp);
   + thread-ptrace_bps[0] = NULL;
   + }
   + ptrace_put_breakpoints(child);
   + return 0;
  
  Shouldn't DEL return an error if there is no existing bp.
 
 
 Same comment as above. We'd like to know what behaviour would help the
 GDB use this interface better as there's no right or wrong way here.

GDB expects DEL to return ENOENT is there's no existing bp.

-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center

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

Re: [PATCH 1/2] [hw-breakpoint] Use generic hw-breakpoint interfaces for new PPC ptrace flags

2011-12-01 Thread K.Prasad 
On Mon, Nov 28, 2011 at 02:11:11PM +1100, David Gibson wrote:
 [snip]
 On Wed, Oct 12, 2011 at 11:09:48PM +0530, K.Prasad wrote:
+   if (bp) {
+   attr = bp-attr;
+   attr.bp_addr = (unsigned long)bp_info-addr  
~HW_BREAKPOINT_ALIGN;
+   arch_bp_generic_fields(dabr 
+   (DABR_DATA_WRITE | 
DABR_DATA_READ),
+   attr.bp_type);
+   attr.bp_len = len;
   
   If gdb is using the new breakpoint interface, surely it should just
   use it, rather than doing this bit frobbing as in the old SET_DABR
   call.
   
  
  I understand that you wanted to avoid this duplication of effort in terms
  of encoding and decoding the breakpoint type from
  PPC_BREAKPOINT_TRIGGER_READ to DABR_DATA_READ to HW_BREAKPOINT_R.
  
  However HW_BREAKPOINT_R is a generic definition used across
  architectures, DABR_DATA_READ is used in the !CONFIG_HAVE_HW_BREAKPOINT
  case while PPC_BREAKPOINT_TRIGGER_READ is used in
  CONFIG_PPC_ADV_DEBUG_REGS case.
  
  While we could define PPC_BREAKPOINT_TRIGGER_READ and DABR_DATA_READ to
  the same value it may not result in any code savings (since the bit
  translation is done for !CONFIG_HAVE_HW_BREAKPOINT case anyway). So, I
  think it is best left the way it is.
 
 That's not what I'm suggesting.  What I'm saying is that ig userspace
 is using the new generic interface, then it should just set the
 bp_type field, and it should not use the DABR_DATA_{READ,WRITE} bits.
 The DABR_DATA bits should *only* be processed in the legacy interface,
 never in the generic interface.
 

The DABR_DATA_{READ,WRITE} bits are neither set by the user, nor
expected by the hw-breakpoint interface. It is an intermediate code used
to re-use the arch_bp_generic_fields function. We could convert directly
from PPC_BREAKPOINT_TRIGGER_READ to HW_BREAKPOINT_R (using a switch-case)
but that may not result in any code savings.

DABR_DATA_{READ,WRITE} is indeed legacy and cannot be set by user-space
for a PPC_PTRACE_SETHWDEBUG + CONFIG_HAVE_HW_BREAKPOINT combination.

[snipped]

  diff --git a/Documentation/powerpc/ptrace.txt 
  b/Documentation/powerpc/ptrace.txt
  index f4a5499..f2a7a39 100644
  --- a/Documentation/powerpc/ptrace.txt
  +++ b/Documentation/powerpc/ptrace.txt
  @@ -127,6 +127,22 @@ Some examples of using the structure to:
 p.addr2   = (uint64_t) end_range;
 p.condition_value = 0;
   
  +- set a watchpoint in server processors (BookS)
  +
  +  p.version = 1;
  +  p.trigger_type= PPC_BREAKPOINT_TRIGGER_RW;
  +  p.addr_mode   = PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE;
  +  or
  +  p.addr_mode   = PPC_BREAKPOINT_MODE_EXACT;
  +
  +  p.condition_mode  = PPC_BREAKPOINT_CONDITION_NONE;
  +  p.addr= (uint64_t) begin_range;
 
 You should probably document the alignment constraint on the address
 here, too.
 

Alignment constraints will be learnt by the user-space during runtime.
We provide that as part of 'struct ppc_debug_info' in
'data_bp_alignment' field.

While the alignment is always 8-bytes for BookS, I think userspace
should be left to learn it through PTRACE_PPC_GETHWDEBUGINFO.

  +  /* For PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE addr2 needs to be specified, 
  where
  +   * addr2 - addr = 8 Bytes.
  +   */
  +  p.addr2   = (uint64_t) end_range;
  +  p.condition_value = 0;
  +
   3. PTRACE_DELHWDEBUG
   
   Takes an integer which identifies an existing breakpoint or watchpoint
  diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
  index 05b7dd2..be5dc57 100644
  --- a/arch/powerpc/kernel/ptrace.c
  +++ b/arch/powerpc/kernel/ptrace.c
  @@ -1339,6 +1339,12 @@ static int set_dac_range(struct task_struct *child,
   static long ppc_set_hwdebug(struct task_struct *child,
   struct ppc_hw_breakpoint *bp_info)
   {
  +#ifdef CONFIG_HAVE_HW_BREAKPOINT
  +   int ret, len = 0;
  +   struct thread_struct *thread = (child-thread);
  +   struct perf_event *bp;
  +   struct perf_event_attr attr;
  +#endif /* CONFIG_HAVE_HW_BREAKPOINT */
   #ifndef CONFIG_PPC_ADV_DEBUG_REGS
  unsigned long dabr;
   #endif
  @@ -1382,13 +1388,9 @@ static long ppc_set_hwdebug(struct task_struct 
  *child,
   */
  if ((bp_info-trigger_type  PPC_BREAKPOINT_TRIGGER_RW) == 0 ||
  (bp_info-trigger_type  ~PPC_BREAKPOINT_TRIGGER_RW) != 0 ||
  -   bp_info-addr_mode != PPC_BREAKPOINT_MODE_EXACT ||
  bp_info-condition_mode != PPC_BREAKPOINT_CONDITION_NONE)
  return -EINVAL;
   
  -   if (child-thread.dabr)
  -   return -ENOSPC;
  -
  if ((unsigned long)bp_info-addr = TASK_SIZE)
  return -EIO;
   
  @@ -1398,15 +1400,75 @@ static long ppc_set_hwdebug(struct task_struct 
  *child,
  dabr |= DABR_DATA_READ;
  if (bp_info-trigger_type  PPC_BREAKPOINT_TRIGGER_WRITE)
  dabr |= DABR_DATA_WRITE;
  +#ifdef

Re: [PATCH 1/2] [hw-breakpoint] Use generic hw-breakpoint interfaces for new PPC ptrace flags

2011-11-27 Thread David Gibson 
[snip]
On Wed, Oct 12, 2011 at 11:09:48PM +0530, K.Prasad wrote:
   + if (bp) {
   + attr = bp-attr;
   + attr.bp_addr = (unsigned long)bp_info-addr  
   ~HW_BREAKPOINT_ALIGN;
   + arch_bp_generic_fields(dabr 
   + (DABR_DATA_WRITE | DABR_DATA_READ),
   + attr.bp_type);
   + attr.bp_len = len;
  
  If gdb is using the new breakpoint interface, surely it should just
  use it, rather than doing this bit frobbing as in the old SET_DABR
  call.
  
 
 I understand that you wanted to avoid this duplication of effort in terms
 of encoding and decoding the breakpoint type from
 PPC_BREAKPOINT_TRIGGER_READ to DABR_DATA_READ to HW_BREAKPOINT_R.
 
 However HW_BREAKPOINT_R is a generic definition used across
 architectures, DABR_DATA_READ is used in the !CONFIG_HAVE_HW_BREAKPOINT
 case while PPC_BREAKPOINT_TRIGGER_READ is used in
 CONFIG_PPC_ADV_DEBUG_REGS case.
 
 While we could define PPC_BREAKPOINT_TRIGGER_READ and DABR_DATA_READ to
 the same value it may not result in any code savings (since the bit
 translation is done for !CONFIG_HAVE_HW_BREAKPOINT case anyway). So, I
 think it is best left the way it is.

That's not what I'm suggesting.  What I'm saying is that ig userspace
is using the new generic interface, then it should just set the
bp_type field, and it should not use the DABR_DATA_{READ,WRITE} bits.
The DABR_DATA bits should *only* be processed in the legacy interface,
never in the generic interface.

 I'm attaching the revised patch (after incorporating your comments).
 Kindly let me know your comments.
 
 Thanks,
 K.Prasad
 -
 
 [hw-breakpoint] Use generic hw-breakpoint interfaces for new PPC ptrace flags
 
 PPC_PTRACE_GETHWDBGINFO, PPC_PTRACE_SETHWDEBUG and PPC_PTRACE_DELHWDEBUG are
 PowerPC specific ptrace flags that use the watchpoint register. While they are
 targeted primarily towards BookE users, user-space applications such as GDB
 have started using them for BookS too. This patch enables the use of generic
 hardware breakpoint interfaces for these new flags.
 
 Apart from the usual benefits of using generic hw-breakpoint interfaces, these
 changes allow debuggers (such as GDB) to use a common set of ptrace flags for
 their watchpoint needs and allow more precise breakpoint specification (length
 of the variable can be specified).
 
 Tested-by: Edjunior Barbosa Machado emach...@linux.vnet.ibm.com
 Signed-off-by: K.Prasad pra...@linux.vnet.ibm.com
 ---
  Documentation/powerpc/ptrace.txt |   16 +++
  arch/powerpc/kernel/ptrace.c |   88 
 +++---
  2 files changed, 98 insertions(+), 6 deletions(-)
 
 diff --git a/Documentation/powerpc/ptrace.txt 
 b/Documentation/powerpc/ptrace.txt
 index f4a5499..f2a7a39 100644
 --- a/Documentation/powerpc/ptrace.txt
 +++ b/Documentation/powerpc/ptrace.txt
 @@ -127,6 +127,22 @@ Some examples of using the structure to:
p.addr2   = (uint64_t) end_range;
p.condition_value = 0;
  
 +- set a watchpoint in server processors (BookS)
 +
 +  p.version = 1;
 +  p.trigger_type= PPC_BREAKPOINT_TRIGGER_RW;
 +  p.addr_mode   = PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE;
 +  or
 +  p.addr_mode   = PPC_BREAKPOINT_MODE_EXACT;
 +
 +  p.condition_mode  = PPC_BREAKPOINT_CONDITION_NONE;
 +  p.addr= (uint64_t) begin_range;

You should probably document the alignment constraint on the address
here, too.

 +  /* For PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE addr2 needs to be specified, 
 where
 +   * addr2 - addr = 8 Bytes.
 +   */
 +  p.addr2   = (uint64_t) end_range;
 +  p.condition_value = 0;
 +
  3. PTRACE_DELHWDEBUG
  
  Takes an integer which identifies an existing breakpoint or watchpoint
 diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
 index 05b7dd2..be5dc57 100644
 --- a/arch/powerpc/kernel/ptrace.c
 +++ b/arch/powerpc/kernel/ptrace.c
 @@ -1339,6 +1339,12 @@ static int set_dac_range(struct task_struct *child,
  static long ppc_set_hwdebug(struct task_struct *child,
struct ppc_hw_breakpoint *bp_info)
  {
 +#ifdef CONFIG_HAVE_HW_BREAKPOINT
 + int ret, len = 0;
 + struct thread_struct *thread = (child-thread);
 + struct perf_event *bp;
 + struct perf_event_attr attr;
 +#endif /* CONFIG_HAVE_HW_BREAKPOINT */
  #ifndef CONFIG_PPC_ADV_DEBUG_REGS
   unsigned long dabr;
  #endif
 @@ -1382,13 +1388,9 @@ static long ppc_set_hwdebug(struct task_struct *child,
*/
   if ((bp_info-trigger_type  PPC_BREAKPOINT_TRIGGER_RW) == 0 ||
   (bp_info-trigger_type  ~PPC_BREAKPOINT_TRIGGER_RW) != 0 ||
 - bp_info-addr_mode != PPC_BREAKPOINT_MODE_EXACT ||
   bp_info-condition_mode != PPC_BREAKPOINT_CONDITION_NONE)
   return -EINVAL;
  
 - if (child-thread.dabr)
 - return -ENOSPC;
 -
   if ((unsigned long)bp_info-addr = TASK_SIZE)
   return -EIO;
  
 @@ -1398,15

Re: [PATCH 1/2] [hw-breakpoint] Use generic hw-breakpoint interfaces for new PPC ptrace flags

2011-10-12 Thread K.Prasad 
On Wed, Oct 12, 2011 at 02:33:59PM +1100, David Gibson wrote:
 On Fri, Sep 16, 2011 at 12:57:10PM +0530, K.Prasad wrote:
  On Fri, Aug 26, 2011 at 03:05:52PM +0530, K.Prasad wrote:
   On Wed, Aug 24, 2011 at 01:59:39PM +1000, David Gibson wrote:
On Tue, Aug 23, 2011 at 02:55:13PM +0530, K.Prasad wrote:
 On Tue, Aug 23, 2011 at 03:08:50PM +1000, David Gibson wrote:
  On Fri, Aug 19, 2011 at 01:21:36PM +0530, K.Prasad wrote:
[snipped]
  [hw-breakpoint] Use generic hw-breakpoint interfaces for new PPC ptrace 
  flags
  
  PPC_PTRACE_GETHWDBGINFO, PPC_PTRACE_SETHWDEBUG and 
  PPC_PTRACE_DELHWDEBUG are
  PowerPC specific ptrace flags that use the watchpoint register. While 
  they are
  targeted primarily towards BookE users, user-space applications such as 
  GDB
  have started using them for BookS too.
  
  This patch enables the use of generic hardware breakpoint interfaces 
  for these
  new flags. The version number of the associated data structures
  ppc_hw_breakpoint and ppc_debug_info is incremented to denote new 
  semantics.
 
 Above pargraph needs revision.
 
 

Sure, done.

  Apart from the usual benefits of using generic hw-breakpoint 
  interfaces, these
  changes allow debuggers (such as GDB) to use a common set of ptrace 
  flags for
  their watchpoint needs and allow more precise breakpoint specification 
  (length
  of the variable can be specified).
  
  [Edjunior: Identified an issue in the patch with the sanity check for 
  version
  numbers]
  
  Tested-by: Edjunior Barbosa Machado emach...@linux.vnet.ibm.com
  Signed-off-by: K.Prasad pra...@linux.vnet.ibm.com
  
  diff --git a/Documentation/powerpc/ptrace.txt 
  b/Documentation/powerpc/ptrace.txt
  index f4a5499..04656ec 100644
  --- a/Documentation/powerpc/ptrace.txt
  +++ b/Documentation/powerpc/ptrace.txt
  @@ -127,6 +127,22 @@ Some examples of using the structure to:
 p.addr2   = (uint64_t) end_range;
 p.condition_value = 0;
   
  +- set a watchpoint in server processors (BookS)
  +
  +  p.version = 1;
  +  p.trigger_type= PPC_BREAKPOINT_TRIGGER_RW;
  +  p.addr_mode   = PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE;
  +  or
  +  p.addr_mode   = PPC_BREAKPOINT_MODE_RANGE_EXACT;
 
 MODE_RANGE_EXACT?  Shouldn't that just be MODE_EXACT?
 

That was silly. Thanks for pointing it out.

  +
  +  p.condition_mode  = PPC_BREAKPOINT_CONDITION_NONE;
  +  p.addr= (uint64_t) begin_range;
  +  /* For PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE addr2 needs to be specified, 
  where
  +   * addr2 - addr = 8 Bytes.
  +   */
  +  p.addr2   = (uint64_t) end_range;
  +  p.condition_value = 0;
  +
   3. PTRACE_DELHWDEBUG
   
   Takes an integer which identifies an existing breakpoint or watchpoint
  diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
  index 05b7dd2..2449495 100644
  --- a/arch/powerpc/kernel/ptrace.c
  +++ b/arch/powerpc/kernel/ptrace.c
  @@ -1339,6 +1339,12 @@ static int set_dac_range(struct task_struct *child,
   static long ppc_set_hwdebug(struct task_struct *child,
   struct ppc_hw_breakpoint *bp_info)
   {
  +#ifdef CONFIG_HAVE_HW_BREAKPOINT
  +   int ret, len = 0;
  +   struct thread_struct *thread = (child-thread);
  +   struct perf_event *bp;
  +   struct perf_event_attr attr;
  +#endif /* CONFIG_HAVE_HW_BREAKPOINT */
   #ifndef CONFIG_PPC_ADV_DEBUG_REGS
  unsigned long dabr;
   #endif
  @@ -1382,13 +1388,9 @@ static long ppc_set_hwdebug(struct task_struct 
  *child,
   */
  if ((bp_info-trigger_type  PPC_BREAKPOINT_TRIGGER_RW) == 0 ||
  (bp_info-trigger_type  ~PPC_BREAKPOINT_TRIGGER_RW) != 0 ||
  -   bp_info-addr_mode != PPC_BREAKPOINT_MODE_EXACT ||
  bp_info-condition_mode != PPC_BREAKPOINT_CONDITION_NONE)
  return -EINVAL;
   
  -   if (child-thread.dabr)
  -   return -ENOSPC;
  -
  if ((unsigned long)bp_info-addr = TASK_SIZE)
  return -EIO;
   
  @@ -1398,15 +1400,83 @@ static long ppc_set_hwdebug(struct task_struct 
  *child,
  dabr |= DABR_DATA_READ;
  if (bp_info-trigger_type  PPC_BREAKPOINT_TRIGGER_WRITE)
  dabr |= DABR_DATA_WRITE;
  +#ifdef CONFIG_HAVE_HW_BREAKPOINT
  +   if (ptrace_get_breakpoints(child)  0)
  +   return -ESRCH;
   
  -   child-thread.dabr = dabr;
  +   bp = thread-ptrace_bps[0];
  +   if (!bp_info-addr) {
 
 I think this is treating a hardware breakpoint at address 0 as if it
 didn't exist.  That seems wrong.
 

Yes, I think the above the condition need not exist after we've agreed
that 0 is a valid address to set breakpoint upon. One needs to use
PPC_PTRACE_DELHWDEBUG to delete a breakpoint and not by writing 0 (as
done with PTRACE_SET_DEBUGREG).

I'll remove that.

  +   if (bp) {
  +   unregister_hw_breakpoint(bp);
  +   thread-ptrace_bps[0] = NULL;
  +   }
  +

Re: [PATCH 1/2] [hw-breakpoint] Use generic hw-breakpoint interfaces for new PPC ptrace flags

2011-10-11 Thread David Gibson 
On Fri, Sep 16, 2011 at 12:57:10PM +0530, K.Prasad wrote:
 On Fri, Aug 26, 2011 at 03:05:52PM +0530, K.Prasad wrote:
  On Wed, Aug 24, 2011 at 01:59:39PM +1000, David Gibson wrote:
   On Tue, Aug 23, 2011 at 02:55:13PM +0530, K.Prasad wrote:
On Tue, Aug 23, 2011 at 03:08:50PM +1000, David Gibson wrote:
 On Fri, Aug 19, 2011 at 01:21:36PM +0530, K.Prasad wrote:
  PPC_PTRACE_GETHWDBGINFO, PPC_PTRACE_SETHWDEBUG and 
  PPC_PTRACE_DELHWDEBUG are
  PowerPC specific ptrace flags that use the watchpoint register. 
  While they are
  targeted primarily towards BookE users, user-space applications 
  such as GDB
  have started using them for BookS too.
  
  This patch enables the use of generic hardware breakpoint 
  interfaces for these
  new flags. The version number of the associated data structures
  ppc_hw_breakpoint and ppc_debug_info is incremented to denote 
  new semantics.
 
 So, the structure itself doesn't seem to have been extended.  I don't
 understand what the semantic difference is - your patch comment needs
 to explain this clearly.


We had a request to extend the structure but thought it was dangerous to
do so. For instance if the user-space used version1 of the structure,
while kernel did a copy_to_user() pertaining to version2, then we'd run
into problems. Unfortunately the ptrace flags weren't designed to accept
a version number as input from the user through the
PPC_PTRACE_GETHWDBGINFO flag (which would have solved this issue).
   
   I still don't follow you.
   
  
  Two things here.
  
  One, the change of semantics warranted an increment of the version
  number. The new semantics accepts PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE on
  BookS, while the old version number did not. I've added a small comment
  in the code to this effect.
  
  Two, regarding changes in the ppc_hw_breakpoint and ppc_debug_info
  structures - we would like to add more members to it if we can (GDB has a
  pending request to add more members to it). However the problem foreseen
  is that there could be a mismatch between the versions of the structure
  used by the user vs kernel-space i.e. if a new version of the structure,
  known to the kernel, had an extra member while the user-space still had
  the old version, then it becomes dangerous because the __copy_to_user
  function would overflow the buffer size in user-space.
  
  This could have been avoided if PPC_PTRACE_GETHWDBGINFO was originally
  designed to accept a version number (and provide corresponding
  struct ppc_debug_info) rather than send a populated ppc_debug_info
  structure along with the version number.
 
 
 Based on further discussions with the code-reviewer (David Gibson
 d...@au1.ibm.com), it was decided that incrementing the version number
 for the proposed changes is unnecessary as the patch only introduces new
 features but not a change in semantics.
 
 Please find a new version of the patch where the version number is
 retained as 1, along with the other planned changes.
 
 Thanks,
 K.Prasad
 
  
 [hw-breakpoint] Use generic hw-breakpoint interfaces for new PPC ptrace 
 flags
 
 PPC_PTRACE_GETHWDBGINFO, PPC_PTRACE_SETHWDEBUG and PPC_PTRACE_DELHWDEBUG 
 are
 PowerPC specific ptrace flags that use the watchpoint register. While 
 they are
 targeted primarily towards BookE users, user-space applications such as 
 GDB
 have started using them for BookS too.
 
 This patch enables the use of generic hardware breakpoint interfaces for 
 these
 new flags. The version number of the associated data structures
 ppc_hw_breakpoint and ppc_debug_info is incremented to denote new 
 semantics.

Above pargraph needs revision.


 Apart from the usual benefits of using generic hw-breakpoint interfaces, 
 these
 changes allow debuggers (such as GDB) to use a common set of ptrace flags 
 for
 their watchpoint needs and allow more precise breakpoint specification 
 (length
 of the variable can be specified).
 
 [Edjunior: Identified an issue in the patch with the sanity check for 
 version
 numbers]
 
 Tested-by: Edjunior Barbosa Machado emach...@linux.vnet.ibm.com
 Signed-off-by: K.Prasad pra...@linux.vnet.ibm.com
 
 diff --git a/Documentation/powerpc/ptrace.txt 
 b/Documentation/powerpc/ptrace.txt
 index f4a5499..04656ec 100644
 --- a/Documentation/powerpc/ptrace.txt
 +++ b/Documentation/powerpc/ptrace.txt
 @@ -127,6 +127,22 @@ Some examples of using the structure to:
p.addr2   = (uint64_t) end_range;
p.condition_value = 0;
  
 +- set a watchpoint in server processors (BookS)
 +
 +  p.version = 1;
 +  p.trigger_type= PPC_BREAKPOINT_TRIGGER_RW;
 +  p.addr_mode   = PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE;
 +  or
 +  p.addr_mode   = PPC_BREAKPOINT_MODE_RANGE_EXACT;

MODE_RANGE_EXACT?  Shouldn't that just be MODE_EXACT?

 +
 +  p.condition_mode  =

Re: [PATCH 1/2] [hw-breakpoint] Use generic hw-breakpoint interfaces for new PPC ptrace flags

2011-09-16 Thread K.Prasad 
On Fri, Aug 26, 2011 at 03:05:52PM +0530, K.Prasad wrote:
 On Wed, Aug 24, 2011 at 01:59:39PM +1000, David Gibson wrote:
  On Tue, Aug 23, 2011 at 02:55:13PM +0530, K.Prasad wrote:
   On Tue, Aug 23, 2011 at 03:08:50PM +1000, David Gibson wrote:
On Fri, Aug 19, 2011 at 01:21:36PM +0530, K.Prasad wrote:
 PPC_PTRACE_GETHWDBGINFO, PPC_PTRACE_SETHWDEBUG and 
 PPC_PTRACE_DELHWDEBUG are
 PowerPC specific ptrace flags that use the watchpoint register. While 
 they are
 targeted primarily towards BookE users, user-space applications such 
 as GDB
 have started using them for BookS too.
 
 This patch enables the use of generic hardware breakpoint interfaces 
 for these
 new flags. The version number of the associated data structures
 ppc_hw_breakpoint and ppc_debug_info is incremented to denote new 
 semantics.

So, the structure itself doesn't seem to have been extended.  I don't
understand what the semantic difference is - your patch comment needs
to explain this clearly.
   
   
   We had a request to extend the structure but thought it was dangerous to
   do so. For instance if the user-space used version1 of the structure,
   while kernel did a copy_to_user() pertaining to version2, then we'd run
   into problems. Unfortunately the ptrace flags weren't designed to accept
   a version number as input from the user through the
   PPC_PTRACE_GETHWDBGINFO flag (which would have solved this issue).
  
  I still don't follow you.
  
 
 Two things here.
 
 One, the change of semantics warranted an increment of the version
 number. The new semantics accepts PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE on
 BookS, while the old version number did not. I've added a small comment
 in the code to this effect.
 
 Two, regarding changes in the ppc_hw_breakpoint and ppc_debug_info
 structures - we would like to add more members to it if we can (GDB has a
 pending request to add more members to it). However the problem foreseen
 is that there could be a mismatch between the versions of the structure
 used by the user vs kernel-space i.e. if a new version of the structure,
 known to the kernel, had an extra member while the user-space still had
 the old version, then it becomes dangerous because the __copy_to_user
 function would overflow the buffer size in user-space.
 
 This could have been avoided if PPC_PTRACE_GETHWDBGINFO was originally
 designed to accept a version number (and provide corresponding
 struct ppc_debug_info) rather than send a populated ppc_debug_info
 structure along with the version number.


Based on further discussions with the code-reviewer (David Gibson
d...@au1.ibm.com), it was decided that incrementing the version number
for the proposed changes is unnecessary as the patch only introduces new
features but not a change in semantics.

Please find a new version of the patch where the version number is
retained as 1, along with the other planned changes.

Thanks,
K.Prasad

 
[hw-breakpoint] Use generic hw-breakpoint interfaces for new PPC ptrace 
flags

PPC_PTRACE_GETHWDBGINFO, PPC_PTRACE_SETHWDEBUG and PPC_PTRACE_DELHWDEBUG are
PowerPC specific ptrace flags that use the watchpoint register. While they 
are
targeted primarily towards BookE users, user-space applications such as GDB
have started using them for BookS too.

This patch enables the use of generic hardware breakpoint interfaces for 
these
new flags. The version number of the associated data structures
ppc_hw_breakpoint and ppc_debug_info is incremented to denote new 
semantics.

Apart from the usual benefits of using generic hw-breakpoint interfaces, 
these
changes allow debuggers (such as GDB) to use a common set of ptrace flags 
for
their watchpoint needs and allow more precise breakpoint specification 
(length
of the variable can be specified).

[Edjunior: Identified an issue in the patch with the sanity check for 
version
numbers]

Tested-by: Edjunior Barbosa Machado emach...@linux.vnet.ibm.com
Signed-off-by: K.Prasad pra...@linux.vnet.ibm.com

diff --git a/Documentation/powerpc/ptrace.txt b/Documentation/powerpc/ptrace.txt
index f4a5499..04656ec 100644
--- a/Documentation/powerpc/ptrace.txt
+++ b/Documentation/powerpc/ptrace.txt
@@ -127,6 +127,22 @@ Some examples of using the structure to:
   p.addr2   = (uint64_t) end_range;
   p.condition_value = 0;
 
+- set a watchpoint in server processors (BookS)
+
+  p.version = 1;
+  p.trigger_type= PPC_BREAKPOINT_TRIGGER_RW;
+  p.addr_mode   = PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE;
+  or
+  p.addr_mode   = PPC_BREAKPOINT_MODE_RANGE_EXACT;
+
+  p.condition_mode  = PPC_BREAKPOINT_CONDITION_NONE;
+  p.addr= (uint64_t) begin_range;
+  /* For PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE addr2 needs to be specified, where
+   * addr2 - addr = 8 Bytes.
+   */
+  p.addr2   = (uint64_t) end_range;
+  p.condition_value = 0;
+

Re: [PATCH 1/2] [hw-breakpoint] Use generic hw-breakpoint interfaces for new PPC ptrace flags

2011-08-26 Thread K.Prasad 
On Wed, Aug 24, 2011 at 01:59:39PM +1000, David Gibson wrote:
 On Tue, Aug 23, 2011 at 02:55:13PM +0530, K.Prasad wrote:
  On Tue, Aug 23, 2011 at 03:08:50PM +1000, David Gibson wrote:
   On Fri, Aug 19, 2011 at 01:21:36PM +0530, K.Prasad wrote:
PPC_PTRACE_GETHWDBGINFO, PPC_PTRACE_SETHWDEBUG and 
PPC_PTRACE_DELHWDEBUG are
PowerPC specific ptrace flags that use the watchpoint register. While 
they are
targeted primarily towards BookE users, user-space applications such as 
GDB
have started using them for BookS too.

This patch enables the use of generic hardware breakpoint interfaces 
for these
new flags. The version number of the associated data structures
ppc_hw_breakpoint and ppc_debug_info is incremented to denote new 
semantics.
   
   So, the structure itself doesn't seem to have been extended.  I don't
   understand what the semantic difference is - your patch comment needs
   to explain this clearly.
  
  
  We had a request to extend the structure but thought it was dangerous to
  do so. For instance if the user-space used version1 of the structure,
  while kernel did a copy_to_user() pertaining to version2, then we'd run
  into problems. Unfortunately the ptrace flags weren't designed to accept
  a version number as input from the user through the
  PPC_PTRACE_GETHWDBGINFO flag (which would have solved this issue).
 
 I still don't follow you.
 

Two things here.

One, the change of semantics warranted an increment of the version
number. The new semantics accepts PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE on
BookS, while the old version number did not. I've added a small comment
in the code to this effect.

Two, regarding changes in the ppc_hw_breakpoint and ppc_debug_info
structures - we would like to add more members to it if we can (GDB has a
pending request to add more members to it). However the problem foreseen
is that there could be a mismatch between the versions of the structure
used by the user vs kernel-space i.e. if a new version of the structure,
known to the kernel, had an extra member while the user-space still had
the old version, then it becomes dangerous because the __copy_to_user
function would overflow the buffer size in user-space.

This could have been avoided if PPC_PTRACE_GETHWDBGINFO was originally
designed to accept a version number (and provide corresponding
struct ppc_debug_info) rather than send a populated ppc_debug_info
structure along with the version number.

  I'll add a comment w.r.t change in semantics - such as the ability to
  accept 'range' breakpoints in BookS.
   
Apart from the usual benefits of using generic hw-breakpoint 
interfaces, these
changes allow debuggers (such as GDB) to use a common set of ptrace 
flags for
their watchpoint needs and allow more precise breakpoint specification 
(length
of the variable can be specified).
   
   What is the mechanism for implementing the range breakpoint on book3s?
   
  
  The hw-breakpoint interface, accepts length as an argument in BookS (any
  value = 8 Bytes) and would filter out extraneous interrupts arising out
  of accesses outside the range comprising addr, addr + len inside
  hw_breakpoint_handler function.
  
  We put that ability to use here.
 
 Ah, so in hardware the breakpoints are always 8 bytes long, but you
 filter out false hits on a shorter range?  Of course, the utility of
 range breakpoints is questionable when length =8, but the start must
 be aligned on an 8-byte boundary.
 

Yes, we ensure that through 
+   attr.bp_addr = (unsigned long)bp_info-addr  ~HW_BREAKPOINT_ALIGN;

 [snip]
if ((unsigned long)bp_info-addr = TASK_SIZE)
return -EIO;
 
@@ -1398,15 +1400,86 @@ static long ppc_set_hwdebug(struct task_struct 
*child,
dabr |= DABR_DATA_READ;
if (bp_info-trigger_type  PPC_BREAKPOINT_TRIGGER_WRITE)
dabr |= DABR_DATA_WRITE;
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+   if (bp_info-version == 1)
+   goto version_one;
   
   There are several legitimate uses of goto in the kernel, but this is
   definitely not one of them.  You're essentially using it to put the
   old and new versions of the same function in one block.  Nasty.
   
  
  Maybe it's the label that's causing bother here. It might look elegant
  if it was called something like exit_* or error_* :-)
  
  The goto here helps reduce code, is similar to the error exits we use
  everywhere.
 
 Rubbish, it is not an exception exit at all, it is two separate code
 paths for the different versions which would be much clearer as two
 different functions.
 

I've re-written this part of the code to avoid a goto statement.

+   if (ptrace_get_breakpoints(child)  0)
+   return -ESRCH;
 
-   child-thread.dabr = dabr;
+   bp = thread-ptrace_bps[0];
+   if (!bp_info-addr) {
+   if (bp) {

Re: [PATCH 1/2] [hw-breakpoint] Use generic hw-breakpoint interfaces for new PPC ptrace flags

2011-08-23 Thread K.Prasad 
On Tue, Aug 23, 2011 at 03:08:50PM +1000, David Gibson wrote:
 On Fri, Aug 19, 2011 at 01:21:36PM +0530, K.Prasad wrote:
  PPC_PTRACE_GETHWDBGINFO, PPC_PTRACE_SETHWDEBUG and PPC_PTRACE_DELHWDEBUG are
  PowerPC specific ptrace flags that use the watchpoint register. While they 
  are
  targeted primarily towards BookE users, user-space applications such as GDB
  have started using them for BookS too.
  
  This patch enables the use of generic hardware breakpoint interfaces for 
  these
  new flags. The version number of the associated data structures
  ppc_hw_breakpoint and ppc_debug_info is incremented to denote new 
  semantics.
 
 So, the structure itself doesn't seem to have been extended.  I don't
 understand what the semantic difference is - your patch comment needs
 to explain this clearly.


We had a request to extend the structure but thought it was dangerous to
do so. For instance if the user-space used version1 of the structure,
while kernel did a copy_to_user() pertaining to version2, then we'd run
into problems. Unfortunately the ptrace flags weren't designed to accept
a version number as input from the user through the
PPC_PTRACE_GETHWDBGINFO flag (which would have solved this issue).

I'll add a comment w.r.t change in semantics - such as the ability to
accept 'range' breakpoints in BookS.
 
  Apart from the usual benefits of using generic hw-breakpoint interfaces, 
  these
  changes allow debuggers (such as GDB) to use a common set of ptrace flags 
  for
  their watchpoint needs and allow more precise breakpoint specification 
  (length
  of the variable can be specified).
 
 What is the mechanism for implementing the range breakpoint on book3s?
 

The hw-breakpoint interface, accepts length as an argument in BookS (any
value = 8 Bytes) and would filter out extraneous interrupts arising out
of accesses outside the range comprising addr, addr + len inside
hw_breakpoint_handler function.

We put that ability to use here.

  [Edjunior: Identified an issue in the patch with the sanity check for 
  version
  numbers]
  
  Tested-by: Edjunior Barbosa Machado emach...@linux.vnet.ibm.com
  Signed-off-by: K.Prasad pra...@linux.vnet.ibm.com
  ---
   Documentation/powerpc/ptrace.txt |   16 ++
   arch/powerpc/kernel/ptrace.c |  104 
  +++---
   2 files changed, 112 insertions(+), 8 deletions(-)
  
  diff --git a/Documentation/powerpc/ptrace.txt 
  b/Documentation/powerpc/ptrace.txt
  index f4a5499..97301ae 100644
  --- a/Documentation/powerpc/ptrace.txt
  +++ b/Documentation/powerpc/ptrace.txt
  @@ -127,6 +127,22 @@ Some examples of using the structure to:
 p.addr2   = (uint64_t) end_range;
 p.condition_value = 0;
   
  +- set a watchpoint in server processors (BookS) using version 2
  +
  +  p.version = 2;
  +  p.trigger_type= PPC_BREAKPOINT_TRIGGER_RW;
  +  p.addr_mode   = PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE;
  +  or
  +  p.addr_mode   = PPC_BREAKPOINT_MODE_RANGE_EXACT;
  +
  +  p.condition_mode  = PPC_BREAKPOINT_CONDITION_NONE;
  +  p.addr= (uint64_t) begin_range;
  +  /* For PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE addr2 needs to be specified, 
  where
  +   * addr2 - addr = 8 Bytes.
  +   */
  +  p.addr2   = (uint64_t) end_range;
  +  p.condition_value = 0;
  +
   3. PTRACE_DELHWDEBUG
   
   Takes an integer which identifies an existing breakpoint or watchpoint
  diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
  index 05b7dd2..18d28b6 100644
  --- a/arch/powerpc/kernel/ptrace.c
  +++ b/arch/powerpc/kernel/ptrace.c
  @@ -1339,11 +1339,17 @@ static int set_dac_range(struct task_struct *child,
   static long ppc_set_hwdebug(struct task_struct *child,
   struct ppc_hw_breakpoint *bp_info)
   {
  +#ifdef CONFIG_HAVE_HW_BREAKPOINT
  +   int ret, len = 0;
  +   struct thread_struct *thread = (child-thread);
  +   struct perf_event *bp;
  +   struct perf_event_attr attr;
  +#endif /* CONFIG_HAVE_HW_BREAKPOINT */
 
 I'm confused.  This compiled before on book3s, and I don't see any
 changes to Makefile or Kconfig in the patch that will result in this
 code compiling  when it previously didn't   Why are these new guards
 added?
 

The code is guarded using the CONFIG_ flags for two reasons.
a) We don't want the code to be included for BookE and other
architectures.
b) In BookS, we're now adding a new ability based on whether
CONFIG_HAVE_HW_BREAKPOINT is defined. Presently this config option is
kept on by default, however there are plans to make this a config-time
option.

   #ifndef CONFIG_PPC_ADV_DEBUG_REGS
  unsigned long dabr;
   #endif
   
  -   if (bp_info-version != 1)
  +   if ((bp_info-version != 1)  (bp_info-version != 2))
  return -ENOTSUPP;
   #ifdef CONFIG_PPC_ADV_DEBUG_REGS
  /*
  @@ -1382,13 +1388,9 @@ static long ppc_set_hwdebug(struct task_struct 
  *child,
   */
  if ((bp_info-trigger_type  PPC_BREAKPOINT_TRIGGER_RW) == 0 ||

Re: [PATCH 1/2] [hw-breakpoint] Use generic hw-breakpoint interfaces for new PPC ptrace flags

2011-08-23 Thread David Gibson 
On Tue, Aug 23, 2011 at 02:55:13PM +0530, K.Prasad wrote:
 On Tue, Aug 23, 2011 at 03:08:50PM +1000, David Gibson wrote:
  On Fri, Aug 19, 2011 at 01:21:36PM +0530, K.Prasad wrote:
   PPC_PTRACE_GETHWDBGINFO, PPC_PTRACE_SETHWDEBUG and PPC_PTRACE_DELHWDEBUG 
   are
   PowerPC specific ptrace flags that use the watchpoint register. While 
   they are
   targeted primarily towards BookE users, user-space applications such as 
   GDB
   have started using them for BookS too.
   
   This patch enables the use of generic hardware breakpoint interfaces for 
   these
   new flags. The version number of the associated data structures
   ppc_hw_breakpoint and ppc_debug_info is incremented to denote new 
   semantics.
  
  So, the structure itself doesn't seem to have been extended.  I don't
  understand what the semantic difference is - your patch comment needs
  to explain this clearly.
 
 
 We had a request to extend the structure but thought it was dangerous to
 do so. For instance if the user-space used version1 of the structure,
 while kernel did a copy_to_user() pertaining to version2, then we'd run
 into problems. Unfortunately the ptrace flags weren't designed to accept
 a version number as input from the user through the
 PPC_PTRACE_GETHWDBGINFO flag (which would have solved this issue).

I still don't follow you.

 I'll add a comment w.r.t change in semantics - such as the ability to
 accept 'range' breakpoints in BookS.
  
   Apart from the usual benefits of using generic hw-breakpoint interfaces, 
   these
   changes allow debuggers (such as GDB) to use a common set of ptrace flags 
   for
   their watchpoint needs and allow more precise breakpoint specification 
   (length
   of the variable can be specified).
  
  What is the mechanism for implementing the range breakpoint on book3s?
  
 
 The hw-breakpoint interface, accepts length as an argument in BookS (any
 value = 8 Bytes) and would filter out extraneous interrupts arising out
 of accesses outside the range comprising addr, addr + len inside
 hw_breakpoint_handler function.
 
 We put that ability to use here.

Ah, so in hardware the breakpoints are always 8 bytes long, but you
filter out false hits on a shorter range?  Of course, the utility of
range breakpoints is questionable when length =8, but the start must
be aligned on an 8-byte boundary.

[snip]
 if ((unsigned long)bp_info-addr = TASK_SIZE)
 return -EIO;

   @@ -1398,15 +1400,86 @@ static long ppc_set_hwdebug(struct task_struct 
   *child,
 dabr |= DABR_DATA_READ;
 if (bp_info-trigger_type  PPC_BREAKPOINT_TRIGGER_WRITE)
 dabr |= DABR_DATA_WRITE;
   +#ifdef CONFIG_HAVE_HW_BREAKPOINT
   + if (bp_info-version == 1)
   + goto version_one;
  
  There are several legitimate uses of goto in the kernel, but this is
  definitely not one of them.  You're essentially using it to put the
  old and new versions of the same function in one block.  Nasty.
  
 
 Maybe it's the label that's causing bother here. It might look elegant
 if it was called something like exit_* or error_* :-)
 
 The goto here helps reduce code, is similar to the error exits we use
 everywhere.

Rubbish, it is not an exception exit at all, it is two separate code
paths for the different versions which would be much clearer as two
different functions.

   + if (ptrace_get_breakpoints(child)  0)
   + return -ESRCH;

   - child-thread.dabr = dabr;
   + bp = thread-ptrace_bps[0];
   + if (!bp_info-addr) {
   + if (bp) {
   + unregister_hw_breakpoint(bp);
   + thread-ptrace_bps[0] = NULL;
   + }
   + ptrace_put_breakpoints(child);
   + return 0;
  
  Why are you making setting a 0 watchpoint remove the existing one (I
  think that's what this does).  I thought there was an explicit del
  breakpoint operation instead.
 
 We had to define the semantics for what writing a 0 to DABR could mean,
 and I think it is intuitive to consider it as deletion
 request...couldn't think of a case where DABR with addr=0 and RW=1 would
 be required.

When a user space program maps pages at virtual address 0, which it
can do.

   + }
   + /*
   +  * Check if the request is for 'range' breakpoints. We can
   +  * support it if range  8 bytes.
   +  */
   + if (bp_info-addr_mode == PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE)
   + len = bp_info-addr2 - bp_info-addr;
  
  So you compute the length here, but I don't see you ever test if it is
   8 and return an error.
  
 
 The hw-breakpoint interfaces would fail if the length was  8.

Ok.

   + else if (bp_info-addr_mode != PPC_BREAKPOINT_MODE_EXACT) {
   + ptrace_put_breakpoints(child);
   + return -EINVAL;
   + }
   + if (bp) {
   + attr = bp-attr;
   + attr.bp_addr = (unsigned long)bp_info-addr  
   ~HW_BREAKPOINT_ALIGN;
   + arch_bp_generic_fields(dabr 
   + (DABR_DATA_WRITE |

Re: [PATCH 1/2] [hw-breakpoint] Use generic hw-breakpoint interfaces for new PPC ptrace flags

2011-08-22 Thread David Gibson 
On Fri, Aug 19, 2011 at 01:21:36PM +0530, K.Prasad wrote:
 PPC_PTRACE_GETHWDBGINFO, PPC_PTRACE_SETHWDEBUG and PPC_PTRACE_DELHWDEBUG are
 PowerPC specific ptrace flags that use the watchpoint register. While they are
 targeted primarily towards BookE users, user-space applications such as GDB
 have started using them for BookS too.
 
 This patch enables the use of generic hardware breakpoint interfaces for these
 new flags. The version number of the associated data structures
 ppc_hw_breakpoint and ppc_debug_info is incremented to denote new 
 semantics.

So, the structure itself doesn't seem to have been extended.  I don't
understand what the semantic difference is - your patch comment needs
to explain this clearly.

 Apart from the usual benefits of using generic hw-breakpoint interfaces, these
 changes allow debuggers (such as GDB) to use a common set of ptrace flags for
 their watchpoint needs and allow more precise breakpoint specification (length
 of the variable can be specified).

What is the mechanism for implementing the range breakpoint on book3s?

 [Edjunior: Identified an issue in the patch with the sanity check for version
 numbers]
 
 Tested-by: Edjunior Barbosa Machado emach...@linux.vnet.ibm.com
 Signed-off-by: K.Prasad pra...@linux.vnet.ibm.com
 ---
  Documentation/powerpc/ptrace.txt |   16 ++
  arch/powerpc/kernel/ptrace.c |  104 
 +++---
  2 files changed, 112 insertions(+), 8 deletions(-)
 
 diff --git a/Documentation/powerpc/ptrace.txt 
 b/Documentation/powerpc/ptrace.txt
 index f4a5499..97301ae 100644
 --- a/Documentation/powerpc/ptrace.txt
 +++ b/Documentation/powerpc/ptrace.txt
 @@ -127,6 +127,22 @@ Some examples of using the structure to:
p.addr2   = (uint64_t) end_range;
p.condition_value = 0;
  
 +- set a watchpoint in server processors (BookS) using version 2
 +
 +  p.version = 2;
 +  p.trigger_type= PPC_BREAKPOINT_TRIGGER_RW;
 +  p.addr_mode   = PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE;
 +  or
 +  p.addr_mode   = PPC_BREAKPOINT_MODE_RANGE_EXACT;
 +
 +  p.condition_mode  = PPC_BREAKPOINT_CONDITION_NONE;
 +  p.addr= (uint64_t) begin_range;
 +  /* For PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE addr2 needs to be specified, 
 where
 +   * addr2 - addr = 8 Bytes.
 +   */
 +  p.addr2   = (uint64_t) end_range;
 +  p.condition_value = 0;
 +
  3. PTRACE_DELHWDEBUG
  
  Takes an integer which identifies an existing breakpoint or watchpoint
 diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
 index 05b7dd2..18d28b6 100644
 --- a/arch/powerpc/kernel/ptrace.c
 +++ b/arch/powerpc/kernel/ptrace.c
 @@ -1339,11 +1339,17 @@ static int set_dac_range(struct task_struct *child,
  static long ppc_set_hwdebug(struct task_struct *child,
struct ppc_hw_breakpoint *bp_info)
  {
 +#ifdef CONFIG_HAVE_HW_BREAKPOINT
 + int ret, len = 0;
 + struct thread_struct *thread = (child-thread);
 + struct perf_event *bp;
 + struct perf_event_attr attr;
 +#endif /* CONFIG_HAVE_HW_BREAKPOINT */

I'm confused.  This compiled before on book3s, and I don't see any
changes to Makefile or Kconfig in the patch that will result in this
code compiling  when it previously didn't   Why are these new guards
added?

  #ifndef CONFIG_PPC_ADV_DEBUG_REGS
   unsigned long dabr;
  #endif
  
 - if (bp_info-version != 1)
 + if ((bp_info-version != 1)  (bp_info-version != 2))
   return -ENOTSUPP;
  #ifdef CONFIG_PPC_ADV_DEBUG_REGS
   /*
 @@ -1382,13 +1388,9 @@ static long ppc_set_hwdebug(struct task_struct *child,
*/
   if ((bp_info-trigger_type  PPC_BREAKPOINT_TRIGGER_RW) == 0 ||
   (bp_info-trigger_type  ~PPC_BREAKPOINT_TRIGGER_RW) != 0 ||
 - bp_info-addr_mode != PPC_BREAKPOINT_MODE_EXACT ||
   bp_info-condition_mode != PPC_BREAKPOINT_CONDITION_NONE)
   return -EINVAL;
  
 - if (child-thread.dabr)
 - return -ENOSPC;
 -

You remove this test to see if the single watchpoint slot is already
in use, but I don't see another test replacing it.

   if ((unsigned long)bp_info-addr = TASK_SIZE)
   return -EIO;
  
 @@ -1398,15 +1400,86 @@ static long ppc_set_hwdebug(struct task_struct *child,
   dabr |= DABR_DATA_READ;
   if (bp_info-trigger_type  PPC_BREAKPOINT_TRIGGER_WRITE)
   dabr |= DABR_DATA_WRITE;
 +#ifdef CONFIG_HAVE_HW_BREAKPOINT
 + if (bp_info-version == 1)
 + goto version_one;

There are several legitimate uses of goto in the kernel, but this is
definitely not one of them.  You're essentially using it to put the
old and new versions of the same function in one block.  Nasty.

 + if (ptrace_get_breakpoints(child)  0)
 + return -ESRCH;
  
 - child-thread.dabr = dabr;
 + bp = thread-ptrace_bps[0];
 + if (!bp_info-addr) {
 + if (bp) {
 + unregister_hw_breakpoint(bp);
 +

[PATCH 1/2] [hw-breakpoint] Use generic hw-breakpoint interfaces for new PPC ptrace flags

2011-08-19 Thread K.Prasad 
PPC_PTRACE_GETHWDBGINFO, PPC_PTRACE_SETHWDEBUG and PPC_PTRACE_DELHWDEBUG are
PowerPC specific ptrace flags that use the watchpoint register. While they are
targeted primarily towards BookE users, user-space applications such as GDB
have started using them for BookS too.

This patch enables the use of generic hardware breakpoint interfaces for these
new flags. The version number of the associated data structures
ppc_hw_breakpoint and ppc_debug_info is incremented to denote new semantics.

Apart from the usual benefits of using generic hw-breakpoint interfaces, these
changes allow debuggers (such as GDB) to use a common set of ptrace flags for
their watchpoint needs and allow more precise breakpoint specification (length
of the variable can be specified).

[Edjunior: Identified an issue in the patch with the sanity check for version
numbers]

Tested-by: Edjunior Barbosa Machado emach...@linux.vnet.ibm.com
Signed-off-by: K.Prasad pra...@linux.vnet.ibm.com
---
 Documentation/powerpc/ptrace.txt |   16 ++
 arch/powerpc/kernel/ptrace.c |  104 +++---
 2 files changed, 112 insertions(+), 8 deletions(-)

diff --git a/Documentation/powerpc/ptrace.txt b/Documentation/powerpc/ptrace.txt
index f4a5499..97301ae 100644
--- a/Documentation/powerpc/ptrace.txt
+++ b/Documentation/powerpc/ptrace.txt
@@ -127,6 +127,22 @@ Some examples of using the structure to:
   p.addr2   = (uint64_t) end_range;
   p.condition_value = 0;
 
+- set a watchpoint in server processors (BookS) using version 2
+
+  p.version = 2;
+  p.trigger_type= PPC_BREAKPOINT_TRIGGER_RW;
+  p.addr_mode   = PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE;
+  or
+  p.addr_mode   = PPC_BREAKPOINT_MODE_RANGE_EXACT;
+
+  p.condition_mode  = PPC_BREAKPOINT_CONDITION_NONE;
+  p.addr= (uint64_t) begin_range;
+  /* For PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE addr2 needs to be specified, where
+   * addr2 - addr = 8 Bytes.
+   */
+  p.addr2   = (uint64_t) end_range;
+  p.condition_value = 0;
+
 3. PTRACE_DELHWDEBUG
 
 Takes an integer which identifies an existing breakpoint or watchpoint
diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index 05b7dd2..18d28b6 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -1339,11 +1339,17 @@ static int set_dac_range(struct task_struct *child,
 static long ppc_set_hwdebug(struct task_struct *child,
 struct ppc_hw_breakpoint *bp_info)
 {
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+   int ret, len = 0;
+   struct thread_struct *thread = (child-thread);
+   struct perf_event *bp;
+   struct perf_event_attr attr;
+#endif /* CONFIG_HAVE_HW_BREAKPOINT */
 #ifndef CONFIG_PPC_ADV_DEBUG_REGS
unsigned long dabr;
 #endif
 
-   if (bp_info-version != 1)
+   if ((bp_info-version != 1)  (bp_info-version != 2))
return -ENOTSUPP;
 #ifdef CONFIG_PPC_ADV_DEBUG_REGS
/*
@@ -1382,13 +1388,9 @@ static long ppc_set_hwdebug(struct task_struct *child,
 */
if ((bp_info-trigger_type  PPC_BREAKPOINT_TRIGGER_RW) == 0 ||
(bp_info-trigger_type  ~PPC_BREAKPOINT_TRIGGER_RW) != 0 ||
-   bp_info-addr_mode != PPC_BREAKPOINT_MODE_EXACT ||
bp_info-condition_mode != PPC_BREAKPOINT_CONDITION_NONE)
return -EINVAL;
 
-   if (child-thread.dabr)
-   return -ENOSPC;
-
if ((unsigned long)bp_info-addr = TASK_SIZE)
return -EIO;
 
@@ -1398,15 +1400,86 @@ static long ppc_set_hwdebug(struct task_struct *child,
dabr |= DABR_DATA_READ;
if (bp_info-trigger_type  PPC_BREAKPOINT_TRIGGER_WRITE)
dabr |= DABR_DATA_WRITE;
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+   if (bp_info-version == 1)
+   goto version_one;
+   if (ptrace_get_breakpoints(child)  0)
+   return -ESRCH;
 
-   child-thread.dabr = dabr;
+   bp = thread-ptrace_bps[0];
+   if (!bp_info-addr) {
+   if (bp) {
+   unregister_hw_breakpoint(bp);
+   thread-ptrace_bps[0] = NULL;
+   }
+   ptrace_put_breakpoints(child);
+   return 0;
+   }
+   /*
+* Check if the request is for 'range' breakpoints. We can
+* support it if range  8 bytes.
+*/
+   if (bp_info-addr_mode == PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE)
+   len = bp_info-addr2 - bp_info-addr;
+   else if (bp_info-addr_mode != PPC_BREAKPOINT_MODE_EXACT) {
+   ptrace_put_breakpoints(child);
+   return -EINVAL;
+   }
+   if (bp) {
+   attr = bp-attr;
+   attr.bp_addr = (unsigned long)bp_info-addr  
~HW_BREAKPOINT_ALIGN;
+   arch_bp_generic_fields(dabr 
+   (DABR_DATA_WRITE | DABR_DATA_READ),
+