[PATCH] powerpc ptrace block-step

[PATCH] powerpc ptrace block-step

[Roland McGrath]

I didn't test the booke variant of this (I don't have that hardware).
I only tested it on a Mac G5 (ppc64, 970FX).

There is a test program at:

http://sources.redhat.com/cgi-bin/cvsweb.cgi/~checkout~/tests/ptrace-tests/tests/block-step.c?cvsroot=systemtap

To build that program for powerpc, remove the && 0 from this line:

# elif defined __powerpc__  && 0	/* XXX not upstream yet */

and compile with -D_GNU_SOURCE.


Thanks,
Roland

---
[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/kernel/ptrace.c |   19 ++++++++++++++++++-
 include/asm-powerpc/ptrace.h |    4 ++++
 2 files changed, 22 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index 2a9fe97..91ee077 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -619,12 +619,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;
@@ -634,7 +651,7 @@ void user_disable_single_step(struct task_struct *task)
 		task->thread.dbcr0 = 0;
 		regs->msr &= ~MSR_DE;
 #else
-		regs->msr &= ~MSR_SE;
+		regs->msr &= ~(MSR_SE | MSR_BE);
 #endif
 	}
 	clear_tsk_thread_flag(task, TIF_SINGLESTEP);
diff --git a/include/asm-powerpc/ptrace.h b/include/asm-powerpc/ptrace.h
index 39023dd..6a3892d 100644
--- a/include/asm-powerpc/ptrace.h
+++ b/include/asm-powerpc/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)
 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__ */
@@ -276,4 +278,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 */

Re: [PATCH] powerpc ptrace block-step

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 899 bytes --]

On Thu, 2008-05-01 at 23:51 -0700, Roland McGrath wrote:
> I didn't test the booke variant of this (I don't have that hardware).
> I only tested it on a Mac G5 (ppc64, 970FX).
> 
> There is a test program at:
> 
> http://sources.redhat.com/cgi-bin/cvsweb.cgi/~checkout~/tests/ptrace-tests/tests/block-step.c?cvsroot=systemtap
> 
> To build that program for powerpc, remove the && 0 from this line:
> 
> # elif defined __powerpc__  && 0	/* XXX not upstream yet */
> 
> and compile with -D_GNU_SOURCE.

I ran this on a power3 and it worked fine, by which I mean the testcase
didn't hit any assertions AFAICT and returned 0.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[PATCH] powerpc ptrace block-step

[Roland McGrath]

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

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)
 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);
-- 
1.6.0.6

Re: [PATCH] powerpc ptrace block-step

[Benjamin Herrenschmidt]

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

Re: [PATCH] powerpc ptrace block-step

[Roland McGrath]

> 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

Re: [PATCH] powerpc ptrace block-step

[Josh Boyer]

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

Re: [PATCH] powerpc ptrace block-step

[Roland McGrath]

> 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

Re: [PATCH] powerpc ptrace block-step

[Frank Ch. Eigler]

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

Re: [PATCH] powerpc ptrace block-step

[Benjamin Herrenschmidt]

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.

Re: [PATCH] powerpc ptrace block-step

[Benjamin Herrenschmidt]

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.

Re: [PATCH] powerpc ptrace block-step

[Roland McGrath]

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


Thanks,
Roland

Re: [PATCH] powerpc ptrace block-step

[Benjamin Herrenschmidt]

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.