Re: [Patch 0/5] PPC64-HWBKPT: Hardware Breakpoint interfaces - ver XXII
On Tue, Jun 15, 2010 at 11:54:59AM +1000, Paul Mackerras wrote: > On Fri, Jun 04, 2010 at 12:21:45PM +0530, K.Prasad wrote: > > > Meanwhile I tested the per-cpu breakpoints with the new emulate_step > > patch (refer linuxppc-dev message-id: > > 20100602112903.gb30...@brick.ozlabs.ibm.com) and they continue to fail > > due to emulate_step() failure, in my case, on a "lwz r0,0(r28)" > > instruction. > > You need to pass the instruction word to emulate_step(), not the > instruction address. Also you need to have the full GPR set > available. The patch below fixes these problems. I'll fold these > changes into your patch 2/5. > > Paul. The breakpoints that used to fail before (due to emulate_step() failure) are now working fine upon applying this patch. Please include this patch along with my 2/5 as indicated by you. Thanks, K.Prasad ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [Patch 0/5] PPC64-HWBKPT: Hardware Breakpoint interfaces - ver XXII
On Fri, Jun 04, 2010 at 12:21:45PM +0530, K.Prasad wrote: > Meanwhile I tested the per-cpu breakpoints with the new emulate_step > patch (refer linuxppc-dev message-id: > 20100602112903.gb30...@brick.ozlabs.ibm.com) and they continue to fail > due to emulate_step() failure, in my case, on a "lwz r0,0(r28)" > instruction. You need to pass the instruction word to emulate_step(), not the instruction address. Also you need to have the full GPR set available. The patch below fixes these problems. I'll fold these changes into your patch 2/5. Paul. --- diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S index 3e423fb..f53029a 100644 --- a/arch/powerpc/kernel/exceptions-64s.S +++ b/arch/powerpc/kernel/exceptions-64s.S @@ -828,6 +828,7 @@ END_FW_FTR_SECTION_IFCLR(FW_FEATURE_ISERIES) /* We have a data breakpoint exception - handle it */ handle_dabr_fault: + bl .save_nvgprs ld r4,_DAR(r1) ld r5,_DSISR(r1) addir3,r1,STACK_FRAME_OVERHEAD diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c index ef70cf0..489049c 100644 --- a/arch/powerpc/kernel/hw_breakpoint.c +++ b/arch/powerpc/kernel/hw_breakpoint.c @@ -35,6 +35,7 @@ #include #include #include +#include /* * Stores the breakpoints currently in use on each breakpoint address @@ -203,6 +204,7 @@ int __kprobes hw_breakpoint_handler(struct die_args *args) int stepped = 1; struct arch_hw_breakpoint *info; unsigned long dar = regs->dar; + unsigned int instr; /* Disable breakpoints during exception handling */ set_dabr(0); @@ -255,7 +257,11 @@ int __kprobes hw_breakpoint_handler(struct die_args *args) goto out; } - stepped = emulate_step(regs, regs->nip); + stepped = 0; + instr = 0; + if (!__get_user_inatomic(instr, (unsigned int *) regs->nip)) + stepped = emulate_step(regs, instr); + /* * emulate_step() could not execute it. We've failed in reliably * handling the hw-breakpoint. Unregister it and throw a warning ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [Patch 0/5] PPC64-HWBKPT: Hardware Breakpoint interfaces - ver XXII
On Wed, Jun 09, 2010 at 04:02:23PM +0530, K.Prasad wrote: > (Given that it's your idea I've added your > signed-off too). Actually, you should never add someone else's signed-off-by unless they specifically ask you to. The signed-off-by lines are supposed to show the path that the patch took to get into the tree, so in general only the original author(s) and the maintainers who passed the patch along should add signed-off-by lines. It would only be a maintainer who would add someone else's signed-off-by, and then only if someone who needs to sign off on the patch had forgotten and then asked the maintainer to correct their mistake. In this case the appropriate way to give credit would be just a sentence in the patch description; no formal tag is needed. Paul. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [Patch 0/5] PPC64-HWBKPT: Hardware Breakpoint interfaces - ver XXII
On Mon, Jun 07, 2010 at 09:25:59PM +1000, Paul Mackerras wrote: > On Mon, Jun 07, 2010 at 12:33:51PM +0530, K.Prasad wrote: > > > Given that 'ptrace_bps' is used only for ptrace originated breakpoints > > and that we return early i.e. before detecting extraneous interrupts > > in hw_breakpoint_handler() (as shown above) they shouldn't overlap each > > other. The following comment in hw_breakpoint_handler() explains the > > same. > > /* > > * To prevent invocation of perf_event_bp(), we shall overload > > * thread.ptrace_bps[] pointer (unused for non-ptrace > > * exceptions) to flag an extraneous interrupt which must be > > * skipped. > > */ > > My point is that while we are using ptrace_bps[0] to mark a non-ptrace > breakpoint that we're single-stepping, some other process could be > ptracing this process and could get into ptrace_set_debugreg() and > would think that the process already has a ptrace breakpoint and call > modify_user_hw_breakpoint() when it should be calling > register_user_hw_breakpoint(). Or this process could die and so we > call flush_ptrace_hw_breakpoint() and it incorrectly thinks we have a > ptrace breakpoint. > > If there is a reason why we can be quite sure that while we are using > current->thread.ptrace_bps[0] in this way, ptrace_set_debugreg() can > never get called with this task as the ptracee, and nor can > flush_ptrace_hw_breakpoint() get called on this task, then maybe it's > safe. But it's not at all obviously safe. So I'd very much rather we > just use an extra flag somewhere, that isn't used elsewhere for > anything else, so we can convince ourselves that it is all correct > without having to look at lots of different pieces of code. > > There are 3 bytes of padding in struct arch_hw_breakpoint; couldn't we > use one of them as a "not really hit" flag? > > Paul. > ___ I get your reasoning now; ptrace_bps[] re-use will cause failures under these circumstances. I've sent a new version of the patchset which adds a new flag in 'struct arch_hw_breakpoint' (I was always thinking of 'struct thread_struct' before and was scared to introduce another new member in it, thereby leading me to incorrectly optimise using ptrace_bps) to flag extraneous_interrupt (Given that it's your idea I've added your signed-off too). Kindly let me know your comments, if any. Thanks, K.Prasad ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [Patch 0/5] PPC64-HWBKPT: Hardware Breakpoint interfaces - ver XXII
On Mon, Jun 07, 2010 at 12:33:51PM +0530, K.Prasad wrote: > Given that 'ptrace_bps' is used only for ptrace originated breakpoints > and that we return early i.e. before detecting extraneous interrupts > in hw_breakpoint_handler() (as shown above) they shouldn't overlap each > other. The following comment in hw_breakpoint_handler() explains the > same. > /* >* To prevent invocation of perf_event_bp(), we shall overload >* thread.ptrace_bps[] pointer (unused for non-ptrace >* exceptions) to flag an extraneous interrupt which must be >* skipped. >*/ My point is that while we are using ptrace_bps[0] to mark a non-ptrace breakpoint that we're single-stepping, some other process could be ptracing this process and could get into ptrace_set_debugreg() and would think that the process already has a ptrace breakpoint and call modify_user_hw_breakpoint() when it should be calling register_user_hw_breakpoint(). Or this process could die and so we call flush_ptrace_hw_breakpoint() and it incorrectly thinks we have a ptrace breakpoint. If there is a reason why we can be quite sure that while we are using current->thread.ptrace_bps[0] in this way, ptrace_set_debugreg() can never get called with this task as the ptracee, and nor can flush_ptrace_hw_breakpoint() get called on this task, then maybe it's safe. But it's not at all obviously safe. So I'd very much rather we just use an extra flag somewhere, that isn't used elsewhere for anything else, so we can convince ourselves that it is all correct without having to look at lots of different pieces of code. There are 3 bytes of padding in struct arch_hw_breakpoint; couldn't we use one of them as a "not really hit" flag? Paul. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [Patch 0/5] PPC64-HWBKPT: Hardware Breakpoint interfaces - ver XXII
On Fri, Jun 04, 2010 at 07:06:48PM +1000, Paul Mackerras wrote:
> On Fri, Jun 04, 2010 at 12:21:45PM +0530, K.Prasad wrote:
>
> > Meanwhile I tested the per-cpu breakpoints with the new emulate_step
> > patch (refer linuxppc-dev message-id:
> > 20100602112903.gb30...@brick.ozlabs.ibm.com) and they continue to fail
> > due to emulate_step() failure, in my case, on a "lwz r0,0(r28)"
> > instruction.
>
> Strange, what was in r28? The emulator should handle that instruction.
>
It must be containing one of the offsets of "struct tracer" which is a
parameter to the function trace_selftest_startup_ksym(). Basically this
function does a selftest over hw-breakpoints by placing read-write
breakpoint on a dummy global-variable and performs read-write access
thereupon. The lwz instruction which fails here corresponds to the
instruction that does read-write. A complete disassemble of the function
upto the failing instruction (at address) is pasted at the end of this
mail.
> > About the latest patchset, given that we chose to ignore extraneous
> > interrupts for non-ptrace breakpoints, I thought that re-using
> > current->thread.ptrace_bps as a flag would be efficient than
> > introducing
> > a new member in 'struct thread_struct' to do the same. I'm not sure
> > if
> > you foresee any issues with that.
>
> I just wonder what provides exclusion between its use as a flag and
> its use to hold a real ptrace breakpoint. As far as I can see nothing
> does. If there is something, it's off in some other source file,
> unless I'm missing something. And in that case there should be a bit
> fat comment explaining why it's safe.
>
The hw_breakpoint_handler() goes like this:
int __kprobes hw_breakpoint_handler(struct die_args *args)
{
...
/*
* Return early after invoking user-callback function without restoring
* DABR if the breakpoint is from ptrace which always operates in
* one-shot mode. The ptrace-ed process will receive the SIGTRAP signal
* generated in do_dabr().
*/
if (is_ptrace_bp) {
perf_bp_event(bp, regs);
rc = NOTIFY_DONE;
goto out;
}
/*
* Verify if dar lies within the address range occupied by the symbol
* being watched to filter extraneous exceptions.
*/
if (!((bp->attr.bp_addr <= dar) &&
(dar <= (bp->attr.bp_addr + bp->attr.bp_len {
/*
* This exception is triggered not because of a memory access
* on the monitored variable but in the double-word address
* range in which it is contained. We will consume this
* exception, considering it as 'noise'.
*/
is_extraneous_interrupt = true;
}
...
}
Given that 'ptrace_bps' is used only for ptrace originated breakpoints
and that we return early i.e. before detecting extraneous interrupts
in hw_breakpoint_handler() (as shown above) they shouldn't overlap each
other. The following comment in hw_breakpoint_handler() explains the
same.
/*
* To prevent invocation of perf_event_bp(), we shall overload
* thread.ptrace_bps[] pointer (unused for non-ptrace
* exceptions) to flag an extraneous interrupt which must be
* skipped.
*/
Thanks,
K.Prasad
(gdb) disass trace_selftest_startup_ksym
Dump of assembler code for function trace_selftest_startup_ksym:
0xc0125580 : mflrr0
0xc0125584 : std r26,-48(r1)
0xc0125588 : std r27,-40(r1)
0xc012558c :std r28,-32(r1)
0xc0125590 :std r29,-24(r1)
0xc0125594 :std r30,-16(r1)
0xc0125598 :std r31,-8(r1)
0xc012559c :std r0,16(r1)
0xc01255a0 :stdur1,-176(r1)
0xc01255a4 :mr r31,r1
0xc01255a8 :ld r30,-17784(r2)
0xc01255ac :mr r26,r4
0xc01255b0 :mr r27,r3
0xc01255b4 :bl
0xc0125510
0xc01255b8 :li r4,3
0xc01255bc :mr. r29,r3
0xc01255c0 :mr r5,r29
0xc01255c4 :beq
0xc01255e0
0xc01255c8 :ld r3,-31520(r30)
0xc01255cc :ld r4,0(r27)
0xc01255d0 :bl
0xc009c560
0xc01255d4 :nop
0xc01255d8 :b
0xc0125690
0xc01255dc :nop
0xc01255e0 :ld r28,-31512(r30)
0xc01255e4 : ld r3,-31504(r30)
0xc01255e8 : stw r29,0(r28)
0xc01255ec : mr r5,r28
0xc01255f0 : bl
0xc0140760
0xc01255f4 : nop
0xc01255f8 : cmpwi cr7,r3,0
0xc01255fc : mr r29,r3
0xc0125600 : blt
cr7,0xc012567c
0xc0125604 : lwz r0,
Re: [Patch 0/5] PPC64-HWBKPT: Hardware Breakpoint interfaces - ver XXII
On Fri, Jun 04, 2010 at 12:21:45PM +0530, K.Prasad wrote: > Meanwhile I tested the per-cpu breakpoints with the new emulate_step > patch (refer linuxppc-dev message-id: > 20100602112903.gb30...@brick.ozlabs.ibm.com) and they continue to fail > due to emulate_step() failure, in my case, on a "lwz r0,0(r28)" > instruction. Strange, what was in r28? The emulator should handle that instruction. > About the latest patchset, given that we chose to ignore extraneous > interrupts for non-ptrace breakpoints, I thought that re-using > current->thread.ptrace_bps as a flag would be efficient than introducing > a new member in 'struct thread_struct' to do the same. I'm not sure if > you foresee any issues with that. I just wonder what provides exclusion between its use as a flag and its use to hold a real ptrace breakpoint. As far as I can see nothing does. If there is something, it's off in some other source file, unless I'm missing something. And in that case there should be a bit fat comment explaining why it's safe. > If so, I'd like to send a new patch (rather than a new version of the > complete patchset) to fix it along with the dangling put_cpu() in > arch_unregister_hw_breakpoint() (I forgot to remove parts of the code > between versions XIX and XX). OK. Paul. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [Patch 0/5] PPC64-HWBKPT: Hardware Breakpoint interfaces - ver XXII
On Wed, Jun 02, 2010 at 09:33:16PM +1000, Paul Mackerras wrote: > On Fri, May 28, 2010 at 12:09:24PM +0530, K.Prasad wrote: > > > Please find a new set of patches that have the following changes. > > Thanks. There are a couple of minor things still remaining (dangling > put_cpu in arch_unregister_hw_breakpoint, plus I don't think reusing > current->thread.ptrace_bps the way you did in patch 5/5 is a good > idea), but I think at this stage I'll put them in a tree together > with my latest emulate_step version and then push them to Ben H and/or > Ingo Molnar once I've done some testing. > > Paul. Hi Paul, Thanks for agreeing to put the patchset into a tree and push it to the appropriate maintainers. Meanwhile I tested the per-cpu breakpoints with the new emulate_step patch (refer linuxppc-dev message-id: 20100602112903.gb30...@brick.ozlabs.ibm.com) and they continue to fail due to emulate_step() failure, in my case, on a "lwz r0,0(r28)" instruction. About the latest patchset, given that we chose to ignore extraneous interrupts for non-ptrace breakpoints, I thought that re-using current->thread.ptrace_bps as a flag would be efficient than introducing a new member in 'struct thread_struct' to do the same. I'm not sure if you foresee any issues with that. If so, I'd like to send a new patch (rather than a new version of the complete patchset) to fix it along with the dangling put_cpu() in arch_unregister_hw_breakpoint() (I forgot to remove parts of the code between versions XIX and XX). Thanks, K.Prasad ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [Patch 0/5] PPC64-HWBKPT: Hardware Breakpoint interfaces - ver XXII
On Fri, May 28, 2010 at 12:09:24PM +0530, K.Prasad wrote: > Please find a new set of patches that have the following changes. Thanks. There are a couple of minor things still remaining (dangling put_cpu in arch_unregister_hw_breakpoint, plus I don't think reusing current->thread.ptrace_bps the way you did in patch 5/5 is a good idea), but I think at this stage I'll put them in a tree together with my latest emulate_step version and then push them to Ben H and/or Ingo Molnar once I've done some testing. Paul. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[Patch 0/5] PPC64-HWBKPT: Hardware Breakpoint interfaces - ver XXII
Hi All, Please find a new set of patches that have the following changes. Changelog - ver XXII (Version XXI: linuxppc-dev ref:20100525091314.ga29...@in.ibm.com) - Extraneous breakpoint exceptions are now properly handled; causative instruction will be single-stepped and debug register values restored. - Restoration of breakpoints during signal handling through thread_change_pc() had defects which are now fixed. - Breakpoints are flushed through flush_ptrace_hw_breakpoint() call in both flush_thread() and prepare_to_copy() functions. 'ptrace_bps[]' and 'last_hit_ubp' members are now promptly cleaned-up. - Single-step exception is now conditionally emulated upon hitting alignment_exception. - Rebased to commit 31f46717997a83bdf6db0dd04810c0a329eb3148 of linux-2.6 tree. Kindly let me know your comments for the same. Thanks, K.Prasad Changelog - ver XXI (Version XX: linuxppc-dev ref:20100524103136.ga8...@in.ibm.com) - Decision to emulate an instruction is now based on whether the causative instruction is in user_mode() or not. - Breakpoints don't have to be cleared during sigreturn. A 'double-hit' on hw_breakpoint_handler() is harmless for non-ptrace instructions. - Minor changes to aid code brevity. Changelog - ver XX (Version XIX: linuxppc-dev ref: 20100524040137.ga20...@in.ibm.com) - Non task-bound breakpoints will only be emulated. Breakpoint will be unregistered with a warning if emulation fails. Changelog - ver XIX (Version XVIII: linuxppc-dev ref: 20100512033055.ga6...@in.ibm.com) - Increased coverage of breakpoints during concurrent alignment_exception and signal handling (which previously had 'blind-spots'). - Support for kernel-thread breakpoints and kernel-space breakpoints inside the context of a user-space process. - Patches re-based to commit f4b87dee923342505e1ddba8d34ce9de33e75050, thereby necessitating minor changes to arch_validate_hwbkpt_settings(). Changelog - ver XVIII (Version XVII: linuxppc-dev ref: 20100414034340.ga6...@in.ibm.com) - Slight code restructuring for brevity and coding-style corrections. - emulate_single_step() now notifies DIE_SSTEP to registered handlers; causes single_step_dabr_instruction() to be invoked after alignment_exception. - hw-breakpoint restoration variables are cleaned-up before unregistration through arch_unregister_hw_breakpoint(). - SIGTRAP is no longer generated for non-ptrace user-space breakpoints. Changelog - ver XVII (Version XVI: linuxppc-dev ref: 20100330095809.ga14...@in.ibm.com) - CONFIG_HAVE_HW_BREAKPOINT is now used to define the scope of the new code (in lieu of CONFIG_PPC_BOOK3S_64). - CONFIG_HAVE_HW_BREAKPOINT is now dependant upon CONFIG_PERF_EVENTS and CONFIG_PPC_BOOK3S_64 (to overcome build failures under certain configs). - Included a target in arch/powerpc/lib/Makefile to build sstep.o when HAVE_HW_BREAKPOINT. - Added a dummy definition for hw_breakpoint_disable() when !HAVE_HW_BREAKPOINT. - Tested builds under defconfigs for ppc64, cell and g5 (found no patch induced failures). Changelog - ver XVI (Version XV: linuxppc-dev ref: 20100323140639.ga21...@in.ibm.com) - Used a new config option CONFIG_PPC_BOOK3S_64 (in lieu of CONFIG_PPC64/CPU_FTR_HAS_DABR) to limit the scope of the new code. - Disabled breakpoints before kexec of the machine using hw_breakpoint_disable(). - Minor optimisation in exception-64s.S to check for data breakpoint exceptions in DSISR finally (after check for other causes) + changes in code comments and representation of DSISR_DABRMATCH constant. - Rebased to commit ae6be51ed01d6c4aaf249a207b4434bc7785853b of linux-2.6. Changelog - ver XV (Version XIV: linuxppc-dev ref: 20100308181232.ga3...@in.ibm.com) - Additional patch to disable interrupts during data breakpoint exception handling. - Moved HBP_NUM definition to cputable.h under a new CPU_FTR definition (CPU_FTR_HAS_DABR). - Filtering of extraneous exceptions (due to accesses outside symbol length) is by-passed for ptrace requests. - Removed flush_ptrace_hw_breakpoint() from __switch_to() due to incorrect coding placement. - Changes to code comments as per community reviews for previous version. - Minor coding-style changes in hw_breakpoint.c as per review comments. - Re-based to commit ae6be51ed01d6c4aaf249a207b4434bc7785853b of linux-2.6 Changelog - ver XIV (Version XIII: linuxppc-dev ref: 20100215055605.gb3...@in.ibm.com) - Removed the 'name' field from 'struct arch_hw_breakpoint'. - All callback invocations through bp->overflow_handler() are replaced with perf_bp_event(). - Removed the check for pre-existing single-stepping mode in hw_breakpoint_handler() as this check is unreliable while in kernel-space. Side effect of this change is the non-triggering of hw-breakpoints while single-stepping ker
