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

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:

quoted

On Mon, Nov 28, 2011 at 02:11:11PM +1100, David Gibson wrote:

quoted

[snip]
On Wed, Oct 12, 2011 at 11:09:48PM +0530, K.Prasad wrote:

quoted

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.
 

quoted

quoted

quoted

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

quoted

quoted

quoted

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