Re: [Qemu-devel] [PATCH v4 2/3] target-ppc: add flag in chech_tlb_flush()

[Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>]

David Gibson <david@gibson.dropbear.id.au> writes:
> [ Unknown signature status ]
> On Wed, Sep 14, 2016 at 11:24:01AM +0530, Nikunj A Dadhania wrote:
>> We flush the qemu TLB lazily. check_tlb_flush is called whenever we hit
>> a context synchronizing event or instruction that requires a pending
>> flush to be performed.
>> 
>> However, we fail to handle broadcast TLB flush operations. In order to
>> fix that efficiently, we want to differenciate whether check_tlb_flush()
>> needs to only apply pending local flushes (isync instructions,
>> interrupts, ...) or also global pending flush operations. The latter is
>> only needed when executing instructions that are defined architecturally
>> as synchronizing global TLB flush operations. This in our case is
>> ptesync on BookS and tlbsync on BookE along with the paravirtualized
>> hypervisor calls.
>
> Much better description, thank you.
>
>> 
>> Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
>> ---

>> diff --git a/target-ppc/helper.h b/target-ppc/helper.h
>> index e75d070..5ececf1 100644
>> --- a/target-ppc/helper.h
>> +++ b/target-ppc/helper.h
>> @@ -18,7 +18,7 @@ DEF_HELPER_1(rfid, void, env)
>>  DEF_HELPER_1(hrfid, void, env)
>>  DEF_HELPER_2(store_lpcr, void, env, tl)
>>  #endif
>> -DEF_HELPER_1(check_tlb_flush, void, env)
>> +DEF_HELPER_2(check_tlb_flush, void, env, i32)

Not sure if I can use bool here, maybe I can use target_ulong.

>> -void helper_check_tlb_flush(CPUPPCState *env)
>> +void helper_check_tlb_flush(CPUPPCState *env, unsigned int global)
>
> You're using an unsigned int for the flag here, but uint32_t for
> check_tlb_flush(), which is a needless inconsistency.

I can make this as uint32_t for consistency.

> You might as well make them both bools, since that's how it's actually
> being used.
>
> As a general rule don't use fixed width types unless you
> actually *need* the fixed width - the type choices are part of the
> interface documentation and using a fixed width type when you don't
> need it sends a misleading message.

I optimized it because to avoid a new variable, and re-used "t":

-static inline void gen_check_tlb_flush(DisasContext *ctx)
+static inline void gen_check_tlb_flush(DisasContext *ctx, uint32_t global)
 {
     TCGv_i32 t;
     TCGLabel *l;
@@ -3078,12 +3078,13 @@ static inline void gen_check_tlb_flush(DisasContext *ctx)
     t = tcg_temp_new_i32();
     tcg_gen_ld_i32(t, cpu_env, offsetof(CPUPPCState, tlb_need_flush));
     tcg_gen_brcondi_i32(TCG_COND_EQ, t, 0, l);
-    gen_helper_check_tlb_flush(cpu_env);
+    tcg_gen_movi_i32(t, global);
+    gen_helper_check_tlb_flush(cpu_env, t);
     gen_set_label(l);
     tcg_temp_free_i32(t);
 }


Regards
Nikunj