All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/7] target/nios2: Convert to TranslatorOps
@ 2021-06-20 22:10 Richard Henderson
  2021-06-20 22:10 ` [PATCH v2 1/7] target/nios2: Replace DISAS_TB_JUMP with DISAS_NORETURN Richard Henderson
                   ` (6 more replies)
  0 siblings, 7 replies; 17+ messages in thread
From: Richard Henderson @ 2021-06-20 22:10 UTC (permalink / raw)
  To: qemu-devel; +Cc: marex, crwulff

I've reached a point where *all* targets must use the translator loop.  
Do that, plus some other obvious cleanups.

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


r~


Richard Henderson (7):
  target/nios2: Replace DISAS_TB_JUMP with DISAS_NORETURN
  target/nios2: Use global cpu_env
  target/nios2: Use global cpu_R
  target/nios2: Add DisasContextBase to DisasContext
  target/nios2: Convert to TranslatorOps
  target/nios2: Remove assignment to env in handle_instruction
  target/nios2: Clean up goto in handle_instruction

 target/nios2/translate.c | 259 +++++++++++++++++++--------------------
 1 file changed, 127 insertions(+), 132 deletions(-)

-- 
2.25.1



^ permalink raw reply	[flat|nested] 17+ messages in thread
* [PATCH v2 1/7] target/nios2: Replace DISAS_TB_JUMP with DISAS_NORETURN
  2021-06-20 22:10 [PATCH v2 0/7] target/nios2: Convert to TranslatorOps Richard Henderson
@ 2021-06-20 22:10 ` Richard Henderson
  2021-06-28 15:14   ` Peter Maydell
  2021-06-20 22:10 ` [PATCH v2 2/7] target/nios2: Use global cpu_env Richard Henderson
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 17+ messages in thread
From: Richard Henderson @ 2021-06-20 22:10 UTC (permalink / raw)
  To: qemu-devel; +Cc: marex, crwulff

The only semantic of DISAS_TB_JUMP is that we've done goto_tb,
which is the same as DISAS_NORETURN -- we've exited the tb.

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

diff --git a/target/nios2/translate.c b/target/nios2/translate.c
index 399f22d938..388fae93a2 100644
--- a/target/nios2/translate.c
+++ b/target/nios2/translate.c
@@ -37,7 +37,6 @@
 /* is_jmp field values */
 #define DISAS_JUMP    DISAS_TARGET_0 /* only pc was modified dynamically */
 #define DISAS_UPDATE  DISAS_TARGET_1 /* cpu state was modified dynamically */
-#define DISAS_TB_JUMP DISAS_TARGET_2 /* only pc was modified statically */
 
 #define INSTRUCTION_FLG(func, flags) { (func), (flags) }
 #define INSTRUCTION(func)                  \
@@ -209,7 +208,7 @@ static void jmpi(DisasContext *dc, uint32_t code, uint32_t flags)
 {
     J_TYPE(instr, code);
     gen_goto_tb(dc, 0, (dc->pc & 0xF0000000) | (instr.imm26 << 2));
-    dc->is_jmp = DISAS_TB_JUMP;
+    dc->is_jmp = DISAS_NORETURN;
 }
 
 static void call(DisasContext *dc, uint32_t code, uint32_t flags)
@@ -269,7 +268,7 @@ static void br(DisasContext *dc, uint32_t code, uint32_t flags)
     I_TYPE(instr, code);
 
     gen_goto_tb(dc, 0, dc->pc + 4 + (instr.imm16.s & -4));
-    dc->is_jmp = DISAS_TB_JUMP;
+    dc->is_jmp = DISAS_NORETURN;
 }
 
 static void gen_bxx(DisasContext *dc, uint32_t code, uint32_t flags)
@@ -281,7 +280,7 @@ static void gen_bxx(DisasContext *dc, uint32_t code, uint32_t flags)
     gen_goto_tb(dc, 0, dc->pc + 4);
     gen_set_label(l1);
     gen_goto_tb(dc, 1, dc->pc + 4 + (instr.imm16.s & -4));
-    dc->is_jmp = DISAS_TB_JUMP;
+    dc->is_jmp = DISAS_NORETURN;
 }
 
 /* Comparison instructions */
@@ -883,7 +882,6 @@ void gen_intermediate_code(CPUState *cs, TranslationBlock *tb, int max_insns)
         break;
 
     case DISAS_NORETURN:
-    case DISAS_TB_JUMP:
         /* nothing more to generate */
         break;
     }
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 17+ messages in thread
* [PATCH v2 2/7] target/nios2: Use global cpu_env
  2021-06-20 22:10 [PATCH v2 0/7] target/nios2: Convert to TranslatorOps Richard Henderson
  2021-06-20 22:10 ` [PATCH v2 1/7] target/nios2: Replace DISAS_TB_JUMP with DISAS_NORETURN Richard Henderson
@ 2021-06-20 22:10 ` Richard Henderson
  2021-06-28 15:15   ` Peter Maydell
  2021-06-20 22:10 ` [PATCH v2 3/7] target/nios2: Use global cpu_R Richard Henderson
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 17+ messages in thread
From: Richard Henderson @ 2021-06-20 22:10 UTC (permalink / raw)
  To: qemu-devel; +Cc: marex, crwulff

We do not need to copy this into DisasContext.

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

diff --git a/target/nios2/translate.c b/target/nios2/translate.c
index 388fae93a2..39538e1870 100644
--- a/target/nios2/translate.c
+++ b/target/nios2/translate.c
@@ -97,7 +97,6 @@
     }
 
 typedef struct DisasContext {
-    TCGv_ptr          cpu_env;
     TCGv             *cpu_R;
     TCGv_i32          zero;
     int               is_jmp;
@@ -147,7 +146,7 @@ static void t_gen_helper_raise_exception(DisasContext *dc,
     TCGv_i32 tmp = tcg_const_i32(index);
 
     tcg_gen_movi_tl(dc->cpu_R[R_PC], dc->pc);
-    gen_helper_raise_exception(dc->cpu_env, tmp);
+    gen_helper_raise_exception(cpu_env, tmp);
     tcg_temp_free_i32(tmp);
     dc->is_jmp = DISAS_NORETURN;
 }
@@ -474,7 +473,7 @@ static void rdctl(DisasContext *dc, uint32_t code, uint32_t flags)
             tcg_gen_mov_tl(dc->cpu_R[instr.c], dc->cpu_R[instr.imm5 + CR_BASE]);
 #ifdef DEBUG_MMU
             TCGv_i32 tmp = tcg_const_i32(instr.imm5 + CR_BASE);
-            gen_helper_mmu_read_debug(dc->cpu_R[instr.c], dc->cpu_env, tmp);
+            gen_helper_mmu_read_debug(dc->cpu_R[instr.c], cpu_env, tmp);
             tcg_temp_free_i32(tmp);
 #endif
         }
@@ -504,7 +503,7 @@ static void wrctl(DisasContext *dc, uint32_t code, uint32_t flags)
     {
 #if !defined(CONFIG_USER_ONLY)
         TCGv_i32 tmp = tcg_const_i32(instr.imm5 + CR_BASE);
-        gen_helper_mmu_write(dc->cpu_env, tmp, load_gpr(dc, instr.a));
+        gen_helper_mmu_write(cpu_env, tmp, load_gpr(dc, instr.a));
         tcg_temp_free_i32(tmp);
 #endif
         break;
@@ -521,7 +520,7 @@ static void wrctl(DisasContext *dc, uint32_t code, uint32_t flags)
         if (tb_cflags(dc->tb) & CF_USE_ICOUNT) {
             gen_io_start();
         }
-        gen_helper_check_interrupts(dc->cpu_env);
+        gen_helper_check_interrupts(cpu_env);
         dc->is_jmp = DISAS_UPDATE;
     }
 #endif
@@ -817,7 +816,6 @@ void gen_intermediate_code(CPUState *cs, TranslationBlock *tb, int max_insns)
     int num_insns;
 
     /* Initialize DC */
-    dc->cpu_env = cpu_env;
     dc->cpu_R   = cpu_R;
     dc->is_jmp  = DISAS_NEXT;
     dc->pc      = tb->pc;
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 17+ messages in thread
* [PATCH v2 3/7] target/nios2: Use global cpu_R
  2021-06-20 22:10 [PATCH v2 0/7] target/nios2: Convert to TranslatorOps Richard Henderson
  2021-06-20 22:10 ` [PATCH v2 1/7] target/nios2: Replace DISAS_TB_JUMP with DISAS_NORETURN Richard Henderson
  2021-06-20 22:10 ` [PATCH v2 2/7] target/nios2: Use global cpu_env Richard Henderson
@ 2021-06-20 22:10 ` Richard Henderson
  2021-06-28 15:16   ` Peter Maydell
  2021-06-20 22:10 ` [PATCH v2 4/7] target/nios2: Add DisasContextBase to DisasContext Richard Henderson
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 17+ messages in thread
From: Richard Henderson @ 2021-06-20 22:10 UTC (permalink / raw)
  To: qemu-devel; +Cc: marex, crwulff

