From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43315) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YXrEf-00055u-Li for qemu-devel@nongnu.org; Tue, 17 Mar 2015 09:08:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YXrEc-0002GT-7m for qemu-devel@nongnu.org; Tue, 17 Mar 2015 09:08:41 -0400 Received: from mailapp01.imgtec.com ([195.59.15.196]:63983) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YXrEb-0002GL-RU for qemu-devel@nongnu.org; Tue, 17 Mar 2015 09:08:38 -0400 Message-ID: <55082750.7020401@imgtec.com> Date: Tue, 17 Mar 2015 13:08:32 +0000 From: Leon Alrae MIME-Version: 1.0 References: <1426586167-1552-1-git-send-email-leon.alrae@imgtec.com> <55080816.5010001@imgtec.com> In-Reply-To: <55080816.5010001@imgtec.com> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit 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: James Hogan , qemu-devel@nongnu.org Cc: aurelien@aurel32.net Hi James, On 17/03/2015 10:55, James Hogan wrote: > Hi Leon, > > On 17/03/15 09:56, Leon Alrae wrote: >> This relatively small architectural feature adds the following: >> >> FIR.FREP: Read-only. If FREP=1, then Config5.FRE and Config5.UFE are available. >> >> Config5.FRE: When enabled all single-precision FP arithmetic instructions, >> LWC1/LWXC1/MTC1, SWC1/SWXC1/MFC1 cause a Reserved Instructions >> exception. >> >> Config5.UFE: Allows user to write/read Config5.FRE using CTC1/CFC1 instructions. >> >> Enable the feature in MIPS64R6-generic CPU. >> >> 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(-) >> >> 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 BadInstr */ >> 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) */ >> >> @@ -843,7 +847,7 @@ static inline void compute_hflags(CPUMIPSState *env) >> env->hflags &= ~(MIPS_HFLAG_COP1X | MIPS_HFLAG_64 | MIPS_HFLAG_CP0 | >> MIPS_HFLAG_F64 | MIPS_HFLAG_FPU | MIPS_HFLAG_KSU | >> MIPS_HFLAG_AWRAP | MIPS_HFLAG_DSP | MIPS_HFLAG_DSPR2 | >> - 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 *env) >> env->hflags |= MIPS_HFLAG_MSA; >> } >> } >> + if (env->active_fpu.fcr0 & (1 << FCR0_FREP)) { >> + if (env->CP0_Config5 & (1 << CP0C5_FRE)) { >> + env->hflags |= MIPS_HFLAG_FRE; >> + } >> + } >> } >> >> #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, uint32_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 = (env->CP0_Config5 >> CP0C5_FRE) & 1; >> + } else { >> + helper_raise_exception(env, EXCP_RI); >> + } >> + } >> + break; >> case 25: >> arg1 = ((env->active_fpu.fcr31 >> 24) & 0xfe) | ((env->active_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 == 0))) { >> + return; >> + } > > If rt != 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 != $0, not UNDEFINED > (and at least UNFR is required to produce an RI exception in r5 > implementations). Hmm, I believe the code above is correct and is doing exactly what you described :) Note that "&& (rt == 0)" is inside parenthesis following the logical NOT operator. It is no-op if rt != 0. > > "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 != 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 != 0 case when FREP=1 to become a > no-op too which seems similarly safer, even though the FRE bit may > technically be "accessible" in user mode when FREP=1 && UFE=1, so not > impossible for an UNPREDICTABLE operation to clobber. > > >> + if (env->CP0_Config5 & (1 << CP0C5_UFE)) { >> + env->CP0_Config5 &= ~(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 == 0))) { >> + return; >> + } >> + if (env->CP0_Config5 & (1 << CP0C5_UFE)) { >> + env->CP0_Config5 |= (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) >> } >> } >> >> +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)? Indeed. I'll add a separate patch. > >> /* 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? The reason is to avoid leaving the TCG temp marked as TEMP_VAL_DEAD which would cause assertion failures in TCG as we still generate code using this temp after returning from gen_load_fpr(). > >> + } >> tcg_gen_trunc_i64_i32(t, fpu_f64[reg]); >> } >> >> -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 = tcg_temp_new_i64(); > > declarations after code. Thanks for spotting this! > >> tcg_gen_extu_i32_i64(t64, t); >> tcg_gen_deposit_i64(fpu_f64[reg], fpu_f64[reg], t64, 0, 32); > > Rest looks okay AFAICT. > Leon