David Gibson <da...@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 <nik...@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
