[PATCH v2 0/3] target/avr: Convert to TranslatorOps

[PATCH v2 0/3] target/avr: Convert to TranslatorOps

[Richard Henderson]

I've reached a point where *all* targets must use the translator loop.  

Changes for v2:
  * Fix (drop) singlestep check for max_insns.
    We already do that generically.
  * Fully initialize DisasContext.

It was that last that caused the check-acceptance regression for v1.
I simply got lucky the first time (with optimization) and the
uninitialized struct members happened to have some zeros.


r~


Richard Henderson (3):
  target/avr: Add DisasContextBase to DisasContext
  target/avr: Change ctx to DisasContext* in gen_intermediate_code
  target/avr: Convert to TranslatorOps

 target/avr/translate.c | 284 ++++++++++++++++++++++-------------------
 1 file changed, 154 insertions(+), 130 deletions(-)

-- 
2.25.1

[PATCH v2 1/3] target/avr: Add DisasContextBase to DisasContext

[Richard Henderson]

Migrate the bstate, tb and singlestep_enabled fields
from DisasContext into the base.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/avr/translate.c | 58 +++++++++++++++++++++---------------------
 1 file changed, 29 insertions(+), 29 deletions(-)

diff --git a/target/avr/translate.c b/target/avr/translate.c
index 850c5941d9..20c5062730 100644
--- a/target/avr/translate.c
+++ b/target/avr/translate.c
@@ -80,7 +80,7 @@ typedef struct DisasContext DisasContext;
 
 /* This is the state at translation time. */
 struct DisasContext {
-    TranslationBlock *tb;
+    DisasContextBase base;
 
     CPUAVRState *env;
     CPUState *cs;
@@ -90,8 +90,6 @@ struct DisasContext {
 
     /* Routine used to access memory */
     int memidx;
-    int bstate;
-    int singlestep;
 
     /*
      * some AVR instructions can make the following instruction to be skipped
@@ -191,7 +189,7 @@ static bool avr_have_feature(DisasContext *ctx, int feature)
 {
     if (!avr_feature(ctx->env, feature)) {
         gen_helper_unsupported(cpu_env);
-        ctx->bstate = DISAS_NORETURN;
+        ctx->base.is_jmp = DISAS_NORETURN;
         return false;
     }
     return true;
@@ -1011,13 +1009,13 @@ static void gen_jmp_ez(DisasContext *ctx)
 {
     tcg_gen_deposit_tl(cpu_pc, cpu_r[30], cpu_r[31], 8, 8);
     tcg_gen_or_tl(cpu_pc, cpu_pc, cpu_eind);
-    ctx->bstate = DISAS_LOOKUP;
+    ctx->base.is_jmp = DISAS_LOOKUP;
 }
 
 static void gen_jmp_z(DisasContext *ctx)
 {
     tcg_gen_deposit_tl(cpu_pc, cpu_r[30], cpu_r[31], 8, 8);
-    ctx->bstate = DISAS_LOOKUP;
+    ctx->base.is_jmp = DISAS_LOOKUP;
 }
 
 static void gen_push_ret(DisasContext *ctx, int ret)
@@ -1083,9 +1081,9 @@ static void gen_pop_ret(DisasContext *ctx, TCGv ret)
 
 static void gen_goto_tb(DisasContext *ctx, int n, target_ulong dest)
 {
-    TranslationBlock *tb = ctx->tb;
+    const TranslationBlock *tb = ctx->base.tb;
 
-    if (ctx->singlestep == 0) {
+    if (!ctx->base.singlestep_enabled) {
         tcg_gen_goto_tb(n);
         tcg_gen_movi_i32(cpu_pc, dest);
         tcg_gen_exit_tb(tb, n);
@@ -1094,7 +1092,7 @@ static void gen_goto_tb(DisasContext *ctx, int n, target_ulong dest)
         gen_helper_debug(cpu_env);
         tcg_gen_exit_tb(NULL, 0);
     }
-    ctx->bstate = DISAS_NORETURN;
+    ctx->base.is_jmp = DISAS_NORETURN;
 }
 
 /*
@@ -1254,7 +1252,7 @@ static bool trans_RET(DisasContext *ctx, arg_RET *a)
 {
     gen_pop_ret(ctx, cpu_pc);
 
-    ctx->bstate = DISAS_LOOKUP;
+    ctx->base.is_jmp = DISAS_LOOKUP;
     return true;
 }
 
@@ -1272,7 +1270,7 @@ static bool trans_RETI(DisasContext *ctx, arg_RETI *a)
     tcg_gen_movi_tl(cpu_If, 1);
 
     /* Need to return to main loop to re-evaluate interrupts.  */
-    ctx->bstate = DISAS_EXIT;
+    ctx->base.is_jmp = DISAS_EXIT;
     return true;
 }
 
@@ -1484,7 +1482,7 @@ static bool trans_BRBC(DisasContext *ctx, arg_BRBC *a)
     gen_goto_tb(ctx, 0, ctx->npc + a->imm);
     gen_set_label(not_taken);
 
-    ctx->bstate = DISAS_CHAIN;
+    ctx->base.is_jmp = DISAS_CHAIN;
     return true;
 }
 
@@ -1533,7 +1531,7 @@ static bool trans_BRBS(DisasContext *ctx, arg_BRBS *a)
     gen_goto_tb(ctx, 0, ctx->npc + a->imm);
     gen_set_label(not_taken);
 
-    ctx->bstate = DISAS_CHAIN;
+    ctx->base.is_jmp = DISAS_CHAIN;
     return true;
 }
 