We do not need to copy this into DisasContext.

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

diff --git a/target/nios2/translate.c b/target/nios2/translate.c
index 39538e1870..6bdd388bd8 100644
--- a/target/nios2/translate.c
+++ b/target/nios2/translate.c
@@ -97,7 +97,6 @@
     }
 
 typedef struct DisasContext {
-    TCGv             *cpu_R;
     TCGv_i32          zero;
     int               is_jmp;
     target_ulong      pc;
@@ -106,6 +105,8 @@ typedef struct DisasContext {
     bool              singlestep_enabled;
 } DisasContext;
 
+static TCGv cpu_R[NUM_CORE_REGS];
+
 typedef struct Nios2Instruction {
     void     (*handler)(DisasContext *dc, uint32_t code, uint32_t flags);
     uint32_t  flags;
@@ -134,7 +135,7 @@ static TCGv load_zero(DisasContext *dc)
 static TCGv load_gpr(DisasContext *dc, uint8_t reg)
 {
     if (likely(reg != R_ZERO)) {
-        return dc->cpu_R[reg];
+        return cpu_R[reg];
     } else {
         return load_zero(dc);
     }
@@ -145,7 +146,7 @@ static void t_gen_helper_raise_exception(DisasContext *dc,
 {
     TCGv_i32 tmp = tcg_const_i32(index);
 
-    tcg_gen_movi_tl(dc->cpu_R[R_PC], dc->pc);
+    tcg_gen_movi_tl(cpu_R[R_PC], dc->pc);
     gen_helper_raise_exception(cpu_env, tmp);
     tcg_temp_free_i32(tmp);
     dc->is_jmp = DISAS_NORETURN;
@@ -170,10 +171,10 @@ static void gen_goto_tb(DisasContext *dc, int n, uint32_t dest)
 
     if (use_goto_tb(dc, dest)) {
         tcg_gen_goto_tb(n);
-        tcg_gen_movi_tl(dc->cpu_R[R_PC], dest);
+        tcg_gen_movi_tl(cpu_R[R_PC], dest);
         tcg_gen_exit_tb(tb, n);
     } else {
-        tcg_gen_movi_tl(dc->cpu_R[R_PC], dest);
+        tcg_gen_movi_tl(cpu_R[R_PC], dest);
         tcg_gen_exit_tb(NULL, 0);
     }
 }
@@ -212,7 +213,7 @@ static void jmpi(DisasContext *dc, uint32_t code, uint32_t flags)
 
 static void call(DisasContext *dc, uint32_t code, uint32_t flags)
 {
-    tcg_gen_movi_tl(dc->cpu_R[R_RA], dc->pc + 4);
+    tcg_gen_movi_tl(cpu_R[R_RA], dc->pc + 4);
     jmpi(dc, code, flags);
 }
 
@@ -234,7 +235,7 @@ static void gen_ldx(DisasContext *dc, uint32_t code, uint32_t flags)
      *          the Nios2 CPU.
      */
     if (likely(instr.b != R_ZERO)) {
-        data = dc->cpu_R[instr.b];
+        data = cpu_R[instr.b];
     } else {
         data = tcg_temp_new();
     }
@@ -275,7 +276,7 @@ static void gen_bxx(DisasContext *dc, uint32_t code, uint32_t flags)
     I_TYPE(instr, code);
 
     TCGLabel *l1 = gen_new_label();
-    tcg_gen_brcond_tl(flags, dc->cpu_R[instr.a], dc->cpu_R[instr.b], l1);
+    tcg_gen_brcond_tl(flags, cpu_R[instr.a], cpu_R[instr.b], l1);
     gen_goto_tb(dc, 0, dc->pc + 4);
     gen_set_label(l1);
     gen_goto_tb(dc, 1, dc->pc + 4 + (instr.imm16.s & -4));
@@ -287,8 +288,7 @@ static void gen_bxx(DisasContext *dc, uint32_t code, uint32_t flags)
 static void (fname)(DisasContext *dc, uint32_t code, uint32_t flags)         \
 {                                                                            \
     I_TYPE(instr, (code));                                                   \
-    tcg_gen_setcondi_tl(flags, (dc)->cpu_R[instr.b], (dc)->cpu_R[instr.a],   \
-                        (op3));                                              \
+    tcg_gen_setcondi_tl(flags, cpu_R[instr.b], cpu_R[instr.a], (op3));       \
 }
 
 gen_i_cmpxx(gen_cmpxxsi, instr.imm16.s)
@@ -302,10 +302,9 @@ static void (fname)(DisasContext *dc, uint32_t code, uint32_t flags)        \
     if (unlikely(instr.b == R_ZERO)) { /* Store to R_ZERO is ignored */     \
         return;                                                             \
     } else if (instr.a == R_ZERO) { /* MOVxI optimizations */               \
-        tcg_gen_movi_tl(dc->cpu_R[instr.b], (resimm) ? (op3) : 0);          \
+        tcg_gen_movi_tl(cpu_R[instr.b], (resimm) ? (op3) : 0);              \
     } else {                                                                \
-        tcg_gen_##insn##_tl((dc)->cpu_R[instr.b], (dc)->cpu_R[instr.a],     \
-                            (op3));                                         \
+        tcg_gen_##insn##_tl(cpu_R[instr.b], cpu_R[instr.a], (op3));         \
     }                                                                       \
 }
 
@@ -400,8 +399,8 @@ static const Nios2Instruction i_type_instructions[] = {
  */
 static void eret(DisasContext *dc, uint32_t code, uint32_t flags)
 {
-    tcg_gen_mov_tl(dc->cpu_R[CR_STATUS], dc->cpu_R[CR_ESTATUS]);
-    tcg_gen_mov_tl(dc->cpu_R[R_PC], dc->cpu_R[R_EA]);
+    tcg_gen_mov_tl(cpu_R[CR_STATUS], cpu_R[CR_ESTATUS]);
+    tcg_gen_mov_tl(cpu_R[R_PC], cpu_R[R_EA]);
 
     dc->is_jmp = DISAS_JUMP;
 }
@@ -409,7 +408,7 @@ static void eret(DisasContext *dc, uint32_t code, uint32_t flags)
 /* PC <- ra */
 static void ret(DisasContext *dc, uint32_t code, uint32_t flags)
 {
-    tcg_gen_mov_tl(dc->cpu_R[R_PC], dc->cpu_R[R_RA]);
+    tcg_gen_mov_tl(cpu_R[R_PC], cpu_R[R_RA]);
 
     dc->is_jmp = DISAS_JUMP;
 }
@@ -417,7 +416,7 @@ static void ret(DisasContext *dc, uint32_t code, uint32_t flags)
 /* PC <- ba */
 static void bret(DisasContext *dc, uint32_t code, uint32_t flags)
 {
-    tcg_gen_mov_tl(dc->cpu_R[R_PC], dc->cpu_R[R_BA]);
+    tcg_gen_mov_tl(cpu_R[R_PC], cpu_R[R_BA]);
 
     dc->is_jmp = DISAS_JUMP;
 }
@@ -427,7 +426,7 @@ static void jmp(DisasContext *dc, uint32_t code, uint32_t flags)
 {
     R_TYPE(instr, code);
 
-    tcg_gen_mov_tl(dc->cpu_R[R_PC], load_gpr(dc, instr.a));
+    tcg_gen_mov_tl(cpu_R[R_PC], load_gpr(dc, instr.a));
 
     dc->is_jmp = DISAS_JUMP;
 }
@@ -438,7 +437,7 @@ static void nextpc(DisasContext *dc, uint32_t code, uint32_t flags)
     R_TYPE(instr, code);
 
     if (likely(instr.c != R_ZERO)) {
-        tcg_gen_movi_tl(dc->cpu_R[instr.c], dc->pc + 4);
+        tcg_gen_movi_tl(cpu_R[instr.c], dc->pc + 4);
     }
 }
 
@@ -450,8 +449,8 @@ static void callr(DisasContext *dc, uint32_t code, uint32_t flags)
 {
     R_TYPE(instr, code);
 
-    tcg_gen_mov_tl(dc->cpu_R[R_PC], load_gpr(dc, instr.a));
-    tcg_gen_movi_tl(dc->cpu_R[R_RA], dc->pc + 4);
+    tcg_gen_mov_tl(cpu_R[R_PC], load_gpr(dc, instr.a));
+    tcg_gen_movi_tl(cpu_R[R_RA], dc->pc + 4);
 
     dc->is_jmp = DISAS_JUMP;
 }
@@ -470,10 +469,10 @@ static void rdctl(DisasContext *dc, uint32_t code, uint32_t flags)
     {
 #if !defined(CONFIG_USER_ONLY)
         if (likely(instr.c != R_ZERO)) {
-            tcg_gen_mov_tl(dc->cpu_R[instr.c], dc->cpu_R[instr.imm5 + CR_BASE]);
+            tcg_gen_mov_tl(cpu_R[instr.c], cpu_R[instr.imm5 + CR_BASE]);
 #ifdef DEBUG_MMU
             TCGv_i32 tmp = tcg_const_i32(instr.imm5 + CR_BASE);
-            gen_helper_mmu_read_debug(dc->cpu_R[instr.c], cpu_env, tmp);
+            gen_helper_mmu_read_debug(cpu_R[instr.c], cpu_env, tmp);
             tcg_temp_free_i32(tmp);
 #endif
         }
@@ -483,7 +482,7 @@ static void rdctl(DisasContext *dc, uint32_t code, uint32_t flags)
 
     default:
         if (likely(instr.c != R_ZERO)) {
-            tcg_gen_mov_tl(dc->cpu_R[instr.c], dc->cpu_R[instr.imm5 + CR_BASE]);
+            tcg_gen_mov_tl(cpu_R[instr.c], cpu_R[instr.imm5 + CR_BASE]);
         }
         break;
     }
@@ -510,7 +509,7 @@ static void wrctl(DisasContext *dc, uint32_t code, uint32_t flags)
     }
 
     default:
-        tcg_gen_mov_tl(dc->cpu_R[instr.imm5 + CR_BASE], load_gpr(dc, instr.a));
+        tcg_gen_mov_tl(cpu_R[instr.imm5 + CR_BASE], load_gpr(dc, instr.a));
         break;
     }
 
