Re: ppc_set_hwdebug vs ptrace_set_debugreg

["K.Prasad" <prasad@linux.vnet.ibm.com>]

On Mon, Dec 13, 2010 at 08:05:36PM +0100, Andreas Schwab wrote:
> "K.Prasad" <prasad@linux.vnet.ibm.com> writes:
> 
> > +#ifdef CONFIG_HAVE_HW_BREAKPOINT
> > +	/* Create a new breakpoint request if one doesn't exist already */
> > +	hw_breakpoint_init(&attr);
> > +	attr.bp_addr = bp_info->addr & ~HW_BREAKPOINT_ALIGN;
> > +	arch_bp_generic_fields(bp_info->addr &
> > +				(DABR_DATA_WRITE | DABR_DATA_READ),
> > +							&attr.bp_type);
> > +
> > +	bp = register_user_hw_breakpoint(&attr, ptrace_triggered, task);
> > +	if (IS_ERR(bp))
> > +		return PTR_ERR(bp);
> > +
> > +	child->thread.ptrace_bps[0] = bp;
> > +#endif /* CONFIG_HAVE_HW_BREAKPOINT */
> > +
> >  	child->thread.dabr = (unsigned long)bp_info->addr;
> 
> That cannot work, see
> <http://permalink.gmane.org/gmane.linux.ports.ppc64.devel/71418>.
>

Ok. The above patch makes it a bit easy.

How about the revised patch below? It is only compile-tested; have you
got a quick test case that I can run?

Enable PPC_PTRACE_SETHWDEBUG and PPC_PTRACE_DELHWDEBUG to use the generic
hardware breakpoint interfaces. This helps prevent conflict for the use of
DABR register in the absence of CONFIG_PPC_ADV_DEBUG_REGS and when
PTRACE_SET_DEBUGREG/PTRACE_GET_DEBUGREG flags are used by ptrace.

Signed-off-by: K.Prasad <prasad@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/ptrace.c |   32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

Index: linux-2.6.set_hwdebug/arch/powerpc/kernel/ptrace.c
===================================================================
--- linux-2.6.set_hwdebug.orig/arch/powerpc/kernel/ptrace.c
+++ linux-2.6.set_hwdebug/arch/powerpc/kernel/ptrace.c
@@ -1316,6 +1316,10 @@ static int set_dac_range(struct task_str
 static long ppc_set_hwdebug(struct task_struct *child,
 		     struct ppc_hw_breakpoint *bp_info)
 {
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+	struct perf_event *bp;
+	struct perf_event_attr attr;
+#endif /* CONFIG_HAVE_HW_BREAKPOINT */
 #ifndef CONFIG_PPC_ADV_DEBUG_REGS
 	unsigned long dabr;
 #endif
@@ -1365,6 +1369,10 @@ static long ppc_set_hwdebug(struct task_
 
 	if (child->thread.dabr)
 		return -ENOSPC;
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+	if (child->thread.ptrace_bps[0])
+		return -ENOSPC;
+#endif /* CONFIG_HAVE_HW_BREAKPOINT */
 
 	if ((unsigned long)bp_info->addr >= TASK_SIZE)
 		return -EIO;
@@ -1376,6 +1384,20 @@ static long ppc_set_hwdebug(struct task_
 	if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_WRITE)
 		dabr |= DABR_DATA_WRITE;
 
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+	/* Create a new breakpoint request if one doesn't exist already */
+	hw_breakpoint_init(&attr);
+	attr.bp_addr = dabr & ~HW_BREAKPOINT_ALIGN;
+	arch_bp_generic_fields(dabr & (DABR_DATA_WRITE | DABR_DATA_READ),
+							&attr.bp_type);
+
+	bp = register_user_hw_breakpoint(&attr, ptrace_triggered, child);
+	if (IS_ERR(bp))
+		return PTR_ERR(bp);
+
+	child->thread.ptrace_bps[0] = bp;
+#endif /* CONFIG_HAVE_HW_BREAKPOINT */
+
 	child->thread.dabr = dabr;
 
 	return 1;
@@ -1405,6 +1427,16 @@ static long ppc_del_hwdebug(struct task_
 		return -EINVAL;
 	if (child->thread.dabr == 0)
 		return -ENOENT;
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+	/*
+	 * There is no way by which address in ptrace_bps[0] and thread.dabr
+	 * can be different. So we don't explicitly check if they're the same
+	 */
+	if (child->thread.ptrace_bps[0]) {
+		unregister_hw_breakpoint(child->thread.ptrace_bps[0]);
+		child->thread.ptrace_bps[0] = NULL;
+	}
+#endif /* CONFIG_HAVE_HW_BREAKPOINT */
 
 	child->thread.dabr = 0;