Re: [Qemu-devel] [2.4 PATCH] target-mips: add Config5.FRE support allowing Status.FR=0 emulation

[Leon Alrae <leon.alrae@imgtec.com>]

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 <leon.alrae@imgtec.com>
>> ---
>>  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