@@ -531,8 +530,8 @@ static void gen_cmpxx(DisasContext *dc, uint32_t code, uint32_t flags)
 {
     R_TYPE(instr, code);
     if (likely(instr.c != R_ZERO)) {
-        tcg_gen_setcond_tl(flags, dc->cpu_R[instr.c], dc->cpu_R[instr.a],
-                           dc->cpu_R[instr.b]);
+        tcg_gen_setcond_tl(flags, cpu_R[instr.c], cpu_R[instr.a],
+                           cpu_R[instr.b]);
     }
 }
 
@@ -542,8 +541,7 @@ static void (fname)(DisasContext *dc, uint32_t code, uint32_t flags)       \
 {                                                                          \
     R_TYPE(instr, (code));                                                 \
     if (likely(instr.c != R_ZERO)) {                                       \
-        tcg_gen_##insn((dc)->cpu_R[instr.c], load_gpr((dc), instr.a),      \
-                       (op3));                                             \
+        tcg_gen_##insn(cpu_R[instr.c], load_gpr((dc), instr.a), (op3));    \
     }                                                                      \
 }
 
@@ -567,8 +565,8 @@ static void (fname)(DisasContext *dc, uint32_t code, uint32_t flags)   \
     R_TYPE(instr, (code));                                             \
     if (likely(instr.c != R_ZERO)) {                                   \
         TCGv t0 = tcg_temp_new();                                      \
-        tcg_gen_##insn(t0, dc->cpu_R[instr.c],                         \
-                       load_gpr(dc, instr.a), load_gpr(dc, instr.b)); \
+        tcg_gen_##insn(t0, cpu_R[instr.c],                             \
+                       load_gpr(dc, instr.a), load_gpr(dc, instr.b));  \
         tcg_temp_free(t0);                                             \
     }                                                                  \
 }
@@ -584,7 +582,7 @@ static void (fname)(DisasContext *dc, uint32_t code, uint32_t flags)       \
     if (likely(instr.c != R_ZERO)) {                                       \
         TCGv t0 = tcg_temp_new();                                          \
         tcg_gen_andi_tl(t0, load_gpr((dc), instr.b), 31);                  \
-        tcg_gen_##insn((dc)->cpu_R[instr.c], load_gpr((dc), instr.a), t0); \
+        tcg_gen_##insn(cpu_R[instr.c], load_gpr((dc), instr.a), t0);       \
         tcg_temp_free(t0);                                                 \
     }                                                                      \
 }
@@ -618,8 +616,8 @@ static void divs(DisasContext *dc, uint32_t code, uint32_t flags)
     tcg_gen_or_tl(t2, t2, t3);
     tcg_gen_movi_tl(t3, 0);
     tcg_gen_movcond_tl(TCG_COND_NE, t1, t2, t3, t2, t1);
-    tcg_gen_div_tl(dc->cpu_R[instr.c], t0, t1);
-    tcg_gen_ext32s_tl(dc->cpu_R[instr.c], dc->cpu_R[instr.c]);
+    tcg_gen_div_tl(cpu_R[instr.c], t0, t1);
+    tcg_gen_ext32s_tl(cpu_R[instr.c], cpu_R[instr.c]);
 
     tcg_temp_free(t3);
     tcg_temp_free(t2);
@@ -644,8 +642,8 @@ static void divu(DisasContext *dc, uint32_t code, uint32_t flags)
     tcg_gen_ext32u_tl(t0, load_gpr(dc, instr.a));
     tcg_gen_ext32u_tl(t1, load_gpr(dc, instr.b));
     tcg_gen_movcond_tl(TCG_COND_EQ, t1, t1, t2, t3, t1);
-    tcg_gen_divu_tl(dc->cpu_R[instr.c], t0, t1);
-    tcg_gen_ext32s_tl(dc->cpu_R[instr.c], dc->cpu_R[instr.c]);
+    tcg_gen_divu_tl(cpu_R[instr.c], t0, t1);
+    tcg_gen_ext32s_tl(cpu_R[instr.c], cpu_R[instr.c]);
 
     tcg_temp_free(t3);
     tcg_temp_free(t2);
@@ -794,8 +792,6 @@ static const char * const regnames[] = {
     "rpc"
 };
 
-static TCGv cpu_R[NUM_CORE_REGS];
-
 #include "exec/gen-icount.h"
 
 static void gen_exception(DisasContext *dc, uint32_t excp)
@@ -816,7 +812,6 @@ void gen_intermediate_code(CPUState *cs, TranslationBlock *tb, int max_insns)
     int num_insns;
 
     /* Initialize DC */
-    dc->cpu_R   = cpu_R;
     dc->is_jmp  = DISAS_NEXT;
     dc->pc      = tb->pc;
     dc->tb      = tb;
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 17+ messages in thread
* [PATCH v2 4/7] target/nios2: Add DisasContextBase to DisasContext
  2021-06-20 22:10 [PATCH v2 0/7] target/nios2: Convert to TranslatorOps Richard Henderson
                   ` (2 preceding siblings ...)
  2021-06-20 22:10 ` [PATCH v2 3/7] target/nios2: Use global cpu_R Richard Henderson
