powerpc ptrace block-step

Maynard asked about user_enable_block_step() support on powerpc.
This is the old patch I've posted before.  I haven't even tried
to compile it lately, but it rebased cleanly.

AFAIK the only reason this didn't go in several months ago was waiting
for someone to decide what the right arch_has_block_step() condition was,
i.e. if it needs to check some cpu_feature or chip identifier bits.

I had hoped that I had passed the buck then to ppc folks to figure that out
and make it so.  But it does not appear to have happened.

Note you can drop the #define PTRACE_SINGLEBLOCK if you want to be
conservative and not touch the user (ptrace) ABI yet.  Then Maynard
could beat on it with internal uses (utrace) before you worry about
whether userland expects the new ptrace request macro to exist.


Thanks,
Roland
---
From 2482ed1a0ced9caf964275889ea2315916e84ada Mon Sep 17 00:00:00 2001
From: Roland McGrath <roland@redhat.com>
Date: Thu, 1 May 2008 23:40:58 -0700
Subject: [PATCH] powerpc ptrace block-step

This adds block-step support on powerpc,
including a PTRACE_SINGLEBLOCK request for ptrace.

Signed-off-by: Roland McGrath <roland@redhat.com>
---
 arch/powerpc/include/asm/ptrace.h |    4 ++++
 arch/powerpc/kernel/ptrace.c      |   19 ++++++++++++++++++-
 2 files changed, 22 insertions(+), 1 deletions(-)

On Wed, 2009-04-01 at 14:59 -0700, Roland McGrath wrote:

> diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
> index c9c678f..d7692b8 100644
> --- a/arch/powerpc/include/asm/ptrace.h
> +++ b/arch/powerpc/include/asm/ptrace.h
> @@ -135,7 +135,9 @@ do {									      \
>   * These are defined as per linux/ptrace.h, which see.
>   */
>  #define arch_has_single_step()	(1)
> +#define arch_has_block_step()	(1)

The patch only implements it for "server/classic" processors, not BookE,
thus it should probably only advertise it for these :-)

Though it wouldn't be too hard to implement it for BookE using DBCR0:BRT
(Branch Taken debug event) though it might need some careful fixups such
as the one we have for single step regarding hitting exception entry
code.

Cheers,
Ben.

>  extern void user_enable_single_step(struct task_struct *);
> +extern void user_enable_block_step(struct task_struct *);
>  extern void user_disable_single_step(struct task_struct *);
>
>  #endif /* __ASSEMBLY__ */
> @@ -288,4 +290,6 @@ extern void user_disable_single_step(struct task_struct *);
>  #define PPC_PTRACE_PEEKUSR_3264  0x91
>  #define PPC_PTRACE_POKEUSR_3264  0x90
>  
> +#define PTRACE_SINGLEBLOCK	0x100	/* resume execution until next branch */
> +
>  #endif /* _ASM_POWERPC_PTRACE_H */
> diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
> index 3635be6..656fea2 100644
> --- a/arch/powerpc/kernel/ptrace.c
> +++ b/arch/powerpc/kernel/ptrace.c
> @@ -707,12 +707,29 @@ void user_enable_single_step(struct task_struct *task)
>  		task->thread.dbcr0 |= DBCR0_IDM | DBCR0_IC;
>  		regs->msr |= MSR_DE;
>  #else
> +		regs->msr &= ~MSR_BE;
>  		regs->msr |= MSR_SE;
>  #endif
>  	}
>  	set_tsk_thread_flag(task, TIF_SINGLESTEP);
>  }
>  
> +void user_enable_block_step(struct task_struct *task)
> +{
> +	struct pt_regs *regs = task->thread.regs;
> +
> +	if (regs != NULL) {
> +#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
> +		task->thread.dbcr0 = DBCR0_IDM | DBCR0_BT;
> +		regs->msr |= MSR_DE;
> +#else
> +		regs->msr &= ~MSR_SE;
> +		regs->msr |= MSR_BE;
> +#endif
> +	}
> +	set_tsk_thread_flag(task, TIF_SINGLESTEP);
> +}
> +
>  void user_disable_single_step(struct task_struct *task)
>  {
>  	struct pt_regs *regs = task->thread.regs;
> @@ -729,7 +746,7 @@ void user_disable_single_step(struct task_struct *task)
>  		task->thread.dbcr0 &= ~(DBCR0_IC | DBCR0_IDM);
>  		regs->msr &= ~MSR_DE;
>  #else
> -		regs->msr &= ~MSR_SE;
> +		regs->msr &= ~(MSR_SE | MSR_BE);
>  #endif
>  	}
>  	clear_tsk_thread_flag(task, TIF_SINGLESTEP);

> The patch only implements it for "server/classic" processors, not BookE,
> thus it should probably only advertise it for these :-)
>
> Though it wouldn't be too hard to implement it for BookE using DBCR0:BRT
> (Branch Taken debug event) though it might need some careful fixups such
> as the one we have for single step regarding hitting exception entry
> code.

In that case, this code seems fairly mysterious:

> > +#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
> > +		task->thread.dbcr0 = DBCR0_IDM | DBCR0_BT;
> > +		regs->msr |= MSR_DE;

That doesn't already do whatever it is you described?

Can we assume now that you or someone else who knows what all that means
will take this up?