@@ -1610,7 +1608,7 @@ static TCGv gen_get_zaddr(void)
  */
 static void gen_data_store(DisasContext *ctx, TCGv data, TCGv addr)
 {
-    if (ctx->tb->flags & TB_FLAGS_FULL_ACCESS) {
+    if (ctx->base.tb->flags & TB_FLAGS_FULL_ACCESS) {
         gen_helper_fullwr(cpu_env, data, addr);
     } else {
         tcg_gen_qemu_st8(data, addr, MMU_DATA_IDX); /* mem[addr] = data */
@@ -1619,7 +1617,7 @@ static void gen_data_store(DisasContext *ctx, TCGv data, TCGv addr)
 
 static void gen_data_load(DisasContext *ctx, TCGv data, TCGv addr)
 {
-    if (ctx->tb->flags & TB_FLAGS_FULL_ACCESS) {
+    if (ctx->base.tb->flags & TB_FLAGS_FULL_ACCESS) {
         gen_helper_fullrd(data, cpu_env, addr);
     } else {
         tcg_gen_qemu_ld8u(data, addr, MMU_DATA_IDX); /* data = mem[addr] */
@@ -2793,7 +2791,7 @@ static bool trans_BREAK(DisasContext *ctx, arg_BREAK *a)
 #ifdef BREAKPOINT_ON_BREAK
     tcg_gen_movi_tl(cpu_pc, ctx->npc - 1);
     gen_helper_debug(cpu_env);
-    ctx->bstate = DISAS_EXIT;
+    ctx->base.is_jmp = DISAS_EXIT;
 #else
     /* NOP */
 #endif
@@ -2819,7 +2817,7 @@ static bool trans_NOP(DisasContext *ctx, arg_NOP *a)
 static bool trans_SLEEP(DisasContext *ctx, arg_SLEEP *a)
 {
     gen_helper_sleep(cpu_env);
-    ctx->bstate = DISAS_NORETURN;
+    ctx->base.is_jmp = DISAS_NORETURN;
     return true;
 }
 
@@ -2850,7 +2848,7 @@ static void translate(DisasContext *ctx)
 
     if (!decode_insn(ctx, opcode)) {
         gen_helper_unsupported(cpu_env);
-        ctx->bstate = DISAS_NORETURN;
+        ctx->base.is_jmp = DISAS_NORETURN;
     }
 }
 
@@ -2903,13 +2901,15 @@ void gen_intermediate_code(CPUState *cs, TranslationBlock *tb, int max_insns)
 {
     CPUAVRState *env = cs->env_ptr;
     DisasContext ctx = {
-        .tb = tb,
+        .base.tb = tb,
+        .base.is_jmp = DISAS_NEXT,
+        .base.pc_first = tb->pc,
+        .base.pc_next = tb->pc,
+        .base.singlestep_enabled = cs->singlestep_enabled,
         .cs = cs,
         .env = env,
         .memidx = 0,
-        .bstate = DISAS_NEXT,
         .skip_cond = TCG_COND_NEVER,
-        .singlestep = cs->singlestep_enabled,
     };
     target_ulong pc_start = tb->pc / 2;
     int num_insns = 0;
@@ -2921,7 +2921,7 @@ void gen_intermediate_code(CPUState *cs, TranslationBlock *tb, int max_insns)
          */
         max_insns = 1;
     }
-    if (ctx.singlestep) {
+    if (ctx.base.singlestep_enabled) {
         max_insns = 1;
     }
 
@@ -2946,7 +2946,7 @@ void gen_intermediate_code(CPUState *cs, TranslationBlock *tb, int max_insns)
          * b main   - sets breakpoint at address 0x00000100 (code)
          * b *0x100 - sets breakpoint at address 0x00800100 (data)
          */
-        if (unlikely(!ctx.singlestep &&
+        if (unlikely(!ctx.base.singlestep_enabled &&
                 (cpu_breakpoint_test(cs, OFFSET_CODE + ctx.npc * 2, BP_ANY) ||
                  cpu_breakpoint_test(cs, OFFSET_DATA + ctx.npc * 2, BP_ANY)))) {
             canonicalize_skip(&ctx);
@@ -2989,11 +2989,11 @@ void gen_intermediate_code(CPUState *cs, TranslationBlock *tb, int max_insns)
         if (skip_label) {
             canonicalize_skip(&ctx);
             gen_set_label(skip_label);
-            if (ctx.bstate == DISAS_NORETURN) {
-                ctx.bstate = DISAS_CHAIN;
+            if (ctx.base.is_jmp == DISAS_NORETURN) {
+                ctx.base.is_jmp = DISAS_CHAIN;
             }
         }
-    } while (ctx.bstate == DISAS_NEXT
+    } while (ctx.base.is_jmp == DISAS_NEXT
              && num_insns < max_insns
              && (ctx.npc - pc_start) * 2 < TARGET_PAGE_SIZE - 4
              && !tcg_op_buf_full());
@@ -3004,7 +3004,7 @@ void gen_intermediate_code(CPUState *cs, TranslationBlock *tb, int max_insns)
 
     bool nonconst_skip = canonicalize_skip(&ctx);
 
-    switch (ctx.bstate) {
+    switch (ctx.base.is_jmp) {
     case DISAS_NORETURN:
         assert(!nonconst_skip);
         break;
@@ -3019,13 +3019,13 @@ void gen_intermediate_code(CPUState *cs, TranslationBlock *tb, int max_insns)
         tcg_gen_movi_tl(cpu_pc, ctx.npc);
         /* fall through */
     case DISAS_LOOKUP:
-        if (!ctx.singlestep) {
+        if (!ctx.base.singlestep_enabled) {
             tcg_gen_lookup_and_goto_ptr();
             break;
         }
         /* fall through */
     case DISAS_EXIT:
-        if (ctx.singlestep) {
+        if (ctx.base.singlestep_enabled) {
             gen_helper_debug(cpu_env);
         } else {
             tcg_gen_exit_tb(NULL, 0);
-- 
2.25.1

Re: [PATCH v2 1/3] target/avr: Add DisasContextBase to DisasContext

[Philippe Mathieu-Daudé]

On 6/20/21 11:50 PM, Richard Henderson wrote:
> Migrate the bstate, tb and singlestep_enabled fields
> from DisasContext into the base.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/avr/translate.c | 58 +++++++++++++++++++++---------------------
>  1 file changed, 29 insertions(+), 29 deletions(-)

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

[PATCH v2 2/3] target/avr: Change ctx to DisasContext* in gen_intermediate_code

[Richard Henderson]

Prepare for receiving it as a pointer input.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/avr/translate.c | 84 +++++++++++++++++++++---------------------
 1 file changed, 43 insertions(+), 41 deletions(-)

diff --git a/target/avr/translate.c b/target/avr/translate.c
index 20c5062730..66e9882422 100644
--- a/target/avr/translate.c
+++ b/target/avr/translate.c
@@ -104,7 +104,7 @@ struct DisasContext {
      * used in the following manner (sketch)
      *
      * TCGLabel *skip_label = NULL;
-     * if (ctx.skip_cond != TCG_COND_NEVER) {
+     * if (ctx->skip_cond != TCG_COND_NEVER) {
      *     skip_label = gen_new_label();
      *     tcg_gen_brcond_tl(skip_cond, skip_var0, skip_var1, skip_label);
      * }
@@ -114,7 +114,7 @@ struct DisasContext {
      *     free_skip_var0 = false;
      * }
      *
-     * translate(&ctx);
+     * translate(ctx);
      *
      * if (skip_label) {
      *     gen_set_label(skip_label);
@@ -2900,7 +2900,7 @@ static bool canonicalize_skip(DisasContext *ctx)
 void gen_intermediate_code(CPUState *cs, TranslationBlock *tb, int max_insns)
 {
     CPUAVRState *env = cs->env_ptr;
-    DisasContext ctx = {
+    DisasContext ctx1 = {
         .base.tb = tb,
         .base.is_jmp = DISAS_NEXT,
         .base.pc_first = tb->pc,
@@ -2911,6 +2911,7 @@ void gen_intermediate_code(CPUState *cs, TranslationBlock *tb, int max_insns)
         .memidx = 0,
         .skip_cond = TCG_COND_NEVER,
     };
+    DisasContext *ctx = &ctx1;
     target_ulong pc_start = tb->pc / 2;
     int num_insns = 0;
 
@@ -2921,23 +2922,23 @@ void gen_intermediate_code(CPUState *cs, TranslationBlock *tb, int max_insns)
          */
         max_insns = 1;
     }
-    if (ctx.base.singlestep_enabled) {
+    if (ctx->base.singlestep_enabled) {
         max_insns = 1;
     }
 
     gen_tb_start(tb);
 
-    ctx.npc = pc_start;
+    ctx->npc = pc_start;
     if (tb->flags & TB_FLAGS_SKIP) {
-        ctx.skip_cond = TCG_COND_ALWAYS;
-        ctx.skip_var0 = cpu_skip;
+        ctx->skip_cond = TCG_COND_ALWAYS;
+        ctx->skip_var0 = cpu_skip;
     }
 
     do {
         TCGLabel *skip_label = NULL;
 
         /* translate current instruction */
-        tcg_gen_insn_start(ctx.npc);
+        tcg_gen_insn_start(ctx->npc);
         num_insns++;
 
         /*
@@ -2946,65 +2947,66 @@ void gen_intermediate_code(CPUState *cs, TranslationBlock *tb, int max_insns)
          * b main   - sets breakpoint at address 0x00000100 (code)
          * b *0x100 - sets breakpoint at address 0x00800100 (data)
          */
-        if (unlikely(!ctx.base.singlestep_enabled &&
-                (cpu_breakpoint_test(cs, OFFSET_CODE + ctx.npc * 2, BP_ANY) ||
-                 cpu_breakpoint_test(cs, OFFSET_DATA + ctx.npc * 2, BP_ANY)))) {
-            canonicalize_skip(&ctx);
-            tcg_gen_movi_tl(cpu_pc, ctx.npc);
+        if (unlikely(!ctx->base.singlestep_enabled &&
+            (cpu_breakpoint_test(cs, OFFSET_CODE + ctx->npc * 2, BP_ANY) ||
+             cpu_breakpoint_test(cs, OFFSET_DATA + ctx->npc * 2, BP_ANY)))) {
+            canonicalize_skip(ctx);
+            tcg_gen_movi_tl(cpu_pc, ctx->npc);
             gen_helper_debug(cpu_env);
             goto done_generating;
         }
 
         /* Conditionally skip the next instruction, if indicated.  */
-        if (ctx.skip_cond != TCG_COND_NEVER) {
+        if (ctx->skip_cond != TCG_COND_NEVER) {
             skip_label = gen_new_label();
-            if (ctx.skip_var0 == cpu_skip) {
+            if (ctx->skip_var0 == cpu_skip) {
                 /*
                  * Copy cpu_skip so that we may zero it before the branch.
                  * This ensures that cpu_skip is non-zero after the label
                  * if and only if the skipped insn itself sets a skip.
                  */
-                ctx.free_skip_var0 = true;
-                ctx.skip_var0 = tcg_temp_new();
-                tcg_gen_mov_tl(ctx.skip_var0, cpu_skip);
+                ctx->free_skip_var0 = true;
+                ctx->skip_var0 = tcg_temp_new();
+                tcg_gen_mov_tl(ctx->skip_var0, cpu_skip);
                 tcg_gen_movi_tl(cpu_skip, 0);
             }
-            if (ctx.skip_var1 == NULL) {
-                tcg_gen_brcondi_tl(ctx.skip_cond, ctx.skip_var0, 0, skip_label);
+            if (ctx->skip_var1 == NULL) {
+                tcg_gen_brcondi_tl(ctx->skip_cond, ctx->skip_var0,
+                                   0, skip_label);
             } else {
-                tcg_gen_brcond_tl(ctx.skip_cond, ctx.skip_var0,
-                                  ctx.skip_var1, skip_label);
-                ctx.skip_var1 = NULL;
+                tcg_gen_brcond_tl(ctx->skip_cond, ctx->skip_var0,
+                                  ctx->skip_var1, skip_label);
+                ctx->skip_var1 = NULL;
             }
-            if (ctx.free_skip_var0) {
-                tcg_temp_free(ctx.skip_var0);
-                ctx.free_skip_var0 = false;
+            if (ctx->free_skip_var0) {
+                tcg_temp_free(ctx->skip_var0);
+                ctx->free_skip_var0 = false;
             }
-            ctx.skip_cond = TCG_COND_NEVER;
-            ctx.skip_var0 = NULL;
+            ctx->skip_cond = TCG_COND_NEVER;
+            ctx->skip_var0 = NULL;
         }
 
-        translate(&ctx);
+        translate(ctx);
 
         if (skip_label) {
-            canonicalize_skip(&ctx);
+            canonicalize_skip(ctx);
             gen_set_label(skip_label);
-            if (ctx.base.is_jmp == DISAS_NORETURN) {
-                ctx.base.is_jmp = DISAS_CHAIN;
+            if (ctx->base.is_jmp == DISAS_NORETURN) {
+                ctx->base.is_jmp = DISAS_CHAIN;
             }
         }
-    } while (ctx.base.is_jmp == DISAS_NEXT
+    } while (ctx->base.is_jmp == DISAS_NEXT
              && num_insns < max_insns
-             && (ctx.npc - pc_start) * 2 < TARGET_PAGE_SIZE - 4
+             && (ctx->npc - pc_start) * 2 < TARGET_PAGE_SIZE - 4
              && !tcg_op_buf_full());
 
     if (tb->cflags & CF_LAST_IO) {
         gen_io_end();
     }
 
-    bool nonconst_skip = canonicalize_skip(&ctx);
+    bool nonconst_skip = canonicalize_skip(ctx);
 
-    switch (ctx.base.is_jmp) {
+    switch (ctx->base.is_jmp) {
     case DISAS_NORETURN:
         assert(!nonconst_skip);
         break;
@@ -3013,19 +3015,19 @@ void gen_intermediate_code(CPUState *cs, TranslationBlock *tb, int max_insns)
     case DISAS_CHAIN:
         if (!nonconst_skip) {
             /* Note gen_goto_tb checks singlestep.  */
-            gen_goto_tb(&ctx, 1, ctx.npc);
+            gen_goto_tb(ctx, 1, ctx->npc);
             break;
         }
-        tcg_gen_movi_tl(cpu_pc, ctx.npc);
+        tcg_gen_movi_tl(cpu_pc, ctx->npc);
         /* fall through */
     case DISAS_LOOKUP:
-        if (!ctx.base.singlestep_enabled) {
+        if (!ctx->base.singlestep_enabled) {
             tcg_gen_lookup_and_goto_ptr();
             break;
         }
         /* fall through */
     case DISAS_EXIT:
-        if (ctx.base.singlestep_enabled) {
+        if (ctx->base.singlestep_enabled) {
             gen_helper_debug(cpu_env);
         } else {
             tcg_gen_exit_tb(NULL, 0);
@@ -3038,7 +3040,7 @@ void gen_intermediate_code(CPUState *cs, TranslationBlock *tb, int max_insns)
 done_generating:
     gen_tb_end(tb, num_insns);
 
-    tb->size = (ctx.npc - pc_start) * 2;
+    tb->size = (ctx->npc - pc_start) * 2;
     tb->icount = num_insns;
 
 #ifdef DEBUG_DISAS
-- 
2.25.1

Re: [PATCH v2 2/3] target/avr: Change ctx to DisasContext* in gen_intermediate_code

[Philippe Mathieu-Daudé]

On 6/20/21 11:50 PM, Richard Henderson wrote:
> Prepare for receiving it as a pointer input.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/avr/translate.c | 84 +++++++++++++++++++++---------------------
>  1 file changed, 43 insertions(+), 41 deletions(-)

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

[PATCH v2 3/3] target/avr: Convert to TranslatorOps

[Richard Henderson]

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/avr/translate.c | 234 ++++++++++++++++++++++-------------------
 1 file changed, 128 insertions(+), 106 deletions(-)

diff --git a/target/avr/translate.c b/target/avr/translate.c
index 66e9882422..c06ce45bc7 100644
--- a/target/avr/translate.c
+++ b/target/avr/translate.c
@@ -2897,113 +2897,131 @@ static bool canonicalize_skip(DisasContext *ctx)
     return true;
 }
 
-void gen_intermediate_code(CPUState *cs, TranslationBlock *tb, int max_insns)
+static void gen_breakpoint(DisasContext *ctx)
 {
+    canonicalize_skip(ctx);
+    tcg_gen_movi_tl(cpu_pc, ctx->npc);
+    gen_helper_debug(cpu_env);
+    ctx->base.is_jmp = DISAS_NORETURN;
+}
+
+static void avr_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
+{
+    DisasContext *ctx = container_of(dcbase, DisasContext, base);
     CPUAVRState *env = cs->env_ptr;
-    DisasContext ctx1 = {
-        .base.tb = tb,
-        .base.is_jmp = DISAS_NEXT,
-        .base.pc_first = tb->pc,
-        .base.pc_next = tb->pc,
-        .base.singlestep_enabled = cs->singlestep_enabled,
-        .cs = cs,
-        .env = env,
-        .memidx = 0,
-        .skip_cond = TCG_COND_NEVER,
-    };
-    DisasContext *ctx = &ctx1;
-    target_ulong pc_start = tb->pc / 2;
-    int num_insns = 0;
+    uint32_t tb_flags = ctx->base.tb->flags;
 
-    if (tb->flags & TB_FLAGS_FULL_ACCESS) {
-        /*
-         * This flag is set by ST/LD instruction we will regenerate it ONLY
-         * with mem/cpu memory access instead of mem access
-         */
-        max_insns = 1;
-    }
-    if (ctx->base.singlestep_enabled) {
-        max_insns = 1;
-    }
+    ctx->cs = cs;
+    ctx->env = env;
+    ctx->npc = ctx->base.pc_first / 2;
 
-    gen_tb_start(tb);
-
-    ctx->npc = pc_start;
-    if (tb->flags & TB_FLAGS_SKIP) {
+    ctx->skip_cond = TCG_COND_NEVER;
+    if (tb_flags & TB_FLAGS_SKIP) {
         ctx->skip_cond = TCG_COND_ALWAYS;
         ctx->skip_var0 = cpu_skip;
     }
 
-    do {
-        TCGLabel *skip_label = NULL;
-
-        /* translate current instruction */
-        tcg_gen_insn_start(ctx->npc);
-        num_insns++;
-
+    if (tb_flags & TB_FLAGS_FULL_ACCESS) {
         /*
-         * this is due to some strange GDB behavior
-         * let's assume main has address 0x100
-         * b main   - sets breakpoint at address 0x00000100 (code)
-         * b *0x100 - sets breakpoint at address 0x00800100 (data)
+         * This flag is set by ST/LD instruction we will regenerate it ONLY
+         * with mem/cpu memory access instead of mem access
          */
-        if (unlikely(!ctx->base.singlestep_enabled &&
-            (cpu_breakpoint_test(cs, OFFSET_CODE + ctx->npc * 2, BP_ANY) ||
-             cpu_breakpoint_test(cs, OFFSET_DATA + ctx->npc * 2, BP_ANY)))) {
-            canonicalize_skip(ctx);
-            tcg_gen_movi_tl(cpu_pc, ctx->npc);
-            gen_helper_debug(cpu_env);
-            goto done_generating;
-        }
+        ctx->base.max_insns = 1;
+    }
+}
 
-        /* Conditionally skip the next instruction, if indicated.  */
-        if (ctx->skip_cond != TCG_COND_NEVER) {
-            skip_label = gen_new_label();
-            if (ctx->skip_var0 == cpu_skip) {
-                /*
-                 * Copy cpu_skip so that we may zero it before the branch.
-                 * This ensures that cpu_skip is non-zero after the label
-                 * if and only if the skipped insn itself sets a skip.
-                 */
-                ctx->free_skip_var0 = true;
-                ctx->skip_var0 = tcg_temp_new();
-                tcg_gen_mov_tl(ctx->skip_var0, cpu_skip);
-                tcg_gen_movi_tl(cpu_skip, 0);
-            }
-            if (ctx->skip_var1 == NULL) {
-                tcg_gen_brcondi_tl(ctx->skip_cond, ctx->skip_var0,
-                                   0, skip_label);
-            } else {
-                tcg_gen_brcond_tl(ctx->skip_cond, ctx->skip_var0,
-                                  ctx->skip_var1, skip_label);
-                ctx->skip_var1 = NULL;
-            }
-            if (ctx->free_skip_var0) {
-                tcg_temp_free(ctx->skip_var0);
-                ctx->free_skip_var0 = false;
-            }
-            ctx->skip_cond = TCG_COND_NEVER;
-            ctx->skip_var0 = NULL;
-        }
+static void avr_tr_tb_start(DisasContextBase *db, CPUState *cs)
+{
+}
 
-        translate(ctx);
+static void avr_tr_insn_start(DisasContextBase *dcbase, CPUState *cs)
+{
+    DisasContext *ctx = container_of(dcbase, DisasContext, base);
 
-        if (skip_label) {
-            canonicalize_skip(ctx);
-            gen_set_label(skip_label);
-            if (ctx->base.is_jmp == DISAS_NORETURN) {
-                ctx->base.is_jmp = DISAS_CHAIN;
-            }
-        }
-    } while (ctx->base.is_jmp == DISAS_NEXT
-             && num_insns < max_insns
-             && (ctx->npc - pc_start) * 2 < TARGET_PAGE_SIZE - 4
-             && !tcg_op_buf_full());
+    tcg_gen_insn_start(ctx->npc);
+}
 
-    if (tb->cflags & CF_LAST_IO) {
-        gen_io_end();
+static bool avr_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cs,
+                                    const CPUBreakpoint *bp)
+{
+    DisasContext *ctx = container_of(dcbase, DisasContext, base);
+
+    gen_breakpoint(ctx);
+    return true;
+}
+
+static void avr_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs)
+{
+    DisasContext *ctx = container_of(dcbase, DisasContext, base);
+    TCGLabel *skip_label = NULL;
+
+    /*
+     * This is due to some strange GDB behavior
+     * Let's assume main has address 0x100:
+     * b main   - sets breakpoint at address 0x00000100 (code)
+     * b *0x100 - sets breakpoint at address 0x00800100 (data)
+     *
+     * The translator driver has already taken care of the code pointer.
+     */
+    if (!ctx->base.singlestep_enabled &&
+        cpu_breakpoint_test(cs, OFFSET_DATA + ctx->base.pc_next, BP_ANY)) {
+        gen_breakpoint(ctx);
+        return;
     }
 
+    /* Conditionally skip the next instruction, if indicated.  */
+    if (ctx->skip_cond != TCG_COND_NEVER) {
+        skip_label = gen_new_label();
+        if (ctx->skip_var0 == cpu_skip) {
+            /*
+             * Copy cpu_skip so that we may zero it before the branch.
+             * This ensures that cpu_skip is non-zero after the label
+             * if and only if the skipped insn itself sets a skip.
+             */
+            ctx->free_skip_var0 = true;
+            ctx->skip_var0 = tcg_temp_new();
+            tcg_gen_mov_tl(ctx->skip_var0, cpu_skip);
+            tcg_gen_movi_tl(cpu_skip, 0);
+        }
+        if (ctx->skip_var1 == NULL) {
+            tcg_gen_brcondi_tl(ctx->skip_cond, ctx->skip_var0, 0, skip_label);
+        } else {
+            tcg_gen_brcond_tl(ctx->skip_cond, ctx->skip_var0,
+                              ctx->skip_var1, skip_label);
+            ctx->skip_var1 = NULL;
+        }
+        if (ctx->free_skip_var0) {
+            tcg_temp_free(ctx->skip_var0);
+            ctx->free_skip_var0 = false;
+        }
+        ctx->skip_cond = TCG_COND_NEVER;
+        ctx->skip_var0 = NULL;
+    }
+
+    translate(ctx);
+
+    ctx->base.pc_next = ctx->npc * 2;
+
+    if (skip_label) {
+        canonicalize_skip(ctx);
+        gen_set_label(skip_label);
+        if (ctx->base.is_jmp == DISAS_NORETURN) {
+            ctx->base.is_jmp = DISAS_CHAIN;
+        }
+    }
+
+    if (ctx->base.is_jmp == DISAS_NEXT) {
+        target_ulong page_first = ctx->base.pc_first & TARGET_PAGE_MASK;
+
+        if ((ctx->base.pc_next - page_first) >= TARGET_PAGE_SIZE - 4) {
+            ctx->base.is_jmp = DISAS_TOO_MANY;
+        }
+    }
+}
+
+static void avr_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs)
+{
+    DisasContext *ctx = container_of(dcbase, DisasContext, base);
     bool nonconst_skip = canonicalize_skip(ctx);
 
     switch (ctx->base.is_jmp) {
@@ -3036,24 +3054,28 @@ void gen_intermediate_code(CPUState *cs, TranslationBlock *tb, int max_insns)
     default:
         g_assert_not_reached();
     }
+}
 
-done_generating:
-    gen_tb_end(tb, num_insns);
+static void avr_tr_disas_log(const DisasContextBase *dcbase, CPUState *cs)
+{
+    qemu_log("IN: %s\n", lookup_symbol(dcbase->pc_first));
+    log_target_disas(cs, dcbase->pc_first, dcbase->tb->size);
+}
 
-    tb->size = (ctx->npc - pc_start) * 2;
-    tb->icount = num_insns;
+static const TranslatorOps avr_tr_ops = {
+    .init_disas_context = avr_tr_init_disas_context,
+    .tb_start           = avr_tr_tb_start,
+    .insn_start         = avr_tr_insn_start,
+    .breakpoint_check   = avr_tr_breakpoint_check,
+    .translate_insn     = avr_tr_translate_insn,
+    .tb_stop            = avr_tr_tb_stop,
+    .disas_log          = avr_tr_disas_log,
+};
 
-#ifdef DEBUG_DISAS
-    if (qemu_loglevel_mask(CPU_LOG_TB_IN_ASM)
-        && qemu_log_in_addr_range(tb->pc)) {
-        FILE *fd;
-        fd = qemu_log_lock();
-        qemu_log("IN: %s\n", lookup_symbol(tb->pc));
-        log_target_disas(cs, tb->pc, tb->size);
-        qemu_log("\n");
-        qemu_log_unlock(fd);
-    }
-#endif
+void gen_intermediate_code(CPUState *cs, TranslationBlock *tb, int max_insns)
+{
+    DisasContext dc = { };
+    translator_loop(&avr_tr_ops, &dc.base, cs, tb, max_insns);
 }
 
 void restore_state_to_opc(CPUAVRState *env, TranslationBlock *tb,
-- 
2.25.1

Re: [PATCH v2 3/3] target/avr: Convert to TranslatorOps

[Michael Rolnik]

[-- Attachment #1: Type: text/plain, Size: 10205 bytes --]

Reviewed-by: Michael Rolnik <mrolnik@gmail.com>
Tested-by: Michael Rolnik <mrolnik@gmail.com>

On Mon, Jun 21, 2021 at 12:50 AM Richard Henderson <
richard.henderson@linaro.org> wrote:

> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/avr/translate.c | 234 ++++++++++++++++++++++-------------------
>  1 file changed, 128 insertions(+), 106 deletions(-)
>
> diff --git a/target/avr/translate.c b/target/avr/translate.c
> index 66e9882422..c06ce45bc7 100644
> --- a/target/avr/translate.c
> +++ b/target/avr/translate.c
> @@ -2897,113 +2897,131 @@ static bool canonicalize_skip(DisasContext *ctx)
>      return true;
>  }
>
> -void gen_intermediate_code(CPUState *cs, TranslationBlock *tb, int
> max_insns)
> +static void gen_breakpoint(DisasContext *ctx)
>  {
> +    canonicalize_skip(ctx);
> +    tcg_gen_movi_tl(cpu_pc, ctx->npc);
> +    gen_helper_debug(cpu_env);
> +    ctx->base.is_jmp = DISAS_NORETURN;
> +}
> +
> +static void avr_tr_init_disas_context(DisasContextBase *dcbase, CPUState
> *cs)
> +{
> +    DisasContext *ctx = container_of(dcbase, DisasContext, base);
>      CPUAVRState *env = cs->env_ptr;
> -    DisasContext ctx1 = {
> -        .base.tb = tb,
> -        .base.is_jmp = DISAS_NEXT,
> -        .base.pc_first = tb->pc,
> -        .base.pc_next = tb->pc,
> -        .base.singlestep_enabled = cs->singlestep_enabled,
> -        .cs = cs,
> -        .env = env,
> -        .memidx = 0,
> -        .skip_cond = TCG_COND_NEVER,
> -    };
> -    DisasContext *ctx = &ctx1;
> -    target_ulong pc_start = tb->pc / 2;
> -    int num_insns = 0;
> +    uint32_t tb_flags = ctx->base.tb->flags;
>
> -    if (tb->flags & TB_FLAGS_FULL_ACCESS) {
> -        /*
> -         * This flag is set by ST/LD instruction we will regenerate it
> ONLY
> -         * with mem/cpu memory access instead of mem access
> -         */
> -        max_insns = 1;
> -    }
> -    if (ctx->base.singlestep_enabled) {
> -        max_insns = 1;
> -    }
> +    ctx->cs = cs;
> +    ctx->env = env;
> +    ctx->npc = ctx->base.pc_first / 2;
>
> -    gen_tb_start(tb);
> -
> -    ctx->npc = pc_start;
> -    if (tb->flags & TB_FLAGS_SKIP) {
> +    ctx->skip_cond = TCG_COND_NEVER;
> +    if (tb_flags & TB_FLAGS_SKIP) {
>          ctx->skip_cond = TCG_COND_ALWAYS;
>          ctx->skip_var0 = cpu_skip;
>      }
>
> -    do {
> -        TCGLabel *skip_label = NULL;
> -
> -        /* translate current instruction */
> -        tcg_gen_insn_start(ctx->npc);
> -        num_insns++;
> -
> +    if (tb_flags & TB_FLAGS_FULL_ACCESS) {
>          /*
> -         * this is due to some strange GDB behavior
> -         * let's assume main has address 0x100
> -         * b main   - sets breakpoint at address 0x00000100 (code)
> -         * b *0x100 - sets breakpoint at address 0x00800100 (data)
> +         * This flag is set by ST/LD instruction we will regenerate it
> ONLY
> +         * with mem/cpu memory access instead of mem access
>           */
> -        if (unlikely(!ctx->base.singlestep_enabled &&
> -            (cpu_breakpoint_test(cs, OFFSET_CODE + ctx->npc * 2, BP_ANY)
> ||
> -             cpu_breakpoint_test(cs, OFFSET_DATA + ctx->npc * 2,
> BP_ANY)))) {
> -            canonicalize_skip(ctx);
> -            tcg_gen_movi_tl(cpu_pc, ctx->npc);
> -            gen_helper_debug(cpu_env);
> -            goto done_generating;
> -        }
> +        ctx->base.max_insns = 1;
> +    }
> +}
>
> -        /* Conditionally skip the next instruction, if indicated.  */
> -        if (ctx->skip_cond != TCG_COND_NEVER) {
> -            skip_label = gen_new_label();
> -            if (ctx->skip_var0 == cpu_skip) {
> -                /*
> -                 * Copy cpu_skip so that we may zero it before the branch.
> -                 * This ensures that cpu_skip is non-zero after the label
> -                 * if and only if the skipped insn itself sets a skip.
> -                 */
> -                ctx->free_skip_var0 = true;
> -                ctx->skip_var0 = tcg_temp_new();
> -                tcg_gen_mov_tl(ctx->skip_var0, cpu_skip);
> -                tcg_gen_movi_tl(cpu_skip, 0);
> -            }
> -            if (ctx->skip_var1 == NULL) {
> -                tcg_gen_brcondi_tl(ctx->skip_cond, ctx->skip_var0,
> -                                   0, skip_label);
> -            } else {
> -                tcg_gen_brcond_tl(ctx->skip_cond, ctx->skip_var0,
> -                                  ctx->skip_var1, skip_label);
> -                ctx->skip_var1 = NULL;
> -            }
> -            if (ctx->free_skip_var0) {
> -                tcg_temp_free(ctx->skip_var0);
> -                ctx->free_skip_var0 = false;
> -            }
> -            ctx->skip_cond = TCG_COND_NEVER;
> -            ctx->skip_var0 = NULL;
> -        }
> +static void avr_tr_tb_start(DisasContextBase *db, CPUState *cs)
> +{
> +}
>
> -        translate(ctx);
> +static void avr_tr_insn_start(DisasContextBase *dcbase, CPUState *cs)
> +{
> +    DisasContext *ctx = container_of(dcbase, DisasContext, base);
>
> -        if (skip_label) {
> -            canonicalize_skip(ctx);
> -            gen_set_label(skip_label);
> -            if (ctx->base.is_jmp == DISAS_NORETURN) {
> -                ctx->base.is_jmp = DISAS_CHAIN;
> -            }
> -        }
> -    } while (ctx->base.is_jmp == DISAS_NEXT
> -             && num_insns < max_insns
> -             && (ctx->npc - pc_start) * 2 < TARGET_PAGE_SIZE - 4
> -             && !tcg_op_buf_full());
> +    tcg_gen_insn_start(ctx->npc);
> +}
>
> -    if (tb->cflags & CF_LAST_IO) {
> -        gen_io_end();
> +static bool avr_tr_breakpoint_check(DisasContextBase *dcbase, CPUState
> *cs,
> +                                    const CPUBreakpoint *bp)
> +{
> +    DisasContext *ctx = container_of(dcbase, DisasContext, base);
> +
> +    gen_breakpoint(ctx);
> +    return true;
> +}
> +
> +static void avr_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs)
> +{
> +    DisasContext *ctx = container_of(dcbase, DisasContext, base);
> +    TCGLabel *skip_label = NULL;
> +
> +    /*
> +     * This is due to some strange GDB behavior
> +     * Let's assume main has address 0x100:
> +     * b main   - sets breakpoint at address 0x00000100 (code)
> +     * b *0x100 - sets breakpoint at address 0x00800100 (data)
> +     *
> +     * The translator driver has already taken care of the code pointer.
> +     */
> +    if (!ctx->base.singlestep_enabled &&
> +        cpu_breakpoint_test(cs, OFFSET_DATA + ctx->base.pc_next, BP_ANY))
> {
> +        gen_breakpoint(ctx);
> +        return;
>      }
>
> +    /* Conditionally skip the next instruction, if indicated.  */
> +    if (ctx->skip_cond != TCG_COND_NEVER) {
> +        skip_label = gen_new_label();
> +        if (ctx->skip_var0 == cpu_skip) {
> +            /*
> +             * Copy cpu_skip so that we may zero it before the branch.
> +             * This ensures that cpu_skip is non-zero after the label
> +             * if and only if the skipped insn itself sets a skip.
> +             */
> +            ctx->free_skip_var0 = true;
> +            ctx->skip_var0 = tcg_temp_new();
> +            tcg_gen_mov_tl(ctx->skip_var0, cpu_skip);
> +            tcg_gen_movi_tl(cpu_skip, 0);
> +        }
> +        if (ctx->skip_var1 == NULL) {
> +            tcg_gen_brcondi_tl(ctx->skip_cond, ctx->skip_var0, 0,
> skip_label);
> +        } else {
> +            tcg_gen_brcond_tl(ctx->skip_cond, ctx->skip_var0,
> +                              ctx->skip_var1, skip_label);
> +            ctx->skip_var1 = NULL;
> +        }
> +        if (ctx->free_skip_var0) {
> +            tcg_temp_free(ctx->skip_var0);
> +            ctx->free_skip_var0 = false;
> +        }
> +        ctx->skip_cond = TCG_COND_NEVER;
> +        ctx->skip_var0 = NULL;
> +    }
> +
> +    translate(ctx);
> +
> +    ctx->base.pc_next = ctx->npc * 2;
> +
> +    if (skip_label) {
> +        canonicalize_skip(ctx);
> +        gen_set_label(skip_label);
> +        if (ctx->base.is_jmp == DISAS_NORETURN) {
> +            ctx->base.is_jmp = DISAS_CHAIN;
> +        }
> +    }
> +
> +    if (ctx->base.is_jmp == DISAS_NEXT) {
> +        target_ulong page_first = ctx->base.pc_first & TARGET_PAGE_MASK;
> +
> +        if ((ctx->base.pc_next - page_first) >= TARGET_PAGE_SIZE - 4) {
> +            ctx->base.is_jmp = DISAS_TOO_MANY;
> +        }
> +    }
> +}
> +
> +static void avr_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs)
> +{
> +    DisasContext *ctx = container_of(dcbase, DisasContext, base);
>      bool nonconst_skip = canonicalize_skip(ctx);
>
>      switch (ctx->base.is_jmp) {
> @@ -3036,24 +3054,28 @@ void gen_intermediate_code(CPUState *cs,
> TranslationBlock *tb, int max_insns)
>      default:
>          g_assert_not_reached();
>      }
> +}
>
> -done_generating:
> -    gen_tb_end(tb, num_insns);
> +static void avr_tr_disas_log(const DisasContextBase *dcbase, CPUState *cs)
> +{
> +    qemu_log("IN: %s\n", lookup_symbol(dcbase->pc_first));
> +    log_target_disas(cs, dcbase->pc_first, dcbase->tb->size);
> +}
>
> -    tb->size = (ctx->npc - pc_start) * 2;
> -    tb->icount = num_insns;
> +static const TranslatorOps avr_tr_ops = {
> +    .init_disas_context = avr_tr_init_disas_context,
> +    .tb_start           = avr_tr_tb_start,
> +    .insn_start         = avr_tr_insn_start,
> +    .breakpoint_check   = avr_tr_breakpoint_check,
> +    .translate_insn     = avr_tr_translate_insn,
> +    .tb_stop            = avr_tr_tb_stop,
> +    .disas_log          = avr_tr_disas_log,
> +};
>
> -#ifdef DEBUG_DISAS
> -    if (qemu_loglevel_mask(CPU_LOG_TB_IN_ASM)
> -        && qemu_log_in_addr_range(tb->pc)) {
> -        FILE *fd;
> -        fd = qemu_log_lock();
> -        qemu_log("IN: %s\n", lookup_symbol(tb->pc));
> -        log_target_disas(cs, tb->pc, tb->size);
> -        qemu_log("\n");
> -        qemu_log_unlock(fd);
> -    }
> -#endif
> +void gen_intermediate_code(CPUState *cs, TranslationBlock *tb, int
> max_insns)
> +{
> +    DisasContext dc = { };
> +    translator_loop(&avr_tr_ops, &dc.base, cs, tb, max_insns);
>  }
>
>  void restore_state_to_opc(CPUAVRState *env, TranslationBlock *tb,
> --
> 2.25.1
>
>

-- 
Best Regards,
Michael Rolnik

[-- Attachment #2: Type: text/html, Size: 12899 bytes --]

Re: [PATCH v2 3/3] target/avr: Convert to TranslatorOps

[Philippe Mathieu-Daudé]

Hi Michael,

On 6/21/21 7:38 AM, Michael Rolnik wrote:
> Reviewed-by: Michael Rolnik <mrolnik@gmail.com <mailto:mrolnik@gmail.com>>
> Tested-by: Michael Rolnik <mrolnik@gmail.com <mailto:mrolnik@gmail.com>>

Are your tags valid for the latest patch or the whole series?

> On Mon, Jun 21, 2021 at 12:50 AM Richard Henderson
> <richard.henderson@linaro.org <mailto:richard.henderson@linaro.org>> wrote:
> 
>     Signed-off-by: Richard Henderson <richard.henderson@linaro.org
>     <mailto:richard.henderson@linaro.org>>
>     ---
>      target/avr/translate.c | 234 ++++++++++++++++++++++-------------------
>      1 file changed, 128 insertions(+), 106 deletions(-)

Re: [PATCH v2 3/3] target/avr: Convert to TranslatorOps

[Michael Rolnik]

[-- Attachment #1: Type: text/plain, Size: 871 bytes --]

The whole series.

On Mon, Jun 21, 2021 at 11:33 AM Philippe Mathieu-Daudé <f4bug@amsat.org>
wrote:

> Hi Michael,
>
> On 6/21/21 7:38 AM, Michael Rolnik wrote:
> > Reviewed-by: Michael Rolnik <mrolnik@gmail.com <mailto:mrolnik@gmail.com
> >>
> > Tested-by: Michael Rolnik <mrolnik@gmail.com <mailto:mrolnik@gmail.com>>
>
> Are your tags valid for the latest patch or the whole series?
>
> > On Mon, Jun 21, 2021 at 12:50 AM Richard Henderson
> > <richard.henderson@linaro.org <mailto:richard.henderson@linaro.org>>
> wrote:
> >
> >     Signed-off-by: Richard Henderson <richard.henderson@linaro.org
> >     <mailto:richard.henderson@linaro.org>>
> >     ---
> >      target/avr/translate.c | 234
> ++++++++++++++++++++++-------------------
> >      1 file changed, 128 insertions(+), 106 deletions(-)
>
>

-- 
Best Regards,
Michael Rolnik

[-- Attachment #2: Type: text/html, Size: 1826 bytes --]