@ 2021-06-20 22:10 ` Richard Henderson
  2021-06-28 15:28   ` Peter Maydell
  2021-06-20 22:10 ` [PATCH v2 5/7] target/nios2: Convert to TranslatorOps Richard Henderson
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 17+ messages in thread
From: Richard Henderson @ 2021-06-20 22:10 UTC (permalink / raw)
  To: qemu-devel; +Cc: marex, crwulff

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

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

diff --git a/target/nios2/translate.c b/target/nios2/translate.c
index 6bdd388bd8..31653b7912 100644
--- a/target/nios2/translate.c
+++ b/target/nios2/translate.c
@@ -97,12 +97,10 @@
     }
 
 typedef struct DisasContext {
+    DisasContextBase  base;
     TCGv_i32          zero;
-    int               is_jmp;
     target_ulong      pc;
-    TranslationBlock *tb;
     int               mem_idx;
-    bool              singlestep_enabled;
 } DisasContext;
 
 static TCGv cpu_R[NUM_CORE_REGS];
@@ -149,17 +147,17 @@ static void t_gen_helper_raise_exception(DisasContext *dc,
     tcg_gen_movi_tl(cpu_R[R_PC], dc->pc);
     gen_helper_raise_exception(cpu_env, tmp);
     tcg_temp_free_i32(tmp);
-    dc->is_jmp = DISAS_NORETURN;
+    dc->base.is_jmp = DISAS_NORETURN;
 }
 
 static bool use_goto_tb(DisasContext *dc, uint32_t dest)
 {
-    if (unlikely(dc->singlestep_enabled)) {
+    if (unlikely(dc->base.singlestep_enabled)) {
         return false;
     }
 
 #ifndef CONFIG_USER_ONLY
-    return (dc->tb->pc & TARGET_PAGE_MASK) == (dest & TARGET_PAGE_MASK);
+    return (dc->base.pc_first & TARGET_PAGE_MASK) == (dest & TARGET_PAGE_MASK);
 #else
     return true;
 #endif
@@ -167,7 +165,7 @@ static bool use_goto_tb(DisasContext *dc, uint32_t dest)
 
 static void gen_goto_tb(DisasContext *dc, int n, uint32_t dest)
 {
-    TranslationBlock *tb = dc->tb;
+    const TranslationBlock *tb = dc->base.tb;
 
     if (use_goto_tb(dc, dest)) {
         tcg_gen_goto_tb(n);
@@ -186,7 +184,7 @@ static void gen_excp(DisasContext *dc, uint32_t code, uint32_t flags)
 
 static void gen_check_supervisor(DisasContext *dc)
 {
-    if (dc->tb->flags & CR_STATUS_U) {
+    if (dc->base.tb->flags & CR_STATUS_U) {
         /* CPU in user mode, privileged instruction called, stop. */
         t_gen_helper_raise_exception(dc, EXCP_SUPERI);
     }
@@ -208,7 +206,7 @@ static void jmpi(DisasContext *dc, uint32_t code, uint32_t flags)
 {
     J_TYPE(instr, code);
     gen_goto_tb(dc, 0, (dc->pc & 0xF0000000) | (instr.imm26 << 2));
-    dc->is_jmp = DISAS_NORETURN;
+    dc->base.is_jmp = DISAS_NORETURN;
 }
 
 static void call(DisasContext *dc, uint32_t code, uint32_t flags)
@@ -268,7 +266,7 @@ static void br(DisasContext *dc, uint32_t code, uint32_t flags)
     I_TYPE(instr, code);
 
     gen_goto_tb(dc, 0, dc->pc + 4 + (instr.imm16.s & -4));
-    dc->is_jmp = DISAS_NORETURN;
+    dc->base.is_jmp = DISAS_NORETURN;
 }
 
 static void gen_bxx(DisasContext *dc, uint32_t code, uint32_t flags)
@@ -280,7 +278,7 @@ static void gen_bxx(DisasContext *dc, uint32_t code, uint32_t flags)
     gen_goto_tb(dc, 0, dc->pc + 4);
     gen_set_label(l1);
     gen_goto_tb(dc, 1, dc->pc + 4 + (instr.imm16.s & -4));
-    dc->is_jmp = DISAS_NORETURN;
+    dc->base.is_jmp = DISAS_NORETURN;
 }
 
 /* Comparison instructions */
@@ -402,7 +400,7 @@ static void eret(DisasContext *dc, uint32_t code, uint32_t flags)
     tcg_gen_mov_tl(cpu_R[CR_STATUS], cpu_R[CR_ESTATUS]);
     tcg_gen_mov_tl(cpu_R[R_PC], cpu_R[R_EA]);
 
-    dc->is_jmp = DISAS_JUMP;
+    dc->base.is_jmp = DISAS_JUMP;
 }
 
 /* PC <- ra */
@@ -410,7 +408,7 @@ static void ret(DisasContext *dc, uint32_t code, uint32_t flags)
 {
     tcg_gen_mov_tl(cpu_R[R_PC], cpu_R[R_RA]);
 
-    dc->is_jmp = DISAS_JUMP;
+    dc->base.is_jmp = DISAS_JUMP;
 }
 
 /* PC <- ba */
@@ -418,7 +416,7 @@ static void bret(DisasContext *dc, uint32_t code, uint32_t flags)
 {
     tcg_gen_mov_tl(cpu_R[R_PC], cpu_R[R_BA]);
 
-    dc->is_jmp = DISAS_JUMP;
+    dc->base.is_jmp = DISAS_JUMP;
 }
 
 /* PC <- rA */
@@ -428,7 +426,7 @@ static void jmp(DisasContext *dc, uint32_t code, uint32_t flags)
 
     tcg_gen_mov_tl(cpu_R[R_PC], load_gpr(dc, instr.a));
 
-    dc->is_jmp = DISAS_JUMP;
+    dc->base.is_jmp = DISAS_JUMP;
 }
 
 /* rC <- PC + 4 */
@@ -452,7 +450,7 @@ static void callr(DisasContext *dc, uint32_t code, uint32_t flags)
     tcg_gen_mov_tl(cpu_R[R_PC], load_gpr(dc, instr.a));
     tcg_gen_movi_tl(cpu_R[R_RA], dc->pc + 4);
 
-    dc->is_jmp = DISAS_JUMP;
+    dc->base.is_jmp = DISAS_JUMP;
 }
 
 /* rC <- ctlN */
@@ -516,11 +514,11 @@ static void wrctl(DisasContext *dc, uint32_t code, uint32_t flags)
     /* If interrupts were enabled using WRCTL, trigger them. */
 #if !defined(CONFIG_USER_ONLY)
     if ((instr.imm5 + CR_BASE) == CR_STATUS) {
-        if (tb_cflags(dc->tb) & CF_USE_ICOUNT) {
+        if (tb_cflags(dc->base.tb) & CF_USE_ICOUNT) {
             gen_io_start();
         }
         gen_helper_check_interrupts(cpu_env);
-        dc->is_jmp = DISAS_UPDATE;
+        dc->base.is_jmp = DISAS_UPDATE;
     }
 #endif
 }
@@ -801,7 +799,7 @@ static void gen_exception(DisasContext *dc, uint32_t excp)
     tcg_gen_movi_tl(cpu_R[R_PC], dc->pc);
     gen_helper_raise_exception(cpu_env, tmp);
     tcg_temp_free_i32(tmp);
-    dc->is_jmp = DISAS_NORETURN;
+    dc->base.is_jmp = DISAS_NORETURN;
 }
 
 /* generate intermediate code for basic block 'tb'.  */
@@ -812,11 +810,16 @@ void gen_intermediate_code(CPUState *cs, TranslationBlock *tb, int max_insns)
     int num_insns;
 
     /* Initialize DC */
-    dc->is_jmp  = DISAS_NEXT;
+
+    dc->base.tb = tb;
+    dc->base.singlestep_enabled = cs->singlestep_enabled;
+    dc->base.is_jmp = DISAS_NEXT;
+    dc->base.pc_first = tb->pc;
+    dc->base.pc_next = tb->pc;
+
+    dc->zero    = NULL;
     dc->pc      = tb->pc;
-    dc->tb      = tb;
     dc->mem_idx = cpu_mmu_index(env, false);
-    dc->singlestep_enabled = cs->singlestep_enabled;
 
     /* Set up instruction counts */
     num_insns = 0;
@@ -855,12 +858,12 @@ void gen_intermediate_code(CPUState *cs, TranslationBlock *tb, int max_insns)
          * 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.  */
-    } while (!dc->is_jmp &&
+    } while (!dc->base.is_jmp &&
              !tcg_op_buf_full() &&
              num_insns < max_insns);
 
     /* Indicate where the next block should start */
-    switch (dc->is_jmp) {
+    switch (dc->base.is_jmp) {
     case DISAS_NEXT:
     case DISAS_UPDATE:
         /* Save the current PC back into the CPU register */
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 17+ messages in thread
* [PATCH v2 5/7] target/nios2: Convert to TranslatorOps
  2021-06-20 22:10 [PATCH v2 0/7] target/nios2: Convert to TranslatorOps Richard Henderson
                   ` (3 preceding siblings ...)
  2021-06-20 22:10 ` [PATCH v2 4/7] target/nios2: Add DisasContextBase to DisasContext Richard Henderson
@ 2021-06-20 22:10 ` Richard Henderson
  2021-06-28 15:57   ` Peter Maydell
  2021-06-20 22:10 ` [PATCH v2 6/7] target/nios2: Remove assignment to env in handle_instruction Richard Henderson
  2021-06-20 22:10 ` [PATCH v2 7/7] target/nios2: Clean up goto " Richard Henderson
  6 siblings, 1 reply; 17+ messages in thread
From: Richard Henderson @ 2021-06-20 22:10 UTC (permalink / raw)
  To: qemu-devel; +Cc: marex, crwulff

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

diff --git a/target/nios2/translate.c b/target/nios2/translate.c
index 31653b7912..06705c894d 100644
--- a/target/nios2/translate.c
+++ b/target/nios2/translate.c
@@ -803,75 +803,72 @@ static void gen_exception(DisasContext *dc, uint32_t excp)
 }
 
 /* generate intermediate code for basic block 'tb'.  */
-void gen_intermediate_code(CPUState *cs, TranslationBlock *tb, int max_insns)
+static void nios2_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
 {
+    DisasContext *dc = container_of(dcbase, DisasContext, base);
     CPUNios2State *env = cs->env_ptr;
-    DisasContext dc1, *dc = &dc1;
-    int num_insns;
-
-    /* Initialize DC */
-
-    dc->base.tb = tb;
-    dc->base.singlestep_enabled = cs->singlestep_enabled;
-    dc->base.is_jmp = DISAS_NEXT;
-    dc->base.pc_first = tb->pc;
-    dc->base.pc_next = tb->pc;
+    target_ulong pc = dc->base.pc_first;
+    int page_insns;
 
     dc->zero    = NULL;
-    dc->pc      = tb->pc;
+    dc->pc      = pc;
     dc->mem_idx = cpu_mmu_index(env, false);
 
-    /* Set up instruction counts */
-    num_insns = 0;
-    if (max_insns > 1) {
-        int page_insns = (TARGET_PAGE_SIZE - (tb->pc & ~TARGET_PAGE_MASK)) / 4;
-        if (max_insns > page_insns) {
-            max_insns = page_insns;
-        }
-    }
+    /* Bound the number of insns to execute to those left on the page.  */
+    page_insns = -(pc | TARGET_PAGE_MASK) / 4;
+    dc->base.max_insns = MIN(page_insns, dc->base.max_insns);
+}
 
-    gen_tb_start(tb);
-    do {
-        tcg_gen_insn_start(dc->pc);
-        num_insns++;
+static void nios2_tr_tb_start(DisasContextBase *db, CPUState *cs)
+{
+}
 
-        if (unlikely(cpu_breakpoint_test(cs, dc->pc, BP_ANY))) {
-            gen_exception(dc, EXCP_DEBUG);
-            /* The address covered by the breakpoint must be included in
-               [tb->pc, tb->pc + tb->size) in order to for it to be
-               properly cleared -- thus we increment the PC here so that
-               the logic setting tb->size below does the right thing.  */
-            dc->pc += 4;
-            break;
-        }
+static void nios2_tr_insn_start(DisasContextBase *dcbase, CPUState *cs)
+{
+    tcg_gen_insn_start(dcbase->pc_next);
+}
 
-        if (num_insns == max_insns && (tb_cflags(tb) & CF_LAST_IO)) {
-            gen_io_start();
-        }
+static bool nios2_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cs,
+                                      const CPUBreakpoint *bp)
+{
+    DisasContext *dc = container_of(dcbase, DisasContext, base);
 
-        /* Decode an instruction */
-        handle_instruction(dc, env);
+    gen_exception(dc, EXCP_DEBUG);
+    /*
+     * The address covered by the breakpoint must be included in
+     * [tb->pc, tb->pc + tb->size) in order to for it to be
+     * properly cleared -- thus we increment the PC here so that
+     * the logic setting tb->size below does the right thing.
+     */
+    dc->pc += 4;
+    return true;
+}
 
-        dc->pc += 4;
+static void nios2_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs)
+{
+    DisasContext *dc = container_of(dcbase, DisasContext, base);
+    CPUNios2State *env = cs->env_ptr;
 
-        /* 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.  */
-    } while (!dc->base.is_jmp &&
-             !tcg_op_buf_full() &&
-             num_insns < max_insns);
+    /* Decode an instruction */
+    handle_instruction(dc, env);
+
+    dc->base.pc_next += 4;
+    dc->pc += 4;
+}
+
+static void nios2_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs)
+{
+    DisasContext *dc = container_of(dcbase, DisasContext, base);
 
     /* Indicate where the next block should start */
     switch (dc->base.is_jmp) {
-    case DISAS_NEXT:
+    case DISAS_TOO_MANY:
     case DISAS_UPDATE:
         /* Save the current PC back into the CPU register */
         tcg_gen_movi_tl(cpu_R[R_PC], dc->pc);
         tcg_gen_exit_tb(NULL, 0);
         break;
 
-    default:
     case DISAS_JUMP:
         /* The jump will already have updated the PC register */
         tcg_gen_exit_tb(NULL, 0);
@@ -880,25 +877,32 @@ void gen_intermediate_code(CPUState *cs, TranslationBlock *tb, int max_insns)
     case DISAS_NORETURN:
         /* nothing more to generate */
         break;
+
+    default:
+        g_assert_not_reached();
     }
+}
 
-    /* End off the block */
-    gen_tb_end(tb, num_insns);
+static void nios2_tr_disas_log(const DisasContextBase *dcbase, CPUState *cpu)
+{
+    qemu_log("IN: %s\n", lookup_symbol(dcbase->pc_first));
+    log_target_disas(cpu, dcbase->pc_first, dcbase->tb->size);
+}
 
-    /* Mark instruction starts for the final generated instruction */
-    tb->size = dc->pc - tb->pc;
-    tb->icount = num_insns;
+static const TranslatorOps nios2_tr_ops = {
+    .init_disas_context = nios2_tr_init_disas_context,
+    .tb_start           = nios2_tr_tb_start,
+    .insn_start         = nios2_tr_insn_start,
+    .breakpoint_check   = nios2_tr_breakpoint_check,
+    .translate_insn     = nios2_tr_translate_insn,
+    .tb_stop            = nios2_tr_tb_stop,
+    .disas_log          = nios2_tr_disas_log,
+};
 
-#ifdef DEBUG_DISAS
-    if (qemu_loglevel_mask(CPU_LOG_TB_IN_ASM)
-        && qemu_log_in_addr_range(tb->pc)) {
-        FILE *logfile = qemu_log_lock();
-        qemu_log("IN: %s\n", lookup_symbol(tb->pc));
-        log_target_disas(cs, tb->pc, dc->pc - tb->pc);
-        qemu_log("\n");
-        qemu_log_unlock(logfile);
-    }
-#endif
+void gen_intermediate_code(CPUState *cs, TranslationBlock *tb, int max_insns)
+{
+    DisasContext dc;
+    translator_loop(&nios2_tr_ops, &dc.base, cs, tb, max_insns);
 }
 
 void nios2_cpu_dump_state(CPUState *cs, FILE *f, int flags)
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 17+ messages in thread
* [PATCH v2 6/7] target/nios2: Remove assignment to env in handle_instruction
  2021-06-20 22:10 [PATCH v2 0/7] target/nios2: Convert to TranslatorOps Richard Henderson
                   ` (4 preceding siblings ...)
  2021-06-20 22:10 ` [PATCH v2 5/7] target/nios2: Convert to TranslatorOps Richard Henderson
@ 2021-06-20 22:10 ` Richard Henderson
  2021-06-28 15:31   ` Peter Maydell
  2021-06-20 22:10 ` [PATCH v2 7/7] target/nios2: Clean up goto " Richard Henderson
  6 siblings, 1 reply; 17+ messages in thread
