[Qemu-devel] [PATCH 0/3] target-mips fixes

[Qemu-devel] [PATCH 0/3] target-mips fixes

[Richard Henderson]

The first patch fixes an abort that I see when booting a mips64
kernel with the 2.4 branch with --enable-tcg-debug.

The second patch eliminates the observed symptom of the first patch,
partially confirming the diagnosis that the errant insn is in fact
data and not an instruction at all.

The third patch reduces some unwanted noise from log files.


r~


Richard Henderson (3):
  target-mips: Copy restrictions from ext/ins to dext/dins
  target-mips: Stop translation after unconditional exceptions
  target-mips: Use CPU_LOG_INT for logging related to interrupts

 target-mips/helper.c    | 30 ++++++----------
 target-mips/op_helper.c |  3 +-
 target-mips/translate.c | 91 +++++++++++++++++++++++++------------------------
 3 files changed, 60 insertions(+), 64 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH 1/3] target-mips: Copy restrictions from ext/ins to dext/dins

[Richard Henderson]

The checks in dins is required to avoid triggering an assertion
in tcg_gen_deposit_tl.  The check in dext is just for completeness.
Fold the other D cases in via fallthru.

In this case the errant dins appears to be data, not code, as
translation failed to stop after a break insn.

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 target-mips/translate.c | 44 ++++++++++++++++++++++++--------------------
 1 file changed, 24 insertions(+), 20 deletions(-)

