[RFC:PATCH 03/03] powerpc: Add support for BookE Debug Reg. traps, exceptions and ptrace

Dave Kleikamp shaggy at linux.vnet.ibm.com
Tue Jan 19 09:31:34 EST 2010


On Fri, 2009-12-11 at 14:26 +1100, David Gibson wrote:
> On Thu, Dec 10, 2009 at 01:57:27PM -0200, Dave Kleikamp wrote:
> > powerpc: Add support for BookE Debug Reg. traps, exceptions and ptrace
> > 
> > From: Torez Smith <lnxtorez at linux.vnet.ibm.com>
> > 
> > This patch defines context switch and trap related functionality
> > for BookE specific Debug Registers. It adds support to ptrace()
> > for setting and getting BookE related Debug Registers

> [snip]
> > +#if !(defined(CONFIG_40x) || defined(CONFIG_BOOKE))
> >  void do_dabr(struct pt_regs *regs, unsigned long address,
> >  		    unsigned long error_code)
> >  {
> > @@ -257,12 +275,6 @@ void do_dabr(struct pt_regs *regs, unsigned long address,
> >  	if (debugger_dabr_match(regs))
> >  		return;
> >  
> > -	/* Clear the DAC and struct entries.  One shot trigger */
> > -#if defined(CONFIG_BOOKE)
> > -	mtspr(SPRN_DBCR0, mfspr(SPRN_DBCR0) & ~(DBSR_DAC1R | DBSR_DAC1W
> > -							| DBCR0_IDM));
> > -#endif
> > -
> >  	/* Clear the DABR */
> >  	set_dabr(0);
> 
> Uh.. does this imply we're keeping the one-shot behaviour for
> new-style breakpoints?  To me the interface really suggests they're
> persistent, although dealing with the semantics of that at signal time
> can get curly.

I've left it as one-shot.  The gdb guys seem happy with that.

[snip]

> >  int set_dabr(unsigned long dabr)
> >  {
> >  	__get_cpu_var(current_dabr) = dabr;
> > @@ -284,7 +358,7 @@ int set_dabr(unsigned long dabr)
> >  		return ppc_md.set_dabr(dabr);
> >  
> >  	/* XXX should we have a CPU_FTR_HAS_DABR ? */
> > -#if defined(CONFIG_BOOKE)
> > +#if defined(CONFIG_BOOKE) || defined(CONFIG_40x)
> >  	mtspr(SPRN_DAC1, dabr);
> 
> Uh.. this would seem to be wrong.  set_dabr(0) is called from the
> debug exception - but nowadays that could be tiggered by a DAC other
> than DAC1.

Right now, the only place left that set_dabr() is called for BOOKE or
40x is from xmon.  I think that's already a problem in that it doesn't
play nicely with ptrace and it fails to set DBCR0_DAC1R or DBCR0_DAC1W.
We probably need an independent patch for that.


> [snip]
> > +#else
> >  	/* As described above, it was assumed 3 bits were passed with the data
> >  	 *  address, but we will assume only the mode bits will be passed
> >  	 *  as to not cause alignment restrictions for DAC-based processors.
> >  	 */
> >  
> >  	/* DAC's hold the whole address without any mode flags */
> > -	task->thread.dabr = data & ~0x3UL;
> > -
> > -	if (task->thread.dabr == 0) {
> > -		task->thread.dbcr0 &= ~(DBSR_DAC1R | DBSR_DAC1W | DBCR0_IDM);
> > -		task->thread.regs->msr &= ~MSR_DE;
> > +	task->thread.dac1 = data & ~0x3UL;
> > +
> > +	if (task->thread.dac1 == 0) {
> > +		dbcr_dac(task) &= ~(DBCR_DAC1R | DBCR_DAC1W);
> > +		if (!DBCR_ACTIVE_EVENTS(task->thread.dbcr0,
> > +					task->thread.dbcr1)) {
> > +			task->thread.regs->msr &= ~MSR_DE;
> > +			task->thread.dbcr0 &= ~DBCR0_IDM;
> > +		}
> >  		return 0;
> >  	}
> 
> Ok, so effectively the old ptrace method of setting the DABR acts as a
> bypass to set DAC1, rather than having the old interface being
> implemented via the new interface.  This has some weirdness - you can
> clobber a new-style breakpoint in DAC1 using the old interface, for
> example.  Still, it might be the simplest approach for a first cut.

I really didn't consider a program using both the old and new interface.

> What *is* a problem though, is that this means that the SIGTRAP will
> always give a slot number, even for a breakpoint established using the
> old interface.  Part of the idea of encoding the registered breakpoint
> number in the siginfo was to be able to distinguish between old-style
> and new-style breakpoints at trap time.

Do we realistically expect both the old and new style breakpoints to be
used together?  I'm having trouble visualizing the scenerio.

> [snip]

> > +	int slot;
> > +
> > +	if (byte_enable && (condition_mode == 0))
> > +		return -EINVAL;
> > +
> > +	if (bp_info->addr >= TASK_SIZE)
> > +		return -EIO;
> > +
> > +	if ((dbcr_dac(child) & (DBCR_DAC1R | DBCR_DAC1W)) == 0) {
> > +		if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_READ)
> > +			dbcr_dac(child) |= DBCR_DAC1R;
> > +		if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_WRITE)
> > +			dbcr_dac(child) |= DBCR_DAC1W;
> > +		child->thread.dac1 = (unsigned long)bp_info->addr;
> > +#ifdef CONFIG_BOOKE
> 
> Better to have a runtime feature bit test here, than use an #ifdef to
> distinguish the 40x and BookE cases.  Plus, you should return an error
> if the user attempts to use a feature not supported on this hardware,
> which doesn't seem to happen here.

I'll take a look at making this a runtime feature.

Shaggy
-- 
David Kleikamp
IBM Linux Technology Center



More information about the Linuxppc-dev mailing list