[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 2/3] target-ppc: add flag in chech_tlb_flush(
From: |
David Gibson |
Subject: |
Re: [Qemu-devel] [PATCH v4 2/3] target-ppc: add flag in chech_tlb_flush() |
Date: |
Thu, 15 Sep 2016 16:16:08 +1000 |
User-agent: |
Mutt/1.7.0 (2016-08-17) |
On Thu, Sep 15, 2016 at 11:32:39AM +0530, Nikunj A Dadhania wrote:
> David Gibson <address@hidden> 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 <address@hidden>
> >> ---
>
> >> 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
[Qemu-devel] [PATCH v4 3/3] target-ppc: tlbie/tlbivax should have global effect, Nikunj A Dadhania, 2016/09/14