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

[James Hogan]

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).

"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)?

>  /* 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]);
>  }
>  
> -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.

>      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

signature.asc
Description: OpenPGP digital signature