[Qemu-devel] [PATCH 0/3] Alternative post-instruction early TB termination

[Qemu-devel] [PATCH 0/3] Alternative post-instruction early TB termination

[Jan Kiszka]

Originally I was hoping to get my whole debugger patch series on the
track, but I'm still stuck with a bug in the x86 debug register support
(weird single step race, triggered by dr-usage). So let's start smaller
with a friction of that series.

These patches introduce a new single step mode that allows the emulator
to generate and execute only a single-instruction TB, but without
triggering a debug event afterwards. This is exploited by
tb_invalidate_phys_page[_range] and later on by the watchpoint subsystem
(patch to be posted). This should also allow to remove cflags
from TranslationBlock, as done by the third patch.

Jan

[Qemu-devel] [PATCH 3/3] remove unused TB cflags

[Jan Kiszka]

TranslationBlock.cflags is unused, now that no one is interested in
CF_SINGLE_INSN anymore. Also the other CF_* flags have no users, so
let's clean this up.

Signed-off-by: Jan Kiszka <jan.kiszka@web.de>
---
 exec-all.h              |    4 ----
 exec.c                  |    7 ++-----
 target-i386/translate.c |    6 ++----
 3 files changed, 4 insertions(+), 13 deletions(-)

Index: b/exec-all.h
===================================================================
--- a/exec-all.h
+++ b/exec-all.h
@@ -123,10 +123,6 @@ typedef struct TranslationBlock {
     uint64_t flags; /* flags defining in which context the code was generated */
     uint16_t size;      /* size of target code for this block (1 <=
                            size <= TARGET_PAGE_SIZE) */
-    uint16_t cflags;    /* compile flags */
-#define CF_TB_FP_USED  0x0002 /* fp ops are used in the TB */
-#define CF_FP_USED     0x0004 /* fp ops are used in the TB or in a chained TB */
-#define CF_SINGLE_INSN 0x0008 /* compile only a single instruction */
 
     uint8_t *tc_ptr;    /* pointer to the translated code */
     /* next matching tb for physical address. */
Index: b/exec.c
===================================================================
--- a/exec.c
+++ b/exec.c
@@ -781,8 +781,7 @@ void tb_invalidate_phys_page_range(targe
                     current_tb = tb_find_pc(env->mem_write_pc);
                 }
             }
