Re: [Patch 3/6] Modify ptrace code to use Hardware Breakpoint interfaces

[David Gibson <dwg@au1.ibm.com>]

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.

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