diff mbox series

[v3,15/23] target/mips: Extract break code into env->error_code

Message ID 20211103140847.454070-16-richard.henderson@linaro.org
State Superseded
Headers show
Series linux-user: Clean up siginfo_t handling | expand

Commit Message

Richard Henderson Nov. 3, 2021, 2:08 p.m. UTC 
Simplify cpu_loop by doing all of the decode in translate.

This fixes a bug in that cpu_loop was not handling the
different layout of the R6 version of break16.  This fixes
a bug in that cpu_loop extracted the wrong bits for the
mips16e break16 instruction.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/mips/tcg/translate.h               |  1 +
 linux-user/mips/cpu_loop.c                | 73 +++--------------------
 target/mips/tcg/translate.c               | 12 +++-
 target/mips/tcg/micromips_translate.c.inc |  6 +-
 target/mips/tcg/mips16e_translate.c.inc   |  2 +-
 5 files changed, 25 insertions(+), 69 deletions(-)

-- 
2.25.1

Comments

Philippe Mathieu-Daudé Nov. 3, 2021, 3:11 p.m. UTC | #1 
On 11/3/21 15:08, Richard Henderson wrote:
> Simplify cpu_loop by doing all of the decode in translate.

> 

> This fixes a bug in that cpu_loop was not handling the

> different layout of the R6 version of break16.  This fixes

> a bug in that cpu_loop extracted the wrong bits for the

> mips16e break16 instruction.

> 

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

> ---

>  target/mips/tcg/translate.h               |  1 +

>  linux-user/mips/cpu_loop.c                | 73 +++--------------------

>  target/mips/tcg/translate.c               | 12 +++-

>  target/mips/tcg/micromips_translate.c.inc |  6 +-

>  target/mips/tcg/mips16e_translate.c.inc   |  2 +-

>  5 files changed, 25 insertions(+), 69 deletions(-)


> diff --git a/linux-user/mips/cpu_loop.c b/linux-user/mips/cpu_loop.c

> index 034b31f853..8efb6d2a24 100644

> --- a/linux-user/mips/cpu_loop.c

> +++ b/linux-user/mips/cpu_loop.c

> @@ -65,6 +65,7 @@ void cpu_loop(CPUMIPSState *env)

