From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37080) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b9oy9-0007Y2-U9 for qemu-devel@nongnu.org; Mon, 06 Jun 2016 03:29:12 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b9oy1-0002zt-TU for qemu-devel@nongnu.org; Mon, 06 Jun 2016 03:29:04 -0400 Received: from 9.mo69.mail-out.ovh.net ([46.105.56.78]:34837) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b9oy1-0002zi-JT for qemu-devel@nongnu.org; Mon, 06 Jun 2016 03:28:57 -0400 Received: from player798.ha.ovh.net (b7.ovh.net [213.186.33.57]) by mo69.mail-out.ovh.net (Postfix) with ESMTP id 717F2FFC3F9 for ; Mon, 6 Jun 2016 09:28:54 +0200 (CEST) References: <1464955880-10176-1-git-send-email-clg@kaod.org> <57518BA1.1030400@ilande.co.uk> <5754645E.8080504@kaod.org> <1465186627.4274.30.camel@kernel.crashing.org> From: =?UTF-8?Q?C=c3=a9dric_Le_Goater?= Message-ID: <57552630.9050605@kaod.org> Date: Mon, 6 Jun 2016 09:28:48 +0200 MIME-Version: 1.0 In-Reply-To: <1465186627.4274.30.camel@kernel.crashing.org> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [Qemu-ppc] [PATCH 0/3] ppc: complete the new HV mode List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Benjamin Herrenschmidt , Mark Cave-Ayland , David Gibson Cc: qemu-ppc@nongnu.org, qemu-devel@nongnu.org On 06/06/2016 06:17 AM, Benjamin Herrenschmidt wrote: > On Sun, 2016-06-05 at 19:41 +0200, C=C3=A9dric Le Goater wrote: >> =20 >> Here is a fix I think. Could you give it a try ?=20 >=20 > This is somewhat wrong... >=20 >> commit cd0c6f473532 ('ppc: Do some batching of TCG tlb flushes') >> introduced an optimisation to flush TLBs only when a context >> synchronizing event is reached (interrupt, rfi). This was done for >> ppc64 but 32bit was forgotten on the way. >=20 > No it didn't. That commit only delays flushes on ppc64. ppc32 is > unaffected, unless I missed something. IE. It will delay flushes caused > by slb instructions (which don't exist on 32-bit) > and ppc_tlb_invalidate_one() only in the 64-bit cases. >=20 > Also what your patch does in practice is not really change that, though > you seem to try to somewhat extend the batching to 32-bit (but > incompletely), you also introduce something which effectively reverts > part of 9fb044911444fdd09f5f072ad0ca269d7f8b841d (split I/D mode). >=20 > I think that's more what's "fixing" your problem, ie, the flush in > IR/DR changes. However it shouldn't be needed. OK. I thought that was needed because of what the 32b specs say in=20 "Synchronization Requirements for Special Registers and for Lookaside=20 Buffers", a "Context-synchronizing instruction" is required after a=20 mtmsr({I,D}R) and also because changing IR can break implicit branching. But I might just misunderstand all of it as I am discovering. > I suspect all of that is papering over another bug somewhere else which > got exposed by the split I/D mode, since we no longer over-flush on > transitions to/from real-mode. So we must be missing flushes elsewhere, > possibly some G3 specific stuff, or there always was some kind of bug > in the TLB flushing on 32-bit that got somewhat masked by the over- > flushing we used to do. >>From what I see, darwin loops on tlbie : 0x000952fc: mtctr r0 0x00095300: tlbie r6 0x00095304: addi r6,r6,4096 0x00095308: bdnz+ 0x95300 0x0009530c: mtcrf 128,r11 0x00095310: sync =20 0x00095314: eieio 0x00095318: bns- 0x95328 and this is done on the G4, but not necessarily on the G3, it depends on r11 which contains some bits of SPRG2 : 0x0009531c: tlbsync 0x00095320: sync =20 0x00095324: isync HID0 is also read and written to but to control cache bits. > I need a repro-case. Booting the darwin CD is enough. Cheers, C.=20 =20 > Cheers, > Ben. >=20 >> Tested on mac99 and g3beige with >> >> qemu-system-ppc -cdrom darwinppc-602.cdr -boot d >> >> Signed-off-by: C=C3=A9dric Le Goater >> --- >> >> I think the hunk in powerpc_excp() is needed if we don't generate a >> context synchronizing event. what is best to do ? >> >> target-ppc/cpu.h | 2 +- >> target-ppc/excp_helper.c | 10 ++++++++++ >> target-ppc/helper_regs.h | 9 ++++++++- >> target-ppc/translate.c | 2 +- >> 4 files changed, 20 insertions(+), 3 deletions(-) >> >> Index: qemu-dgibson-for-2.7.git/target-ppc/translate.c >> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >> --- qemu-dgibson-for-2.7.git.orig/target-ppc/translate.c >> +++ qemu-dgibson-for-2.7.git/target-ppc/translate.c >> @@ -3290,7 +3290,7 @@ static void gen_eieio(DisasContext *ctx) >> { >> } >> =20 >> -#if !defined(CONFIG_USER_ONLY) && defined(TARGET_PPC64) >> +#if !defined(CONFIG_USER_ONLY) >> static inline void gen_check_tlb_flush(DisasContext *ctx) >> { >> TCGv_i32 t =3D tcg_temp_new_i32(); >> Index: qemu-dgibson-for-2.7.git/target-ppc/cpu.h >> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >> --- qemu-dgibson-for-2.7.git.orig/target-ppc/cpu.h >> +++ qemu-dgibson-for-2.7.git/target-ppc/cpu.h >> @@ -958,9 +958,9 @@ struct CPUPPCState { >> /* PowerPC 64 SLB area */ >> ppc_slb_t slb[MAX_SLB_ENTRIES]; >> int32_t slb_nr; >> +#endif >> /* tcg TLB needs flush (deferred slb inval instruction >> typically) */ >> uint32_t tlb_need_flush; >> -#endif >> /* segment registers */ >> hwaddr htab_base; >> /* mask used to normalize hash value to PTEG index */ >> Index: qemu-dgibson-for-2.7.git/target-ppc/helper_regs.h >> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >> --- qemu-dgibson-for-2.7.git.orig/target-ppc/helper_regs.h >> +++ qemu-dgibson-for-2.7.git/target-ppc/helper_regs.h >> @@ -121,6 +121,13 @@ static inline int hreg_store_msr(CPUPPCS >> } >> if (((value >> MSR_IR) & 1) !=3D msr_ir || >> ((value >> MSR_DR) & 1) !=3D msr_dr) { >> + /* A change of the instruction relocation bit in the MSR can >> + * cause an implicit branch in the address space. This >> + * requires a tlb flush. >> + */ >> + if (env->mmu_model & POWERPC_MMU_32B) { >> + env->tlb_need_flush =3D 1; >> + } >> cs->interrupt_request |=3D CPU_INTERRUPT_EXITTB; >> } >> if ((env->mmu_model & POWERPC_MMU_BOOKE) && >> @@ -151,7 +158,7 @@ static inline int hreg_store_msr(CPUPPCS >> return excp; >> } >> =20 >> -#if !defined(CONFIG_USER_ONLY) && defined(TARGET_PPC64) >> +#if !defined(CONFIG_USER_ONLY) >> static inline void check_tlb_flush(CPUPPCState *env) >> { >> CPUState *cs =3D CPU(ppc_env_get_cpu(env)); >> Index: qemu-dgibson-for-2.7.git/target-ppc/excp_helper.c >> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >> --- qemu-dgibson-for-2.7.git.orig/target-ppc/excp_helper.c >> +++ qemu-dgibson-for-2.7.git/target-ppc/excp_helper.c >> @@ -709,6 +709,16 @@ static inline void powerpc_excp(PowerPCC >> } >> } >> #endif >> + if (((new_msr >> MSR_IR) & 1) !=3D msr_ir || >> + ((new_msr >> MSR_DR) & 1) !=3D msr_dr) { >> + /* A change of the instruction relocation bit in the MSR can >> + * cause an implicit branch in the address space. This >> + * requires a tlb flush. >> + */ >> + if (env->mmu_model & POWERPC_MMU_32B) { >> + env->tlb_need_flush =3D 1; >> + } >> + } >> /* We don't use hreg_store_msr here as already have treated >> * any special case that could occur. Just store MSR and update >> hflags >> * >>