[Qemu-devel] [PATCH v2 0/6] Fix decoding mechanisms of the R5900

[Qemu-devel] [PATCH v2 0/6] Fix decoding mechanisms of the R5900

[Fredrik Noring]

This series amends the R5900 support with the following changes:

- MFLO1, MFHI1, MTLO1 and MTHI1 are generated in gen_HILO1_tx79 instead
  of the generic gen_HILO.

- DIV1 and DIVU1 are generated in gen_div1_tx79 instead of the generic
  gen_muldiv.

- MOVN, MOVZ, MFHI, MFLO, MTHI, MTLO, MULT, MULTU, DIV, DIVU, DMULT,
  DMULTU, DDIV, DDIVU and JR are decoded in decode_opc_special_tx79
  instead of the generic decode_opc_special_legacy.

- Guard check_insn_opc_user_only with INSN_R5900 check.

- Guard check_insn with INSN_R5900 check.

- Fix HI[ac] and LO[ac] 32-bit truncation with the MIPS64 DSP ASE.

This series has been successfully built with the 8 different build
configurations

    {gcc,clang} x -m64 x mips{,64}el-{linux-user,softmmu}

in addition successfully completing the R5900 test suite

    cd tests/tcg/mips/mipsr5900 && make check

Changes in v2:
- Fix HI and LO 32-bit truncation with the MIPS64 DSP ASE
- Decode special R5900 opcodes in decode_opc_special_tx79
- Guard check_insn_opc_user_only with INSN_R5900 check
- Guard check_insn with INSN_R5900 check

Fredrik Noring (6):
  target/mips: Fix decoding mechanism of R5900 MFLO1, MFHI1, MTLO1 and MTHI1
  target/mips: Fix decoding mechanism of R5900 DIV1 and DIVU1
  target/mips: Fix HI[ac] and LO[ac] 32-bit truncation with MIPS64 DSP ASE
  target/mips: Fix decoding mechanism of special R5900 opcodes
  target/mips: Guard check_insn_opc_user_only with INSN_R5900 check
  target/mips: Guard check_insn with INSN_R5900 check

 target/mips/translate.c | 229 +++++++++++++++++++++++++++++-----------
 1 file changed, 170 insertions(+), 59 deletions(-)

-- 
2.18.1

[Qemu-devel] [PATCH v2 1/6] target/mips: Fix decoding mechanism of R5900 MFLO1, MFHI1, MTLO1 and MTHI1

[Fredrik Noring]

MFLO1, MFHI1, MTLO1 and MTHI1 are generated in gen_HILO1_tx79 instead of
the generic gen_HILO.

Signed-off-by: Fredrik Noring <noring@nocrew.org>
---
 target/mips/translate.c | 51 ++++++++++++++++++++++++++++++++---------
 1 file changed, 40 insertions(+), 11 deletions(-)

diff --git a/target/mips/translate.c b/target/mips/translate.c
index 60320cbe69..8601333554 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -4359,24 +4359,56 @@ static void gen_shift(DisasContext *ctx, uint32_t opc,
     tcg_temp_free(t1);
 }
 