From: Richard Henderson @ 2021-06-20 22:10 UTC (permalink / raw)
  To: qemu-devel; +Cc: marex, crwulff

Direct assignments to env during translation do not work.

As it happens, the only way we can get here is if env->pc
is already set to dc->pc.  We will trap on the first insn
we execute anywhere on the page.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/nios2/translate.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/nios2/translate.c b/target/nios2/translate.c
index 06705c894d..31f63d9faa 100644
--- a/target/nios2/translate.c
+++ b/target/nios2/translate.c
@@ -740,14 +740,15 @@ static void handle_instruction(DisasContext *dc, CPUNios2State *env)
     uint32_t code;
     uint8_t op;
     const Nios2Instruction *instr;
+
 #if defined(CONFIG_USER_ONLY)
     /* FIXME: Is this needed ? */
     if (dc->pc >= 0x1000 && dc->pc < 0x2000) {
-        env->regs[R_PC] = dc->pc;
         t_gen_helper_raise_exception(dc, 0xaa);
         return;
     }
 #endif
+
     code = cpu_ldl_code(env, dc->pc);
     op = get_opcode(code);
 
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 17+ messages in thread
* [PATCH v2 7/7] target/nios2: Clean up goto in handle_instruction
  2021-06-20 22:10 [PATCH v2 0/7] target/nios2: Convert to TranslatorOps Richard Henderson
                   ` (5 preceding siblings ...)
  2021-06-20 22:10 ` [PATCH v2 6/7] target/nios2: Remove assignment to env in handle_instruction Richard Henderson
