Skip to contentSkip to conversation

Fix decoding mechanisms of the R5900

PATCH In review Fredrik Noring 3 participants 23 messages Nov 7, 2018 – Nov 17, 2018

lore
Share & tools
lore

Fredrik Noring

author
#0 lore raw

v2 cover Fix decoding mechanisms of the R5900

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(-)
Signature · 1 line
2.18.1
author
#1 lore raw

PATCH v2 1/6 target/mips: Fix decoding mechanism of R5900 MFLO1, MFHI1, MTLO1 and MTHI1

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

Signed-off-by Fredrik Noring <[email protected]>
---
 target/mips/translate.c | 51 ++++++++++++++++++++++++++++++++---------
 1 file changed, 40 insertions(+), 11 deletions(-)
target/mips/translate.c +40 -11
index 60320cbe69..8601333554 100644
@@ -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 */
Signature · 1 line
2.18.1
author
#3 lore raw

PATCH v2 2/6 target/mips: Fix decoding mechanism of R5900 DIV1 and DIVU1

In reply to Fredrik Noring

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

Signed-off-by Fredrik Noring <[email protected]>
Reviewed-by Philippe Mathieu-Daudé <[email protected]>
---
 target/mips/translate.c | 65 +++++++++++++++++++++++++++++++++++++----
 1 file changed, 59 insertions(+), 6 deletions(-)
target/mips/translate.c +59 -6
index 8601333554..3ddd70043a 100644
@@ -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:
Signature · 1 line
2.18.1
author
#4 lore raw

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

In reply to 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 <[email protected]>
Reported-by Maciej W. Rozycki <[email protected]>
Signed-off-by Fredrik Noring <[email protected]>
---
 target/mips/translate.c | 36 ++++--------------------------------
 1 file changed, 4 insertions(+), 32 deletions(-)
target/mips/translate.c +4 -32
index 3ddd70043a..19ae7d2f1c 100644
@@ -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);
         }
Signature · 1 line
2.18.1
#5 lore raw

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

Quoted 17 lines

From: Fredrik Noring <[email protected]>
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; [email protected]
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
Quoted 8 lines
Fixes 4133498f8e53 ("Use correct acc value to index cpu_HI/cpu_LO rather than using > a fix number")
Cc Jia Liu <[email protected]>
Reported-by Maciej W. Rozycki <[email protected]>
Signed-off-by Fredrik Noring <[email protected]>
---
 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

Quoted 62 lines
target/mips/translate.c +4 -33
index 3ddd70043a..19ae7d2f1c 100644
@@ -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

author
#6 lore raw

PATCH v2 4/6 target/mips: Fix decoding mechanism of special R5900 opcodes

In reply to 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 <[email protected]>
---
 target/mips/translate.c | 54 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 50 insertions(+), 4 deletions(-)
target/mips/translate.c +50 -4
index 19ae7d2f1c..45ad70c097 100644
@@ -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);
         }
Signature · 1 line
2.18.1
#7 lore raw

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

Quoted 14 lines

From: Fredrik Noring <[email protected]>
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 <[email protected]>
---
 target/mips/translate.c | 54 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 50 insertions(+), 4 deletions(-)

target/mips/translate.c static void decode_opc_special_r6(CPUMIPSState *env, DisasContext *ctx) · line 23874

Quoted 9 lines
target/mips/translate.c +9 -0
+#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

author
#8 lore raw

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

Hi Aleksandar,

Quoted 7 lines

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

#9 lore raw

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

Quoted 11 lines · Fredrik Noring
Quoted 7 lines

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

#10 lore raw

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

In reply to Aleksandar Markovic
Quoted 9 lines

From: Fredrik Noring <[email protected]>
Subject: Re: [PATCH v2 4/6] target/mips: Fix decoding mechanism of special R5900 opcodes

Hi Aleksandar,

Quoted 4 lines

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?

Quoted 5 lines
Quoted 3 lines

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.

Quoted 3 lines

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

author
#11 lore raw

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

Hi Aleksandar,

Quoted 4 lines
Quoted 1 line

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.

Quoted 6 lines
Quoted 3 lines

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

Quoted 2 lines

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

#12 lore raw

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

Quoted 9 lines

From: Fredrik Noring <[email protected]>
Subject: Re: [PATCH v2 4/6] target/mips: Fix decoding mechanism of special R5900 opcodes

Quoted 3 lines

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

author
#13 lore raw

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

Hi Aleksandar,

Quoted 5 lines

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

#14 lore raw

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

Quoted 5 lines

From: Fredrik Noring <[email protected]>
Subject: Re: [PATCH v2 4/6] target/mips: Fix decoding mechanism of special R5900 opcodes

Hi Aleksandar,

Quoted 8 lines
Quoted 5 lines

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

#15 lore raw

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

Quoted 5 lines

From: Fredrik Noring <[email protected]>
Subject: Re: [PATCH v2 4/6] target/mips: Fix decoding mechanism of special R5900 opcodes

Hi Aleksandar,

Quoted 8 lines
Quoted 5 lines

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

#16 lore raw

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

Quoted 11 lines · Aleksandar Markovic
Quoted 5 lines
Quoted 2 lines

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. <[email protected]>.

Maciej

#17 lore raw

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

In reply to Aleksandar Markovic
Quoted 6 lines · Aleksandar Markovic
Quoted 3 lines

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

Reviewed-by on v2 4/6
#18 lore raw

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

In reply to Fredrik Noring
Quoted 9 lines

From: Fredrik Noring <[email protected]>
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 <[email protected]>

---

Reviewed-by Aleksandar Markovic <[email protected]>

with caveat that this should be resolved in 3.1+.

author
#19 lore raw

PATCH v2 5/6 target/mips: Guard check_insn_opc_user_only with INSN_R5900 check

In reply to Fredrik Noring
Signed-off-by Fredrik Noring <[email protected]>
---
 target/mips/translate.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)
target/mips/translate.c +12 -4
index 45ad70c097..c3ed4c21ce 100644
@@ -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;
Signature · 1 line
2.18.1
author
#21 lore raw

PATCH v2 6/6 target/mips: Guard check_insn with INSN_R5900 check

In reply to Fredrik Noring
Signed-off-by Fredrik Noring <[email protected]>
---
 target/mips/translate.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
target/mips/translate.c +5 -2
index c3ed4c21ce..007dfd2975 100644
@@ -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;
Signature · 1 line
2.18.1