Re: [Qemu-devel] [PATCH] tcg: Initialize return value after exit_atomic

[aNikunj A Dadhania]

Richard Henderson <address@hidden> writes:

> On 04/25/2017 01:21 PM, Nikunj A Dadhania wrote:
>> Richard Henderson <address@hidden> writes:
>> 
>>> Users of tcg_gen_atomic_cmpxchg and do_atomic_op rightfully utilize
>>> the output.  Even though this code is dead, it gets translated, and
>>> without the initialization we encounter a tcg_error.
>>>
>>> Reported-by: Nikunj A Dadhania <address@hidden>
>>> Signed-off-by: Richard Henderson <address@hidden>
>> 
>> With this the tcg_error goes away.
>> 
>> But then powernv skiboot code [1] enters into infinite loop. Basically,
>> in target/ppc/translate.c:gen_conditional_store(), setcond_tl will
>> always fail, and CRF_EQ_BIT will never be set, the lock will never be
>> taken.
>
> The setcond_tl *shouldn't* always fail.

Correct, in fact we never get here it.

> If that's the case, then we have another bug in the !parallel_cpus
> code path for gen_conditional_store.

Something interesting is happening, I have instrumented the code such
that I get some prints for load with reservation and store conditional:

First case is the success case for 32bit atomic_cmpxchg.

    $ ./configure --target-list=ppc64-softmmu  --cc=clang --host-cc=clang
    $ ./ppc64-softmmu/qemu-system-ppc64  -machine powernv,usb=off  -vga none 
-nographic
[lwarx]
    helper_myprint: t0 cafe0000 t1 cafe0000
    helper_myprint: t0 cafe0001 t1 cafe0001
    helper_myprint: t0 cafe0002 t1 cafe0002
    helper_myprint: t0 f0 t1 0
[stwcx]
    helper_myprint: t0 dead0000 t1 dead0000
    helper_myprint: t0 f0 t1 0
    helper_myprint: t0 dead0001 t1 dead0001
    helper_myprint: t0 dead0011 t1 dead0011
    helper_myprint: t0 0 t1 0
[success as t0 and cpu_reserve_val is same]

[ldarx]
    helper_myprint: t0 cafe0000 t1 cafe0000
    helper_myprint: t0 cafe0001 t1 cafe0001
    helper_myprint: t0 cafe0002 t1 cafe0002
    helper_myprint: t0 30200018 t1 0

[ cpu_reserve = 30200018, cpu_reserve_val = 0 after load ]

[stdcx]
    helper_myprint: t0 dead0000 t1 dead0000
    helper_myprint: t0 30200018 t1 0
    helper_myprint: t0 dead0001 t1 dead0001

[ That is before atomic_cmpxchg_tl, and suddenly we exit out, we did
not reach setcond_tl ]

    helper_myprint: t0 dead0000 t1 dead0000

**** [ re-entering gen_store_conditional() ] ****

    helper_myprint: t0 ffffffffffffffff t1 0

**** [ cpu_reserve is corrupted ] ****

    helper_myprint: t0 dead0020 t1 dead0020

[ Exit as cpu_reserve_val and EA does not match]

    helper_myprint: t0 cafe0000 t1 cafe0000
    helper_myprint: t0 cafe0001 t1 cafe0001
    helper_myprint: t0 cafe0002 t1 cafe0002
    helper_myprint: t0 30200018 t1 0
    helper_myprint: t0 dead0000 t1 dead0000
    helper_myprint: t0 30200018 t1 0
    helper_myprint: t0 dead0001 t1 dead0001
    helper_myprint: t0 dead0000 t1 dead0000
    helper_myprint: t0 ffffffffffffffff t1 0
    helper_myprint: t0 dead0020 t1 dead0020

[ Same thing repeats again and again ]

    helper_myprint: t0 cafe0000 t1 cafe0000
    helper_myprint: t0 cafe0001 t1 cafe0001
    helper_myprint: t0 cafe0002 t1 cafe0002
    helper_myprint: t0 30200018 t1 0
    helper_myprint: t0 dead0000 t1 dead0000
    helper_myprint: t0 30200018 t1 0
    helper_myprint: t0 dead0001 t1 dead0001
    helper_myprint: t0 dead0000 t1 dead0000
    helper_myprint: t0 ffffffffffffffff t1 0
    helper_myprint: t0 dead0020 t1 dead0020
[...]


Regards,
Nikunj



