Re: [Qemu-arm] [PATCH v6 9/9] target-mips: Implement FCR31's R/W bitmask and related functionalities

[Leon Alrae]

> - Add preprocessor constants for all bits of FCR31 and related masks
>   for its subfields.

Introducing all these constants for fcr31_rw_bitmask doesn't seem necessary
or useful

> 
> - Modify handling of CFC1 and CTC1 instructions (cases 25, 26, 28)
>   so that they utilize newly-defind constants. This is just a cosmetic
>   change, to make the code more readable, and to avoid usage of
>   hardcoded constants.

this an unrelated change; it should be moved to a separate patch

> +static inline void restore_snan_bit_mode(CPUMIPSState *env)
> +{
> +    set_snan_bit_is_one(!((env->active_fpu.fcr31 & (1 << FCR31_NAN2008)) != 
> 0),

this is just: (env->active_fpu.fcr31 & (1 << FCR31_NAN2008)) == 0

> @@ -89,11 +89,9 @@ int mips_cpu_gdb_write_register(CPUState *cs, uint8_t 
> *mem_buf, int n)
>      if (env->CP0_Config1 & (1 << CP0C1_FP) && n >= 38 && n < 72) {
>          switch (n) {
>          case 70:
> -            env->active_fpu.fcr31 = tmp & 0xFF83FFFF;
> -            /* set rounding mode */
> -            restore_rounding_mode(env);
> -            /* set flush-to-zero mode */
> -            restore_flush_mode(env);
> +            env->active_fpu.fcr31 = (tmp & env->active_fpu.fcr31_rw_bitmask) 
> |
> +               (env->active_fpu.fcr31 & !(env->active_fpu.fcr31_rw_bitmask));

I think you wanted bitwise-not here

> +            restore_fp_status(env);
>              break;
>          case 71:
>              /* FIR is read-only.  Ignore writes.  */
> diff --git a/target-mips/op_helper.c b/target-mips/op_helper.c
> index 0d1e959..cb890bc 100644
> --- a/target-mips/op_helper.c
> +++ b/target-mips/op_helper.c
> @@ -2490,15 +2490,23 @@ target_ulong helper_cfc1(CPUMIPSState *env, uint32_t 
> reg)
>          }
>          break;
>      case 25:
> -        arg1 = ((env->active_fpu.fcr31 >> 24) & 0xfe) | 
> ((env->active_fpu.fcr31 >> 23) & 0x1);
> +        /* read from Floating Point Condition Codes Register (FCCR) */
> +        arg1 = ((env->active_fpu.fcr31 >> 24) & 0xfe) |
> +               ((env->active_fpu.fcr31 >> 23) & 0x1);

this is an unrelated cosmetic change, please remove it from this patch (there
are more changes like that in this patch)

> @@ -2558,42 +2566,66 @@ void helper_ctc1(CPUMIPSState *env, target_ulong 
> arg1, uint32_t fs, uint32_t rt)
>          }
>          break;
>      case 25:
> +        /* write to Floating Point Condition Codes Register (FCCR) */
>          if ((env->insn_flags & ISA_MIPS32R6) || (arg1 & 0xffffff00)) {
>              return;
>          }
> -        env->active_fpu.fcr31 = (env->active_fpu.fcr31 & 0x017fffff) | 
> ((arg1 & 0xfe) << 24) |
> -                     ((arg1 & 0x1) << 23);
> +        env->active_fpu.fcr31 =
> +                (env->active_fpu.fcr31 & FCR31_ROUNDING_MODE_MASK) |
> +                (env->active_fpu.fcr31 & FCR31_FLAGS_MASK) |
> +                (env->active_fpu.fcr31 & FCR31_ENABLE_MASK) |
> +                (env->active_fpu.fcr31 & FCR31_CAUSE_MASK) |
> +                (env->active_fpu.fcr31 & FCR31_IEEE2008_MASK) |
> +                (env->active_fpu.fcr31 & FCR31_IMPL_MASK) |
> +                (env->active_fpu.fcr31 & FCR31_FCC_MASK) |
> +                ((arg1 & 0x1) << 23) |
> +                (env->active_fpu.fcr31 & FCR31_FS_MASK) |
> +                ((arg1 & 0xfe) << 24);
>          break;

I don't find it easier to read. CTC1 and CFC1 helpers became a mixture of
various sub-masks and hardcoded constants. Also, it is now harder to map
the implementation to the lines from CTC1/CFC1 in the MIPS64BIS manual:

elseif fs = 25 then /* FCCR */
if temp31..8 != then
    UNPREDICTABLE
else
    FCSR <- temp7..1 || FCSR24 || temp0 || FCSR22..0


Thanks,
Leon