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

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:

quoted

On Fri, Aug 19, 2011 at 01:21:36PM +0530, K.Prasad wrote:

quoted

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.
 

quoted

quoted

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]

quoted

quoted

 	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.

quoted

quoted

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

quoted

quoted

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

quoted

quoted

+	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