All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH 0/2] target/openrisc: translator loop conversion
@ 2018-02-18  1:32 Emilio G. Cota
  2018-02-18  1:32 ` [Qemu-devel] [PATCH 1/2] target/openrisc: convert to DisasContextBase Emilio G. Cota
  2018-02-18  1:32 ` [Qemu-devel] [PATCH 2/2] target/openrisc: convert to TranslatorOps Emilio G. Cota
  0 siblings, 2 replies; 8+ messages in thread
From: Emilio G. Cota @ 2018-02-18  1:32 UTC (permalink / raw)
  To: qemu-devel; +Cc: Richard Henderson, Stafford Horne

Tested on the image linked from the wiki:
  https://wiki.qemu.org/Testing/System_Images
Boot after decompressing with:
  or1k-softmmu/qemu-system-or1k -cpu or1200 -M or1k-sim \
	-kernel path/to/or1k-linux-4.10 \
	-serial stdio -nographic -monitor none

Thanks,

		Emilio

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [Qemu-devel] [PATCH 1/2] target/openrisc: convert to DisasContextBase
  2018-02-18  1:32 [Qemu-devel] [PATCH 0/2] target/openrisc: translator loop conversion Emilio G. Cota
@ 2018-02-18  1:32 ` Emilio G. Cota
  2018-02-18  3:10   ` Stafford Horne
  2018-02-21 18:22   ` Richard Henderson
  2018-02-18  1:32 ` [Qemu-devel] [PATCH 2/2] target/openrisc: convert to TranslatorOps Emilio G. Cota
  1 sibling, 2 replies; 8+ messages in thread
From: Emilio G. Cota @ 2018-02-18  1:32 UTC (permalink / raw)
  To: qemu-devel; +Cc: Richard Henderson, Stafford Horne

Signed-off-by: Emilio G. Cota <cota@braap.org>
---
 target/openrisc/translate.c | 87 ++++++++++++++++++++++-----------------------
 1 file changed, 43 insertions(+), 44 deletions(-)

diff --git a/target/openrisc/translate.c b/target/openrisc/translate.c
index 2747b24..0450144 100644
--- a/target/openrisc/translate.c
+++ b/target/openrisc/translate.c
@@ -36,7 +36,8 @@
 #include "exec/log.h"
 
 #define LOG_DIS(str, ...) \
