QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/5] target/mips: Make check_cp0_enabled() return a boolean
@ 2021-04-20 19:34 Philippe Mathieu-Daudé
  2021-04-20 19:34 ` [PATCH 1/5] target/mips: Remove duplicated check_cp0_enabled() calls in gen_cp0() Philippe Mathieu-Daudé
                   ` (4 more replies)
  0 siblings, 5 replies; 12+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-04-20 19:34 UTC (permalink / raw)
  To: qemu-devel
  Cc: Richard Henderson, Philippe Mathieu-Daudé,
	Aurelien Jarno, Aleksandar Rikalo

Address the following remark from Richard:\r
\r
 (1) check_cp0_enabled must return a boolean, so that the\r
     caller can avoid emitting dead code after the\r
     exception is emitted.\r
\r
https://www.mail-archive.com/qemu-devel@nongnu.org/msg800114.html\r
\r
Based-on: <20210420175426.1875746-1-f4bug@amsat.org>\r
\r
Philippe Mathieu-Daudé (5):\r
  target/mips: Remove duplicated check_cp0_enabled() calls in gen_cp0()\r
  target/mips: Simplify CP0 check in nanoMIPS P.LS.E0 EVA pool\r
  target/mips: Make check_cp0_enabled() return a boolean\r
  target/mips: Use check_cp0_enabled() returned value\r
  target/mips: Restrict EVA opcodes to system emulation\r
\r
 target/mips/translate.h |   7 +-\r
 target/mips/translate.c | 178 ++++++++++++++++++++--------------------\r
 2 files changed, 96 insertions(+), 89 deletions(-)\r
\r
-- \r
2.26.3\r
\r


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/5] target/mips: Remove duplicated check_cp0_enabled() calls in gen_cp0()
  2021-04-20 19:34 [PATCH 0/5] target/mips: Make check_cp0_enabled() return a boolean Philippe Mathieu-Daudé
@ 2021-04-20 19:34 ` Philippe Mathieu-Daudé
  2021-04-21  1:31   ` Richard Henderson
  2021-04-21  1:37   ` Richard Henderson
  2021-04-20 19:34 ` [PATCH 2/5] target/mips: Simplify CP0 check in nanoMIPS P.LS.E0 EVA pool Philippe Mathieu-Daudé
                   ` (3 subsequent siblings)
  4 siblings, 2 replies; 12+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-04-20 19:34 UTC (permalink / raw)
  To: qemu-devel
  Cc: Richard Henderson, Philippe Mathieu-Daudé,
	Aurelien Jarno, Aleksandar Rikalo

We already check for CP0 enabled at the beginning of gen_cp0(),
no need to check it again after.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 target/mips/translate.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/target/mips/translate.c b/target/mips/translate.c
index 5dad75cdf37..9acca6ef045 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -9477,7 +9477,6 @@ static void gen_cp0(CPUMIPSState *env, DisasContext *ctx, uint32_t opc,
         opn = "mthc0";
         break;
     case OPC_MFTR:
-        check_cp0_enabled(ctx);
         if (rd == 0) {
             /* Treat as NOP. */
             return;
@@ -9487,7 +9486,6 @@ static void gen_cp0(CPUMIPSState *env, DisasContext *ctx, uint32_t opc,
         opn = "mftr";
         break;
     case OPC_MTTR:
-        check_cp0_enabled(ctx);
         gen_mttr(env, ctx, rd, rt, (ctx->opcode >> 5) & 1,
                  ctx->opcode & 0x7, (ctx->opcode >> 4) & 1);
         opn = "mttr";
-- 
2.26.3



^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 2/5] target/mips: Simplify CP0 check in nanoMIPS P.LS.E0 EVA pool
  2021-04-20 19:34 [PATCH 0/5] target/mips: Make check_cp0_enabled() return a boolean Philippe Mathieu-Daudé
  2021-04-20 19:34 ` [PATCH 1/5] target/mips: Remove duplicated check_cp0_enabled() calls in gen_cp0() Philippe Mathieu-Daudé
