Re: [Qemu-devel] TCG problem with cpu_{st,ld}x_data ?

[Richard Henderson]

On 07/25/2016 10:45 AM, Benjamin Herrenschmidt wrote:
> On Mon, 2016-07-25 at 06:04 +0530, Richard Henderson wrote:
> > I noticed a related problem recently, while working on the cmpxchg patch set.
> >
> > In my opinion, we should (1) merge GETRA and GETPC so there's no confusion
> > between the two, (2) push all adjustment down to the final moment before use,
> > perhaps in cpu_restore_state.
> >
> > Thus a null value would be properly retained until checked, and one can easily
> > call the memory helper functions without confusion.
>
> Ok, after a bit more scrubbing of the code I think I understand what you
> mean.
>
> Now assuming we fix that, there is still a problem if the target code, such
> as the PPC code, calls a helper that might cause a fault without first
> updating the PC in the env, right ? IE. On powerpc for example, that means
> that any instruction using a helper that might potentially do loads or stores
> needs to first call gen_update_nip().

Well, that's the bug with the current code, yes.

But what we gain by transforming this code to use (via indirection) 
cpu_loop_exit_restore, is the ability to reconstruct values, like NIP, without 
first having saved them.

Any data that you give to tcg_gen_insn_start will be given to 
restore_state_to_opc.  PPC currently does

    tcg_gen_insn_start(ctx.nip);

and

    env->nip = data[0];

so you're good there.

For some targets, we also restore part of the flags computation with this 
mechanism.  With more trickery, ARM is (intending to?) compute exception 
syndrome information with this.  As I understand it, this is very much akin to 
the PPC gen_set_access_type, so perhaps in future there's some savings to be 
had there.

> Either that, or change the helpers to capture the PC using GETPC/GETRA from
> the first level of helper function (so as to ensure the return address is
> correct).
>
> Am I right ?

You are right.

> IE. Even if we fix the 0 vs. -2 problem, I still need this patch:
>
> --- a/target-ppc/translate.c
> +++ b/target-ppc/translate.c
> @@ -6916,6 +6916,7 @@ static void gen_lve##name(DisasContext *ctx)              
>               \
>          if (size > 1) {                                                 \
>              tcg_gen_andi_tl(EA, EA, ~(size - 1));                       \
>          }                                                               \
> +        gen_update_nip(ctx, ctx->nip - 4);                              \
>          rs = gen_avr_ptr(rS(ctx->opcode));                              \
>          gen_helper_lve##name(cpu_env, rs, EA);                          \
>          tcg_temp_free(EA);                                              \
> @@ -6937,6 +6938,7 @@ static void gen_stve##name(DisasContext *ctx)             
>               \
>          if (size > 1) {                                                 \
>              tcg_gen_andi_tl(EA, EA, ~(size - 1));                       \
>          }                                                               \
> +        gen_update_nip(ctx, ctx->nip - 4);                              \
>          rs = gen_avr_ptr(rS(ctx->opcode));                              \
>          gen_helper_stve##name(cpu_env, rs, EA);                         \
>          tcg_temp_free(EA);                                              \
>
> (And possibly others I haven't yet audited)

Yes indeed.  I'm amused by this, since I read your emails in the wrong order, 
and so discovered exactly this problem while answering the other email.


r~