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

* [Qemu-devel] PowerPC code generation and the program counter
@ 2010-09-13  4:51 Stu Grossman
  2010-09-13  7:53 ` Alexander Graf
  2010-09-14 16:10 ` Blue Swirl
  0 siblings, 2 replies; 6+ messages in thread
From: Stu Grossman @ 2010-09-13  4:51 UTC (permalink / raw)
  To: Qemu-devel

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

^ permalink raw reply	[flat|nested] 6+ messages in thread