[Qemu-devel] [PATCH] Fix broken PPC user space single stepping

[Qemu-devel] [PATCH] Fix broken PPC user space single stepping

[Jason Wessel]

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

This is a refreshed patch from the prior version I submitted with an
updated description of the problem of the PPC single stepping problem. 
It also fixes the generator to correctly handle MSR_SE on branches.

Please consider applying it, or comment as to what further changes are
needed and I will gladly make further changes.

Thanks,
Jason.


[-- Attachment #2: ppc_system_single_step.patch --]
[-- Type: text/x-patch, Size: 4770 bytes --]

From: Jason Wessel <jason.wessel@windriver.com>
Subject: [PATCH] Fix ppc user space single stepping

User space OS single stepping will hang a qemu instance because the
instruction translation executes an internal debug exception that is
meant for the qemu backend debugger.  

The test case is to use any program which executes a ptrace attach to
a process and then "ptrace(PTRACE_SINGLESTEP,pid,0,0);".  In general
you could simply use gdb on the target and request it to "instruction
step" a process with "si", so long as gdb is setup to not emulate
single stepping with breakpoints.

This patch splits the run-time single stepping from the debugger stub
single stepping such that each type of single stepping gets the
correct exception generation.

This patch also fixes the defect where branches were incorrectly
calculated for the update to the env->nip when setting MSR_SE on
a branch instruction.

Signed-off-by: Jason Wessel <jason.wessel@windriver.com>

---
 target-ppc/translate.c |   41 ++++++++++++++++++++++++++++++-----------
 1 file changed, 30 insertions(+), 11 deletions(-)

--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -29,6 +29,9 @@
 #include "tcg-op.h"
 #include "qemu-common.h"
 
+#define CPU_SINGLE_STEP 0x1
+#define GDBSTUB_SINGLE_STEP 0x2
+
 /* Include definitions for instructions classes and implementations flags */
 //#define DO_SINGLE_STEP
 //#define PPC_DEBUG_DISAS
@@ -2803,8 +2806,17 @@ static always_inline void gen_goto_tb (D
         else
 #endif
             gen_op_b_T1();
-        if (ctx->singlestep_enabled)
-            gen_op_debug();
+       if (unlikely(ctx->singlestep_enabled)) {
+           if (ctx->singlestep_enabled & GDBSTUB_SINGLE_STEP) {
+               gen_update_nip(ctx, dest);
+               gen_op_debug();
+           } else {
+               target_ulong tmp = ctx->nip;
+               ctx->nip = dest;
+               GEN_EXCP(ctx, POWERPC_EXCP_TRACE, 0);
+               ctx->nip = tmp;
+           }
+        }
         tcg_gen_exit_tb(0);
     }
 }
@@ -2985,8 +2997,10 @@ static always_inline void gen_bcond (Dis
 #endif
             gen_op_btest_T1(ctx->nip);
     no_test:
-        if (ctx->singlestep_enabled)
+        if (ctx->singlestep_enabled & GDBSTUB_SINGLE_STEP) {
+            gen_update_nip(ctx, ctx->nip);
             gen_op_debug();
+        }
         tcg_gen_exit_tb(0);
     }
  out:
@@ -6150,7 +6164,7 @@ static always_inline int gen_intermediat
     target_ulong pc_start;
     uint16_t *gen_opc_end;
     int supervisor, little_endian;
-    int single_step, branch_step;
+    int branch_step;
     int j, lj = -1;
 
     pc_start = tb->pc;
@@ -6184,14 +6198,15 @@ static always_inline int gen_intermediat
     else
         ctx.altivec_enabled = 0;
     if ((env->flags & POWERPC_FLAG_SE) && msr_se)
-        single_step = 1;
+        ctx.singlestep_enabled = CPU_SINGLE_STEP;
     else
-        single_step = 0;
+        ctx.singlestep_enabled = 0;
     if ((env->flags & POWERPC_FLAG_BE) && msr_be)
         branch_step = 1;
     else
         branch_step = 0;
-    ctx.singlestep_enabled = env->singlestep_enabled || single_step == 1;
+    if (unlikely(env->singlestep_enabled))
+        ctx.singlestep_enabled |= GDBSTUB_SINGLE_STEP;
 #if defined (DO_SINGLE_STEP) && 0
     /* Single step trace mode */
     msr_se = 1;
