[PATCH 0/2] Changes to PowerPC ptrace flags using watchpoints

[PATCH 0/2] Changes to PowerPC ptrace flags using watchpoints

[K.Prasad]

Hi All,
        Please find a set of two patches that introduce generic
hw-breakpoint interfaces for a couple of ptrace flags used by server-class
processors and another change to advertise a feature to user-space hitherto
available and used, but not claimed as supported.

GDB, which recently began using the
PPC_PTRACE_GETHWDBGINFO/PPC_PTRACE_SETHWDEBUG/PPC_PTRACE_DELHWDEBUG
flags on BookS processors is presently unable to set watchpoints. The
changes in Patch1, will fix that issue and help it use a common set of code
across BookE and BookS.

K.Prasad (2):
  [hw-breakpoint] Use generic hw-breakpoint interfaces for new PPC
    ptrace flags
  [PowerPC Book3E] Introduce new ptrace debug feature flag

 Documentation/powerpc/ptrace.txt  |   16 ++++++
 arch/powerpc/include/asm/ptrace.h |    1 +
 arch/powerpc/kernel/ptrace.c      |  105
++++++++++++++++++++++++++++++++++---
 3 files changed, 114 insertions(+), 8 deletions(-)

Thanks,
K.Prasad

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

[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 <emachado@linux.vnet.ibm.com>
Signed-off-by: K.Prasad <prasad@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),
+							&attr.bp_type);
+		attr.bp_len = len;
+		ret =  modify_user_hw_breakpoint(bp, &attr);
+		if (ret) {
+			ptrace_put_breakpoints(child);
+			return ret;
+		}
+		thread->ptrace_bps[0] = bp;
+		ptrace_put_breakpoints(child);
+		thread->dabr = dabr;
+		return 0;
+	}
 
