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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3503 bytes --]

On Thu, Sep 15, 2016 at 11:32:39AM +0530, Nikunj A Dadhania wrote:
> 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.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]