ppc_set_hwdebug vs ptrace_set_debugreg

ppc_set_hwdebug vs ptrace_set_debugreg

[Andreas Schwab]

Why does ptrace_set_debugreg call register_user_hw_breakpoint, but
ppc_set_hwdebug doesn't?  Shouldn't ppc_set_hwdebug set the
DABR_DATA_(READ|WRITE|TRANSLATION) bits in the dabr?

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: ppc_set_hwdebug vs ptrace_set_debugreg

[K.Prasad]

On Sat, Nov 27, 2010 at 08:36:30PM +0100, Andreas Schwab wrote:
> Why does ptrace_set_debugreg call register_user_hw_breakpoint, but
> ppc_set_hwdebug doesn't?  Shouldn't ppc_set_hwdebug set the
> DABR_DATA_(READ|WRITE|TRANSLATION) bits in the dabr?
> 
> Andreas.

The hw-breakpoint interfaces were initially planned for the old ptrace
option PTRACE_SET_DEBUGREG,while, the newer ptrace options are mostly
to exploit the advanced debug features of Book3E processors.

Although ppc_set_hwdebug() can set DABR through set_dabr() in
arch/powerpc/kernel/process.c, it is good to have it converted to use
register_user_hw_breakpoint(). This was planned to be done alongside the
conversion of all ptrace options enabled by CONFIG_PPC_ADV_DEBUG_REGS,
which is yet to be done.

Are you looking to use debug registers through perf, or somesuch, that
you need register_user_hw_breakpoint() to be used for these new ptrace
flags?

Thanks,
K.Prasad

Re: ppc_set_hwdebug vs ptrace_set_debugreg

[Andreas Schwab]

"K.Prasad" <prasad@linux.vnet.ibm.com> writes:

> Although ppc_set_hwdebug() can set DABR through set_dabr() in
> arch/powerpc/kernel/process.c, it is good to have it converted to use
> register_user_hw_breakpoint().

What do you mean with "good to have"?  It doesn't work without it unless
I disable PERF_EVENTS (which is the only way to disable
HAVE_HW_BREAKPOINT).

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: ppc_set_hwdebug vs ptrace_set_debugreg

[K.Prasad]

On Mon, Nov 29, 2010 at 11:15:51AM +0100, Andreas Schwab wrote:
> "K.Prasad" <prasad@linux.vnet.ibm.com> writes:
> 
> > Although ppc_set_hwdebug() can set DABR through set_dabr() in
> > arch/powerpc/kernel/process.c, it is good to have it converted to use
> > register_user_hw_breakpoint().
> 
> What do you mean with "good to have"?  It doesn't work without it unless
> I disable PERF_EVENTS (which is the only way to disable
> HAVE_HW_BREAKPOINT).
> 
> Andreas.
>

Let me see if I can cook up a patch for this i.e. make set_dabr() invoke
register_user_hw_breakpoint() when CONFIG_PPC_BOOK3S is defined; before I
head out on my vacation (starting second week of this month).

Thanks,
K.Prasad
 

Re: ppc_set_hwdebug vs ptrace_set_debugreg

[K.Prasad]

On Wed, Dec 01, 2010 at 10:07:58AM +0530, K.Prasad wrote:
> On Mon, Nov 29, 2010 at 11:15:51AM +0100, Andreas Schwab wrote:
> > "K.Prasad" <prasad@linux.vnet.ibm.com> writes:
> > 
> > > Although ppc_set_hwdebug() can set DABR through set_dabr() in
> > > arch/powerpc/kernel/process.c, it is good to have it converted to use
> > > register_user_hw_breakpoint().
> > 
> > What do you mean with "good to have"?  It doesn't work without it unless
> > I disable PERF_EVENTS (which is the only way to disable
> > HAVE_HW_BREAKPOINT).
> > 
> > Andreas.
> >
> 
> Let me see if I can cook up a patch for this i.e. make set_dabr() invoke
> register_user_hw_breakpoint() when CONFIG_PPC_BOOK3S is defined; before I
> head out on my vacation (starting second week of this month).
> 
> Thanks,
> K.Prasad
>

Hi,
	Can you check if the following patch (compile tested only) works
for you?

Thanks,
K.Prasad
  
Signed-off-by: K.Prasad <prasad@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/ptrace.c |   34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 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,11 @@ 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 */
+
 	if (bp_info->version != 1)
 		return -ENOTSUPP;
 #ifdef CONFIG_PPC_ADV_DEBUG_REGS
@@ -1362,10 +1367,29 @@ 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;
 
+#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;
 
 	return 1;
@@ -1395,6 +1419,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;
 

Re: ppc_set_hwdebug vs ptrace_set_debugreg

[Andreas Schwab]

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

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: ppc_set_hwdebug vs ptrace_set_debugreg

[K.Prasad]

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;
 

Re: ppc_set_hwdebug vs ptrace_set_debugreg

[Andreas Schwab]

"K.Prasad" <prasad@linux.vnet.ibm.com> writes:

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

Try the watchpoint tests in gdb.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: ppc_set_hwdebug vs ptrace_set_debugreg

[Andreas Schwab]

"K.Prasad" <prasad@linux.vnet.ibm.com> writes:

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

It crashes the kernel when running the watch-vfork test.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: ppc_set_hwdebug vs ptrace_set_debugreg

[K.Prasad]

On Thu, Dec 16, 2010 at 06:07:47PM +0100, Andreas Schwab wrote:
> "K.Prasad" <prasad@linux.vnet.ibm.com> writes:
> 
> > How about the revised patch below? It is only compile-tested; have you
> > got a quick test case that I can run?
> 
> It crashes the kernel when running the watch-vfork test.
> 
> Andreas.
>

Hi Andreas,
	I tried running it multiple times but saw no crash (or error
messages in dmesg). Can you send me the crash logs? What's the behaviour
when the testcase is run on an unpatched kernel?

The watch-vfork test actually fails on my system (4 unexpected failures)
irrespective of the kernel containing the patch or not.

Thanks,
K.Prasad
P.S.: I'd been on vacation and couldn't look at this issue during then.

Re: ppc_set_hwdebug vs ptrace_set_debugreg

[Andreas Schwab]

"K.Prasad" <prasad@linux.vnet.ibm.com> writes:

> The watch-vfork test actually fails on my system (4 unexpected failures)

It should pass all four tests.  If gdb cannot even set a watchpoint it
cannot trigger the crash, of course.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."