diff --git a/target-mips/translate.c b/target-mips/translate.c
index d1de35a..2a91565 100644
--- a/target-mips/translate.c
+++ b/target-mips/translate.c
@@ -4750,48 +4750,52 @@ static void gen_bitops (DisasContext *ctx, uint32_t opc, int rt,
     gen_load_gpr(t1, rs);
     switch (opc) {
     case OPC_EXT:
-        if (lsb + msb > 31)
+        if (lsb + msb > 31) {
             goto fail;
+        }
         tcg_gen_shri_tl(t0, t1, lsb);
         if (msb != 31) {
-            tcg_gen_andi_tl(t0, t0, (1 << (msb + 1)) - 1);
+            tcg_gen_andi_tl(t0, t0, (1U << (msb + 1)) - 1);
         } else {
             tcg_gen_ext32s_tl(t0, t0);
         }
         break;
 #if defined(TARGET_MIPS64)
-    case OPC_DEXTM:
-        tcg_gen_shri_tl(t0, t1, lsb);
-        if (msb != 31) {
-            tcg_gen_andi_tl(t0, t0, (1ULL << (msb + 1 + 32)) - 1);
-        }
-        break;
     case OPC_DEXTU:
-        tcg_gen_shri_tl(t0, t1, lsb + 32);
-        tcg_gen_andi_tl(t0, t0, (1ULL << (msb + 1)) - 1);
-        break;
+        lsb += 32;
+        /* FALLTHRU */
+    case OPC_DEXTM:
+        msb += 32;
+        /* FALLTHRU */
     case OPC_DEXT:
+        if (lsb + msb > 63) {
+            goto fail;
+        }
         tcg_gen_shri_tl(t0, t1, lsb);
-        tcg_gen_andi_tl(t0, t0, (1ULL << (msb + 1)) - 1);
+        if (msb != 63) {
+            tcg_gen_andi_tl(t0, t0, (1ULL << (msb + 1)) - 1);
+        }
         break;
 #endif
     case OPC_INS:
-        if (lsb > msb)
+        if (lsb > msb) {
             goto fail;
+        }
         gen_load_gpr(t0, rt);
         tcg_gen_deposit_tl(t0, t0, t1, lsb, msb - lsb + 1);
         tcg_gen_ext32s_tl(t0, t0);
         break;
 #if defined(TARGET_MIPS64)
-    case OPC_DINSM:
-        gen_load_gpr(t0, rt);
-        tcg_gen_deposit_tl(t0, t0, t1, lsb, msb + 32 - lsb + 1);
-        break;
     case OPC_DINSU:
-        gen_load_gpr(t0, rt);
-        tcg_gen_deposit_tl(t0, t0, t1, lsb + 32, msb - lsb + 1);
-        break;
+        lsb += 32;
+        /* FALLTHRU */
+    case OPC_DINSM:
+        msb += 32;
+        /* FALLTHRU */
     case OPC_DINS:
+        if (lsb > msb) {
+            goto fail;
+        }
         gen_load_gpr(t0, rt);
         tcg_gen_deposit_tl(t0, t0, t1, lsb, msb - lsb + 1);
         break;
-- 
2.4.3

[Qemu-devel] [PATCH 2/3] target-mips: Stop translation after unconditional exceptions

[Richard Henderson]

Since there are many more unconditional exceptions than conditional,
introduce a new generate_exception_cond to mark conditionals.  Also
delete those few cases where we did attempt to stop translation after
an exception, as these are now subsumed in the change.

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 target-mips/translate.c | 47 +++++++++++++++++++++++------------------------
 1 file changed, 23 insertions(+), 24 deletions(-)

diff --git a/target-mips/translate.c b/target-mips/translate.c
index 2a91565..7d9eb91 100644
--- a/target-mips/translate.c
+++ b/target-mips/translate.c
@@ -1615,14 +1615,21 @@ static inline void generate_exception_err(DisasContext *ctx, int excp, int err)
     gen_helper_raise_exception_err(cpu_env, texcp, terr);
     tcg_temp_free_i32(terr);
     tcg_temp_free_i32(texcp);
+    ctx->bstate = BS_EXCP;
 }
 
-static inline void generate_exception(DisasContext *ctx, int excp)
+static inline void generate_exception_cond(DisasContext *ctx, int excp)
 {
     save_cpu_state(ctx, 1);
     gen_helper_0e0i(raise_exception, excp);
 }
 
+static inline void generate_exception(DisasContext *ctx, int excp)
+{
+    generate_exception_cond(ctx, excp);
+    ctx->bstate = BS_EXCP;
+}
+
 /* Floating point register moves. */
 static void gen_load_fpr32(DisasContext *ctx, TCGv_i32 t, int reg)
 {
@@ -2044,14 +2051,14 @@ static inline void op_st_##insn(TCGv arg1, TCGv arg2, int rt, DisasContext *ctx)
                                                                              \
     tcg_gen_andi_tl(t0, arg2, almask);                                       \
     tcg_gen_brcondi_tl(TCG_COND_EQ, t0, 0, l1);                              \
-    tcg_gen_st_tl(arg2, cpu_env, offsetof(CPUMIPSState, CP0_BadVAddr));          \
-    generate_exception(ctx, EXCP_AdES);                                      \
+    tcg_gen_st_tl(arg2, cpu_env, offsetof(CPUMIPSState, CP0_BadVAddr));      \
+    generate_exception_cond(ctx, EXCP_AdES);                                 \
     gen_set_label(l1);                                                       \
-    tcg_gen_ld_tl(t0, cpu_env, offsetof(CPUMIPSState, lladdr));                  \
+    tcg_gen_ld_tl(t0, cpu_env, offsetof(CPUMIPSState, lladdr));              \
     tcg_gen_brcond_tl(TCG_COND_NE, arg2, t0, l2);                            \
     tcg_gen_movi_tl(t0, rt | ((almask << 3) & 0x20));                        \
-    tcg_gen_st_tl(t0, cpu_env, offsetof(CPUMIPSState, llreg));                   \
-    tcg_gen_st_tl(arg1, cpu_env, offsetof(CPUMIPSState, llnewval));              \
+    tcg_gen_st_tl(t0, cpu_env, offsetof(CPUMIPSState, llreg));               \
+    tcg_gen_st_tl(arg1, cpu_env, offsetof(CPUMIPSState, llnewval));          \
     gen_helper_0e0i(raise_exception, EXCP_SC);                               \
     gen_set_label(l2);                                                       \
     tcg_gen_movi_tl(t0, 0);                                                  \
@@ -2506,7 +2513,7 @@ static void gen_arith_imm(DisasContext *ctx, uint32_t opc,
             tcg_gen_brcondi_tl(TCG_COND_GE, t1, 0, l1);
             tcg_temp_free(t1);
             /* operands of same sign, result different sign */
-            generate_exception(ctx, EXCP_OVERFLOW);
+            generate_exception_cond(ctx, EXCP_OVERFLOW);
             gen_set_label(l1);
             tcg_gen_ext32s_tl(t0, t0);
             gen_store_gpr(t0, rt);
@@ -2541,7 +2548,7 @@ static void gen_arith_imm(DisasContext *ctx, uint32_t opc,
             tcg_gen_brcondi_tl(TCG_COND_GE, t1, 0, l1);
             tcg_temp_free(t1);
             /* operands of same sign, result different sign */
-            generate_exception(ctx, EXCP_OVERFLOW);
+            generate_exception_cond(ctx, EXCP_OVERFLOW);
             gen_set_label(l1);
             gen_store_gpr(t0, rt);
             tcg_temp_free(t0);
@@ -2772,7 +2779,7 @@ static void gen_arith(DisasContext *ctx, uint32_t opc,
             tcg_gen_brcondi_tl(TCG_COND_GE, t1, 0, l1);
             tcg_temp_free(t1);
             /* operands of same sign, result different sign */
-            generate_exception(ctx, EXCP_OVERFLOW);
+            generate_exception_cond(ctx, EXCP_OVERFLOW);
             gen_set_label(l1);
             gen_store_gpr(t0, rd);
             tcg_temp_free(t0);
@@ -2810,7 +2817,7 @@ static void gen_arith(DisasContext *ctx, uint32_t opc,
             tcg_gen_brcondi_tl(TCG_COND_GE, t1, 0, l1);
             tcg_temp_free(t1);
             /* operands of different sign, first operand and result different sign */
-            generate_exception(ctx, EXCP_OVERFLOW);
+            generate_exception_cond(ctx, EXCP_OVERFLOW);
             gen_set_label(l1);
             gen_store_gpr(t0, rd);
             tcg_temp_free(t0);
@@ -2849,7 +2856,7 @@ static void gen_arith(DisasContext *ctx, uint32_t opc,
             tcg_gen_brcondi_tl(TCG_COND_GE, t1, 0, l1);
             tcg_temp_free(t1);
             /* operands of same sign, result different sign */
-            generate_exception(ctx, EXCP_OVERFLOW);
+            generate_exception_cond(ctx, EXCP_OVERFLOW);
             gen_set_label(l1);
             gen_store_gpr(t0, rd);
             tcg_temp_free(t0);
@@ -2885,7 +2892,7 @@ static void gen_arith(DisasContext *ctx, uint32_t opc,
             tcg_gen_brcondi_tl(TCG_COND_GE, t1, 0, l1);
             tcg_temp_free(t1);
             /* operands of different sign, first operand and result different sign */
-            generate_exception(ctx, EXCP_OVERFLOW);
+            generate_exception_cond(ctx, EXCP_OVERFLOW);
             gen_set_label(l1);
             gen_store_gpr(t0, rd);
             tcg_temp_free(t0);
@@ -4282,7 +4289,7 @@ static void gen_loongson_multimedia(DisasContext *ctx, int rd, int rs, int rt)
             tcg_gen_andc_i64(t1, t2, t1);
             tcg_temp_free_i64(t2);
             tcg_gen_brcondi_i64(TCG_COND_GE, t1, 0, lab);
-            generate_exception(ctx, EXCP_OVERFLOW);
+            generate_exception_cond(ctx, EXCP_OVERFLOW);
             gen_set_label(lab);
 
             opn = (opc == OPC_ADD_CP2 ? "add" : "dadd");
@@ -4305,7 +4312,7 @@ static void gen_loongson_multimedia(DisasContext *ctx, int rd, int rs, int rt)
             tcg_gen_and_i64(t1, t1, t2);
             tcg_temp_free_i64(t2);
             tcg_gen_brcondi_i64(TCG_COND_GE, t1, 0, lab);
-            generate_exception(ctx, EXCP_OVERFLOW);
+            generate_exception_cond(ctx, EXCP_OVERFLOW);
             gen_set_label(lab);
 
             opn = (opc == OPC_SUB_CP2 ? "sub" : "dsub");
@@ -4432,7 +4439,7 @@ static void gen_trap (DisasContext *ctx, uint32_t opc,
             tcg_gen_brcond_tl(TCG_COND_EQ, t0, t1, l1);
             break;
         }
-        generate_exception(ctx, EXCP_TRAP);
+        generate_exception_cond(ctx, EXCP_TRAP);
         gen_set_label(l1);
     }
     tcg_temp_free(t0);
@@ -13664,7 +13671,6 @@ static void gen_pool32axf (CPUMIPSState *env, DisasContext *ctx, int rt, int rs)
             break;
         case SYSCALL:
             generate_exception(ctx, EXCP_SYSCALL);
-            ctx->bstate = BS_STOP;
             break;
         case SDBBP:
             if (is_uhi(extract32(ctx->opcode, 16, 10))) {
@@ -15248,7 +15254,6 @@ static int decode_micromips_opc (CPUMIPSState *env, DisasContext *ctx)
     if (ctx->pc & 0x1) {
         env->CP0_BadVAddr = ctx->pc;
         generate_exception(ctx, EXCP_AdEL);
-        ctx->bstate = BS_STOP;
         return 2;
     }
 
@@ -15268,8 +15273,6 @@ static int decode_micromips_opc (CPUMIPSState *env, DisasContext *ctx)
         /* LB32, LH32, LWC132, LDC132, LW32 */
             if (ctx->hflags & MIPS_HFLAG_BDS16) {
                 generate_exception(ctx, EXCP_RI);
-                /* Just stop translation; the user is confused.  */
-                ctx->bstate = BS_STOP;
                 return 2;
             }
             break;
@@ -15281,8 +15284,6 @@ static int decode_micromips_opc (CPUMIPSState *env, DisasContext *ctx)
         /* MOVE16, ANDI16, POOL16D, POOL16E, BEQZ16, BNEZ16, B16, LI16 */
             if (ctx->hflags & MIPS_HFLAG_BDS32) {
                 generate_exception(ctx, EXCP_RI);
-                /* Just stop translation; the user is confused.  */
-                ctx->bstate = BS_STOP;
                 return 2;
             }
             break;
@@ -17510,7 +17511,6 @@ static void decode_opc_special(CPUMIPSState *env, DisasContext *ctx)
         break;
     case OPC_SYSCALL:
         generate_exception(ctx, EXCP_SYSCALL);
-        ctx->bstate = BS_STOP;
         break;
     case OPC_BREAK:
         generate_exception(ctx, EXCP_BREAK);
@@ -18885,6 +18885,7 @@ static void gen_msa_3r(CPUMIPSState *env, DisasContext *ctx)
     case OPC_HSUB_U_df:
         if (df == DF_BYTE) {
             generate_exception(ctx, EXCP_RI);
+            break;
         }
         switch (MASK_MSA_3R(ctx->opcode)) {
         case OPC_DOTP_S_df:
@@ -19497,7 +19498,6 @@ static void decode_opc(CPUMIPSState *env, DisasContext *ctx)
     if (ctx->pc & 0x3) {
         env->CP0_BadVAddr = ctx->pc;
         generate_exception_err(ctx, EXCP_AdEL, EXCP_INST_NOTAVAIL);
-        ctx->bstate = BS_STOP;
         return;
     }
 
@@ -20247,7 +20247,6 @@ gen_intermediate_code_internal(MIPSCPU *cpu, TranslationBlock *tb,
             insn_bytes = decode_mips16_opc(env, &ctx);
         } else {
             generate_exception(&ctx, EXCP_RI);
-            ctx.bstate = BS_STOP;
             break;
         }
 
-- 
2.4.3

[Qemu-devel] [PATCH 3/3] target-mips: Use CPU_LOG_INT for logging related to interrupts

[Richard Henderson]

There are now no unconditional uses of qemu_log in the subdirectory.

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 target-mips/helper.c    | 30 +++++++++++-------------------
 target-mips/op_helper.c |  3 ++-
 2 files changed, 13 insertions(+), 20 deletions(-)

diff --git a/target-mips/helper.c b/target-mips/helper.c
index 04ba19f..f44edbb 100644
--- a/target-mips/helper.c
+++ b/target-mips/helper.c
@@ -127,10 +127,6 @@ static int get_physical_address (CPUMIPSState *env, hwaddr *physical,
     /* effective address (modified for KVM T&E kernel segments) */
     target_ulong address = real_address;
 
-#if 0
-    qemu_log("user mode %d h %08x\n", user_mode, env->hflags);
-#endif
-
 #define USEG_LIMIT      0x7FFFFFFFUL
 #define KSEG0_BASE      0x80000000UL
 #define KSEG1_BASE      0xA0000000UL
@@ -227,11 +223,6 @@ static int get_physical_address (CPUMIPSState *env, hwaddr *physical,
             ret = TLBRET_BADADDR;
         }
     }
-#if 0
-    qemu_log(TARGET_FMT_lx " %d %d => %" HWADDR_PRIx " %d (%d)\n",
-            address, rw, access_type, *physical, *prot, ret);
-#endif
-
     return ret;
 }
 #endif
@@ -487,14 +478,16 @@ void mips_cpu_do_interrupt(CPUState *cs)
     int cause = -1;
     const char *name;
 
-    if (qemu_log_enabled() && cs->exception_index != EXCP_EXT_INTERRUPT) {
+    if (qemu_loglevel_mask(CPU_LOG_INT)
+        && cs->exception_index != EXCP_EXT_INTERRUPT) {
         if (cs->exception_index < 0 || cs->exception_index > EXCP_LAST) {
             name = "unknown";
         } else {
             name = excp_names[cs->exception_index];
         }
 
-        qemu_log("%s enter: PC " TARGET_FMT_lx " EPC " TARGET_FMT_lx " %s exception\n",
+        qemu_log("%s enter: PC " TARGET_FMT_lx " EPC " TARGET_FMT_lx
+                 " %s exception\n",
                  __func__, env->active_tc.PC, env->CP0_EPC, name);
     }
     if (cs->exception_index == EXCP_EXT_INTERRUPT &&
@@ -747,16 +740,15 @@ void mips_cpu_do_interrupt(CPUState *cs)
         env->CP0_Cause = (env->CP0_Cause & ~(0x1f << CP0Ca_EC)) | (cause << CP0Ca_EC);
         break;
     default:
-        qemu_log("Invalid MIPS exception %d. Exiting\n", cs->exception_index);
-        printf("Invalid MIPS exception %d. Exiting\n", cs->exception_index);
-        exit(1);
+        abort();
     }
-    if (qemu_log_enabled() && cs->exception_index != EXCP_EXT_INTERRUPT) {
+    if (qemu_loglevel_mask(CPU_LOG_INT)
+        && cs->exception_index != EXCP_EXT_INTERRUPT) {
         qemu_log("%s: PC " TARGET_FMT_lx " EPC " TARGET_FMT_lx " cause %d\n"
-                "    S %08x C %08x A " TARGET_FMT_lx " D " TARGET_FMT_lx "\n",
-                __func__, env->active_tc.PC, env->CP0_EPC, cause,
-                env->CP0_Status, env->CP0_Cause, env->CP0_BadVAddr,
-                env->CP0_DEPC);
+                 "    S %08x C %08x A " TARGET_FMT_lx " D " TARGET_FMT_lx "\n",
+                 __func__, env->active_tc.PC, env->CP0_EPC, cause,
+                 env->CP0_Status, env->CP0_Cause, env->CP0_BadVAddr,
+                 env->CP0_DEPC);
     }
 #endif
     cs->exception_index = EXCP_NONE;
diff --git a/target-mips/op_helper.c b/target-mips/op_helper.c
index db4f6b9..809a061 100644
--- a/target-mips/op_helper.c
+++ b/target-mips/op_helper.c
@@ -38,7 +38,8 @@ static inline void QEMU_NORETURN do_raise_exception_err(CPUMIPSState *env,
     CPUState *cs = CPU(mips_env_get_cpu(env));
 
     if (exception < EXCP_SC) {
-        qemu_log("%s: %d %d\n", __func__, exception, error_code);
+        qemu_log_mask(CPU_LOG_INT, "%s: %d %d\n",
+                      __func__, exception, error_code);
     }
     cs->exception_index = exception;
     env->error_code = error_code;
-- 
2.4.3

Re: [Qemu-devel] [PATCH 1/3] target-mips: Copy restrictions from ext/ins to dext/dins

[Richard Henderson]

On 08/03/2015 11:49 AM, Richard Henderson wrote:
>  #if defined(TARGET_MIPS64)
> -    case OPC_DEXTM:
> -        tcg_gen_shri_tl(t0, t1, lsb);
> -        if (msb != 31) {
> -            tcg_gen_andi_tl(t0, t0, (1ULL << (msb + 1 + 32)) - 1);
> -        }
> -        break;
>      case OPC_DEXTU:
> -        tcg_gen_shri_tl(t0, t1, lsb + 32);
> -        tcg_gen_andi_tl(t0, t0, (1ULL << (msb + 1)) - 1);
> -        break;
> +        lsb += 32;
> +        /* FALLTHRU */
> +    case OPC_DEXTM:
> +        msb += 32;
> +        /* FALLTHRU */
>      case OPC_DEXT:
> +        if (lsb + msb > 63) {
> +            goto fail;
> +        }

Apologies: this bit is wrong for DEXTU.  I'll re-send.


r~

[Qemu-devel] [PATCH v2 1/3] target-mips: Copy restrictions from ext/ins to dext/dins

[Richard Henderson]

The checks in dins is required to avoid triggering an assertion
in tcg_gen_deposit_tl.  The check in dext is just for completeness.
Fold the other D cases in via fallthru.

In this case the errant dins appears to be data, not code, as
translation failed to stop after a break insn.

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 target-mips/translate.c | 45 +++++++++++++++++++++++++--------------------
 1 file changed, 25 insertions(+), 20 deletions(-)

diff --git a/target-mips/translate.c b/target-mips/translate.c
index d1de35a..1cba415 100644
--- a/target-mips/translate.c
+++ b/target-mips/translate.c
@@ -4750,48 +4750,53 @@ static void gen_bitops (DisasContext *ctx, uint32_t opc, int rt,
     gen_load_gpr(t1, rs);
     switch (opc) {
     case OPC_EXT:
-        if (lsb + msb > 31)
+        if (lsb + msb > 31) {
             goto fail;
+        }
         tcg_gen_shri_tl(t0, t1, lsb);
         if (msb != 31) {
-            tcg_gen_andi_tl(t0, t0, (1 << (msb + 1)) - 1);
+            tcg_gen_andi_tl(t0, t0, (1U << (msb + 1)) - 1);
         } else {
             tcg_gen_ext32s_tl(t0, t0);
         }
         break;
 #if defined(TARGET_MIPS64)
-    case OPC_DEXTM:
-        tcg_gen_shri_tl(t0, t1, lsb);
-        if (msb != 31) {
-            tcg_gen_andi_tl(t0, t0, (1ULL << (msb + 1 + 32)) - 1);
-        }
-        break;
     case OPC_DEXTU:
-        tcg_gen_shri_tl(t0, t1, lsb + 32);
-        tcg_gen_andi_tl(t0, t0, (1ULL << (msb + 1)) - 1);
-        break;
+        lsb += 32;
+        goto do_dext;
+    case OPC_DEXTM:
+        msb += 32;
+        goto do_dext;
     case OPC_DEXT:
+    do_dext:
+        if (lsb + msb > 63) {
+            goto fail;
+        }
         tcg_gen_shri_tl(t0, t1, lsb);
-        tcg_gen_andi_tl(t0, t0, (1ULL << (msb + 1)) - 1);
+        if (msb != 63) {
+            tcg_gen_andi_tl(t0, t0, (1ULL << (msb + 1)) - 1);
+        }
         break;
 #endif
     case OPC_INS:
-        if (lsb > msb)
+        if (lsb > msb) {
             goto fail;
+        }
         gen_load_gpr(t0, rt);
         tcg_gen_deposit_tl(t0, t0, t1, lsb, msb - lsb + 1);
         tcg_gen_ext32s_tl(t0, t0);
         break;
 #if defined(TARGET_MIPS64)
-    case OPC_DINSM:
-        gen_load_gpr(t0, rt);
-        tcg_gen_deposit_tl(t0, t0, t1, lsb, msb + 32 - lsb + 1);
-        break;
     case OPC_DINSU:
-        gen_load_gpr(t0, rt);
-        tcg_gen_deposit_tl(t0, t0, t1, lsb + 32, msb - lsb + 1);
-        break;
+        lsb += 32;
+        /* FALLTHRU */
+    case OPC_DINSM:
+        msb += 32;
+        /* FALLTHRU */
     case OPC_DINS:
+        if (lsb > msb) {
+            goto fail;
+        }
         gen_load_gpr(t0, rt);
         tcg_gen_deposit_tl(t0, t0, t1, lsb, msb - lsb + 1);
         break;
-- 
2.4.3

Re: [Qemu-devel] [PATCH v2 1/3] target-mips: Copy restrictions from ext/ins to dext/dins

[Aurelien Jarno]

On 2015-08-03 12:35, Richard Henderson wrote:
> The checks in dins is required to avoid triggering an assertion
> in tcg_gen_deposit_tl.  The check in dext is just for completeness.
> Fold the other D cases in via fallthru.
> 
> In this case the errant dins appears to be data, not code, as
> translation failed to stop after a break insn.
> 
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
>  target-mips/translate.c | 45 +++++++++++++++++++++++++--------------------
>  1 file changed, 25 insertions(+), 20 deletions(-)
> 
> diff --git a/target-mips/translate.c b/target-mips/translate.c
> index d1de35a..1cba415 100644
> --- a/target-mips/translate.c
> +++ b/target-mips/translate.c
> @@ -4750,48 +4750,53 @@ static void gen_bitops (DisasContext *ctx, uint32_t opc, int rt,
>      gen_load_gpr(t1, rs);
>      switch (opc) {
>      case OPC_EXT:
> -        if (lsb + msb > 31)
> +        if (lsb + msb > 31) {
>              goto fail;
> +        }
>          tcg_gen_shri_tl(t0, t1, lsb);
>          if (msb != 31) {
> -            tcg_gen_andi_tl(t0, t0, (1 << (msb + 1)) - 1);
> +            tcg_gen_andi_tl(t0, t0, (1U << (msb + 1)) - 1);

Is this change really needed?

>          } else {
>              tcg_gen_ext32s_tl(t0, t0);
>          }
>          break;
>  #if defined(TARGET_MIPS64)
> -    case OPC_DEXTM:
> -        tcg_gen_shri_tl(t0, t1, lsb);
> -        if (msb != 31) {
> -            tcg_gen_andi_tl(t0, t0, (1ULL << (msb + 1 + 32)) - 1);
> -        }
> -        break;
>      case OPC_DEXTU:
> -        tcg_gen_shri_tl(t0, t1, lsb + 32);
> -        tcg_gen_andi_tl(t0, t0, (1ULL << (msb + 1)) - 1);
> -        break;
> +        lsb += 32;
> +        goto do_dext;
> +    case OPC_DEXTM:
> +        msb += 32;
> +        goto do_dext;
>      case OPC_DEXT:
> +    do_dext:
> +        if (lsb + msb > 63) {
> +            goto fail;
> +        }

Note that DEXT can't fail as both lsb and msb are in the range 0..31.
DEXTU and DEXTM can.

>          tcg_gen_shri_tl(t0, t1, lsb);
> -        tcg_gen_andi_tl(t0, t0, (1ULL << (msb + 1)) - 1);
> +        if (msb != 63) {
> +            tcg_gen_andi_tl(t0, t0, (1ULL << (msb + 1)) - 1);
> +        }
>          break;
>  #endif
>      case OPC_INS:
> -        if (lsb > msb)
> +        if (lsb > msb) {
>              goto fail;
> +        }
>          gen_load_gpr(t0, rt);
>          tcg_gen_deposit_tl(t0, t0, t1, lsb, msb - lsb + 1);
>          tcg_gen_ext32s_tl(t0, t0);
>          break;
>  #if defined(TARGET_MIPS64)
> -    case OPC_DINSM:
> -        gen_load_gpr(t0, rt);
> -        tcg_gen_deposit_tl(t0, t0, t1, lsb, msb + 32 - lsb + 1);
> -        break;
>      case OPC_DINSU:
> -        gen_load_gpr(t0, rt);
> -        tcg_gen_deposit_tl(t0, t0, t1, lsb + 32, msb - lsb + 1);
> -        break;
> +        lsb += 32;
> +        /* FALLTHRU */
> +    case OPC_DINSM:
> +        msb += 32;

The same way DINSM can't fail.

> +        /* FALLTHRU */
>      case OPC_DINS:
> +        if (lsb > msb) {
> +            goto fail;
> +        }
>          gen_load_gpr(t0, rt);
>          tcg_gen_deposit_tl(t0, t0, t1, lsb, msb - lsb + 1);
>          break;

Despite the above comments:

Reviewed-by: Aurelien Jarno <aurelien@aurel32.net>

Should we try to get this one into 2.4, if not already too late?

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH 2/3] target-mips: Stop translation after unconditional exceptions

[Aurelien Jarno]

On 2015-08-03 11:49, Richard Henderson wrote:
> Since there are many more unconditional exceptions than conditional,
> introduce a new generate_exception_cond to mark conditionals.  Also
> delete those few cases where we did attempt to stop translation after
> an exception, as these are now subsumed in the change.
> 
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
>  target-mips/translate.c | 47 +++++++++++++++++++++++------------------------
>  1 file changed, 23 insertions(+), 24 deletions(-)

Isn't that a bit redundant with the work from Pavel Dovgalyuk:

https://lists.gnu.org/archive/html/qemu-devel/2015-07/msg02581.html

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH 3/3] target-mips: Use CPU_LOG_INT for logging related to interrupts

[Aurelien Jarno]

On 2015-08-03 11:49, Richard Henderson wrote:
> There are now no unconditional uses of qemu_log in the subdirectory.
> 
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
>  target-mips/helper.c    | 30 +++++++++++-------------------
>  target-mips/op_helper.c |  3 ++-
>  2 files changed, 13 insertions(+), 20 deletions(-)

Reviewed-by: Aurelien Jarno <aurelien@aurel32.net>

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH v2 1/3] target-mips: Copy restrictions from ext/ins to dext/dins

[Richard Henderson]

On 08/03/2015 02:31 PM, Aurelien Jarno wrote:
> On 2015-08-03 12:35, Richard Henderson wrote:
>>           if (msb != 31) {
>> -            tcg_gen_andi_tl(t0, t0, (1 << (msb + 1)) - 1);
>> +            tcg_gen_andi_tl(t0, t0, (1U << (msb + 1)) - 1);
>
> Is this change really needed?

msb == 30 means 1 << 31.  Which officially must be unsigned to be correct.  If 
we were to run under ubsan, this would trigger an error.

> Note that DEXT can't fail as both lsb and msb are in the range 0..31.
> DEXTU and DEXTM can.
...
> The same way DINSM can't fail.

Yes, I know.  But it seems cleaner to do the checks always, unifying all of the 
code.

> Should we try to get this one into 2.4, if not already too late?

Perhaps.  Otherwise via stable after the fact.


r~

Re: [Qemu-devel] [PATCH 2/3] target-mips: Stop translation after unconditional exceptions

[Richard Henderson]

On 08/03/2015 02:31 PM, Aurelien Jarno wrote:
> On 2015-08-03 11:49, Richard Henderson wrote:
>> Since there are many more unconditional exceptions than conditional,
>> introduce a new generate_exception_cond to mark conditionals.  Also
>> delete those few cases where we did attempt to stop translation after
>> an exception, as these are now subsumed in the change.
>>
>> Signed-off-by: Richard Henderson <rth@twiddle.net>
>> ---
>>   target-mips/translate.c | 47 +++++++++++++++++++++++------------------------
>>   1 file changed, 23 insertions(+), 24 deletions(-)
>
> Isn't that a bit redundant with the work from Pavel Dovgalyuk:
>
> https://lists.gnu.org/archive/html/qemu-devel/2015-07/msg02581.html
>

Yes, it is.  I'd forgotten about it.  It is interesting that he makes the 
opposite choice -- changing all of the places that unconditional exceptions are 
raised, rather than the conditional exceptions.

That said, I guess it really doesn't matter either way.


r~

Re: [Qemu-devel] [PATCH v2 1/3] target-mips: Copy restrictions from ext/ins to dext/dins

[Aurelien Jarno]

On 2015-08-03 14:41, Richard Henderson wrote:
> On 08/03/2015 02:31 PM, Aurelien Jarno wrote:
> >On 2015-08-03 12:35, Richard Henderson wrote:
> >>          if (msb != 31) {
> >>-            tcg_gen_andi_tl(t0, t0, (1 << (msb + 1)) - 1);
> >>+            tcg_gen_andi_tl(t0, t0, (1U << (msb + 1)) - 1);
> >
> >Is this change really needed?
> 
> msb == 30 means 1 << 31.  Which officially must be unsigned to be correct.
> If we were to run under ubsan, this would trigger an error.

Ok.

> >Note that DEXT can't fail as both lsb and msb are in the range 0..31.
> >DEXTU and DEXTM can.
> ...
> >The same way DINSM can't fail.
> 
> Yes, I know.  But it seems cleaner to do the checks always, unifying all of
> the code.

Agreed.

> >Should we try to get this one into 2.4, if not already too late?
> 
> Perhaps.  Otherwise via stable after the fact.

Ok. Leon, do you have other pending patches for 2.4/2.4.1? The
semihosting microMIPS R6 one maybe?

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH v2 1/3] target-mips: Copy restrictions from ext/ins to dext/dins

[Leon Alrae]

On 03/08/2015 23:09, Aurelien Jarno wrote:
>>> Should we try to get this one into 2.4, if not already too late?
>>
>> Perhaps.  Otherwise via stable after the fact.
> 
> Ok. Leon, do you have other pending patches for 2.4/2.4.1? The
> semihosting microMIPS R6 one maybe?

Yes, it would be good also to include the semihosting microMIPS R6 fix in 2.4.

Leon

Re: [Qemu-devel] [PATCH v2 1/3] target-mips: Copy restrictions from ext/ins to dext/dins

[Leon Alrae]

On 03/08/2015 20:35, Richard Henderson wrote:
> The checks in dins is required to avoid triggering an assertion
> in tcg_gen_deposit_tl.  The check in dext is just for completeness.
> Fold the other D cases in via fallthru.
> 
> In this case the errant dins appears to be data, not code, as
> translation failed to stop after a break insn.
> 
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
>  target-mips/translate.c | 45 +++++++++++++++++++++++++--------------------
>  1 file changed, 25 insertions(+), 20 deletions(-)

Reviewed-by: Leon Alrae <leon.alrae@imgtec.com>

I'll send out the pull request including this single patch from the series as
well as semihosting microMIPS R6 fix soon (just waiting for tests to finish...).

Thanks,
Leon