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

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

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:

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

Cheers,
Ben.