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