[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [2.4 PATCH] target-mips: add Config5.FRE support allowi
From: |
Leon Alrae |
Subject: |
Re: [Qemu-devel] [2.4 PATCH] target-mips: add Config5.FRE support allowing Status.FR=0 emulation |
Date: |
Tue, 17 Mar 2015 13:08:32 +0000 |
User-agent: |
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20100101 Thunderbird/24.5.0 |
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 <address@hidden>
>> ---
>> 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