Thanks,
Roland

On Thu, Apr 02, 2009 at 05:44:50PM -0700, Roland McGrath wrote:
>> The patch only implements it for "server/classic" processors, not BookE,
>> thus it should probably only advertise it for these :-)
>>
>> Though it wouldn't be too hard to implement it for BookE using DBCR0:BRT
>> (Branch Taken debug event) though it might need some careful fixups such
>> as the one we have for single step regarding hitting exception entry
>> code.
>
>In that case, this code seems fairly mysterious:
>
>> > +#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
>> > +		task->thread.dbcr0 = DBCR0_IDM | DBCR0_BT;
>> > +		regs->msr |= MSR_DE;
>
>That doesn't already do whatever it is you described?
>
>Can we assume now that you or someone else who knows what all that means
>will take this up?

I will try and look at the patch a bit more tomorrow, yes.

I don't think having it working for BookE is really a requirement before this
gets in though.  If we can get it working with minimal effort for ppc64, that
would help get systemtap and related things functioning correctly there.

While I would love to believe that systemtap should work everywhere, I can't
really see it running on an embedded board at this point.

josh

On Thu, 2009-04-02 at 17:44 -0700, Roland McGrath wrote:
> > The patch only implements it for "server/classic" processors, not BookE,
> > thus it should probably only advertise it for these :-)
> >
> > Though it wouldn't be too hard to implement it for BookE using DBCR0:BRT
> > (Branch Taken debug event) though it might need some careful fixups such
> > as the one we have for single step regarding hitting exception entry
> > code.
> 
> In that case, this code seems fairly mysterious:
> 
> > > +#if defined(CONFIG_40x) || defined(CONFIG_BOOKE)
> > > +		task->thread.dbcr0 = DBCR0_IDM | DBCR0_BT;
> > > +		regs->msr |= MSR_DE;
> 
> That doesn't already do whatever it is you described?

It should, I missed that bit. Except for the possible issue with
interrupts.

> Can we assume now that you or someone else who knows what all that means
> will take this up?

I can take this up after I'm back from vacation, which will be in about
4 weeks from now, but maybe Josh can give it a go in the meantime.

Basically, the "issue" with BookE is that the debug interrupts aren't
masked by the fact of taking an exception. So for example, if you have
single step enabled and take a TLB miss on a userland load, you'll take
a single step exception on the first (or rather the second but that's
a detail) instruction of the TLB miss exception vector.

The code for our BookE debug interrupts has a workaround that detects
that case and returns to the TLB miss vector with MSR:DE cleared, but
I think that code will not properly catch a similar things happening
due to block step. Though is should be easy to fix.

Cheers,
Ben.

> I don't think having it working for BookE is really a requirement before this
> gets in though.  If we can get it working with minimal effort for ppc64, that
> would help get systemtap and related things functioning correctly there.

Sure, just conditionalize arch_has_block_step() however is correct for that
and put in what already works earlier rather than later, I'd say.  Then
users of the excluded chips can come forward when they care, and you can
worry about it then if you don't happen to get to it first.


Thanks,
Roland

Josh Boyer <jwboyer@linux.vnet.ibm.com> writes:

> [...]  While I would love to believe that systemtap should work
> everywhere, I can't really see it running on an embedded board at
> this point.

FWIW, we have had people running (not compiling) systemtap probe
modules on embedded systems.  See "stap -p4" or "stap-server".

- FChE

On Wed, 2009-04-01 at 14:59 -0700, Roland McGrath wrote:
> Maynard asked about user_enable_block_step() support on powerpc.
> This is the old patch I've posted before.  I haven't even tried
> to compile it lately, but it rebased cleanly.
> 
> AFAIK the only reason this didn't go in several months ago was waiting
> for someone to decide what the right arch_has_block_step() condition was,
> i.e. if it needs to check some cpu_feature or chip identifier bits.
> 
> I had hoped that I had passed the buck then to ppc folks to figure that out
> and make it so.  But it does not appear to have happened.
> 
> Note you can drop the #define PTRACE_SINGLEBLOCK if you want to be
> conservative and not touch the user (ptrace) ABI yet.  Then Maynard
> could beat on it with internal uses (utrace) before you worry about
> whether userland expects the new ptrace request macro to exist.

So the patch had some issues, such as missing clearing of DBCR0 bits,
missing changes to code in traps.c to properly identify the new cause
of debug interrupts, etc...

I've spinned a new version, I'll post it as soon as I got to do some
quick tests. It will then go into the next merge window hopefully.

Note: I've verified, blockstep seems to be implemented by all the core
variants -except- the old 601.

Cheers,
Ben.

Thanks!  I'm very glad to finally see this ironed out by someone who
actually knows about powerpc innards.


Thanks,
Roland

On Fri, 2009-05-29 at 00:32 -0700, Roland McGrath wrote:
> Thanks!  I'm very glad to finally see this ironed out by someone who
> actually knows about powerpc innards.

yeah, it's been on my todo list for some time... decided that it stayed
rotting for too long. We also did a little test program to exercise wich
is how I discovered the subtle difference between BookE and server.

Any comment about my approach of making BookE "look like" server by
sticking a single step in there ? IE. Is the semantic of stopping on the
-target- of the branch what userspace expects ?

Cheers,
Ben.