[Qemu-devel] PowerPC code generation and the program counter

[Stu Grossman]

I've been using qemu-12.4 to trace accesses to non-existent addresses, but I've
found that the PC is incorrect when cpu_abort() is called from within the
unassigned memory helper routines (unassigned_mem_read[bwl] and
unassigned_mem_write[bwl]).  Even nearby instructions (plus or minus 10
instructions or so) don't match the type of load or store being done, so this
isn't a PC being current_instr+4 kind of thing.

I ended up modifying the GEN_LD* and GEN_ST* macros (in target-ppc/translate.c)
to call gen_update_nip(ctx, ctx->nip - 4).  This fixed the above problem, which
has helped enormously.

Since I'm not a qemu expert, I was wondering about several things:

        1) Was it really necessary to add gen_update_nip to the load and store
           instructions in order to get the correct PC?  Could the correct PC
           have been derived some other way, without a runtime cost for all
           basic loads and stores?
        2) As the current code lacks that fix, the basic load and store
           instructions will save an incorrect PC if an exception occurs.  If
           so, how come nobody noticed this before?  I think that exceptions
           would have srr0 pointing at the last instruction which called
           gen_update_nip.  So when the target returns from a data exception,
           it might re-execute some instructions.  Possibly harmless, but could
           lead to subtle bugs...

        Thanks, Stu

Here's the patch if anybody is interested:

*** translate.c~        Sat Sep 11 23:43:25 2010
--- translate.c Sun Sep 12 20:49:53 2010
***************
*** 2549,2554 ****
--- 2549,2555 ----
  {
         \
      TCGv EA;
         \
      gen_set_access_type(ctx, ACCESS_INT);
         \
+     gen_update_nip(ctx, ctx->nip - 4);
               \
      EA = tcg_temp_new();
         \
      gen_addr_imm_index(ctx, EA, 0);
         \
      gen_qemu_##ldop(ctx, cpu_gpr[rD(ctx->opcode)], EA);
         \
***************
*** 2564,2569 ****
--- 2565,2571 ----
          gen_inval_exception(ctx, POWERPC_EXCP_INVAL_INVAL);
         \
          return;
         \
      }
         \
+     gen_update_nip(ctx, ctx->nip - 4);
               \
      gen_set_access_type(ctx, ACCESS_INT);
         \
      EA = tcg_temp_new();
         \
      if (type == PPC_64B)
         \
***************
*** 2584,2589 ****
--- 2586,2592 ----
          gen_inval_exception(ctx, POWERPC_EXCP_INVAL_INVAL);
         \
          return;
         \
      }
         \
+     gen_update_nip(ctx, ctx->nip - 4);
               \
      gen_set_access_type(ctx, ACCESS_INT);
         \
      EA = tcg_temp_new();
         \
      gen_addr_reg_index(ctx, EA);
         \
***************
*** 2596,2601 ****
--- 2599,2605 ----
  static void glue(gen_, name##x)(DisasContext *ctx)
         \
  {
         \
      TCGv EA;
         \
+     gen_update_nip(ctx, ctx->nip - 4);
               \
      gen_set_access_type(ctx, ACCESS_INT);
         \
      EA = tcg_temp_new();
         \
      gen_addr_reg_index(ctx, EA);
         \
***************
*** 2693,2698 ****
--- 2697,2703 ----
  static void glue(gen_, name)(DisasContext *ctx)
                 \
  {
         \
      TCGv EA;
         \
+     gen_update_nip(ctx, ctx->nip - 4);
               \
      gen_set_access_type(ctx, ACCESS_INT);
         \
      EA = tcg_temp_new();
         \
      gen_addr_imm_index(ctx, EA, 0);
         \
***************
*** 2708,2713 ****
--- 2713,2719 ----
          gen_inval_exception(ctx, POWERPC_EXCP_INVAL_INVAL);
         \
          return;
         \
      }
         \
+     gen_update_nip(ctx, ctx->nip - 4);
               \
      gen_set_access_type(ctx, ACCESS_INT);
         \
      EA = tcg_temp_new();
         \
      if (type == PPC_64B)
         \
***************
*** 2727,2732 ****
--- 2733,2739 ----
          gen_inval_exception(ctx, POWERPC_EXCP_INVAL_INVAL);
         \
          return;
         \
      }
         \
+     gen_update_nip(ctx, ctx->nip - 4);
               \
      gen_set_access_type(ctx, ACCESS_INT);
         \
      EA = tcg_temp_new();
         \
      gen_addr_reg_index(ctx, EA);
         \
***************
*** 2739,2744 ****
--- 2746,2752 ----
  static void glue(gen_, name##x)(DisasContext *ctx)
                 \
  {
         \
      TCGv EA;
         \
+     gen_update_nip(ctx, ctx->nip - 4);
               \
      gen_set_access_type(ctx, ACCESS_INT);
         \
      EA = tcg_temp_new();
         \
      gen_addr_reg_index(ctx, EA);
         \