-    qemu_log_mask(CPU_LOG_TB_IN_ASM, "%08x: " str, dc->pc, ## __VA_ARGS__)
+    qemu_log_mask(CPU_LOG_TB_IN_ASM, "%08x: " str, dc->base.pc_next,    \
+                  ## __VA_ARGS__)
 
 /* is_jmp field values */
 #define DISAS_JUMP    DISAS_TARGET_0 /* only pc was modified dynamically */
@@ -44,13 +45,10 @@
 #define DISAS_TB_JUMP DISAS_TARGET_2 /* only pc was modified statically */
 
 typedef struct DisasContext {
-    TranslationBlock *tb;
-    target_ulong pc;
-    uint32_t is_jmp;
+    DisasContextBase base;
     uint32_t mem_idx;
     uint32_t tb_flags;
     uint32_t delayed_branch;
-    bool singlestep_enabled;
 } DisasContext;
 
 static TCGv cpu_sr;
@@ -126,9 +124,9 @@ static void gen_exception(DisasContext *dc, unsigned int excp)
 
 static void gen_illegal_exception(DisasContext *dc)
 {
-    tcg_gen_movi_tl(cpu_pc, dc->pc);
+    tcg_gen_movi_tl(cpu_pc, dc->base.pc_next);
     gen_exception(dc, EXCP_ILLEGAL);
-    dc->is_jmp = DISAS_UPDATE;
+    dc->base.is_jmp = DISAS_UPDATE;
 }
 
 /* not used yet, open it when we need or64.  */
@@ -166,12 +164,12 @@ static void check_ov64s(DisasContext *dc)
 
 static inline bool use_goto_tb(DisasContext *dc, target_ulong 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.tb->pc & TARGET_PAGE_MASK) == (dest & TARGET_PAGE_MASK);
 #else
     return true;
 #endif
@@ -182,10 +180,10 @@ static void gen_goto_tb(DisasContext *dc, int n, target_ulong dest)
     if (use_goto_tb(dc, dest)) {
         tcg_gen_movi_tl(cpu_pc, dest);
         tcg_gen_goto_tb(n);
-        tcg_gen_exit_tb((uintptr_t)dc->tb + n);
+        tcg_gen_exit_tb((uintptr_t)dc->base.tb + n);
     } else {
         tcg_gen_movi_tl(cpu_pc, dest);
-        if (dc->singlestep_enabled) {
+        if (dc->base.singlestep_enabled) {
             gen_exception(dc, EXCP_DEBUG);
         }
         tcg_gen_exit_tb(0);
@@ -194,16 +192,16 @@ static void gen_goto_tb(DisasContext *dc, int n, target_ulong dest)
 
 static void gen_jump(DisasContext *dc, int32_t n26, uint32_t reg, uint32_t op0)
 {
-    target_ulong tmp_pc = dc->pc + n26 * 4;
+    target_ulong tmp_pc = dc->base.pc_next + n26 * 4;
 
     switch (op0) {
     case 0x00:     /* l.j */
         tcg_gen_movi_tl(jmp_pc, tmp_pc);
         break;
     case 0x01:     /* l.jal */
-        tcg_gen_movi_tl(cpu_R[9], dc->pc + 8);
+        tcg_gen_movi_tl(cpu_R[9], dc->base.pc_next + 8);
         /* Optimize jal being used to load the PC for PIC.  */
-        if (tmp_pc == dc->pc + 8) {
+        if (tmp_pc == dc->base.pc_next + 8) {
             return;
         }
         tcg_gen_movi_tl(jmp_pc, tmp_pc);
@@ -211,7 +209,7 @@ static void gen_jump(DisasContext *dc, int32_t n26, uint32_t reg, uint32_t op0)
     case 0x03:     /* l.bnf */
     case 0x04:     /* l.bf  */
         {
-            TCGv t_next = tcg_const_tl(dc->pc + 8);
+            TCGv t_next = tcg_const_tl(dc->base.pc_next + 8);
             TCGv t_true = tcg_const_tl(tmp_pc);
             TCGv t_zero = tcg_const_tl(0);
 
@@ -227,7 +225,7 @@ static void gen_jump(DisasContext *dc, int32_t n26, uint32_t reg, uint32_t op0)
         tcg_gen_mov_tl(jmp_pc, cpu_R[reg]);
         break;
     case 0x12:     /* l.jalr */
-        tcg_gen_movi_tl(cpu_R[9], (dc->pc + 8));
+        tcg_gen_movi_tl(cpu_R[9], (dc->base.pc_next + 8));
         tcg_gen_mov_tl(jmp_pc, cpu_R[reg]);
         break;
     default:
@@ -795,7 +793,7 @@ static void dec_misc(DisasContext *dc, uint32_t insn)
                 return;
             }
             gen_helper_rfe(cpu_env);
-            dc->is_jmp = DISAS_UPDATE;
+            dc->base.is_jmp = DISAS_UPDATE;
 #endif
         }
         break;
@@ -1254,14 +1252,14 @@ static void dec_sys(DisasContext *dc, uint32_t insn)
     switch (op0) {
     case 0x000:    /* l.sys */
         LOG_DIS("l.sys %d\n", K16);
-        tcg_gen_movi_tl(cpu_pc, dc->pc);
+        tcg_gen_movi_tl(cpu_pc, dc->base.pc_next);
         gen_exception(dc, EXCP_SYSCALL);
-        dc->is_jmp = DISAS_UPDATE;
+        dc->base.is_jmp = DISAS_UPDATE;
         break;
 
     case 0x100:    /* l.trap */
         LOG_DIS("l.trap %d\n", K16);
-        tcg_gen_movi_tl(cpu_pc, dc->pc);
+        tcg_gen_movi_tl(cpu_pc, dc->base.pc_next);
         gen_exception(dc, EXCP_TRAP);
         break;
 
@@ -1479,7 +1477,7 @@ static void disas_openrisc_insn(DisasContext *dc, OpenRISCCPU *cpu)
 {
     uint32_t op0;
     uint32_t insn;
-    insn = cpu_ldl_code(&cpu->env, dc->pc);
+    insn = cpu_ldl_code(&cpu->env, dc->base.pc_next);
     op0 = extract32(insn, 26, 6);
 
     switch (op0) {
@@ -1532,14 +1530,15 @@ void gen_intermediate_code(CPUState *cs, struct TranslationBlock *tb)
     int max_insns;
 
     pc_start = tb->pc;
-    dc->tb = tb;
 
-    dc->is_jmp = DISAS_NEXT;
-    dc->pc = pc_start;
+    dc->base.tb = tb;
+    dc->base.singlestep_enabled = cs->singlestep_enabled;
+    dc->base.pc_next = pc_start;
+    dc->base.is_jmp = DISAS_NEXT;
+
     dc->mem_idx = cpu_mmu_index(&cpu->env, false);
-    dc->tb_flags = tb->flags;
+    dc->tb_flags = dc->base.tb->flags;
     dc->delayed_branch = (dc->tb_flags & TB_FLAGS_DFLAG) != 0;
-    dc->singlestep_enabled = cs->singlestep_enabled;
 
     next_page_start = (pc_start & TARGET_PAGE_MASK) + TARGET_PAGE_SIZE;
     num_insns = 0;
@@ -1570,19 +1569,19 @@ void gen_intermediate_code(CPUState *cs, struct TranslationBlock *tb)
     }
 
     do {
-        tcg_gen_insn_start(dc->pc, (dc->delayed_branch ? 1 : 0)
+        tcg_gen_insn_start(dc->base.pc_next, (dc->delayed_branch ? 1 : 0)
 			   | (num_insns ? 2 : 0));
         num_insns++;
 
-        if (unlikely(cpu_breakpoint_test(cs, dc->pc, BP_ANY))) {
-            tcg_gen_movi_tl(cpu_pc, dc->pc);
+        if (unlikely(cpu_breakpoint_test(cs, dc->base.pc_next, BP_ANY))) {
+            tcg_gen_movi_tl(cpu_pc, dc->base.pc_next);
             gen_exception(dc, EXCP_DEBUG);
-            dc->is_jmp = DISAS_UPDATE;
+            dc->base.is_jmp = DISAS_UPDATE;
             /* 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;
+            dc->base.pc_next += 4;
             break;
         }
 
@@ -1590,7 +1589,7 @@ void gen_intermediate_code(CPUState *cs, struct TranslationBlock *tb)
             gen_io_start();
         }
         disas_openrisc_insn(dc, cpu);
-        dc->pc = dc->pc + 4;
+        dc->base.pc_next += 4;
 
         /* delay slot */
         if (dc->delayed_branch) {
@@ -1598,15 +1597,15 @@ void gen_intermediate_code(CPUState *cs, struct TranslationBlock *tb)
             if (!dc->delayed_branch) {
                 tcg_gen_mov_tl(cpu_pc, jmp_pc);
                 tcg_gen_discard_tl(jmp_pc);
-                dc->is_jmp = DISAS_UPDATE;
+                dc->base.is_jmp = DISAS_UPDATE;
                 break;
             }
         }
-    } while (!dc->is_jmp
+    } while (!dc->base.is_jmp
              && !tcg_op_buf_full()
-             && !cs->singlestep_enabled
+             && !dc->base.singlestep_enabled
              && !singlestep
-             && (dc->pc < next_page_start)
+             && (dc->base.pc_next < next_page_start)
              && num_insns < max_insns);
 
     if (tb_cflags(tb) & CF_LAST_IO) {
@@ -1617,17 +1616,17 @@ void gen_intermediate_code(CPUState *cs, struct TranslationBlock *tb)
         tcg_gen_movi_i32(cpu_dflag, dc->delayed_branch != 0);
     }
 
-    tcg_gen_movi_tl(cpu_ppc, dc->pc - 4);
-    if (dc->is_jmp == DISAS_NEXT) {
-        dc->is_jmp = DISAS_UPDATE;
-        tcg_gen_movi_tl(cpu_pc, dc->pc);
+    tcg_gen_movi_tl(cpu_ppc, dc->base.pc_next - 4);
+    if (dc->base.is_jmp == DISAS_NEXT) {
+        dc->base.is_jmp = DISAS_UPDATE;
+        tcg_gen_movi_tl(cpu_pc, dc->base.pc_next);
     }
-    if (unlikely(cs->singlestep_enabled)) {
+    if (unlikely(dc->base.singlestep_enabled)) {
         gen_exception(dc, EXCP_DEBUG);
     } else {
-        switch (dc->is_jmp) {
+        switch (dc->base.is_jmp) {
         case DISAS_NEXT:
-            gen_goto_tb(dc, 0, dc->pc);
+            gen_goto_tb(dc, 0, dc->base.pc_next);
             break;
         default:
         case DISAS_JUMP:
@@ -1645,7 +1644,7 @@ void gen_intermediate_code(CPUState *cs, struct TranslationBlock *tb)
 
     gen_tb_end(tb, num_insns);
 
-    tb->size = dc->pc - pc_start;
+    tb->size = dc->base.pc_next - pc_start;
     tb->icount = num_insns;
 
     if (qemu_loglevel_mask(CPU_LOG_TB_IN_ASM)
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [Qemu-devel] [PATCH 2/2] target/openrisc: convert to TranslatorOps
  2018-02-18  1:32 [Qemu-devel] [PATCH 0/2] target/openrisc: translator loop conversion Emilio G. Cota
  2018-02-18  1:32 ` [Qemu-devel] [PATCH 1/2] target/openrisc: convert to DisasContextBase Emilio G. Cota
@ 2018-02-18  1:32 ` Emilio G. Cota
  2018-02-18  3:13   ` Stafford Horne
  2018-02-21 18:24   ` Richard Henderson
  1 sibling, 2 replies; 8+ messages in thread
From: Emilio G. Cota @ 2018-02-18  1:32 UTC (permalink / raw)
  To: qemu-devel; +Cc: Richard Henderson, Stafford Horne

Notes:

- Changed the num_insns test in tb_start to check for
  dc->base.num_insns > 1, since when tb_start is first
  called in a TB, base.num_insns is already set to 1.

- Removed DISAS_NEXT from the switch on tb_stop; use DISAS_TOO_MANY
  instead.

- Added an assert_not_reached on tb_stop for DISAS_NEXT and the default
  case.

- Merged the two separate log_target_disas calls into the disas_log op.

Signed-off-by: Emilio G. Cota <cota@braap.org>
---
 target/openrisc/translate.c | 168 ++++++++++++++++++++++----------------------
 1 file changed, 85 insertions(+), 83 deletions(-)

diff --git a/target/openrisc/translate.c b/target/openrisc/translate.c
index 0450144..4af4569 100644
--- a/target/openrisc/translate.c
+++ b/target/openrisc/translate.c
@@ -49,6 +49,7 @@ typedef struct DisasContext {
     uint32_t mem_idx;
     uint32_t tb_flags;
     uint32_t delayed_branch;
+    uint32_t next_page_start;
 } DisasContext;
 
 static TCGv cpu_sr;
@@ -1519,46 +1520,23 @@ static void disas_openrisc_insn(DisasContext *dc, OpenRISCCPU *cpu)
     }
 }
 
-void gen_intermediate_code(CPUState *cs, struct TranslationBlock *tb)
+static int openrisc_tr_init_disas_context(DisasContextBase *dcbase,
+                                          CPUState *cs, int max_insns)
 {
+    DisasContext *dc = container_of(dcbase, DisasContext, base);
     CPUOpenRISCState *env = cs->env_ptr;
-    OpenRISCCPU *cpu = openrisc_env_get_cpu(env);
-    struct DisasContext ctx, *dc = &ctx;
-    uint32_t pc_start;
-    uint32_t next_page_start;
-    int num_insns;
-    int max_insns;
 
-    pc_start = tb->pc;
-
-    dc->base.tb = tb;
-    dc->base.singlestep_enabled = cs->singlestep_enabled;
-    dc->base.pc_next = pc_start;
-    dc->base.is_jmp = DISAS_NEXT;
-
-    dc->mem_idx = cpu_mmu_index(&cpu->env, false);
+    dc->mem_idx = cpu_mmu_index(env, false);
     dc->tb_flags = dc->base.tb->flags;
     dc->delayed_branch = (dc->tb_flags & TB_FLAGS_DFLAG) != 0;
+    dc->next_page_start = (dc->base.pc_first & TARGET_PAGE_MASK) +
+        TARGET_PAGE_SIZE;
+    return max_insns;
+}
 
-    next_page_start = (pc_start & TARGET_PAGE_MASK) + TARGET_PAGE_SIZE;
-    num_insns = 0;
-    max_insns = tb_cflags(tb) & CF_COUNT_MASK;
-
-    if (max_insns == 0) {
-        max_insns = CF_COUNT_MASK;
-    }
-    if (max_insns > TCG_MAX_INSNS) {
-        max_insns = TCG_MAX_INSNS;
-    }
-
-    if (qemu_loglevel_mask(CPU_LOG_TB_IN_ASM)
-        && qemu_log_in_addr_range(pc_start)) {
-        qemu_log_lock();
-        qemu_log("----------------\n");
-        qemu_log("IN: %s\n", lookup_symbol(pc_start));
-    }
-
-    gen_tb_start(tb);
+static void openrisc_tr_tb_start(DisasContextBase *db, CPUState *cs)
+{
+    DisasContext *dc = container_of(db, DisasContext, base);
 
     /* Allow the TCG optimizer to see that R0 == 0,
        when it's true, which is the common case.  */
@@ -1567,50 +1545,60 @@ void gen_intermediate_code(CPUState *cs, struct TranslationBlock *tb)
     } else {
         cpu_R[0] = cpu_R0;
     }
+}
+
+static void openrisc_tr_insn_start(DisasContextBase *dcbase, CPUState *cs)
+{
+    DisasContext *dc = container_of(dcbase, DisasContext, base);
 
-    do {
-        tcg_gen_insn_start(dc->base.pc_next, (dc->delayed_branch ? 1 : 0)
-			   | (num_insns ? 2 : 0));
-        num_insns++;
+    tcg_gen_insn_start(dc->base.pc_next, (dc->delayed_branch ? 1 : 0)
+                       | (dc->base.num_insns > 1 ? 2 : 0));
+}
 
-        if (unlikely(cpu_breakpoint_test(cs, dc->base.pc_next, BP_ANY))) {
-            tcg_gen_movi_tl(cpu_pc, dc->base.pc_next);
-            gen_exception(dc, EXCP_DEBUG);
+static bool openrisc_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cs,
+                                         const CPUBreakpoint *bp)
+{
+    DisasContext *dc = container_of(dcbase, DisasContext, base);
+
+    tcg_gen_movi_tl(cpu_pc, dc->base.pc_next);
+    gen_exception(dc, EXCP_DEBUG);
+    dc->base.is_jmp = DISAS_UPDATE;
+    /* 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->base.pc_next += 4;
+    return true;
+}
+
+static void openrisc_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs)
+{
+    DisasContext *dc = container_of(dcbase, DisasContext, base);
+    OpenRISCCPU *cpu = OPENRISC_CPU(cs);
+
+    disas_openrisc_insn(dc, cpu);
+    dc->base.pc_next += 4;
+
+    /* delay slot */
+    if (dc->delayed_branch) {
+        dc->delayed_branch--;
+        if (!dc->delayed_branch) {
+            tcg_gen_mov_tl(cpu_pc, jmp_pc);
+            tcg_gen_discard_tl(jmp_pc);
             dc->base.is_jmp = DISAS_UPDATE;
-            /* 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->base.pc_next += 4;
-            break;
+            return;
         }
+    }
 
-        if (num_insns == max_insns && (tb_cflags(tb) & CF_LAST_IO)) {
-            gen_io_start();
-        }
-        disas_openrisc_insn(dc, cpu);
-        dc->base.pc_next += 4;
-
-        /* delay slot */
-        if (dc->delayed_branch) {
-            dc->delayed_branch--;
-            if (!dc->delayed_branch) {
-                tcg_gen_mov_tl(cpu_pc, jmp_pc);
-                tcg_gen_discard_tl(jmp_pc);
-                dc->base.is_jmp = DISAS_UPDATE;
-                break;
-            }
-        }
-    } while (!dc->base.is_jmp
-             && !tcg_op_buf_full()
-             && !dc->base.singlestep_enabled
-             && !singlestep
-             && (dc->base.pc_next < next_page_start)
-             && num_insns < max_insns);
-
-    if (tb_cflags(tb) & CF_LAST_IO) {
-        gen_io_end();
+    if (dc->base.is_jmp == DISAS_NEXT &&
+        dc->base.pc_next >= dc->next_page_start) {
+        dc->base.is_jmp = DISAS_TOO_MANY;
     }
+}
+
+static void openrisc_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs)
+{
+    DisasContext *dc = container_of(dcbase, DisasContext, base);
 
     if ((dc->tb_flags & TB_FLAGS_DFLAG ? 1 : 0) != (dc->delayed_branch != 0)) {
         tcg_gen_movi_i32(cpu_dflag, dc->delayed_branch != 0);
@@ -1625,10 +1613,9 @@ void gen_intermediate_code(CPUState *cs, struct TranslationBlock *tb)
         gen_exception(dc, EXCP_DEBUG);
     } else {
         switch (dc->base.is_jmp) {
-        case DISAS_NEXT:
+        case DISAS_TOO_MANY:
             gen_goto_tb(dc, 0, dc->base.pc_next);
             break;
-        default:
         case DISAS_JUMP:
             break;
         case DISAS_UPDATE:
@@ -1639,20 +1626,35 @@ void gen_intermediate_code(CPUState *cs, struct TranslationBlock *tb)
         case DISAS_TB_JUMP:
             /* nothing more to generate */
             break;
+        default:
+            g_assert_not_reached();
         }
     }
+}
+
+static void openrisc_tr_disas_log(const DisasContextBase *dcbase, CPUState *cs)
+{
+    DisasContext *s = container_of(dcbase, DisasContext, base);
 
-    gen_tb_end(tb, num_insns);
+    qemu_log("IN: %s\n", lookup_symbol(s->base.pc_first));
+    log_target_disas(cs, s->base.pc_first, s->base.tb->size);
+}
 
-    tb->size = dc->base.pc_next - pc_start;
-    tb->icount = num_insns;
+static const TranslatorOps openrisc_tr_ops = {
+    .init_disas_context = openrisc_tr_init_disas_context,
+    .tb_start           = openrisc_tr_tb_start,
+    .insn_start         = openrisc_tr_insn_start,
+    .breakpoint_check   = openrisc_tr_breakpoint_check,
+    .translate_insn     = openrisc_tr_translate_insn,
+    .tb_stop            = openrisc_tr_tb_stop,
+    .disas_log          = openrisc_tr_disas_log,
+};
 
-    if (qemu_loglevel_mask(CPU_LOG_TB_IN_ASM)
-        && qemu_log_in_addr_range(pc_start)) {
-        log_target_disas(cs, pc_start, tb->size);
-        qemu_log("\n");
-        qemu_log_unlock();
-    }
+void gen_intermediate_code(CPUState *cs, struct TranslationBlock *tb)
+{
+    DisasContext ctx;
+
+    translator_loop(&openrisc_tr_ops, &ctx.base, cs, tb);
 }
 
 void openrisc_cpu_dump_state(CPUState *cs, FILE *f,
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH 1/2] target/openrisc: convert to DisasContextBase
  2018-02-18  1:32 ` [Qemu-devel] [PATCH 1/2] target/openrisc: convert to DisasContextBase Emilio G. Cota
@ 2018-02-18  3:10   ` Stafford Horne
  2018-02-18  6:11     ` Emilio G. Cota
  2018-02-21 18:22   ` Richard Henderson
  1 sibling, 1 reply; 8+ messages in thread
From: Stafford Horne @ 2018-02-18  3:10 UTC (permalink / raw)
  To: Emilio G. Cota; +Cc: qemu-devel, Richard Henderson

On Sat, Feb 17, 2018 at 08:32:36PM -0500, Emilio G. Cota wrote:
> Signed-off-by: Emilio G. Cota <cota@braap.org>

Hello,

This looks ok to me, and thanks for testing.  However, I am not so familiar with
the DisasContextBase.  Is this something new?

It would be good to have a commit message to say what it is any why we are
making the change?

-Stafford

> ---
>  target/openrisc/translate.c | 87 ++++++++++++++++++++++-----------------------
>  1 file changed, 43 insertions(+), 44 deletions(-)
> 
> diff --git a/target/openrisc/translate.c b/target/openrisc/translate.c
> index 2747b24..0450144 100644
> --- a/target/openrisc/translate.c
> +++ b/target/openrisc/translate.c
> @@ -36,7 +36,8 @@
>  #include "exec/log.h"
>  
>  #define LOG_DIS(str, ...) \
> -    qemu_log_mask(CPU_LOG_TB_IN_ASM, "%08x: " str, dc->pc, ## __VA_ARGS__)
> +    qemu_log_mask(CPU_LOG_TB_IN_ASM, "%08x: " str, dc->base.pc_next,    \
> +                  ## __VA_ARGS__)
>  
>  /* is_jmp field values */
>  #define DISAS_JUMP    DISAS_TARGET_0 /* only pc was modified dynamically */
> @@ -44,13 +45,10 @@
>  #define DISAS_TB_JUMP DISAS_TARGET_2 /* only pc was modified statically */
>  
>  typedef struct DisasContext {
> -    TranslationBlock *tb;
> -    target_ulong pc;
> -    uint32_t is_jmp;
> +    DisasContextBase base;
>      uint32_t mem_idx;
>      uint32_t tb_flags;
>      uint32_t delayed_branch;
> -    bool singlestep_enabled;
>  } DisasContext;
>  
>  static TCGv cpu_sr;
> @@ -126,9 +124,9 @@ static void gen_exception(DisasContext *dc, unsigned int excp)
>  
>  static void gen_illegal_exception(DisasContext *dc)
>  {
> -    tcg_gen_movi_tl(cpu_pc, dc->pc);
> +    tcg_gen_movi_tl(cpu_pc, dc->base.pc_next);
>      gen_exception(dc, EXCP_ILLEGAL);
> -    dc->is_jmp = DISAS_UPDATE;
> +    dc->base.is_jmp = DISAS_UPDATE;
>  }
>  
>  /* not used yet, open it when we need or64.  */
> @@ -166,12 +164,12 @@ static void check_ov64s(DisasContext *dc)
>  
>  static inline bool use_goto_tb(DisasContext *dc, target_ulong 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.tb->pc & TARGET_PAGE_MASK) == (dest & TARGET_PAGE_MASK);
>  #else
>      return true;
>  #endif
> @@ -182,10 +180,10 @@ static void gen_goto_tb(DisasContext *dc, int n, target_ulong dest)
>      if (use_goto_tb(dc, dest)) {
>          tcg_gen_movi_tl(cpu_pc, dest);
>          tcg_gen_goto_tb(n);
> -        tcg_gen_exit_tb((uintptr_t)dc->tb + n);
> +        tcg_gen_exit_tb((uintptr_t)dc->base.tb + n);
>      } else {
>          tcg_gen_movi_tl(cpu_pc, dest);
> -        if (dc->singlestep_enabled) {
> +        if (dc->base.singlestep_enabled) {
>              gen_exception(dc, EXCP_DEBUG);
>          }
>          tcg_gen_exit_tb(0);
> @@ -194,16 +192,16 @@ static void gen_goto_tb(DisasContext *dc, int n, target_ulong dest)
>  
>  static void gen_jump(DisasContext *dc, int32_t n26, uint32_t reg, uint32_t op0)
>  {
> -    target_ulong tmp_pc = dc->pc + n26 * 4;
> +    target_ulong tmp_pc = dc->base.pc_next + n26 * 4;
>  
>      switch (op0) {
>      case 0x00:     /* l.j */
>          tcg_gen_movi_tl(jmp_pc, tmp_pc);
>          break;
>      case 0x01:     /* l.jal */
> -        tcg_gen_movi_tl(cpu_R[9], dc->pc + 8);
> +        tcg_gen_movi_tl(cpu_R[9], dc->base.pc_next + 8);
>          /* Optimize jal being used to load the PC for PIC.  */
> -        if (tmp_pc == dc->pc + 8) {
> +        if (tmp_pc == dc->base.pc_next + 8) {
>              return;
>          }
>          tcg_gen_movi_tl(jmp_pc, tmp_pc);
> @@ -211,7 +209,7 @@ static void gen_jump(DisasContext *dc, int32_t n26, uint32_t reg, uint32_t op0)
>      case 0x03:     /* l.bnf */
>      case 0x04:     /* l.bf  */
>          {
> -            TCGv t_next = tcg_const_tl(dc->pc + 8);
> +            TCGv t_next = tcg_const_tl(dc->base.pc_next + 8);
>              TCGv t_true = tcg_const_tl(tmp_pc);
>              TCGv t_zero = tcg_const_tl(0);
>  
> @@ -227,7 +225,7 @@ static void gen_jump(DisasContext *dc, int32_t n26, uint32_t reg, uint32_t op0)
>          tcg_gen_mov_tl(jmp_pc, cpu_R[reg]);
>          break;
>      case 0x12:     /* l.jalr */
> -        tcg_gen_movi_tl(cpu_R[9], (dc->pc + 8));
> +        tcg_gen_movi_tl(cpu_R[9], (dc->base.pc_next + 8));
>          tcg_gen_mov_tl(jmp_pc, cpu_R[reg]);
>          break;
>      default:
> @@ -795,7 +793,7 @@ static void dec_misc(DisasContext *dc, uint32_t insn)
>                  return;
>              }
>              gen_helper_rfe(cpu_env);
> -            dc->is_jmp = DISAS_UPDATE;
> +            dc->base.is_jmp = DISAS_UPDATE;
>  #endif
>          }
>          break;
> @@ -1254,14 +1252,14 @@ static void dec_sys(DisasContext *dc, uint32_t insn)
>      switch (op0) {
>      case 0x000:    /* l.sys */
>          LOG_DIS("l.sys %d\n", K16);
> -        tcg_gen_movi_tl(cpu_pc, dc->pc);
> +        tcg_gen_movi_tl(cpu_pc, dc->base.pc_next);
>          gen_exception(dc, EXCP_SYSCALL);
> -        dc->is_jmp = DISAS_UPDATE;
> +        dc->base.is_jmp = DISAS_UPDATE;
>          break;
>  
>      case 0x100:    /* l.trap */
>          LOG_DIS("l.trap %d\n", K16);
> -        tcg_gen_movi_tl(cpu_pc, dc->pc);
> +        tcg_gen_movi_tl(cpu_pc, dc->base.pc_next);
>          gen_exception(dc, EXCP_TRAP);
>          break;
>  
> @@ -1479,7 +1477,7 @@ static void disas_openrisc_insn(DisasContext *dc, OpenRISCCPU *cpu)
>  {
>      uint32_t op0;
>      uint32_t insn;
> -    insn = cpu_ldl_code(&cpu->env, dc->pc);
> +    insn = cpu_ldl_code(&cpu->env, dc->base.pc_next);
>      op0 = extract32(insn, 26, 6);
>  
>      switch (op0) {
> @@ -1532,14 +1530,15 @@ void gen_intermediate_code(CPUState *cs, struct TranslationBlock *tb)
>      int max_insns;
>  
>      pc_start = tb->pc;
> -    dc->tb = tb;
>  
> -    dc->is_jmp = DISAS_NEXT;
> -    dc->pc = pc_start;
> +    dc->base.tb = tb;
> +    dc->base.singlestep_enabled = cs->singlestep_enabled;
> +    dc->base.pc_next = pc_start;
> +    dc->base.is_jmp = DISAS_NEXT;
> +
>      dc->mem_idx = cpu_mmu_index(&cpu->env, false);
> -    dc->tb_flags = tb->flags;
> +    dc->tb_flags = dc->base.tb->flags;
>      dc->delayed_branch = (dc->tb_flags & TB_FLAGS_DFLAG) != 0;
> -    dc->singlestep_enabled = cs->singlestep_enabled;
>  
>      next_page_start = (pc_start & TARGET_PAGE_MASK) + TARGET_PAGE_SIZE;
>      num_insns = 0;
> @@ -1570,19 +1569,19 @@ void gen_intermediate_code(CPUState *cs, struct TranslationBlock *tb)
>      }
>  
>      do {
> -        tcg_gen_insn_start(dc->pc, (dc->delayed_branch ? 1 : 0)
> +        tcg_gen_insn_start(dc->base.pc_next, (dc->delayed_branch ? 1 : 0)
>  			   | (num_insns ? 2 : 0));
>          num_insns++;
>  
> -        if (unlikely(cpu_breakpoint_test(cs, dc->pc, BP_ANY))) {
> -            tcg_gen_movi_tl(cpu_pc, dc->pc);
> +        if (unlikely(cpu_breakpoint_test(cs, dc->base.pc_next, BP_ANY))) {
> +            tcg_gen_movi_tl(cpu_pc, dc->base.pc_next);
>              gen_exception(dc, EXCP_DEBUG);
> -            dc->is_jmp = DISAS_UPDATE;
> +            dc->base.is_jmp = DISAS_UPDATE;
>              /* 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;
> +            dc->base.pc_next += 4;
>              break;
>          }
>  
> @@ -1590,7 +1589,7 @@ void gen_intermediate_code(CPUState *cs, struct TranslationBlock *tb)
>              gen_io_start();
>          }
>          disas_openrisc_insn(dc, cpu);
> -        dc->pc = dc->pc + 4;
> +        dc->base.pc_next += 4;
>  
>          /* delay slot */
>          if (dc->delayed_branch) {
> @@ -1598,15 +1597,15 @@ void gen_intermediate_code(CPUState *cs, struct TranslationBlock *tb)
>              if (!dc->delayed_branch) {
>                  tcg_gen_mov_tl(cpu_pc, jmp_pc);
>                  tcg_gen_discard_tl(jmp_pc);
> -                dc->is_jmp = DISAS_UPDATE;
> +                dc->base.is_jmp = DISAS_UPDATE;
>                  break;
>              }
>          }
> -    } while (!dc->is_jmp
> +    } while (!dc->base.is_jmp
>               && !tcg_op_buf_full()
> -             && !cs->singlestep_enabled
> +             && !dc->base.singlestep_enabled
>               && !singlestep
> -             && (dc->pc < next_page_start)
> +             && (dc->base.pc_next < next_page_start)
>               && num_insns < max_insns);
>  
>      if (tb_cflags(tb) & CF_LAST_IO) {
> @@ -1617,17 +1616,17 @@ void gen_intermediate_code(CPUState *cs, struct TranslationBlock *tb)
>          tcg_gen_movi_i32(cpu_dflag, dc->delayed_branch != 0);
>      }
>  
> -    tcg_gen_movi_tl(cpu_ppc, dc->pc - 4);
> -    if (dc->is_jmp == DISAS_NEXT) {
> -        dc->is_jmp = DISAS_UPDATE;
> -        tcg_gen_movi_tl(cpu_pc, dc->pc);
> +    tcg_gen_movi_tl(cpu_ppc, dc->base.pc_next - 4);
> +    if (dc->base.is_jmp == DISAS_NEXT) {
> +        dc->base.is_jmp = DISAS_UPDATE;
> +        tcg_gen_movi_tl(cpu_pc, dc->base.pc_next);
>      }
> -    if (unlikely(cs->singlestep_enabled)) {
> +    if (unlikely(dc->base.singlestep_enabled)) {
>          gen_exception(dc, EXCP_DEBUG);
>      } else {
> -        switch (dc->is_jmp) {
> +        switch (dc->base.is_jmp) {
>          case DISAS_NEXT:
> -            gen_goto_tb(dc, 0, dc->pc);
> +            gen_goto_tb(dc, 0, dc->base.pc_next);
>              break;
>          default:
>          case DISAS_JUMP:
> @@ -1645,7 +1644,7 @@ void gen_intermediate_code(CPUState *cs, struct TranslationBlock *tb)
>  
>      gen_tb_end(tb, num_insns);
>  
> -    tb->size = dc->pc - pc_start;
> +    tb->size = dc->base.pc_next - pc_start;
>      tb->icount = num_insns;
>  
>      if (qemu_loglevel_mask(CPU_LOG_TB_IN_ASM)
> -- 
> 2.7.4
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH 2/2] target/openrisc: convert to TranslatorOps
  2018-02-18  1:32 ` [Qemu-devel] [PATCH 2/2] target/openrisc: convert to TranslatorOps Emilio G. Cota
@ 2018-02-18  3:13   ` Stafford Horne
  2018-02-21 18:24   ` Richard Henderson
  1 sibling, 0 replies; 8+ messages in thread
From: Stafford Horne @ 2018-02-18  3:13 UTC (permalink / raw)
  To: Emilio G. Cota; +Cc: qemu-devel, Richard Henderson

On Sat, Feb 17, 2018 at 08:32:37PM -0500, Emilio G. Cota wrote:
> Notes:
> 
> - Changed the num_insns test in tb_start to check for
>   dc->base.num_insns > 1, since when tb_start is first
>   called in a TB, base.num_insns is already set to 1.
> 
> - Removed DISAS_NEXT from the switch on tb_stop; use DISAS_TOO_MANY
>   instead.
> 
> - Added an assert_not_reached on tb_stop for DISAS_NEXT and the default
>   case.
> 
> - Merged the two separate log_target_disas calls into the disas_log op.

Hello, thanks for this again.  But just wondering if you can add some background
to the commit message?  Whats the benefit?  Probably something like "Updating
translate core to use new DisasContext and other apis which are being adopted in
other targets... "

-Stafford

> Signed-off-by: Emilio G. Cota <cota@braap.org>
> ---
>  target/openrisc/translate.c | 168 ++++++++++++++++++++++----------------------
>  1 file changed, 85 insertions(+), 83 deletions(-)
> 
> diff --git a/target/openrisc/translate.c b/target/openrisc/translate.c
> index 0450144..4af4569 100644
> --- a/target/openrisc/translate.c
> +++ b/target/openrisc/translate.c
> @@ -49,6 +49,7 @@ typedef struct DisasContext {
>      uint32_t mem_idx;
>      uint32_t tb_flags;
>      uint32_t delayed_branch;
> +    uint32_t next_page_start;
>  } DisasContext;
>  
>  static TCGv cpu_sr;
> @@ -1519,46 +1520,23 @@ static void disas_openrisc_insn(DisasContext *dc, OpenRISCCPU *cpu)
>      }
>  }
>  
> -void gen_intermediate_code(CPUState *cs, struct TranslationBlock *tb)
> +static int openrisc_tr_init_disas_context(DisasContextBase *dcbase,
> +                                          CPUState *cs, int max_insns)
>  {
> +    DisasContext *dc = container_of(dcbase, DisasContext, base);
>      CPUOpenRISCState *env = cs->env_ptr;
> -    OpenRISCCPU *cpu = openrisc_env_get_cpu(env);
> -    struct DisasContext ctx, *dc = &ctx;
> -    uint32_t pc_start;
> -    uint32_t next_page_start;
> -    int num_insns;
> -    int max_insns;
>  
> -    pc_start = tb->pc;
> -
> -    dc->base.tb = tb;
> -    dc->base.singlestep_enabled = cs->singlestep_enabled;
> -    dc->base.pc_next = pc_start;
> -    dc->base.is_jmp = DISAS_NEXT;
> -
> -    dc->mem_idx = cpu_mmu_index(&cpu->env, false);
> +    dc->mem_idx = cpu_mmu_index(env, false);
>      dc->tb_flags = dc->base.tb->flags;
>      dc->delayed_branch = (dc->tb_flags & TB_FLAGS_DFLAG) != 0;
> +    dc->next_page_start = (dc->base.pc_first & TARGET_PAGE_MASK) +
> +        TARGET_PAGE_SIZE;
> +    return max_insns;
> +}
>  
> -    next_page_start = (pc_start & TARGET_PAGE_MASK) + TARGET_PAGE_SIZE;
> -    num_insns = 0;
> -    max_insns = tb_cflags(tb) & CF_COUNT_MASK;
> -
> -    if (max_insns == 0) {
> -        max_insns = CF_COUNT_MASK;
> -    }
> -    if (max_insns > TCG_MAX_INSNS) {
> -        max_insns = TCG_MAX_INSNS;
> -    }
> -
> -    if (qemu_loglevel_mask(CPU_LOG_TB_IN_ASM)
> -        && qemu_log_in_addr_range(pc_start)) {
> -        qemu_log_lock();
> -        qemu_log("----------------\n");
> -        qemu_log("IN: %s\n", lookup_symbol(pc_start));
> -    }
> -
> -    gen_tb_start(tb);
> +static void openrisc_tr_tb_start(DisasContextBase *db, CPUState *cs)
> +{
> +    DisasContext *dc = container_of(db, DisasContext, base);
>  
>      /* Allow the TCG optimizer to see that R0 == 0,
>         when it's true, which is the common case.  */
> @@ -1567,50 +1545,60 @@ void gen_intermediate_code(CPUState *cs, struct TranslationBlock *tb)
>      } else {
>          cpu_R[0] = cpu_R0;
>      }
> +}
> +
> +static void openrisc_tr_insn_start(DisasContextBase *dcbase, CPUState *cs)
> +{
> +    DisasContext *dc = container_of(dcbase, DisasContext, base);
>  
> -    do {
> -        tcg_gen_insn_start(dc->base.pc_next, (dc->delayed_branch ? 1 : 0)
> -			   | (num_insns ? 2 : 0));
> -        num_insns++;
> +    tcg_gen_insn_start(dc->base.pc_next, (dc->delayed_branch ? 1 : 0)
> +                       | (dc->base.num_insns > 1 ? 2 : 0));
> +}
>  
> -        if (unlikely(cpu_breakpoint_test(cs, dc->base.pc_next, BP_ANY))) {
> -            tcg_gen_movi_tl(cpu_pc, dc->base.pc_next);
> -            gen_exception(dc, EXCP_DEBUG);
> +static bool openrisc_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cs,
> +                                         const CPUBreakpoint *bp)
> +{
> +    DisasContext *dc = container_of(dcbase, DisasContext, base);
> +
> +    tcg_gen_movi_tl(cpu_pc, dc->base.pc_next);
> +    gen_exception(dc, EXCP_DEBUG);
> +    dc->base.is_jmp = DISAS_UPDATE;
> +    /* 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->base.pc_next += 4;
> +    return true;
> +}
> +
> +static void openrisc_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs)
> +{
> +    DisasContext *dc = container_of(dcbase, DisasContext, base);
> +    OpenRISCCPU *cpu = OPENRISC_CPU(cs);
> +
> +    disas_openrisc_insn(dc, cpu);
> +    dc->base.pc_next += 4;
> +
> +    /* delay slot */
> +    if (dc->delayed_branch) {
> +        dc->delayed_branch--;
> +        if (!dc->delayed_branch) {
> +            tcg_gen_mov_tl(cpu_pc, jmp_pc);
> +            tcg_gen_discard_tl(jmp_pc);
>              dc->base.is_jmp = DISAS_UPDATE;
> -            /* 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->base.pc_next += 4;
> -            break;
> +            return;
>          }
> +    }
>  
> -        if (num_insns == max_insns && (tb_cflags(tb) & CF_LAST_IO)) {
> -            gen_io_start();
> -        }
> -        disas_openrisc_insn(dc, cpu);
> -        dc->base.pc_next += 4;
> -
> -        /* delay slot */
> -        if (dc->delayed_branch) {
> -            dc->delayed_branch--;
> -            if (!dc->delayed_branch) {
> -                tcg_gen_mov_tl(cpu_pc, jmp_pc);
> -                tcg_gen_discard_tl(jmp_pc);
> -                dc->base.is_jmp = DISAS_UPDATE;
> -                break;
> -            }
> -        }
> -    } while (!dc->base.is_jmp
> -             && !tcg_op_buf_full()
> -             && !dc->base.singlestep_enabled
> -             && !singlestep
> -             && (dc->base.pc_next < next_page_start)
> -             && num_insns < max_insns);
> -
> -    if (tb_cflags(tb) & CF_LAST_IO) {
> -        gen_io_end();
> +    if (dc->base.is_jmp == DISAS_NEXT &&
> +        dc->base.pc_next >= dc->next_page_start) {
> +        dc->base.is_jmp = DISAS_TOO_MANY;
>      }
> +}
> +
> +static void openrisc_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs)
> +{
> +    DisasContext *dc = container_of(dcbase, DisasContext, base);
>  
>      if ((dc->tb_flags & TB_FLAGS_DFLAG ? 1 : 0) != (dc->delayed_branch != 0)) {
>          tcg_gen_movi_i32(cpu_dflag, dc->delayed_branch != 0);
> @@ -1625,10 +1613,9 @@ void gen_intermediate_code(CPUState *cs, struct TranslationBlock *tb)
>          gen_exception(dc, EXCP_DEBUG);
>      } else {
>          switch (dc->base.is_jmp) {
> -        case DISAS_NEXT:
> +        case DISAS_TOO_MANY:
>              gen_goto_tb(dc, 0, dc->base.pc_next);
>              break;
> -        default:
>          case DISAS_JUMP:
>              break;
>          case DISAS_UPDATE:
> @@ -1639,20 +1626,35 @@ void gen_intermediate_code(CPUState *cs, struct TranslationBlock *tb)
>          case DISAS_TB_JUMP:
>              /* nothing more to generate */
>              break;
> +        default:
> +            g_assert_not_reached();
>          }
>      }
> +}
> +
> +static void openrisc_tr_disas_log(const DisasContextBase *dcbase, CPUState *cs)
> +{
> +    DisasContext *s = container_of(dcbase, DisasContext, base);
>  
> -    gen_tb_end(tb, num_insns);
> +    qemu_log("IN: %s\n", lookup_symbol(s->base.pc_first));
> +    log_target_disas(cs, s->base.pc_first, s->base.tb->size);
> +}
>  
> -    tb->size = dc->base.pc_next - pc_start;
> -    tb->icount = num_insns;
> +static const TranslatorOps openrisc_tr_ops = {
> +    .init_disas_context = openrisc_tr_init_disas_context,
> +    .tb_start           = openrisc_tr_tb_start,
> +    .insn_start         = openrisc_tr_insn_start,
> +    .breakpoint_check   = openrisc_tr_breakpoint_check,
> +    .translate_insn     = openrisc_tr_translate_insn,
> +    .tb_stop            = openrisc_tr_tb_stop,
> +    .disas_log          = openrisc_tr_disas_log,
> +};
>  
> -    if (qemu_loglevel_mask(CPU_LOG_TB_IN_ASM)
> -        && qemu_log_in_addr_range(pc_start)) {
> -        log_target_disas(cs, pc_start, tb->size);
> -        qemu_log("\n");
> -        qemu_log_unlock();
> -    }
> +void gen_intermediate_code(CPUState *cs, struct TranslationBlock *tb)
> +{
> +    DisasContext ctx;
> +
> +    translator_loop(&openrisc_tr_ops, &ctx.base, cs, tb);
>  }
>  
>  void openrisc_cpu_dump_state(CPUState *cs, FILE *f,
> -- 
> 2.7.4
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH 1/2] target/openrisc: convert to DisasContextBase
  2018-02-18  3:10   ` Stafford Horne
@ 2018-02-18  6:11     ` Emilio G. Cota
  0 siblings, 0 replies; 8+ messages in thread
From: Emilio G. Cota @ 2018-02-18  6:11 UTC (permalink / raw)
  To: Stafford Horne; +Cc: qemu-devel, Richard Henderson

On Sun, Feb 18, 2018 at 12:10:46 +0900, Stafford Horne wrote:
> On Sat, Feb 17, 2018 at 08:32:36PM -0500, Emilio G. Cota wrote:
> > Signed-off-by: Emilio G. Cota <cota@braap.org>
> This looks ok to me, and thanks for testing.  However, I am not so familiar with
> the DisasContextBase.  Is this something new?

The work on having a generic translation loop started a while ago,
picking up steam in June'17 -- look for "Generic translation framework"
threads on the mailing list.

The goal is to have a single loop (accel/tcg/translator.c) to
translate from target code to TCG IR. Apart from reducing code
duplication, this will eventually ease things like inserting
instrumentation, which will have a single injection point
instead of having to patch all targets' translation loops.

Transitioning to the generic translation loop typically
involves three steps:
1- Use of DisasJumpType to mark the exits from the translation loop
2- Use of DisasContextBase to keep track of some state that applies
   to all targets (e.g. num_insns, program counter)
3- Conversion to TranslatorOps, which is a set of function pointers
   called from translator_loop in accel/tcg/translator.c.

You can see an example of 1-3 for Alpha in commits 3de811c, c5f8065
and 99a92b9, respectively.

Quite a few targets have already been converted (you can see which
ones with "git grep '^\s*translator_loop('"); I'm in the
process of converting the remaining ones as long as I can test
them with a boot image (I've been spamming the list with
conversion patches the last few days).

> It would be good to have a commit message to say what it is any why we are
> making the change?

I considered it, but didn't want to annoy everyone by sending the
same explanation many times (once for each converted target).
The purpose and value of this consolidation work
is well-known among people who follow TCG-related threads on
the mailing list (as I said above this work has been ongoing
for a while), so I think it's reasonable to keep the commit
message empty.

I figured some people would have to be filled in though (like
yourself), and that's why I just wrote the above; now
I can point to this message if this happens again :-)

Hope the background I gave above helps; please let me know
if anything is unclear.

Thanks,

		Emilio

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH 1/2] target/openrisc: convert to DisasContextBase
  2018-02-18  1:32 ` [Qemu-devel] [PATCH 1/2] target/openrisc: convert to DisasContextBase Emilio G. Cota
  2018-02-18  3:10   ` Stafford Horne
@ 2018-02-21 18:22   ` Richard Henderson
  1 sibling, 0 replies; 8+ messages in thread
From: Richard Henderson @ 2018-02-21 18:22 UTC (permalink / raw)
  To: Emilio G. Cota, qemu-devel; +Cc: Stafford Horne, Richard Henderson

On 02/17/2018 05:32 PM, Emilio G. Cota wrote:
> Signed-off-by: Emilio G. Cota <cota@braap.org>
> ---
>  target/openrisc/translate.c | 87 ++++++++++++++++++++++-----------------------
>  1 file changed, 43 insertions(+), 44 deletions(-)
> 
> diff --git a/target/openrisc/translate.c b/target/openrisc/translate.c
> index 2747b24..0450144 100644
> --- a/target/openrisc/translate.c
> +++ b/target/openrisc/translate.c
> @@ -36,7 +36,8 @@
>  #include "exec/log.h"
>  
>  #define LOG_DIS(str, ...) \
> -    qemu_log_mask(CPU_LOG_TB_IN_ASM, "%08x: " str, dc->pc, ## __VA_ARGS__)
> +    qemu_log_mask(CPU_LOG_TB_IN_ASM, "%08x: " str, dc->base.pc_next,    \
> +                  ## __VA_ARGS__)
>  
>  /* is_jmp field values */
>  #define DISAS_JUMP    DISAS_TARGET_0 /* only pc was modified dynamically */
> @@ -44,13 +45,10 @@
>  #define DISAS_TB_JUMP DISAS_TARGET_2 /* only pc was modified statically */
>  
>  typedef struct DisasContext {
> -    TranslationBlock *tb;
> -    target_ulong pc;
> -    uint32_t is_jmp;
> +    DisasContextBase base;
>      uint32_t mem_idx;
>      uint32_t tb_flags;
>      uint32_t delayed_branch;
> -    bool singlestep_enabled;
>  } DisasContext;
>  
>  static TCGv cpu_sr;
> @@ -126,9 +124,9 @@ static void gen_exception(DisasContext *dc, unsigned int excp)
>  
>  static void gen_illegal_exception(DisasContext *dc)
>  {
> -    tcg_gen_movi_tl(cpu_pc, dc->pc);
> +    tcg_gen_movi_tl(cpu_pc, dc->base.pc_next);
>      gen_exception(dc, EXCP_ILLEGAL);
> -    dc->is_jmp = DISAS_UPDATE;
> +    dc->base.is_jmp = DISAS_UPDATE;

Should be NORETURN.

> @@ -1254,14 +1252,14 @@ static void dec_sys(DisasContext *dc, uint32_t insn)
>      switch (op0) {
>      case 0x000:    /* l.sys */
>          LOG_DIS("l.sys %d\n", K16);
> -        tcg_gen_movi_tl(cpu_pc, dc->pc);
> +        tcg_gen_movi_tl(cpu_pc, dc->base.pc_next);
>          gen_exception(dc, EXCP_SYSCALL);
> -        dc->is_jmp = DISAS_UPDATE;
> +        dc->base.is_jmp = DISAS_UPDATE;

Likewise.


r~

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH 2/2] target/openrisc: convert to TranslatorOps
  2018-02-18  1:32 ` [Qemu-devel] [PATCH 2/2] target/openrisc: convert to TranslatorOps Emilio G. Cota
  2018-02-18  3:13   ` Stafford Horne
@ 2018-02-21 18:24   ` Richard Henderson
  1 sibling, 0 replies; 8+ messages in thread
From: Richard Henderson @ 2018-02-21 18:24 UTC (permalink / raw)
  To: Emilio G. Cota, qemu-devel; +Cc: Stafford Horne, Richard Henderson

On 02/17/2018 05:32 PM, Emilio G. Cota wrote:
> +    dc->next_page_start = (dc->base.pc_first & TARGET_PAGE_MASK) +
> +        TARGET_PAGE_SIZE;
> +    return max_insns;

OpenRISC is standard 4-byte risc insn; should use the same bound mechanism as
ppc and sparc.


r~

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2018-02-21 18:24 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-18  1:32 [Qemu-devel] [PATCH 0/2] target/openrisc: translator loop conversion Emilio G. Cota
2018-02-18  1:32 ` [Qemu-devel] [PATCH 1/2] target/openrisc: convert to DisasContextBase Emilio G. Cota
2018-02-18  3:10   ` Stafford Horne
2018-02-18  6:11     ` Emilio G. Cota
2018-02-21 18:22   ` Richard Henderson
2018-02-18  1:32 ` [Qemu-devel] [PATCH 2/2] target/openrisc: convert to TranslatorOps Emilio G. Cota
2018-02-18  3:13   ` Stafford Horne
2018-02-21 18:24   ` Richard Henderson

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.