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 <address@hidden>
> 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 <address@hidden>
> 
> ---
>  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
 `. `'   address@hidden         | address@hidden
   `-    people.debian.org/~aurel32 | www.aurel32.net