On Thu, Sep 15, 2016 at 11:32:39AM +0530, Nikunj A Dadhania wrote: > 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.
I think target_ulong would make more sense.
> >> -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.
As below, I'd prefer not. Actually I hadn't thought through the TCG
helper constraints, so I think having it target_ulong in the helper
and bool in the direct function makes sense.
> > 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":
Oh, I see. Hmm. I don't know if that will make a real difference in
TCG or not.
> -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
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