@ 2021-04-20 19:34 ` Philippe Mathieu-Daudé
  2021-04-21  1:35   ` Richard Henderson
  2021-04-20 19:34 ` [PATCH 3/5] target/mips: Make check_cp0_enabled() return a boolean Philippe Mathieu-Daudé
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 12+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-04-20 19:34 UTC (permalink / raw)
  To: qemu-devel
  Cc: Richard Henderson, Philippe Mathieu-Daudé,
	Aurelien Jarno, Aleksandar Rikalo

The nanoMIPS P.LS.E0 pool contains the EVA instructions,
which are privileged. Simplify by moving the CP0 check
at the top of the pool swich case.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 target/mips/translate.c | 16 +---------------
 1 file changed, 1 insertion(+), 15 deletions(-)

diff --git a/target/mips/translate.c b/target/mips/translate.c
index 9acca6ef045..03fb67f6f22 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -20906,27 +20906,24 @@ static int decode_nanomips_32_48_opc(CPUMIPSState *env, DisasContext *ctx)
                 }
                 break;
             case NM_P_LS_E0:
+                check_cp0_enabled(ctx);
                 switch (extract32(ctx->opcode, 11, 4)) {
                 case NM_LBE:
                     check_eva(ctx);
-                    check_cp0_enabled(ctx);
                     gen_ld(ctx, OPC_LBE, rt, rs, s);
                     break;
                 case NM_SBE:
                     check_eva(ctx);
-                    check_cp0_enabled(ctx);
                     gen_st(ctx, OPC_SBE, rt, rs, s);
                     break;
                 case NM_LBUE:
                     check_eva(ctx);
-                    check_cp0_enabled(ctx);
                     gen_ld(ctx, OPC_LBUE, rt, rs, s);
                     break;
                 case NM_P_PREFE:
                     if (rt == 31) {
                         /* case NM_SYNCIE */
                         check_eva(ctx);
-                        check_cp0_enabled(ctx);
                         /*
                          * Break the TB to be able to sync copied instructions
                          * immediately.
@@ -20935,39 +20932,32 @@ static int decode_nanomips_32_48_opc(CPUMIPSState *env, DisasContext *ctx)
                     } else {
                         /* case NM_PREFE */
                         check_eva(ctx);
-                        check_cp0_enabled(ctx);
                         /* Treat as NOP. */
                     }
                     break;
                 case NM_LHE:
                     check_eva(ctx);
-                    check_cp0_enabled(ctx);
                     gen_ld(ctx, OPC_LHE, rt, rs, s);
                     break;
                 case NM_SHE:
                     check_eva(ctx);
-                    check_cp0_enabled(ctx);
                     gen_st(ctx, OPC_SHE, rt, rs, s);
                     break;
                 case NM_LHUE:
                     check_eva(ctx);
-                    check_cp0_enabled(ctx);
                     gen_ld(ctx, OPC_LHUE, rt, rs, s);
                     break;
                 case NM_CACHEE:
                     check_eva(ctx);
-                    check_cp0_enabled(ctx);
                     check_nms_dl_il_sl_tl_l2c(ctx);
                     gen_cache_operation(ctx, rt, rs, s);
                     break;
                 case NM_LWE:
                     check_eva(ctx);
-                    check_cp0_enabled(ctx);
                     gen_ld(ctx, OPC_LWE, rt, rs, s);
                     break;
                 case NM_SWE:
                     check_eva(ctx);
-                    check_cp0_enabled(ctx);
                     gen_st(ctx, OPC_SWE, rt, rs, s);
                     break;
                 case NM_P_LLE:
@@ -20975,13 +20965,11 @@ static int decode_nanomips_32_48_opc(CPUMIPSState *env, DisasContext *ctx)
                     case NM_LLE:
                         check_xnp(ctx);
                         check_eva(ctx);
-                        check_cp0_enabled(ctx);
                         gen_ld(ctx, OPC_LLE, rt, rs, s);
                         break;
                     case NM_LLWPE:
                         check_xnp(ctx);
                         check_eva(ctx);
-                        check_cp0_enabled(ctx);
                         gen_llwp(ctx, rs, 0, rt, extract32(ctx->opcode, 3, 5));
                         break;
                     default:
@@ -20994,13 +20982,11 @@ static int decode_nanomips_32_48_opc(CPUMIPSState *env, DisasContext *ctx)
                     case NM_SCE:
                         check_xnp(ctx);
                         check_eva(ctx);
-                        check_cp0_enabled(ctx);
                         gen_st_cond(ctx, rt, rs, s, MO_TESL, true);
                         break;
                     case NM_SCWPE:
                         check_xnp(ctx);
                         check_eva(ctx);
-                        check_cp0_enabled(ctx);
                         gen_scwp(ctx, rs, 0, rt, extract32(ctx->opcode, 3, 5),
                                  true);
                         break;
-- 
2.26.3



^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 3/5] target/mips: Make check_cp0_enabled() return a boolean
  2021-04-20 19:34 [PATCH 0/5] target/mips: Make check_cp0_enabled() return a boolean Philippe Mathieu-Daudé
  2021-04-20 19:34 ` [PATCH 1/5] target/mips: Remove duplicated check_cp0_enabled() calls in gen_cp0() Philippe Mathieu-Daudé
  2021-04-20 19:34 ` [PATCH 2/5] target/mips: Simplify CP0 check in nanoMIPS P.LS.E0 EVA pool Philippe Mathieu-Daudé
@ 2021-04-20 19:34 ` Philippe Mathieu-Daudé
  2021-04-21  1:35   ` Richard Henderson
  2021-04-20 19:34 ` [PATCH 4/5] target/mips: Use check_cp0_enabled() returned value Philippe Mathieu-Daudé
  2021-04-20 19:34 ` [RFC PATCH 5/5] target/mips: Restrict EVA opcodes to system emulation Philippe Mathieu-Daudé
  4 siblings, 1 reply; 12+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-04-20 19:34 UTC (permalink / raw)
  To: qemu-devel
  Cc: Richard Henderson, Philippe Mathieu-Daudé,
	Aurelien Jarno, Aleksandar Rikalo

To avoid callers to emit dead code if check_cp0_enabled()
raise an exception, let it return a boolean value, whether
CP0 is enabled or not.

Suggested-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 target/mips/translate.h | 7 ++++++-
 target/mips/translate.c | 4 +++-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/target/mips/translate.h b/target/mips/translate.h
index 2b3c7a69ec6..61442590340 100644
--- a/target/mips/translate.h
+++ b/target/mips/translate.h
@@ -120,7 +120,12 @@ void gen_reserved_instruction(DisasContext *ctx);
 
 void check_insn(DisasContext *ctx, uint64_t flags);
 void check_mips_64(DisasContext *ctx);
-void check_cp0_enabled(DisasContext *ctx);
+/**
+ * check_cp0_enabled:
+ * Return %true if CP0 is enabled, otherwise return %false
+ * and emit a 'coprocessor unusable' exception.
+ */
+bool check_cp0_enabled(DisasContext *ctx);
 void check_cp1_enabled(DisasContext *ctx);
 void check_cp1_64bitmode(DisasContext *ctx);
 void check_cp1_registers(DisasContext *ctx, int regs);
diff --git a/target/mips/translate.c b/target/mips/translate.c
index 03fb67f6f22..be5382b41f2 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -1572,11 +1572,13 @@ void gen_move_high32(TCGv ret, TCGv_i64 arg)
 #endif
 }
 
-void check_cp0_enabled(DisasContext *ctx)
+bool check_cp0_enabled(DisasContext *ctx)
 {
     if (unlikely(!(ctx->hflags & MIPS_HFLAG_CP0))) {
         generate_exception_end(ctx, EXCP_CpU);
+        return false;
     }
+    return true;
 }
 
 void check_cp1_enabled(DisasContext *ctx)
-- 
2.26.3



^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 4/5] target/mips: Use check_cp0_enabled() returned value
  2021-04-20 19:34 [PATCH 0/5] target/mips: Make check_cp0_enabled() return a boolean Philippe Mathieu-Daudé
                   ` (2 preceding siblings ...)
  2021-04-20 19:34 ` [PATCH 3/5] target/mips: Make check_cp0_enabled() return a boolean Philippe Mathieu-Daudé
@ 2021-04-20 19:34 ` Philippe Mathieu-Daudé
  2021-04-21  1:43   ` Richard Henderson
  2021-04-20 19:34 ` [RFC PATCH 5/5] target/mips: Restrict EVA opcodes to system emulation Philippe Mathieu-Daudé
  4 siblings, 1 reply; 12+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-04-20 19:34 UTC (permalink / raw)
  To: qemu-devel
  Cc: Richard Henderson, Philippe Mathieu-Daudé,
	Aurelien Jarno, Aleksandar Rikalo

