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
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
