From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36326) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YXp9o-0000zF-RU for qemu-devel@nongnu.org; Tue, 17 Mar 2015 06:55:34 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YXp9l-00075Q-Et for qemu-devel@nongnu.org; Tue, 17 Mar 2015 06:55:32 -0400 Received: from mailapp01.imgtec.com ([195.59.15.196]:40651 helo=imgpgp01.kl.imgtec.org) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YXp9l-00074q-4C for qemu-devel@nongnu.org; Tue, 17 Mar 2015 06:55:29 -0400 Message-ID: <55080816.5010001@imgtec.com> Date: Tue, 17 Mar 2015 10:55:18 +0000 From: James Hogan MIME-Version: 1.0 References: <1426586167-1552-1-git-send-email-leon.alrae@imgtec.com> In-Reply-To: <1426586167-1552-1-git-send-email-leon.alrae@imgtec.com> Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="Vsg5HkJtagCpCrMCKTB2OxpsBg1p7DdJc" Subject: Re: [Qemu-devel] [2.4 PATCH] target-mips: add Config5.FRE support allowing Status.FR=0 emulation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Leon Alrae , qemu-devel@nongnu.org Cc: aurelien@aurel32.net --Vsg5HkJtagCpCrMCKTB2OxpsBg1p7DdJc Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable Hi Leon, On 17/03/15 09:56, Leon Alrae wrote: > This relatively small architectural feature adds the following: >=20 > FIR.FREP: Read-only. If FREP=3D1, then Config5.FRE and Config5.UFE are = available. >=20 > Config5.FRE: When enabled all single-precision FP arithmetic instructio= ns, > LWC1/LWXC1/MTC1, SWC1/SWXC1/MFC1 cause a Reserved Instruct= ions > exception. >=20 > Config5.UFE: Allows user to write/read Config5.FRE using CTC1/CFC1 inst= ructions. >=20 > Enable the feature in MIPS64R6-generic CPU. >=20 > Signed-off-by: Leon Alrae > --- > target-mips/cpu.h | 13 +- > target-mips/op_helper.c | 34 +++++ > target-mips/translate.c | 307 ++++++++++++++++++++++-------------= -------- > target-mips/translate_init.c | 9 +- > 4 files changed, 208 insertions(+), 155 deletions(-) >=20 > diff --git a/target-mips/cpu.h b/target-mips/cpu.h > index f9d2b4c..03eb888 100644 > --- a/target-mips/cpu.h > +++ b/target-mips/cpu.h > @@ -100,6 +100,7 @@ struct CPUMIPSFPUContext { > float_status fp_status; > /* fpu implementation/revision register (fir) */ > uint32_t fcr0; > +#define FCR0_FREP 29 > #define FCR0_UFRP 28 > #define FCR0_F64 22 > #define FCR0_L 21 > @@ -462,6 +463,8 @@ struct CPUMIPSState { > #define CP0C5_CV 29 > #define CP0C5_EVA 28 > #define CP0C5_MSAEn 27 > +#define CP0C5_UFE 9 > +#define CP0C5_FRE 8 > #define CP0C5_SBRI 6 > #define CP0C5_UFR 2 > #define CP0C5_NFExists 0 > @@ -514,7 +517,7 @@ struct CPUMIPSState { > #define EXCP_INST_NOTAVAIL 0x2 /* No valid instruction word for BadIns= tr */ > uint32_t hflags; /* CPU State */ > /* TMASK defines different execution modes */ > -#define MIPS_HFLAG_TMASK 0x15807FF > +#define MIPS_HFLAG_TMASK 0x35807FF > #define MIPS_HFLAG_MODE 0x00007 /* execution modes = */ > /* The KSU flags must be the lowest bits in hflags. The flag order= > must be the same as defined for CP0 Status. This allows to use > @@ -561,6 +564,7 @@ struct CPUMIPSState { > #define MIPS_HFLAG_SBRI 0x400000 /* R6 SDBBP causes RI excpt. in user= mode */ > #define MIPS_HFLAG_FBNSLOT 0x800000 /* Forbidden slot = */ > #define MIPS_HFLAG_MSA 0x1000000 > +#define MIPS_HFLAG_FRE 0x2000000 /* FRE enabled */ > target_ulong btarget; /* Jump / branch target = */ > target_ulong bcond; /* Branch condition (if needed) = */ > =20 > @@ -843,7 +847,7 @@ static inline void compute_hflags(CPUMIPSState *env= ) > env->hflags &=3D ~(MIPS_HFLAG_COP1X | MIPS_HFLAG_64 | MIPS_HFLAG_C= P0 | > MIPS_HFLAG_F64 | MIPS_HFLAG_FPU | MIPS_HFLAG_KSU = | > MIPS_HFLAG_AWRAP | MIPS_HFLAG_DSP | MIPS_HFLAG_DS= PR2 | > - MIPS_HFLAG_SBRI | MIPS_HFLAG_MSA); > + MIPS_HFLAG_SBRI | MIPS_HFLAG_MSA | MIPS_HFLAG_FRE= ); > if (!(env->CP0_Status & (1 << CP0St_EXL)) && > !(env->CP0_Status & (1 << CP0St_ERL)) && > !(env->hflags & MIPS_HFLAG_DM)) { > @@ -924,6 +928,11 @@ static inline void compute_hflags(CPUMIPSState *en= v) > env->hflags |=3D MIPS_HFLAG_MSA; > } > } > + if (env->active_fpu.fcr0 & (1 << FCR0_FREP)) { > + if (env->CP0_Config5 & (1 << CP0C5_FRE)) { > + env->hflags |=3D MIPS_HFLAG_FRE; > + } > + } > } > =20 > #ifndef CONFIG_USER_ONLY > diff --git a/target-mips/op_helper.c b/target-mips/op_helper.c > index 73a8e45..dd89068 100644 > --- a/target-mips/op_helper.c > +++ b/target-mips/op_helper.c > @@ -2303,6 +2303,16 @@ target_ulong helper_cfc1(CPUMIPSState *env, uint= 32_t reg) > } > } > break; > + case 5: > + /* FRE Support - read Config5.FRE bit */ > + if (env->active_fpu.fcr0 & (1 << FCR0_FREP)) { > + if (env->CP0_Config5 & (1 << CP0C5_UFE)) { > + arg1 =3D (env->CP0_Config5 >> CP0C5_FRE) & 1; > + } else { > + helper_raise_exception(env, EXCP_RI); > + } > + } > + break; > case 25: > arg1 =3D ((env->active_fpu.fcr31 >> 24) & 0xfe) | ((env->activ= e_fpu.fcr31 >> 23) & 0x1); > break; > @@ -2347,6 +2357,30 @@ void helper_ctc1(CPUMIPSState *env, target_ulong= arg1, uint32_t fs, uint32_t rt) > helper_raise_exception(env, EXCP_RI); > } > break; > + case 5: > + /* FRE Support - clear Config5.FRE bit */ > + if (!((env->active_fpu.fcr0 & (1 << FCR0_FREP)) && (rt =3D=3D = 0))) { > + return; > + } If rt !=3D 0, is it really desired for a Config5 bit (which is privileged= state) to be modified? Assuming these behave similarly to UFR/UNFR, the behaviour is architecturally UNPREDICTABLE when rt !=3D $0, not UNDEFINED= (and at least UNFR is required to produce an RI exception in r5 implementations). "UNPREDICTABLE operations must not read, write, or modify the contents of memory or internal state which is inaccessible in the current processor mode. For example, UNPREDICTABLE operations executed in user mode must not access memory or internal state that is only accessible in Kernel Mode or Debug Mode or in another process" Probably same below and for UFR/UNFR really. Should it be more like this?: if (!((env->active_fpu.fcr0 & (1 << FCR0_FREP)) || (rt !=3D 0))) { That still ignores the potential RI that may or may not be required, but that behaviour seems vaguely defined. It also causes the UNPREDICTABLE rt !=3D 0 case when FREP=3D1 to become a= no-op too which seems similarly safer, even though the FRE bit may technically be "accessible" in user mode when FREP=3D1 && UFE=3D1, so not= impossible for an UNPREDICTABLE operation to clobber. > + if (env->CP0_Config5 & (1 << CP0C5_UFE)) { > + env->CP0_Config5 &=3D ~(1 << CP0C5_FRE); > + compute_hflags(env); > + } else { > + helper_raise_exception(env, EXCP_RI); > + } > + break; > + case 6: > + /* FRE Support - set Config5.FRE bit */ > + if (!((env->active_fpu.fcr0 & (1 << FCR0_FREP)) && (rt =3D=3D = 0))) { > + return; > + } > + if (env->CP0_Config5 & (1 << CP0C5_UFE)) { > + env->CP0_Config5 |=3D (1 << CP0C5_FRE); > + compute_hflags(env); > + } else { > + helper_raise_exception(env, EXCP_RI); > + } > + break; > case 25: > if ((env->insn_flags & ISA_MIPS32R6) || (arg1 & 0xffffff00)) {= > return; > diff --git a/target-mips/translate.c b/target-mips/translate.c > index 7030734..b9fcc8b 100644 > --- a/target-mips/translate.c > +++ b/target-mips/translate.c > @@ -1557,14 +1557,22 @@ static inline void gen_store_srsgpr (int from, = int to) > } > } > =20 > +static inline void generate_exception(DisasContext *ctx, int excp); > + cleaner to just swap the "Floating point register moves" and "Tests" groups of functions (as a separate commit)? > /* Floating point register moves. */ > -static void gen_load_fpr32(TCGv_i32 t, int reg) > +static void gen_load_fpr32(DisasContext *ctx, TCGv_i32 t, int reg) > { > + if (ctx->hflags & MIPS_HFLAG_FRE) { > + generate_exception(ctx, EXCP_RI); Maybe return to avoid generating dead code? > + } > tcg_gen_trunc_i64_i32(t, fpu_f64[reg]); > } > =20 > -static void gen_store_fpr32(TCGv_i32 t, int reg) > +static void gen_store_fpr32(DisasContext *ctx, TCGv_i32 t, int reg) > { > + if (ctx->hflags & MIPS_HFLAG_FRE) { > + generate_exception(ctx, EXCP_RI); same > + } > TCGv_i64 t64 =3D tcg_temp_new_i64(); declarations after code. > tcg_gen_extu_i32_i64(t64, t); > tcg_gen_deposit_i64(fpu_f64[reg], fpu_f64[reg], t64, 0, 32); Rest looks okay AFAICT. Thanks! James --Vsg5HkJtagCpCrMCKTB2OxpsBg1p7DdJc Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBAgAGBQJVCAgdAAoJEGwLaZPeOHZ69J0P+gKX/WtYWdCnhcP/RXeVx3AC b6Y3OgD8GiHsvX35R+lbTlLuKzpLpU8OHLaL5hRnuVSgjRkmbelee72l1YHaeOJA Po2bWFkDwjQ11+GNiguSvvVBYDPPyij7VkGvps026c+ncxWjAj6IcfT9RZD9Gmtd eDx7GDoZvLXQj64/gthYAoe168vxFIvsUNRybwD/YqwGVxexqtytn8Wp4IZrBcG4 G20JcHLhTMtzzf89cQaLSZCh9okxxIyCEnBTTNgz6fvw5N57EJuLfriKBMA1DK77 woiGr16VGO6nsUORg3HYrgo0UyDucMnbd8LEUiUUupSLUmliLqfDNfpIgS64toBu E1iykELIZMIC89jPY6CHQBr5E4RN9nnemUwILdsFCYLSGb4iaozSZ0YfZunMNlcB umriKgh795ddOUVZQVCSMtZKFqpmMx2HObGZcq53H5Z/uW45RSG0x0qcbbTIqLsR bDfqKDhONSybwVF+9BLUyYjZgDN62CFgz3/E/c4XXQ0GSaXd9Y0gYLElDcyvgZ88 933daIIL7DAeYFvWjZOgOHSaVmSCQLEdL8BnMcW7c6IXAIDXZBu+ANUbgZjUKo6k JhqoBbQpkulUVFoX5cDHvqEqi3wzkrmhyXtXvNnfNVaXXcNll093c7pZQH3+CrUw kDUf3ORBffSqWDId7M/7 =+YN8 -----END PGP SIGNATURE----- --Vsg5HkJtagCpCrMCKTB2OxpsBg1p7DdJc--