[RFC][PATCH 0/2] powerpc/booke: PTRACE_SINGLEBLOCK support for BookE

[RFC][PATCH 0/2] powerpc/booke: PTRACE_SINGLEBLOCK support for BookE

[James Yang]

PTRACE_SINGLEBLOCK support for BookE currently stops on the instruction
after taken branches.  This is different from the behavior on Server
where it stops after all branches.  

BookE was made to simulate Server by taking a single step after the
branch taken exception.  It is understood that the reason for making
PTRACE_SINGLEBLOCK on BookE to simulate Server was to make the semantics
exposed to user space identicial on both, but this is not really
possible due to the fundamental difference that untaken branches do not
trigger the branch taken exception in BookE.

BookE ISA's branch taken exception triggers before a branch that will be
taken executes.  This allows software to examine the branch and the
conditions under which it will be taken.  It also means software can
tell where basic blocks end (at least the ones which are terminated by
taken branches).  There are no architected registers that report the
address of the branch instruction after it has executed.

Server's branch trace exception triggers after a branch executes
regardless of whether or not it was taken.  The exception stops on the
instruction after fall-through branches.

Two mutually-exclusive patches are provided for RFC that expose BookE's
branch taken debug exception behavior accessible through
PTRACE_SINGLEBLOCK:  

- The first patch keeps the semantic behavior of the existing support by
  using the ptrace() addr parameter to select between the modes.  This
  requires a new bit in the TIF as well as changes in kernel/ptrace.c.

- The second patch makes PTRACE_SINGLEBLOCK reflect the BookE native
  behavior, which stops on the branch instruction.  The changes are
  isolated to arch/powerpc/kernel/traps.c.

IMHO, the only reason not to do the 2nd patch would be to maintain
compatibility for any tools that actually rely on the inaccurate
simulation of Server's behavior when run on a BookE system.  Are there
any tools that actually rely upon the behavior currently implemented for
BookE in Linux -- SIGTRAP only after taken branches?  Even if there are,
it should be possible to modify such a tool to issue a PTRACE_SINGLESTEP
after receiving the SIGTRAP on the branch to retain equivalent
functionality.

[RFC][PATCH 1/2] powerpc/booke: extend PTRACE_SINGLEBLOCK for BookE Branch Taken Debug

[James Yang]

A BookE branch taken debug exception followed by a single step does not
accurately simulate Server's branch execute debug exception.  BookE's
branch taken debug exception stops before the branch is to be executed
and only happens if the branch will actually be taken.  Server's branch
execute trace exception stops on the instruction after the branch
executes, regardless of whether or not the branch redirected the program
counter.

The existing PTRACE_SINGLEBLOCK support for BookE hardcodes a single
step after the branch taken exception is taken in order to simulate
Server's behavior, but this misses fall-through branch instructions
(i.e., branches that are NOT taken).  Also, the si_code became masked as
TRAP_TRACE instead of TRAP_BRANCH.

This patch makes available the unmodified BookE branch taken debug
exception through PTRACE_SINGLEBLOCK if the ptrace() addr parameter is
set to 2.  (The existing behavior of PTRACE_SINGLEBLOCK is retained for
any other addr parameter value, e.g., 0.)  SIGTRAP will be signaled with
the NIP pointing to the branch instruction before it has executed.  The
ptrace-calling program can then examine the program state.  It should
then request a PTRACE_SINGLESTEP in order to advance the program to the
next instruction or a PTRACE_CONT to resume normal program execution.
The si_code now also reports TRAP_BRANCH.

Signed-off-by: James Yang <James.Yang@freescale.com>
---
 arch/powerpc/include/asm/thread_info.h |    3 ++
 arch/powerpc/kernel/traps.c            |   51 ++++++++++++++++++++++++-------
 kernel/ptrace.c                        |   13 ++++++--
 3 files changed, 52 insertions(+), 15 deletions(-)

diff --git a/arch/powerpc/include/asm/thread_info.h b/arch/powerpc/include/asm/thread_info.h
index ba7b197..ab7c257 100644
--- a/arch/powerpc/include/asm/thread_info.h
+++ b/arch/powerpc/include/asm/thread_info.h
@@ -107,6 +107,8 @@ static inline struct thread_info *current_thread_info(void)
 #define TIF_EMULATE_STACK_STORE	16	/* Is an instruction emulation
 						for stack store? */
 #define TIF_MEMDIE		17	/* is terminating due to OOM killer */
+#define TIF_BOOKE_SINGLEBLOCK	18	/* Do not advance PC after Branch Taken
+					   debug exception for BookE */
 
 /* as above, but as bit values */
 #define _TIF_SYSCALL_TRACE	(1<<TIF_SYSCALL_TRACE)