@ 2021-06-20 22:10 ` Richard Henderson
  2021-06-28 15:33   ` Peter Maydell
  6 siblings, 1 reply; 17+ messages in thread
From: Richard Henderson @ 2021-06-20 22:10 UTC (permalink / raw)
  To: qemu-devel; +Cc: marex, crwulff

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

diff --git a/target/nios2/translate.c b/target/nios2/translate.c
index 31f63d9faa..276643cee0 100644
--- a/target/nios2/translate.c
+++ b/target/nios2/translate.c
@@ -753,7 +753,8 @@ static void handle_instruction(DisasContext *dc, CPUNios2State *env)
     op = get_opcode(code);
 
     if (unlikely(op >= ARRAY_SIZE(i_type_instructions))) {
-        goto illegal_op;
+        t_gen_helper_raise_exception(dc, EXCP_ILLEGAL);
+        return;
     }
 
     dc->zero = NULL;
@@ -764,11 +765,6 @@ static void handle_instruction(DisasContext *dc, CPUNios2State *env)
     if (dc->zero) {
         tcg_temp_free(dc->zero);
     }
-
-    return;
-
-illegal_op:
-    t_gen_helper_raise_exception(dc, EXCP_ILLEGAL);
 }
 
 static const char * const regnames[] = {
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/7] target/nios2: Replace DISAS_TB_JUMP with DISAS_NORETURN
  2021-06-20 22:10 ` [PATCH v2 1/7] target/nios2: Replace DISAS_TB_JUMP with DISAS_NORETURN Richard Henderson
@ 2021-06-28 15:14   ` Peter Maydell
  0 siblings, 0 replies; 17+ messages in thread
From: Peter Maydell @ 2021-06-28 15:14 UTC (permalink / raw)
  To: Richard Henderson; +Cc: Marek Vasut, Chris Wulff, QEMU Developers

On Sun, 20 Jun 2021 at 23:15, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> The only semantic of DISAS_TB_JUMP is that we've done goto_tb,
> which is the same as DISAS_NORETURN -- we've exited the tb.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM


^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/7] target/nios2: Use global cpu_env
  2021-06-20 22:10 ` [PATCH v2 2/7] target/nios2: Use global cpu_env Richard Henderson
@ 2021-06-28 15:15   ` Peter Maydell
  0 siblings, 0 replies; 17+ messages in thread
From: Peter Maydell @ 2021-06-28 15:15 UTC (permalink / raw)
  To: Richard Henderson; +Cc: Marek Vasut, Chris Wulff, QEMU Developers

On Sun, 20 Jun 2021 at 23:16, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> We do not need to copy this into DisasContext.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/nios2/translate.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)

just replied to the v1 version of this mail by mistake, so:

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM


^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH v2 3/7] target/nios2: Use global cpu_R
  2021-06-20 22:10 ` [PATCH v2 3/7] target/nios2: Use global cpu_R Richard Henderson
@ 2021-06-28 15:16   ` Peter Maydell
  0 siblings, 0 replies; 17+ messages in thread
From: Peter Maydell @ 2021-06-28 15:16 UTC (permalink / raw)
  To: Richard Henderson; +Cc: Marek Vasut, Chris Wulff, QEMU Developers

On Sun, 20 Jun 2021 at 23:13, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> We do not need to copy this into DisasContext.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM


^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH v2 4/7] target/nios2: Add DisasContextBase to DisasContext
  2021-06-20 22:10 ` [PATCH v2 4/7] target/nios2: Add DisasContextBase to DisasContext Richard Henderson
@ 2021-06-28 15:28   ` Peter Maydell
  0 siblings, 0 replies; 17+ messages in thread
From: Peter Maydell @ 2021-06-28 15:28 UTC (permalink / raw)
  To: Richard Henderson; +Cc: Marek Vasut, Chris Wulff, QEMU Developers

On Sun, 20 Jun 2021 at 23:13, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Migrate the is_jmp, tb and singlestep_enabled fields
> from DisasContext into the base.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/nios2/translate.c | 51 +++++++++++++++++++++-------------------
>  1 file changed, 27 insertions(+), 24 deletions(-)

Bah, clicked on the v1 email again. Comments from over there apply
but anyway
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM


^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH v2 6/7] target/nios2: Remove assignment to env in handle_instruction
  2021-06-20 22:10 ` [PATCH v2 6/7] target/nios2: Remove assignment to env in handle_instruction Richard Henderson
@ 2021-06-28 15:31   ` Peter Maydell
  2021-06-28 15:58     ` Richard Henderson
  0 siblings, 1 reply; 17+ messages in thread
From: Peter Maydell @ 2021-06-28 15:31 UTC (permalink / raw)
  To: Richard Henderson; +Cc: Marek Vasut, Chris Wulff, QEMU Developers

On Sun, 20 Jun 2021 at 23:15, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Direct assignments to env during translation do not work.
>
> As it happens, the only way we can get here is if env->pc
> is already set to dc->pc.

More to the point, t_gen_helper_raise_exception() does
    tcg_gen_movi_tl(dc->cpu_R[R_PC], dc->pc);
before raising the exception (as you would expect it to).

> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/nios2/translate.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/target/nios2/translate.c b/target/nios2/translate.c
> index 06705c894d..31f63d9faa 100644
> --- a/target/nios2/translate.c
> +++ b/target/nios2/translate.c
> @@ -740,14 +740,15 @@ static void handle_instruction(DisasContext *dc, CPUNios2State *env)
>      uint32_t code;
>      uint8_t op;
>      const Nios2Instruction *instr;
> +
>  #if defined(CONFIG_USER_ONLY)
>      /* FIXME: Is this needed ? */
>      if (dc->pc >= 0x1000 && dc->pc < 0x2000) {
> -        env->regs[R_PC] = dc->pc;
>          t_gen_helper_raise_exception(dc, 0xaa);
>          return;
>      }
>  #endif
> +
>      code = cpu_ldl_code(env, dc->pc);
>      op = get_opcode(code);

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
but you probably want to tweak the commit message.

thanks
-- PMM


^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH v2 7/7] target/nios2: Clean up goto in handle_instruction
  2021-06-20 22:10 ` [PATCH v2 7/7] target/nios2: Clean up goto " Richard Henderson