-            if (current_tb == tb &&
-                !(current_tb->cflags & CF_SINGLE_INSN)) {
+            if (current_tb == tb) {
                 /* If we are modifying the current TB, we must stop
                 its execution. We could be more precise by checking
                 that the modification is after the current PC, but it
@@ -899,8 +898,7 @@ static void tb_invalidate_phys_page(targ
         n = (long)tb & 3;
         tb = (TranslationBlock *)((long)tb & ~3);
 #ifdef TARGET_HAS_PRECISE_SMC
-        if (current_tb == tb &&
-            !(current_tb->cflags & CF_SINGLE_INSN)) {
+        if (current_tb == tb) {
                 /* If we are modifying the current TB, we must stop
                    its execution. We could be more precise by checking
                    that the modification is after the current PC, but it
@@ -1002,7 +1000,6 @@ TranslationBlock *tb_alloc(target_ulong 
         return NULL;
     tb = &tbs[nb_tbs++];
     tb->pc = pc;
-    tb->cflags = 0;
     return tb;
 }
 
Index: b/target-i386/translate.c
===================================================================
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -7101,7 +7101,7 @@ static inline int gen_intermediate_code_
     DisasContext dc1, *dc = &dc1;
     target_ulong pc_ptr;
     uint16_t *gen_opc_end;
-    int j, lj, cflags;
+    int j, lj;
     uint64_t flags;
     target_ulong pc_start;
     target_ulong cs_base;
@@ -7110,7 +7110,6 @@ static inline int gen_intermediate_code_
     pc_start = tb->pc;
     cs_base = tb->cs_base;
     flags = tb->flags;
-    cflags = tb->cflags;
 
     dc->pe = (flags >> HF_PE_SHIFT) & 1;
     dc->code32 = (flags >> HF_CS32_SHIFT) & 1;
@@ -7206,8 +7205,7 @@ static inline int gen_intermediate_code_
            the flag and abort the translation to give the irqs a
            change to be happen */
         if (dc->tf || dc->singlestep_enabled ||
-            (flags & HF_INHIBIT_IRQ_MASK) ||
-            (cflags & CF_SINGLE_INSN)) {
+            (flags & HF_INHIBIT_IRQ_MASK)) {
             gen_jmp_im(pc_ptr - dc->cs_base);
             gen_eob(dc);
             break;

[Qemu-devel] [PATCH 3/3] remove unused TB cflags - v2

[Jan Kiszka]

[ updated after fixing patch 2 ]

TranslationBlock.cflags is unused, now that no one is interested in
CF_SINGLE_INSN anymore. Also the other CF_* flags have no users, so
let's clean this up.

Signed-off-by: Jan Kiszka <jan.kiszka@web.de>
---
 exec-all.h              |    4 ----
 exec.c                  |    1 -
 target-i386/translate.c |    6 ++----
 3 files changed, 2 insertions(+), 9 deletions(-)

Index: b/exec-all.h
===================================================================
--- a/exec-all.h
+++ b/exec-all.h
@@ -123,10 +123,6 @@ typedef struct TranslationBlock {
     uint64_t flags; /* flags defining in which context the code was generated */
     uint16_t size;      /* size of target code for this block (1 <=
                            size <= TARGET_PAGE_SIZE) */
-    uint16_t cflags;    /* compile flags */
-#define CF_TB_FP_USED  0x0002 /* fp ops are used in the TB */
-#define CF_FP_USED     0x0004 /* fp ops are used in the TB or in a chained TB */
-#define CF_SINGLE_INSN 0x0008 /* compile only a single instruction */
 
     uint8_t *tc_ptr;    /* pointer to the translated code */
     /* next matching tb for physical address. */
Index: b/exec.c
===================================================================
--- a/exec.c
+++ b/exec.c
@@ -978,7 +978,6 @@ TranslationBlock *tb_alloc(target_ulong 
         return NULL;
     tb = &tbs[nb_tbs++];
     tb->pc = pc;
-    tb->cflags = 0;
     return tb;
 }
 
Index: b/target-i386/translate.c
===================================================================
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -7101,7 +7101,7 @@ static inline int gen_intermediate_code_
     DisasContext dc1, *dc = &dc1;
     target_ulong pc_ptr;
     uint16_t *gen_opc_end;
-    int j, lj, cflags;
+    int j, lj;
     uint64_t flags;
     target_ulong pc_start;
     target_ulong cs_base;
@@ -7110,7 +7110,6 @@ static inline int gen_intermediate_code_
     pc_start = tb->pc;
     cs_base = tb->cs_base;
     flags = tb->flags;
-    cflags = tb->cflags;
 
     dc->pe = (flags >> HF_PE_SHIFT) & 1;
     dc->code32 = (flags >> HF_CS32_SHIFT) & 1;
@@ -7206,8 +7205,7 @@ static inline int gen_intermediate_code_
            the flag and abort the translation to give the irqs a
            change to be happen */
         if (dc->tf || dc->singlestep_enabled ||
-            (flags & HF_INHIBIT_IRQ_MASK) ||
-            (cflags & CF_SINGLE_INSN)) {
+            (flags & HF_INHIBIT_IRQ_MASK)) {
             gen_jmp_im(pc_ptr - dc->cs_base);
             gen_eob(dc);
             break;

[Qemu-devel] [PATCH 1/3] Introduce SSTEP_INTERNAL

[Jan Kiszka]

Introducing SSTEP_INTERNAL, this patch allows to reuse the
(host-injected) single-step infrastructure to let the emulator generate
and execute TBs that only include one instruction.

Signed-off-by: Jan Kiszka <jan.kiszka@web.de>
---
 cpu-all.h               |    7 ++++---
 cpu-exec.c              |    4 +++-
 exec.c                  |    2 ++
 gdbstub.c               |    4 ++--
 target-arm/translate.c  |    2 +-
 target-cris/translate.c |    2 +-
 target-i386/translate.c |    2 +-
 target-m68k/translate.c |    4 ++--
 target-mips/translate.c |    2 +-
 target-ppc/translate.c  |    2 +-
 target-sh4/translate.c  |    6 +++---
 vl.c                    |    7 ++++---
 12 files changed, 25 insertions(+), 19 deletions(-)

Index: b/cpu-all.h
===================================================================
--- a/cpu-all.h
+++ b/cpu-all.h
@@ -804,9 +804,10 @@ int cpu_breakpoint_insert(CPUState *env,
 int cpu_breakpoint_remove(CPUState *env, target_ulong pc);
 void cpu_breakpoint_remove_all(CPUState *env);
 
-#define SSTEP_ENABLE  0x1  /* Enable simulated HW single stepping */
-#define SSTEP_NOIRQ   0x2  /* Do not use IRQ while single stepping */
-#define SSTEP_NOTIMER 0x4  /* Do not Timers while single stepping */
+#define SSTEP_DEBUG    0x1  /* Enable simulated HW single stepping */
+#define SSTEP_NOIRQ    0x2  /* Do not use IRQ while single stepping */
+#define SSTEP_NOTIMER  0x4  /* Do not Timers while single stepping */
+#define SSTEP_INTERNAL 0x8  /* QEMU internal, do not generate breakpoint */
 
 void cpu_single_step(CPUState *env, int enabled);
 void cpu_reset(CPUState *s);
Index: b/cpu-exec.c
===================================================================
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -369,7 +369,8 @@ int cpu_exec(CPUState *env1)
             for(;;) {
                 interrupt_request = env->interrupt_request;
                 if (__builtin_expect(interrupt_request, 0) &&
-                    likely(!(env->singlestep_enabled & SSTEP_NOIRQ))) {
+                    likely(!(env->singlestep_enabled &
+                             (SSTEP_NOIRQ | SSTEP_INTERNAL)))) {
                     if (interrupt_request & CPU_INTERRUPT_DEBUG) {
                         env->interrupt_request &= ~CPU_INTERRUPT_DEBUG;
                         env->exception_index = EXCP_DEBUG;
@@ -609,6 +610,7 @@ int cpu_exec(CPUState *env1)
 #endif
                 next_tb = tcg_qemu_tb_exec(tc_ptr);
                 env->current_tb = NULL;
+                env->singlestep_enabled &= ~SSTEP_INTERNAL;
                 /* reset soft MMU for next block (it can currently
                    only be set by a memory fault) */
 #if defined(USE_KQEMU)
Index: b/exec.c
===================================================================
--- a/exec.c
+++ b/exec.c
@@ -1292,6 +1292,8 @@ int cpu_breakpoint_remove(CPUState *env,
 void cpu_single_step(CPUState *env, int enabled)
 {
 #if defined(TARGET_HAS_ICE)
+    enabled &= SSTEP_DEBUG | SSTEP_NOIRQ | SSTEP_NOTIMER;
+    enabled |= env->singlestep_enabled & SSTEP_INTERNAL;
     if (env->singlestep_enabled != enabled) {
         env->singlestep_enabled = enabled;
         /* must flush all the translated code to avoid inconsistancies */
Index: b/target-arm/translate.c
===================================================================
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -8666,7 +8666,7 @@ static inline int gen_intermediate_code_
     /* At this stage dc->condjmp will only be set when the skipped
        instruction was a conditional branch or trap, and the PC has
        already been written.  */
-    if (__builtin_expect(env->singlestep_enabled, 0)) {
+    if (__builtin_expect(env->singlestep_enabled & SSTEP_DEBUG, 0)) {
         /* Make sure the pc is updated, and raise a debug exception.  */
         if (dc->condjmp) {
             gen_set_condexec(dc);
Index: b/target-cris/translate.c
===================================================================
--- a/target-cris/translate.c
+++ b/target-cris/translate.c
@@ -3067,7 +3067,7 @@ gen_intermediate_code_internal(CPUState 
 
 	cris_evaluate_flags (dc);
   done:
-	if (__builtin_expect(env->singlestep_enabled, 0)) {
+	if (__builtin_expect(env->singlestep_enabled & SSTEP_DEBUG, 0)) {
 		t_gen_raise_exception(EXCP_DEBUG);
 	} else {
 		switch(dc->is_jmp) {
Index: b/target-i386/translate.c
===================================================================
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -2618,7 +2618,7 @@ static void gen_eob(DisasContext *s)
     if (s->tb->flags & HF_INHIBIT_IRQ_MASK) {
         tcg_gen_helper_0_0(helper_reset_inhibit_irq);
     }
-    if (s->singlestep_enabled) {
+    if (s->singlestep_enabled & SSTEP_DEBUG) {
         tcg_gen_helper_0_0(helper_debug);
     } else if (s->tf) {
 	tcg_gen_helper_0_0(helper_single_step);
Index: b/target-m68k/translate.c
===================================================================
--- a/target-m68k/translate.c
+++ b/target-m68k/translate.c
@@ -871,7 +871,7 @@ static void gen_jmp_tb(DisasContext *s, 
     TranslationBlock *tb;
 
     tb = s->tb;
-    if (__builtin_expect (s->singlestep_enabled, 0)) {
+    if (__builtin_expect (s->singlestep_enabled & SSTEP_DEBUG, 0)) {
         gen_exception(s, dest, EXCP_DEBUG);
     } else if ((tb->pc & TARGET_PAGE_MASK) == (dest & TARGET_PAGE_MASK) ||
                (s->pc & TARGET_PAGE_MASK) == (dest & TARGET_PAGE_MASK)) {
@@ -2974,7 +2974,7 @@ gen_intermediate_code_internal(CPUState 
              !env->singlestep_enabled &&
              (pc_offset) < (TARGET_PAGE_SIZE - 32));
 
-    if (__builtin_expect(env->singlestep_enabled, 0)) {
+    if (__builtin_expect(env->singlestep_enabled & SSTEP_DEBUG, 0)) {
         /* Make sure the pc is updated, and raise a debug exception.  */
         if (!dc->is_jmp) {
             gen_flush_cc_op(dc);
Index: b/target-mips/translate.c
===================================================================
--- a/target-mips/translate.c
+++ b/target-mips/translate.c
@@ -7259,7 +7259,7 @@ gen_intermediate_code_internal (CPUState
         break;
 #endif
     }
-    if (env->singlestep_enabled) {
+    if (env->singlestep_enabled & SSTEP_DEBUG) {
         save_cpu_state(&ctx, ctx.bstate == BS_NONE);
         gen_op_debug();
     } else {
Index: b/target-ppc/translate.c
===================================================================
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -6319,7 +6319,7 @@ static always_inline int gen_intermediat
     if (ctx.exception == POWERPC_EXCP_NONE) {
         gen_goto_tb(&ctx, 0, ctx.nip);
     } else if (ctx.exception != POWERPC_EXCP_BRANCH) {
-        if (unlikely(env->singlestep_enabled)) {
+        if (unlikely(env->singlestep_enabled & SSTEP_DEBUG)) {
             gen_update_nip(&ctx, ctx.nip);
             gen_op_debug();
         }
Index: b/target-sh4/translate.c
===================================================================
--- a/target-sh4/translate.c
+++ b/target-sh4/translate.c
@@ -161,7 +161,7 @@ static void gen_goto_tb(DisasContext * c
         tcg_gen_exit_tb((long) tb + n);
     } else {
         gen_op_movl_imm_PC(dest);
-        if (ctx->singlestep_enabled)
+        if (ctx->singlestep_enabled & SSTEP_DEBUG)
             gen_op_debug();
         tcg_gen_exit_tb(0);
     }
@@ -173,7 +173,7 @@ static void gen_jump(DisasContext * ctx)
 	/* Target is not statically known, it comes necessarily from a
 	   delayed jump as immediate jump are conditinal jumps */
 	gen_op_movl_delayed_pc_PC();
-	if (ctx->singlestep_enabled)
+	if (ctx->singlestep_enabled & SSTEP_DEBUG)
 	    gen_op_debug();
 	tcg_gen_exit_tb(0);
     } else {
@@ -1251,7 +1251,7 @@ gen_intermediate_code_internal(CPUState 
 	break;
 #endif
     }
-    if (env->singlestep_enabled) {
+    if (env->singlestep_enabled & SSTEP_DEBUG) {
         gen_op_debug();
     } else {
 	switch (ctx.bstate) {
Index: b/vl.c
===================================================================
--- a/vl.c
+++ b/vl.c
@@ -7032,9 +7032,10 @@ void main_loop_wait(int timeout)
     qemu_aio_poll();
 
     if (vm_running) {
-        if (likely(!(cur_cpu->singlestep_enabled & SSTEP_NOTIMER)))
-        qemu_run_timers(&active_timers[QEMU_TIMER_VIRTUAL],
-                        qemu_get_clock(vm_clock));
+        if (likely(!(cur_cpu->singlestep_enabled &
+                     (SSTEP_NOTIMER | SSTEP_INTERNAL))))
+            qemu_run_timers(&active_timers[QEMU_TIMER_VIRTUAL],
+                            qemu_get_clock(vm_clock));
         /* run dma transfers, if any */
         DMA_run();
     }
Index: b/gdbstub.c
===================================================================
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -77,7 +77,7 @@ typedef struct GDBState {
 /* By default use no IRQs and no timers while single stepping so as to
  * make single stepping like an ICE HW step.
  */
-static int sstep_flags = SSTEP_ENABLE|SSTEP_NOIRQ|SSTEP_NOTIMER;
+static int sstep_flags = SSTEP_DEBUG | SSTEP_NOIRQ | SSTEP_NOTIMER;
 
 #ifdef CONFIG_USER_ONLY
 /* XXX: This is not thread safe.  Do we care?  */
@@ -1144,7 +1144,7 @@ static int gdb_handle_packet(GDBState *s
         if (!strcmp(p,"qemu.sstepbits")) {
             /* Query Breakpoint bit definitions */
             sprintf(buf,"ENABLE=%x,NOIRQ=%x,NOTIMER=%x",
-                    SSTEP_ENABLE,
+                    SSTEP_DEBUG,
                     SSTEP_NOIRQ,
                     SSTEP_NOTIMER);
             put_packet(s, buf);

[Qemu-devel] [PATCH 2/3] Replace CF_SINGLE_INSN with SSTEP_INTERNAL

[Jan Kiszka]

With the help of SSTEP_INTERNAL, we can overcome CF_SINGLE_INSN and,
thus, tb_gen_code.

Signed-off-by: Jan Kiszka <jan.kiszka@web.de>
---
 exec.c |   43 ++-----------------------------------------
 1 file changed, 2 insertions(+), 41 deletions(-)

Index: b/exec.c
===================================================================
--- a/exec.c
+++ b/exec.c
@@ -723,43 +723,6 @@ static void build_page_bitmap(PageDesc *
     }
 }
 
-#ifdef TARGET_HAS_PRECISE_SMC
-
-static void tb_gen_code(CPUState *env,
-                        target_ulong pc, target_ulong cs_base, int flags,
-                        int cflags)
-{
-    TranslationBlock *tb;
-    uint8_t *tc_ptr;
-    target_ulong phys_pc, phys_page2, virt_page2;
-    int code_gen_size;
-
-    phys_pc = get_phys_addr_code(env, pc);
-    tb = tb_alloc(pc);
-    if (!tb) {
-        /* flush must be done */
-        tb_flush(env);
-        /* cannot fail at this point */
-        tb = tb_alloc(pc);
-    }
-    tc_ptr = code_gen_ptr;
-    tb->tc_ptr = tc_ptr;
-    tb->cs_base = cs_base;
-    tb->flags = flags;
-    tb->cflags = cflags;
-    cpu_gen_code(env, tb, &code_gen_size);
-    code_gen_ptr = (void *)(((unsigned long)code_gen_ptr + code_gen_size + CODE_GEN_ALIGN - 1) & ~(CODE_GEN_ALIGN - 1));
-
-    /* check next page if needed */
-    virt_page2 = (pc + tb->size - 1) & TARGET_PAGE_MASK;
-    phys_page2 = -1;
-    if ((pc & TARGET_PAGE_MASK) != virt_page2) {
-        phys_page2 = get_phys_addr_code(env, virt_page2);
-    }
-    tb_link_phys(tb, phys_pc, phys_page2);
-}
-#endif
-
 /* invalidate all TBs which intersect with the target physical page
    starting in range [start;end[. NOTE: start and end must refer to
    the same physical page. 'is_cpu_write_access' should be true if called
@@ -870,8 +833,7 @@ void tb_invalidate_phys_page_range(targe
            modifying the memory. It will ensure that it cannot modify
            itself */
         env->current_tb = NULL;
-        tb_gen_code(env, current_pc, current_cs_base, current_flags,
-                    CF_SINGLE_INSN);
+        env->singlestep_enabled |= SSTEP_INTERNAL;
         cpu_resume_from_signal(env, NULL);
     }
 #endif
@@ -967,8 +929,7 @@ static void tb_invalidate_phys_page(targ
            modifying the memory. It will ensure that it cannot modify
            itself */
         env->current_tb = NULL;
-        tb_gen_code(env, current_pc, current_cs_base, current_flags,
-                    CF_SINGLE_INSN);
+        env->singlestep_enabled |= SSTEP_INTERNAL;
         cpu_resume_from_signal(env, puc);
     }
 #endif

[Qemu-devel] Re: [PATCH 2/3] Replace CF_SINGLE_INSN with SSTEP_INTERNAL - v2

[Jan Kiszka]

[ Missed even more cleanup possibilities. ]

With the help of SSTEP_INTERNAL, we can overcome CF_SINGLE_INSN and,
thus, tb_gen_code with its setup code.

Signed-off-by: Jan Kiszka <jan.kiszka@web.de>
---
 exec.c |   71 +++--------------------------------------------------------------
 1 file changed, 4 insertions(+), 67 deletions(-)

Index: b/exec.c
===================================================================
--- a/exec.c
+++ b/exec.c
@@ -723,43 +723,6 @@ static void build_page_bitmap(PageDesc *
     }
 }
 
-#ifdef TARGET_HAS_PRECISE_SMC
-
-static void tb_gen_code(CPUState *env,
-                        target_ulong pc, target_ulong cs_base, int flags,
-                        int cflags)
-{
-    TranslationBlock *tb;
-    uint8_t *tc_ptr;
-    target_ulong phys_pc, phys_page2, virt_page2;
-    int code_gen_size;
-
-    phys_pc = get_phys_addr_code(env, pc);
-    tb = tb_alloc(pc);
-    if (!tb) {
-        /* flush must be done */
-        tb_flush(env);
-        /* cannot fail at this point */
-        tb = tb_alloc(pc);
-    }
-    tc_ptr = code_gen_ptr;
-    tb->tc_ptr = tc_ptr;
-    tb->cs_base = cs_base;
-    tb->flags = flags;
-    tb->cflags = cflags;
-    cpu_gen_code(env, tb, &code_gen_size);
-    code_gen_ptr = (void *)(((unsigned long)code_gen_ptr + code_gen_size + CODE_GEN_ALIGN - 1) & ~(CODE_GEN_ALIGN - 1));
-
-    /* check next page if needed */
-    virt_page2 = (pc + tb->size - 1) & TARGET_PAGE_MASK;
-    phys_page2 = -1;
-    if ((pc & TARGET_PAGE_MASK) != virt_page2) {
-        phys_page2 = get_phys_addr_code(env, virt_page2);
-    }
-    tb_link_phys(tb, phys_pc, phys_page2);
-}
-#endif
-
 /* invalidate all TBs which intersect with the target physical page
    starting in range [start;end[. NOTE: start and end must refer to
    the same physical page. 'is_cpu_write_access' should be true if called
@@ -768,12 +731,11 @@ static void tb_gen_code(CPUState *env,
 void tb_invalidate_phys_page_range(target_phys_addr_t start, target_phys_addr_t end,
                                    int is_cpu_write_access)
 {
-    int n, current_tb_modified, current_tb_not_found, current_flags;
+    int n, current_tb_modified, current_tb_not_found;
     CPUState *env = cpu_single_env;
     PageDesc *p;
     TranslationBlock *tb, *tb_next, *current_tb, *saved_tb;
     target_ulong tb_start, tb_end;
-    target_ulong current_pc, current_cs_base;
 
     p = page_find(start >> TARGET_PAGE_BITS);
     if (!p)
@@ -790,9 +752,6 @@ void tb_invalidate_phys_page_range(targe
     current_tb_not_found = is_cpu_write_access;
     current_tb_modified = 0;
     current_tb = NULL; /* avoid warning */
-    current_pc = 0; /* avoid warning */
-    current_cs_base = 0; /* avoid warning */
-    current_flags = 0; /* avoid warning */
     tb = p->first_tb;
     while (tb != NULL) {
         n = (long)tb & 3;
@@ -829,14 +788,6 @@ void tb_invalidate_phys_page_range(targe
                 current_tb_modified = 1;
                 cpu_restore_state(current_tb, env,
                                   env->mem_write_pc, NULL);
-#if defined(TARGET_I386)
-                current_flags = env->hflags;
-                current_flags |= (env->eflags & (IOPL_MASK | TF_MASK | VM_MASK));
-                current_cs_base = (target_ulong)env->segs[R_CS].base;
-                current_pc = current_cs_base + env->eip;
-#else
-#error unsupported CPU
-#endif
             }
 #endif /* TARGET_HAS_PRECISE_SMC */
             /* we need to do that to handle the case where a signal
@@ -870,8 +821,7 @@ void tb_invalidate_phys_page_range(targe
            modifying the memory. It will ensure that it cannot modify
            itself */
         env->current_tb = NULL;
-        tb_gen_code(env, current_pc, current_cs_base, current_flags,
-                    CF_SINGLE_INSN);
+        env->singlestep_enabled |= SSTEP_INTERNAL;
         cpu_resume_from_signal(env, NULL);
     }
 #endif
@@ -910,8 +860,7 @@ static inline void tb_invalidate_phys_pa
 static void tb_invalidate_phys_page(target_phys_addr_t addr,
                                     unsigned long pc, void *puc)
 {
-    int n, current_flags, current_tb_modified;
-    target_ulong current_pc, current_cs_base;
+    int n, current_tb_modified;
     PageDesc *p;
     TranslationBlock *tb, *current_tb;
 #ifdef TARGET_HAS_PRECISE_SMC
@@ -925,9 +874,6 @@ static void tb_invalidate_phys_page(targ
     tb = p->first_tb;
     current_tb_modified = 0;
     current_tb = NULL;
-    current_pc = 0; /* avoid warning */
-    current_cs_base = 0; /* avoid warning */
-    current_flags = 0; /* avoid warning */
 #ifdef TARGET_HAS_PRECISE_SMC
     if (tb && pc != 0) {
         current_tb = tb_find_pc(pc);
@@ -947,14 +893,6 @@ static void tb_invalidate_phys_page(targ
 
             current_tb_modified = 1;
             cpu_restore_state(current_tb, env, pc, puc);
-#if defined(TARGET_I386)
-            current_flags = env->hflags;
-            current_flags |= (env->eflags & (IOPL_MASK | TF_MASK | VM_MASK));
-            current_cs_base = (target_ulong)env->segs[R_CS].base;
-            current_pc = current_cs_base + env->eip;
-#else
-#error unsupported CPU
-#endif
         }
 #endif /* TARGET_HAS_PRECISE_SMC */
         tb_phys_invalidate(tb, addr);
@@ -967,8 +905,7 @@ static void tb_invalidate_phys_page(targ
            modifying the memory. It will ensure that it cannot modify
            itself */
         env->current_tb = NULL;
-        tb_gen_code(env, current_pc, current_cs_base, current_flags,
-                    CF_SINGLE_INSN);
+        env->singlestep_enabled |= SSTEP_INTERNAL;
         cpu_resume_from_signal(env, puc);
     }
 #endif

Re: [Qemu-devel] [PATCH 2/3] Replace CF_SINGLE_INSN with SSTEP_INTERNAL

[Fabrice Bellard]

Jan Kiszka wrote:
> With the help of SSTEP_INTERNAL, we can overcome CF_SINGLE_INSN and,
> thus, tb_gen_code.

Does your code avoid looping if an instruction modifies itself ?

Fabrice.

[Qemu-devel] [PATCH 2/3] Replace CF_SINGLE_INSN with SSTEP_INTERNAL - v2

[Jan Kiszka]

Fabrice Bellard wrote:
> Jan Kiszka wrote:
>> With the help of SSTEP_INTERNAL, we can overcome CF_SINGLE_INSN and,
>> thus, tb_gen_code.
> 
> Does your code avoid looping if an instruction modifies itself ?

Err, well, of course not. Not completely understanding what I patched, I
missed the meaning of those two CF_SINGLE_INSN checks. Version below
gets this right. Successfully tested with qemu-x86_64 system&userspace.
An updated patch 3 will follow, too. Thanks!

------------

With the help of SSTEP_INTERNAL, we can overcome CF_SINGLE_INSN and,
thus, tb_gen_code with its setup code.

Signed-off-by: Jan Kiszka <jan.kiszka@web.de>
---
 exec.c |   75 +++++------------------------------------------------------------
 1 file changed, 6 insertions(+), 69 deletions(-)

Index: b/exec.c
===================================================================
--- a/exec.c
+++ b/exec.c
@@ -723,43 +723,6 @@ static void build_page_bitmap(PageDesc *
     }
 }
 
-#ifdef TARGET_HAS_PRECISE_SMC
-
-static void tb_gen_code(CPUState *env,
-                        target_ulong pc, target_ulong cs_base, int flags,
-                        int cflags)
-{
-    TranslationBlock *tb;
-    uint8_t *tc_ptr;
-    target_ulong phys_pc, phys_page2, virt_page2;
-    int code_gen_size;
-
-    phys_pc = get_phys_addr_code(env, pc);
-    tb = tb_alloc(pc);
-    if (!tb) {
-        /* flush must be done */
-        tb_flush(env);
-        /* cannot fail at this point */
-        tb = tb_alloc(pc);
-    }
-    tc_ptr = code_gen_ptr;
-    tb->tc_ptr = tc_ptr;
-    tb->cs_base = cs_base;
-    tb->flags = flags;
-    tb->cflags = cflags;
-    cpu_gen_code(env, tb, &code_gen_size);
-    code_gen_ptr = (void *)(((unsigned long)code_gen_ptr + code_gen_size + CODE_GEN_ALIGN - 1) & ~(CODE_GEN_ALIGN - 1));
-
-    /* check next page if needed */
-    virt_page2 = (pc + tb->size - 1) & TARGET_PAGE_MASK;
-    phys_page2 = -1;
-    if ((pc & TARGET_PAGE_MASK) != virt_page2) {
-        phys_page2 = get_phys_addr_code(env, virt_page2);
-    }
-    tb_link_phys(tb, phys_pc, phys_page2);
-}
-#endif
-
 /* invalidate all TBs which intersect with the target physical page
    starting in range [start;end[. NOTE: start and end must refer to
    the same physical page. 'is_cpu_write_access' should be true if called
@@ -768,12 +731,11 @@ static void tb_gen_code(CPUState *env,
 void tb_invalidate_phys_page_range(target_phys_addr_t start, target_phys_addr_t end,
                                    int is_cpu_write_access)
 {
-    int n, current_tb_modified, current_tb_not_found, current_flags;
+    int n, current_tb_modified, current_tb_not_found;
     CPUState *env = cpu_single_env;
     PageDesc *p;
     TranslationBlock *tb, *tb_next, *current_tb, *saved_tb;
     target_ulong tb_start, tb_end;
-    target_ulong current_pc, current_cs_base;
 
     p = page_find(start >> TARGET_PAGE_BITS);
     if (!p)
@@ -790,9 +752,6 @@ void tb_invalidate_phys_page_range(targe
     current_tb_not_found = is_cpu_write_access;
     current_tb_modified = 0;
     current_tb = NULL; /* avoid warning */
-    current_pc = 0; /* avoid warning */
-    current_cs_base = 0; /* avoid warning */
-    current_flags = 0; /* avoid warning */
     tb = p->first_tb;
     while (tb != NULL) {
         n = (long)tb & 3;
@@ -819,7 +778,7 @@ void tb_invalidate_phys_page_range(targe
                 }
             }
             if (current_tb == tb &&
-                !(current_tb->cflags & CF_SINGLE_INSN)) {
+                !(env->singlestep_enabled & SSTEP_INTERNAL)) {
                 /* If we are modifying the current TB, we must stop
                 its execution. We could be more precise by checking
                 that the modification is after the current PC, but it
@@ -829,14 +788,6 @@ void tb_invalidate_phys_page_range(targe
                 current_tb_modified = 1;
                 cpu_restore_state(current_tb, env,
                                   env->mem_write_pc, NULL);
-#if defined(TARGET_I386)
-                current_flags = env->hflags;
-                current_flags |= (env->eflags & (IOPL_MASK | TF_MASK | VM_MASK));
-                current_cs_base = (target_ulong)env->segs[R_CS].base;
-                current_pc = current_cs_base + env->eip;
-#else
-#error unsupported CPU
-#endif
             }
 #endif /* TARGET_HAS_PRECISE_SMC */
             /* we need to do that to handle the case where a signal
@@ -870,8 +821,7 @@ void tb_invalidate_phys_page_range(targe
            modifying the memory. It will ensure that it cannot modify
            itself */
         env->current_tb = NULL;
-        tb_gen_code(env, current_pc, current_cs_base, current_flags,
-                    CF_SINGLE_INSN);
+        env->singlestep_enabled |= SSTEP_INTERNAL;
         cpu_resume_from_signal(env, NULL);
     }
 #endif
@@ -910,8 +860,7 @@ static inline void tb_invalidate_phys_pa
 static void tb_invalidate_phys_page(target_phys_addr_t addr,
                                     unsigned long pc, void *puc)
 {
-    int n, current_flags, current_tb_modified;
-    target_ulong current_pc, current_cs_base;
+    int n, current_tb_modified;
     PageDesc *p;
     TranslationBlock *tb, *current_tb;
 #ifdef TARGET_HAS_PRECISE_SMC
@@ -925,9 +874,6 @@ static void tb_invalidate_phys_page(targ
     tb = p->first_tb;
     current_tb_modified = 0;
     current_tb = NULL;
-    current_pc = 0; /* avoid warning */
-    current_cs_base = 0; /* avoid warning */
-    current_flags = 0; /* avoid warning */
 #ifdef TARGET_HAS_PRECISE_SMC
     if (tb && pc != 0) {
         current_tb = tb_find_pc(pc);
@@ -938,7 +884,7 @@ static void tb_invalidate_phys_page(targ
         tb = (TranslationBlock *)((long)tb & ~3);
 #ifdef TARGET_HAS_PRECISE_SMC
         if (current_tb == tb &&
-            !(current_tb->cflags & CF_SINGLE_INSN)) {
+            !(env->singlestep_enabled & SSTEP_INTERNAL)) {
                 /* If we are modifying the current TB, we must stop
                    its execution. We could be more precise by checking
                    that the modification is after the current PC, but it
@@ -947,14 +893,6 @@ static void tb_invalidate_phys_page(targ
 
             current_tb_modified = 1;
             cpu_restore_state(current_tb, env, pc, puc);
-#if defined(TARGET_I386)
-            current_flags = env->hflags;
-            current_flags |= (env->eflags & (IOPL_MASK | TF_MASK | VM_MASK));
-            current_cs_base = (target_ulong)env->segs[R_CS].base;
-            current_pc = current_cs_base + env->eip;
-#else
-#error unsupported CPU
-#endif
         }
 #endif /* TARGET_HAS_PRECISE_SMC */
         tb_phys_invalidate(tb, addr);
@@ -967,8 +905,7 @@ static void tb_invalidate_phys_page(targ
            modifying the memory. It will ensure that it cannot modify
            itself */
         env->current_tb = NULL;
-        tb_gen_code(env, current_pc, current_cs_base, current_flags,
-                    CF_SINGLE_INSN);
+        env->singlestep_enabled |= SSTEP_INTERNAL;
         cpu_resume_from_signal(env, puc);
     }
 #endif