[Qemu-devel] [PATCH 0/2] target/mips: Fix decoding mechanisms of R5900 M{F, T}{HI, LO}1 and DIV[U]1

[Qemu-devel] [PATCH 0/2] target/mips: Fix decoding mechanisms of R5900 M{F, T}{HI, LO}1 and DIV[U]1

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

Fredrik Noring (2):
  target/mips: Fix decoding mechanism of R5900 MFLO1, MFHI1, MTLO1 and MTHI1
  target/mips: Fix decoding mechanism of R5900 DIV1 and DIVU1

 target/mips/translate.c | 132 ++++++++++++++++++++++++++++++++++------
 1 file changed, 115 insertions(+), 17 deletions(-)

-- 
2.18.1

[Qemu-devel] [PATCH 1/2] 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 | 67 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 56 insertions(+), 11 deletions(-)

diff --git a/target/mips/translate.c b/target/mips/translate.c
index 60320cbe69..f3993cf7d7 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -4359,24 +4359,72 @@ static void gen_shift(DisasContext *ctx, uint32_t opc,
     tcg_temp_free(t1);
 }
 
+/* Move to and from TX79 HI1/LO1 registers. */
+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:
+#if defined(TARGET_MIPS64)
+        tcg_gen_ext32s_tl(cpu_gpr[reg], cpu_HI[1]);
+#else
+        tcg_gen_mov_tl(cpu_gpr[reg], cpu_HI[1]);
+#endif
+        break;
+    case TX79_MMI_MFLO1:
+#if defined(TARGET_MIPS64)
+        tcg_gen_ext32s_tl(cpu_gpr[reg], cpu_LO[1]);
+#else
+        tcg_gen_mov_tl(cpu_gpr[reg], cpu_LO[1]);
+#endif
+        break;
+    case TX79_MMI_MTHI1:
+        if (reg != 0) {
+#if defined(TARGET_MIPS64)
+            tcg_gen_ext32s_tl(cpu_HI[1], cpu_gpr[reg]);
+#else
+            tcg_gen_mov_tl(cpu_HI[1], cpu_gpr[reg]);
+#endif
+        } else {
+            tcg_gen_movi_tl(cpu_HI[1], 0);
+        }
+        break;
+    case TX79_MMI_MTLO1:
+        if (reg != 0) {
+#if defined(TARGET_MIPS64)
+            tcg_gen_ext32s_tl(cpu_LO[1], cpu_gpr[reg]);
+#else
+            tcg_gen_mov_tl(cpu_LO[1], cpu_gpr[reg]);
+#endif
+        } else {
+            tcg_gen_movi_tl(cpu_LO[1], 0);
+        }
+        break;
+    default:
+        MIPS_INVAL("MFTHILO 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 +4435,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 +4445,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 +4459,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 +26545,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 1/2] target/mips: Fix decoding mechanism of R5900 MFLO1, MFHI1, MTLO1 and MTHI1

[Philippe Mathieu-Daudé]

On 2/11/18 17:08, Fredrik Noring wrote:
> MFLO1, MFHI1, MTLO1 and MTHI1 are generated in gen_HILO1_tx79 instead of
> the generic gen_HILO.
> 

Aleksandar, if you are OK with this patch, can you add:

Fixes: 8d927f7cb4b

> Signed-off-by: Fredrik Noring <noring@nocrew.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>   target/mips/translate.c | 67 ++++++++++++++++++++++++++++++++++-------
>   1 file changed, 56 insertions(+), 11 deletions(-)
> 
> diff --git a/target/mips/translate.c b/target/mips/translate.c
> index 60320cbe69..f3993cf7d7 100644
> --- a/target/mips/translate.c
> +++ b/target/mips/translate.c
> @@ -4359,24 +4359,72 @@ static void gen_shift(DisasContext *ctx, uint32_t opc,
>       tcg_temp_free(t1);
>   }
>   
> +/* Move to and from TX79 HI1/LO1 registers. */
> +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:
> +#if defined(TARGET_MIPS64)
> +        tcg_gen_ext32s_tl(cpu_gpr[reg], cpu_HI[1]);
> +#else
> +        tcg_gen_mov_tl(cpu_gpr[reg], cpu_HI[1]);
> +#endif
> +        break;
> +    case TX79_MMI_MFLO1:
> +#if defined(TARGET_MIPS64)
> +        tcg_gen_ext32s_tl(cpu_gpr[reg], cpu_LO[1]);
> +#else
> +        tcg_gen_mov_tl(cpu_gpr[reg], cpu_LO[1]);
> +#endif
> +        break;
> +    case TX79_MMI_MTHI1:
> +        if (reg != 0) {
> +#if defined(TARGET_MIPS64)
> +            tcg_gen_ext32s_tl(cpu_HI[1], cpu_gpr[reg]);
> +#else
> +            tcg_gen_mov_tl(cpu_HI[1], cpu_gpr[reg]);
> +#endif
> +        } else {
> +            tcg_gen_movi_tl(cpu_HI[1], 0);
> +        }
> +        break;
> +    case TX79_MMI_MTLO1:
> +        if (reg != 0) {
> +#if defined(TARGET_MIPS64)
> +            tcg_gen_ext32s_tl(cpu_LO[1], cpu_gpr[reg]);
> +#else
> +            tcg_gen_mov_tl(cpu_LO[1], cpu_gpr[reg]);
> +#endif
> +        } else {
> +            tcg_gen_movi_tl(cpu_LO[1], 0);
> +        }
> +        break;
> +    default:
> +        MIPS_INVAL("MFTHILO 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 +4435,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 +4445,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 +4459,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 +26545,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 */
> 

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

[Richard Henderson]

On 11/2/18 4:08 PM, Fredrik Noring wrote:
> +    switch (opc) {
> +    case TX79_MMI_MFHI1:
> +#if defined(TARGET_MIPS64)
> +        tcg_gen_ext32s_tl(cpu_gpr[reg], cpu_HI[1]);
> +#else
> +        tcg_gen_mov_tl(cpu_gpr[reg], cpu_HI[1]);
> +#endif

You do not need this ifdef.  This is already done in tcg/tcg-op.h:

$ grep tcg_gen_ext32s_tl tcg/tcg-op.h
#define tcg_gen_ext32s_tl tcg_gen_ext32s_i64
#define tcg_gen_ext32s_tl tcg_gen_mov_i32


r~

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

[Fredrik Noring]

Thank you for your reviews, Philippe and Richard,

> > +    switch (opc) {
> > +    case TX79_MMI_MFHI1:
> > +#if defined(TARGET_MIPS64)
> > +        tcg_gen_ext32s_tl(cpu_gpr[reg], cpu_HI[1]);
> > +#else
> > +        tcg_gen_mov_tl(cpu_gpr[reg], cpu_HI[1]);
> > +#endif
> 
> You do not need this ifdef.  This is already done in tcg/tcg-op.h:
> 
> $ grep tcg_gen_ext32s_tl tcg/tcg-op.h
> #define tcg_gen_ext32s_tl tcg_gen_ext32s_i64
> #define tcg_gen_ext32s_tl tcg_gen_mov_i32

It appears the correct function is tcg_gen_mov_tl because the TX79 manual
says

	MFHI:  GPR[rd]63..0 <- HI63..0
	MFLO:  GPR[rd]63..0 <- LO63..0
	MTHI:  HI63..0 <- GPR[rs]63..0
	MTLO:  LO63..0 <- GPR[rs]63..0
	MFHI1: GPR[rd]63..0 <- HI127..64
	MFLO1: GPR[rd]63..0 <- LO127..64
	MTHI1: HI127..64 <- GPR[rs]63..0
	MTLO1: LO127..64 <- GPR[rs]63..0

so the GPR is copied to/from in full in all cases. This is slightly
different to how acc = 1 is handled in gen_HILO.

Fredrik

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

[Maciej W. Rozycki]

On Sun, 4 Nov 2018, Fredrik Noring wrote:

> It appears the correct function is tcg_gen_mov_tl because the TX79 manual
> says
> 
> 	MFHI:  GPR[rd]63..0 <- HI63..0
> 	MFLO:  GPR[rd]63..0 <- LO63..0
> 	MTHI:  HI63..0 <- GPR[rs]63..0
> 	MTLO:  LO63..0 <- GPR[rs]63..0
> 	MFHI1: GPR[rd]63..0 <- HI127..64
> 	MFLO1: GPR[rd]63..0 <- LO127..64
> 	MTHI1: HI127..64 <- GPR[rs]63..0
> 	MTLO1: LO127..64 <- GPR[rs]63..0
> 
> so the GPR is copied to/from in full in all cases. This is slightly
> different to how acc = 1 is handled in gen_HILO.

 However `gen_HILO' looks wrong to me as it'll truncate the values of 
$acc3-$acc1 with the 64-bit DSP ASE.

  Maciej

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

[Fredrik Noring]

Thanks for checking this, Maciej,

[ Cc-ing Jia Liu, who added MIPS ASE DSP support in commit 4133498f8e532f
"Use correct acc value to index cpu_HI/cpu_LO rather than using a fix
number", in case there are known ISA deviations. ]

>  However `gen_HILO' looks wrong to me as it'll truncate the values of 
> $acc3-$acc1 with the 64-bit DSP ASE.

I can post a patch to fix gen_HILO. The best reference I have found so far
is "MIPS Architecture for Programmers Volume IV-e: MIPS DSP Module for
microMIPS64 Architecture"

https://s3-eu-west-1.amazonaws.com/downloads-mips/documents/MD00765-2B-microMIPS64DSP-AFP-03.02.pdf

that says

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

where ac can range from 0 to 3. Do you have a link to a better reference,
by chance, that isn't tied to microMIPS?

Fredrik

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

[Maciej W. Rozycki]

On Mon, 5 Nov 2018, Fredrik Noring wrote:

> where ac can range from 0 to 3. Do you have a link to a better reference,
> by chance, that isn't tied to microMIPS?

 Here: <https://www.mips.com/downloads/> you'll find everything you need I 
believe.

 HTH,

  Maciej

[Qemu-devel] [PATCH 2/2] 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>
---
 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 f3993cf7d7..6e5a8a2565 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -4759,6 +4759,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)
 {
@@ -4771,14 +4828,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();
@@ -4800,7 +4854,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);
@@ -26541,7 +26594,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

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

[Philippe Mathieu-Daudé]

On 2/11/18 17:08, Fredrik Noring wrote:
> DIV1 and DIVU1 are generated in gen_div1_tx79 instead of the generic
> gen_muldiv.
> 

Fixes: be9c42c90d1 (R5900-specific opcodes overlap with generic opcodes)

> 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 f3993cf7d7..6e5a8a2565 100644
> --- a/target/mips/translate.c
> +++ b/target/mips/translate.c
> @@ -4759,6 +4759,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)
>   {
> @@ -4771,14 +4828,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();
> @@ -4800,7 +4854,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);
> @@ -26541,7 +26594,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:
> 

Re: [Qemu-devel] [PATCH 0/2] target/mips: Fix decoding mechanisms of R5900 M{F, T}{HI, LO}1 and DIV[U]1

[Aleksandar Markovic]

> From: Fredrik Noring <noring@nocrew.org>
>
> Subject: [PATCH 0/2] target/mips: Fix decoding mechanisms of R5900 M{F,T}{HI,LO}1 and DIV[U]1
> 
> This series amends the R5900 support with the following changes:
>
> ...

Hi, Fridrek,

The two patches look very good. Yes, there will be some code duplication now, but the overall MIPS code is safer, R5900 code better encapsulated - and you have more flexibility to tweak R5900 functionality.

For LL, SC, LLD and SCD instructions, there is a need to properly insulate their R5900 versions too, similar to this:

    case OPC_SC:
        if(ctx->insn_flags & INSN_R5900) {
             check_insn_opc_user_only(ctx, INSN_R5900);
        } else {
            check_insn(ctx, ISA_MIPS2);
        }
        gen_st_cond(ctx, op, rt, rs, imm);
        break;

(the code above is just a form of pseudocode illustrating the idea; I don't guarantee the correctness for build purposes, or if this is the best code organization)

Non-R5900 code (for the time being) should never invoke check_insn_opc_user_only(). *The only way* of distinguishing R5900 code paths from the other CPUs code paths should be by using  "if(ctx->insn_flags & INSN_R5900)"!

For changes in decode_opc_special_legacy(), there shouldn't be there, but there should be a separate function decode_opc_special_tx59() or so.

Thanks,
Aleksandar

Re: [Qemu-devel] [PATCH 0/2] target/mips: Fix decoding mechanisms of R5900 M{F, T}{HI, LO}1 and DIV[U]1

[Fredrik Noring]

Thank you for your review, Aleksandar,

> For LL, SC, LLD and SCD instructions, there is a need to properly insulate
> their R5900 versions too, similar to this:
> 
>     case OPC_SC:
>         if(ctx->insn_flags & INSN_R5900) {
>              check_insn_opc_user_only(ctx, INSN_R5900);
>         } else {
>             check_insn(ctx, ISA_MIPS2);
>         }
>         gen_st_cond(ctx, op, rt, rs, imm);
>         break;

Would you accept the simplification to omit the else clause? Like this:

    case OPC_SC:
        if (ctx->insn_flags & INSN_R5900) {
            check_insn_opc_user_only(ctx, INSN_R5900);
        }
        check_insn(ctx, ISA_MIPS2);
        check_insn_opc_removed(ctx, ISA_MIPS32R6);
        gen_st_cond(ctx, op, rt, rs, imm);
        break;

The code will, of course, expand into a double-check of INSN_R5900:

        if (ctx->insn_flags & INSN_R5900) {
#ifndef CONFIG_USER_ONLY
            if (unlikely(ctx->insn_flags & INSN_R5900)) {
                generate_exception_end(ctx, EXCP_RI);
            }
#endif
        }

> (the code above is just a form of pseudocode illustrating the idea; I
> don't guarantee the correctness for build purposes, or if this is the best
> code organization)
> 
> Non-R5900 code (for the time being) should never invoke
> check_insn_opc_user_only(). *The only way* of distinguishing R5900 code
> paths from the other CPUs code paths should be by using
> "if(ctx->insn_flags & INSN_R5900)"!

OK.

> For changes in decode_opc_special_legacy(), there shouldn't be there, but
> there should be a separate function decode_opc_special_tx59() or so.

Sure, I will copy the 82 line function then, and patch the following:

--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -23904,7 +23904,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 */
@@ -23931,8 +23931,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);
         }
@@ -23947,7 +23945,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;

Fredrik

Re: [Qemu-devel] [PATCH 0/2] target/mips: Fix decoding mechanisms of R5900 M{F, T}{HI, LO}1 and DIV[U]1

[Aleksandar Markovic]

Hello, Fredrik.

I appreciate your response and efforts!

>
> From: Fredrik Noring <noring@nocrew.org>
>
> Subject: Re: [PATCH 0/2] target/mips: Fix decoding mechanisms of R5900 M{F,T}{HI,LO}1 and DIV[> U]1
>
> Thank you for your review, Aleksandar,
>
> > For LL, SC, LLD and SCD instructions, there is a need to properly insulate
> > their R5900 versions too, similar to this:
> >
> >     case OPC_SC:
> >         if(ctx->insn_flags & INSN_R5900) {
> >              check_insn_opc_user_only(ctx, INSN_R5900);
> >         } else {
> >             check_insn(ctx, ISA_MIPS2);
> >         }
> >         gen_st_cond(ctx, op, rt, rs, imm);
> >         break;
>
> Would you accept the simplification to omit the else clause? Like this:
>
>     case OPC_SC:
>         if (ctx->insn_flags & INSN_R5900) {
>             check_insn_opc_user_only(ctx, INSN_R5900);
>         }
>         check_insn(ctx, ISA_MIPS2);
>         check_insn_opc_removed(ctx, ISA_MIPS32R6);
>         gen_st_cond(ctx, op, rt, rs, imm);
>         break;
>

I think the following code would be even better:

    case OPC_SC:
        check_insn(ctx, ISA_MIPS2);
        check_insn_opc_removed(ctx, ISA_MIPS32R6);
        if (ctx->insn_flags & INSN_R5900) {
            check_insn_opc_user_only(ctx, INSN_R5900);
        }
        gen_st_cond(ctx, op, rt, rs, imm);
        break;

> The code will, of course, expand into a double-check of INSN_R5900:
>
>         if (ctx->insn_flags & INSN_R5900) {
> #ifndef CONFIG_USER_ONLY
>             if (unlikely(ctx->insn_flags & INSN_R5900)) {
>                 generate_exception_end(ctx, EXCP_RI);
>             }
> #endif
>         }
>

I don't mind. Later on, we can drop the second argument of check_insn_opc_user_only() altogether, and the code would expand to the minimal and clear:

        if (ctx->insn_flags & INSN_R5900) {
#ifndef CONFIG_USER_ONLY
            generate_exception_end(ctx, EXCP_RI);
#endif
        }

but at this moment this is not a source of concern to me at all.

> > (the code above is just a form of pseudocode illustrating the idea; I
> > don't guarantee the correctness for build purposes, or if this is the best
> > code organization)
> >
> > Non-R5900 code (for the time being) should never invoke
> > check_insn_opc_user_only(). *The only way* of distinguishing R5900 code
> > paths from the other CPUs code paths should be by using
> > "if(ctx->insn_flags & INSN_R5900)"!
>
> OK.
>
> > For changes in decode_opc_special_legacy(), there shouldn't be there, but
> > there should be a separate function decode_opc_special_tx59() or so.
>
> Sure, I will copy the 82 line function then...
>

No, no, you don't need to copy 82 lines. First, you can assume that you are already in INSN_R5900 case - no need for frequent check_insn()s. Further, you can omit everything that is not needed for R5900 (for example, entire OPC_MOVCI case).

> ..., and patch the following:
>
> --- a/target/mips/translate.c
> +++ b/target/mips/translate.c
> @@ -23904,7 +23904,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 */
> @@ -23931,8 +23931,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);
>          }
> @@ -23947,7 +23945,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;
>

