| Message ID | 20090603163524.GD5197@in.ibm.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Gibson |
| Headers | show |
On Wed, Jun 03, 2009 at 10:05:24PM +0530, K.Prasad wrote: > Modify the ptrace code to use the hardware breakpoint interfaces for user-space. > > Signed-off-by: K.Prasad <prasad@linux.vnet.ibm.com> > --- > arch/powerpc/kernel/ptrace.c | 47 +++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 47 insertions(+) > > Index: linux-2.6-tip.hbkpt/arch/powerpc/kernel/ptrace.c > =================================================================== > --- linux-2.6-tip.hbkpt.orig/arch/powerpc/kernel/ptrace.c > +++ linux-2.6-tip.hbkpt/arch/powerpc/kernel/ptrace.c > @@ -37,6 +37,7 @@ > #include <asm/page.h> > #include <asm/pgtable.h> > #include <asm/system.h> > +#include <asm/hw_breakpoint.h> > > /* > * does not yet catch signals sent when the child dies. > @@ -735,9 +736,26 @@ void user_disable_single_step(struct tas > clear_tsk_thread_flag(task, TIF_SINGLESTEP); > } > > +void ptrace_triggered(struct hw_breakpoint *bp, struct pt_regs *regs) > +{ > + /* > + * Unregister the breakpoint request here since ptrace has defined a > + * one-shot behaviour for breakpoint exceptions in PPC64. > + * The SIGTRAP signal is generated automatically for us in do_dabr(). > + * We don't have to do anything here > + */ > + unregister_user_hw_breakpoint(current, bp); > + kfree(bp); Couldn't you also clear the saved dabr info here, to avoid having to special case this in the actual breakpoint handler. Also, I think you should be delivering the signal here - for gdb compatibility I think we'll need to match the old behaviour which has the TRAP delivered before executing the breakpointed instruction. > +} > + > int ptrace_set_debugreg(struct task_struct *task, unsigned long addr, > unsigned long data) > { > +#ifdef CONFIG_PPC64 > + struct thread_struct *thread = &(task->thread); > + struct hw_breakpoint *bp; > + int ret; > +#endif > /* For ppc64 we support one DABR and no IABR's at the moment (ppc64). > * For embedded processors we support one DAC and no IAC's at the > * moment. > @@ -767,6 +785,35 @@ int ptrace_set_debugreg(struct task_stru > if (data && !(data & DABR_TRANSLATION)) > return -EIO; > > +#ifdef CONFIG_PPC64 > + bp = thread->hbp[0]; > + if (data == 0) { > + if (bp) { > + unregister_user_hw_breakpoint(task, bp); > + kfree(bp); > + } > + return 0; > + } > + > + if (bp) { > + bp->info.type = data & HW_BREAKPOINT_RW; > + task->thread.dabr = bp->info.address = data; > + return modify_user_hw_breakpoint(task, bp); > + } > + bp = kzalloc(sizeof(struct hw_breakpoint), GFP_KERNEL); > + if (!bp) > + return -ENOMEM; > + > + /* Store the type of breakpoint */ > + bp->info.type = data & HW_BREAKPOINT_RW; > + bp->triggered = ptrace_triggered; > + task->thread.dabr = bp->info.address = data; > + > + ret = register_user_hw_breakpoint(task, bp); > + if (ret) > + return ret; > +#endif /* CONFIG_PPC64 */ > + > /* Move contents to the DABR register */ > task->thread.dabr = data; > > > _______________________________________________ > Linuxppc-dev mailing list > Linuxppc-dev@ozlabs.org > https://ozlabs.org/mailman/listinfo/linuxppc-dev >
On Fri, Jun 05, 2009 at 03:13:45PM +1000, David Gibson wrote: > On Wed, Jun 03, 2009 at 10:05:24PM +0530, K.Prasad wrote: > > Modify the ptrace code to use the hardware breakpoint interfaces for user-space. > > > > Signed-off-by: K.Prasad <prasad@linux.vnet.ibm.com> > > --- > > arch/powerpc/kernel/ptrace.c | 47 +++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 47 insertions(+) > > > > Index: linux-2.6-tip.hbkpt/arch/powerpc/kernel/ptrace.c > > =================================================================== > > --- linux-2.6-tip.hbkpt.orig/arch/powerpc/kernel/ptrace.c > > +++ linux-2.6-tip.hbkpt/arch/powerpc/kernel/ptrace.c > > @@ -37,6 +37,7 @@ > > #include <asm/page.h> > > #include <asm/pgtable.h> > > #include <asm/system.h> > > +#include <asm/hw_breakpoint.h> > > > > /* > > * does not yet catch signals sent when the child dies. > > @@ -735,9 +736,26 @@ void user_disable_single_step(struct tas > > clear_tsk_thread_flag(task, TIF_SINGLESTEP); > > } > > > > +void ptrace_triggered(struct hw_breakpoint *bp, struct pt_regs *regs) > > +{ > > + /* > > + * Unregister the breakpoint request here since ptrace has defined a > > + * one-shot behaviour for breakpoint exceptions in PPC64. > > + * The SIGTRAP signal is generated automatically for us in do_dabr(). > > + * We don't have to do anything here > > + */ > > + unregister_user_hw_breakpoint(current, bp); > > + kfree(bp); > > Couldn't you also clear the saved dabr info here, to avoid having to > special case this in the actual breakpoint handler. > The saved dabr_data is created as a static variable and I didn't want to modify its value across files. > Also, I think you should be delivering the signal here - for gdb > compatibility I think we'll need to match the old behaviour which has > the TRAP delivered before executing the breakpointed instruction. > We could do it either way. Return a NOTIFY_DONE from hw_breakpoint_handler() and allow the do_dabr() code to deliver the signal, or deliver a signal here and return a NOTIFY_STOP in the exception handler. I chose the former as it doesn't duplicate the code. > -- > 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 Thanks, K.Prasad
On Wed, Jun 10, 2009 at 12:20:02PM +0530, K.Prasad wrote: > On Fri, Jun 05, 2009 at 03:13:45PM +1000, David Gibson wrote: > > On Wed, Jun 03, 2009 at 10:05:24PM +0530, K.Prasad wrote: > > > Modify the ptrace code to use the hardware breakpoint interfaces for user-space. > > > > > > Signed-off-by: K.Prasad <prasad@linux.vnet.ibm.com> > > > --- > > > arch/powerpc/kernel/ptrace.c | 47 +++++++++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 47 insertions(+) > > > > > > Index: linux-2.6-tip.hbkpt/arch/powerpc/kernel/ptrace.c > > > =================================================================== > > > --- linux-2.6-tip.hbkpt.orig/arch/powerpc/kernel/ptrace.c > > > +++ linux-2.6-tip.hbkpt/arch/powerpc/kernel/ptrace.c > > > @@ -37,6 +37,7 @@ > > > #include <asm/page.h> > > > #include <asm/pgtable.h> > > > #include <asm/system.h> > > > +#include <asm/hw_breakpoint.h> > > > > > > /* > > > * does not yet catch signals sent when the child dies. > > > @@ -735,9 +736,26 @@ void user_disable_single_step(struct tas > > > clear_tsk_thread_flag(task, TIF_SINGLESTEP); > > > } > > > > > > +void ptrace_triggered(struct hw_breakpoint *bp, struct pt_regs *regs) > > > +{ > > > + /* > > > + * Unregister the breakpoint request here since ptrace has defined a > > > + * one-shot behaviour for breakpoint exceptions in PPC64. > > > + * The SIGTRAP signal is generated automatically for us in do_dabr(). > > > + * We don't have to do anything here > > > + */ > > > + unregister_user_hw_breakpoint(current, bp); > > > + kfree(bp); > > > > Couldn't you also clear the saved dabr info here, to avoid having to > > special case this in the actual breakpoint handler. > > The saved dabr_data is created as a static variable and I didn't want to > modify its value across files. Hrm. I'm not sure which of these options is the uglier, to be honest. > > Also, I think you should be delivering the signal here - for gdb > > compatibility I think we'll need to match the old behaviour which has > > the TRAP delivered before executing the breakpointed instruction. > > > > We could do it either way. Return a NOTIFY_DONE from > hw_breakpoint_handler() and allow the do_dabr() > code to deliver the signal, or deliver a signal here and return a > NOTIFY_STOP in the exception handler. I chose the former as it doesn't > duplicate the code. I see. The thing is, since you're aiming to make *the* hardware breakpoint interface here, I think you really should be rewriting do_dabr entirely as part of this framework, not just adding your stuff as one option in there alongside the old-style ways of doing it.
Index: linux-2.6-tip.hbkpt/arch/powerpc/kernel/ptrace.c =================================================================== --- linux-2.6-tip.hbkpt.orig/arch/powerpc/kernel/ptrace.c +++ linux-2.6-tip.hbkpt/arch/powerpc/kernel/ptrace.c @@ -37,6 +37,7 @@ #include <asm/page.h> #include <asm/pgtable.h> #include <asm/system.h> +#include <asm/hw_breakpoint.h> /* * does not yet catch signals sent when the child dies. @@ -735,9 +736,26 @@ void user_disable_single_step(struct tas clear_tsk_thread_flag(task, TIF_SINGLESTEP); } +void ptrace_triggered(struct hw_breakpoint *bp, struct pt_regs *regs) +{ + /* + * Unregister the breakpoint request here since ptrace has defined a + * one-shot behaviour for breakpoint exceptions in PPC64. + * The SIGTRAP signal is generated automatically for us in do_dabr(). + * We don't have to do anything here + */ + unregister_user_hw_breakpoint(current, bp); + kfree(bp); +} + int ptrace_set_debugreg(struct task_struct *task, unsigned long addr, unsigned long data) { +#ifdef CONFIG_PPC64 + struct thread_struct *thread = &(task->thread); + struct hw_breakpoint *bp; + int ret; +#endif /* For ppc64 we support one DABR and no IABR's at the moment (ppc64). * For embedded processors we support one DAC and no IAC's at the * moment. @@ -767,6 +785,35 @@ int ptrace_set_debugreg(struct task_stru if (data && !(data & DABR_TRANSLATION)) return -EIO; +#ifdef CONFIG_PPC64 + bp = thread->hbp[0]; + if (data == 0) { + if (bp) { + unregister_user_hw_breakpoint(task, bp); + kfree(bp); + } + return 0; + } + + if (bp) { + bp->info.type = data & HW_BREAKPOINT_RW; + task->thread.dabr = bp->info.address = data; + return modify_user_hw_breakpoint(task, bp); + } + bp = kzalloc(sizeof(struct hw_breakpoint), GFP_KERNEL); + if (!bp) + return -ENOMEM; + + /* Store the type of breakpoint */ + bp->info.type = data & HW_BREAKPOINT_RW; + bp->triggered = ptrace_triggered; + task->thread.dabr = bp->info.address = data; + + ret = register_user_hw_breakpoint(task, bp); + if (ret) + return ret; +#endif /* CONFIG_PPC64 */ + /* Move contents to the DABR register */ task->thread.dabr = data;
Modify the ptrace code to use the hardware breakpoint interfaces for user-space. Signed-off-by: K.Prasad <prasad@linux.vnet.ibm.com> --- arch/powerpc/kernel/ptrace.c | 47 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+)