We need not check for ARM vs Thumb state in order to dispatch
disassembly of every instruction.

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 target/arm/translate.c | 134 +++++++++++++++++++++++++++++++------------------
 1 file changed, 86 insertions(+), 48 deletions(-)

diff --git a/target/arm/translate.c b/target/arm/translate.c
index ebe1c1a..d7c3c10 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -11944,20 +11944,17 @@ static bool arm_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cpu,
     return true;
 }
 
-static void arm_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
+static bool arm_pre_translate_insn(DisasContext *dc)
 {
-    DisasContext *dc = container_of(dcbase, DisasContext, base);
-    CPUARMState *env = cpu->env_ptr;
-
 #ifdef CONFIG_USER_ONLY
-        /* Intercept jump to the magic kernel page.  */
-        if (dc->pc >= 0xffff0000) {
-            /* We always get here via a jump, so know we are not in a
-               conditional execution block.  */
-            gen_exception_internal(EXCP_KERNEL_TRAP);
-            dc->base.is_jmp = DISAS_NORETURN;
-            return;
-        }
+    /* Intercept jump to the magic kernel page.  */
+    if (dc->pc >= 0xffff0000) {
+        /* We always get here via a jump, so know we are not in a
+           conditional execution block.  */
+        gen_exception_internal(EXCP_KERNEL_TRAP);
+        dc->base.is_jmp = DISAS_NORETURN;
+        return true;
+    }
 #endif
 
     if (dc->ss_active && !dc->pstate_ss) {
@@ -11975,54 +11972,82 @@ static void arm_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
         gen_exception(EXCP_UDEF, syn_swstep(dc->ss_same_el, 0, 0),
                       default_exception_el(dc));
         dc->base.is_jmp = DISAS_NORETURN;
-        return;
+        return true;
     }
 
-    if (dc->thumb) {
-        disas_thumb_insn(env, dc);
-        if (dc->condexec_mask) {
-            dc->condexec_cond = (dc->condexec_cond & 0xe)
-                | ((dc->condexec_mask >> 4) & 1);
-            dc->condexec_mask = (dc->condexec_mask << 1) & 0x1f;
-            if (dc->condexec_mask == 0) {
-                dc->condexec_cond = 0;
-            }
-        }
-    } else {
-        unsigned int insn = arm_ldl_code(env, dc->pc, dc->sctlr_b);
-        dc->pc += 4;
-        disas_arm_insn(dc, insn);
+    return false;
+}
+
+static void arm_post_translate_insn(CPUARMState *env, DisasContext *dc)
+{
+    /* Translation stops when a conditional branch is encountered.
+     * Otherwise the subsequent code could get translated several times.
+     * Also stop translation when a page boundary is reached.  This
+     * ensures prefetch aborts occur at the right place.
+     *
+     * We want to stop the TB if the next insn starts in a new page,
+     * or if it spans between this page and the next. This means that
+     * if we're looking at the last halfword in the page we need to
+     * see if it's a 16-bit Thumb insn (which will fit in this TB)
+     * or a 32-bit Thumb insn (which won't).
+     * This is to avoid generating a silly TB with a single 16-bit insn
+     * in it at the end of this page (which would execute correctly
+     * but isn't very efficient).
+     */
+    if (dc->base.is_jmp == DISAS_NEXT
+        && (dc->pc >= dc->next_page_start
+            || (dc->pc >= dc->next_page_start - 3
+                && insn_crosses_page(env, dc)))) {
+        dc->base.is_jmp = DISAS_TOO_MANY;
     }
 
     if (dc->condjmp && !dc->base.is_jmp) {
         gen_set_label(dc->condlabel);
         dc->condjmp = 0;
     }
+    dc->base.pc_next = dc->pc;
+    translator_loop_temp_check(&dc->base);
+}
 
-    if (dc->base.is_jmp == DISAS_NEXT) {
-        /* Translation stops when a conditional branch is encountered.
-         * Otherwise the subsequent code could get translated several times.
-         * Also stop translation when a page boundary is reached.  This
-         * ensures prefetch aborts occur at the right place.  */
-
-        if (dc->pc >= dc->next_page_start ||
-            (dc->pc >= dc->next_page_start - 3 &&
-             insn_crosses_page(env, dc))) {
-            /* We want to stop the TB if the next insn starts in a new page,
-             * or if it spans between this page and the next. This means that
-             * if we're looking at the last halfword in the page we need to
-             * see if it's a 16-bit Thumb insn (which will fit in this TB)
-             * or a 32-bit Thumb insn (which won't).
-             * This is to avoid generating a silly TB with a single 16-bit insn
-             * in it at the end of this page (which would execute correctly
-             * but isn't very efficient).
-             */
-            dc->base.is_jmp = DISAS_TOO_MANY;
+static void arm_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
+{
+    DisasContext *dc = container_of(dcbase, DisasContext, base);
+    CPUARMState *env = cpu->env_ptr;
+    unsigned int insn;
+
+    if (arm_pre_translate_insn(dc)) {
+        return;
+    }
+
+    insn = arm_ldl_code(env, dc->pc, dc->sctlr_b);
+    dc->pc += 4;
+    disas_arm_insn(dc, insn);
+
+    arm_post_translate_insn(env, dc);
+}
+
+static void thumb_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
+{
+    DisasContext *dc = container_of(dcbase, DisasContext, base);
+    CPUARMState *env = cpu->env_ptr;
+
+    if (arm_pre_translate_insn(dc)) {
+        return;
+    }
+
+    disas_thumb_insn(env, dc);
+
+    /* Advance the Thumb condexec condition.  */
+    if (dc->condexec_mask) {
+        dc->condexec_cond = ((dc->condexec_cond & 0xe) |
+                             ((dc->condexec_mask >> 4) & 1));
+        dc->condexec_mask = (dc->condexec_mask << 1) & 0x1f;
+        if (dc->condexec_mask == 0) {
+            dc->condexec_cond = 0;
         }
     }
 
-    dc->base.pc_next = dc->pc;
-    translator_loop_temp_check(&dc->base);
+    arm_post_translate_insn(env, dc);
 }
 
 static void arm_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
@@ -12161,12 +12186,25 @@ static const TranslatorOps arm_translator_ops = {
     .disas_log          = arm_tr_disas_log,
 };
 
+static const TranslatorOps thumb_translator_ops = {
+    .init_disas_context = arm_tr_init_disas_context,
+    .tb_start           = arm_tr_tb_start,
+    .insn_start         = arm_tr_insn_start,
+    .breakpoint_check   = arm_tr_breakpoint_check,
+    .translate_insn     = thumb_tr_translate_insn,
+    .tb_stop            = arm_tr_tb_stop,
+    .disas_log          = arm_tr_disas_log,
+};
+
 /* generate intermediate code for basic block 'tb'.  */
 void gen_intermediate_code(CPUState *cpu, TranslationBlock *tb)
 {
     DisasContext dc;
     const TranslatorOps *ops = &arm_translator_ops;
 
+    if (ARM_TBFLAG_THUMB(tb->flags)) {
+        ops = &thumb_translator_ops;
+    }
 #ifdef TARGET_AARCH64
     if (ARM_TBFLAG_AARCH64_STATE(tb->flags)) {
         ops = &aarch64_translator_ops;
-- 
2.9.4

On Fri, Jul 14, 2017 at 23:42:42 -1000, Richard Henderson wrote:
> We need not check for ARM vs Thumb state in order to dispatch
> disassembly of every instruction.
> 
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
>  target/arm/translate.c | 134 +++++++++++++++++++++++++++++++------------------
>  1 file changed, 86 insertions(+), 48 deletions(-)
> 
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index ebe1c1a..d7c3c10 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -11944,20 +11944,17 @@ static bool arm_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cpu,
>      return true;
>  }
>  
> -static void arm_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
> +static bool arm_pre_translate_insn(DisasContext *dc)
>  {
> -    DisasContext *dc = container_of(dcbase, DisasContext, base);
> -    CPUARMState *env = cpu->env_ptr;
> -
>  #ifdef CONFIG_USER_ONLY
> -        /* Intercept jump to the magic kernel page.  */
> -        if (dc->pc >= 0xffff0000) {
> -            /* We always get here via a jump, so know we are not in a
> -               conditional execution block.  */
> -            gen_exception_internal(EXCP_KERNEL_TRAP);
> -            dc->base.is_jmp = DISAS_NORETURN;
> -            return;
> -        }
> +    /* Intercept jump to the magic kernel page.  */
> +    if (dc->pc >= 0xffff0000) {
> +        /* We always get here via a jump, so know we are not in a
> +           conditional execution block.  */
> +        gen_exception_internal(EXCP_KERNEL_TRAP);
> +        dc->base.is_jmp = DISAS_NORETURN;
> +        return true;
> +    }

See my comment in patch 24 about this.

Other than that, this is neat.
Reviewed-by: Emilio G. Cota <cota@braap.org>

		E.

On Fri, Jul 14, 2017 at 23:42:42 -1000, Richard Henderson wrote:
> We need not check for ARM vs Thumb state in order to dispatch
> disassembly of every instruction.
> 
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
>  target/arm/translate.c | 134 +++++++++++++++++++++++++++++++------------------
>  1 file changed, 86 insertions(+), 48 deletions(-)
> 
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index ebe1c1a..d7c3c10 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
(snip)
> +static void arm_post_translate_insn(CPUARMState *env, DisasContext *dc)
> +{
> +    /* Translation stops when a conditional branch is encountered.
> +     * Otherwise the subsequent code could get translated several times.
> +     * Also stop translation when a page boundary is reached.  This
> +     * ensures prefetch aborts occur at the right place.
> +     *
> +     * We want to stop the TB if the next insn starts in a new page,
> +     * or if it spans between this page and the next. This means that
> +     * if we're looking at the last halfword in the page we need to
> +     * see if it's a 16-bit Thumb insn (which will fit in this TB)
> +     * or a 32-bit Thumb insn (which won't).
> +     * This is to avoid generating a silly TB with a single 16-bit insn
> +     * in it at the end of this page (which would execute correctly
> +     * but isn't very efficient).
> +     */
> +    if (dc->base.is_jmp == DISAS_NEXT
> +        && (dc->pc >= dc->next_page_start
> +            || (dc->pc >= dc->next_page_start - 3
> +                && insn_crosses_page(env, dc)))) {
> +        dc->base.is_jmp = DISAS_TOO_MANY;
>      }
>  
>      if (dc->condjmp && !dc->base.is_jmp) {
>          gen_set_label(dc->condlabel);
>          dc->condjmp = 0;
>      }
> +    dc->base.pc_next = dc->pc;
> +    translator_loop_temp_check(&dc->base);
> +}
>  
> -    if (dc->base.is_jmp == DISAS_NEXT) {
> -        /* Translation stops when a conditional branch is encountered.
> -         * Otherwise the subsequent code could get translated several times.
> -         * Also stop translation when a page boundary is reached.  This
> -         * ensures prefetch aborts occur at the right place.  */
> -
> -        if (dc->pc >= dc->next_page_start ||
> -            (dc->pc >= dc->next_page_start - 3 &&
> -             insn_crosses_page(env, dc))) {
> -            /* We want to stop the TB if the next insn starts in a new page,
> -             * or if it spans between this page and the next. This means that
> -             * if we're looking at the last halfword in the page we need to
> -             * see if it's a 16-bit Thumb insn (which will fit in this TB)
> -             * or a 32-bit Thumb insn (which won't).
> -             * This is to avoid generating a silly TB with a single 16-bit insn
> -             * in it at the end of this page (which would execute correctly
> -             * but isn't very efficient).
> -             */
> -            dc->base.is_jmp = DISAS_TOO_MANY;

Note that we've moved the above hunk ("if (dc->base.is_jmp == DISAS_NEXT)")
above the "if (dc->condjmp && !dc->base.is_jmp)" one; so when we now set
is_jmp to DISAS_TOO_MANY, dc->condjmp might be wrongly cleared by the
second hunk.

My review missed this, but luckily TCG asserts caught it while booting
debian-arm:

[  OK  ] Started Increase datagram queue length.
systemd[1]: Started Increase datagram queue length.
[  OK  ] Mounted POSIX Message Queue File System.
systemd[1]: Mounted POSIX Message Queue File System.
qemu-system-arm: /data/src/qemu/tcg/tcg-op.c:2584: tcg_gen_goto_tb: Assertion `(tcg_ctx.goto_tb_issue_mask & (1 << idx)) == 0' failed.
Aborted (core dumped)

Keeping the original order fixes it.

		Emilio

--8<--
---
 target/arm/translate.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/target/arm/translate.c b/target/arm/translate.c
index 77fe6a9..9cbace5 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -11979,6 +11979,11 @@ static bool arm_pre_translate_insn(DisasContext *dc)
 
 static void arm_post_translate_insn(CPUARMState *env, DisasContext *dc)
 {
+    if (dc->condjmp && !dc->base.is_jmp) {
+        gen_set_label(dc->condlabel);
+        dc->condjmp = 0;
+    }
+
     /* Translation stops when a conditional branch is encountered.
      * Otherwise the subsequent code could get translated several times.
      * Also stop translation when a page boundary is reached.  This
@@ -12000,10 +12005,6 @@ static void arm_post_translate_insn(CPUARMState *env, DisasContext *dc)
         dc->base.is_jmp = DISAS_TOO_MANY;
     }
 
-    if (dc->condjmp && !dc->base.is_jmp) {
-        gen_set_label(dc->condlabel);
-        dc->condjmp = 0;
-    }
     dc->base.pc_next = dc->pc;
     translator_loop_temp_check(&dc->base);
 }
-- 
2.7.4

On 07/21/2017 02:35 PM, Emilio G. Cota wrote:
>   static void arm_post_translate_insn(CPUARMState *env, DisasContext *dc)
>   {
> +    if (dc->condjmp && !dc->base.is_jmp) {
> +        gen_set_label(dc->condlabel);
> +        dc->condjmp = 0;
> +    }
> +

Thanks for the catch.

I have merged the !dc->base.is_jmp check with the
following dc->base.is_jmp == DISAS_NEXT.

I hope that's clearer.


r~