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

[Blue Swirl]

On Mon, Sep 13, 2010 at 4:51 AM, Stu Grossman <address@hidden> wrote:
> 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.

If PC is not updated to match the value at the access instruction, it
will point to the last instruction that did update PC, or start of the
translation block (TB).

> 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?

This is the way used by Sparc. There save_state() updates PC, NPC and
forces lazy flag calculation.

It may be possible to avoid updating the state, if TB generation was
limited to allow only one instruction which can update the state per
TB. But shorter TBs will also decrease performance, so the trade-off
should be evaluated.

>        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...

Yes. Also, page fault handlers are not interested in the exact
location, only the page. Because we ensure that TBs will never cross
page boundaries, the page will be correct.

>
>        Thanks, Stu
>
> Here's the patch if anybody is interested:

Please resubmit with git send-email, with Signed-off-by line and a
short description. I think it should be applied.

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