>  {

>      CPUState *cs = env_cpu(env);

>      int trapnr, si_code;

> +    unsigned int code;

>      abi_long ret;

>  # ifdef TARGET_ABI_MIPSO32

>      unsigned int syscall_num;

> @@ -185,71 +186,15 @@ done_syscall:

>           * handling code in arch/mips/kernel/traps.c.

>           */

>          case EXCP_BREAK:

> -            {

> -                abi_ulong trap_instr;

> -                unsigned int code;

> -

> -                /*

> -                 * FIXME: It would be better to decode the trap number

> -                 * during translate, and store it in error_code while

> -                 * raising the exception.  We should not be re-reading

> -                 * the opcode here.

> -                 */

> -

> -                if (env->hflags & MIPS_HFLAG_M16) {

> -                    if (env->insn_flags & ASE_MICROMIPS) {

> -                        /* microMIPS mode */

> -                        ret = get_user_u16(trap_instr, env->active_tc.PC);

> -                        if (ret != 0) {

> -                            goto error;

> -                        }

> -

> -                        if ((trap_instr >> 10) == 0x11) {

> -                            /* 16-bit instruction */

> -                            code = trap_instr & 0xf;

> -                        } else {

> -                            /* 32-bit instruction */

> -                            abi_ulong instr_lo;

> -

> -                            ret = get_user_u16(instr_lo,

> -                                               env->active_tc.PC + 2);

> -                            if (ret != 0) {

> -                                goto error;

> -                            }

> -                            trap_instr = (trap_instr << 16) | instr_lo;

> -                            code = ((trap_instr >> 6) & ((1 << 20) - 1));

> -                            /* Unfortunately, microMIPS also suffers from

> -                               the old assembler bug...  */

> -                            if (code >= (1 << 10)) {

> -                                code >>= 10;

> -                            }

> -                        }

> -                    } else {

> -                        /* MIPS16e mode */

> -                        ret = get_user_u16(trap_instr, env->active_tc.PC);

> -                        if (ret != 0) {

> -                            goto error;

> -                        }

> -                        code = (trap_instr >> 6) & 0x3f;

> -                    }

> -                } else {

> -                    ret = get_user_u32(trap_instr, env->active_tc.PC);

> -                    if (ret != 0) {

> -                        goto error;

> -                    }

> -

> -                    /* As described in the original Linux kernel code, the

> -                     * below checks on 'code' are to work around an old

> -                     * assembly bug.

> -                     */

> -                    code = ((trap_instr >> 6) & ((1 << 20) - 1));

> -                    if (code >= (1 << 10)) {

> -                        code >>= 10;

> -                    }

> -                }

> -

> -                do_tr_or_bp(env, code, false);

> +            /*

> +             * As described in the original Linux kernel code, the below

> +             * checks on 'code' are to work around an old assembly bug.

> +             */

> +            code = env->error_code;

> +            if (code >= (1 << 10)) {

> +                code >>= 10;


Shouldn't we also move this to the translation (not in R6_BREAK16)?

>              }

> +            do_tr_or_bp(env, code, false);

>              break;

>          case EXCP_TRAP:

>              {

> diff --git a/target/mips/tcg/translate.c b/target/mips/tcg/translate.c

> index 47db35d7dd..a42f507aed 100644

> --- a/target/mips/tcg/translate.c

> +++ b/target/mips/tcg/translate.c

> @@ -1367,6 +1367,16 @@ void generate_exception_end(DisasContext *ctx, int excp)

>      generate_exception_err(ctx, excp, 0);

>  }

>  

> +void generate_exception_break(DisasContext *ctx, int code)

> +{

> +#ifdef CONFIG_USER_ONLY

> +    /* Pass the break code along to cpu_loop. */

> +    tcg_gen_st_i32(tcg_constant_i32(code), cpu_env,

> +                   offsetof(CPUMIPSState, error_code));

> +#endif

> +    generate_exception_end(ctx, EXCP_BREAK);

> +}

> +

>  void gen_reserved_instruction(DisasContext *ctx)

>  {

>      generate_exception_end(ctx, EXCP_RI);

> @@ -14160,7 +14170,7 @@ static void decode_opc_special(CPUMIPSState *env, DisasContext *ctx)

>          generate_exception_end(ctx, EXCP_SYSCALL);

>          break;

>      case OPC_BREAK:

> -        generate_exception_end(ctx, EXCP_BREAK);

> +        generate_exception_break(ctx, extract32(ctx->opcode, 6, 20));

>          break;

>      case OPC_SYNC:

>          check_insn(ctx, ISA_MIPS2);

> diff --git a/target/mips/tcg/micromips_translate.c.inc b/target/mips/tcg/micromips_translate.c.inc

> index 0da4c802a3..f91f7a96cd 100644

> --- a/target/mips/tcg/micromips_translate.c.inc

> +++ b/target/mips/tcg/micromips_translate.c.inc

> @@ -822,7 +822,7 @@ static void gen_pool16c_insn(DisasContext *ctx)

>          gen_HILO(ctx, OPC_MFLO, 0, uMIPS_RS5(ctx->opcode));

>          break;

>      case BREAK16:

> -        generate_exception_end(ctx, EXCP_BREAK);

> +        generate_exception_break(ctx, extract32(ctx->opcode, 0, 4));

>          break;

>      case SDBBP16:

>          if (is_uhi(extract32(ctx->opcode, 0, 4))) {

> @@ -937,7 +937,7 @@ static void gen_pool16c_r6_insn(DisasContext *ctx)

>              break;

>          case R6_BREAK16:

>              /* BREAK16 */

> -            generate_exception(ctx, EXCP_BREAK);

> +            generate_exception_break(ctx, extract32(ctx->opcode, 6, 4));

>              break;

>          case R6_SDBBP16:

>              /* SDBBP16 */

> @@ -1812,7 +1812,7 @@ static void decode_micromips32_opc(CPUMIPSState *env, DisasContext *ctx)

>              gen_pool32axf(env, ctx, rt, rs);

>              break;

>          case BREAK32:

> -            generate_exception_end(ctx, EXCP_BREAK);

> +            generate_exception_break(ctx, extract32(ctx->opcode, 6, 20));

>              break;

>          case SIGRIE:

>              check_insn(ctx, ISA_MIPS_R6);

> diff --git a/target/mips/tcg/mips16e_translate.c.inc b/target/mips/tcg/mips16e_translate.c.inc

> index 84d816603a..f57e0a5f2a 100644

> --- a/target/mips/tcg/mips16e_translate.c.inc

> +++ b/target/mips/tcg/mips16e_translate.c.inc

> @@ -969,7 +969,7 @@ static int decode_ase_mips16e(CPUMIPSState *env, DisasContext *ctx)

>              gen_slt(ctx, OPC_SLTU, 24, rx, ry);

>              break;

>          case RR_BREAK:

> -            generate_exception_end(ctx, EXCP_BREAK);

> +            generate_exception_break(ctx, extract32(ctx->opcode, 5, 6));

>              break;

>          case RR_SLLV:

>              gen_shift(ctx, OPC_SLLV, ry, rx, ry);

>
Richard Henderson Nov. 3, 2021, 3:38 p.m. UTC | #2 
On 11/3/21 11:11 AM, Philippe Mathieu-Daudé wrote:
>> +            /*

>> +             * As described in the original Linux kernel code, the below

>> +             * checks on 'code' are to work around an old assembly bug.

>> +             */

>> +            code = env->error_code;

>> +            if (code >= (1 << 10)) {

>> +                code >>= 10;

> 

> Shouldn't we also move this to the translation (not in R6_BREAK16)?


No, because it's a kernel thing, not a cpu thing:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/mips/kernel/traps.c#n1056


r~


PS: Just above, it looks like the kernel would need to grow support for R6_BREAK.  At 
present it only handles mips16e and micromips.
Philippe Mathieu-Daudé Nov. 3, 2021, 5:01 p.m. UTC | #3 
On 11/3/21 16:38, Richard Henderson wrote:
> On 11/3/21 11:11 AM, Philippe Mathieu-Daudé wrote:

>>> +            /*

>>> +             * As described in the original Linux kernel code, the

>>> below

>>> +             * checks on 'code' are to work around an old assembly bug.

>>> +             */

>>> +            code = env->error_code;

>>> +            if (code >= (1 << 10)) {

>>> +                code >>= 10;

>>

>> Shouldn't we also move this to the translation (not in R6_BREAK16)?

> 

> No, because it's a kernel thing, not a cpu thing:

> 

> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/mips/kernel/traps.c#n1056


Oh indeed, I see.

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


> PS: Just above, it looks like the kernel would need to grow support for

> R6_BREAK.  At present it only handles mips16e and micromips.

>
diff mbox series

Patch

diff --git a/target/mips/tcg/translate.h b/target/mips/tcg/translate.h
index 6111493651..ae01515efe 100644
--- a/target/mips/tcg/translate.h
+++ b/target/mips/tcg/translate.h
@@ -129,6 +129,7 @@  enum {
 void generate_exception(DisasContext *ctx, int excp);
 void generate_exception_err(DisasContext *ctx, int excp, int err);
 void generate_exception_end(DisasContext *ctx, int excp);
+void generate_exception_break(DisasContext *ctx, int code);
 void gen_reserved_instruction(DisasContext *ctx);
 
 void check_insn(DisasContext *ctx, uint64_t flags);
diff --git a/linux-user/mips/cpu_loop.c b/linux-user/mips/cpu_loop.c
index 034b31f853..8efb6d2a24 100644
--- a/linux-user/mips/cpu_loop.c
+++ b/linux-user/mips/cpu_loop.c
@@ -65,6 +65,7 @@  void cpu_loop(CPUMIPSState *env)
 {
     CPUState *cs = env_cpu(env);
     int trapnr, si_code;
+    unsigned int code;
     abi_long ret;
 # ifdef TARGET_ABI_MIPSO32
     unsigned int syscall_num;
@@ -185,71 +186,15 @@  done_syscall:
          * handling code in arch/mips/kernel/traps.c.
          */
         case EXCP_BREAK:
-            {
-                abi_ulong trap_instr;
-                unsigned int code;
-
-                /*
-                 * FIXME: It would be better to decode the trap number
-                 * during translate, and store it in error_code while
-                 * raising the exception.  We should not be re-reading
-                 * the opcode here.
-                 */
-
-                if (env->hflags & MIPS_HFLAG_M16) {
-                    if (env->insn_flags & ASE_MICROMIPS) {
-                        /* microMIPS mode */
-                        ret = get_user_u16(trap_instr, env->active_tc.PC);
-                        if (ret != 0) {
-                            goto error;
-                        }
-
-                        if ((trap_instr >> 10) == 0x11) {
-                            /* 16-bit instruction */
-                            code = trap_instr & 0xf;
-                        } else {
-                            /* 32-bit instruction */
-                            abi_ulong instr_lo;
-
-                            ret = get_user_u16(instr_lo,
-                                               env->active_tc.PC + 2);
-                            if (ret != 0) {
-                                goto error;
-                            }
-                            trap_instr = (trap_instr << 16) | instr_lo;
-                            code = ((trap_instr >> 6) & ((1 << 20) - 1));
-                            /* Unfortunately, microMIPS also suffers from
-                               the old assembler bug...  */
-                            if (code >= (1 << 10)) {
-                                code >>= 10;
-                            }
-                        }
-                    } else {
-                        /* MIPS16e mode */
-                        ret = get_user_u16(trap_instr, env->active_tc.PC);
-                        if (ret != 0) {
-                            goto error;
-                        }
-                        code = (trap_instr >> 6) & 0x3f;
-                    }
-                } else {
-                    ret = get_user_u32(trap_instr, env->active_tc.PC);
-                    if (ret != 0) {
-                        goto error;
-                    }
-
-                    /* As described in the original Linux kernel code, the
-                     * below checks on 'code' are to work around an old
-                     * assembly bug.
-                     */
-                    code = ((trap_instr >> 6) & ((1 << 20) - 1));
-                    if (code >= (1 << 10)) {
-                        code >>= 10;
-                    }
-                }
-
-                do_tr_or_bp(env, code, false);
+            /*
+             * As described in the original Linux kernel code, the below
+             * checks on 'code' are to work around an old assembly bug.
+             */
+            code = env->error_code;
+            if (code >= (1 << 10)) {
+                code >>= 10;
             }
+            do_tr_or_bp(env, code, false);
             break;
         case EXCP_TRAP:
             {
diff --git a/target/mips/tcg/translate.c b/target/mips/tcg/translate.c
index 47db35d7dd..a42f507aed 100644
--- a/target/mips/tcg/translate.c
+++ b/target/mips/tcg/translate.c
@@ -1367,6 +1367,16 @@  void generate_exception_end(DisasContext *ctx, int excp)
     generate_exception_err(ctx, excp, 0);
 }
 
+void generate_exception_break(DisasContext *ctx, int code)
+{
+#ifdef CONFIG_USER_ONLY
+    /* Pass the break code along to cpu_loop. */
+    tcg_gen_st_i32(tcg_constant_i32(code), cpu_env,
+                   offsetof(CPUMIPSState, error_code));
+#endif
+    generate_exception_end(ctx, EXCP_BREAK);
+}
+
 void gen_reserved_instruction(DisasContext *ctx)
 {
     generate_exception_end(ctx, EXCP_RI);
@@ -14160,7 +14170,7 @@  static void decode_opc_special(CPUMIPSState *env, DisasContext *ctx)
         generate_exception_end(ctx, EXCP_SYSCALL);
         break;
     case OPC_BREAK:
-        generate_exception_end(ctx, EXCP_BREAK);
+        generate_exception_break(ctx, extract32(ctx->opcode, 6, 20));
         break;
     case OPC_SYNC:
         check_insn(ctx, ISA_MIPS2);
diff --git a/target/mips/tcg/micromips_translate.c.inc b/target/mips/tcg/micromips_translate.c.inc
index 0da4c802a3..f91f7a96cd 100644
--- a/target/mips/tcg/micromips_translate.c.inc
+++ b/target/mips/tcg/micromips_translate.c.inc
@@ -822,7 +822,7 @@  static void gen_pool16c_insn(DisasContext *ctx)
         gen_HILO(ctx, OPC_MFLO, 0, uMIPS_RS5(ctx->opcode));
         break;
     case BREAK16:
-        generate_exception_end(ctx, EXCP_BREAK);
+        generate_exception_break(ctx, extract32(ctx->opcode, 0, 4));
         break;
     case SDBBP16:
         if (is_uhi(extract32(ctx->opcode, 0, 4))) {
@@ -937,7 +937,7 @@  static void gen_pool16c_r6_insn(DisasContext *ctx)
             break;
         case R6_BREAK16:
             /* BREAK16 */
-            generate_exception(ctx, EXCP_BREAK);
+            generate_exception_break(ctx, extract32(ctx->opcode, 6, 4));
             break;
         case R6_SDBBP16:
             /* SDBBP16 */
@@ -1812,7 +1812,7 @@  static void decode_micromips32_opc(CPUMIPSState *env, DisasContext *ctx)
             gen_pool32axf(env, ctx, rt, rs);
             break;
         case BREAK32:
-            generate_exception_end(ctx, EXCP_BREAK);
+            generate_exception_break(ctx, extract32(ctx->opcode, 6, 20));
             break;
         case SIGRIE:
             check_insn(ctx, ISA_MIPS_R6);
diff --git a/target/mips/tcg/mips16e_translate.c.inc b/target/mips/tcg/mips16e_translate.c.inc
index 84d816603a..f57e0a5f2a 100644
--- a/target/mips/tcg/mips16e_translate.c.inc
+++ b/target/mips/tcg/mips16e_translate.c.inc
@@ -969,7 +969,7 @@  static int decode_ase_mips16e(CPUMIPSState *env, DisasContext *ctx)
             gen_slt(ctx, OPC_SLTU, 24, rx, ry);
             break;
         case RR_BREAK:
-            generate_exception_end(ctx, EXCP_BREAK);
+            generate_exception_break(ctx, extract32(ctx->opcode, 5, 6));
             break;
         case RR_SLLV:
             gen_shift(ctx, OPC_SLLV, ry, rx, ry);