From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47310) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bkPyg-0004Ge-5l for qemu-devel@nongnu.org; Thu, 15 Sep 2016 02:16:55 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bkPyb-0001KM-MB for qemu-devel@nongnu.org; Thu, 15 Sep 2016 02:16:53 -0400 Date: Thu, 15 Sep 2016 16:16:08 +1000 From: David Gibson Message-ID: <20160915061608.GR15077@voom.fritz.box> References: <1473832442-17762-1-git-send-email-nikunj@linux.vnet.ibm.com> <1473832442-17762-3-git-send-email-nikunj@linux.vnet.ibm.com> <20160915002035.GD15077@voom.fritz.box> <8760pxzon4.fsf@abhimanyu.i-did-not-set--mail-host-address--so-tickle-me> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="173oMW6+eDUtB8Ng" Content-Disposition: inline In-Reply-To: <8760pxzon4.fsf@abhimanyu.i-did-not-set--mail-host-address--so-tickle-me> Subject: Re: [Qemu-devel] [PATCH v4 2/3] target-ppc: add flag in chech_tlb_flush() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nikunj A Dadhania Cc: qemu-ppc@nongnu.org, benh@kernel.crashing.org, alex.bennee@linaro.org, qemu-devel@nongnu.org, rth@twiddle.net --173oMW6+eDUtB8Ng Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Sep 15, 2016 at 11:32:39AM +0530, Nikunj A Dadhania wrote: > David Gibson 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. > >>=20 > >> 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 architectural= ly > >> 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. > > > >>=20 > >> Signed-off-by: Nikunj A Dadhania > >> --- >=20 > >> 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) >=20 > 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. >=20 > 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. >=20 > 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 globa= l) > { > TCGv_i32 t; > TCGLabel *l; > @@ -3078,12 +3078,13 @@ static inline void gen_check_tlb_flush(DisasConte= xt *ctx) > t =3D 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); > } >=20 >=20 > Regards > Nikunj >=20 --=20 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 --173oMW6+eDUtB8Ng Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIcBAEBAgAGBQJX2jyoAAoJEGw4ysog2bOSYSsP/A7WLPpSVJhDK+UuTT3c3Lzz 4F3E6F+pPBaot922hNhqM9u6qZFZYsq6ip6A+kilOCniJ5V6EDmey+EoBo6zEgcA vfF9bQxc2KaPEyRtP7PSL2eBTgNr4NtAf/koJCWDERONTYWkQ3i5FAUvTgA38bEe J0XMs72PA39HPr521VADKhRKKuyICgmfOX0d+d1Q/DY1g3gTHHFOnoFPcUWmVroR JUL3ncLNY4G/Hgyacz9/O7v2bF9daAzlwRjKtUSNLzmOW/IYhCH+v8jat5A1qMbD IGIEcbLB6+2OoSrYZVPIVxwXqWHnc8cWwgIKOYuqonNrDPFylkYEhCZrTgpUlONM 51h1A23nFpen0rhtp/G5OM+jYUu05ktLgsGE+QUFnAZkhquLM0hW0izK+iXsBYrW Ha+0AifMmJlDNwbQUvNS48EKjB7HRQQcQnuVZYvIV/0qYtJxv1O+doxIPUBxTcZz OuRdMPqN2u/pQhwRTpfYqPVkh4JS/7s0zBiTUuxnYp62pUpMw/7oKuRZ/uixbli1 X2CY9dKgPTwSVT4JcL0ocKOkNBMRWHLXMwFScl53loWSA0XlyFRzhIlxthTDY/jf DAH7NStxQucXkzk2r8afeyp5NUARxI7h/XIaqGC9rnXAqv83DNijcr6bPxJo3lCs tPaNtRf9yrjIiFPeR1SM =pZ4D -----END PGP SIGNATURE----- --173oMW6+eDUtB8Ng--