Exactly!

> Fredrik
>

Aleksandar

________________________________________
From: Fredrik Noring <noring@nocrew.org>
Sent: Monday, November 5, 2018 7:12:42 PM
To: Aleksandar Markovic
Cc: Aurelien Jarno; Philippe Mathieu-Daudé; Jürgen Urban; Maciej W. Rozycki; qemu-devel@nongnu.org
Subject: Re: [PATCH 0/2] target/mips: Fix decoding mechanisms of R5900 M{F,T}{HI,LO}1 and DIV[U]1

Thank you for your review, Aleksandar,

> For LL, SC, LLD and SCD instructions, there is a need to properly insulate
> their R5900 versions too, similar to this:
>
>     case OPC_SC:
>         if(ctx->insn_flags & INSN_R5900) {
>              check_insn_opc_user_only(ctx, INSN_R5900);
>         } else {
>             check_insn(ctx, ISA_MIPS2);
>         }
>         gen_st_cond(ctx, op, rt, rs, imm);
>         break;

Would you accept the simplification to omit the else clause? Like this:

    case OPC_SC:
        if (ctx->insn_flags & INSN_R5900) {
            check_insn_opc_user_only(ctx, INSN_R5900);
        }
        check_insn(ctx, ISA_MIPS2);
        check_insn_opc_removed(ctx, ISA_MIPS32R6);
        gen_st_cond(ctx, op, rt, rs, imm);
        break;