+/* Copy GPR to and from TX79 HI1/LO1 register. */
+static void gen_HILO1_tx79(DisasContext *ctx, uint32_t opc, int reg)
+{
+    if (reg == 0 && (opc == TX79_MMI_MFHI1 || opc == TX79_MMI_MFLO1)) {
+        /* Treat as NOP. */
+        return;
+    }
+
+    switch (opc) {
+    case TX79_MMI_MFHI1:
+        tcg_gen_mov_tl(cpu_gpr[reg], cpu_HI[1]);
+        break;
+    case TX79_MMI_MFLO1:
+        tcg_gen_mov_tl(cpu_gpr[reg], cpu_LO[1]);
+        break;
+    case TX79_MMI_MTHI1:
+        if (reg != 0) {
+            tcg_gen_mov_tl(cpu_HI[1], cpu_gpr[reg]);
+        } else {
+            tcg_gen_movi_tl(cpu_HI[1], 0);
+        }
+        break;
+    case TX79_MMI_MTLO1:
+        if (reg != 0) {
+            tcg_gen_mov_tl(cpu_LO[1], cpu_gpr[reg]);
+        } else {
+            tcg_gen_movi_tl(cpu_LO[1], 0);
+        }
+        break;
+    default:
+        MIPS_INVAL("mfthilo1 TX79");
+        generate_exception_end(ctx, EXCP_RI);
+        break;
+    }
+}
+
 /* Arithmetic on HI/LO registers */
 static void gen_HILO(DisasContext *ctx, uint32_t opc, int acc, int reg)
 {
-    if (reg == 0 && (opc == OPC_MFHI || opc == TX79_MMI_MFHI1 ||
-                     opc == OPC_MFLO || opc == TX79_MMI_MFLO1)) {
+    if (reg == 0 && (opc == OPC_MFHI || opc == OPC_MFLO)) {
         /* Treat as NOP. */
         return;
     }
 
     if (acc != 0) {
-        if (!(ctx->insn_flags & INSN_R5900)) {
-            check_dsp(ctx);
-        }
+        check_dsp(ctx);
     }
 
     switch (opc) {
     case OPC_MFHI:
-    case TX79_MMI_MFHI1:
 #if defined(TARGET_MIPS64)
         if (acc != 0) {
             tcg_gen_ext32s_tl(cpu_gpr[reg], cpu_HI[acc]);
@@ -4387,7 +4419,6 @@ static void gen_HILO(DisasContext *ctx, uint32_t opc, int acc, int reg)
         }
         break;
     case OPC_MFLO:
-    case TX79_MMI_MFLO1:
 #if defined(TARGET_MIPS64)
         if (acc != 0) {
             tcg_gen_ext32s_tl(cpu_gpr[reg], cpu_LO[acc]);
@@ -4398,7 +4429,6 @@ static void gen_HILO(DisasContext *ctx, uint32_t opc, int acc, int reg)
         }
         break;
     case OPC_MTHI:
-    case TX79_MMI_MTHI1:
         if (reg != 0) {
 #if defined(TARGET_MIPS64)
             if (acc != 0) {
@@ -4413,7 +4443,6 @@ static void gen_HILO(DisasContext *ctx, uint32_t opc, int acc, int reg)
         }
         break;
     case OPC_MTLO:
-    case TX79_MMI_MTLO1:
         if (reg != 0) {
 #if defined(TARGET_MIPS64)
             if (acc != 0) {
@@ -26500,11 +26529,11 @@ static void decode_tx79_mmi(CPUMIPSState *env, DisasContext *ctx)
         break;
     case TX79_MMI_MTLO1:
     case TX79_MMI_MTHI1:
-        gen_HILO(ctx, opc, 1, rs);
+        gen_HILO1_tx79(ctx, opc, rs);
         break;
     case TX79_MMI_MFLO1:
     case TX79_MMI_MFHI1:
-        gen_HILO(ctx, opc, 1, rd);
+        gen_HILO1_tx79(ctx, opc, rd);
         break;
     case TX79_MMI_MADD:          /* TODO: TX79_MMI_MADD */
     case TX79_MMI_MADDU:         /* TODO: TX79_MMI_MADDU */
-- 
2.18.1

Re: [Qemu-devel] [PATCH v2 1/6] target/mips: Fix decoding mechanism of R5900 MFLO1, MFHI1, MTLO1 and MTHI1

[Aleksandar Markovic]

> From: Fredrik Noring <noring@nocrew.org>
> Subject: [PATCH v2 1/6] target/mips: Fix decoding mechanism of R5900 MFLO1, MFHI1, MTLO1 and MTHI1

Reviewed-by: Aleksandar Markovic <amarkovic@wavecomp.com>

[Qemu-devel] [PATCH v2 2/6] target/mips: Fix decoding mechanism of R5900 DIV1 and DIVU1

[Fredrik Noring]

DIV1 and DIVU1 are generated in gen_div1_tx79 instead of the generic
gen_muldiv.

Signed-off-by: Fredrik Noring <noring@nocrew.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 target/mips/translate.c | 65 +++++++++++++++++++++++++++++++++++++----
 1 file changed, 59 insertions(+), 6 deletions(-)

diff --git a/target/mips/translate.c b/target/mips/translate.c
index 8601333554..3ddd70043a 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -4743,6 +4743,63 @@ static void gen_r6_muldiv(DisasContext *ctx, int opc, int rd, int rs, int rt)
     tcg_temp_free(t1);
 }
 
+static void gen_div1_tx79(DisasContext *ctx, uint32_t opc, int rs, int rt)
+{
+    TCGv t0, t1;
+
+    t0 = tcg_temp_new();
+    t1 = tcg_temp_new();
+
+    gen_load_gpr(t0, rs);
+    gen_load_gpr(t1, rt);
+
+    switch (opc) {
+    case TX79_MMI_DIV1:
+        {
+            TCGv t2 = tcg_temp_new();
+            TCGv t3 = tcg_temp_new();
+            tcg_gen_ext32s_tl(t0, t0);
+            tcg_gen_ext32s_tl(t1, t1);
+            tcg_gen_setcondi_tl(TCG_COND_EQ, t2, t0, INT_MIN);
+            tcg_gen_setcondi_tl(TCG_COND_EQ, t3, t1, -1);
+            tcg_gen_and_tl(t2, t2, t3);
+            tcg_gen_setcondi_tl(TCG_COND_EQ, t3, t1, 0);
+            tcg_gen_or_tl(t2, t2, t3);
+            tcg_gen_movi_tl(t3, 0);
+            tcg_gen_movcond_tl(TCG_COND_NE, t1, t2, t3, t2, t1);
+            tcg_gen_div_tl(cpu_LO[1], t0, t1);
+            tcg_gen_rem_tl(cpu_HI[1], t0, t1);
+            tcg_gen_ext32s_tl(cpu_LO[1], cpu_LO[1]);
+            tcg_gen_ext32s_tl(cpu_HI[1], cpu_HI[1]);
+            tcg_temp_free(t3);
+            tcg_temp_free(t2);
+        }
+        break;
+    case TX79_MMI_DIVU1:
+        {
+            TCGv t2 = tcg_const_tl(0);
+            TCGv t3 = tcg_const_tl(1);
+            tcg_gen_ext32u_tl(t0, t0);
+            tcg_gen_ext32u_tl(t1, t1);
+            tcg_gen_movcond_tl(TCG_COND_EQ, t1, t1, t2, t3, t1);
+            tcg_gen_divu_tl(cpu_LO[1], t0, t1);
+            tcg_gen_remu_tl(cpu_HI[1], t0, t1);
+            tcg_gen_ext32s_tl(cpu_LO[1], cpu_LO[1]);
+            tcg_gen_ext32s_tl(cpu_HI[1], cpu_HI[1]);
+            tcg_temp_free(t3);
+            tcg_temp_free(t2);
+        }
+        break;
+    default:
+        MIPS_INVAL("div1 TX79");
+        generate_exception_end(ctx, EXCP_RI);
+        goto out;
+    }
+ out:
+    tcg_temp_free(t0);
+    tcg_temp_free(t1);
+}
+
 static void gen_muldiv(DisasContext *ctx, uint32_t opc,
                        int acc, int rs, int rt)
 {
@@ -4755,14 +4812,11 @@ static void gen_muldiv(DisasContext *ctx, uint32_t opc,
     gen_load_gpr(t1, rt);
 
     if (acc != 0) {
-        if (!(ctx->insn_flags & INSN_R5900)) {
-            check_dsp(ctx);
-        }
+        check_dsp(ctx);
     }
 
     switch (opc) {
     case OPC_DIV:
-    case TX79_MMI_DIV1:
         {
             TCGv t2 = tcg_temp_new();
             TCGv t3 = tcg_temp_new();
@@ -4784,7 +4838,6 @@ static void gen_muldiv(DisasContext *ctx, uint32_t opc,
         }
         break;
     case OPC_DIVU:
-    case TX79_MMI_DIVU1:
         {
             TCGv t2 = tcg_const_tl(0);
             TCGv t3 = tcg_const_tl(1);
@@ -26525,7 +26578,7 @@ static void decode_tx79_mmi(CPUMIPSState *env, DisasContext *ctx)
         break;
     case TX79_MMI_DIV1:
     case TX79_MMI_DIVU1:
-        gen_muldiv(ctx, opc, 1, rs, rt);
+        gen_div1_tx79(ctx, opc, rs, rt);
         break;
     case TX79_MMI_MTLO1:
     case TX79_MMI_MTHI1:
-- 
2.18.1

[Qemu-devel] [PATCH v2 3/6] target/mips: Fix HI[ac] and LO[ac] 32-bit truncation with MIPS64 DSP ASE

[Fredrik Noring]

This change removes the 32-bit truncation of the HI[ac] and LO[ac]
special purpose registers when ac range from 1 to 3 for the instructions
MFHI, MFLO, MTHI and MTLO. The "MIPS Architecture for Programmers Volume
IV-e: MIPS DSP Module for MIPS64 Architecture" manual specifies that all
64 bits are copied in all cases:

   MFHI: GPR[rd]63..0 <- HI[ac]63..0
   MFLO: GPR[rd]63..0 <- LO[ac]63..0
   MTHI: HI[ac]63..0 <- GPR[rs]63..0
   MTLO: LO[ac]63..0 <- GPR[rs]63..0

Fixes: 4133498f8e53 ("Use correct acc value to index cpu_HI/cpu_LO rather than using a fix number")
Cc: Jia Liu <proljc@gmail.com>
Reported-by: Maciej W. Rozycki <macro@linux-mips.org>
Signed-off-by: Fredrik Noring <noring@nocrew.org>
---
 target/mips/translate.c | 36 ++++--------------------------------
 1 file changed, 4 insertions(+), 32 deletions(-)

diff --git a/target/mips/translate.c b/target/mips/translate.c
index 3ddd70043a..19ae7d2f1c 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -4409,49 +4409,21 @@ static void gen_HILO(DisasContext *ctx, uint32_t opc, int acc, int reg)
 
     switch (opc) {
     case OPC_MFHI:
-#if defined(TARGET_MIPS64)
-        if (acc != 0) {
-            tcg_gen_ext32s_tl(cpu_gpr[reg], cpu_HI[acc]);
-        } else
-#endif
-        {
-            tcg_gen_mov_tl(cpu_gpr[reg], cpu_HI[acc]);
-        }
+        tcg_gen_mov_tl(cpu_gpr[reg], cpu_HI[acc]);
         break;
     case OPC_MFLO:
-#if defined(TARGET_MIPS64)
-        if (acc != 0) {
-            tcg_gen_ext32s_tl(cpu_gpr[reg], cpu_LO[acc]);
-        } else
-#endif
-        {
-            tcg_gen_mov_tl(cpu_gpr[reg], cpu_LO[acc]);
-        }
+        tcg_gen_mov_tl(cpu_gpr[reg], cpu_LO[acc]);
         break;
     case OPC_MTHI:
         if (reg != 0) {
-#if defined(TARGET_MIPS64)
-            if (acc != 0) {
-                tcg_gen_ext32s_tl(cpu_HI[acc], cpu_gpr[reg]);
-            } else
-#endif
-            {
-                tcg_gen_mov_tl(cpu_HI[acc], cpu_gpr[reg]);
-            }
+            tcg_gen_mov_tl(cpu_HI[acc], cpu_gpr[reg]);
         } else {
             tcg_gen_movi_tl(cpu_HI[acc], 0);
         }
         break;
     case OPC_MTLO:
         if (reg != 0) {
-#if defined(TARGET_MIPS64)
-            if (acc != 0) {
-                tcg_gen_ext32s_tl(cpu_LO[acc], cpu_gpr[reg]);
-            } else
-#endif
-            {
-                tcg_gen_mov_tl(cpu_LO[acc], cpu_gpr[reg]);
-            }
+            tcg_gen_mov_tl(cpu_LO[acc], cpu_gpr[reg]);
         } else {
             tcg_gen_movi_tl(cpu_LO[acc], 0);
         }
-- 
2.18.1

Re: [Qemu-devel] [PATCH v2 3/6] target/mips: Fix HI[ac] and LO[ac] 32-bit truncation with MIPS64 DSP ASE

[Aleksandar Markovic]

> From: Fredrik Noring <noring@nocrew.org>
> Sent: Wednesday, November 7, 2018 8:18 PM
> To: Aleksandar Markovic; Aurelien Jarno; Philippe Mathieu-Daudé; Richard Henderson
> Cc: Jürgen Urban; Maciej W. Rozycki; Jia Liu; qemu-devel@nongnu.org
> Subject: [PATCH v2 3/6] target/mips: Fix HI[ac] and LO[ac] 32-bit truncation with > MIPS64 DSP ASE
> 
> This change removes the 32-bit truncation of the HI[ac] and LO[ac]
> special purpose registers when ac range from 1 to 3 for the instructions
> MFHI, MFLO, MTHI and MTLO. The "MIPS Architecture for Programmers Volume
> IV-e: MIPS DSP Module for MIPS64 Architecture" manual specifies that all
> 64 bits are copied in all cases:
> 
>    MFHI: GPR[rd]63..0 <- HI[ac]63..0
>    MFLO: GPR[rd]63..0 <- LO[ac]63..0
>    MTHI: HI[ac]63..0 <- GPR[rs]63..0
>    MTLO: LO[ac]63..0 <- GPR[rs]63..0
> 
> Fixes: 4133498f8e53 ("Use correct acc value to index cpu_HI/cpu_LO rather than using > a fix number")
> Cc: Jia Liu <proljc@gmail.com>
> Reported-by: Maciej W. Rozycki <macro@linux-mips.org>
> Signed-off-by: Fredrik Noring <noring@nocrew.org>
> ---
>  target/mips/translate.c | 36 ++++--------------------------------
>  1 file changed, 4 insertions(+), 32 deletions(-)
> 

Hi, Fredrik.

Thanks for this follow-up patch. I will consider it, even if I perhaps don't include it for 3.1. Let's see if Jia has some comments.

Aleksandar

> diff --git a/target/mips/translate.c b/target/mips/translate.c
> index 3ddd70043a..19ae7d2f1c 100644
> --- a/target/mips/translate.c
> +++ b/target/mips/translate.c
> @@ -4409,49 +4409,21 @@ static void gen_HILO(DisasContext *ctx, uint32_t opc, int > acc, int reg)
> 
>      switch (opc) {
>      case OPC_MFHI:
> -#if defined(TARGET_MIPS64)
> -        if (acc != 0) {
> -            tcg_gen_ext32s_tl(cpu_gpr[reg], cpu_HI[acc]);
> -        } else
> -#endif
> -        {
> -            tcg_gen_mov_tl(cpu_gpr[reg], cpu_HI[acc]);
> -        }
> +        tcg_gen_mov_tl(cpu_gpr[reg], cpu_HI[acc]);
>          break;
>      case OPC_MFLO:
> -#if defined(TARGET_MIPS64)
> -        if (acc != 0) {
> -            tcg_gen_ext32s_tl(cpu_gpr[reg], cpu_LO[acc]);
> -        } else
> -#endif
> -        {
> -            tcg_gen_mov_tl(cpu_gpr[reg], cpu_LO[acc]);
> -        }
> +        tcg_gen_mov_tl(cpu_gpr[reg], cpu_LO[acc]);
>          break;
>      case OPC_MTHI:
>          if (reg != 0) {
> -#if defined(TARGET_MIPS64)
> -            if (acc != 0) {
> -                tcg_gen_ext32s_tl(cpu_HI[acc], cpu_gpr[reg]);
> -            } else
> -#endif
> -            {
> -                tcg_gen_mov_tl(cpu_HI[acc], cpu_gpr[reg]);
> -            }
> +            tcg_gen_mov_tl(cpu_HI[acc], cpu_gpr[reg]);
>          } else {
>              tcg_gen_movi_tl(cpu_HI[acc], 0);
>          }
>          break;
>      case OPC_MTLO:
>          if (reg != 0) {
> -#if defined(TARGET_MIPS64)
> -            if (acc != 0) {
> -                tcg_gen_ext32s_tl(cpu_LO[acc], cpu_gpr[reg]);
> -            } else
> -#endif
> -            {
> -                tcg_gen_mov_tl(cpu_LO[acc], cpu_gpr[reg]);
> -            }
> +            tcg_gen_mov_tl(cpu_LO[acc], cpu_gpr[reg]);
>          } else {
>              tcg_gen_movi_tl(cpu_LO[acc], 0);
>          }
> --
> 2.18.1
> 
> 

[Qemu-devel] [PATCH v2 4/6] target/mips: Fix decoding mechanism of special R5900 opcodes

[Fredrik Noring]

MOVN, MOVZ, MFHI, MFLO, MTHI, MTLO, MULT, MULTU, DIV, DIVU, DMULT,
DMULTU, DDIV, DDIVU and JR are decoded in decode_opc_special_tx79
instead of the generic decode_opc_special_legacy.

Signed-off-by: Fredrik Noring <noring@nocrew.org>
---
 target/mips/translate.c | 54 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 50 insertions(+), 4 deletions(-)

diff --git a/target/mips/translate.c b/target/mips/translate.c
index 19ae7d2f1c..45ad70c097 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -23835,6 +23835,53 @@ static void decode_opc_special_r6(CPUMIPSState *env, DisasContext *ctx)
     }
 }
 
+static void decode_opc_special_tx79(CPUMIPSState *env, DisasContext *ctx)
+{
+    int rs = extract32(ctx->opcode, 21, 5);
+    int rt = extract32(ctx->opcode, 16, 5);
+    int rd = extract32(ctx->opcode, 11, 5);
+    uint32_t op1 = MASK_SPECIAL(ctx->opcode);
+
+    switch (op1) {
+    case OPC_MOVN:         /* Conditional move */
+    case OPC_MOVZ:
+        gen_cond_move(ctx, op1, rd, rs, rt);
+        break;
+    case OPC_MFHI:          /* Move from HI/LO */
+    case OPC_MFLO:
+        gen_HILO(ctx, op1, 0, rd);
+        break;
+    case OPC_MTHI:
+    case OPC_MTLO:          /* Move to HI/LO */
+        gen_HILO(ctx, op1, 0, rs);
+        break;
+    case OPC_MULT:
+    case OPC_MULTU:
+        gen_mul_txx9(ctx, op1, rd, rs, rt);
+        break;
+    case OPC_DIV:
+    case OPC_DIVU:
+        gen_muldiv(ctx, op1, 0, rs, rt);
+        break;
+#if defined(TARGET_MIPS64)
+    case OPC_DMULT:
+    case OPC_DMULTU:
+    case OPC_DDIV:
+    case OPC_DDIVU:
+        check_insn_opc_user_only(ctx, INSN_R5900);
+        gen_muldiv(ctx, op1, 0, rs, rt);
+        break;
+#endif
+    case OPC_JR:
+        gen_compute_branch(ctx, op1, 4, rs, 0, 0, 4);
+        break;
+    default:            /* Invalid */
+        MIPS_INVAL("special_tx79");
+        generate_exception_end(ctx, EXCP_RI);
+        break;
+    }
+}
+
 static void decode_opc_special_legacy(CPUMIPSState *env, DisasContext *ctx)
 {
     int rs, rt, rd, sa;
@@ -23850,7 +23897,7 @@ static void decode_opc_special_legacy(CPUMIPSState *env, DisasContext *ctx)
     case OPC_MOVN:         /* Conditional move */
     case OPC_MOVZ:
         check_insn(ctx, ISA_MIPS4 | ISA_MIPS32 |
-                   INSN_LOONGSON2E | INSN_LOONGSON2F | INSN_R5900);
+                   INSN_LOONGSON2E | INSN_LOONGSON2F);
         gen_cond_move(ctx, op1, rd, rs, rt);
         break;
     case OPC_MFHI:          /* Move from HI/LO */
@@ -23877,8 +23924,6 @@ static void decode_opc_special_legacy(CPUMIPSState *env, DisasContext *ctx)
             check_insn(ctx, INSN_VR54XX);
             op1 = MASK_MUL_VR54XX(ctx->opcode);
             gen_mul_vr54xx(ctx, op1, rd, rs, rt);
-        } else if (ctx->insn_flags & INSN_R5900) {
-            gen_mul_txx9(ctx, op1, rd, rs, rt);
         } else {
             gen_muldiv(ctx, op1, rd & 3, rs, rt);
         }
@@ -23893,7 +23938,6 @@ static void decode_opc_special_legacy(CPUMIPSState *env, DisasContext *ctx)
     case OPC_DDIV:
     case OPC_DDIVU:
         check_insn(ctx, ISA_MIPS3);
-        check_insn_opc_user_only(ctx, INSN_R5900);
         check_mips_64(ctx);
         gen_muldiv(ctx, op1, 0, rs, rt);
         break;
@@ -24120,6 +24164,8 @@ static void decode_opc_special(CPUMIPSState *env, DisasContext *ctx)
     default:
         if (ctx->insn_flags & ISA_MIPS32R6) {
             decode_opc_special_r6(env, ctx);
+        } else if (ctx->insn_flags & INSN_R5900) {
+            decode_opc_special_tx79(env, ctx);
         } else {
             decode_opc_special_legacy(env, ctx);
         }
-- 
2.18.1

Re: [Qemu-devel] [PATCH v2 4/6] target/mips: Fix decoding mechanism of special R5900 opcodes

[Aleksandar Markovic]

> 
> From: Fredrik Noring <noring@nocrew.org>
> Subject: [PATCH v2 4/6] target/mips: Fix decoding mechanism of special R5900 opcodes
> 
> MOVN, MOVZ, MFHI, MFLO, MTHI, MTLO, MULT, MULTU, DIV, DIVU, DMULT,
> DMULTU, DDIV, DDIVU and JR are decoded in decode_opc_special_tx79
> instead of the generic decode_opc_special_legacy.
> 
> Signed-off-by: Fredrik Noring <noring@nocrew.org>
> ---
>  target/mips/translate.c | 54 ++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 50 insertions(+), 4 deletions(-)
> 

> +#if defined(TARGET_MIPS64)
> +    case OPC_DMULT:
> +    case OPC_DMULTU:
> +    case OPC_DDIV:
> +    case OPC_DDIVU:
> +        check_insn_opc_user_only(ctx, INSN_R5900);
> +        gen_muldiv(ctx, op1, 0, rs, rt);
> +        break;
> +#endif

Fredrik, do you know by any chance if a document exists that would justify inclusion of non-R5900 DMULT, DMULTU, DDIV, DDIVU in R5900 executables by gcc for R5900? Is it included by cross-gcc or by native gcc, or by both?

I think gcc folks must have had a good reason for that, some kind of design - it can't be 'I really like/miss this instruction, let's include it...'

Thanks,
Aleksandar

Re: [Qemu-devel] [PATCH v2 4/6] target/mips: Fix decoding mechanism of special R5900 opcodes

[Fredrik Noring]

Hi Aleksandar,

> Fredrik, do you know by any chance if a document exists that would justify
> inclusion of non-R5900 DMULT, DMULTU, DDIV, DDIVU in R5900 executables by
> gcc for R5900? Is it included by cross-gcc or by native gcc, or by both?
> 
> I think gcc folks must have had a good reason for that, some kind of
> design - it can't be 'I really like/miss this instruction, let's include
> it...'

The R5900 reports itself as MIPS III and DMULT, DMULTU, DDIV and DDIVU
are part of the MIPS III ISA. They are emulated in user mode to support
generic MIPS III programs.

I have now obtained an R5900 n32 ABI toolchain. R5900 n32 ABI emulation
support is recognised with

http://lists.nongnu.org/archive/html/qemu-devel/2018-11/msg01609.html

and a test of DMULT emulation is available with

http://lists.nongnu.org/archive/html/qemu-devel/2018-11/msg01610.html

Fredrik

Re: [Qemu-devel] [PATCH v2 4/6] target/mips: Fix decoding mechanism of special R5900 opcodes

[Maciej W. Rozycki]

On Thu, 8 Nov 2018, Fredrik Noring wrote:

> > Fredrik, do you know by any chance if a document exists that would justify
> > inclusion of non-R5900 DMULT, DMULTU, DDIV, DDIVU in R5900 executables by
> > gcc for R5900? Is it included by cross-gcc or by native gcc, or by both?
> > 
> > I think gcc folks must have had a good reason for that, some kind of
> > design - it can't be 'I really like/miss this instruction, let's include
> > it...'
> 
> The R5900 reports itself as MIPS III and DMULT, DMULTU, DDIV and DDIVU
> are part of the MIPS III ISA. They are emulated in user mode to support
> generic MIPS III programs.

 FAOD, GCC does not emit these instructions if the R5900 architecture has 
been selected for compilation, e.g.:

/* ISA supports instructions DMULT and DMULTU. */
#define ISA_HAS_DMULT		(TARGET_64BIT				\
				 && !TARGET_MIPS5900			\
				 && mips_isa_rev <= 5)

however they are a part of the base 64-bit MIPS Linux user psABI, which is 
the whole of the MIPS III ISA, so the runtime has to support them one way 
or another (just like LL, SC and SYNC are a part of the 32-bit MIPS Linux 
user psABI even though they are not supported by MIPS I hardware).

  Maciej

Re: [Qemu-devel] [PATCH v2 4/6] target/mips: Fix decoding mechanism of special R5900 opcodes

[Aleksandar Markovic]

> From: Fredrik Noring <noring@nocrew.org>
> Subject: Re: [PATCH v2 4/6] target/mips: Fix decoding mechanism of special R5900 opcodes
> 
> Hi Aleksandar,
> 
> > Fredrik, do you know by any chance if a document exists that would justify
> > inclusion of non-R5900 DMULT, DMULTU, DDIV, DDIVU in R5900 executables by
> > gcc for R5900? Is it included by cross-gcc or by native gcc, or by both?
> >
> > I think gcc folks must have had a good reason for that, some kind of
> > design - it can't be 'I really like/miss this instruction, let's include
> > it...'
> 
> The R5900 reports itself as MIPS III ...

This is very unclear. What do you mean by this? How does R5900 do that? I can't find any trace of such intentions in R5900 docs.

> ... and DMULT, DMULTU, DDIV and DDIVU
> are part of the MIPS III ISA. They are emulated in user mode to support
> generic MIPS III programs.

Pure MIPS III executables should not be a concern of the R5900 emulation, but R5900 executables.

Could you please provide a document that would justify inclusion of these non-R5900 instruction in an R5900 emulation?

Thanks,
Aleksandar

Re: [Qemu-devel] [PATCH v2 4/6] target/mips: Fix decoding mechanism of special R5900 opcodes

[Fredrik Noring]

Hi Aleksandar,

> > The R5900 reports itself as MIPS III ...
> 
> This is very unclear. What do you mean by this? How does R5900 do that? I
> can't find any trace of such intentions in R5900 docs.

In QEMU, we have previously defined the R5900 as MIPS III by

#define CPU_R5900 (CPU_MIPS3 | INSN_R5900)

by referring to the "Toshiba TX System RISC TX79 Core Architecture" manual

https://wiki.qemu.org/File:C790.pdf

It says in the introduction, for example, that

	3.1 Introduction

	The C790 supports all MIPS III instructions with the exception of
	64-bit multiply, 64-bit divide, Load Linked and Store Conditional
	instructions. It also supports a limited number of MIPS IV
	instructions and additional C790-specific instructions, such as
	Multiply/Add instructions and multimedia instructions.

The manual mentions MIPS III in several other places, and appendix A-1
"CPU Instruction Set Details" describes the implemented MIPS III subset.

> > ... and DMULT, DMULTU, DDIV and DDIVU
> > are part of the MIPS III ISA. They are emulated in user mode to support
> > generic MIPS III programs.
> 
> Pure MIPS III executables should not be a concern of the R5900 emulation,
> but R5900 executables.

Many Linux distributions rely on precompiled packages, and they are often
made to work with a broad range of hardware. This is done in part by
choosing a common denominator, such as MIPS III. Compatibility with other
implementations means that the R5900 more easily can be part of the MIPS
Linux ecosystem.

In contrast, Gentoo Linux mainly builds its packages locally from source,
often optimised for the hardware, such as the R5900. Even so, Gentoo does
rely on a small set of precompiled "stage 3" packages for the installation,
and so the argument above still applies.

> Could you please provide a document that would justify inclusion of these
> non-R5900 instruction in an R5900 emulation?

Would you accept the TX79 manual mentioned above as such a document?

Fredrik

Re: [Qemu-devel] [PATCH v2 4/6] target/mips: Fix decoding mechanism of special R5900 opcodes

[Aleksandar Markovic]

> From: Fredrik Noring <noring@nocrew.org>
> Subject: Re: [PATCH v2 4/6] target/mips: Fix decoding mechanism of special R5900 opcodes
> 
> >
> > Could you please provide a document that would justify inclusion of these
> > non-R5900 instruction in an R5900 emulation?
> 
> Would you accept the TX79 manual mentioned above as such a document?
>

Tx79 mentions the opposite: that DDIV, DDIVU, DMULT, DMULTU are not included in R5900 set.

I think that the best solution that you exclude DDIV, DDIVU, DMULT, DMULTU in a separate patch - there is no document to support their inclusion.

Aleksandar

Re: [Qemu-devel] [PATCH v2 4/6] target/mips: Fix decoding mechanism of special R5900 opcodes

[Fredrik Noring]

Hi Aleksandar,

> Tx79 mentions the opposite: that DDIV, DDIVU, DMULT, DMULTU are not
> included in R5900 set.
> 
> I think that the best solution that you exclude DDIV, DDIVU, DMULT, DMULTU
> in a separate patch - there is no document to support their inclusion.

As Maciej noted, the 64-bit MIPS Linux psABI is indivisible, so how could
your alternative possibly work?

Fredrik

Re: [Qemu-devel] [PATCH v2 4/6] target/mips: Fix decoding mechanism of special R5900 opcodes

[Aleksandar Markovic]

> From: Fredrik Noring <noring@nocrew.org>
> Subject: Re: [PATCH v2 4/6] target/mips: Fix decoding mechanism of special R5900 opcodes
> 
> Hi Aleksandar,
> 
> > Tx79 mentions the opposite: that DDIV, DDIVU, DMULT, DMULTU are not
> > included in R5900 set.
> >
> > I think that the best solution that you exclude DDIV, DDIVU, DMULT, DMULTU
> > in a separate patch - there is no document to support their inclusion.
> 
> As Maciej noted, the 64-bit MIPS Linux psABI is indivisible, so how could
> your alternative possibly work?

Provide the document, and then let's talk.

Thanks,
Aleksandar

Re: [Qemu-devel] [PATCH v2 4/6] target/mips: Fix decoding mechanism of special R5900 opcodes

[Aleksandar Markovic]

> From: Fredrik Noring <noring@nocrew.org>
> Subject: Re: [PATCH v2 4/6] target/mips: Fix decoding mechanism of special R5900 opcodes
>
> Hi Aleksandar,
>
> > Tx79 mentions the opposite: that DDIV, DDIVU, DMULT, DMULTU are not
> > included in R5900 set.
> >
> > I think that the best solution that you exclude DDIV, DDIVU, DMULT, DMULTU
> > in a separate patch - there is no document to support their inclusion.
>
> As Maciej noted, the 64-bit MIPS Linux psABI is indivisible, so how could
> your alternative possibly work?

Since we are rapidly approaching 3.1 release, we don't have time for prolonged discussions - so please provide the patch that removes emulation of these instructions that don't belong to R5900 set, and, if you find a justification document later on, they can be reintroduced in 3.1+ timeframe.

Thanks,
Aleksnadar

Re: [Qemu-devel] [PATCH v2 4/6] target/mips: Fix decoding mechanism of special R5900 opcodes

[Maciej W. Rozycki]

On Fri, 9 Nov 2018, Aleksandar Markovic wrote:

> > > I think that the best solution that you exclude DDIV, DDIVU, DMULT, DMULTU
> > > in a separate patch - there is no document to support their inclusion.
> >
> > As Maciej noted, the 64-bit MIPS Linux psABI is indivisible, so how could
> > your alternative possibly work?
> 
> Since we are rapidly approaching 3.1 release, we don't have time for 
> prolonged discussions - so please provide the patch that removes 
> emulation of these instructions that don't belong to R5900 set, and, if 
> you find a justification document later on, they can be reintroduced in 
> 3.1+ timeframe.

 You know well enough that nobody was bothered over the years to actually 
document the 64-bit MIPS Linux psABI (there is the 64-bit ELF document 
from SGI, relevant for IRIX, which has been partially implemented by 
Linux) and even when it comes to the 32-bit psABI the only document is 
from SGI from mid 1990s, that has several errors and surely was not 
written with Linux in mind (and FWIW not entirely with IRIX either).

 The psABI has been set by the architecture back in 1991 and what Linux 
has implemented on top of that around 2001, along with common sense.  You 
can't question what was done 17 years ago asking for a backing piece of 
paper (possibly virtual).  You can put anything on paper and if it does 
not match reality, then it is irrelevant.

 If you question what I state, then ask the MIPS/Linux kernel developers 
at the relevant mailing list, i.e. <linux-mips@linux-mips.org>.

  Maciej

Re: [Qemu-devel] [PATCH v2 4/6] target/mips: Fix decoding mechanism of special R5900 opcodes

[Maciej W. Rozycki]

On Fri, 9 Nov 2018, Aleksandar Markovic wrote:

> > ... and DMULT, DMULTU, DDIV and DDIVU
> > are part of the MIPS III ISA. They are emulated in user mode to support
> > generic MIPS III programs.
> 
> Pure MIPS III executables should not be a concern of the R5900 
> emulation, but R5900 executables.

 I repeat: MIPS III is the available instruction set defined with the base 
64-bit MIPS Linux psABI and must not be subsetted.  You have to support it 
in QEMU Linux user emulation mode for any 64-bit MIPS processor and this 
is not debatable, period.  QEMU has no control over the Linux ABI, it has 
to accept it as it is.

  Maciej

Re: [Qemu-devel] [PATCH v2 4/6] target/mips: Fix decoding mechanism of special R5900 opcodes

[Aleksandar Markovic]

> From: Fredrik Noring <noring@nocrew.org>
> Subject: [PATCH v2 4/6] target/mips: Fix decoding mechanism of special R5900 opcodes
> 
> MOVN, MOVZ, MFHI, MFLO, MTHI, MTLO, MULT, MULTU, DIV, DIVU, DMULT,
> DMULTU, DDIV, DDIVU and JR are decoded in decode_opc_special_tx79
> instead of the generic decode_opc_special_legacy.
> 
> Signed-off-by: Fredrik Noring <noring@nocrew.org>
> ---

Reviewed-by: Aleksandar Markovic <amarkovic@wavecomp.com>

with caveat that this should be resolved in 3.1+.

[Qemu-devel] [PATCH v2 5/6] target/mips: Guard check_insn_opc_user_only with INSN_R5900 check

[Fredrik Noring]

Signed-off-by: Fredrik Noring <noring@nocrew.org>
---
 target/mips/translate.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/target/mips/translate.c b/target/mips/translate.c
index 45ad70c097..c3ed4c21ce 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -28285,7 +28285,9 @@ static void decode_opc(CPUMIPSState *env, DisasContext *ctx)
          break;
     case OPC_LL: /* Load and stores */
         check_insn(ctx, ISA_MIPS2);
-        check_insn_opc_user_only(ctx, INSN_R5900);
+        if (ctx->insn_flags & INSN_R5900) {
+            check_insn_opc_user_only(ctx, INSN_R5900);
+        }
         /* Fallthrough */
     case OPC_LWL:
     case OPC_LWR:
@@ -28311,7 +28313,9 @@ static void decode_opc(CPUMIPSState *env, DisasContext *ctx)
     case OPC_SC:
         check_insn(ctx, ISA_MIPS2);
          check_insn_opc_removed(ctx, ISA_MIPS32R6);
-        check_insn_opc_user_only(ctx, INSN_R5900);
+        if (ctx->insn_flags & INSN_R5900) {
+            check_insn_opc_user_only(ctx, INSN_R5900);
+        }
          gen_st_cond(ctx, op, rt, rs, imm);
          break;
     case OPC_CACHE:
@@ -28579,7 +28583,9 @@ static void decode_opc(CPUMIPSState *env, DisasContext *ctx)
 #if defined(TARGET_MIPS64)
     /* MIPS64 opcodes */
     case OPC_LLD:
-        check_insn_opc_user_only(ctx, INSN_R5900);
+        if (ctx->insn_flags & INSN_R5900) {
+            check_insn_opc_user_only(ctx, INSN_R5900);
+        }
         /* fall through */
     case OPC_LDL:
     case OPC_LDR:
@@ -28603,7 +28609,9 @@ static void decode_opc(CPUMIPSState *env, DisasContext *ctx)
     case OPC_SCD:
         check_insn_opc_removed(ctx, ISA_MIPS32R6);
         check_insn(ctx, ISA_MIPS3);
-        check_insn_opc_user_only(ctx, INSN_R5900);
+        if (ctx->insn_flags & INSN_R5900) {
+            check_insn_opc_user_only(ctx, INSN_R5900);
+        }
         check_mips_64(ctx);
         gen_st_cond(ctx, op, rt, rs, imm);
         break;
-- 
2.18.1

Re: [Qemu-devel] [PATCH v2 5/6] target/mips: Guard check_insn_opc_user_only with INSN_R5900 check

[Aleksandar Markovic]

> From: Fredrik Noring <noring@nocrew.org>
> Subject: [PATCH v2 5/6] target/mips: Guard check_insn_opc_user_only with INSN_R5900 check

Reviewed-by: Aleksandar Markovic <amarkovic@wavecomp.com>

Some minor changes will be made before integrating.

[Qemu-devel] [PATCH v2 6/6] target/mips: Guard check_insn with INSN_R5900 check

[Fredrik Noring]

Signed-off-by: Fredrik Noring <noring@nocrew.org>
---
 target/mips/translate.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/target/mips/translate.c b/target/mips/translate.c
index c3ed4c21ce..007dfd2975 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -28329,8 +28329,11 @@ static void decode_opc(CPUMIPSState *env, DisasContext *ctx)
         break;
     case OPC_PREF:
         check_insn_opc_removed(ctx, ISA_MIPS32R6);
-        check_insn(ctx, ISA_MIPS4 | ISA_MIPS32 |
-                   INSN_R5900);
+        if (ctx->insn_flags & INSN_R5900) {
+            /* The R5900 implements PREF. */
+        } else {
+            check_insn(ctx, ISA_MIPS4 | ISA_MIPS32);
+        }
         /* Treat as NOP. */
         break;
 
-- 
2.18.1

Re: [Qemu-devel] [PATCH v2 6/6] target/mips: Guard check_insn with INSN_R5900 check

[Aleksandar Markovic]

> From: Fredrik Noring <noring@nocrew.org>
> Subject: [PATCH v2 6/6] target/mips: Guard check_insn with INSN_R5900 check

Reviewed-by: Aleksandar Markovic <amarkovic@wavecomp.com>

Some minor changes will be made before integrating.