Re: [Patch 1/1] PPC64-HWBKPT: Implement hw-breakpoints for PPC64

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

On Sun, Feb 21, 2010 at 02:01:37AM +0100, Frederic Weisbecker wrote:
> On Mon, Feb 15, 2010 at 11:29:14AM +0530, K.Prasad wrote:
> > +struct arch_hw_breakpoint {
> > +	u8		len; /* length of the target symbol */
> > +	int		type;
> > +	char		*name; /* Contains name of the symbol to set bkpt */
> > +	unsigned long	address;
> > +};
> 
> 
> 
> 
> I don't think it's a good idea to integrate the name of
> the target. This is something that should be done in a higher
> level, not in an arch backend.
> We don't even need to store it anywhere as we can resolve
> back an address easily. Symbol awareness is not something
> the hardware breakpoint should care about, neither in the
> arch nor the generic level.
> 

The 'name' field here is actually a legacy inherited from x86 code. It
is part of x86's arch-specific hw-breakpoint structure since:
- inspired by the kprobe implementation which accepts symbol name as
  input.
- kallsyms_lookup_name() was 'unexported' and a module could not resolve
  symbol names externally, so the core-infrastructure had to provide
  such facilities.
- I wasn't sure about the discussions behind 'unexporting' of
  kallsyms_lookup_name(), so did not venture to export them again (which
  you rightfully did :-)

Having said that, I'll be glad to remove this field (along with that in
x86), provided we know that there's a way for the user to resolve symbol
names on its own i.e. routines like kallsyms_lookup_name() will remain
exported.

> Also, do you think addr/len/type is enough to abstract out
> any ppc breakpoints?
> 
> This looks enough to me to express range breakpoints and
> simple breakpoints. But what about value comparison?
> (And still, there may be other trickier implementations
> I don't know in ppc).
> 

The above implementation is for PPC64 architecture that supports only
'simple' breakpoints of fixed length (no range breakpoints, no value
comparison). More on that below.
 
> > +
> > +#include <linux/kdebug.h>
> > +#include <asm/reg.h>
> > +#include <asm/system.h>
> > +
> > +/* Total number of available HW breakpoint registers */
> > +#define HBP_NUM 1
> 
> 
> Looking at the G2 PowerPc implementation, DABR and DABR2 can either
> express two different watchpoints or one range watchpoint.
> 
> There are also IABR and IABR2 for instruction breakpoints that
> follow the same above scheme. I'm not sure we can abstract that
> using a constant max linear number of resources.
> 
> 

As stated above, the patch implements breakpoints for PPC64 processors
only (http://www.power.org/resources/downloads/PowerISA_V2.06_PUBLIC.pdf),
which does not support advanced breakpoint features (which its ppc
Book-E counterpart has).
 
> > +static inline void hw_breakpoint_disable(void)
> > +{
> > +	set_dabr(0);
> > +}
> 
> 
> So, this is only about data breakpoints?
> 
> 

Yes, newer PPC64 processors do not support IABR.

> > +	/*
> > +	 * As a policy, the callback is invoked in a 'trigger-after-execute'
> > +	 * fashion
> > +	 */
> > +	(bp->overflow_handler)(bp, 0, NULL, regs);
> 
> 
> Why are you calling this explicitly instead of using the perf_bp_event()
> thing? This looks like it won't work with perf as the event won't
> be recorded by perf.
> 

Yes, should have invoked perf_bp_event() for perf to work well (on a
side note, it makes me wonder at the amount of 'extra' code that each
breakpoint exception would execute if it were not called through perf
sys-call...well, the costs of integrating with a generic infrastructure!)

> > +void ptrace_triggered(struct perf_event *bp, int nmi,
> > +		      struct perf_sample_data *data, struct pt_regs *regs)
> > +{
> > +	struct perf_event_attr attr;
> > +
> > +	/*
> > +	 * Disable 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 about that here
> > +	 */
> 
> 
> Oh, why does ptrace use a one-shot behaviour in ppc? Breakpoints
> only trigger once?
> 

Yes, ptrace breakpoints on PPC64 are designed to trigger once and this
patch retains that behaviour. It is very convenient to use one-shot
behaviour on archs where exceptions are triggered-before-execute.

> > +	if (bp) {
> > +		attr = bp->attr;
> > +		attr.bp_addr = data & ~HW_BREAKPOINT_ALIGN;
> > +
> > +		switch (data & (DABR_DATA_WRITE | DABR_DATA_READ)) {
> > +		case DABR_DATA_READ:
> > +			attr.bp_type = HW_BREAKPOINT_R;
> > +			break;
> > +		case DABR_DATA_WRITE:
> > +			attr.bp_type = HW_BREAKPOINT_W;
> > +			break;
> > +		case (DABR_DATA_WRITE | DABR_DATA_READ):
> > +			attr.bp_type = HW_BREAKPOINT_R | HW_BREAKPOINT_W;
> > +			break;
> > +		}
> > +		ret =  modify_user_hw_breakpoint(bp, &attr);
> > +		if (ret)
> > +			return ret;
> > +		thread->ptrace_bps[0] = bp;
> > +		thread->dabr = data;
> > +		return 0;
> > +	}
> > +
> > +	/* Create a new breakpoint request if one doesn't exist already */
> > +	hw_breakpoint_init(&attr);
> > +	attr.bp_addr = data & ~HW_BREAKPOINT_ALIGN;
> > +	switch (data & (DABR_DATA_WRITE | DABR_DATA_READ)) {
> > +	case DABR_DATA_READ:
> > +		attr.bp_type = HW_BREAKPOINT_R;
> > +		break;
> > +	case DABR_DATA_WRITE:
> > +		attr.bp_type = HW_BREAKPOINT_W;
> > +		break;
> > +	case (DABR_DATA_WRITE | DABR_DATA_READ):
> > +		attr.bp_type = HW_BREAKPOINT_R | HW_BREAKPOINT_W;
> > +		break;
> > +	}
> > +	thread->ptrace_bps[0] = bp = register_user_hw_breakpoint(&attr,
> > +							ptrace_triggered, task);
> > +	if (IS_ERR(bp)) {
> > +		thread->ptrace_bps[0] = NULL;
> > +		return PTR_ERR(bp);
> > +	}
> > +
> > +#endif /* CONFIG_PPC64 */
> 
> 
> 
> This looks fine for basic breakpoints. And this can probably be
> improved to integrate ranges.
> 
> But I think we did something wrong with the generic breakpoint
> interface. We are translating the arch values to generic
> attributes. Then this all will be translated back to arch
> values.
> 
> Having generic attributes is necessary for any perf event
> use from userspace. But it looks like a waste for ptrace
> that already gives us arch values. And the problem
> is the same for x86.
> 
> So I think we should implement a register_ptrace_breakpoint()
> that doesn't take perf_event_attr but specific arch informations,
> so that we don't need to pass through a generic conversion, which:
> 

I agree that the layers of conversion from generic to arch-specific
breakpoint constants is wasteful.
Can't the arch_bp_generic_fields() function be moved to
arch/x86/kernel/ptrace.c instead of a new interface?

> - is wasteful
> - won't be able to express 100% of any arch capabilities. We
>   can certainly express most arch breakpoints features through
>   the generic interface, but not all of them (given how tricky
>   the data value comparison features can be)
> 
> I will rework that during the next cycle.
> 
> Thanks.
>

Thank you for the comments. I will re-send a new version of the patch
with the perf_bp_event() substitution.

-- K.Prasad