The code will, of course, expand into a double-check of INSN_R5900:

        if (ctx->insn_flags & INSN_R5900) {
#ifndef CONFIG_USER_ONLY
            if (unlikely(ctx->insn_flags & INSN_R5900)) {
                generate_exception_end(ctx, EXCP_RI);
            }
#endif
        }

> (the code above is just a form of pseudocode illustrating the idea; I
> don't guarantee the correctness for build purposes, or if this is the best
> code organization)
>
> Non-R5900 code (for the time being) should never invoke
> check_insn_opc_user_only(). *The only way* of distinguishing R5900 code
> paths from the other CPUs code paths should be by using
> "if(ctx->insn_flags & INSN_R5900)"!

OK.

> For changes in decode_opc_special_legacy(), there shouldn't be there, but
> there should be a separate function decode_opc_special_tx59() or so.

Sure, I will copy the 82 line function then, and patch the following:

--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -23904,7 +23904,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 */
@@ -23931,8 +23931,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);
         }
@@ -23947,7 +23945,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;

Fredrik

Re: [Qemu-devel] [PATCH 0/2] target/mips: Fix decoding mechanisms of R5900 M{F, T}{HI, LO}1 and DIV[U]1

[Fredrik Noring]

Hi Aleksandar,

> I think the following code would be even better:
> 
>     case OPC_SC:
>         check_insn(ctx, ISA_MIPS2);
>         check_insn_opc_removed(ctx, ISA_MIPS32R6);
>         if (ctx->insn_flags & INSN_R5900) {
>             check_insn_opc_user_only(ctx, INSN_R5900);
>         }
>         gen_st_cond(ctx, op, rt, rs, imm);
>         break;

Done.

> I don't mind. Later on, we can drop the second argument of
> check_insn_opc_user_only() altogether, and the code would expand to the
> minimal and clear:
> 
>         if (ctx->insn_flags & INSN_R5900) {
> #ifndef CONFIG_USER_ONLY
>             generate_exception_end(ctx, EXCP_RI);
> #endif
>         }
> 
> but at this moment this is not a source of concern to me at all.

OK.

> No, no, you don't need to copy 82 lines. First, you can assume that you
> are already in INSN_R5900 case - no need for frequent check_insn()s.
> Further, you can omit everything that is not needed for R5900 (for
> example, entire OPC_MOVCI case).

Yes, of course. I've made some general updates as well, for example
making use of extract32. I will post v2 in a moment.

Fredrik