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

[David Gibson]

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:
> 
> Hi David,
> 	Sorry for the delay in response below. In the meanwhile, I
> discovered an issue in detecting stray exceptions that affected
> user-space handling of breakpoints. I've made some changes to correct
> that behaviour which will be included in version VI of the patchset.

[snip]
> > > +	/*
> > > +	 * User-space requests will always have the address field populated
> > > +	 * For kernel-addresses, either the address or symbol name can be
> > > +	 * specified.
> > > +	 */
> > > +	if (bp->info.name)
> > > +		bp->info.address = (unsigned long)
> > > +					kallsyms_lookup_name(bp->info.name);
> > 
> > Archs don't have to implement this name lookup stuff, but it looks
> > like most of them would - so it looks like there ought to be a helper
> > function in generic code that will do the check / name lookup stuff.
> 
> It doesn't turn out to be very generic. The IO breakpoints in x86, the
> address-range (only) breakpoints in S390 and perhaps 4xx powerpc
> processors were what made me think that this should remain in
> arch-specific code. In these cases, we might have to deal only with
> breakpoint addresses and not names.

Hrm, ok.  I was suggesting a helper function that those archs which
can use it call at need, not moving the address lookup to generic code
in the sense of being used everywhere.  Whatever, it's not that
important at this stage.

> > > +	if (bp->info.address)
> > > +		return 0;
> > 
> > Hrm.. you realise there's no theoretical reason a userspace program
> > couldn't put a breakpoint at address 0...?
> 
> I agree. I think there must be parts of code that works based on this
> assumption. Will check and remove them.

Ok, good.

[snip]
> > > +	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 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]
> > > +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?

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