@ 2021-06-28 15:33   ` Peter Maydell
  0 siblings, 0 replies; 17+ messages in thread
From: Peter Maydell @ 2021-06-28 15:33 UTC (permalink / raw)
  To: Richard Henderson; +Cc: Marek Vasut, Chris Wulff, QEMU Developers

On Sun, 20 Jun 2021 at 23:13, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/nios2/translate.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/target/nios2/translate.c b/target/nios2/translate.c
> index 31f63d9faa..276643cee0 100644
> --- a/target/nios2/translate.c
> +++ b/target/nios2/translate.c
> @@ -753,7 +753,8 @@ static void handle_instruction(DisasContext *dc, CPUNios2State *env)
>      op = get_opcode(code);
>
>      if (unlikely(op >= ARRAY_SIZE(i_type_instructions))) {
> -        goto illegal_op;
> +        t_gen_helper_raise_exception(dc, EXCP_ILLEGAL);
> +        return;
>      }
>
>      dc->zero = NULL;
> @@ -764,11 +765,6 @@ static void handle_instruction(DisasContext *dc, CPUNios2State *env)
>      if (dc->zero) {
>          tcg_temp_free(dc->zero);
>      }
> -
> -    return;
> -
> -illegal_op:
> -    t_gen_helper_raise_exception(dc, EXCP_ILLEGAL);
>  }

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

For consistency, we should do the same for the identical code pattern
in handle_r_type_instr().

thanks
-- PMM


^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH v2 5/7] target/nios2: Convert to TranslatorOps
  2021-06-20 22:10 ` [PATCH v2 5/7] target/nios2: Convert to TranslatorOps Richard Henderson
@ 2021-06-28 15:57   ` Peter Maydell
  2021-06-28 16:04     ` Richard Henderson
  0 siblings, 1 reply; 17+ messages in thread
From: Peter Maydell @ 2021-06-28 15:57 UTC (permalink / raw)
  To: Richard Henderson; +Cc: Marek Vasut, Chris Wulff, QEMU Developers

On Sun, 20 Jun 2021 at 23:18, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/nios2/translate.c | 130 ++++++++++++++++++++-------------------
>  1 file changed, 67 insertions(+), 63 deletions(-)
>
> diff --git a/target/nios2/translate.c b/target/nios2/translate.c
> index 31653b7912..06705c894d 100644
> --- a/target/nios2/translate.c
> +++ b/target/nios2/translate.c
> @@ -803,75 +803,72 @@ static void gen_exception(DisasContext *dc, uint32_t excp)
>  }
>
>  /* generate intermediate code for basic block 'tb'.  */
> -void gen_intermediate_code(CPUState *cs, TranslationBlock *tb, int max_insns)
> +static void nios2_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
>  {
> +    DisasContext *dc = container_of(dcbase, DisasContext, base);
>      CPUNios2State *env = cs->env_ptr;
> -    DisasContext dc1, *dc = &dc1;
> -    int num_insns;
> -
> -    /* Initialize DC */
> -
> -    dc->base.tb = tb;
> -    dc->base.singlestep_enabled = cs->singlestep_enabled;
> -    dc->base.is_jmp = DISAS_NEXT;
> -    dc->base.pc_first = tb->pc;
> -    dc->base.pc_next = tb->pc;
> +    target_ulong pc = dc->base.pc_first;

The local variable doesn't really seem necessary -- you could just
write "dc->pc = dc->base.pc_first" and then use "dc->pc" in the
calculation of page_insns.

> +    int page_insns;
>
>      dc->zero    = NULL;
> -    dc->pc      = tb->pc;
> +    dc->pc      = pc;
>      dc->mem_idx = cpu_mmu_index(env, false);
>
> -    /* Set up instruction counts */
> -    num_insns = 0;
> -    if (max_insns > 1) {
> -        int page_insns = (TARGET_PAGE_SIZE - (tb->pc & ~TARGET_PAGE_MASK)) / 4;
> -        if (max_insns > page_insns) {
> -            max_insns = page_insns;
> -        }
> -    }
> +    /* Bound the number of insns to execute to those left on the page.  */
> +    page_insns = -(pc | TARGET_PAGE_MASK) / 4;
> +    dc->base.max_insns = MIN(page_insns, dc->base.max_insns);
> +}
>
> -    gen_tb_start(tb);
> -    do {
> -        tcg_gen_insn_start(dc->pc);
> -        num_insns++;
> +static void nios2_tr_tb_start(DisasContextBase *db, CPUState *cs)
> +{
> +}
>
> -        if (unlikely(cpu_breakpoint_test(cs, dc->pc, BP_ANY))) {
> -            gen_exception(dc, EXCP_DEBUG);
> -            /* The address covered by the breakpoint must be included in
> -               [tb->pc, tb->pc + tb->size) in order to for it to be
> -               properly cleared -- thus we increment the PC here so that
> -               the logic setting tb->size below does the right thing.  */
> -            dc->pc += 4;
> -            break;
> -        }
> +static void nios2_tr_insn_start(DisasContextBase *dcbase, CPUState *cs)
> +{
> +    tcg_gen_insn_start(dcbase->pc_next);
> +}
>
> -        if (num_insns == max_insns && (tb_cflags(tb) & CF_LAST_IO)) {
> -            gen_io_start();
> -        }
> +static bool nios2_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cs,
> +                                      const CPUBreakpoint *bp)
> +{
> +    DisasContext *dc = container_of(dcbase, DisasContext, base);
>
> -        /* Decode an instruction */
> -        handle_instruction(dc, env);
> +    gen_exception(dc, EXCP_DEBUG);
> +    /*
> +     * The address covered by the breakpoint must be included in
> +     * [tb->pc, tb->pc + tb->size) in order to for it to be
> +     * properly cleared -- thus we increment the PC here so that
> +     * the logic setting tb->size below does the right thing.
> +     */
> +    dc->pc += 4;

Don't we need to increment dc->base.pc_next here, not dc->pc? The
generic setting of tb->size in accel/tcg uses "db->pc_next - db->pc_first".

Side note: that comment about "setting tb->size below" seems to have
been copied-and-pasted into most of the front-ends, but it's wrong:
the setting of tb->size is no longer "below" but in the common code.

(More generally, some followup patches rationalizing the use of
dc->pc along the lines of the Arm dc->pc_curr vs dc->base.pc_next
might help. There seems to be a fair amount of code which uses
"dc->pc + 4" that could be using pc_next instead.)

> +    return true;

The arm versions of the breakpoint_check hook also set dc->base.is_jmp
to DISAS_NORETURN.
Are they doing that unnecessarily, or do we need to do that here ?

> +}
>
> -        dc->pc += 4;
> +static void nios2_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs)
> +{
> +    DisasContext *dc = container_of(dcbase, DisasContext, base);
> +    CPUNios2State *env = cs->env_ptr;
>
> -        /* 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.  */
> -    } while (!dc->base.is_jmp &&
> -             !tcg_op_buf_full() &&
> -             num_insns < max_insns);
> +    /* Decode an instruction */
> +    handle_instruction(dc, env);
> +
> +    dc->base.pc_next += 4;
> +    dc->pc += 4;

This isn't wrong, but I think that a setup like the Arm translator
that does
       dc->pc_curr = s->base.pc_next;
       code = cpu_ldl_code(env, s->base.pc_next);
       s->base.pc_next += 4;
       /* dispatch to handler function here */

would be nicer (dunno whether clearer to do as a single thing or
first to do this conversion and then do a followup patch).

thanks
-- PMM


^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH v2 6/7] target/nios2: Remove assignment to env in handle_instruction
  2021-06-28 15:31   ` Peter Maydell
@ 2021-06-28 15:58     ` Richard Henderson
  0 siblings, 0 replies; 17+ messages in thread
