From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59852) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a11pr-0001MB-IJ for qemu-devel@nongnu.org; Mon, 23 Nov 2015 19:51:56 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a11po-0008TX-7a for qemu-devel@nongnu.org; Mon, 23 Nov 2015 19:51:55 -0500 Message-ID: <1448326304.4574.29.camel@kernel.crashing.org> From: Benjamin Herrenschmidt Date: Tue, 24 Nov 2015 11:51:44 +1100 In-Reply-To: <20151120074526.GB7118@voom.redhat.com> References: <1447201710-10229-1-git-send-email-benh@kernel.crashing.org> <1447201710-10229-22-git-send-email-benh@kernel.crashing.org> <20151120074526.GB7118@voom.redhat.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [Qemu-ppc] [PATCH 21/77] ppc: Rework generation of priv and inval interrupts List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson Cc: qemu-ppc@nongnu.org, qemu-devel@nongnu.org On Fri, 2015-11-20 at 18:45 +1100, David Gibson wrote: > snip] > >=C2=A0 /* tlbiel */ > >=C2=A0 static void gen_tlbiel(DisasContext *ctx) > >=C2=A0 { > >=C2=A0 #if defined(CONFIG_USER_ONLY) > > -=C2=A0=C2=A0=C2=A0 gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC); > > +=C2=A0=C2=A0=C2=A0 GEN_PRIV; > >=C2=A0 #else > > -=C2=A0=C2=A0=C2=A0 if (unlikely(ctx->pr || !ctx->hv)) { > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 gen_inval_exception(ctx, = POWERPC_EXCP_PRIV_OPC); > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return; > > -=C2=A0=C2=A0=C2=A0 } > > +=C2=A0=C2=A0=C2=A0 CHK_SV; >=20 > You have CHK_SV here, but the original code checks for HV, as does > your new code for tlbia and tlbiel, is that right? Yes. tlbiel is supervisor accessible (for weird reasons). > [snip] > >=C2=A0 /* tlbsync */ > >=C2=A0 static void gen_tlbsync(DisasContext *ctx) > >=C2=A0 { > >=C2=A0 #if defined(CONFIG_USER_ONLY) > > -=C2=A0=C2=A0=C2=A0 gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC); > > -#else > > -=C2=A0=C2=A0=C2=A0 if (unlikely(ctx->pr)) { > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 gen_inval_exception(ctx, = POWERPC_EXCP_PRIV_OPC); > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return; > > -=C2=A0=C2=A0=C2=A0 } > > +=C2=A0=C2=A0=C2=A0 GEN_PRIV; > > +#else=C2=A0=C2=A0=C2=A0=20 > > +=C2=A0=C2=A0=C2=A0 CHK_HV; > > + >=20 > Old code didn't check for HV, mode, but AFAICT it should have, so > this looks correct. Yes, this is a hypervisor instruction. > [snip] > > @@ -5941,18 +5921,16 @@ static void gen_mfapidi(DisasContext *ctx) > >=C2=A0 static void gen_tlbiva(DisasContext *ctx) > >=C2=A0 { > >=C2=A0 #if defined(CONFIG_USER_ONLY) > > -=C2=A0=C2=A0=C2=A0 gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC); > > +=C2=A0=C2=A0=C2=A0 GEN_PRIV; > >=C2=A0 #else > >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 TCGv t0; > > -=C2=A0=C2=A0=C2=A0 if (unlikely(ctx->pr)) { > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 gen_inval_exception(ctx, = POWERPC_EXCP_PRIV_OPC); > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return; > > -=C2=A0=C2=A0=C2=A0 } > > + > > +=C2=A0=C2=A0=C2=A0 CHK_SV; >=20 > Is the same thing as tlbivax, or some ancient instruction?=C2=A0 AFAICT > the ISA says tlbivax is hypervisor privileged. "tlbiva" is the 4xx variant, there is no hypervisor mode on these things. > >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 t0 =3D tcg_temp_new(); > >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 gen_addr_reg_index(ctx, t0); > >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 gen_helper_tlbie(cpu_env, cpu_gpr[rB(ct= x->opcode)]); > >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 tcg_temp_free(t0); > > -#endif > > +#endif /* defined(CONFIG_USER_ONLY) */ > >=C2=A0 } >=20 > [snip] > >=C2=A0 static void gen_tlbivax_booke206(DisasContext *ctx) > >=C2=A0 { > >=C2=A0 #if defined(CONFIG_USER_ONLY) > > -=C2=A0=C2=A0=C2=A0 gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC); > > +=C2=A0=C2=A0=C2=A0 GEN_PRIV; > >=C2=A0 #else > >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 TCGv t0; > > -=C2=A0=C2=A0=C2=A0 if (unlikely(ctx->pr)) { > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 gen_inval_exception(ctx, = POWERPC_EXCP_PRIV_OPC); > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return; > > -=C2=A0=C2=A0=C2=A0 } > >=C2=A0=20 > > +=C2=A0=C2=A0=C2=A0 CHK_SV; >=20 > ISA says tlbivax is hypervisor privileged when the CPU has a > hypervisor mode, which I guess booke206 probably doesn't? Right so here, the "problem" is that afaik, TCG doesn't implement the BookE hypervisor mode. So with my limited BookE testing ability I prefer sticking to a mechanical replacement that matches the existing code. It can be fixed later if necessary. > >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 t0 =3D tcg_temp_new(); > >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 gen_addr_reg_index(ctx, t0); > > - > >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 gen_helper_booke206_tlbivax(cpu_env, t0= ); > >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 tcg_temp_free(t0); > > -#endif > > +#endif /* defined(CONFIG_USER_ONLY) */ > >=C2=A0 } > >=C2=A0=20 > >=C2=A0 static void gen_tlbilx_booke206(DisasContext *ctx) > >=C2=A0 { > >=C2=A0 #if defined(CONFIG_USER_ONLY) > > -=C2=A0=C2=A0=C2=A0 gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC); > > +=C2=A0=C2=A0=C2=A0 GEN_PRIV; > >=C2=A0 #else > >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 TCGv t0; > > -=C2=A0=C2=A0=C2=A0 if (unlikely(ctx->pr)) { > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 gen_inval_exception(ctx, = POWERPC_EXCP_PRIV_OPC); > > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return; > > -=C2=A0=C2=A0=C2=A0 } > >=C2=A0=20 > > +=C2=A0=C2=A0=C2=A0 CHK_SV; >=20 > And apparently hv vs. sv privilege of tlbilx depends on the EPCR > register.=C2=A0 Again, may not be relevant for 2.06. Well, here too, I basically preserve existing BookE TCG behaviour, whether it's correct or not. That can be fixed separately if somebody cares about BookE HV mode. > >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 t0 =3D tcg_temp_new(); > >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 gen_addr_reg_index(ctx, t0); > >=C2=A0=20 > > @@ -6672,7 +6574,7 @@ static void gen_tlbilx_booke206(DisasContext > *ctx) > >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } > >=C2=A0=20 > >=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 tcg_temp_free(t0); > > -#endif > > +#endif /* defined(CONFIG_USER_ONLY) */ > >=C2=A0 } >=20