@@ -125,6 +127,7 @@ static inline struct thread_info *current_thread_info(void)
 #define _TIF_UPROBE		(1<<TIF_UPROBE)
 #define _TIF_SYSCALL_TRACEPOINT	(1<<TIF_SYSCALL_TRACEPOINT)
 #define _TIF_EMULATE_STACK_STORE	(1<<TIF_EMULATE_STACK_STORE)
+#define _TIF_BOOKE_SINGLEBLOCK	(1<<TIF_BOOKE_SINGLEBLOCK)
 #define _TIF_NOHZ		(1<<TIF_NOHZ)
 #define _TIF_SYSCALL_T_OR_A	(_TIF_SYSCALL_TRACE | _TIF_SYSCALL_AUDIT | \
 				 _TIF_SECCOMP | _TIF_SYSCALL_TRACEPOINT | \
diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index c3ceaa2..ee899b3 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -1518,11 +1518,14 @@ void __kprobes DebugException(struct pt_regs *regs, unsigned long debug_status)
 {
 	current->thread.dbsr = debug_status;
 
-	/* Hack alert: On BookE, Branch Taken stops on the branch itself, while
-	 * on server, it stops on the target of the branch. In order to simulate
-	 * the server behaviour, we thus restart right away with a single step
-	 * instead of stopping here when hitting a BT
+	/* Hack alert: On BookE, Branch Taken stops on the branch itself iff the
+	 * branch will be taken, while on server, it stops on the target of the
+	 * branch, regardless of whether or not the branch was taken. In order
+	 * to simulate the server behaviour (at least for taken branches), we
+	 * thus restart right away with a single step instead of stopping here
+	 * when hitting a BT.
 	 */
+
 	if (debug_status & DBSR_BT) {
 		regs->msr &= ~MSR_DE;
 
@@ -1531,20 +1534,44 @@ void __kprobes DebugException(struct pt_regs *regs, unsigned long debug_status)
 		/* Clear the BT event */
 		mtspr(SPRN_DBSR, DBSR_BT);
 
-		/* Do the single step trick only when coming from userspace */
-		if (user_mode(regs)) {
-			current->thread.dbcr0 &= ~DBCR0_BT;
-			current->thread.dbcr0 |= DBCR0_IDM | DBCR0_IC;
-			regs->msr |= MSR_DE;
-			return;
-		}
-
 		if (notify_die(DIE_SSTEP, "block_step", regs, 5,
 			       5, SIGTRAP) == NOTIFY_STOP) {
 			return;
 		}
+
 		if (debugger_sstep(regs))
 			return;
+
+		if (user_mode(regs)) {
+
+			/* Do the single step trick only when coming from
+			 * userspace and if BOOKE SINGLEBLOCK mode is NOT
+			 * specified.
+			 */
+
+			if (test_thread_flag(TIF_BOOKE_SINGLEBLOCK)) {
+				current->thread.dbcr0 &= ~DBCR0_BT;
+				if (DBCR_ACTIVE_EVENTS(current->thread.dbcr0,
+							current->thread.dbcr1))
+					regs->msr |= MSR_DE;
+				else
+					/* Make sure the IDM bit is off */
+					current->thread.dbcr0 &= ~DBCR0_IDM;
+
+				_exception(SIGTRAP, regs, TRAP_BRANCH,
+					   regs->nip);
+			} else {
+				current->thread.dbcr0 &= ~DBCR0_BT;
+				current->thread.dbcr0 |= DBCR0_IDM | DBCR0_IC;
+				regs->msr |= MSR_DE;
+
+				/* XXX: Is this the correct fix? */
+				mtmsr(mfmsr() & ~MSR_DE);
+				mtspr(SPRN_DBCR0, current->thread.dbcr0);
+			}
+			return;
+		}
+
 	} else if (debug_status & DBSR_IC) { 	/* Instruction complete */
 		regs->msr &= ~MSR_DE;
 
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index aed981a..78db9f8 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -715,7 +715,7 @@ static int ptrace_peek_siginfo(struct task_struct *child,
 #endif
 
 static int ptrace_resume(struct task_struct *child, long request,
-			 unsigned long data)
+			 unsigned long addr, unsigned long data)
 {
 	if (!valid_signal(data))
 		return -EIO;
@@ -736,6 +736,13 @@ static int ptrace_resume(struct task_struct *child, long request,
 		if (unlikely(!arch_has_block_step()))
 			return -EIO;
 		user_enable_block_step(child);
+#ifdef TIF_BOOKE_SINGLEBLOCK
+		if (addr == 2) {	/* 2 is arbitrary */
+			set_tsk_thread_flag(child, TIF_BOOKE_SINGLEBLOCK);
+		} else {
+			clear_tsk_thread_flag(child, TIF_BOOKE_SINGLEBLOCK);
+		}
+#endif
 	} else if (is_singlestep(request) || is_sysemu_singlestep(request)) {
 		if (unlikely(!arch_has_single_step()))
 			return -EIO;
@@ -937,12 +944,12 @@ int ptrace_request(struct task_struct *child, long request,
 #endif
 	case PTRACE_SYSCALL:
 	case PTRACE_CONT:
-		return ptrace_resume(child, request, data);
+		return ptrace_resume(child, request, addr, data);
 
 	case PTRACE_KILL:
 		if (child->exit_state)	/* already dead */
 			return 0;
-		return ptrace_resume(child, request, SIGKILL);
+		return ptrace_resume(child, request, addr, SIGKILL);
 
 #ifdef CONFIG_HAVE_ARCH_TRACEHOOK
 	case PTRACE_GETREGSET:
-- 
1.7.0.4

[RFC][PATCH 2/2] powerpc/booke: revert PTRACE_SINGLEBLOCK to BookE behavior

[James Yang]

A BookE branch taken debug exception followed by a single step does not
accurately simulate Server's branch execute debug exception.  BookE's
branch taken debug exception stops before the branch is to be executed
and only happens if the branch will actually be taken.  Server's branch
execute trace exception stops on the instruction after the branch
executes, regardless of whether or not the branch redirected the program
counter.

The existing PTRACE_SINGLEBLOCK support for BookE hardcodes a single
step after the branch taken exception is taken in order to simulate
Server's behavior, but this misses fall-through branch instructions
(i.e., branches that are NOT taken).  Also, the si_code became masked as
TRAP_TRACE instead of TRAP_BRANCH.

This patch provides native support for the BookE branch taken debug
exception's behavior:  PTRACE_SINGLEBLOCK stops with a SIGTRAP before a
branch-that-would-be-taken would execute.  Userspace software will be
able to examine the process state upon catching the SIGTRAP, and it
will need to issue a PTRACE_SINGLESTEP or PTRACE_CONT to resume program
execution past the branch.

Signed-off-by: James Yang <James.Yang@freescale.com>
---
 arch/powerpc/kernel/traps.c |   40 +++++++++++++++++++++++++++-------------
 1 files changed, 27 insertions(+), 13 deletions(-)

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index c3ceaa2..5837d7f 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -1518,12 +1518,21 @@ void __kprobes DebugException(struct pt_regs *regs, unsigned long debug_status)
 {
 	current->thread.dbsr = debug_status;
 
-	/* Hack alert: On BookE, Branch Taken stops on the branch itself, while
-	 * on server, it stops on the target of the branch. In order to simulate
-	 * the server behaviour, we thus restart right away with a single step
-	 * instead of stopping here when hitting a BT
+	/* BookE's Branch Taken Debug Exception stops on the branch iff the
+	 * branch is resolved to be taken.  No exception occurs if the branch
+	 * is not taken (no exception if the branch does not redirect the PC).
+	 * This is unlike Classic/Server's behavior where the exception occurs
+	 * after the branch executes, regardless of whether or not the branch
+	 * redirected the PC.
+	 *
+	 * The past behavior of this function was to simulate Classic/Server's
+	 * behavior by performing a single-step upon a branch taken exception.
+	 * However, the simulation is not accurate because fall-through non-
+	 * taken branches would not result in a SIGTRAP.  Also, that SIGTRAP's
+	 * si_code would be reported as a TRAP_TRACE instead of a TRAP_BRANCH.
 	 */
-	if (debug_status & DBSR_BT) {
+
+	if (debug_status & DBSR_BT) {	/* Branch Taken */
 		regs->msr &= ~MSR_DE;
 
 		/* Disable BT */
@@ -1531,20 +1540,25 @@ void __kprobes DebugException(struct pt_regs *regs, unsigned long debug_status)
 		/* Clear the BT event */
 		mtspr(SPRN_DBSR, DBSR_BT);
 
-		/* Do the single step trick only when coming from userspace */
-		if (user_mode(regs)) {
-			current->thread.dbcr0 &= ~DBCR0_BT;
-			current->thread.dbcr0 |= DBCR0_IDM | DBCR0_IC;
-			regs->msr |= MSR_DE;
-			return;
-		}
-
 		if (notify_die(DIE_SSTEP, "block_step", regs, 5,
 			       5, SIGTRAP) == NOTIFY_STOP) {
 			return;
 		}
+
 		if (debugger_sstep(regs))
 			return;
+
+		if (user_mode(regs)) {
+			current->thread.dbcr0 &= ~DBCR0_BT;
+			if (DBCR_ACTIVE_EVENTS(current->thread.dbcr0,
+					       current->thread.dbcr1))
+				regs->msr |= MSR_DE;
+			else
+				/* Make sure the IDM bit is off */
+				current->thread.dbcr0 &= ~DBCR0_IDM;
+		}
+
+		_exception(SIGTRAP, regs, TRAP_BRANCH, regs->nip);
 	} else if (debug_status & DBSR_IC) { 	/* Instruction complete */
 		regs->msr &= ~MSR_DE;
 
-- 
1.7.0.4

Re: [RFC][PATCH 2/2] powerpc/booke: revert PTRACE_SINGLEBLOCK to BookE behavior

[Benjamin Herrenschmidt]

On Fri, 2013-07-05 at 17:11 -0500, James Yang wrote:
> A BookE branch taken debug exception followed by a single step does not
> accurately simulate Server's branch execute debug exception.  BookE's
> branch taken debug exception stops before the branch is to be executed
> and only happens if the branch will actually be taken.  Server's branch
> execute trace exception stops on the instruction after the branch
> executes, regardless of whether or not the branch redirected the program
> counter.
> 
> The existing PTRACE_SINGLEBLOCK support for BookE hardcodes a single
> step after the branch taken exception is taken in order to simulate
> Server's behavior, but this misses fall-through branch instructions
> (i.e., branches that are NOT taken).  Also, the si_code became masked as
> TRAP_TRACE instead of TRAP_BRANCH.

But that changes the user visible behaviour, won't that break gdb
expectations ?

Another way to "fix" it is to instead use lib/sstep.c to emulate the
single step maybe ?

On the other hand, I tend to think that trapping before the branch is
actually more useful especially if you don't have the CFAR register.

Cheers,
Ben.

Re: [RFC][PATCH 2/2] powerpc/booke: revert PTRACE_SINGLEBLOCK to BookE behavior

[James Yang]

On Sat, 6 Jul 2013, Benjamin Herrenschmidt wrote:

> On Fri, 2013-07-05 at 17:11 -0500, James Yang wrote:
> > A BookE branch taken debug exception followed by a single step does not
> > accurately simulate Server's branch execute debug exception.  BookE's
> > branch taken debug exception stops before the branch is to be executed
> > and only happens if the branch will actually be taken.  Server's branch
> > execute trace exception stops on the instruction after the branch
> > executes, regardless of whether or not the branch redirected the program
> > counter.
> > 
> > The existing PTRACE_SINGLEBLOCK support for BookE hardcodes a single
> > step after the branch taken exception is taken in order to simulate
> > Server's behavior, but this misses fall-through branch instructions
> > (i.e., branches that are NOT taken).  Also, the si_code became masked as
> > TRAP_TRACE instead of TRAP_BRANCH.
> 
> But that changes the user visible behaviour, won't that break gdb
> expectations ?

I am having a difficult time finding any use of PTRACE_SINGLEBLOCK in 
any arch in the various versions of gdb source trees I downloaded.  
Would you please provide a pointer or at least a hint as to where you 
think it would be?  I don't know gdb internals at all, but grepping 
the sources for PTRACE_SINGLEBLOCK returns nothing.


> Another way to "fix" it is to instead use lib/sstep.c to emulate the
> single step maybe ?

I don't think there's any issue any more with the hard coded single 
step with the fixes that Scott and Bharat recently provided.  
Actually, I don't even know if this feature was practically useful on 
BookE before those fixes due to the hangs and non-deterministic 
behavior.

Hypothetically, sstep.c could let server to emulate BookE's behavior..


> On the other hand, I tend to think that trapping before the branch is
> actually more useful especially if you don't have the CFAR register.

And there's no exception for fall-through branches.

So, yeah, this really is the question:  are there actually any tools 
that rely on PTRACE_SINGLEBLOCK behaving in the different ways it 
currently does on the two Power subarchiectures?  For BookE targes, 
how do they handle not being able to catch fall-through branches?  
For server targets, how do they capture the old LR value for blrl 
after the branch?  (I'm guessing not even sstep emulation can provide 
this information, though it might not be practically useful.)

Re: [RFC][PATCH 1/2] powerpc/booke: extend PTRACE_SINGLEBLOCK for BookE Branch Taken Debug

[Scott Wood]

On 07/05/2013 05:11:04 PM, James Yang wrote:
> This patch makes available the unmodified BookE branch taken debug
> exception through PTRACE_SINGLEBLOCK if the ptrace() addr parameter is
> set to 2.

This is hacky -- if we must retain the existing PTRACE_SINGLEBLOCK =20
semantics (which aren't actually documented anywhere AFAICT -- if the =20
details are meant to be implementation-defined, then don't we have some =20
freedom here to actually do what the hardware does, especially if we =20
can't find existing users?), then perhaps a new PTRACE_WHATEVER should =20
be defined.

At the very least, magic numbers should be given symbolic names.

-Scott=