+	/* 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 */
+
+version_one:
+	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 */
 }
 
 static long ppc_del_hwdebug(struct task_struct *child, long addr, long data)
 {
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+	struct thread_struct *thread = &(child->thread);
+	struct perf_event *bp;
+#endif /* CONFIG_HAVE_HW_BREAKPOINT */
 #ifdef CONFIG_PPC_ADV_DEBUG_REGS
 	int rc;
 
@@ -1426,10 +1499,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;
+#else /* CONFIG_HAVE_HW_BREAKPOINT */
 	if (child->thread.dabr == 0)
 		return -ENOENT;
 
 	child->thread.dabr = 0;
+#endif /* CONFIG_HAVE_HW_BREAKPOINT */
 
 	return 0;
 #endif
@@ -1536,7 +1623,8 @@ long arch_ptrace(struct task_struct *child, long request,
 	case PPC_PTRACE_GETHWDBGINFO: {
 		struct ppc_debug_info dbginfo;
 
-		dbginfo.version = 1;
+		/* We return the highest version number supported */
+		dbginfo.version = 2;
 #ifdef CONFIG_PPC_ADV_DEBUG_REGS
 		dbginfo.num_instruction_bps = CONFIG_PPC_ADV_DEBUG_IACS;
 		dbginfo.num_data_bps = CONFIG_PPC_ADV_DEBUG_DACS;
@@ -1560,7 +1648,7 @@ long arch_ptrace(struct task_struct *child, long request,
 		dbginfo.data_bp_alignment = 4;
 #endif
 		dbginfo.sizeof_condition = 0;
-		dbginfo.features = 0;
+		dbginfo.features = PPC_DEBUG_FEATURE_DATA_BP_RANGE;
 #endif /* CONFIG_PPC_ADV_DEBUG_REGS */
 
 		if (!access_ok(VERIFY_WRITE, datavp,
-- 
1.7.4.1

[PATCH 2/2] [PowerPC Book3E] Introduce new ptrace debug feature flag

[K.Prasad]

While PPC_PTRACE_SETHWDEBUG ptrace flag in PowerPC accepts
PPC_BREAKPOINT_MODE_EXACT mode of breakpoint, the same is not intimated to the
user-space debuggers (like GDB) who may want to use it. Hence we introduce a
new PPC_DEBUG_FEATURE_DATA_BP_EXACT flag which will be populated on the
"features" member of "struct ppc_debug_info" to advertise support for the
same on Book3E PowerPC processors.

Signed-off-by: K.Prasad <prasad@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/ptrace.h |    1 +
 arch/powerpc/kernel/ptrace.c      |    1 +
 2 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
index 48223f9..cf014f9 100644
--- a/arch/powerpc/include/asm/ptrace.h
+++ b/arch/powerpc/include/asm/ptrace.h
@@ -380,6 +380,7 @@ struct ppc_debug_info {
 #define PPC_DEBUG_FEATURE_INSN_BP_MASK		0x0000000000000002
 #define PPC_DEBUG_FEATURE_DATA_BP_RANGE		0x0000000000000004
 #define PPC_DEBUG_FEATURE_DATA_BP_MASK		0x0000000000000008
+#define PPC_DEBUG_FEATURE_DATA_BP_EXACT		0x0000000000000010
 
 #ifndef __ASSEMBLY__
 
diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index 18d28b6..71db5a6 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -1636,6 +1636,7 @@ long arch_ptrace(struct task_struct *child, long request,
 #ifdef CONFIG_PPC_ADV_DEBUG_DAC_RANGE
 		dbginfo.features |=
 				   PPC_DEBUG_FEATURE_DATA_BP_RANGE |
+				   PPC_DEBUG_FEATURE_DATA_BP_EXACT |
 				   PPC_DEBUG_FEATURE_DATA_BP_MASK;
 #endif
 #else /* !CONFIG_PPC_ADV_DEBUG_REGS */
-- 
1.7.4.1

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

[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 <emachado@linux.vnet.ibm.com>
> Signed-off-by: K.Prasad <prasad@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);
> +			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.

> +	}
> +	/*
> +	 * 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.

> +	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),
> +							&attr.bp_type);
> +		attr.bp_len = len;
> +		ret =  modify_user_hw_breakpoint(bp, &attr);
> +		if (ret) {
> +			ptrace_put_breakpoints(child);
> +			return ret;
> +		}
> +		thread->ptrace_bps[0] = bp;
> +		ptrace_put_breakpoints(child);
> +		thread->dabr = dabr;
> +		return 0;
> +	}
>  
> +	/* 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;

You seem to be silently masking the given address, which seems
completely wrong.

> +	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 */
> +
> +version_one:
> +	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 */
>  }
>  
>  static long ppc_del_hwdebug(struct task_struct *child, long addr, long data)
>  {
> +#ifdef CONFIG_HAVE_HW_BREAKPOINT
> +	struct thread_struct *thread = &(child->thread);
> +	struct perf_event *bp;
> +#endif /* CONFIG_HAVE_HW_BREAKPOINT */
>  #ifdef CONFIG_PPC_ADV_DEBUG_REGS
>  	int rc;
>  
> @@ -1426,10 +1499,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;
> +#else /* CONFIG_HAVE_HW_BREAKPOINT */
>  	if (child->thread.dabr == 0)
>  		return -ENOENT;
>  
>  	child->thread.dabr = 0;
> +#endif /* CONFIG_HAVE_HW_BREAKPOINT */
>  
>  	return 0;
>  #endif
> @@ -1536,7 +1623,8 @@ long arch_ptrace(struct task_struct *child, long request,
>  	case PPC_PTRACE_GETHWDBGINFO: {
>  		struct ppc_debug_info dbginfo;
>  
> -		dbginfo.version = 1;
> +		/* We return the highest version number supported */
> +		dbginfo.version = 2;
>  #ifdef CONFIG_PPC_ADV_DEBUG_REGS
>  		dbginfo.num_instruction_bps = CONFIG_PPC_ADV_DEBUG_IACS;
>  		dbginfo.num_data_bps = CONFIG_PPC_ADV_DEBUG_DACS;
> @@ -1560,7 +1648,7 @@ long arch_ptrace(struct task_struct *child, long request,
>  		dbginfo.data_bp_alignment = 4;
>  #endif
>  		dbginfo.sizeof_condition = 0;
> -		dbginfo.features = 0;
> +		dbginfo.features = PPC_DEBUG_FEATURE_DATA_BP_RANGE;
>  #endif /* CONFIG_PPC_ADV_DEBUG_REGS */
>  
>  		if (!access_ok(VERIFY_WRITE, datavp,

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

Re: [PATCH 2/2] [PowerPC Book3E] Introduce new ptrace debug feature flag

[David Gibson]

On Fri, Aug 19, 2011 at 01:23:38PM +0530, K.Prasad wrote:
> 
> While PPC_PTRACE_SETHWDEBUG ptrace flag in PowerPC accepts
> PPC_BREAKPOINT_MODE_EXACT mode of breakpoint, the same is not intimated to the
> user-space debuggers (like GDB) who may want to use it. Hence we introduce a
> new PPC_DEBUG_FEATURE_DATA_BP_EXACT flag which will be populated on the
> "features" member of "struct ppc_debug_info" to advertise support for the
> same on Book3E PowerPC processors.

I thought the idea was that the BP_EXACT mode was the default - if the
new interface was supported at all, then BP_EXACT was always
supported.  So, why do you need a new flag?

> 
> Signed-off-by: K.Prasad <prasad@linux.vnet.ibm.com>
> ---
>  arch/powerpc/include/asm/ptrace.h |    1 +
>  arch/powerpc/kernel/ptrace.c      |    1 +
>  2 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
> index 48223f9..cf014f9 100644
> --- a/arch/powerpc/include/asm/ptrace.h
> +++ b/arch/powerpc/include/asm/ptrace.h
> @@ -380,6 +380,7 @@ struct ppc_debug_info {
>  #define PPC_DEBUG_FEATURE_INSN_BP_MASK		0x0000000000000002
>  #define PPC_DEBUG_FEATURE_DATA_BP_RANGE		0x0000000000000004
>  #define PPC_DEBUG_FEATURE_DATA_BP_MASK		0x0000000000000008
> +#define PPC_DEBUG_FEATURE_DATA_BP_EXACT		0x0000000000000010
>  
>  #ifndef __ASSEMBLY__
>  
> diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
> index 18d28b6..71db5a6 100644
> --- a/arch/powerpc/kernel/ptrace.c
> +++ b/arch/powerpc/kernel/ptrace.c
> @@ -1636,6 +1636,7 @@ long arch_ptrace(struct task_struct *child, long request,
>  #ifdef CONFIG_PPC_ADV_DEBUG_DAC_RANGE
>  		dbginfo.features |=
>  				   PPC_DEBUG_FEATURE_DATA_BP_RANGE |
> +				   PPC_DEBUG_FEATURE_DATA_BP_EXACT |
>  				   PPC_DEBUG_FEATURE_DATA_BP_MASK;
>  #endif
>  #else /* !CONFIG_PPC_ADV_DEBUG_REGS */

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

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

[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 <emachado@linux.vnet.ibm.com>
> > Signed-off-by: K.Prasad <prasad@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 ||
> >  	    (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.
> 

This test is retained for !CONFIG_HAVE_HW_BREAKPOINT case. In case of
using hw-breakpoint interfaces, we have a double check through
thread->ptrace_bps[0] and using register_user_hw_breakpoint function
(which would error out if not enough free slots are available).

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

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

> > +	}
> > +	/*
> > +	 * 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.

> > +	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),
> > +							&attr.bp_type);
> > +		attr.bp_len = len;
> > +		ret =  modify_user_hw_breakpoint(bp, &attr);
> > +		if (ret) {
> > +			ptrace_put_breakpoints(child);
> > +			return ret;
> > +		}
> > +		thread->ptrace_bps[0] = bp;
> > +		ptrace_put_breakpoints(child);
> > +		thread->dabr = dabr;
> > +		return 0;
> > +	}
> >  
> > +	/* 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;
> 
> You seem to be silently masking the given address, which seems
> completely wrong.
> 

We have two ways of looking at the input address.
a) Assume that the input address is not multiplexed with the read/write
bits and return -EINVAL (for not confirming to the 8-byte alignment
requirement).
b) Consider the input address to be encoded with the read/write
watchpoint type request and align the address by default. This is how
the code behaves presently for the !CONFIG_HAVE_HW_BREAKPOINT case.

I chose to go with b) and discard the last 3-bits from the address.

Thanks for the detailed review. Looking forward for your comments.

Thanks,
K.Prasad

Re: [PATCH 2/2] [PowerPC Book3E] Introduce new ptrace debug feature flag

[K.Prasad]

On Tue, Aug 23, 2011 at 03:09:31PM +1000, David Gibson wrote:
> On Fri, Aug 19, 2011 at 01:23:38PM +0530, K.Prasad wrote:
> > 
> > While PPC_PTRACE_SETHWDEBUG ptrace flag in PowerPC accepts
> > PPC_BREAKPOINT_MODE_EXACT mode of breakpoint, the same is not intimated to the
> > user-space debuggers (like GDB) who may want to use it. Hence we introduce a
> > new PPC_DEBUG_FEATURE_DATA_BP_EXACT flag which will be populated on the
> > "features" member of "struct ppc_debug_info" to advertise support for the
> > same on Book3E PowerPC processors.
> 
> I thought the idea was that the BP_EXACT mode was the default - if the
> new interface was supported at all, then BP_EXACT was always
> supported.  So, why do you need a new flag?
> 

Yes, BP_EXACT was always supported but not advertised through
PPC_PTRACE_GETHWDBGINFO. We're now doing that.

Thanks,
K.Prasad

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

[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 | DABR_DATA_READ),
> > > +							&attr.bp_type);
> > > +		attr.bp_len = len;
> > > +		ret =  modify_user_hw_breakpoint(bp, &attr);
> > > +		if (ret) {
> > > +			ptrace_put_breakpoints(child);
> > > +			return ret;
> > > +		}
> > > +		thread->ptrace_bps[0] = bp;
> > > +		ptrace_put_breakpoints(child);
> > > +		thread->dabr = dabr;
> > > +		return 0;
> > > +	}
> > >  
> > > +	/* 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;
> > 
> > You seem to be silently masking the given address, which seems
> > completely wrong.
> > 
> 
> We have two ways of looking at the input address.
> a) Assume that the input address is not multiplexed with the read/write
> bits and return -EINVAL (for not confirming to the 8-byte alignment
> requirement).
> b) Consider the input address to be encoded with the read/write
> watchpoint type request and align the address by default. This is how
> the code behaves presently for the !CONFIG_HAVE_HW_BREAKPOINT case.

Hrm, ok, but this needs commenting.

> I chose to go with b) and discard the last 3-bits from the address.
> 
> Thanks for the detailed review. Looking forward for your comments.
> 
> Thanks,
> K.Prasad
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
> 

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

Re: [PATCH 2/2] [PowerPC Book3E] Introduce new ptrace debug feature flag

[David Gibson]

On Tue, Aug 23, 2011 at 02:57:56PM +0530, K.Prasad wrote:
> On Tue, Aug 23, 2011 at 03:09:31PM +1000, David Gibson wrote:
> > On Fri, Aug 19, 2011 at 01:23:38PM +0530, K.Prasad wrote:
> > > 
> > > While PPC_PTRACE_SETHWDEBUG ptrace flag in PowerPC accepts
> > > PPC_BREAKPOINT_MODE_EXACT mode of breakpoint, the same is not intimated to the
> > > user-space debuggers (like GDB) who may want to use it. Hence we introduce a
> > > new PPC_DEBUG_FEATURE_DATA_BP_EXACT flag which will be populated on the
> > > "features" member of "struct ppc_debug_info" to advertise support for the
> > > same on Book3E PowerPC processors.
> > 
> > I thought the idea was that the BP_EXACT mode was the default - if the
> > new interface was supported at all, then BP_EXACT was always
> > supported.  So, why do you need a new flag?
> > 
> 
> Yes, BP_EXACT was always supported but not advertised through
> PPC_PTRACE_GETHWDBGINFO. We're now doing that.

I can see that.  But you haven't answered why.

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

Re: [PATCH 2/2] [PowerPC Book3E] Introduce new ptrace debug feature flag

[Thiago Jung Bauermann]

On Wed, 2011-08-24 at 14:00 +1000, David Gibson wrote:
> On Tue, Aug 23, 2011 at 02:57:56PM +0530, K.Prasad wrote:
> > On Tue, Aug 23, 2011 at 03:09:31PM +1000, David Gibson wrote:
> > > On Fri, Aug 19, 2011 at 01:23:38PM +0530, K.Prasad wrote:
> > > > 
> > > > While PPC_PTRACE_SETHWDEBUG ptrace flag in PowerPC accepts
> > > > PPC_BREAKPOINT_MODE_EXACT mode of breakpoint, the same is not intimated to the
> > > > user-space debuggers (like GDB) who may want to use it. Hence we introduce a
> > > > new PPC_DEBUG_FEATURE_DATA_BP_EXACT flag which will be populated on the
> > > > "features" member of "struct ppc_debug_info" to advertise support for the
> > > > same on Book3E PowerPC processors.
> > > 
> > > I thought the idea was that the BP_EXACT mode was the default - if the
> > > new interface was supported at all, then BP_EXACT was always
> > > supported.  So, why do you need a new flag?
> > > 
> > 
> > Yes, BP_EXACT was always supported but not advertised through
> > PPC_PTRACE_GETHWDBGINFO. We're now doing that.
> 
> I can see that.  But you haven't answered why.

BookS doesn't support BP_EXACT, that's why I suggested this flag.

A BP_EXACT watchpoint triggers only when there's a memory access exactly
at the given address. It doesn't trigger when there's (for example) a
4-byte write at an address immediately before which also changes the
memory contents of the byte watched by the BP_EXACT watchpoint. a ranged
watchpoint would trigger, so the semantics are different.

As a general rule, GDB only sets ranged watchpoints and only uses
BP_EXACT ones when the user sets a flag. I want GDB to fail when the
user sets the flag on BookS since it can't provide the feature.
-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center

Re: [PATCH 2/2] [PowerPC Book3E] Introduce new ptrace debug feature flag

[David Gibson]

On Wed, Aug 24, 2011 at 09:41:43PM -0300, Thiago Jung Bauermann wrote:
> On Wed, 2011-08-24 at 14:00 +1000, David Gibson wrote:
> > On Tue, Aug 23, 2011 at 02:57:56PM +0530, K.Prasad wrote:
> > > On Tue, Aug 23, 2011 at 03:09:31PM +1000, David Gibson wrote:
> > > > On Fri, Aug 19, 2011 at 01:23:38PM +0530, K.Prasad wrote:
> > > > > 
> > > > > While PPC_PTRACE_SETHWDEBUG ptrace flag in PowerPC accepts
> > > > > PPC_BREAKPOINT_MODE_EXACT mode of breakpoint, the same is not intimated to the
> > > > > user-space debuggers (like GDB) who may want to use it. Hence we introduce a
> > > > > new PPC_DEBUG_FEATURE_DATA_BP_EXACT flag which will be populated on the
> > > > > "features" member of "struct ppc_debug_info" to advertise support for the
> > > > > same on Book3E PowerPC processors.
> > > > 
> > > > I thought the idea was that the BP_EXACT mode was the default - if the
> > > > new interface was supported at all, then BP_EXACT was always
> > > > supported.  So, why do you need a new flag?
> > > > 
> > > 
> > > Yes, BP_EXACT was always supported but not advertised through
> > > PPC_PTRACE_GETHWDBGINFO. We're now doing that.
> > 
> > I can see that.  But you haven't answered why.
> 
> BookS doesn't support BP_EXACT, that's why I suggested this flag.

Surely you can support it with exactly the same sort of filtering
you're using for the 8-byte ranges now?

> A BP_EXACT watchpoint triggers only when there's a memory access exactly
> at the given address. It doesn't trigger when there's (for example) a
> 4-byte write at an address immediately before which also changes the
> memory contents of the byte watched by the BP_EXACT watchpoint. a ranged
> watchpoint would trigger, so the semantics are different.
> 
> As a general rule, GDB only sets ranged watchpoints and only uses
> BP_EXACT ones when the user sets a flag. I want GDB to fail when the
> user sets the flag on BookS since it can't provide the feature.

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

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

[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) {
> > > > +			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.
> 

Agreed. I've removed the code under if (!bp_info->addr) branch.

> > > > +	}
> > > > +	/*
> > > > +	 * 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 | DABR_DATA_READ),
> > > > +							&attr.bp_type);
> > > > +		attr.bp_len = len;
> > > > +		ret =  modify_user_hw_breakpoint(bp, &attr);
> > > > +		if (ret) {
> > > > +			ptrace_put_breakpoints(child);
> > > > +			return ret;
> > > > +		}
> > > > +		thread->ptrace_bps[0] = bp;
> > > > +		ptrace_put_breakpoints(child);
> > > > +		thread->dabr = dabr;
> > > > +		return 0;
> > > > +	}
> > > >  
> > > > +	/* 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;
> > > 
> > > You seem to be silently masking the given address, which seems
> > > completely wrong.
> > > 
> > 
> > We have two ways of looking at the input address.
> > a) Assume that the input address is not multiplexed with the read/write
> > bits and return -EINVAL (for not confirming to the 8-byte alignment
> > requirement).
> > b) Consider the input address to be encoded with the read/write
> > watchpoint type request and align the address by default. This is how
> > the code behaves presently for the !CONFIG_HAVE_HW_BREAKPOINT case.
> 
> Hrm, ok, but this needs commenting.
> 

Added a comment to this effect.

I'm pasting the modified patch below. Kindly let me know your comments.

    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]

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..f9a4548 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;
 
@@ -1399,14 +1401,84 @@ static long ppc_set_hwdebug(struct task_struct *child,
 	if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_WRITE)
 		dabr |= DABR_DATA_WRITE;
 
-	child->thread.dabr = dabr;
+	if (bp_info->version == 1) {
+		if (bp_info->addr_mode != PPC_BREAKPOINT_MODE_EXACT)
+			return -EINVAL;
+		if (child->thread.dabr)
+			return -ENOSPC;
+		child->thread.dabr = dabr;
+		return 1;
+	}
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+	/*
+	 * We will use version = 2, to denote the use of
+	 * PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE mode of watchpoints.
+	 */
+	if (bp_info->version != 2)
+		return -EINVAL;
+	if (ptrace_get_breakpoints(child) < 0)
+		return -ESRCH;
 
+	bp = thread->ptrace_bps[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;
+		/*
+		 * Consider the input address to be encoded with the read/write
+		 * watchpoint type request and align the address by default.
+		 */
+		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;
+		ret =  modify_user_hw_breakpoint(bp, &attr);
+		if (ret) {
+			ptrace_put_breakpoints(child);
+			return ret;
+		}
+		thread->ptrace_bps[0] = bp;
+		ptrace_put_breakpoints(child);
+		thread->dabr = dabr;
+		return 0;
+	}
+
+	/* 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 */
 #endif /* !CONFIG_PPC_ADV_DEBUG_DVCS */
 }
 
 static long ppc_del_hwdebug(struct task_struct *child, long addr, long data)
 {
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+	struct thread_struct *thread = &(child->thread);
+	struct perf_event *bp;
+#endif /* CONFIG_HAVE_HW_BREAKPOINT */
 #ifdef CONFIG_PPC_ADV_DEBUG_REGS
 	int rc;
 
@@ -1426,10 +1498,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;
+#else /* CONFIG_HAVE_HW_BREAKPOINT */
 	if (child->thread.dabr == 0)
 		return -ENOENT;
 
 	child->thread.dabr = 0;
+#endif /* CONFIG_HAVE_HW_BREAKPOINT */
 
 	return 0;
 #endif
@@ -1536,7 +1622,8 @@ long arch_ptrace(struct task_struct *child, long request,
 	case PPC_PTRACE_GETHWDBGINFO: {
 		struct ppc_debug_info dbginfo;
 
-		dbginfo.version = 1;
+		/* We return the highest version number supported */
+		dbginfo.version = 2;
 #ifdef CONFIG_PPC_ADV_DEBUG_REGS
 		dbginfo.num_instruction_bps = CONFIG_PPC_ADV_DEBUG_IACS;
 		dbginfo.num_data_bps = CONFIG_PPC_ADV_DEBUG_DACS;
@@ -1560,7 +1647,7 @@ long arch_ptrace(struct task_struct *child, long request,
 		dbginfo.data_bp_alignment = 4;
 #endif
 		dbginfo.sizeof_condition = 0;
-		dbginfo.features = 0;
+		dbginfo.features = PPC_DEBUG_FEATURE_DATA_BP_RANGE;
 #endif /* CONFIG_PPC_ADV_DEBUG_REGS */
 
 		if (!access_ok(VERIFY_WRITE, datavp,

Re: [PATCH 2/2] [PowerPC Book3E] Introduce new ptrace debug feature flag

[Thiago Jung Bauermann]

On Fri, 2011-08-26 at 14:41 +1000, David Gibson wrote:
> On Wed, Aug 24, 2011 at 09:41:43PM -0300, Thiago Jung Bauermann wrote:
> > On Wed, 2011-08-24 at 14:00 +1000, David Gibson wrote:
> > > On Tue, Aug 23, 2011 at 02:57:56PM +0530, K.Prasad wrote:
> > > > On Tue, Aug 23, 2011 at 03:09:31PM +1000, David Gibson wrote:
> > > > > On Fri, Aug 19, 2011 at 01:23:38PM +0530, K.Prasad wrote:
> > > > > > 
> > > > > > While PPC_PTRACE_SETHWDEBUG ptrace flag in PowerPC accepts
> > > > > > PPC_BREAKPOINT_MODE_EXACT mode of breakpoint, the same is not intimated to the
> > > > > > user-space debuggers (like GDB) who may want to use it. Hence we introduce a
> > > > > > new PPC_DEBUG_FEATURE_DATA_BP_EXACT flag which will be populated on the
> > > > > > "features" member of "struct ppc_debug_info" to advertise support for the
> > > > > > same on Book3E PowerPC processors.
> > > > > 
> > > > > I thought the idea was that the BP_EXACT mode was the default - if the
> > > > > new interface was supported at all, then BP_EXACT was always
> > > > > supported.  So, why do you need a new flag?
> > > > > 
> > > > 
> > > > Yes, BP_EXACT was always supported but not advertised through
> > > > PPC_PTRACE_GETHWDBGINFO. We're now doing that.
> > > 
> > > I can see that.  But you haven't answered why.
> > 
> > BookS doesn't support BP_EXACT, that's why I suggested this flag.
> 
> Surely you can support it with exactly the same sort of filtering
> you're using for the 8-byte ranges now?

Yes, but to detect that the processor doesn't support BP_EXACT in
hardware I'd have to send a ptrace request, and have it rejected. Only
then I'd step back and simulate one with ranges. Considering that it's
easy and backwards compatible to add a new flag to signal that BP_EXACT
is not supported, I don't know why it would be better to go with the
more convoluted process.

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

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

[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
<dwg@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 <emachado@linux.vnet.ibm.com>
    Signed-off-by: K.Prasad <prasad@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;
+
 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) {
+		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),
+							&attr.bp_type);
+		attr.bp_len = len;
+		ret =  modify_user_hw_breakpoint(bp, &attr);
+		if (ret) {
+			ptrace_put_breakpoints(child);
+			return ret;
+		}
+		thread->ptrace_bps[0] = bp;
+		ptrace_put_breakpoints(child);
+		thread->dabr = dabr;
+		return 0;
+	}
 
+	/* 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 */
 }
 
 static long ppc_del_hwdebug(struct task_struct *child, long addr, long data)
 {
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+	struct thread_struct *thread = &(child->thread);
+	struct perf_event *bp;
+#endif /* CONFIG_HAVE_HW_BREAKPOINT */
 #ifdef CONFIG_PPC_ADV_DEBUG_REGS
 	int rc;
 
@@ -1426,10 +1496,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;
+#else /* CONFIG_HAVE_HW_BREAKPOINT */
 	if (child->thread.dabr == 0)
 		return -ENOENT;
 
 	child->thread.dabr = 0;
+#endif /* CONFIG_HAVE_HW_BREAKPOINT */
 
 	return 0;
 #endif
@@ -1560,7 +1644,7 @@ long arch_ptrace(struct task_struct *child, long request,
 		dbginfo.data_bp_alignment = 4;
 #endif
 		dbginfo.sizeof_condition = 0;
-		dbginfo.features = 0;
+		dbginfo.features = PPC_DEBUG_FEATURE_DATA_BP_RANGE;
 #endif /* CONFIG_PPC_ADV_DEBUG_REGS */
 
 		if (!access_ok(VERIFY_WRITE, datavp,

Re: [PATCH 2/2] [PowerPC Book3E] Introduce new ptrace debug feature flag

[David Gibson]

On Tue, Aug 30, 2011 at 09:27:41PM -0300, Thiago Jung Bauermann wrote:
> On Fri, 2011-08-26 at 14:41 +1000, David Gibson wrote:
> > On Wed, Aug 24, 2011 at 09:41:43PM -0300, Thiago Jung Bauermann wrote:
> > > On Wed, 2011-08-24 at 14:00 +1000, David Gibson wrote:
> > > > On Tue, Aug 23, 2011 at 02:57:56PM +0530, K.Prasad wrote:
> > > > > On Tue, Aug 23, 2011 at 03:09:31PM +1000, David Gibson wrote:
> > > > > > On Fri, Aug 19, 2011 at 01:23:38PM +0530, K.Prasad wrote:
> > > > > > > 
> > > > > > > While PPC_PTRACE_SETHWDEBUG ptrace flag in PowerPC accepts
> > > > > > > PPC_BREAKPOINT_MODE_EXACT mode of breakpoint, the same is not intimated to the
> > > > > > > user-space debuggers (like GDB) who may want to use it. Hence we introduce a
> > > > > > > new PPC_DEBUG_FEATURE_DATA_BP_EXACT flag which will be populated on the
> > > > > > > "features" member of "struct ppc_debug_info" to advertise support for the
> > > > > > > same on Book3E PowerPC processors.
> > > > > > 
> > > > > > I thought the idea was that the BP_EXACT mode was the default - if the
> > > > > > new interface was supported at all, then BP_EXACT was always
> > > > > > supported.  So, why do you need a new flag?
> > > > > > 
> > > > > 
> > > > > Yes, BP_EXACT was always supported but not advertised through
> > > > > PPC_PTRACE_GETHWDBGINFO. We're now doing that.
> > > > 
> > > > I can see that.  But you haven't answered why.
> > > 
> > > BookS doesn't support BP_EXACT, that's why I suggested this flag.
> > 
> > Surely you can support it with exactly the same sort of filtering
> > you're using for the 8-byte ranges now?
> 
> Yes, but to detect that the processor doesn't support BP_EXACT in
> hardware I'd have to send a ptrace request, and have it rejected. Only
> then I'd step back and simulate one with ranges. Considering that it's
> easy and backwards compatible to add a new flag to signal that BP_EXACT
> is not supported, I don't know why it would be better to go with the
> more convoluted process.

No, I'm saying why not implement BP_EXACT on server.

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

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

[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
> <dwg@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 <emachado@linux.vnet.ibm.com>
>     Signed-off-by: K.Prasad <prasad@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  = 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.

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

Misindent here.  This statement would be clearer if you had {} on all
of the if branches.

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

> +		ret =  modify_user_hw_breakpoint(bp, &attr);
> +		if (ret) {
> +			ptrace_put_breakpoints(child);
> +			return ret;
> +		}
> +		thread->ptrace_bps[0] = bp;
> +		ptrace_put_breakpoints(child);
> +		thread->dabr = dabr;
> +		return 0;
> +	}
>  
> +	/* 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 */
>  }
>  
>  static long ppc_del_hwdebug(struct task_struct *child, long addr, long data)
>  {
> +#ifdef CONFIG_HAVE_HW_BREAKPOINT
> +	struct thread_struct *thread = &(child->thread);
> +	struct perf_event *bp;
> +#endif /* CONFIG_HAVE_HW_BREAKPOINT */
>  #ifdef CONFIG_PPC_ADV_DEBUG_REGS
>  	int rc;
>  
> @@ -1426,10 +1496,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;
> +#else /* CONFIG_HAVE_HW_BREAKPOINT */
>  	if (child->thread.dabr == 0)
>  		return -ENOENT;
>  
>  	child->thread.dabr = 0;
> +#endif /* CONFIG_HAVE_HW_BREAKPOINT */
>  
>  	return 0;
>  #endif
> @@ -1560,7 +1644,7 @@ long arch_ptrace(struct task_struct *child, long request,
>  		dbginfo.data_bp_alignment = 4;
>  #endif
>  		dbginfo.sizeof_condition = 0;
> -		dbginfo.features = 0;
> +		dbginfo.features = PPC_DEBUG_FEATURE_DATA_BP_RANGE;
>  #endif /* CONFIG_PPC_ADV_DEBUG_REGS */
>  
>  		if (!access_ok(VERIFY_WRITE, datavp,
> 

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

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

[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 <emachado@linux.vnet.ibm.com>
> >     Signed-off-by: K.Prasad <prasad@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;
> > +		}
> > +		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;
> > +		}
> 
> Misindent here.  This statement would be clearer if you had {} on all
> of the if branches.
>

Okay, done.
 
> > +	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.

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 <emachado@linux.vnet.ibm.com>
Signed-off-by: K.Prasad <prasad@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;
+  /* 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 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) {
+		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;
+		ret =  modify_user_hw_breakpoint(bp, &attr);
+		if (ret) {
+			ptrace_put_breakpoints(child);
+			return ret;
+		}
+		thread->ptrace_bps[0] = bp;
+		ptrace_put_breakpoints(child);
+		thread->dabr = dabr;
+		return 0;
+	}
+
+	/* 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 */
 }
 
 static long ppc_del_hwdebug(struct task_struct *child, long addr, long data)
 {
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+	struct thread_struct *thread = &(child->thread);
+	struct perf_event *bp;
+#endif /* CONFIG_HAVE_HW_BREAKPOINT */
 #ifdef CONFIG_PPC_ADV_DEBUG_REGS
 	int rc;
 
@@ -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;
+#else /* CONFIG_HAVE_HW_BREAKPOINT */
 	if (child->thread.dabr == 0)
 		return -ENOENT;
 
 	child->thread.dabr = 0;
+#endif /* CONFIG_HAVE_HW_BREAKPOINT */
 
 	return 0;
 #endif
@@ -1560,7 +1636,7 @@ long arch_ptrace(struct task_struct *child, long request,
 		dbginfo.data_bp_alignment = 4;
 #endif
 		dbginfo.sizeof_condition = 0;
-		dbginfo.features = 0;
+		dbginfo.features = PPC_DEBUG_FEATURE_DATA_BP_RANGE;
 #endif /* CONFIG_PPC_ADV_DEBUG_REGS */
 
 		if (!access_ok(VERIFY_WRITE, datavp,
-- 
1.7.4.1

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

[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 <emachado@linux.vnet.ibm.com>
> Signed-off-by: K.Prasad <prasad@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 +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 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;

You are overindented here.

> +	}
> +	bp = thread->ptrace_bps[0];
> +	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);

You still have this code which has no business in the generic
interface path.

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


> +		thread->ptrace_bps[0] = bp;
> +		ptrace_put_breakpoints(child);
> +		thread->dabr = dabr;
> +		return 0;
> +	}
> +
> +	/* 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 */
>  }
>  
>  static long ppc_del_hwdebug(struct task_struct *child, long addr, long data)
>  {
> +#ifdef CONFIG_HAVE_HW_BREAKPOINT
> +	struct thread_struct *thread = &(child->thread);
> +	struct perf_event *bp;
> +#endif /* CONFIG_HAVE_HW_BREAKPOINT */
>  #ifdef CONFIG_PPC_ADV_DEBUG_REGS
>  	int rc;
>  
> @@ -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.

> +#else /* CONFIG_HAVE_HW_BREAKPOINT */
>  	if (child->thread.dabr == 0)
>  		return -ENOENT;
>  
>  	child->thread.dabr = 0;
> +#endif /* CONFIG_HAVE_HW_BREAKPOINT */
>  
>  	return 0;
>  #endif
> @@ -1560,7 +1636,7 @@ long arch_ptrace(struct task_struct *child, long request,
>  		dbginfo.data_bp_alignment = 4;
>  #endif
>  		dbginfo.sizeof_condition = 0;
> -		dbginfo.features = 0;
> +		dbginfo.features = PPC_DEBUG_FEATURE_DATA_BP_RANGE;
>  #endif /* CONFIG_PPC_ADV_DEBUG_REGS */
>  
>  		if (!access_ok(VERIFY_WRITE, datavp,

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

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

[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 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;
> 
> You are overindented here.

I must have been confused!...Even scripts/checkpath.pl didn't throw an error
at this line. Will correct it.

> > +	}
> > +	bp = thread->ptrace_bps[0];
> > +	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);
> 
> You still have this code which has no business in the generic
> interface path.


Same explanation as above. If I have to avoid the call to
arch_bp_generic_fields() then, it should be replaced with

switch(bp_info->trigger_type) {

case PPC_BREAKPOINT_TRIGGER_READ:
	attr.bp_type = HW_BREAKPOINT_R;

case PPC_BREAKPOINT_TRIGGER_WRITE:
	attr.bp_type = HW_BREAKPOINT_W;

case PPC_BREAKPOINT_TRIGGER_RW:
	attr.bp_type = (HW_BREAKPOINT_W | HW_BREAKPOINT_R);
}

All these additional lines for no extra benefit (or I haven't
still understood your comments fully).

> > +		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.
 
> > +		thread->ptrace_bps[0] = bp;
> > +		ptrace_put_breakpoints(child);
> > +		thread->dabr = dabr;
> > +		return 0;
> > +	}
> > +
> > +	/* 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 */
> >  }
> >  
> >  static long ppc_del_hwdebug(struct task_struct *child, long addr, long data)
> >  {
> > +#ifdef CONFIG_HAVE_HW_BREAKPOINT
> > +	struct thread_struct *thread = &(child->thread);
> > +	struct perf_event *bp;
> > +#endif /* CONFIG_HAVE_HW_BREAKPOINT */
> >  #ifdef CONFIG_PPC_ADV_DEBUG_REGS
> >  	int rc;
> >  
> > @@ -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.

Thanks again for your patient review. I will post the modified patch
after hearing comments from all.

Thanks.
K.Prasad

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

[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

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

[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
 

Re: [PATCH 2/2] [PowerPC Book3E] Introduce new ptrace debug feature flag

[Thiago Jung Bauermann]

On Mon, 2012-01-16 at 19:18 +1100, David Gibson wrote:
> Hrm.  An EXACT breakpoint is not exactly the same as a range
> breakpoint of length 1 (consider unaligned accesses).  But despite
> that, it should be possible to implement exact breakpoints on Book3S
> server hardware with some software filtering.
> 
> And since that leaves no hardware that *can't* implement exact
> breakpoints (directly or indirectly), I'm not yet convinced of the
> need for a flag bit.

I agree.

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

Re: [PATCH 2/2] [PowerPC Book3E] Introduce new ptrace debug feature flag

[David Gibson]

On Thu, Dec 22, 2011 at 03:04:47PM +0530, K.Prasad wrote:
> On Wed, Dec 21, 2011 at 11:55:02AM +1100, David Gibson wrote:
> > On Thu, Dec 08, 2011 at 04:53:30PM +0530, K.Prasad wrote:
> > > While PPC_PTRACE_SETHWDEBUG ptrace flag in PowerPC accepts
> > > PPC_BREAKPOINT_MODE_EXACT mode of breakpoint, the same is not intimated to the
> > > user-space debuggers (like GDB) who may want to use it. Hence we introduce a
> > > new PPC_DEBUG_FEATURE_DATA_BP_EXACT flag which will be populated on the
> > > "features" member of "struct ppc_debug_info" to advertise support for the
> > > same on Book3E PowerPC processors.
> > 
> > Hrm.  I had assumed the reason there wasn't a feature bit for EXACT
> > originally was that EXACT breakpoints were *always* supposed to be
> > supported by the new interface.
> >
> 
> Okay. Although BookS doesn't support EXACT breakpoints, it is possible
> (after the introduction of new hw-breakpoint interfaces) to request for
> a breakpoint of length 1 Byte.

Hrm.  An EXACT breakpoint is not exactly the same as a range
breakpoint of length 1 (consider unaligned accesses).  But despite
that, it should be possible to implement exact breakpoints on Book3S
server hardware with some software filtering.

And since that leaves no hardware that *can't* implement exact
breakpoints (directly or indirectly), I'm not yet convinced of the
need for a flag bit.

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

Re: [PATCH 2/2] [PowerPC Book3E] Introduce new ptrace debug feature flag

[K.Prasad]

On Wed, Dec 21, 2011 at 11:55:02AM +1100, David Gibson wrote:
> On Thu, Dec 08, 2011 at 04:53:30PM +0530, K.Prasad wrote:
> > While PPC_PTRACE_SETHWDEBUG ptrace flag in PowerPC accepts
> > PPC_BREAKPOINT_MODE_EXACT mode of breakpoint, the same is not intimated to the
> > user-space debuggers (like GDB) who may want to use it. Hence we introduce a
> > new PPC_DEBUG_FEATURE_DATA_BP_EXACT flag which will be populated on the
> > "features" member of "struct ppc_debug_info" to advertise support for the
> > same on Book3E PowerPC processors.
> 
> Hrm.  I had assumed the reason there wasn't a feature bit for EXACT
> originally was that EXACT breakpoints were *always* supposed to be
> supported by the new interface.
>

Okay. Although BookS doesn't support EXACT breakpoints, it is possible
(after the introduction of new hw-breakpoint interfaces) to request for
a breakpoint of length 1 Byte.

The hw-breakpoint infrastructure would take care of filtering the
extraneous interrupts arising out of accesses in the neighbourhood, in
such a case.

David,
	Can you Ack this patch too, if you find the changes acceptable?

Thanks,
K.Prasad
 

Re: [PATCH 2/2] [PowerPC Book3E] Introduce new ptrace debug feature flag

[David Gibson]

On Thu, Dec 08, 2011 at 04:53:30PM +0530, K.Prasad wrote:
> While PPC_PTRACE_SETHWDEBUG ptrace flag in PowerPC accepts
> PPC_BREAKPOINT_MODE_EXACT mode of breakpoint, the same is not intimated to the
> user-space debuggers (like GDB) who may want to use it. Hence we introduce a
> new PPC_DEBUG_FEATURE_DATA_BP_EXACT flag which will be populated on the
> "features" member of "struct ppc_debug_info" to advertise support for the
> same on Book3E PowerPC processors.

Hrm.  I had assumed the reason there wasn't a feature bit for EXACT
originally was that EXACT breakpoints were *always* supposed to be
supported by the new interface.

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

[PATCH 2/2] [PowerPC Book3E] Introduce new ptrace debug feature flag

[K.Prasad]

While PPC_PTRACE_SETHWDEBUG ptrace flag in PowerPC accepts
PPC_BREAKPOINT_MODE_EXACT mode of breakpoint, the same is not intimated to the
user-space debuggers (like GDB) who may want to use it. Hence we introduce a
new PPC_DEBUG_FEATURE_DATA_BP_EXACT flag which will be populated on the
"features" member of "struct ppc_debug_info" to advertise support for the
same on Book3E PowerPC processors.

Signed-off-by: K.Prasad <prasad@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/ptrace.h |    1 +
 arch/powerpc/kernel/ptrace.c      |    1 +
 2 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
index 48223f9..cf014f9 100644
--- a/arch/powerpc/include/asm/ptrace.h
+++ b/arch/powerpc/include/asm/ptrace.h
@@ -380,6 +380,7 @@ struct ppc_debug_info {
 #define PPC_DEBUG_FEATURE_INSN_BP_MASK		0x0000000000000002
 #define PPC_DEBUG_FEATURE_DATA_BP_RANGE		0x0000000000000004
 #define PPC_DEBUG_FEATURE_DATA_BP_MASK		0x0000000000000008
+#define PPC_DEBUG_FEATURE_DATA_BP_EXACT		0x0000000000000010
 
 #ifndef __ASSEMBLY__
 
diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index 18d28b6..71db5a6 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -1636,6 +1636,7 @@ long arch_ptrace(struct task_struct *child, long request,
 #ifdef CONFIG_PPC_ADV_DEBUG_DAC_RANGE
 		dbginfo.features |=
 				   PPC_DEBUG_FEATURE_DATA_BP_RANGE |
+				   PPC_DEBUG_FEATURE_DATA_BP_EXACT |
 				   PPC_DEBUG_FEATURE_DATA_BP_MASK;
 #endif
 #else /* !CONFIG_PPC_ADV_DEBUG_REGS */