Re: [Patch 2/6] Introduce PPC64 specific Hardware Breakpoint interfaces

["K.Prasad" <prasad@linux.vnet.ibm.com>]

On Mon, Jun 15, 2009 at 04:40:45PM +1000, David Gibson wrote:
> On Wed, Jun 10, 2009 at 12:13:49PM +0530, K.Prasad wrote:
> > On Fri, Jun 05, 2009 at 03:11:58PM +1000, David Gibson wrote:
> > > On Wed, Jun 03, 2009 at 10:05:11PM +0530, K.Prasad wrote:
> > 
> > > > +	else {
> > > > +		/*
> > > > +		 * 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'.
> > > > +		 */
> > > > +		rc = NOTIFY_STOP;
> > > > +		goto out;
> > > > +	}
> > > > +	is_one_shot = (bp->triggered == ptrace_triggered) ? 1 : 0;
> > > 
> > > Ouch, explicitly special-casing ptrace_triggered is pretty nasty.
> > > Since the bp_info is already arch specific, maybe it should include a
> > > flag to indicate whether the breakpoint is one-shot or not.
> > > 
> > 
> > The reason to check for ptrace_triggered is to contain the one-shot
> > behaviour only to ptrace (thus retaining the semantics) and not to extend
> > them to all user-space requests through
> > register_user_hw_breakpoint().
> 
> Right, but couldn't you implement that withing ptrace_triggered
> itself, without a special test here, by having it cancel the
> breakpoint.
> 

A special check (either using the callback routine as above, or using a
special flag) will be required in hw_breakpoint_handler() to enable
early return (without single-stepping). I'm not sure if I got your
suggestion right, and let me know if you think so.

> > A one-shot behaviour for all user-space requests would create more work
> > for the user-space programs (such as re-registration) and will leave open
> > a small window of opportunity for debug register grabbing by kernel-space
> > requests.
> > 
> > So, in effect a request through register_user_hw_breakpoint() interface
> > will behave as under:
> > - Single-step over the causative instruction that triggered the
> >   breakpoint exception handler.
> > - Deliver the SIGTRAP signal to user-space after executing the causative
> >   instruction.
> > 
> > This behaviour is in consonance with that of kernel-space requests and
> > those on x86 processors, and helps define a consistent behaviour across
> > architectures for user-space.
> > 
> > Let me know what you think on the same.
> 
> I certainly see the value in consistent semantics across archs.
> However, I can also see uses for the powerpc trap-before-execute
> behaviour.  That's why I'm suggesting it might be worth having an
> arch-specific flag.
> 
> [snip]

So, you suggest that the 'one-shot' behaviour should be driven by
user-request and not just confined to ptrace? (The default behaviour for
all breakpoints-minus-ptrace will remain 'continuous' though).

It can be implemented through an additional flag in 'struct
arch_hw_breakpoint'. I can send a new version 7 of the patchset with this
change (with the hope that the version 6 of the patchset looks fine in
its present form!). Meanwhile, we'd like to know what uses you see in
addition to the present one if the one-shot behaviour is made
user-defined. Are those uses beyond what can be achieved through the
present ptrace interface?

> > > > +int __kprobes single_step_dabr_instruction(struct die_args *args)
> > > > +{
> > > > +	struct pt_regs *regs = args->regs;
> > > > +	int cpu = get_cpu();
> > > > +	int ret = NOTIFY_DONE;
> > > > +	siginfo_t info;
> > > > +	unsigned long this_dabr_data = per_cpu(dabr_data, cpu);
> > > > +
> > > > +	/*
> > > > +	 * Check if we are single-stepping as a result of a
> > > > +	 * previous HW Breakpoint exception
> > > > +	 */
> > > > +	if (this_dabr_data == 0)
> > > > +		goto out;
> > > > +
> > > > +	regs->msr &= ~MSR_SE;
> > > > +	/* Deliver signal to user-space */
> > > > +	if (this_dabr_data < TASK_SIZE) {
> > > > +		info.si_signo = SIGTRAP;
> > > > +		info.si_errno = 0;
> > > > +		info.si_code = TRAP_HWBKPT;
> > > > +		info.si_addr = (void __user *)(per_cpu(dabr_data, cpu));
> > > > +		force_sig_info(SIGTRAP, &info, current);
> > > 
> > > Uh.. I recall mentioning in my previous review that in order to match
> > > previous behaviour we need to deliver the userspace signal *before*
> > > stepping over the breakpointed instruction, rather than after (which
> > > I guess is why breakpoints are one-shot in the old scheme).
> > 
> > This code would implement the behaviour as stated in the comment for
> > user-space requests above.
> 
> And you're relying on the old trap-sending code in do_dabr for ptrace
> requests?
> 

Yes.

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