| Message ID | 20110819075136.GB21817@in.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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. > 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? > [Edjunior: Identified an issue in the patch with the sanity check for version > numbers] > > Tested-by: Edjunior Barbosa Machado <emachado@linux.vnet.ibm.com> > Signed-off-by: K.Prasad <prasad@linux.vnet.ibm.com> > --- > Documentation/powerpc/ptrace.txt | 16 ++++++ > arch/powerpc/kernel/ptrace.c | 104 +++++++++++++++++++++++++++++++++++--- > 2 files changed, 112 insertions(+), 8 deletions(-) > > diff --git a/Documentation/powerpc/ptrace.txt b/Documentation/powerpc/ptrace.txt > index f4a5499..97301ae 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) using version 2 > + > + p.version = 2; > + p.trigger_type = PPC_BREAKPOINT_TRIGGER_RW; > + p.addr_mode = PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE; > + or > + p.addr_mode = PPC_BREAKPOINT_MODE_RANGE_EXACT; > + > + p.condition_mode = PPC_BREAKPOINT_CONDITION_NONE; > + p.addr = (uint64_t) begin_range; > + /* For PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE addr2 needs to be specified, where > + * addr2 - addr <= 8 Bytes. > + */ > + p.addr2 = (uint64_t) end_range; > + p.condition_value = 0; > + > 3. PTRACE_DELHWDEBUG > > Takes an integer which identifies an existing breakpoint or watchpoint > diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c > index 05b7dd2..18d28b6 100644 > --- a/arch/powerpc/kernel/ptrace.c > +++ b/arch/powerpc/kernel/ptrace.c > @@ -1339,11 +1339,17 @@ static int set_dac_range(struct task_struct *child, > static long ppc_set_hwdebug(struct task_struct *child, > struct ppc_hw_breakpoint *bp_info) > { > +#ifdef CONFIG_HAVE_HW_BREAKPOINT > + int ret, len = 0; > + struct thread_struct *thread = &(child->thread); > + struct perf_event *bp; > + struct perf_event_attr attr; > +#endif /* CONFIG_HAVE_HW_BREAKPOINT */ I'm confused. This compiled before on book3s, and I don't see any changes to Makefile or Kconfig in the patch that will result in this code compiling when it previously didn't Why are these new guards added? > #ifndef CONFIG_PPC_ADV_DEBUG_REGS > unsigned long dabr; > #endif > > - if (bp_info->version != 1) > + if ((bp_info->version != 1) && (bp_info->version != 2)) > return -ENOTSUPP; > #ifdef CONFIG_PPC_ADV_DEBUG_REGS > /* > @@ -1382,13 +1388,9 @@ static long ppc_set_hwdebug(struct task_struct *child, > */ > if ((bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_RW) == 0 || > (bp_info->trigger_type & ~PPC_BREAKPOINT_TRIGGER_RW) != 0 || > - bp_info->addr_mode != PPC_BREAKPOINT_MODE_EXACT || > bp_info->condition_mode != PPC_BREAKPOINT_CONDITION_NONE) > return -EINVAL; > > - if (child->thread.dabr) > - return -ENOSPC; > - You remove this test to see if the single watchpoint slot is already in use, but I don't see another test replacing it. > 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. > + 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. > + } > + /* > + * 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. > + 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. > + attr.bp_len = len; > + arch_bp_generic_fields(dabr & (DABR_DATA_WRITE | DABR_DATA_READ), > + &attr.bp_type); > + > + thread->ptrace_bps[0] = bp = register_user_hw_breakpoint(&attr, > + ptrace_triggered, NULL, child); > + if (IS_ERR(bp)) { > + thread->ptrace_bps[0] = NULL; > + ptrace_put_breakpoints(child); > + return PTR_ERR(bp); > + } > + > + ptrace_put_breakpoints(child); > + return 1; > +#endif /* CONFIG_HAVE_HW_BREAKPOINT */ > + > +version_one: > + if (bp_info->addr_mode != PPC_BREAKPOINT_MODE_EXACT) > + return -EINVAL; > + > + if (child->thread.dabr) > + return -ENOSPC; > + > + child->thread.dabr = dabr; > return 1; > #endif /* !CONFIG_PPC_ADV_DEBUG_DVCS */ > } > > static long ppc_del_hwdebug(struct task_struct *child, long addr, long data) > { > +#ifdef CONFIG_HAVE_HW_BREAKPOINT > + struct thread_struct *thread = &(child->thread); > + struct perf_event *bp; > +#endif /* CONFIG_HAVE_HW_BREAKPOINT */ > #ifdef CONFIG_PPC_ADV_DEBUG_REGS > int rc; > > @@ -1426,10 +1499,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; > +#else /* CONFIG_HAVE_HW_BREAKPOINT */ > if (child->thread.dabr == 0) > return -ENOENT; > > child->thread.dabr = 0; > +#endif /* CONFIG_HAVE_HW_BREAKPOINT */ > > return 0; > #endif > @@ -1536,7 +1623,8 @@ long arch_ptrace(struct task_struct *child, long request, > case PPC_PTRACE_GETHWDBGINFO: { > struct ppc_debug_info dbginfo; > > - dbginfo.version = 1; > + /* We return the highest version number supported */ > + dbginfo.version = 2; > #ifdef CONFIG_PPC_ADV_DEBUG_REGS > dbginfo.num_instruction_bps = CONFIG_PPC_ADV_DEBUG_IACS; > dbginfo.num_data_bps = CONFIG_PPC_ADV_DEBUG_DACS; > @@ -1560,7 +1648,7 @@ long arch_ptrace(struct task_struct *child, long request, > dbginfo.data_bp_alignment = 4; > #endif > dbginfo.sizeof_condition = 0; > - dbginfo.features = 0; > + dbginfo.features = PPC_DEBUG_FEATURE_DATA_BP_RANGE; > #endif /* CONFIG_PPC_ADV_DEBUG_REGS */ > > if (!access_ok(VERIFY_WRITE, datavp,
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'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. > > [Edjunior: Identified an issue in the patch with the sanity check for version > > numbers] > > > > Tested-by: Edjunior Barbosa Machado <emachado@linux.vnet.ibm.com> > > Signed-off-by: K.Prasad <prasad@linux.vnet.ibm.com> > > --- > > Documentation/powerpc/ptrace.txt | 16 ++++++ > > arch/powerpc/kernel/ptrace.c | 104 +++++++++++++++++++++++++++++++++++--- > > 2 files changed, 112 insertions(+), 8 deletions(-) > > > > diff --git a/Documentation/powerpc/ptrace.txt b/Documentation/powerpc/ptrace.txt > > index f4a5499..97301ae 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) using version 2 > > + > > + p.version = 2; > > + p.trigger_type = PPC_BREAKPOINT_TRIGGER_RW; > > + p.addr_mode = PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE; > > + or > > + p.addr_mode = PPC_BREAKPOINT_MODE_RANGE_EXACT; > > + > > + p.condition_mode = PPC_BREAKPOINT_CONDITION_NONE; > > + p.addr = (uint64_t) begin_range; > > + /* For PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE addr2 needs to be specified, where > > + * addr2 - addr <= 8 Bytes. > > + */ > > + p.addr2 = (uint64_t) end_range; > > + p.condition_value = 0; > > + > > 3. PTRACE_DELHWDEBUG > > > > Takes an integer which identifies an existing breakpoint or watchpoint > > diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c > > index 05b7dd2..18d28b6 100644 > > --- a/arch/powerpc/kernel/ptrace.c > > +++ b/arch/powerpc/kernel/ptrace.c > > @@ -1339,11 +1339,17 @@ static int set_dac_range(struct task_struct *child, > > static long ppc_set_hwdebug(struct task_struct *child, > > struct ppc_hw_breakpoint *bp_info) > > { > > +#ifdef CONFIG_HAVE_HW_BREAKPOINT > > + int ret, len = 0; > > + struct thread_struct *thread = &(child->thread); > > + struct perf_event *bp; > > + struct perf_event_attr attr; > > +#endif /* CONFIG_HAVE_HW_BREAKPOINT */ > > I'm confused. This compiled before on book3s, and I don't see any > changes to Makefile or Kconfig in the patch that will result in this > code compiling when it previously didn't Why are these new guards > added? > The code is guarded using the CONFIG_ flags for two reasons. a) We don't want the code to be included for BookE and other architectures. b) In BookS, we're now adding a new ability based on whether CONFIG_HAVE_HW_BREAKPOINT is defined. Presently this config option is kept on by default, however there are plans to make this a config-time option. > > #ifndef CONFIG_PPC_ADV_DEBUG_REGS > > unsigned long dabr; > > #endif > > > > - if (bp_info->version != 1) > > + if ((bp_info->version != 1) && (bp_info->version != 2)) > > return -ENOTSUPP; > > #ifdef CONFIG_PPC_ADV_DEBUG_REGS > > /* > > @@ -1382,13 +1388,9 @@ static long ppc_set_hwdebug(struct task_struct *child, > > */ > > if ((bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_RW) == 0 || > > (bp_info->trigger_type & ~PPC_BREAKPOINT_TRIGGER_RW) != 0 || > > - bp_info->addr_mode != PPC_BREAKPOINT_MODE_EXACT || > > bp_info->condition_mode != PPC_BREAKPOINT_CONDITION_NONE) > > return -EINVAL; > > > > - if (child->thread.dabr) > > - return -ENOSPC; > > - > > You remove this test to see if the single watchpoint slot is already > in use, but I don't see another test replacing it. > This test is retained for !CONFIG_HAVE_HW_BREAKPOINT case. In case of using hw-breakpoint interfaces, we have a double check through thread->ptrace_bps[0] and using register_user_hw_breakpoint function (which would error out if not enough free slots are available). > > 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. > > + 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. > > + } > > + /* > > + * 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. > > + 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. 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
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 >
diff --git a/Documentation/powerpc/ptrace.txt b/Documentation/powerpc/ptrace.txt index f4a5499..97301ae 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) using version 2 + + p.version = 2; + p.trigger_type = PPC_BREAKPOINT_TRIGGER_RW; + p.addr_mode = PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE; + or + p.addr_mode = PPC_BREAKPOINT_MODE_RANGE_EXACT; + + p.condition_mode = PPC_BREAKPOINT_CONDITION_NONE; + p.addr = (uint64_t) begin_range; + /* For PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE addr2 needs to be specified, where + * addr2 - addr <= 8 Bytes. + */ + p.addr2 = (uint64_t) end_range; + p.condition_value = 0; + 3. PTRACE_DELHWDEBUG Takes an integer which identifies an existing breakpoint or watchpoint diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c index 05b7dd2..18d28b6 100644 --- a/arch/powerpc/kernel/ptrace.c +++ b/arch/powerpc/kernel/ptrace.c @@ -1339,11 +1339,17 @@ static int set_dac_range(struct task_struct *child, static long ppc_set_hwdebug(struct task_struct *child, struct ppc_hw_breakpoint *bp_info) { +#ifdef CONFIG_HAVE_HW_BREAKPOINT + int ret, len = 0; + struct thread_struct *thread = &(child->thread); + struct perf_event *bp; + struct perf_event_attr attr; +#endif /* CONFIG_HAVE_HW_BREAKPOINT */ #ifndef CONFIG_PPC_ADV_DEBUG_REGS unsigned long dabr; #endif - if (bp_info->version != 1) + if ((bp_info->version != 1) && (bp_info->version != 2)) return -ENOTSUPP; #ifdef CONFIG_PPC_ADV_DEBUG_REGS /* @@ -1382,13 +1388,9 @@ static long ppc_set_hwdebug(struct task_struct *child, */ if ((bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_RW) == 0 || (bp_info->trigger_type & ~PPC_BREAKPOINT_TRIGGER_RW) != 0 || - bp_info->addr_mode != PPC_BREAKPOINT_MODE_EXACT || bp_info->condition_mode != PPC_BREAKPOINT_CONDITION_NONE) return -EINVAL; - if (child->thread.dabr) - return -ENOSPC; - 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; + 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; + } + /* + * 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; + 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; + attr.bp_len = len; + arch_bp_generic_fields(dabr & (DABR_DATA_WRITE | DABR_DATA_READ), + &attr.bp_type); + + thread->ptrace_bps[0] = bp = register_user_hw_breakpoint(&attr, + ptrace_triggered, NULL, child); + if (IS_ERR(bp)) { + thread->ptrace_bps[0] = NULL; + ptrace_put_breakpoints(child); + return PTR_ERR(bp); + } + + ptrace_put_breakpoints(child); + return 1; +#endif /* CONFIG_HAVE_HW_BREAKPOINT */ + +version_one: + if (bp_info->addr_mode != PPC_BREAKPOINT_MODE_EXACT) + return -EINVAL; + + if (child->thread.dabr) + return -ENOSPC; + + child->thread.dabr = dabr; return 1; #endif /* !CONFIG_PPC_ADV_DEBUG_DVCS */ } static long ppc_del_hwdebug(struct task_struct *child, long addr, long data) { +#ifdef CONFIG_HAVE_HW_BREAKPOINT + struct thread_struct *thread = &(child->thread); + struct perf_event *bp; +#endif /* CONFIG_HAVE_HW_BREAKPOINT */ #ifdef CONFIG_PPC_ADV_DEBUG_REGS int rc; @@ -1426,10 +1499,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; +#else /* CONFIG_HAVE_HW_BREAKPOINT */ if (child->thread.dabr == 0) return -ENOENT; child->thread.dabr = 0; +#endif /* CONFIG_HAVE_HW_BREAKPOINT */ return 0; #endif @@ -1536,7 +1623,8 @@ long arch_ptrace(struct task_struct *child, long request, case PPC_PTRACE_GETHWDBGINFO: { struct ppc_debug_info dbginfo; - dbginfo.version = 1; + /* We return the highest version number supported */ + dbginfo.version = 2; #ifdef CONFIG_PPC_ADV_DEBUG_REGS dbginfo.num_instruction_bps = CONFIG_PPC_ADV_DEBUG_IACS; dbginfo.num_data_bps = CONFIG_PPC_ADV_DEBUG_DACS; @@ -1560,7 +1648,7 @@ long arch_ptrace(struct task_struct *child, long request, dbginfo.data_bp_alignment = 4; #endif dbginfo.sizeof_condition = 0; - dbginfo.features = 0; + dbginfo.features = PPC_DEBUG_FEATURE_DATA_BP_RANGE; #endif /* CONFIG_PPC_ADV_DEBUG_REGS */ if (!access_ok(VERIFY_WRITE, datavp,