* [Qemu-devel] [PATCH] tcg: Initialize return value after exit_atomic
@ 2017-04-25 10:43 Richard Henderson
2017-04-25 11:21 ` Nikunj A Dadhania
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Richard Henderson @ 2017-04-25 10:43 UTC (permalink / raw)
To: qemu-devel; +Cc: Nikunj A Dadhania
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] tcg: Initialize return value after exit_atomic
2017-04-25 10:43 [Qemu-devel] [PATCH] tcg: Initialize return value after exit_atomic Richard Henderson
@ 2017-04-25 11:21 ` Nikunj A Dadhania
2017-04-25 11:25 ` Richard Henderson
2017-04-26 11:38 ` Nikunj A Dadhania
2017-04-26 17:20 ` Peter Maydell
2 siblings, 1 reply; 9+ messages in thread
From: Nikunj A Dadhania @ 2017-04-25 11:21 UTC (permalink / raw)
To: Richard Henderson, qemu-devel; +Cc: David Gibson
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] tcg: Initialize return value after exit_atomic
2017-04-25 11:21 ` Nikunj A Dadhania
@ 2017-04-25 11:25 ` Richard Henderson
2017-04-26 7:06 ` aNikunj A Dadhania
0 siblings, 1 reply; 9+ messages in thread
From: Richard Henderson @ 2017-04-25 11:25 UTC (permalink / raw)
To: Nikunj A Dadhania, qemu-devel; +Cc: David Gibson
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~
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] tcg: Initialize return value after exit_atomic
2017-04-25 11:25 ` Richard Henderson
@ 2017-04-26 7:06 ` aNikunj A Dadhania
2017-04-26 10:40 ` Nikunj A Dadhania
0 siblings, 1 reply; 9+ messages in thread
From: aNikunj A Dadhania @ 2017-04-26 7:06 UTC (permalink / raw)
To: Richard Henderson, qemu-devel; +Cc: David Gibson
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] tcg: Initialize return value after exit_atomic
2017-04-26 7:06 ` aNikunj A Dadhania
@ 2017-04-26 10:40 ` Nikunj A Dadhania
2017-04-26 11:23 ` Nikunj A Dadhania
2017-04-26 11:41 ` Richard Henderson
0 siblings, 2 replies; 9+ messages in thread
From: Nikunj A Dadhania @ 2017-04-26 10:40 UTC (permalink / raw)
To: Richard Henderson, qemu-devel; +Cc: David Gibson
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] tcg: Initialize return value after exit_atomic
2017-04-26 10:40 ` Nikunj A Dadhania
@ 2017-04-26 11:23 ` Nikunj A Dadhania
2017-04-26 11:41 ` Richard Henderson
1 sibling, 0 replies; 9+ messages in thread
From: Nikunj A Dadhania @ 2017-04-26 11:23 UTC (permalink / raw)
To: Richard Henderson, qemu-devel; +Cc: David Gibson
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] tcg: Initialize return value after exit_atomic
2017-04-25 10:43 [Qemu-devel] [PATCH] tcg: Initialize return value after exit_atomic Richard Henderson
2017-04-25 11:21 ` Nikunj A Dadhania
@ 2017-04-26 11:38 ` Nikunj A Dadhania
2017-04-26 17:20 ` Peter Maydell
2 siblings, 0 replies; 9+ messages in thread
From: Nikunj A Dadhania @ 2017-04-26 11:38 UTC (permalink / raw)
To: Richard Henderson, qemu-devel; +Cc: David Gibson
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] tcg: Initialize return value after exit_atomic
2017-04-26 10:40 ` Nikunj A Dadhania
2017-04-26 11:23 ` Nikunj A Dadhania
@ 2017-04-26 11:41 ` Richard Henderson
1 sibling, 0 replies; 9+ messages in thread
From: Richard Henderson @ 2017-04-26 11:41 UTC (permalink / raw)
To: Nikunj A Dadhania, qemu-devel; +Cc: David Gibson
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~
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH] tcg: Initialize return value after exit_atomic
2017-04-25 10:43 [Qemu-devel] [PATCH] tcg: Initialize return value after exit_atomic Richard Henderson
2017-04-25 11:21 ` Nikunj A Dadhania
2017-04-26 11:38 ` Nikunj A Dadhania
@ 2017-04-26 17:20 ` Peter Maydell
2 siblings, 0 replies; 9+ messages in thread
From: Peter Maydell @ 2017-04-26 17:20 UTC (permalink / raw)
To: Richard Henderson; +Cc: QEMU Developers, Nikunj A Dadhania
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-04-26 17:21 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-25 10:43 [Qemu-devel] [PATCH] tcg: Initialize return value after exit_atomic Richard Henderson
2017-04-25 11:21 ` Nikunj A Dadhania
2017-04-25 11:25 ` Richard Henderson
2017-04-26 7:06 ` aNikunj A Dadhania
2017-04-26 10:40 ` Nikunj A Dadhania
2017-04-26 11:23 ` Nikunj A Dadhania
2017-04-26 11:41 ` Richard Henderson
2017-04-26 11:38 ` Nikunj A Dadhania
2017-04-26 17:20 ` Peter Maydell
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.