From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34352) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aQ1u9-0005IM-FK for qemu-devel@nongnu.org; Sun, 31 Jan 2016 18:59:42 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aQ1u6-0004Q3-8O for qemu-devel@nongnu.org; Sun, 31 Jan 2016 18:59:41 -0500 Date: Mon, 1 Feb 2016 10:50:51 +1100 From: David Gibson Message-ID: <20160131235051.GW23043@voom.redhat.com> References: <1454092821-46795-1-git-send-email-jrtc27@jrtc27.com> <1454092821-46795-3-git-send-email-jrtc27@jrtc27.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="GdlkuMH+DRYbUHkj" Content-Disposition: inline In-Reply-To: <1454092821-46795-3-git-send-email-jrtc27@jrtc27.com> Subject: Re: [Qemu-devel] [PATCH v2 2/2] target-ppc: mcrfs should always update FEX/VX and only clear exception bits List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: James Clarke Cc: qemu-ppc@nongnu.org, qemu-devel@nongnu.org --GdlkuMH+DRYbUHkj Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Jan 29, 2016 at 06:40:21PM +0000, James Clarke wrote: > Here is the description of the mcrfs instruction from the PowerPC Archite= cture > Book, Version 2.02, Book I: PowerPC User Instruction Set Architecture > (http://www.ibm.com/developerworks/systems/library/es-archguide-v2.html),= found > on page 120: >=20 > The contents of FPSCR field BFA are copied to Condition Register fiel= d BF. > All exception bits copied are set to 0 in the FPSCR. If the FX bit is > copied, it is set to 0 in the FPSCR. >=20 > Special Registers Altered: > CR field BF > FX OX (if BFA=3D0) > UX ZX XX VXSNAN (if BFA=3D1) > VXISI VXIDI VXZDZ VXIMZ (if BFA=3D2) > VXVC (if BFA=3D3) > VXSOFT VXSQRT VXCVI (if BFA=3D5) >=20 > However, currently every bit in FPSCR field BFA is set to 0, including on= es not > on that list. >=20 > This can be seen in the following simple C program: >=20 > #include > #include >=20 > int main(int argc, char **argv) { > int ret; > ret =3D fegetround(); > printf("Current rounding: %d\n", ret); > ret =3D fesetround(FE_UPWARD); > printf("Setting to FE_UPWARD (%d): %d\n", FE_UPWARD, ret); > ret =3D fegetround(); > printf("Current rounding: %d\n", ret); > ret =3D fegetround(); > printf("Current rounding: %d\n", ret); > return 0; > } >=20 > which gave the output (before this commit): >=20 > Current rounding: 0 > Setting to FE_UPWARD (2): 0 > Current rounding: 2 > Current rounding: 0 >=20 > instead of (after this commit): >=20 > Current rounding: 0 > Setting to FE_UPWARD (2): 0 > Current rounding: 2 > Current rounding: 2 >=20 > The relevant disassembly is in fegetround(), which, on my system, is: >=20 > __GI___fegetround: > <+0>: mcrfs cr7, cr7 > <+4>: mfcr r3 > <+8>: clrldi r3, r3, 62 > <+12>: blr >=20 > What happens is that, the first time fegetround() is called, FPSCR field = 7 is > retrieved. However, because of the bug in mcrfs, the entirety of field 7 = is set > to 0, which includes the rounding mode. >=20 > There are other issues this will fix, such as condition flags not persist= ing > when they should if read, and if you were to read a specific field with s= ome > exception bits set, but no others were set in the entire register, then t= he > bits would be cleared correctly, but FEX/VX would not be updated to 0 as = they > should be. >=20 > Signed-off-by: James Clarke Thanks for the fixup. It actually looks like helper_store_fpscr() should really take a target_ulong instead of u64 and have the (single) caller which wants to pass a 64 do the truncate. But that can be a cleanup for another day. Applied to ppc-for-2.6. > --- > target-ppc/cpu.h | 6 ++++++ > target-ppc/translate.c | 21 +++++++++++++++++---- > 2 files changed, 23 insertions(+), 4 deletions(-) >=20 > diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h > index 3a967b7..d811bc9 100644 > --- a/target-ppc/cpu.h > +++ b/target-ppc/cpu.h > @@ -718,6 +718,12 @@ enum { > #define FP_RN1 (1ull << FPSCR_RN1) > #define FP_RN (1ull << FPSCR_RN) > =20 > +/* the exception bits which can be cleared by mcrfs - includes FX */ > +#define FP_EX_CLEAR_BITS (FP_FX | FP_OX | FP_UX | FP_ZX = | \ > + FP_XX | FP_VXSNAN | FP_VXISI | FP_VXIDI = | \ > + FP_VXZDZ | FP_VXIMZ | FP_VXVC | FP_VXSOFT = | \ > + FP_VXSQRT | FP_VXCVI) > + > /***********************************************************************= ******/ > /* Vector status and control register */ > #define VSCR_NJ 16 /* Vector non-java */ > diff --git a/target-ppc/translate.c b/target-ppc/translate.c > index 4be7eaa..ca10bd1 100644 > --- a/target-ppc/translate.c > +++ b/target-ppc/translate.c > @@ -2500,18 +2500,31 @@ static void gen_fmrgow(DisasContext *ctx) > static void gen_mcrfs(DisasContext *ctx) > { > TCGv tmp =3D tcg_temp_new(); > + TCGv_i32 tmask; > + TCGv_i64 tnew_fpscr =3D tcg_temp_new_i64(); > int bfa; > + int nibble; > + int shift; > =20 > if (unlikely(!ctx->fpu_enabled)) { > gen_exception(ctx, POWERPC_EXCP_FPU); > return; > } > - bfa =3D 4 * (7 - crfS(ctx->opcode)); > - tcg_gen_shri_tl(tmp, cpu_fpscr, bfa); > + bfa =3D crfS(ctx->opcode); > + nibble =3D 7 - bfa; > + shift =3D 4 * nibble; > + tcg_gen_shri_tl(tmp, cpu_fpscr, shift); > tcg_gen_trunc_tl_i32(cpu_crf[crfD(ctx->opcode)], tmp); > - tcg_temp_free(tmp); > tcg_gen_andi_i32(cpu_crf[crfD(ctx->opcode)], cpu_crf[crfD(ctx->opcod= e)], 0xf); > - tcg_gen_andi_tl(cpu_fpscr, cpu_fpscr, ~(0xF << bfa)); > + tcg_temp_free(tmp); > + tcg_gen_extu_tl_i64(tnew_fpscr, cpu_fpscr); > + /* Only the exception bits (including FX) should be cleared if read = */ > + tcg_gen_andi_i64(tnew_fpscr, tnew_fpscr, ~((0xF << shift) & FP_EX_CL= EAR_BITS)); > + /* FEX and VX need to be updated, so don't set fpscr directly */ > + tmask =3D tcg_const_i32(1 << nibble); > + gen_helper_store_fpscr(cpu_env, tnew_fpscr, tmask); > + tcg_temp_free_i32(tmask); > + tcg_temp_free_i64(tnew_fpscr); > } > =20 > /* mffs */ --=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 --GdlkuMH+DRYbUHkj Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJWrp3bAAoJEGw4ysog2bOSonUP/jpLJFrM6PwzleSd+zzuk59M d8VOOzUQ7/Jqfywjbr8ECZvybFDp5AH475+QhOihbMVidJ1gMrYjT+iBJDKRjESr +PEgwnN+ae55K42F8YyoiVVkyx59Oi14W3NdsbsurYkMTQcsdK8vF+cwJ+OC9zah QAGmncLL6QoivGl1Xw5zTwmBuBzlBwXfZqmnYNL0j3igXSm+8vG+/Om2dOgWAsdX fRsZ2glrsK+q87yOS53Upj9xgIn6Zlc/Sha3CFPv0anuhvXprK4Xb3PKGo3qyvm2 kyY+WG2tZIrK800+/eVota1JCUxgEoSam7mMgTdFzRHibRGl6jfa93IjioVOJjKr XluZ5DpC4zjZ0CtCn1BebeTXlnHNnPBkKLjA8YA4XhXr6TV+Wis5hn08Ik10JG8F fjYyOiyo7FC0Se/1cFAMXKT2YEaQ+tWAHEbras2gx5kFfbAc/kpR0CwIsd5Zf+su XfNsQcCCvu/rgIYImWpWICWaOUI7paWHJCW+CPOVKiVWj32BwAl+/HvsL1CbGF2c DlFZL1UGKE5JhTnEkRI2tT4HtqSlF1yu3PxyxPAKgkAq/zAhznhu1NibZN4OnyaH SmrQ6hu/eimZ6yZLQmkt6MmZEgXQqB9ROo4c3CwI7KroBWctnofAiy1NbygaZPCN ZmP/WcUKV1gAVYMUgtrU =TEXU -----END PGP SIGNATURE----- --GdlkuMH+DRYbUHkj--