TCG problem with cpu_{st,ld}x_data ?

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

(The same way we seem to do before generating other exceptions such as traps,
well provided we do it correctly everywhere, I need to double check).

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 ?

IE. Even if we fix the 0 vs. -2 problem, I still need this patch:


(And possibly others I haven't yet audited)

Cheers,
Ben.

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~

On Mon, 2016-07-25 at 19:42 +0530, Richard Henderson wrote:
> 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

Indeed. Another issue we have is generating the opcode bits in DSISR.
Now thankfully for most modern CPUs this is no longer done in HW but at
the moment, I dont like how we call cpu_ldl_code() in powerpc_excp.

Cheers,
Ben.