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

[Nikunj A Dadhania]

Nikunj A Dadhania <address@hidden> writes:

> aNikunj A Dadhania <address@hidden> writes:
>
>> 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 ] ****
>>
>
> 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