| Message ID | 87k267myv1.fsf@abhimanyu.i-did-not-set--mail-host-address--so-tickle-me (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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~
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