diff --git a/target/ppc/helper.h b/target/ppc/helper.h
index bb6a94a..afbb901 100644
--- a/target/ppc/helper.h
+++ b/target/ppc/helper.h
@@ -795,3 +795,5 @@ DEF_HELPER_4(dscliq, void, env, fprp, fprp, i32)
 
 DEF_HELPER_1(tbegin, void, env)
 DEF_HELPER_FLAGS_1(fixup_thrm, TCG_CALL_NO_RWG, void, env)
+
+DEF_HELPER_2(myprint, void, tl, tl)
diff --git a/target/ppc/int_helper.c b/target/ppc/int_helper.c
index da4e1a6..f555cb9 100644
--- a/target/ppc/int_helper.c
+++ b/target/ppc/int_helper.c
@@ -3521,3 +3521,8 @@ target_ulong helper_dlmzb(CPUPPCState *env, target_ulong 
high,
     }
     return i;
 }
+
+void helper_myprint(target_ulong t0, target_ulong t1)
+{
+    fprintf(stderr, "%s: t0 %lx t1 %lx\n", __func__, t0, t1);
+}
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 4a1f24a..363369e 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -3020,10 +3020,16 @@ static void gen_##name(DisasContext *ctx)               
             \
 {                                                                    \
     TCGv t0;                                                         \
     TCGv gpr = cpu_gpr[rD(ctx->opcode)];                             \
+    TCGv my;                                                         \
+    my = tcg_temp_local_new();                                       \
+    tcg_gen_movi_tl(my, 0xCAFE0000);                                 \
+    gen_helper_myprint(my, my);                                      \
     int len = MEMOP_GET_SIZE(memop);                                 \
     gen_set_access_type(ctx, ACCESS_RES);                            \
     t0 = tcg_temp_local_new();                                       \
     gen_addr_reg_index(ctx, t0);                                     \
+    tcg_gen_addi_tl(my, my, 1);                                      \
+    gen_helper_myprint(my, my);                                      \
     if ((len) > 1) {                                                 \
         gen_check_align(ctx, t0, (len)-1);                           \
     }                                                                \
@@ -3031,6 +3037,10 @@ static void gen_##name(DisasContext *ctx)                
            \
     tcg_gen_mov_tl(cpu_reserve, t0);                                 \
     tcg_gen_mov_tl(cpu_reserve_val, gpr);                            \
     tcg_gen_mb(TCG_MO_ALL | TCG_BAR_LDAQ);                           \
+    tcg_gen_addi_tl(my, my, 1);                                      \
+    gen_helper_myprint(my, my);                                      \
+    gen_helper_myprint(cpu_reserve, cpu_reserve_val);                \
+    tcg_temp_free(my);                                               \
     tcg_temp_free(t0);                                               \
 }
 
@@ -3165,13 +3175,23 @@ static void gen_conditional_store(DisasContext *ctx, 
TCGv EA,
     TCGLabel *l1 = gen_new_label();
     TCGLabel *l2 = gen_new_label();
     TCGv t0;
+    TCGv my;
 
+    my = tcg_temp_local_new();
+    tcg_gen_movi_tl(my, 0xDEAD0000);
+    gen_helper_myprint(my, my);
+    gen_helper_myprint(cpu_reserve, cpu_reserve_val);
     tcg_gen_brcond_tl(TCG_COND_NE, EA, cpu_reserve, l1);
 
+    tcg_gen_addi_tl(my, my, 1);
+    gen_helper_myprint(my, my);
     t0 = tcg_temp_new();
     tcg_gen_atomic_cmpxchg_tl(t0, cpu_reserve, cpu_reserve_val,
                               cpu_gpr[reg], ctx->mem_idx,
                               DEF_MEMOP(memop) | MO_ALIGN);
+    tcg_gen_addi_tl(my, my, 0x10);
+    gen_helper_myprint(my, my);
+    gen_helper_myprint(t0, cpu_reserve_val);
     tcg_gen_setcond_tl(TCG_COND_EQ, t0, t0, cpu_reserve_val);
     tcg_gen_shli_tl(t0, t0, CRF_EQ_BIT);
     tcg_gen_or_tl(t0, t0, cpu_so);
@@ -3180,6 +3200,8 @@ static void gen_conditional_store(DisasContext *ctx, TCGv 
EA,
     tcg_gen_br(l2);
 
     gen_set_label(l1);
+    tcg_gen_addi_tl(my, my, 0x20);
+    gen_helper_myprint(my, my);
 
     /* Address mismatch implies failure.  But we still need to provide the
        memory barrier semantics of the instruction.  */
@@ -3188,6 +3210,7 @@ static void gen_conditional_store(DisasContext *ctx, TCGv 
EA,
 
     gen_set_label(l2);
     tcg_gen_movi_tl(cpu_reserve, -1);
+    tcg_temp_free(my);
 }
 #endif