From: Richard Henderson @ 2021-06-28 15:58 UTC (permalink / raw)
  To: Peter Maydell; +Cc: Marek Vasut, Chris Wulff, QEMU Developers

On 6/28/21 8:31 AM, Peter Maydell wrote:
> On Sun, 20 Jun 2021 at 23:15, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> Direct assignments to env during translation do not work.
>>
>> As it happens, the only way we can get here is if env->pc
>> is already set to dc->pc.
> 
> More to the point, t_gen_helper_raise_exception() does
>      tcg_gen_movi_tl(dc->cpu_R[R_PC], dc->pc);
> before raising the exception (as you would expect it to).

Ah, didn't notice that.  My comment works from the other direction:

Within the page, 0x1000-0x1fff, any insn executed will raise the 0xaa exception, which 
means pc will be set for that insn.


r~


^ permalink raw reply	[flat|nested] 17+ messages in thread
* Re: [PATCH v2 5/7] target/nios2: Convert to TranslatorOps
  2021-06-28 15:57   ` Peter Maydell
@ 2021-06-28 16:04     ` Richard Henderson
  0 siblings, 0 replies; 17+ messages in thread
From: Richard Henderson @ 2021-06-28 16:04 UTC (permalink / raw)
  To: Peter Maydell; +Cc: Marek Vasut, Chris Wulff, QEMU Developers

On 6/28/21 8:57 AM, Peter Maydell wrote:
> On Sun, 20 Jun 2021 at 23:18, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>   target/nios2/translate.c | 130 ++++++++++++++++++++-------------------
>>   1 file changed, 67 insertions(+), 63 deletions(-)
>>
>> diff --git a/target/nios2/translate.c b/target/nios2/translate.c
>> index 31653b7912..06705c894d 100644
>> --- a/target/nios2/translate.c
>> +++ b/target/nios2/translate.c
>> @@ -803,75 +803,72 @@ static void gen_exception(DisasContext *dc, uint32_t excp)
>>   }
>>
>>   /* generate intermediate code for basic block 'tb'.  */
>> -void gen_intermediate_code(CPUState *cs, TranslationBlock *tb, int max_insns)
>> +static void nios2_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
>>   {
>> +    DisasContext *dc = container_of(dcbase, DisasContext, base);
>>       CPUNios2State *env = cs->env_ptr;
>> -    DisasContext dc1, *dc = &dc1;
>> -    int num_insns;
>> -
>> -    /* Initialize DC */
>> -
>> -    dc->base.tb = tb;
>> -    dc->base.singlestep_enabled = cs->singlestep_enabled;
>> -    dc->base.is_jmp = DISAS_NEXT;
>> -    dc->base.pc_first = tb->pc;
>> -    dc->base.pc_next = tb->pc;
>> +    target_ulong pc = dc->base.pc_first;
> 
> The local variable doesn't really seem necessary -- you could just
> write "dc->pc = dc->base.pc_first" and then use "dc->pc" in the
> calculation of page_insns.
> 
>> +    int page_insns;
>>
>>       dc->zero    = NULL;
>> -    dc->pc      = tb->pc;
>> +    dc->pc      = pc;
>>       dc->mem_idx = cpu_mmu_index(env, false);
>>
>> -    /* Set up instruction counts */
>> -    num_insns = 0;
>> -    if (max_insns > 1) {
>> -        int page_insns = (TARGET_PAGE_SIZE - (tb->pc & ~TARGET_PAGE_MASK)) / 4;
>> -        if (max_insns > page_insns) {
>> -            max_insns = page_insns;
>> -        }
>> -    }
>> +    /* Bound the number of insns to execute to those left on the page.  */
>> +    page_insns = -(pc | TARGET_PAGE_MASK) / 4;
>> +    dc->base.max_insns = MIN(page_insns, dc->base.max_insns);
>> +}
>>
>> -    gen_tb_start(tb);
>> -    do {
>> -        tcg_gen_insn_start(dc->pc);
>> -        num_insns++;
>> +static void nios2_tr_tb_start(DisasContextBase *db, CPUState *cs)
>> +{
>> +}
>>
>> -        if (unlikely(cpu_breakpoint_test(cs, dc->pc, BP_ANY))) {
>> -            gen_exception(dc, EXCP_DEBUG);
>> -            /* The address covered by the breakpoint must be included in
>> -               [tb->pc, tb->pc + tb->size) in order to for it to be
>> -               properly cleared -- thus we increment the PC here so that
>> -               the logic setting tb->size below does the right thing.  */
>> -            dc->pc += 4;
>> -            break;
>> -        }
>> +static void nios2_tr_insn_start(DisasContextBase *dcbase, CPUState *cs)
>> +{
>> +    tcg_gen_insn_start(dcbase->pc_next);
>> +}
>>
>> -        if (num_insns == max_insns && (tb_cflags(tb) & CF_LAST_IO)) {
>> -            gen_io_start();
>> -        }
>> +static bool nios2_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cs,
>> +                                      const CPUBreakpoint *bp)
>> +{
>> +    DisasContext *dc = container_of(dcbase, DisasContext, base);
>>
>> -        /* Decode an instruction */
>> -        handle_instruction(dc, env);
>> +    gen_exception(dc, EXCP_DEBUG);
>> +    /*
>> +     * The address covered by the breakpoint must be included in
>> +     * [tb->pc, tb->pc + tb->size) in order to for it to be
>> +     * properly cleared -- thus we increment the PC here so that
>> +     * the logic setting tb->size below does the right thing.
>> +     */
>> +    dc->pc += 4;
> 
> Don't we need to increment dc->base.pc_next here, not dc->pc? The
> generic setting of tb->size in accel/tcg uses "db->pc_next - db->pc_first".

Yep, thanks.

>> +    return true;
> 
> The arm versions of the breakpoint_check hook also set dc->base.is_jmp
> to DISAS_NORETURN.
> Are they doing that unnecessarily, or do we need to do that here ?

Here it's done in gen_exception.

>> +    handle_instruction(dc, env);
>> +
>> +    dc->base.pc_next += 4;
>> +    dc->pc += 4;
> 
> This isn't wrong, but I think that a setup like the Arm translator
> that does
>         dc->pc_curr = s->base.pc_next;
>         code = cpu_ldl_code(env, s->base.pc_next);
>         s->base.pc_next += 4;
>         /* dispatch to handler function here */
> 
> would be nicer (dunno whether clearer to do as a single thing or
> first to do this conversion and then do a followup patch).

You're right that advancing pc_next first and using that instead of pc+4 globally would be 
a good clean up.


r~



^ permalink raw reply	[flat|nested] 17+ messages in thread
end of thread, other threads:[~2021-06-28 16:08 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-20 22:10 [PATCH v2 0/7] target/nios2: Convert to TranslatorOps Richard Henderson
2021-06-20 22:10 ` [PATCH v2 1/7] target/nios2: Replace DISAS_TB_JUMP with DISAS_NORETURN Richard Henderson
2021-06-28 15:14   ` Peter Maydell
2021-06-20 22:10 ` [PATCH v2 2/7] target/nios2: Use global cpu_env Richard Henderson
2021-06-28 15:15   ` Peter Maydell
2021-06-20 22:10 ` [PATCH v2 3/7] target/nios2: Use global cpu_R Richard Henderson
2021-06-28 15:16   ` Peter Maydell
2021-06-20 22:10 ` [PATCH v2 4/7] target/nios2: Add DisasContextBase to DisasContext Richard Henderson
2021-06-28 15:28   ` Peter Maydell
2021-06-20 22:10 ` [PATCH v2 5/7] target/nios2: Convert to TranslatorOps Richard Henderson
2021-06-28 15:57   ` Peter Maydell
2021-06-28 16:04     ` Richard Henderson
2021-06-20 22:10 ` [PATCH v2 6/7] target/nios2: Remove assignment to env in handle_instruction Richard Henderson
2021-06-28 15:31   ` Peter Maydell
2021-06-28 15:58     ` Richard Henderson
2021-06-20 22:10 ` [PATCH v2 7/7] target/nios2: Clean up goto " Richard Henderson
2021-06-28 15:33   ` Peter Maydell

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.