* [Qemu-devel] [PATCH 0/2] target/mips: Fix decoding mechanisms of R5900 M{F, T}{HI, LO}1 and DIV[U]1
@ 2018-11-02 16:07 Fredrik Noring
2018-11-02 16:08 ` [Qemu-devel] [PATCH 1/2] target/mips: Fix decoding mechanism of R5900 MFLO1, MFHI1, MTLO1 and MTHI1 Fredrik Noring
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Fredrik Noring @ 2018-11-02 16:07 UTC (permalink / raw)
To: Aleksandar Markovic, Aurelien Jarno, Philippe Mathieu-Daudé
Cc: Jürgen Urban, Maciej W. Rozycki, qemu-devel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [Qemu-devel] [PATCH 1/2] target/mips: Fix decoding mechanism of R5900 MFLO1, MFHI1, MTLO1 and MTHI1
2018-11-02 16:07 [Qemu-devel] [PATCH 0/2] target/mips: Fix decoding mechanisms of R5900 M{F, T}{HI, LO}1 and DIV[U]1 Fredrik Noring
@ 2018-11-02 16:08 ` Fredrik Noring
2018-11-02 18:10 ` Philippe Mathieu-Daudé
2018-11-04 10:15 ` Richard Henderson
2018-11-02 16:08 ` [Qemu-devel] [PATCH 2/2] target/mips: Fix decoding mechanism of R5900 DIV1 and DIVU1 Fredrik Noring
2018-11-05 13:18 ` [Qemu-devel] [PATCH 0/2] target/mips: Fix decoding mechanisms of R5900 M{F, T}{HI, LO}1 and DIV[U]1 Aleksandar Markovic
2 siblings, 2 replies; 14+ messages in thread
From: Fredrik Noring @ 2018-11-02 16:08 UTC (permalink / raw)
To: Aleksandar Markovic, Aurelien Jarno, Philippe Mathieu-Daudé
Cc: Jürgen Urban, Maciej W. Rozycki, qemu-devel
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [Qemu-devel] [PATCH 2/2] target/mips: Fix decoding mechanism of R5900 DIV1 and DIVU1
2018-11-02 16:07 [Qemu-devel] [PATCH 0/2] target/mips: Fix decoding mechanisms of R5900 M{F, T}{HI, LO}1 and DIV[U]1 Fredrik Noring
2018-11-02 16:08 ` [Qemu-devel] [PATCH 1/2] target/mips: Fix decoding mechanism of R5900 MFLO1, MFHI1, MTLO1 and MTHI1 Fredrik Noring
@ 2018-11-02 16:08 ` Fredrik Noring
2018-11-02 18:15 ` Philippe Mathieu-Daudé
2018-11-05 13:18 ` [Qemu-devel] [PATCH 0/2] target/mips: Fix decoding mechanisms of R5900 M{F, T}{HI, LO}1 and DIV[U]1 Aleksandar Markovic
2 siblings, 1 reply; 14+ messages in thread
From: Fredrik Noring @ 2018-11-02 16:08 UTC (permalink / raw)
To: Aleksandar Markovic, Aurelien Jarno, Philippe Mathieu-Daudé
Cc: Jürgen Urban, Maciej W. Rozycki, qemu-devel
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] target/mips: Fix decoding mechanism of R5900 MFLO1, MFHI1, MTLO1 and MTHI1
2018-11-02 16:08 ` [Qemu-devel] [PATCH 1/2] target/mips: Fix decoding mechanism of R5900 MFLO1, MFHI1, MTLO1 and MTHI1 Fredrik Noring
@ 2018-11-02 18:10 ` Philippe Mathieu-Daudé
2018-11-04 10:15 ` Richard Henderson
1 sibling, 0 replies; 14+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-11-02 18:10 UTC (permalink / raw)
To: Fredrik Noring, Aleksandar Markovic, Aurelien Jarno,
Philippe Mathieu-Daudé
Cc: Jürgen Urban, qemu-devel, Maciej W. Rozycki
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 */
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] target/mips: Fix decoding mechanism of R5900 DIV1 and DIVU1
2018-11-02 16:08 ` [Qemu-devel] [PATCH 2/2] target/mips: Fix decoding mechanism of R5900 DIV1 and DIVU1 Fredrik Noring
@ 2018-11-02 18:15 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 14+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-11-02 18:15 UTC (permalink / raw)
To: Fredrik Noring, Aleksandar Markovic, Aurelien Jarno,
Philippe Mathieu-Daudé
Cc: Jürgen Urban, qemu-devel, Maciej W. Rozycki
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:
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] target/mips: Fix decoding mechanism of R5900 MFLO1, MFHI1, MTLO1 and MTHI1
2018-11-02 16:08 ` [Qemu-devel] [PATCH 1/2] target/mips: Fix decoding mechanism of R5900 MFLO1, MFHI1, MTLO1 and MTHI1 Fredrik Noring
2018-11-02 18:10 ` Philippe Mathieu-Daudé
@ 2018-11-04 10:15 ` Richard Henderson
2018-11-04 13:12 ` Fredrik Noring
1 sibling, 1 reply; 14+ messages in thread
From: Richard Henderson @ 2018-11-04 10:15 UTC (permalink / raw)
To: Fredrik Noring, Aleksandar Markovic, Aurelien Jarno,
Philippe Mathieu-Daudé
Cc: Jürgen Urban, qemu-devel, Maciej W. Rozycki
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~
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] target/mips: Fix decoding mechanism of R5900 MFLO1, MFHI1, MTLO1 and MTHI1
2018-11-04 10:15 ` Richard Henderson
@ 2018-11-04 13:12 ` Fredrik Noring
2018-11-04 15:09 ` Maciej W. Rozycki
0 siblings, 1 reply; 14+ messages in thread
From: Fredrik Noring @ 2018-11-04 13:12 UTC (permalink / raw)
To: Richard Henderson, Philippe Mathieu-Daudé
Cc: Aleksandar Markovic, Aurelien Jarno, Maciej W. Rozycki,
Jürgen Urban, qemu-devel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] target/mips: Fix decoding mechanism of R5900 MFLO1, MFHI1, MTLO1 and MTHI1
2018-11-04 13:12 ` Fredrik Noring
@ 2018-11-04 15:09 ` Maciej W. Rozycki
2018-11-05 15:40 ` Fredrik Noring
0 siblings, 1 reply; 14+ messages in thread
From: Maciej W. Rozycki @ 2018-11-04 15:09 UTC (permalink / raw)
To: Fredrik Noring
Cc: Richard Henderson, Philippe Mathieu-Daudé,
Aleksandar Markovic, Aurelien Jarno, Jürgen Urban,
qemu-devel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 0/2] target/mips: Fix decoding mechanisms of R5900 M{F, T}{HI, LO}1 and DIV[U]1
2018-11-02 16:07 [Qemu-devel] [PATCH 0/2] target/mips: Fix decoding mechanisms of R5900 M{F, T}{HI, LO}1 and DIV[U]1 Fredrik Noring
2018-11-02 16:08 ` [Qemu-devel] [PATCH 1/2] target/mips: Fix decoding mechanism of R5900 MFLO1, MFHI1, MTLO1 and MTHI1 Fredrik Noring
2018-11-02 16:08 ` [Qemu-devel] [PATCH 2/2] target/mips: Fix decoding mechanism of R5900 DIV1 and DIVU1 Fredrik Noring
@ 2018-11-05 13:18 ` Aleksandar Markovic
2018-11-05 18:12 ` Fredrik Noring
2 siblings, 1 reply; 14+ messages in thread
From: Aleksandar Markovic @ 2018-11-05 13:18 UTC (permalink / raw)
To: Fredrik Noring, Aurelien Jarno, Philippe Mathieu-Daudé
Cc: Jürgen Urban, Maciej W. Rozycki, qemu-devel
> 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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] target/mips: Fix decoding mechanism of R5900 MFLO1, MFHI1, MTLO1 and MTHI1
2018-11-04 15:09 ` Maciej W. Rozycki
@ 2018-11-05 15:40 ` Fredrik Noring
2018-11-05 16:06 ` Maciej W. Rozycki
0 siblings, 1 reply; 14+ messages in thread
From: Fredrik Noring @ 2018-11-05 15:40 UTC (permalink / raw)
To: Maciej W. Rozycki
Cc: Richard Henderson, Philippe Mathieu-Daudé,
Aleksandar Markovic, Aurelien Jarno, Jürgen Urban, Jia Liu,
qemu-devel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] target/mips: Fix decoding mechanism of R5900 MFLO1, MFHI1, MTLO1 and MTHI1
2018-11-05 15:40 ` Fredrik Noring
@ 2018-11-05 16:06 ` Maciej W. Rozycki
0 siblings, 0 replies; 14+ messages in thread
From: Maciej W. Rozycki @ 2018-11-05 16:06 UTC (permalink / raw)
To: Fredrik Noring
Cc: Richard Henderson, Philippe Mathieu-Daudé,
Aleksandar Markovic, Aurelien Jarno, Jürgen Urban, Jia Liu,
qemu-devel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 0/2] target/mips: Fix decoding mechanisms of R5900 M{F, T}{HI, LO}1 and DIV[U]1
2018-11-05 13:18 ` [Qemu-devel] [PATCH 0/2] target/mips: Fix decoding mechanisms of R5900 M{F, T}{HI, LO}1 and DIV[U]1 Aleksandar Markovic
@ 2018-11-05 18:12 ` Fredrik Noring
2018-11-05 18:58 ` Aleksandar Markovic
0 siblings, 1 reply; 14+ messages in thread
From: Fredrik Noring @ 2018-11-05 18:12 UTC (permalink / raw)
To: Aleksandar Markovic
Cc: Aurelien Jarno, Philippe Mathieu-Daudé,
Jürgen Urban, Maciej W. Rozycki, qemu-devel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 0/2] target/mips: Fix decoding mechanisms of R5900 M{F, T}{HI, LO}1 and DIV[U]1
2018-11-05 18:12 ` Fredrik Noring
@ 2018-11-05 18:58 ` Aleksandar Markovic
2018-11-07 19:17 ` Fredrik Noring
0 siblings, 1 reply; 14+ messages in thread
From: Aleksandar Markovic @ 2018-11-05 18:58 UTC (permalink / raw)
To: Fredrik Noring
Cc: Aurelien Jarno, Philippe Mathieu-Daudé,
Jürgen Urban, Maciej W. Rozycki, qemu-devel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH 0/2] target/mips: Fix decoding mechanisms of R5900 M{F, T}{HI, LO}1 and DIV[U]1
2018-11-05 18:58 ` Aleksandar Markovic
@ 2018-11-07 19:17 ` Fredrik Noring
0 siblings, 0 replies; 14+ messages in thread
From: Fredrik Noring @ 2018-11-07 19:17 UTC (permalink / raw)
To: Aleksandar Markovic
Cc: Aurelien Jarno, Philippe Mathieu-Daudé,
Jürgen Urban, Maciej W. Rozycki, qemu-devel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2018-11-07 19:17 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-02 16:07 [Qemu-devel] [PATCH 0/2] target/mips: Fix decoding mechanisms of R5900 M{F, T}{HI, LO}1 and DIV[U]1 Fredrik Noring
2018-11-02 16:08 ` [Qemu-devel] [PATCH 1/2] target/mips: Fix decoding mechanism of R5900 MFLO1, MFHI1, MTLO1 and MTHI1 Fredrik Noring
2018-11-02 18:10 ` Philippe Mathieu-Daudé
2018-11-04 10:15 ` Richard Henderson
2018-11-04 13:12 ` Fredrik Noring
2018-11-04 15:09 ` Maciej W. Rozycki
2018-11-05 15:40 ` Fredrik Noring
2018-11-05 16:06 ` Maciej W. Rozycki
2018-11-02 16:08 ` [Qemu-devel] [PATCH 2/2] target/mips: Fix decoding mechanism of R5900 DIV1 and DIVU1 Fredrik Noring
2018-11-02 18:15 ` Philippe Mathieu-Daudé
2018-11-05 13:18 ` [Qemu-devel] [PATCH 0/2] target/mips: Fix decoding mechanisms of R5900 M{F, T}{HI, LO}1 and DIV[U]1 Aleksandar Markovic
2018-11-05 18:12 ` Fredrik Noring
2018-11-05 18:58 ` Aleksandar Markovic
2018-11-07 19:17 ` Fredrik Noring
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.