1. Patchew
  2. QEMU
  3. View series

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

Richard Henderson posted 1 patch 6 years, 12 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20170425104338.31984-1-rth@twiddle.net
Test checkpatch passed
Test docker passed
Test s390x passed
tcg/tcg-op.c | 6 ++++++
1 file changed, 6 insertions(+)
[Qemu-devel] [PATCH] tcg: Initialize return value after exit_atomic
Posted by Richard Henderson 6 years, 12 months ago 
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 <nikunj@linux.vnet.ibm.com>
Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 tcg/tcg-op.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tcg/tcg-op.c b/tcg/tcg-op.c
index 95a39b7..6b1f415 100644
--- a/tcg/tcg-op.c
+++ b/tcg/tcg-op.c
@@ -2861,6 +2861,9 @@ void tcg_gen_atomic_cmpxchg_i64(TCGv_i64 retv, TCGv addr, TCGv_i64 cmpv,
 #endif
 #else
         gen_helper_exit_atomic(tcg_ctx.tcg_env);
+        /* Produce a result, so that we have a well-formed opcode stream
+           with respect to uses of the result in the (dead) code following.  */
+        tcg_gen_movi_i64(retv, 0);
 #endif /* CONFIG_ATOMIC64 */
     } else {
         TCGv_i32 c32 = tcg_temp_new_i32();
@@ -2966,6 +2969,9 @@ static void do_atomic_op_i64(TCGv_i64 ret, TCGv addr, TCGv_i64 val,
 #endif
 #else
         gen_helper_exit_atomic(tcg_ctx.tcg_env);
+        /* Produce a result, so that we have a well-formed opcode stream
+           with respect to uses of the result in the (dead) code following.  */
+        tcg_gen_movi_i64(ret, 0);
 #endif /* CONFIG_ATOMIC64 */
     } else {
         TCGv_i32 v32 = tcg_temp_new_i32();
-- 
2.9.3
Re: [Qemu-devel] [PATCH] tcg: Initialize return value after exit_atomic
Posted by Nikunj A Dadhania 6 years, 12 months ago 
Richard Henderson <rth@twiddle.net> 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 <nikunj@linux.vnet.ibm.com>
> Signed-off-by: Richard Henderson <rth@twiddle.net>

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.

So "make check" still fails at powernv serial test.

./configure --target-list=ppc64-softmmu  --cc=clang --host-cc=clang && make && make check

> ---
>  tcg/tcg-op.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/tcg/tcg-op.c b/tcg/tcg-op.c
> index 95a39b7..6b1f415 100644
> --- a/tcg/tcg-op.c
> +++ b/tcg/tcg-op.c
> @@ -2861,6 +2861,9 @@ void tcg_gen_atomic_cmpxchg_i64(TCGv_i64 retv, TCGv addr, TCGv_i64 cmpv,
>  #endif
>  #else
>          gen_helper_exit_atomic(tcg_ctx.tcg_env);
> +        /* Produce a result, so that we have a well-formed opcode stream
> +           with respect to uses of the result in the (dead) code following.  */
> +        tcg_gen_movi_i64(retv, 0);
>  #endif /* CONFIG_ATOMIC64 */
>      } else {
>          TCGv_i32 c32 = tcg_temp_new_i32();
> @@ -2966,6 +2969,9 @@ static void do_atomic_op_i64(TCGv_i64 ret, TCGv addr, TCGv_i64 val,
>  #endif
>  #else
>          gen_helper_exit_atomic(tcg_ctx.tcg_env);
> +        /* Produce a result, so that we have a well-formed opcode stream
> +           with respect to uses of the result in the (dead) code following.  */
> +        tcg_gen_movi_i64(ret, 0);
>  #endif /* CONFIG_ATOMIC64 */
>      } else {
>          TCGv_i32 v32 = tcg_temp_new_i32();
> -- 

Regards,
Nikunj

1. https://github.com/open-power/skiboot/blob/master/asm/lock.S#L36
Re: [Qemu-devel] [PATCH] tcg: Initialize return value after exit_atomic
Posted by Richard Henderson 6 years, 12 months ago 
On 04/25/2017 01:21 PM, Nikunj A Dadhania wrote:
> Richard Henderson <rth@twiddle.net> 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 <nikunj@linux.vnet.ibm.com>
>> Signed-off-by: Richard Henderson <rth@twiddle.net>
> 
> 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.  If that's the case, then we have 
another bug in the !parallel_cpus code path for gen_conditional_store.


r~
Re: [Qemu-devel] [PATCH] tcg: Initialize return value after exit_atomic
Posted by aNikunj A Dadhania 6 years, 12 months ago 
Richard Henderson <rth@twiddle.net> writes:

> On 04/25/2017 01:21 PM, Nikunj A Dadhania wrote:
>> Richard Henderson <rth@twiddle.net> 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 <nikunj@linux.vnet.ibm.com>
>>> Signed-off-by: Richard Henderson <rth@twiddle.net>
>> 
>> 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
Re: [Qemu-devel] [PATCH] tcg: Initialize return value after exit_atomic
Posted by Nikunj A Dadhania 6 years, 12 months ago 
aNikunj A Dadhania <nikunj@linux.vnet.ibm.com> writes:

> Richard Henderson <rth@twiddle.net> writes:
>
>> On 04/25/2017 01:21 PM, Nikunj A Dadhania wrote:
>>> Richard Henderson <rth@twiddle.net> 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 <nikunj@linux.vnet.ibm.com>
>>>> Signed-off-by: Richard Henderson <rth@twiddle.net>
>>> 
>>> 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 ] ****
>

That is because of the following:

tcg_gen_atomic_cmpxchg_tl()
 -> gen_helper_exit_atomic()
     -> HELPER(exit_atomic)
         -> cpu_loop_exit_atomic() -> EXCP_ATOMIC
             -> qemu_tcg_cpu_thread_fn() => case EXCP_ATOMIC
                 -> cpu_exec_step_atomic()
                     -> cpu_step_atomic()
                         -> cc->cpu_exec_enter() = ppc_cpu_exec_enter()
                            Sets env->reserve_addr = -1;

So when we re-enter back, reserve_addr is rubbed out. After getting rid
of this reset of reserve_addr, I do get ahead a bit and stdcx is
successful.


[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

[stdcx.]

    helper_myprint: t0 dead0000 t1 dead0000
    helper_myprint: t0 30200018 t1 0
    helper_myprint: t0 dead0001 t1 dead0001

[ re-enters ]

    helper_myprint: t0 dead0000 t1 dead0000

[ cpu_reserve valud is intact now ]

    helper_myprint: t0 30200018 t1 0
    helper_myprint: t0 dead0001 t1 dead0001
    helper_myprint: t0 dead0011 t1 dead0011
    helper_myprint: t0 0 t1 0

[ And there is a match ]

But then the code is getting stuck here.

Regards
Nikunj
Re: [Qemu-devel] [PATCH] tcg: Initialize return value after exit_atomic
Posted by Nikunj A Dadhania 6 years, 12 months ago 
Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> writes:

> aNikunj A Dadhania <nikunj@linux.vnet.ibm.com> writes:
>
>> Richard Henderson <rth@twiddle.net> writes:
>>
>>> On 04/25/2017 01:21 PM, Nikunj A Dadhania wrote:
>>>> Richard Henderson <rth@twiddle.net> 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 <nikunj@linux.vnet.ibm.com>
>>>>> Signed-off-by: Richard Henderson <rth@twiddle.net>
>>>> 
>>>> 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 ] ****
>>
>
> That is because of the following:
>
> tcg_gen_atomic_cmpxchg_tl()
>  -> gen_helper_exit_atomic()
>      -> HELPER(exit_atomic)
>          -> cpu_loop_exit_atomic() -> EXCP_ATOMIC
>              -> qemu_tcg_cpu_thread_fn() => case EXCP_ATOMIC
>                  -> cpu_exec_step_atomic()
>                      -> cpu_step_atomic()
>                          -> cc->cpu_exec_enter() = ppc_cpu_exec_enter()
>                             Sets env->reserve_addr = -1;
>
> So when we re-enter back, reserve_addr is rubbed out. After getting rid
> of this reset of reserve_addr, I do get ahead a bit and stdcx is
> successful.
>
>
> [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
>
> [stdcx.]
>
>     helper_myprint: t0 dead0000 t1 dead0000
>     helper_myprint: t0 30200018 t1 0
>     helper_myprint: t0 dead0001 t1 dead0001
>
> [ re-enters ]
>
>     helper_myprint: t0 dead0000 t1 dead0000
>
> [ cpu_reserve valud is intact now ]
>
>     helper_myprint: t0 30200018 t1 0
>     helper_myprint: t0 dead0001 t1 dead0001
>     helper_myprint: t0 dead0011 t1 dead0011
>     helper_myprint: t0 0 t1 0
>
> [ And there is a match ]
>
> But then the code is getting stuck here.

Ok, that was due to some debug code that I had added in skiboot. I
confirm that with this patch and the below change in
target/ppc/translate_init.c, I am able pass powernv boot serial test.

diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
index 77e5463..4eb0219 100644
--- a/target/ppc/translate_init.c
+++ b/target/ppc/translate_init.c
@@ -10428,10 +10428,6 @@ static bool ppc_cpu_has_work(CPUState *cs)
 
 static void ppc_cpu_exec_enter(CPUState *cs)
 {
-    PowerPCCPU *cpu = POWERPC_CPU(cs);
-    CPUPPCState *env = &cpu->env;
-
-    env->reserve_addr = -1;
 }
 
 /* CPUClass::reset() */



I will need to see the implication of this in other context.

Regards,
Nikunj
Re: [Qemu-devel] [PATCH] tcg: Initialize return value after exit_atomic
Posted by Richard Henderson 6 years, 12 months ago 
On 04/26/2017 12:40 PM, Nikunj A Dadhania wrote:
>                           -> cc->cpu_exec_enter() = ppc_cpu_exec_enter()
>                              Sets env->reserve_addr = -1;

This is the bug.  We should be doing this in powerpc_excp instead.

I'm quite frankly surprised that we don't see this same failure with 
-singlestep, but I guess we usually stay in the cpu loop long enough.


r~
Re: [Qemu-devel] [PATCH] tcg: Initialize return value after exit_atomic
Posted by Nikunj A Dadhania 6 years, 12 months ago 
Richard Henderson <rth@twiddle.net> 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 <nikunj@linux.vnet.ibm.com>
> Signed-off-by: Richard Henderson <rth@twiddle.net>

Tested-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>

> ---
>  tcg/tcg-op.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/tcg/tcg-op.c b/tcg/tcg-op.c
> index 95a39b7..6b1f415 100644
> --- a/tcg/tcg-op.c
> +++ b/tcg/tcg-op.c
> @@ -2861,6 +2861,9 @@ void tcg_gen_atomic_cmpxchg_i64(TCGv_i64 retv, TCGv addr, TCGv_i64 cmpv,
>  #endif
>  #else
>          gen_helper_exit_atomic(tcg_ctx.tcg_env);
> +        /* Produce a result, so that we have a well-formed opcode stream
> +           with respect to uses of the result in the (dead) code following.  */
> +        tcg_gen_movi_i64(retv, 0);
>  #endif /* CONFIG_ATOMIC64 */
>      } else {
>          TCGv_i32 c32 = tcg_temp_new_i32();
> @@ -2966,6 +2969,9 @@ static void do_atomic_op_i64(TCGv_i64 ret, TCGv addr, TCGv_i64 val,
>  #endif
>  #else
>          gen_helper_exit_atomic(tcg_ctx.tcg_env);
> +        /* Produce a result, so that we have a well-formed opcode stream
> +           with respect to uses of the result in the (dead) code following.  */
> +        tcg_gen_movi_i64(ret, 0);
>  #endif /* CONFIG_ATOMIC64 */
>      } else {
>          TCGv_i32 v32 = tcg_temp_new_i32();
> -- 
> 2.9.3
Re: [Qemu-devel] [PATCH] tcg: Initialize return value after exit_atomic
Posted by Peter Maydell 6 years, 12 months ago 
On 25 April 2017 at 11:43, Richard Henderson <rth@twiddle.net> wrote:
> 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 <nikunj@linux.vnet.ibm.com>
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
>  tcg/tcg-op.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/tcg/tcg-op.c b/tcg/tcg-op.c
> index 95a39b7..6b1f415 100644
> --- a/tcg/tcg-op.c
> +++ b/tcg/tcg-op.c
> @@ -2861,6 +2861,9 @@ void tcg_gen_atomic_cmpxchg_i64(TCGv_i64 retv, TCGv addr, TCGv_i64 cmpv,
>  #endif
>  #else
>          gen_helper_exit_atomic(tcg_ctx.tcg_env);
> +        /* Produce a result, so that we have a well-formed opcode stream
> +           with respect to uses of the result in the (dead) code following.  */
> +        tcg_gen_movi_i64(retv, 0);
>  #endif /* CONFIG_ATOMIC64 */
>      } else {
>          TCGv_i32 c32 = tcg_temp_new_i32();
> @@ -2966,6 +2969,9 @@ static void do_atomic_op_i64(TCGv_i64 ret, TCGv addr, TCGv_i64 val,
>  #endif
>  #else
>          gen_helper_exit_atomic(tcg_ctx.tcg_env);
> +        /* Produce a result, so that we have a well-formed opcode stream
> +           with respect to uses of the result in the (dead) code following.  */
> +        tcg_gen_movi_i64(ret, 0);
>  #endif /* CONFIG_ATOMIC64 */
>      } else {
>          TCGv_i32 v32 = tcg_temp_new_i32();
> --

Tested-by: Peter Maydell <peter.maydell@linaro.org>

Without this patch an AArch64 QEMU crashes on startup if I build it
with clang and with optimization enabled. We should probably get this
into master sooner rather than later...

thanks
-- PMM

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.