@@ -6287,11 +6302,11 @@ static always_inline int gen_intermediat
         if (unlikely(branch_step != 0 &&
                      ctx.exception == POWERPC_EXCP_BRANCH)) {
             GEN_EXCP(ctxp, POWERPC_EXCP_TRACE, 0);
-        } else if (unlikely(single_step != 0 &&
-                            (ctx.nip <= 0x100 || ctx.nip > 0xF00 ||
-                             (ctx.nip & 0xFC) != 0x04) &&
+        } else if (unlikely(ctx.singlestep_enabled & CPU_SINGLE_STEP &&
+                            (ctx.nip <= 0x100 || ctx.nip > 0xF00) &&
                             ctx.exception != POWERPC_SYSCALL &&
-                            ctx.exception != POWERPC_EXCP_TRAP)) {
+                            ctx.exception != POWERPC_EXCP_TRAP &&
+                            ctx.exception != POWERPC_EXCP_BRANCH)) {
             GEN_EXCP(ctxp, POWERPC_EXCP_TRACE, 0);
         } else if (unlikely(((ctx.nip & (TARGET_PAGE_SIZE - 1)) == 0) ||
                             (env->singlestep_enabled))) {
@@ -6307,6 +6322,10 @@ static always_inline int gen_intermediat
     if (ctx.exception == POWERPC_EXCP_NONE) {
         gen_goto_tb(&ctx, 0, ctx.nip);
     } else if (ctx.exception != POWERPC_EXCP_BRANCH) {
+        if (unlikely(ctx.singlestep_enabled & GDBSTUB_SINGLE_STEP)) {
+            gen_update_nip(&ctx, ctx.nip);
+            gen_op_debug();
+        }
         /* Generate the return instruction */
         tcg_gen_exit_tb(0);
     }

Re: [Qemu-devel] [PATCH] Fix broken PPC user space single stepping

[Aurelien Jarno]

On Thu, May 08, 2008 at 11:33:42PM -0500, Jason Wessel wrote:
> This is a refreshed patch from the prior version I submitted with an
> updated description of the problem of the PPC single stepping problem. 
> It also fixes the generator to correctly handle MSR_SE on branches.
> 
> Please consider applying it, or comment as to what further changes are
> needed and I will gladly make further changes.
> 
> Thanks,
> Jason.
> 

> From: Jason Wessel <jason.wessel@windriver.com>
> Subject: [PATCH] Fix ppc user space single stepping
> 
> User space OS single stepping will hang a qemu instance because the
> instruction translation executes an internal debug exception that is
> meant for the qemu backend debugger.  
> 
> The test case is to use any program which executes a ptrace attach to
> a process and then "ptrace(PTRACE_SINGLESTEP,pid,0,0);".  In general
> you could simply use gdb on the target and request it to "instruction
> step" a process with "si", so long as gdb is setup to not emulate
> single stepping with breakpoints.
> 
> This patch splits the run-time single stepping from the debugger stub
> single stepping such that each type of single stepping gets the
> correct exception generation.
> 
> This patch also fixes the defect where branches were incorrectly
> calculated for the update to the env->nip when setting MSR_SE on
> a branch instruction.

Please find my comments inside.

> 
> Signed-off-by: Jason Wessel <jason.wessel@windriver.com>
> 
> ---
>  target-ppc/translate.c |   41 ++++++++++++++++++++++++++++++-----------
>  1 file changed, 30 insertions(+), 11 deletions(-)
> 
> --- a/target-ppc/translate.c
> +++ b/target-ppc/translate.c
> @@ -29,6 +29,9 @@
>  #include "tcg-op.h"
>  #include "qemu-common.h"
>  
> +#define CPU_SINGLE_STEP 0x1
> +#define GDBSTUB_SINGLE_STEP 0x2
> +
>  /* Include definitions for instructions classes and implementations flags */
>  //#define DO_SINGLE_STEP
>  //#define PPC_DEBUG_DISAS
> @@ -2803,8 +2806,17 @@ static always_inline void gen_goto_tb (D
>          else
>  #endif
>              gen_op_b_T1();
> -        if (ctx->singlestep_enabled)
> -            gen_op_debug();
> +       if (unlikely(ctx->singlestep_enabled)) {
> +           if (ctx->singlestep_enabled & GDBSTUB_SINGLE_STEP) {
> +               gen_update_nip(ctx, dest);
> +               gen_op_debug();
> +           } else {
> +               target_ulong tmp = ctx->nip;
> +               ctx->nip = dest;
> +               GEN_EXCP(ctx, POWERPC_EXCP_TRACE, 0);
> +               ctx->nip = tmp;

What's the point of generating POWERPC_EXCP_TRACE in gen_goto_tb()?
ctx->singlestep_enabled == CPU_SINGLE_STEP corresponds to MSR_SE, which
does not generate trace exception for branch instructions.

MSR_BE does generate trace exception for branch instructions, but that's
already handled in gen_intermediate_code_internal() by testing
branch_step.

> +           }
> +        }
>          tcg_gen_exit_tb(0);
>      }
>  }
> @@ -2985,8 +2997,10 @@ static always_inline void gen_bcond (Dis
>  #endif
>              gen_op_btest_T1(ctx->nip);
>      no_test:
> -        if (ctx->singlestep_enabled)
> +        if (ctx->singlestep_enabled & GDBSTUB_SINGLE_STEP) {
> +            gen_update_nip(ctx, ctx->nip);
>              gen_op_debug();
> +        }
>          tcg_gen_exit_tb(0);
>      }
>   out:
> @@ -6150,7 +6164,7 @@ static always_inline int gen_intermediat
>      target_ulong pc_start;
>      uint16_t *gen_opc_end;
>      int supervisor, little_endian;
> -    int single_step, branch_step;
> +    int branch_step;
>      int j, lj = -1;
>  
>      pc_start = tb->pc;
> @@ -6184,14 +6198,15 @@ static always_inline int gen_intermediat
>      else
>          ctx.altivec_enabled = 0;
>      if ((env->flags & POWERPC_FLAG_SE) && msr_se)
> -        single_step = 1;
> +        ctx.singlestep_enabled = CPU_SINGLE_STEP;
>      else
> -        single_step = 0;
> +        ctx.singlestep_enabled = 0;
>      if ((env->flags & POWERPC_FLAG_BE) && msr_be)
>          branch_step = 1;
>      else
>          branch_step = 0;
> -    ctx.singlestep_enabled = env->singlestep_enabled || single_step == 1;
> +    if (unlikely(env->singlestep_enabled))
> +        ctx.singlestep_enabled |= GDBSTUB_SINGLE_STEP;
>  #if defined (DO_SINGLE_STEP) && 0
>      /* Single step trace mode */
>      msr_se = 1;
> @@ -6287,11 +6302,11 @@ static always_inline int gen_intermediat
>          if (unlikely(branch_step != 0 &&
>                       ctx.exception == POWERPC_EXCP_BRANCH)) {
>              GEN_EXCP(ctxp, POWERPC_EXCP_TRACE, 0);
> -        } else if (unlikely(single_step != 0 &&
> -                            (ctx.nip <= 0x100 || ctx.nip > 0xF00 ||
> -                             (ctx.nip & 0xFC) != 0x04) &&
> +        } else if (unlikely(ctx.singlestep_enabled & CPU_SINGLE_STEP &&
> +                            (ctx.nip <= 0x100 || ctx.nip > 0xF00) &&
>                              ctx.exception != POWERPC_SYSCALL &&
> -                            ctx.exception != POWERPC_EXCP_TRAP)) {
> +                            ctx.exception != POWERPC_EXCP_TRAP &&
> +                            ctx.exception != POWERPC_EXCP_BRANCH)) {
>              GEN_EXCP(ctxp, POWERPC_EXCP_TRACE, 0);
>          } else if (unlikely(((ctx.nip & (TARGET_PAGE_SIZE - 1)) == 0) ||
>                              (env->singlestep_enabled))) {
> @@ -6307,6 +6322,10 @@ static always_inline int gen_intermediat
>      if (ctx.exception == POWERPC_EXCP_NONE) {
>          gen_goto_tb(&ctx, 0, ctx.nip);
>      } else if (ctx.exception != POWERPC_EXCP_BRANCH) {
> +        if (unlikely(ctx.singlestep_enabled & GDBSTUB_SINGLE_STEP)) {
> +            gen_update_nip(&ctx, ctx.nip);
> +            gen_op_debug();
> +        }
>          /* Generate the return instruction */
>          tcg_gen_exit_tb(0);
>      }

Other parts of the patch looks ok.

-- 
  .''`.  Aurelien Jarno	            | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   aurel32@debian.org         | aurelien@aurel32.net
   `-    people.debian.org/~aurel32 | www.aurel32.net

Re: [Qemu-devel] [PATCH] Fix broken PPC user space single stepping

[Jason Wessel]

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

Aurelien Jarno wrote:
> On Thu, May 08, 2008 at 11:33:42PM -0500, Jason Wessel wrote:
>> This patch also fixes the defect where branches were incorrectly
>> calculated for the update to the env->nip when setting MSR_SE on
>> a branch instruction.
>
> Please find my comments inside.
>
>> ---
>>  target-ppc/translate.c |   41 ++++++++++++++++++++++++++++++-----------
>>  1 file changed, 30 insertions(+), 11 deletions(-)
>>
[clip]
>> @@ -2803,8 +2806,17 @@ static always_inline void gen_goto_tb (D
>>          else
>>  #endif
>>              gen_op_b_T1();
>> -        if (ctx->singlestep_enabled)
>> -            gen_op_debug();
>> +       if (unlikely(ctx->singlestep_enabled)) {
>> +           if (ctx->singlestep_enabled & GDBSTUB_SINGLE_STEP) {
>> +               gen_update_nip(ctx, dest);
>> +               gen_op_debug();
>> +           } else {
>> +               target_ulong tmp = ctx->nip;
>> +               ctx->nip = dest;
>> +               GEN_EXCP(ctx, POWERPC_EXCP_TRACE, 0);
>> +               ctx->nip = tmp;
>
> What's the point of generating POWERPC_EXCP_TRACE in gen_goto_tb()?
> ctx->singlestep_enabled == CPU_SINGLE_STEP corresponds to MSR_SE, which
> does not generate trace exception for branch instructions.
>
> MSR_BE does generate trace exception for branch instructions, but that's
> already handled in gen_intermediate_code_internal() by testing
> branch_step.

But in this case that is not the way the hardware works.  The linux
kernel is only setting up the MSR_SE bit to step the instructions in
user space for the PPC 6xx architecture.

The 604 processor manual indicates (as well as when I tested it on
real hardware) that the MSR_SE should generate a trace exception upon
the completion of any instruction except for the "special cases"
(see below).

You have certainly pointed out a different problem however.  I tested
the MSR_BE functionality and it does not work at all for the same
reason that the MSR_SE did not work with branches.  The exception is
generated in the wrong place, and the result is that the simulation
stops at the wrong instruction when MSR_BE is set.  To address this,
I created a new patch which is attached.  The new patch also fixes the
case for using the qemu backend debugger and the MSR_SE or MSR_BE on
the simulated cpu at the same time.

The reference here for MSR_SE the single stepping is from:

http://www-01.ibm.com/chips/techlib/techlib.nsf/techdocs/852569B20050FF7785256996006E34F3/$file/604eUM_book.pdf

--- from manual page 4-8 ---

The processor generates a single-step trace exception upon the
successful execution of the next instruction (unless that instruction
is an rfi instruction). Successful execution means that the
instruction caused no other exception.

-------

And then there is the general 32bit ppc arch manual.

http://www.freescale.com/files/product/doc/MPCFPE32B.pdf

In there it states that the MSR_SE is an optional feature  and that
you should see the specific ppc part documentation, but it
does further describe:

--- from manual page 6-29 ---

If this facility is implemented, a trace interrupt occurs when no
higher priority interrupt exists and either of the conditions
described above exist. The following are not traced:

     * rfi instruction
     * sc and trap instructions that trap
     * Other instructions that cause interrupts (other than trace
interrupts)
     * The first instruction of any interrupt handler
     * Instructions that are emulated by software

MSR[SE,BE] are both cleared when the trace interrupt is taken. In the
normal use of this function,

MSR[SE,BE] are restored when the interrupt handler returns to the
interrupted program using an rfi instruction.

------

At least with these changes you can now more accurately debug a user
space application with gdb running inside linux on the qemu-system-ppc
as well as freely use the qemu backend debugger with single stepping.

Jason.

ps

The "return;" just after the "out:"  is there to eliminate some compiler
warnings.  This could be fixed by further changing all the "goto out;"
statements with return;  I had to move the ctx->exception type update
prior to any calls to gen_goto_tb(), so the single step logic would
apply correctly only in the branch case and not for any other kind of
exception.  I figured I'd limit the change scope to make the patch as
readable as possible.


[-- Attachment #2: ppc_system_single_step.patch --]
[-- Type: text/x-patch, Size: 6241 bytes --]

From: Jason Wessel <jason.wessel@windriver.com>
Subject: [PATCH] Fix ppc user space single stepping

User space OS single stepping will hang a qemu instance because the
instruction translation executes an internal debug exception that is
meant for the qemu backend debugger.  

The test case is to use any program which executes a ptrace attach to
a process and then "ptrace(PTRACE_SINGLESTEP,pid,0,0);".  In general
you could simply use gdb on the target and request it to "instruction
step" a process with "si", so long as gdb is setup to not emulate
single stepping with breakpoints.

This patch splits the run-time single stepping from the debugger stub
single stepping such that each type of single stepping gets the
correct exception generation.

This patch also fixes the defect where branches were incorrectly
calculated for the update to the env->nip when setting MSR_SE or
MSR_BE on a branch instruction.

Signed-off-by: Jason Wessel <jason.wessel@windriver.com>

---
 target-ppc/translate.c |   60 +++++++++++++++++++++++++++++++------------------
 1 file changed, 39 insertions(+), 21 deletions(-)

--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -29,6 +29,10 @@
 #include "tcg-op.h"
 #include "qemu-common.h"
 
+#define CPU_SINGLE_STEP 0x1
+#define CPU_BRANCH_STEP 0x2
+#define GDBSTUB_SINGLE_STEP 0x4
+
 /* Include definitions for instructions classes and implementations flags */
 //#define DO_SINGLE_STEP
 //#define PPC_DEBUG_DISAS
@@ -2785,7 +2789,7 @@ static always_inline void gen_goto_tb (D
     TranslationBlock *tb;
     tb = ctx->tb;
     if ((tb->pc & TARGET_PAGE_MASK) == (dest & TARGET_PAGE_MASK) &&
-        !ctx->singlestep_enabled) {
+        likely(!ctx->singlestep_enabled)) {
         tcg_gen_goto_tb(n);
         gen_set_T1(dest);
 #if defined(TARGET_PPC64)
@@ -2803,8 +2807,20 @@ static always_inline void gen_goto_tb (D
         else
 #endif
             gen_op_b_T1();
-        if (ctx->singlestep_enabled)
-            gen_op_debug();
+        if (unlikely(ctx->singlestep_enabled)) {
+            if ((ctx->singlestep_enabled &
+                 (CPU_BRANCH_STEP | CPU_SINGLE_STEP)) &&
+                ctx->exception == POWERPC_EXCP_BRANCH) {
+                target_ulong tmp = ctx->nip;
+                ctx->nip = dest;
+                GEN_EXCP(ctx, POWERPC_EXCP_TRACE, 0);
+                ctx->nip = tmp;
+            }
+            if (ctx->singlestep_enabled & GDBSTUB_SINGLE_STEP) {
+                gen_update_nip(ctx, dest);
+                gen_op_debug();
+            }
+        }
         tcg_gen_exit_tb(0);
     }
 }
@@ -2824,6 +2840,7 @@ GEN_HANDLER(b, 0x12, 0xFF, 0xFF, 0x00000
 {
     target_ulong li, target;
 
+    ctx->exception = POWERPC_EXCP_BRANCH;
     /* sign extend LI */
 #if defined(TARGET_PPC64)
     if (ctx->sf_mode)
@@ -2842,7 +2859,6 @@ GEN_HANDLER(b, 0x12, 0xFF, 0xFF, 0x00000
     if (LK(ctx->opcode))
         gen_setlr(ctx, ctx->nip);
     gen_goto_tb(ctx, 0, target);
-    ctx->exception = POWERPC_EXCP_BRANCH;
 }
 
 #define BCOND_IM  0
@@ -2857,6 +2873,7 @@ static always_inline void gen_bcond (Dis
     uint32_t bi = BI(ctx->opcode);
     uint32_t mask;
 
+    ctx->exception = POWERPC_EXCP_BRANCH;
     if ((bo & 0x4) == 0)
         gen_op_dec_ctr();
     switch(type) {
@@ -2985,12 +3002,14 @@ static always_inline void gen_bcond (Dis
 #endif
             gen_op_btest_T1(ctx->nip);
     no_test:
-        if (ctx->singlestep_enabled)
+        if (ctx->singlestep_enabled & GDBSTUB_SINGLE_STEP) {
+            gen_update_nip(ctx, ctx->nip);
             gen_op_debug();
+        }
         tcg_gen_exit_tb(0);
     }
  out:
-    ctx->exception = POWERPC_EXCP_BRANCH;
+    return;
 }
 
 GEN_HANDLER(bc, 0x10, 0xFF, 0xFF, 0x00000000, PPC_FLOW)
@@ -6150,7 +6169,6 @@ static always_inline int gen_intermediat
     target_ulong pc_start;
     uint16_t *gen_opc_end;
     int supervisor, little_endian;
-    int single_step, branch_step;
     int j, lj = -1;
 
     pc_start = tb->pc;
@@ -6184,14 +6202,13 @@ static always_inline int gen_intermediat
     else
         ctx.altivec_enabled = 0;
     if ((env->flags & POWERPC_FLAG_SE) && msr_se)
-        single_step = 1;
+        ctx.singlestep_enabled = CPU_SINGLE_STEP;
     else
-        single_step = 0;
+        ctx.singlestep_enabled = 0;
     if ((env->flags & POWERPC_FLAG_BE) && msr_be)
-        branch_step = 1;
-    else
-        branch_step = 0;
-    ctx.singlestep_enabled = env->singlestep_enabled || single_step == 1;
+        ctx.singlestep_enabled |= CPU_BRANCH_STEP;
+    if (unlikely(env->singlestep_enabled))
+        ctx.singlestep_enabled |= GDBSTUB_SINGLE_STEP;
 #if defined (DO_SINGLE_STEP) && 0
     /* Single step trace mode */
     msr_se = 1;
@@ -6284,14 +6301,11 @@ static always_inline int gen_intermediat
         handler->count++;
 #endif
         /* Check trace mode exceptions */
-        if (unlikely(branch_step != 0 &&
-                     ctx.exception == POWERPC_EXCP_BRANCH)) {
-            GEN_EXCP(ctxp, POWERPC_EXCP_TRACE, 0);
-        } else if (unlikely(single_step != 0 &&
-                            (ctx.nip <= 0x100 || ctx.nip > 0xF00 ||
-                             (ctx.nip & 0xFC) != 0x04) &&
-                            ctx.exception != POWERPC_SYSCALL &&
-                            ctx.exception != POWERPC_EXCP_TRAP)) {
+        if (unlikely(ctx.singlestep_enabled & CPU_SINGLE_STEP &&
+                     (ctx.nip <= 0x100 || ctx.nip > 0xF00) &&
+                     ctx.exception != POWERPC_SYSCALL &&
+                     ctx.exception != POWERPC_EXCP_TRAP &&
+                     ctx.exception != POWERPC_EXCP_BRANCH)) {
             GEN_EXCP(ctxp, POWERPC_EXCP_TRACE, 0);
         } else if (unlikely(((ctx.nip & (TARGET_PAGE_SIZE - 1)) == 0) ||
                             (env->singlestep_enabled))) {
@@ -6307,6 +6321,10 @@ static always_inline int gen_intermediat
     if (ctx.exception == POWERPC_EXCP_NONE) {
         gen_goto_tb(&ctx, 0, ctx.nip);
     } else if (ctx.exception != POWERPC_EXCP_BRANCH) {
+        if (unlikely(env->singlestep_enabled)) {
+            gen_update_nip(&ctx, ctx.nip);
+            gen_op_debug();
+        }
         /* Generate the return instruction */
         tcg_gen_exit_tb(0);
     }