From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33315) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bmJZI-0004Yb-0t for qemu-devel@nongnu.org; Tue, 20 Sep 2016 07:50:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bmJZD-0001qk-ID for qemu-devel@nongnu.org; Tue, 20 Sep 2016 07:50:31 -0400 Date: Tue, 20 Sep 2016 17:43:20 +1000 From: David Gibson Message-ID: <20160920074320.GB20488@umbus> References: <1474266132-5397-1-git-send-email-nikunj@linux.vnet.ibm.com> <1474266132-5397-3-git-send-email-nikunj@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="q/d9vTEvvdeKbPNw" Content-Disposition: inline In-Reply-To: <1474266132-5397-3-git-send-email-nikunj@linux.vnet.ibm.com> Subject: Re: [Qemu-devel] [PATCH v5 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 --q/d9vTEvvdeKbPNw Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Sep 19, 2016 at 11:52:11AM +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 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. >=20 > Signed-off-by: Nikunj A Dadhania Sorry, there's still a couple of changes I'd like to this. > --- > hw/ppc/spapr_hcall.c | 4 ++-- > target-ppc/excp_helper.c | 4 ++-- > target-ppc/helper.h | 2 +- > target-ppc/helper_regs.h | 4 ++-- > target-ppc/mmu_helper.c | 4 ++-- > target-ppc/translate.c | 20 ++++++++++---------- > 6 files changed, 19 insertions(+), 19 deletions(-) >=20 > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c > index 73af112..ef12ea0 100644 > --- a/hw/ppc/spapr_hcall.c > +++ b/hw/ppc/spapr_hcall.c > @@ -201,7 +201,7 @@ static target_ulong h_remove(PowerPCCPU *cpu, sPAPRMa= chineState *spapr, > =20 > switch (ret) { > case REMOVE_SUCCESS: > - check_tlb_flush(env); > + check_tlb_flush(env, 1); > return H_SUCCESS; > =20 > case REMOVE_NOT_FOUND: > @@ -282,7 +282,7 @@ static target_ulong h_bulk_remove(PowerPCCPU *cpu, sP= APRMachineState *spapr, > } > } > exit: > - check_tlb_flush(env); > + check_tlb_flush(env, 1); > =20 > return rc; > } > diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c > index 04ed4da..3b78126 100644 > --- a/target-ppc/excp_helper.c > +++ b/target-ppc/excp_helper.c > @@ -711,7 +711,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int = excp_model, int excp) > /* Any interrupt is context synchronizing, check if TCG TLB > * needs a delayed flush on ppc64 > */ > - check_tlb_flush(env); > + check_tlb_flush(env, 0); > } > =20 > void ppc_cpu_do_interrupt(CPUState *cs) > @@ -973,7 +973,7 @@ static inline void do_rfi(CPUPPCState *env, target_ul= ong nip, target_ulong msr) > cs->interrupt_request |=3D CPU_INTERRUPT_EXITTB; > =20 > /* Context synchronizing: check if TCG TLB needs flush */ > - check_tlb_flush(env); > + check_tlb_flush(env, 0); > } > =20 > void helper_rfi(CPUPPCState *env) > 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) > #endif > =20 > DEF_HELPER_3(lmw, void, env, tl, i32) > diff --git a/target-ppc/helper_regs.h b/target-ppc/helper_regs.h > index 69204a5..bcf65ce 100644 > --- a/target-ppc/helper_regs.h > +++ b/target-ppc/helper_regs.h > @@ -154,7 +154,7 @@ static inline int hreg_store_msr(CPUPPCState *env, ta= rget_ulong value, > } > =20 > #if !defined(CONFIG_USER_ONLY) > -static inline void check_tlb_flush(CPUPPCState *env) > +static inline void check_tlb_flush(CPUPPCState *env, uint32_t global) Please use a bool here.. > { > CPUState *cs =3D CPU(ppc_env_get_cpu(env)); > if (env->tlb_need_flush & TLB_NEED_LOCAL_FLUSH) { > @@ -163,7 +163,7 @@ static inline void check_tlb_flush(CPUPPCState *env) > } > } > #else > -static inline void check_tlb_flush(CPUPPCState *env) { } > +static inline void check_tlb_flush(CPUPPCState *env, uint32_t global) { } > #endif > =20 > #endif /* HELPER_REGS_H */ > diff --git a/target-ppc/mmu_helper.c b/target-ppc/mmu_helper.c > index d59d2f8..bf9f329 100644 > --- a/target-ppc/mmu_helper.c > +++ b/target-ppc/mmu_helper.c > @@ -2867,9 +2867,9 @@ void helper_booke206_tlbflush(CPUPPCState *env, tar= get_ulong type) > } > =20 > =20 > -void helper_check_tlb_flush(CPUPPCState *env) > +void helper_check_tlb_flush(CPUPPCState *env, unsigned int global) > { > - check_tlb_flush(env); > + check_tlb_flush(env, global); =2E.which you can convert to from the i32 here. Also.. your helper definition says i32, and I'm not sure that's guaranteed to match unsigned int on all platforms. I think you want an explicit uint32_t here. Or.. you could have separate tlb_flush_local() and tlb_flush_global helpers= =2E. > } > =20 > /***********************************************************************= ******/ > diff --git a/target-ppc/translate.c b/target-ppc/translate.c > index e747c1f..1ed5868 100644 > --- a/target-ppc/translate.c > +++ b/target-ppc/translate.c > @@ -3022,7 +3022,7 @@ static void gen_eieio(DisasContext *ctx) > } > =20 > #if !defined(CONFIG_USER_ONLY) > -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; > @@ -3034,12 +3034,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); =2E. which would save you a TCG op here. > + gen_helper_check_tlb_flush(cpu_env, t); > gen_set_label(l); > tcg_temp_free_i32(t); > } > #else > -static inline void gen_check_tlb_flush(DisasContext *ctx) { } > +static inline void gen_check_tlb_flush(DisasContext *ctx, uint32_t globa= l) { } > #endif > =20 > /* isync */ > @@ -3050,7 +3051,7 @@ static void gen_isync(DisasContext *ctx) > * kernel mode however so check MSR_PR > */ > if (!ctx->pr) { > - gen_check_tlb_flush(ctx); > + gen_check_tlb_flush(ctx, 0); > } > gen_stop_exception(ctx); > } > @@ -3230,7 +3231,7 @@ static void gen_sync(DisasContext *ctx) > * check MSR_PR as well. > */ > if (((l =3D=3D 2) || !(ctx->insns_flags & PPC_64B)) && !ctx->pr) { > - gen_check_tlb_flush(ctx); > + gen_check_tlb_flush(ctx, 1); > } > } > =20 > @@ -4439,11 +4440,10 @@ static void gen_tlbsync(DisasContext *ctx) > #else > CHK_HV; > =20 > - /* tlbsync is a nop for server, ptesync handles delayed tlb flush, > - * embedded however needs to deal with tlbsync. We don't try to be > - * fancy and swallow the overhead of checking for both. > - */ > - gen_check_tlb_flush(ctx); > + /* BookS does both ptesync and tlbsync make tlbsync a nop for server= */ > + if (ctx->insns_flags & PPC_BOOKE) { > + gen_check_tlb_flush(ctx, 1); > + } > #endif /* defined(CONFIG_USER_ONLY) */ > } > =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 --q/d9vTEvvdeKbPNw Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJX4OiVAAoJEGw4ysog2bOSfzUQAK6J39563h+gkAdpqCHgODwB rbELyu2RGLOErOHxH9ac1sur5Ry0lX8g4qa6uYvhVXpX2x+b0s5FOmGoYpBZJE2o MltuMbP2LMNM1SGoFI9HBxQ/HFRcs2PGcIU29m3ClH/vtHFSgP2gDX7Ch7ke2uia XVG/esOBYm1cvwa87GDjWdFr8jh/jmUW5wXhaeG/wV9+tJBL+U+j9jruFfsntHQY +mtTOcAFs+W3MXtyjYZ1zy/tFw7dCm/3ywb4dn3arxW0ZCDtGH5tzByTuIiQp7CT beoG+kb5wjIleZ9Rne3wKRTnauAtzgSHfkBwiTRCyRjk77mt2ZlaG29dbNVgHsO4 sLY8uSp24kTG143EMEKg2utGfJs4vrfpF3Ocmxlmc0Jv1l/woL/RNbX3rSoHBpDh 4d0Ffa0bczKYZzH7mb07Zh5WMl47SwZCpaBWyI5eGJqF95CnPxeXSHro77B4baGL 4/k4otR6efb98//xOCRu5QawSV8jQD2UvS6NuizxtxgjcDw9hAZWSGpR1L9BbSgU /v9JCzat740j07dDVNU1wVK/MSRL8DeFVR6NInlez0MQOMCWDS/+tncRnFGVCZa+ 9UgPMha1UPFzBo6VcH1WAWNUBLnC/T3mE7BAEadzOXJZwKojqUVKQoeiHdXG3yIh 9UlmGPZdoGZlFKvMDXhr =m5pN -----END PGP SIGNATURE----- --q/d9vTEvvdeKbPNw--