Re: [Qemu-devel] [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 & 0xFF83;
> -/* 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 & 0xff00)) {
>  return;
>  }
> -env->active_fpu.fcr31 = (env->active_fpu.fcr31 & 0x017f) | 
> ((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

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

[Aleksandar Markovic]

From: Aleksandar Markovic 

This patch implements read and write access rules for Mips floating
point control and status register (FCR31). The change can be divided
into following parts:

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

- Add correspondant fields that will keep FCR31's R/W bitmask in
  procesor definitions and processor float_status structure.

- Add appropriate value for FCR31's R/W bitmask for each supported
  processor.

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

- Modify handling of CTC1 (case 31) instruction to use FCR31's R/W
  bitmask.

- Modify handling user mode executables for Mips, in relation to the
  bit EF_MIPS_NAN2008 from ELF header, that is in turn related to
  reading and writing to FCR31.

- Modify gdb behavior in relation to FCR31.

Signed-off-by: Thomas Schwinge 
Signed-off-by: Maciej W. Rozycki 
Signed-off-by: Aleksandar Markovic 
---
 linux-user/main.c| 14 
 target-mips/cpu.h| 75 +--
 target-mips/gdbstub.c|  8 ++---
 target-mips/op_helper.c  | 76 +++-
 target-mips/translate.c  |  9 ++
 target-mips/translate_init.c | 54 +++
 6 files changed, 199 insertions(+), 37 deletions(-)

diff --git a/linux-user/main.c b/linux-user/main.c
index 5f3ec97..cc21057 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -4608,6 +4608,20 @@ int main(int argc, char **argv, char **envp)
 if (regs->cp0_epc & 1) {
 env->hflags |= MIPS_HFLAG_M16;
 }
+if (((info->elf_flags & EF_MIPS_NAN2008) != 0) !=
+((env->active_fpu.fcr31 & (1 << FCR31_NAN2008)) != 0)) {
+if ((env->active_fpu.fcr31_rw_bitmask &
+  (1 << FCR31_NAN2008)) == 0) {
+fprintf(stderr, "ELF binary's NaN mode not supported by 
CPU\n");
+exit(1);
+}
+if ((info->elf_flags & EF_MIPS_NAN2008) != 0) {
+env->active_fpu.fcr31 |= (1 << FCR31_NAN2008);
+} else {
+env->active_fpu.fcr31 &= ~(1 << FCR31_NAN2008);
+}
+restore_snan_bit_mode(env);
+}
 }
 #elif defined(TARGET_OPENRISC)
 {
diff --git a/target-mips/cpu.h b/target-mips/cpu.h
index 6478420..bbf81c7 100644
--- a/target-mips/cpu.h
+++ b/target-mips/cpu.h
@@ -110,9 +110,71 @@ struct CPUMIPSFPUContext {
 #define FCR0_PRID 8
 #define FCR0_REV 0
 /* fcsr */
+uint32_t fcr31_rw_bitmask;
 uint32_t fcr31;
-#define FCR31_ABS2008 19
-#define FCR31_NAN2008 18
+#define FCR31_ROUNDING_MODE_0 0
+#define FCR31_ROUNDING_MODE_1 1
+#define FCR31_FLAGS_INEXACT   2
+#define FCR31_FLAGS_UNDERFLOW 3
+#define FCR31_FLAGS_OVERFLOW  4
+#define FCR31_FLAGS_DIV0  5
+#define FCR31_FLAGS_INVALID   6
+#define FCR31_ENABLE_INEXACT  7
+#define FCR31_ENABLE_UNDERFLOW8
+#define FCR31_ENABLE_OVERFLOW 9
+#define FCR31_ENABLE_DIV0 10
+#define FCR31_ENABLE_INVALID  11
+#define FCR31_CAUSE_INEXACT   12
+#define FCR31_CAUSE_UNDERFLOW 13
+#define FCR31_CAUSE_OVERFLOW  14
+#define FCR31_CAUSE_DIV0  15
+#define FCR31_CAUSE_INVALID   16
+#define FCR31_CAUSE_UNIMPLEMENTED 17
+#define FCR31_NAN2008 18
+#define FCR31_ABS2008 19
+#define FCR31_0   20
+#define FCR31_IMPL_0  21
+#define FCR31_IMPL_1  22
+#define FCR31_FCC_COND23
+#define FCR31_FS  24
+#define FCR31_FCC_COND_1  25
+#define FCR31_FCC_COND_2  26
+#define FCR31_FCC_COND_3  27
+#define FCR31_FCC_COND_4  28
+#define FCR31_FCC_COND_5  29
+#define FCR31_FCC_COND_6  30
+#define FCR31_FCC_COND_7  31
+#define FCR31_ROUNDING_MODE_MASK  ((1 << FCR31_ROUNDING_MODE_0) | \
+   (1 << FCR31_ROUNDING_MODE_1))
+#define FCR31_FLAGS_MASK  ((1 << FCR31_FLAGS_INEXACT) | \
+   (1 << FCR31_FLAGS_UNDERFLOW) | \
+   (1 << FCR31_FLAGS_OVERFLOW) | \
+   (1 << FCR31_FLAGS_DIV0) | \
+   (1 << FCR31_FLAGS_INVALID))
+#define FCR31_ENABLE_MASK ((1 << FCR31_ENABLE_INEXACT) | \
+   (1 << FCR31_ENABLE_UNDERFLOW) | \
+   (1 << FCR31_ENABLE_OVERFLOW) | \
+   (1 << FCR31_ENABLE_DIV0) | \
+   (1 << FCR31_ENABLE_INVALID))
+#define FCR31_CAUSE_MASK  ((1 <<