From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37308) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zzhzz-0000j2-0w for qemu-devel@nongnu.org; Fri, 20 Nov 2015 04:28:56 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zzhzx-0002ec-Ak for qemu-devel@nongnu.org; Fri, 20 Nov 2015 04:28:54 -0500 Date: Fri, 20 Nov 2015 18:45:26 +1100 From: David Gibson Message-ID: <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> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="i0/AhcQY5QxfSsSZ" Content-Disposition: inline In-Reply-To: <1447201710-10229-22-git-send-email-benh@kernel.crashing.org> 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: Benjamin Herrenschmidt Cc: qemu-ppc@nongnu.org, qemu-devel@nongnu.org --i0/AhcQY5QxfSsSZ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Nov 11, 2015 at 11:27:34AM +1100, Benjamin Herrenschmidt wrote: > Recent server processors use the Hypervisor Emulation Assistance > interrupt for illegal instructions and *some* type of SPR accesses. >=20 > Also the code was always generating inval instructions even for priv > violations due to setting the wrong flags >=20 > Finally, the checking for PR/HV was open coded everywhere. >=20 > This reworks it all, using little helper macros for checking, and > adding the HV interrupt (which gets converted back to program check > in the slow path of excp_helper.c on CPUs that don't want it). >=20 > Signed-off-by: Benjamin Herrenschmidt [snip] > static void spr_noaccess(DisasContext *ctx, int gprn, int sprn) > @@ -4340,7 +4350,7 @@ static inline void gen_op_mfspr(DisasContext *ctx) > printf("Trying to read privileged spr %d (0x%03x) at " > TARGET_FMT_lx "\n", sprn, sprn, ctx->nip - 4); > } > - gen_inval_exception(ctx, POWERPC_EXCP_PRIV_REG); > + gen_priv_exception(ctx, POWERPC_EXCP_PRIV_REG); > } > } else { > /* Not defined */ > @@ -4348,7 +4358,25 @@ static inline void gen_op_mfspr(DisasContext *ctx) > TARGET_FMT_lx "\n", sprn, sprn, ctx->nip - 4); > printf("Trying to read invalid spr %d (0x%03x) at " > TARGET_FMT_lx "\n", sprn, sprn, ctx->nip - 4); So, I'm not 100% following the logic below, but it looks like the existing code used SPR_NOACCESS to mark things which generated a privilege exception compared to NULL for things which generated an invalid instruction exception. Using that encoding, can you simplify the logic here? Alternatively can you use the logic here to avoid the SPR_NOACESS encoding? > - gen_inval_exception(ctx, POWERPC_EXCP_INVAL_SPR); > + > + /* The behaviour depends on MSR:PR and SPR# bit 0x10, > + * it can generate a priv, a hv emu or a no-op > + */ > + if (sprn & 0x10) { > + if (ctx->pr) { > + gen_priv_exception(ctx, POWERPC_EXCP_INVAL_SPR); > + } > + } else { > + if (ctx->pr || sprn =3D=3D 0 || sprn =3D=3D 4 || sprn =3D=3D= 5 || sprn =3D=3D 6) { > + gen_hvpriv_exception(ctx, POWERPC_EXCP_INVAL_SPR); > + } > + } > +#if !defined(CONFIG_USER_ONLY) > + /* HV priv */ > + if (ctx->spr_cb[sprn].hea_read) { > + gen_priv_exception(ctx, POWERPC_EXCP_INVAL_SPR); > + } If you're in PR mode, and it's an SPR with an hea_read function and has the 0x10 bit set, won't this call gen_priv_exception twice? I also see no path here which will call gen_inval_exception(), is that right? If you're in HV mode and it's a truly invalid SPRN, isn't that what you'd want? > +#endif > } > } > =20 > @@ -4395,13 +4423,9 @@ static void gen_mtcrf(DisasContext *ctx) > #if defined(TARGET_PPC64) > static void gen_mtmsrd(DisasContext *ctx) > { > -#if defined(CONFIG_USER_ONLY) > - gen_inval_exception(ctx, POWERPC_EXCP_PRIV_REG); > -#else > - if (unlikely(ctx->pr)) { > - gen_inval_exception(ctx, POWERPC_EXCP_PRIV_REG); > - return; > - } > + CHK_SV; > + > +#if !defined(CONFIG_USER_ONLY) > if (ctx->opcode & 0x00010000) { > /* Special form that does not need any synchronisation */ > TCGv t0 =3D tcg_temp_new(); > @@ -4420,20 +4444,16 @@ static void gen_mtmsrd(DisasContext *ctx) > /* Note that mtmsr is not always defined as context-synchronizin= g */ > gen_stop_exception(ctx); > } > -#endif > +#endif /* !defined(CONFIG_USER_ONLY) */ > } > -#endif > +#endif /* defined(TARGET_PPC64) */ > =20 > static void gen_mtmsr(DisasContext *ctx) > { > -#if defined(CONFIG_USER_ONLY) > - gen_inval_exception(ctx, POWERPC_EXCP_PRIV_REG); > -#else > - if (unlikely(ctx->pr)) { > - gen_inval_exception(ctx, POWERPC_EXCP_PRIV_REG); > - return; > - } > - if (ctx->opcode & 0x00010000) { > + CHK_SV; > + > +#if !defined(CONFIG_USER_ONLY) > + if (ctx->opcode & 0x00010000) { > /* Special form that does not need any synchronisation */ > TCGv t0 =3D tcg_temp_new(); > tcg_gen_andi_tl(t0, cpu_gpr[rS(ctx->opcode)], (1 << MSR_RI) | (1= << MSR_EE)); > @@ -4488,7 +4508,7 @@ static void gen_mtspr(DisasContext *ctx) > TARGET_FMT_lx "\n", sprn, sprn, ctx->nip - 4); > printf("Trying to write privileged spr %d (0x%03x) at " > TARGET_FMT_lx "\n", sprn, sprn, ctx->nip - 4); > - gen_inval_exception(ctx, POWERPC_EXCP_PRIV_REG); > + gen_priv_exception(ctx, POWERPC_EXCP_PRIV_REG); > } > } else { > /* Not defined */ > @@ -4496,7 +4516,25 @@ static void gen_mtspr(DisasContext *ctx) > TARGET_FMT_lx "\n", sprn, sprn, ctx->nip - 4); > printf("Trying to write invalid spr %d (0x%03x) at " > TARGET_FMT_lx "\n", sprn, sprn, ctx->nip - 4); > - gen_inval_exception(ctx, POWERPC_EXCP_INVAL_SPR); > + > + /* The behaviour depends on MSR:PR and SPR# bit 0x10, > + * it can generate a priv, a hv emu or a no-op > + */ > + if (sprn & 0x10) { > + if (ctx->pr) { > + gen_priv_exception(ctx, POWERPC_EXCP_INVAL_SPR); > + } > + } else { > + if (ctx->pr || sprn =3D=3D 0) { > + gen_hvpriv_exception(ctx, POWERPC_EXCP_INVAL_SPR); > + } > + } > +#if !defined(CONFIG_USER_ONLY) > + /* HV priv */ > + if (ctx->spr_cb[sprn].hea_write) { > + gen_priv_exception(ctx, POWERPC_EXCP_INVAL_SPR); > + } > +#endif Same concerns here as for mfspr. [snip] > /* tlbiel */ > static void gen_tlbiel(DisasContext *ctx) > { > #if defined(CONFIG_USER_ONLY) > - gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC); > + GEN_PRIV; > #else > - if (unlikely(ctx->pr || !ctx->hv)) { > - gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC); > - return; > - } > + CHK_SV; You have CHK_SV here, but the original code checks for HV, as does your new code for tlbia and tlbiel, is that right? [snip] > /* tlbsync */ > static void gen_tlbsync(DisasContext *ctx) > { > #if defined(CONFIG_USER_ONLY) > - gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC); > -#else > - if (unlikely(ctx->pr)) { > - gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC); > - return; > - } > + GEN_PRIV; > +#else =20 > + CHK_HV; > + Old code didn't check for HV, mode, but AFAICT it should have, so this looks correct. [snip] > @@ -5941,18 +5921,16 @@ static void gen_mfapidi(DisasContext *ctx) > static void gen_tlbiva(DisasContext *ctx) > { > #if defined(CONFIG_USER_ONLY) > - gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC); > + GEN_PRIV; > #else > TCGv t0; > - if (unlikely(ctx->pr)) { > - gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC); > - return; > - } > + > + CHK_SV; Is the same thing as tlbivax, or some ancient instruction? AFAICT the ISA says tlbivax is hypervisor privileged. > t0 =3D tcg_temp_new(); > gen_addr_reg_index(ctx, t0); > gen_helper_tlbie(cpu_env, cpu_gpr[rB(ctx->opcode)]); > tcg_temp_free(t0); > -#endif > +#endif /* defined(CONFIG_USER_ONLY) */ > } [snip] > static void gen_tlbivax_booke206(DisasContext *ctx) > { > #if defined(CONFIG_USER_ONLY) > - gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC); > + GEN_PRIV; > #else > TCGv t0; > - if (unlikely(ctx->pr)) { > - gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC); > - return; > - } > =20 > + CHK_SV; ISA says tlbivax is hypervisor privileged when the CPU has a hypervisor mode, which I guess booke206 probably doesn't? > t0 =3D tcg_temp_new(); > gen_addr_reg_index(ctx, t0); > - > gen_helper_booke206_tlbivax(cpu_env, t0); > tcg_temp_free(t0); > -#endif > +#endif /* defined(CONFIG_USER_ONLY) */ > } > =20 > static void gen_tlbilx_booke206(DisasContext *ctx) > { > #if defined(CONFIG_USER_ONLY) > - gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC); > + GEN_PRIV; > #else > TCGv t0; > - if (unlikely(ctx->pr)) { > - gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC); > - return; > - } > =20 > + CHK_SV; And apparently hv vs. sv privilege of tlbilx depends on the EPCR register. Again, may not be relevant for 2.06. > t0 =3D tcg_temp_new(); > gen_addr_reg_index(ctx, t0); > =20 > @@ -6672,7 +6574,7 @@ static void gen_tlbilx_booke206(DisasContext *ctx) > } > =20 > tcg_temp_free(t0); > -#endif > +#endif /* defined(CONFIG_USER_ONLY) */ > } --=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 --i0/AhcQY5QxfSsSZ Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJWTs+VAAoJEGw4ysog2bOSu94QAM+RNvSFdk0kb6+KBoEeXVTL KmbbJhcrJUj2t0gGEZGEjBZOjXUF3uaxjsJKeBsWrKsFxq8Z3sJf2whbopjxw8Td T1hBVNxHyI4mNDrECYFuyPyET6kJT+s6yfxSGvPND4sz5YODw2jSGFSy86J8FZEh H8qtUy5SSBpG+rZZNoxZlNwntNS4Bt+5yftIVnIGGdszxHRXP1rGllEDcFTnrPCX bARGRE+6DgTpe81HS7Wb/vqASVb4ghetfsZcCA70VsThcrSzV2OXqt1tI2kR3BUb XZt0aezTpgMD//rbZnTWnPT2ROcxXMWDcm5GjxRs9EldA9ljXnGw0Vy7aZ2hq873 kNOvmMNRncXWgZu/o83BkgK46n2FSl7ZcpFf0MBqGlqHsiscIfvcGpMQJkOH69VI g6gCiYb8JOfnxRgpWyx29LLiLKb/CXSR4UtCRts+7joDKGM87B8QICmK4v8HW/bR dFi/mvrw5Six9y5eCzaccs3RHBXnL9itstbWV+JULRtl9vN0WA9hHuPkPjxQkCli 6G3HTt8iAoNzO9PrmB0nA+xDaTcPSL2jO6YmK2oM1XuqCEH/26Dp+gGN+EkMjl4T St+eLpJ44+dWhIqSi//InLeedp4gTl2rRClGgQt46xAREg5bgFjJwP7rXeiOI4Gm W7mZTUnkyqxzgDVUCdQX =Yi/m -----END PGP SIGNATURE----- --i0/AhcQY5QxfSsSZ--