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

[David Gibson]

On Thu, Jun 18, 2009 at 11:50:45PM +0530, K.Prasad wrote:
> On Wed, Jun 17, 2009 at 02:32:24PM +1000, David Gibson wrote:
> > On Wed, Jun 10, 2009 at 02:38:06PM +0530, K.Prasad wrote:
[snip]
> > > +int arch_validate_hwbkpt_settings(struct hw_breakpoint *bp,
> > > +						struct task_struct *tsk)
> > > +{
> > > +	int is_kernel, ret = -EINVAL;
> > > +
> > > +	if (!bp)
> > > +		return ret;
> > > +
> > > +	switch (bp->info.type) {
> > > +	case HW_BREAKPOINT_READ:
> > > +	case HW_BREAKPOINT_WRITE:
> > > +	case HW_BREAKPOINT_RW:
> > > +		break;
> > > +	default:
> > > +		return ret;
> > > +	}
> > > +
> > > +	if (bp->triggered)
> > > +		ret = arch_store_info(bp, tsk);
> > 
> > Under what circumstances would triggered be NULL?  It's not clear to
> > me that you wouldn't still need arch_store_info() in this case.
> > 
> 
> Simple, triggered would be NULL when a user fails to specify it!
> This function would return EINVAL if that's the case.

Ah, right, yes.  This seems a really non obvious control flow for a
NULL triggered value to lead to EINVAL.  I'd prefer:

	if (!bp->triggered)
		return -EINVAL

	/* Then the kernel address test */

	return arch_store_info(bp, tsk);

[snip]
> > > +	/* Verify if dar lies within the address range occupied by the symbol
> > > +	 * being watched. Since we cannot get the symbol size for
> > > +	 * user-space requests we skip this check in that case
> > > +	 */
> > > +	if ((hbp_kernel_pos == 0) &&
> > > +	    !((bp->info.address <= dar) &&
> > > +	     (dar <= (bp->info.address + bp->info.symbolsize))))
> > > +		/*
> > > +		 * 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'.
> > > +		 */
> > > +		goto out;
> > > +
> > > +	(bp->triggered)(bp, regs);
> > 
> > So this confuses me.  You go to great efforts to step over the
> > instruction to generate a SIGTRAP after the instruction, for
> > consistency with x86.  But that SIGTRAP is *never* used, since the
> > only way to set userspace breakpoints is through ptrace at the moment.
> > At the same time, the triggered function is called here before the
> > instruction is executed, so not consistent with x86 anyway.
> > 
> > It just seems strange to me that sending a SIGTRAP is a special case
> > anyway.  Why can't sending a SIGTRAP be just a particular triggered
> > function.
> 
> The consistency in the interface across architectures that I referred to
> in my previous mail was w.r.t. continuous triggering of breakpoints and
> not to implement a trigger-before or trigger-after behaviour across
> architectures. In fact, this behaviour differs even on the same
> processor depending upon the breakpoint type (trigger-before for
> instructions and trigger-after for data in x86), and introducing
> uniformity might be a) at the cost of loss of some unique & innovative
> uses for each of them b) may not be always possible e.g. trigger-after
> to trigger-before.

Hrm.  Well (a) is why I was suggesting an option to allow trigger
before on powerpc.  Plus, I don't see why you think (a) is important
for the triggered function, but not for the timing of a SIGTRAP to
userspace.

As for (b), well there are already a bunch of things which can only be
done on certain archs/processors, so I don't see that's anything
really new.

How do you handle continuous breakpoints in the trigger-before case
(instruction breakpoints) on x86?  Do you need to step over an
instruction is the same manner as you do for powerpc?  In this case
where does x86 send the SIGTRAP?

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