If CP0 is disabled, it is pointless to emit more code,
since the 'coprocessor unusable' exception will be raised.
Use the returned value from check_cp0_enabled() to return
early.

Reported-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 target/mips/translate.c | 144 +++++++++++++++++++++-------------------
 1 file changed, 76 insertions(+), 68 deletions(-)

diff --git a/target/mips/translate.c b/target/mips/translate.c
index be5382b41f2..dfa26569077 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -9417,7 +9417,9 @@ static void gen_cp0(CPUMIPSState *env, DisasContext *ctx, uint32_t opc,
 {
     const char *opn = "ldst";
 
-    check_cp0_enabled(ctx);
+    if (!check_cp0_enabled(ctx)) {
+        return;
+    }
     switch (opc) {
     case OPC_MFC0:
         if (rt == 0) {
@@ -14651,17 +14653,17 @@ static void gen_pool32axf(CPUMIPSState *env, DisasContext *ctx, int rt, int rs)
 #ifndef CONFIG_USER_ONLY
     case MFC0:
     case MFC0 + 32:
-        check_cp0_enabled(ctx);
-        if (rt == 0) {
-            /* Treat as NOP. */
-            break;
+        if (check_cp0_enabled(ctx)) {
+            if (rt == 0) {
+                /* Treat as NOP. */
+                break;
+            }
+            gen_mfc0(ctx, cpu_gpr[rt], rs, (ctx->opcode >> 11) & 0x7);
         }
-        gen_mfc0(ctx, cpu_gpr[rt], rs, (ctx->opcode >> 11) & 0x7);
         break;
     case MTC0:
     case MTC0 + 32:
-        check_cp0_enabled(ctx);
-        {
+        if (check_cp0_enabled(ctx)) {
             TCGv t0 = tcg_temp_new();
 
             gen_load_gpr(t0, rt);
@@ -14809,14 +14811,15 @@ static void gen_pool32axf(CPUMIPSState *env, DisasContext *ctx, int rt, int rs)
         }
         break;
     case 0x05:
+        if (!check_cp0_enabled(ctx)) {
+            break;
+        }
         switch (minor) {
         case RDPGPR:
-            check_cp0_enabled(ctx);
             check_insn(ctx, ISA_MIPS_R2);
             gen_load_srsgpr(rs, rt);
             break;
         case WRPGPR:
-            check_cp0_enabled(ctx);
             check_insn(ctx, ISA_MIPS_R2);
             gen_store_srsgpr(rs, rt);
             break;
@@ -14863,8 +14866,7 @@ static void gen_pool32axf(CPUMIPSState *env, DisasContext *ctx, int rt, int rs)
     case 0x1d:
         switch (minor) {
         case DI:
-            check_cp0_enabled(ctx);
-            {
+            if (check_cp0_enabled(ctx)) {
                 TCGv t0 = tcg_temp_new();
 
                 save_cpu_state(ctx, 1);
@@ -14879,8 +14881,7 @@ static void gen_pool32axf(CPUMIPSState *env, DisasContext *ctx, int rt, int rs)
             }
             break;
         case EI:
-            check_cp0_enabled(ctx);
-            {
+            if (check_cp0_enabled(ctx)) {
                 TCGv t0 = tcg_temp_new();
 
                 save_cpu_state(ctx, 1);
@@ -15449,8 +15450,7 @@ static void decode_micromips32_opc(CPUMIPSState *env, DisasContext *ctx)
         minor = (ctx->opcode >> 12) & 0xf;
         switch (minor) {
         case CACHE:
-            check_cp0_enabled(ctx);
-            if (ctx->hflags & MIPS_HFLAG_ITC_CACHE) {
+            if (check_cp0_enabled(ctx) && ctx->hflags & MIPS_HFLAG_ITC_CACHE) {
                 gen_cache_operation(ctx, rt, rs, imm);
             }
             break;
@@ -16211,7 +16211,9 @@ static void decode_micromips32_opc(CPUMIPSState *env, DisasContext *ctx)
                 gen_reserved_instruction(ctx);
                 break;
             }
-            check_cp0_enabled(ctx);
+            if (!check_cp0_enabled(ctx)) {
+                break;
+            }
 
             minor2 = (ctx->opcode >> 9) & 0x7;
             offset = sextract32(ctx->opcode, 0, 9);
@@ -16250,7 +16252,9 @@ static void decode_micromips32_opc(CPUMIPSState *env, DisasContext *ctx)
                 gen_reserved_instruction(ctx);
                 break;
             }
-            check_cp0_enabled(ctx);
+            if (!check_cp0_enabled(ctx)) {
+                break;
+            }
 
             minor2 = (ctx->opcode >> 9) & 0x7;
             offset = sextract32(ctx->opcode, 0, 9);
@@ -17995,24 +17999,24 @@ static void gen_pool32a0_nanomips_insn(CPUMIPSState *env, DisasContext *ctx)
         if (rd == 0) {
             /* P_DVP */
 #ifndef CONFIG_USER_ONLY
-            TCGv t0 = tcg_temp_new();
-            switch (extract32(ctx->opcode, 10, 1)) {
-            case NM_DVP:
-                if (ctx->vp) {
-                    check_cp0_enabled(ctx);
-                    gen_helper_dvp(t0, cpu_env);
-                    gen_store_gpr(t0, rt);
+            if (check_cp0_enabled(ctx)) {
+                TCGv t0 = tcg_temp_new();
+                switch (extract32(ctx->opcode, 10, 1)) {
+                case NM_DVP:
+                    if (ctx->vp) {
+                        gen_helper_dvp(t0, cpu_env);
+                        gen_store_gpr(t0, rt);
+                    }
+                    break;
+                case NM_EVP:
+                    if (ctx->vp) {
+                        gen_helper_evp(t0, cpu_env);
+                        gen_store_gpr(t0, rt);
+                    }
+                    break;
                 }
-                break;
-            case NM_EVP:
-                if (ctx->vp) {
-                    check_cp0_enabled(ctx);
-                    gen_helper_evp(t0, cpu_env);
-                    gen_store_gpr(t0, rt);
-                }
-                break;
+                tcg_temp_free(t0);
             }
-            tcg_temp_free(t0);
 #endif
         } else {
             gen_slt(ctx, OPC_SLTU, rd, rs, rt);
@@ -18067,16 +18071,16 @@ static void gen_pool32a0_nanomips_insn(CPUMIPSState *env, DisasContext *ctx)
         break;
 #ifndef CONFIG_USER_ONLY
     case NM_MFC0:
-        check_cp0_enabled(ctx);
-        if (rt == 0) {
-            /* Treat as NOP. */
-            break;
+        if (check_cp0_enabled(ctx)) {
+            if (rt == 0) {
+                /* Treat as NOP. */
+                break;
+            }
+            gen_mfc0(ctx, cpu_gpr[rt], rs, extract32(ctx->opcode, 11, 3));
         }
-        gen_mfc0(ctx, cpu_gpr[rt], rs, extract32(ctx->opcode, 11, 3));
         break;
     case NM_MTC0:
-        check_cp0_enabled(ctx);
-        {
+        if (check_cp0_enabled(ctx)) {
             TCGv t0 = tcg_temp_new();
 
             gen_load_gpr(t0, rt);
@@ -18140,19 +18144,23 @@ static void gen_pool32a0_nanomips_insn(CPUMIPSState *env, DisasContext *ctx)
         break;
     case NM_MFTR:
     case NM_MFHTR:
-        check_cp0_enabled(ctx);
-        if (rd == 0) {
-            /* Treat as NOP. */
-            return;
+        if (check_cp0_enabled(ctx)) {
+            if (rd == 0) {
+                /* Treat as NOP. */
+                return;
+            }
+            gen_mftr(env, ctx, rs, rt, extract32(ctx->opcode, 10, 1),
+                     extract32(ctx->opcode, 11, 5),
+                     extract32(ctx->opcode, 3, 1));
         }
-        gen_mftr(env, ctx, rs, rt, extract32(ctx->opcode, 10, 1),
-                 extract32(ctx->opcode, 11, 5), extract32(ctx->opcode, 3, 1));
         break;
     case NM_MTTR:
     case NM_MTHTR:
-        check_cp0_enabled(ctx);
-        gen_mttr(env, ctx, rs, rt, extract32(ctx->opcode, 10, 1),
-                 extract32(ctx->opcode, 11, 5), extract32(ctx->opcode, 3, 1));
+        if (check_cp0_enabled(ctx)) {
+            gen_mttr(env, ctx, rs, rt, extract32(ctx->opcode, 10, 1),
+                     extract32(ctx->opcode, 11, 5),
+                     extract32(ctx->opcode, 3, 1));
+        }
         break;
     case NM_YIELD:
         check_mt(ctx);
@@ -18943,8 +18951,7 @@ static void gen_pool32axf_nanomips_insn(CPUMIPSState *env, DisasContext *ctx)
             gen_cp0(env, ctx, OPC_TLBINVF, 0, 0);
             break;
         case NM_DI:
-            check_cp0_enabled(ctx);
-            {
+            if (check_cp0_enabled(ctx)) {
                 TCGv t0 = tcg_temp_new();
 
                 save_cpu_state(ctx, 1);
@@ -18956,8 +18963,7 @@ static void gen_pool32axf_nanomips_insn(CPUMIPSState *env, DisasContext *ctx)
             }
             break;
         case NM_EI:
-            check_cp0_enabled(ctx);
-            {
+            if (check_cp0_enabled(ctx)) {
                 TCGv t0 = tcg_temp_new();
 
                 save_cpu_state(ctx, 1);
@@ -20900,15 +20906,17 @@ static int decode_nanomips_32_48_opc(CPUMIPSState *env, DisasContext *ctx)
                     }
                     break;
                 case NM_CACHE:
-                    check_cp0_enabled(ctx);
-                    if (ctx->hflags & MIPS_HFLAG_ITC_CACHE) {
+                    if (check_cp0_enabled(ctx)
+                            && ctx->hflags & MIPS_HFLAG_ITC_CACHE) {
                         gen_cache_operation(ctx, rt, rs, s);
                     }
                     break;
                 }
                 break;
             case NM_P_LS_E0:
-                check_cp0_enabled(ctx);
+                if (check_cp0_enabled(ctx)) {
+                    break;
+                }
                 switch (extract32(ctx->opcode, 11, 4)) {
                 case NM_LBE:
                     check_eva(ctx);
@@ -23770,8 +23778,7 @@ static void decode_opc_special3_r6(CPUMIPSState *env, DisasContext *ctx)
         /* Treat as NOP. */
         break;
     case R6_OPC_CACHE:
-        check_cp0_enabled(ctx);
-        if (ctx->hflags & MIPS_HFLAG_ITC_CACHE) {
+        if (check_cp0_enabled(ctx) && ctx->hflags & MIPS_HFLAG_ITC_CACHE) {
             gen_cache_operation(ctx, rt, rs, imm);
         }
         break;
@@ -23806,7 +23813,9 @@ static void decode_opc_special3_r6(CPUMIPSState *env, DisasContext *ctx)
         if (unlikely(ctx->gi <= 1)) {
             gen_reserved_instruction(ctx);
         }
-        check_cp0_enabled(ctx);
+        if (!check_cp0_enabled(ctx)) {
+            break;
+        }
         switch ((ctx->opcode >> 6) & 3) {
         case 0:    /* GINVI */
             /* Treat as NOP. */
@@ -24493,6 +24502,9 @@ static void decode_opc_special3(CPUMIPSState *env, DisasContext *ctx)
      * EVA is absent.
      */
     if (ctx->eva) {
+        if (!check_cp0_enabled(ctx)) {
+            return;
+        }
         switch (op1) {
         case OPC_LWLE:
         case OPC_LWRE:
@@ -24502,7 +24514,6 @@ static void decode_opc_special3(CPUMIPSState *env, DisasContext *ctx)
         case OPC_LHE:
         case OPC_LLE:
         case OPC_LWE:
-            check_cp0_enabled(ctx);
             gen_ld(ctx, op1, rt, rs, imm);
             return;
         case OPC_SWLE:
@@ -24510,22 +24521,18 @@ static void decode_opc_special3(CPUMIPSState *env, DisasContext *ctx)
         case OPC_SBE:
         case OPC_SHE:
         case OPC_SWE:
-            check_cp0_enabled(ctx);
             gen_st(ctx, op1, rt, rs, imm);
             return;
         case OPC_SCE:
-            check_cp0_enabled(ctx);
             gen_st_cond(ctx, rt, rs, imm, MO_TESL, true);
             return;
         case OPC_CACHEE:
             check_eva(ctx);
-            check_cp0_enabled(ctx);
             if (ctx->hflags & MIPS_HFLAG_ITC_CACHE) {
                 gen_cache_operation(ctx, rt, rs, imm);
             }
             return;
         case OPC_PREFE:
-            check_cp0_enabled(ctx);
             /* Treat as NOP. */
             return;
         }
@@ -24750,7 +24757,9 @@ static bool decode_opc_legacy(CPUMIPSState *env, DisasContext *ctx)
         }
         break;
     case OPC_CP0:
-        check_cp0_enabled(ctx);
+        if (!check_cp0_enabled(ctx)) {
+            break;
+        }
         op1 = MASK_CP0(ctx->opcode);
         switch (op1) {
         case OPC_MFC0:
@@ -24990,9 +24999,8 @@ static bool decode_opc_legacy(CPUMIPSState *env, DisasContext *ctx)
         gen_st_cond(ctx, rt, rs, imm, MO_TESL, false);
         break;
     case OPC_CACHE:
-        check_cp0_enabled(ctx);
         check_insn(ctx, ISA_MIPS3 | ISA_MIPS_R1);
-        if (ctx->hflags & MIPS_HFLAG_ITC_CACHE) {
+        if (check_cp0_enabled(ctx) && ctx->hflags & MIPS_HFLAG_ITC_CACHE) {
             gen_cache_operation(ctx, rt, rs, imm);
         }
         /* Treat as NOP. */
-- 
2.26.3



^ permalink raw reply	[flat|nested] 12+ messages in thread

* [RFC PATCH 5/5] target/mips: Restrict EVA opcodes to system emulation
  2021-04-20 19:34 [PATCH 0/5] target/mips: Make check_cp0_enabled() return a boolean Philippe Mathieu-Daudé
                   ` (3 preceding siblings ...)
  2021-04-20 19:34 ` [PATCH 4/5] target/mips: Use check_cp0_enabled() returned value Philippe Mathieu-Daudé
@ 2021-04-20 19:34 ` Philippe Mathieu-Daudé
  2021-04-21  1:40   ` Richard Henderson
  4 siblings, 1 reply; 12+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-04-20 19:34 UTC (permalink / raw)
  To: qemu-devel
  Cc: Richard Henderson, Philippe Mathieu-Daudé,
	Aurelien Jarno, Aleksandar Rikalo

Enhanced Virtual Address (EVA) instructions are restricted
to Kernel execution mode, thus not available on user emulation.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
RFC because I'd rather not use such #ifdef'ry again.
TODO: have the compiler elide this code.

 target/mips/translate.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/target/mips/translate.c b/target/mips/translate.c
index dfa26569077..77115721a97 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -15245,8 +15245,11 @@ static void decode_micromips32_opc(CPUMIPSState *env, DisasContext *ctx)
     uint16_t insn;
     int rt, rs, rd, rr;
     int16_t imm;
-    uint32_t op, minor, minor2, mips32_op;
+    uint32_t op, minor, mips32_op;
     uint32_t cond, fmt, cc;
+#ifndef CONFIG_USER_ONLY
+    uint32_t minor2;
+#endif /* !CONFIG_USER_ONLY */
 
     insn = translator_lduw(env, ctx->base.pc_next + 2);
     ctx->opcode = (ctx->opcode << 16) | insn;
@@ -16205,6 +16208,7 @@ static void decode_micromips32_opc(CPUMIPSState *env, DisasContext *ctx)
             gen_st_cond(ctx, rt, rs, offset, MO_TEQ, false);
             break;
 #endif
+#ifndef CONFIG_USER_ONLY
         case LD_EVA:
             if (!ctx->eva) {
                 MIPS_INVAL("pool32c ld-eva");
@@ -16294,6 +16298,7 @@ static void decode_micromips32_opc(CPUMIPSState *env, DisasContext *ctx)
                 goto do_st_lr;
             };
             break;
+#endif /* !CONFIG_USER_ONLY */
         case PREF:
             /* Treat as no-op */
             if ((ctx->insn_flags & ISA_MIPS_R6) && (rt >= 24)) {
@@ -24486,16 +24491,18 @@ static void decode_opc_special3(CPUMIPSState *env, DisasContext *ctx)
 {
     int rs, rt, rd, sa;
     uint32_t op1, op2;
-    int16_t imm;
 
     rs = (ctx->opcode >> 21) & 0x1f;
     rt = (ctx->opcode >> 16) & 0x1f;
     rd = (ctx->opcode >> 11) & 0x1f;
     sa = (ctx->opcode >> 6) & 0x1f;
-    imm = sextract32(ctx->opcode, 7, 9);
 
     op1 = MASK_SPECIAL3(ctx->opcode);
 
+#ifndef CONFIG_USER_ONLY
+    int16_t imm;
+
+    imm = sextract32(ctx->opcode, 7, 9);
     /*
      * EVA loads and stores overlap Loongson 2E instructions decoded by
      * decode_opc_special3_legacy(), so be careful to allow their decoding when
@@ -24537,6 +24544,7 @@ static void decode_opc_special3(CPUMIPSState *env, DisasContext *ctx)
             return;
         }
     }
+#endif /* !CONFIG_USER_ONLY */
 
     switch (op1) {
     case OPC_EXT:
-- 
2.26.3



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/5] target/mips: Remove duplicated check_cp0_enabled() calls in gen_cp0()
  2021-04-20 19:34 ` [PATCH 1/5] target/mips: Remove duplicated check_cp0_enabled() calls in gen_cp0() Philippe Mathieu-Daudé
@ 2021-04-21  1:31   ` Richard Henderson
  2021-04-21  1:37   ` Richard Henderson
  1 sibling, 0 replies; 12+ messages in thread
From: Richard Henderson @ 2021-04-21  1:31 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-devel; +Cc: Aleksandar Rikalo, Aurelien Jarno

On 4/20/21 12:34 PM, Philippe Mathieu-Daudé wrote:
> We already check for CP0 enabled at the beginning of gen_cp0(),
> no need to check it again after.
> 
> Signed-off-by: Philippe Mathieu-Daudé<f4bug@amsat.org>
> ---
>   target/mips/translate.c | 2 --
>   1 file changed, 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/5] target/mips: Simplify CP0 check in nanoMIPS P.LS.E0 EVA pool
  2021-04-20 19:34 ` [PATCH 2/5] target/mips: Simplify CP0 check in nanoMIPS P.LS.E0 EVA pool Philippe Mathieu-Daudé
@ 2021-04-21  1:35   ` Richard Henderson
  0 siblings, 0 replies; 12+ messages in thread
From: Richard Henderson @ 2021-04-21  1:35 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-devel; +Cc: Aleksandar Rikalo, Aurelien Jarno

On 4/20/21 12:34 PM, Philippe Mathieu-Daudé wrote:
> The nanoMIPS P.LS.E0 pool contains the EVA instructions,
> which are privileged. Simplify by moving the CP0 check
> at the top of the pool swich case.
> 
> Signed-off-by: Philippe Mathieu-Daudé<f4bug@amsat.org>
> ---
>   target/mips/translate.c | 16 +---------------
>   1 file changed, 1 insertion(+), 15 deletions(-)

This isn't the same for the default case, raising RI instead of CuP.


r~


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/5] target/mips: Make check_cp0_enabled() return a boolean
  2021-04-20 19:34 ` [PATCH 3/5] target/mips: Make check_cp0_enabled() return a boolean Philippe Mathieu-Daudé
@ 2021-04-21  1:35   ` Richard Henderson
  0 siblings, 0 replies; 12+ messages in thread
From: Richard Henderson @ 2021-04-21  1:35 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-devel; +Cc: Aleksandar Rikalo, Aurelien Jarno

On 4/20/21 12:34 PM, Philippe Mathieu-Daudé wrote:
> To avoid callers to emit dead code if check_cp0_enabled()
> raise an exception, let it return a boolean value, whether
> CP0 is enabled or not.
> 
> Suggested-by: Richard Henderson<richard.henderson@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé<f4bug@amsat.org>
> ---
>   target/mips/translate.h | 7 ++++++-
>   target/mips/translate.c | 4 +++-
>   2 files changed, 9 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/5] target/mips: Remove duplicated check_cp0_enabled() calls in gen_cp0()
  2021-04-20 19:34 ` [PATCH 1/5] target/mips: Remove duplicated check_cp0_enabled() calls in gen_cp0() Philippe Mathieu-Daudé
  2021-04-21  1:31   ` Richard Henderson
@ 2021-04-21  1:37   ` Richard Henderson
  1 sibling, 0 replies; 12+ messages in thread
From: Richard Henderson @ 2021-04-21  1:37 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-devel; +Cc: Aleksandar Rikalo, Aurelien Jarno

On 4/20/21 12:34 PM, Philippe Mathieu-Daudé wrote:
> We already check for CP0 enabled at the beginning of gen_cp0(),
> no need to check it again after.
> 
> Signed-off-by: Philippe Mathieu-Daudé<f4bug@amsat.org>
> ---
>   target/mips/translate.c | 2 --
>   1 file changed, 2 deletions(-)

Having noticed the default case for the nanomips decode, I do wonder if having 
the cp0 check happen before the switch in gen_cp0 is actually correct.


r~


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC PATCH 5/5] target/mips: Restrict EVA opcodes to system emulation
  2021-04-20 19:34 ` [RFC PATCH 5/5] target/mips: Restrict EVA opcodes to system emulation Philippe Mathieu-Daudé
@ 2021-04-21  1:40   ` Richard Henderson
  0 siblings, 0 replies; 12+ messages in thread
From: Richard Henderson @ 2021-04-21  1:40 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-devel; +Cc: Aleksandar Rikalo, Aurelien Jarno

On 4/20/21 12:34 PM, Philippe Mathieu-Daudé wrote:
> Enhanced Virtual Address (EVA) instructions are restricted
> to Kernel execution mode, thus not available on user emulation.
> 
> Signed-off-by: Philippe Mathieu-Daudé<f4bug@amsat.org>
> ---
> RFC because I'd rather not use such #ifdef'ry again.
> TODO: have the compiler elide this code.
> 
>   target/mips/translate.c | 14 +++++++++++---
>   1 file changed, 11 insertions(+), 3 deletions(-)

Yeah, I would approach this via a smaller ifdef in check_cp0_enabled first.


r~


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 4/5] target/mips: Use check_cp0_enabled() returned value
  2021-04-20 19:34 ` [PATCH 4/5] target/mips: Use check_cp0_enabled() returned value Philippe Mathieu-Daudé
@ 2021-04-21  1:43   ` Richard Henderson
  0 siblings, 0 replies; 12+ messages in thread
From: Richard Henderson @ 2021-04-21  1:43 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-devel; +Cc: Aleksandar Rikalo, Aurelien Jarno

On 4/20/21 12:34 PM, Philippe Mathieu-Daudé wrote:
> @@ -14809,14 +14811,15 @@ static void gen_pool32axf(CPUMIPSState *env, DisasContext *ctx, int rt, int rs)
>           }
>           break;
>       case 0x05:
> +        if (!check_cp0_enabled(ctx)) {
> +            break;
> +        }
>           switch (minor) {
>           case RDPGPR:
> -            check_cp0_enabled(ctx);
>               check_insn(ctx, ISA_MIPS_R2);
>               gen_load_srsgpr(rs, rt);
>               break;
>           case WRPGPR:
> -            check_cp0_enabled(ctx);
>               check_insn(ctx, ISA_MIPS_R2);
>               gen_store_srsgpr(rs, rt);
>               break;

cp0 check before decode complete.  at least 2 more instances in this patch.


r~




^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, back to index

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-20 19:34 [PATCH 0/5] target/mips: Make check_cp0_enabled() return a boolean Philippe Mathieu-Daudé
2021-04-20 19:34 ` [PATCH 1/5] target/mips: Remove duplicated check_cp0_enabled() calls in gen_cp0() Philippe Mathieu-Daudé
2021-04-21  1:31   ` Richard Henderson
2021-04-21  1:37   ` Richard Henderson
2021-04-20 19:34 ` [PATCH 2/5] target/mips: Simplify CP0 check in nanoMIPS P.LS.E0 EVA pool Philippe Mathieu-Daudé
2021-04-21  1:35   ` Richard Henderson
2021-04-20 19:34 ` [PATCH 3/5] target/mips: Make check_cp0_enabled() return a boolean Philippe Mathieu-Daudé
2021-04-21  1:35   ` Richard Henderson
2021-04-20 19:34 ` [PATCH 4/5] target/mips: Use check_cp0_enabled() returned value Philippe Mathieu-Daudé
2021-04-21  1:43   ` Richard Henderson
2021-04-20 19:34 ` [RFC PATCH 5/5] target/mips: Restrict EVA opcodes to system emulation Philippe Mathieu-Daudé
2021-04-21  1:40   ` Richard Henderson

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git
	git clone --mirror https://lore.kernel.org/qemu-devel/2 qemu-devel/git/2.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git