[PATCH for-6.1 v6 00/17] tcg: breakpoint reorg

[PATCH for-6.1 v6 00/17] tcg: breakpoint reorg

[Richard Henderson]

This is fixing #404 ("windows xp boot takes much longer...")
and several other similar reports.

Changes for v6:
  * Reinstate accidental loss of singlestep overriding breakpoint check.
    Shows up in the record-replay avocado tests failing to make progress.
  * Add CPUState.gdb_adjust_breakpoint hook for AVR weirdness.

Changes for v5:
  * Include missing hunk in tb_gen_code, as noted in reply to v4.
  * Remove helper_check_breakpoints from target/arm/.
  * Reorg cflags_for_breakpoints into check_for_breakpoints;
    reorg cpu_exec to use a break instead of a longjmp.
  * Move singlestep_enabled check from cflags_for_breakpoints
    to curr_cflags, which makes cpu_exec_step_atomic cleaner.

Changes for v4:
  * Issue breakpoints directly from cflags_for_breakpoints.
    Do not generate code for a TB beginning with a BP at all.
  * Drop the problematic TranslatorOps.breakpoint_check hook entirely.

Changes for v3:
  * Map CF_COUNT_MASK == 0 -> TCG_MAX_INSNS.
  * Split out *_breakpoint_check fixes for avr, mips, riscv.

Changes for v2:
  * All prerequisites and 7 of the patches from v1 with are merged.

Patches lacking review:
  11-hw-core-Introduce-CPUClass.gdb_adjust_breakpoint.patch
  12-target-avr-Implement-gdb_adjust_breakpoint.patch
  15-accel-tcg-Remove-TranslatorOps.breakpoint_check.patch
  17-accel-tcg-Record-singlestep_enabled-in-tb-cflags.patch


r~


Richard Henderson (17):
  accel/tcg: Reduce CF_COUNT_MASK to match TCG_MAX_INSNS
  accel/tcg: Move curr_cflags into cpu-exec.c
  target/alpha: Drop goto_tb path in gen_call_pal
  accel/tcg: Add CF_NO_GOTO_TB and CF_NO_GOTO_PTR
  accel/tcg: Drop CF_NO_GOTO_PTR from -d nochain
  accel/tcg: Handle -singlestep in curr_cflags
  accel/tcg: Use CF_NO_GOTO_{TB, PTR} in cpu_exec_step_atomic
  hw/core: Introduce TCGCPUOps.debug_check_breakpoint
  target/arm: Implement debug_check_breakpoint
  target/i386: Implement debug_check_breakpoint
  hw/core: Introduce CPUClass.gdb_adjust_breakpoint
  target/avr: Implement gdb_adjust_breakpoint
  accel/tcg: Merge tb_find into its only caller
  accel/tcg: Move breakpoint recognition outside translation
  accel/tcg: Remove TranslatorOps.breakpoint_check
  accel/tcg: Hoist tb_cflags to a local in translator_loop
  accel/tcg: Record singlestep_enabled in tb->cflags

 include/exec/exec-all.h       |  24 ++--
 include/exec/translator.h     |  11 --
 include/hw/core/cpu.h         |   4 +
 include/hw/core/tcg-cpu-ops.h |   6 +
 target/arm/helper.h           |   2 -
 target/arm/internals.h        |   3 +
 target/avr/cpu.h              |   1 +
 accel/tcg/cpu-exec.c          | 205 ++++++++++++++++++++++++++--------
 accel/tcg/translate-all.c     |   7 +-
 accel/tcg/translator.c        |  39 ++-----
 cpu.c                         |  34 ++----
 target/alpha/translate.c      |  31 +----
 target/arm/cpu.c              |   1 +
 target/arm/cpu_tcg.c          |   1 +
 target/arm/debug_helper.c     |  12 +-
 target/arm/translate-a64.c    |  25 -----
 target/arm/translate.c        |  29 -----
 target/avr/cpu.c              |   1 +
 target/avr/gdbstub.c          |  13 +++
 target/avr/translate.c        |  32 ------
 target/cris/translate.c       |  20 ----
 target/hexagon/translate.c    |  17 ---
 target/hppa/translate.c       |  11 --
 target/i386/tcg/tcg-cpu.c     |  12 ++
 target/i386/tcg/translate.c   |  28 -----
 target/m68k/translate.c       |  18 ---
 target/microblaze/translate.c |  18 ---
 target/mips/tcg/translate.c   |  19 ----
 target/nios2/translate.c      |  27 -----
 target/openrisc/translate.c   |  17 ---
 target/ppc/translate.c        |  18 ---
 target/riscv/translate.c      |  17 ---
 target/rx/translate.c         |  14 ---
 target/s390x/tcg/translate.c  |  24 ----
 target/sh4/translate.c        |  18 ---
 target/sparc/translate.c      |  17 ---
 target/tricore/translate.c    |  16 ---
 target/xtensa/translate.c     |  17 ---
 tcg/tcg-op.c                  |  28 ++---
 39 files changed, 248 insertions(+), 589 deletions(-)

-- 
2.25.1

[PATCH for-6.1 v6 01/17] accel/tcg: Reduce CF_COUNT_MASK to match TCG_MAX_INSNS

[Richard Henderson]

The space reserved for CF_COUNT_MASK was overly large.
Reduce to free up cflags bits and eliminate an extra test.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Message-Id: <20210717221851.2124573-2-richard.henderson@linaro.org>
---
 include/exec/exec-all.h   | 4 +++-
 accel/tcg/translate-all.c | 5 ++---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index 754f4130c9..dfe82ed19c 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -492,7 +492,9 @@ struct TranslationBlock {
     target_ulong cs_base; /* CS base for this block */
     uint32_t flags; /* flags defining in which context the code was generated */
     uint32_t cflags;    /* compile flags */
-#define CF_COUNT_MASK  0x00007fff
+
+/* Note that TCG_MAX_INSNS is 512; we validate this match elsewhere. */
+#define CF_COUNT_MASK  0x000001ff
 #define CF_LAST_IO     0x00008000 /* Last insn may be an IO access.  */
 #define CF_MEMI_ONLY   0x00010000 /* Only instrument memory ops */
 #define CF_USE_ICOUNT  0x00020000
diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index 4df26de858..5cc01d693b 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -1428,11 +1428,10 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
 
     max_insns = cflags & CF_COUNT_MASK;
     if (max_insns == 0) {
-        max_insns = CF_COUNT_MASK;
-    }
-    if (max_insns > TCG_MAX_INSNS) {
         max_insns = TCG_MAX_INSNS;
     }
+    QEMU_BUILD_BUG_ON(CF_COUNT_MASK + 1 != TCG_MAX_INSNS);
+
     if (cpu->singlestep_enabled || singlestep) {
         max_insns = 1;
     }
-- 
2.25.1

[PATCH for-6.1 v6 02/17] accel/tcg: Move curr_cflags into cpu-exec.c

[Richard Henderson]

We will shortly have more than a simple member read here,
with stuff not necessarily exposed to exec/exec-all.h.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Message-Id: <20210717221851.2124573-3-richard.henderson@linaro.org>
---
 include/exec/exec-all.h | 5 +----
 accel/tcg/cpu-exec.c    | 5 +++++
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index dfe82ed19c..ae7603ca75 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -565,10 +565,7 @@ static inline uint32_t tb_cflags(const TranslationBlock *tb)
 }
 
 /* current cflags for hashing/comparison */
-static inline uint32_t curr_cflags(CPUState *cpu)
-{
-    return cpu->tcg_cflags;
-}
+uint32_t curr_cflags(CPUState *cpu);
 
 /* TranslationBlock invalidate API */
 #if defined(CONFIG_USER_ONLY)
diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index e22bcb99f7..ef4214d893 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -145,6 +145,11 @@ static void init_delay_params(SyncClocks *sc, const CPUState *cpu)
 }
 #endif /* CONFIG USER ONLY */
 
+uint32_t curr_cflags(CPUState *cpu)
+{
+    return cpu->tcg_cflags;
+}
+
 /* Might cause an exception, so have a longjmp destination ready */
 static inline TranslationBlock *tb_lookup(CPUState *cpu, target_ulong pc,
                                           target_ulong cs_base,
-- 
2.25.1

[PATCH for-6.1 v6 03/17] target/alpha: Drop goto_tb path in gen_call_pal

[Richard Henderson]

We are certain of a page crossing here, entering the
PALcode image, so the call to use_goto_tb that should
have been here will never succeed.

We are shortly going to add an assert to tcg_gen_goto_tb
that would trigger for this case.

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/alpha/translate.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/target/alpha/translate.c b/target/alpha/translate.c
index 103c6326a2..949ba6ffde 100644
--- a/target/alpha/translate.c
+++ b/target/alpha/translate.c
@@ -1207,19 +1207,8 @@ static DisasJumpType gen_call_pal(DisasContext *ctx, int palcode)
                   ? 0x2000 + (palcode - 0x80) * 64
                   : 0x1000 + palcode * 64);
 
-        /* Since the destination is running in PALmode, we don't really
-           need the page permissions check.  We'll see the existence of
-           the page when we create the TB, and we'll flush all TBs if
-           we change the PAL base register.  */
-        if (!ctx->base.singlestep_enabled) {
-            tcg_gen_goto_tb(0);
-            tcg_gen_movi_i64(cpu_pc, entry);
-            tcg_gen_exit_tb(ctx->base.tb, 0);
-            return DISAS_NORETURN;
-        } else {
-            tcg_gen_movi_i64(cpu_pc, entry);
-            return DISAS_PC_UPDATED;
-        }
+        tcg_gen_movi_i64(cpu_pc, entry);
+        return DISAS_PC_UPDATED;
     }
 #endif
 }
-- 
2.25.1

[PATCH for-6.1 v6 04/17] accel/tcg: Add CF_NO_GOTO_TB and CF_NO_GOTO_PTR

[Richard Henderson]

Move the -d nochain check to bits on tb->cflags.
These will be used for more than -d nochain shortly.

Set bits during curr_cflags, test them in translator_use_goto_tb,
assert we're not doing anything odd in tcg_gen_goto_tb.  The test
in tcg_gen_exit_tb is redundant with the assert for goto_tb_issue_mask.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Message-Id: <20210717221851.2124573-4-richard.henderson@linaro.org>
---
 include/exec/exec-all.h | 16 +++++++++-------
 accel/tcg/cpu-exec.c    |  8 +++++++-
 accel/tcg/translator.c  |  5 +++++
 tcg/tcg-op.c            | 28 ++++++++++++----------------
 4 files changed, 33 insertions(+), 24 deletions(-)

diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index ae7603ca75..6873cce8df 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -494,13 +494,15 @@ struct TranslationBlock {
     uint32_t cflags;    /* compile flags */
 
 /* Note that TCG_MAX_INSNS is 512; we validate this match elsewhere. */
-#define CF_COUNT_MASK  0x000001ff
-#define CF_LAST_IO     0x00008000 /* Last insn may be an IO access.  */
-#define CF_MEMI_ONLY   0x00010000 /* Only instrument memory ops */
-#define CF_USE_ICOUNT  0x00020000
-#define CF_INVALID     0x00040000 /* TB is stale. Set with @jmp_lock held */
-#define CF_PARALLEL    0x00080000 /* Generate code for a parallel context */
-#define CF_CLUSTER_MASK 0xff000000 /* Top 8 bits are cluster ID */
+#define CF_COUNT_MASK    0x000001ff
+#define CF_NO_GOTO_TB    0x00000200 /* Do not chain with goto_tb */
+#define CF_NO_GOTO_PTR   0x00000400 /* Do not chain with goto_ptr */
+#define CF_LAST_IO       0x00008000 /* Last insn may be an IO access.  */
+#define CF_MEMI_ONLY     0x00010000 /* Only instrument memory ops */
+#define CF_USE_ICOUNT    0x00020000
+#define CF_INVALID       0x00040000 /* TB is stale. Set with @jmp_lock held */
+#define CF_PARALLEL      0x00080000 /* Generate code for a parallel context */
+#define CF_CLUSTER_MASK  0xff000000 /* Top 8 bits are cluster ID */
 #define CF_CLUSTER_SHIFT 24
 
     /* Per-vCPU dynamic tracing state used to generate this TB */
diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index ef4214d893..d3232d5764 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -147,7 +147,13 @@ static void init_delay_params(SyncClocks *sc, const CPUState *cpu)
 
 uint32_t curr_cflags(CPUState *cpu)
 {
-    return cpu->tcg_cflags;
+    uint32_t cflags = cpu->tcg_cflags;
+
+    if (qemu_loglevel_mask(CPU_LOG_TB_NOCHAIN)) {
+        cflags |= CF_NO_GOTO_TB | CF_NO_GOTO_PTR;
+    }
+
+    return cflags;
 }
 
 /* Might cause an exception, so have a longjmp destination ready */
diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c
index 59804af37b..2ea5a74f30 100644
--- a/accel/tcg/translator.c
+++ b/accel/tcg/translator.c
@@ -33,6 +33,11 @@ void translator_loop_temp_check(DisasContextBase *db)
 
 bool translator_use_goto_tb(DisasContextBase *db, target_ulong dest)
 {
+    /* Suppress goto_tb if requested. */
+    if (tb_cflags(db->tb) & CF_NO_GOTO_TB) {
+        return false;
+    }
+
     /* Suppress goto_tb in the case of single-steping.  */
     if (db->singlestep_enabled || singlestep) {
         return false;
diff --git a/tcg/tcg-op.c b/tcg/tcg-op.c
index 0c561fb253..e0d54d537f 100644
--- a/tcg/tcg-op.c
+++ b/tcg/tcg-op.c
@@ -2723,10 +2723,6 @@ void tcg_gen_exit_tb(const TranslationBlock *tb, unsigned idx)
            seen this numbered exit before, via tcg_gen_goto_tb.  */
         tcg_debug_assert(tcg_ctx->goto_tb_issue_mask & (1 << idx));
 #endif
-        /* When not chaining, exit without indicating a link.  */
-        if (qemu_loglevel_mask(CPU_LOG_TB_NOCHAIN)) {
-            val = 0;
-        }
     } else {
         /* This is an exit via the exitreq label.  */
         tcg_debug_assert(idx == TB_EXIT_REQUESTED);
@@ -2738,6 +2734,8 @@ void tcg_gen_exit_tb(const TranslationBlock *tb, unsigned idx)
 
 void tcg_gen_goto_tb(unsigned idx)
 {
+    /* We tested CF_NO_GOTO_TB in translator_use_goto_tb. */
+    tcg_debug_assert(!(tcg_ctx->tb_cflags & CF_NO_GOTO_TB));
     /* We only support two chained exits.  */
     tcg_debug_assert(idx <= TB_EXIT_IDXMAX);
 #ifdef CONFIG_DEBUG_TCG
@@ -2746,25 +2744,23 @@ void tcg_gen_goto_tb(unsigned idx)
     tcg_ctx->goto_tb_issue_mask |= 1 << idx;
 #endif
     plugin_gen_disable_mem_helpers();
-    /* When not chaining, we simply fall through to the "fallback" exit.  */
-    if (!qemu_loglevel_mask(CPU_LOG_TB_NOCHAIN)) {
-        tcg_gen_op1i(INDEX_op_goto_tb, idx);
-    }
+    tcg_gen_op1i(INDEX_op_goto_tb, idx);
 }
 
 void tcg_gen_lookup_and_goto_ptr(void)
 {
-    if (!qemu_loglevel_mask(CPU_LOG_TB_NOCHAIN)) {
-        TCGv_ptr ptr;
+    TCGv_ptr ptr;
 
-        plugin_gen_disable_mem_helpers();
-        ptr = tcg_temp_new_ptr();
-        gen_helper_lookup_tb_ptr(ptr, cpu_env);
-        tcg_gen_op1i(INDEX_op_goto_ptr, tcgv_ptr_arg(ptr));
-        tcg_temp_free_ptr(ptr);
-    } else {
+    if (tcg_ctx->tb_cflags & CF_NO_GOTO_PTR) {
         tcg_gen_exit_tb(NULL, 0);
+        return;
     }
+
+    plugin_gen_disable_mem_helpers();
+    ptr = tcg_temp_new_ptr();
+    gen_helper_lookup_tb_ptr(ptr, cpu_env);
+    tcg_gen_op1i(INDEX_op_goto_ptr, tcgv_ptr_arg(ptr));
+    tcg_temp_free_ptr(ptr);
 }
 
 static inline MemOp tcg_canonicalize_memop(MemOp op, bool is64, bool st)
-- 
2.25.1

[PATCH for-6.1 v6 05/17] accel/tcg: Drop CF_NO_GOTO_PTR from -d nochain

[Richard Henderson]

The purpose of suppressing goto_ptr from -d nochain had been
to return to the main loop so that -d cpu would be recognized.
But we now include -d cpu logging in helper_lookup_tb_ptr so
there is no need to exclude goto_ptr.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Message-Id: <20210717221851.2124573-5-richard.henderson@linaro.org>
---
 accel/tcg/cpu-exec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index d3232d5764..70ea3c7d68 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -150,7 +150,7 @@ uint32_t curr_cflags(CPUState *cpu)
     uint32_t cflags = cpu->tcg_cflags;
 
     if (qemu_loglevel_mask(CPU_LOG_TB_NOCHAIN)) {
-        cflags |= CF_NO_GOTO_TB | CF_NO_GOTO_PTR;
+        cflags |= CF_NO_GOTO_TB;
     }
 
     return cflags;
-- 
2.25.1

[PATCH for-6.1 v6 06/17] accel/tcg: Handle -singlestep in curr_cflags

[Richard Henderson]

Exchange the test in translator_use_goto_tb for CF_NO_GOTO_TB,
and the test in tb_gen_code for setting CF_COUNT_MASK to 1.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Message-Id: <20210717221851.2124573-6-richard.henderson@linaro.org>
---
 accel/tcg/cpu-exec.c      | 8 +++++++-
 accel/tcg/translate-all.c | 2 +-
 accel/tcg/translator.c    | 2 +-
 3 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index 70ea3c7d68..2206c463f5 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -149,7 +149,13 @@ uint32_t curr_cflags(CPUState *cpu)
 {
     uint32_t cflags = cpu->tcg_cflags;
 
-    if (qemu_loglevel_mask(CPU_LOG_TB_NOCHAIN)) {
+    /*
+     * For singlestep and -d nochain, suppress goto_tb so that
+     * we can log -d cpu,exec after every TB.
+     */
+    if (singlestep) {
+        cflags |= CF_NO_GOTO_TB | 1;
+    } else if (qemu_loglevel_mask(CPU_LOG_TB_NOCHAIN)) {
         cflags |= CF_NO_GOTO_TB;
     }
 
diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index 5cc01d693b..bf82c15aab 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -1432,7 +1432,7 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
     }
     QEMU_BUILD_BUG_ON(CF_COUNT_MASK + 1 != TCG_MAX_INSNS);
 
-    if (cpu->singlestep_enabled || singlestep) {
+    if (cpu->singlestep_enabled) {
         max_insns = 1;
     }
 
diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c
index 2ea5a74f30..a59eb7c11b 100644
--- a/accel/tcg/translator.c
+++ b/accel/tcg/translator.c
@@ -39,7 +39,7 @@ bool translator_use_goto_tb(DisasContextBase *db, target_ulong dest)
     }
 
     /* Suppress goto_tb in the case of single-steping.  */
-    if (db->singlestep_enabled || singlestep) {
+    if (db->singlestep_enabled) {
         return false;
     }
 
-- 
2.25.1

[PATCH for-6.1 v6 07/17] accel/tcg: Use CF_NO_GOTO_{TB, PTR} in cpu_exec_step_atomic

[Richard Henderson]

Request that the one TB returns immediately, so that
we release the exclusive lock as soon as possible.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20210717221851.2124573-7-richard.henderson@linaro.org>
---
 accel/tcg/cpu-exec.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index 2206c463f5..5bb099174f 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -330,8 +330,7 @@ void cpu_exec_step_atomic(CPUState *cpu)
     CPUArchState *env = (CPUArchState *)cpu->env_ptr;
     TranslationBlock *tb;
     target_ulong cs_base, pc;
-    uint32_t flags;
-    uint32_t cflags = (curr_cflags(cpu) & ~CF_PARALLEL) | 1;
+    uint32_t flags, cflags;
     int tb_exit;
 
     if (sigsetjmp(cpu->jmp_env, 0) == 0) {
@@ -341,8 +340,14 @@ void cpu_exec_step_atomic(CPUState *cpu)
         cpu->running = true;
 
         cpu_get_tb_cpu_state(env, &pc, &cs_base, &flags);
-        tb = tb_lookup(cpu, pc, cs_base, flags, cflags);
 
+        cflags = curr_cflags(cpu);
+        /* Execute in a serial context. */
+        cflags &= ~CF_PARALLEL;
+        /* After 1 insn, return and release the exclusive lock. */
+        cflags |= CF_NO_GOTO_TB | CF_NO_GOTO_PTR | 1;
+
+        tb = tb_lookup(cpu, pc, cs_base, flags, cflags);
         if (tb == NULL) {
             mmap_lock();
             tb = tb_gen_code(cpu, pc, cs_base, flags, cflags);
-- 
2.25.1

[PATCH for-6.1 v6 08/17] hw/core: Introduce TCGCPUOps.debug_check_breakpoint

[Richard Henderson]

New hook to return true when an architectural breakpoint is
to be recognized and false when it should be suppressed.

First use must wait until other pieces are in place.

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/hw/core/tcg-cpu-ops.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/hw/core/tcg-cpu-ops.h b/include/hw/core/tcg-cpu-ops.h
index 72d791438c..eab27d0c03 100644
--- a/include/hw/core/tcg-cpu-ops.h
+++ b/include/hw/core/tcg-cpu-ops.h
@@ -88,6 +88,12 @@ struct TCGCPUOps {
      */
     bool (*debug_check_watchpoint)(CPUState *cpu, CPUWatchpoint *wp);
 
+    /**
+     * @debug_check_breakpoint: return true if the architectural
+     * breakpoint whose PC has matched should really fire.
+     */
+    bool (*debug_check_breakpoint)(CPUState *cpu);
+
     /**
      * @io_recompile_replay_branch: Callback for cpu_io_recompile.
      *
-- 
2.25.1

Re: [PATCH for-6.1 v6 08/17] hw/core: Introduce TCGCPUOps.debug_check_breakpoint

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> New hook to return true when an architectural breakpoint is
> to be recognized and false when it should be suppressed.
>
> First use must wait until other pieces are in place.
>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée

[PATCH for-6.1 v6 09/17] target/arm: Implement debug_check_breakpoint

[Richard Henderson]

Reuse the code at the bottom of helper_check_breakpoints,
which is what we currently call from *_tr_breakpoint_check.

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/internals.h    | 3 +++
 target/arm/cpu.c          | 1 +
 target/arm/cpu_tcg.c      | 1 +
 target/arm/debug_helper.c | 7 +++----
 4 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/target/arm/internals.h b/target/arm/internals.h
index 3ba86e8af8..11a72013f5 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -282,6 +282,9 @@ void hw_breakpoint_update(ARMCPU *cpu, int n);
  */
 void hw_breakpoint_update_all(ARMCPU *cpu);
 
+/* Callback function for checking if a breakpoint should trigger. */
+bool arm_debug_check_breakpoint(CPUState *cs);
+
 /* Callback function for checking if a watchpoint should trigger. */
 bool arm_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp);
 
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 9cddfd6a44..752b15bb79 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1984,6 +1984,7 @@ static const struct TCGCPUOps arm_tcg_ops = {
     .do_unaligned_access = arm_cpu_do_unaligned_access,
     .adjust_watchpoint_address = arm_adjust_watchpoint_address,
     .debug_check_watchpoint = arm_debug_check_watchpoint,
+    .debug_check_breakpoint = arm_debug_check_breakpoint,
 #endif /* !CONFIG_USER_ONLY */
 };
 #endif /* CONFIG_TCG */
diff --git a/target/arm/cpu_tcg.c b/target/arm/cpu_tcg.c
index d2d97115ea..ed444bf436 100644
--- a/target/arm/cpu_tcg.c
+++ b/target/arm/cpu_tcg.c
@@ -911,6 +911,7 @@ static const struct TCGCPUOps arm_v7m_tcg_ops = {
     .do_unaligned_access = arm_cpu_do_unaligned_access,
     .adjust_watchpoint_address = arm_adjust_watchpoint_address,
     .debug_check_watchpoint = arm_debug_check_watchpoint,
+    .debug_check_breakpoint = arm_debug_check_breakpoint,
 #endif /* !CONFIG_USER_ONLY */
 };
 #endif /* CONFIG_TCG */
diff --git a/target/arm/debug_helper.c b/target/arm/debug_helper.c
index 2ff72d47d1..4a0c479527 100644
--- a/target/arm/debug_helper.c
+++ b/target/arm/debug_helper.c
@@ -216,8 +216,9 @@ static bool check_watchpoints(ARMCPU *cpu)
     return false;
 }
 
-static bool check_breakpoints(ARMCPU *cpu)
+bool arm_debug_check_breakpoint(CPUState *cs)
 {
+    ARMCPU *cpu = ARM_CPU(cs);
     CPUARMState *env = &cpu->env;
     int n;
 
@@ -240,9 +241,7 @@ static bool check_breakpoints(ARMCPU *cpu)
 
 void HELPER(check_breakpoints)(CPUARMState *env)
 {
-    ARMCPU *cpu = env_archcpu(env);
-
-    if (check_breakpoints(cpu)) {
+    if (arm_debug_check_breakpoint(env_cpu(env))) {
         HELPER(exception_internal(env, EXCP_DEBUG));
     }
 }
-- 
2.25.1

Re: [PATCH for-6.1 v6 09/17] target/arm: Implement debug_check_breakpoint

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> Reuse the code at the bottom of helper_check_breakpoints,
> which is what we currently call from *_tr_breakpoint_check.
>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée

[PATCH for-6.1 v6 10/17] target/i386: Implement debug_check_breakpoint

[Richard Henderson]

Return false for RF set, as we do in i386_tr_breakpoint_check.

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/i386/tcg/tcg-cpu.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/target/i386/tcg/tcg-cpu.c b/target/i386/tcg/tcg-cpu.c
index e96ec9bbcc..238e3a9395 100644
--- a/target/i386/tcg/tcg-cpu.c
+++ b/target/i386/tcg/tcg-cpu.c
@@ -54,6 +54,17 @@ static void x86_cpu_synchronize_from_tb(CPUState *cs,
     cpu->env.eip = tb->pc - tb->cs_base;
 }
 
+#ifndef CONFIG_USER_ONLY
+static bool x86_debug_check_breakpoint(CPUState *cs)
+{
+    X86CPU *cpu = X86_CPU(cs);
+    CPUX86State *env = &cpu->env;
+
+    /* RF disables all architectural breakpoints. */
+    return !(env->eflags & RF_MASK);
+}
+#endif
+
 #include "hw/core/tcg-cpu-ops.h"
 
 static const struct TCGCPUOps x86_tcg_ops = {
@@ -66,6 +77,7 @@ static const struct TCGCPUOps x86_tcg_ops = {
     .tlb_fill = x86_cpu_tlb_fill,
 #ifndef CONFIG_USER_ONLY
     .debug_excp_handler = breakpoint_handler,
+    .debug_check_breakpoint = x86_debug_check_breakpoint,
 #endif /* !CONFIG_USER_ONLY */
 };
 
-- 
2.25.1

[PATCH for-6.1 v6 11/17] hw/core: Introduce CPUClass.gdb_adjust_breakpoint

[Richard Henderson]

This will allow a breakpoint hack to move out of AVR's translator.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/hw/core/cpu.h |  4 ++++
 cpu.c                 | 10 ++++++++++
 2 files changed, 14 insertions(+)

diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 4e0ea68efc..bc864564ce 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -103,6 +103,9 @@ struct SysemuCPUOps;
  *       also implement the synchronize_from_tb hook.
  * @gdb_read_register: Callback for letting GDB read a register.
  * @gdb_write_register: Callback for letting GDB write a register.
+ * @gdb_adjust_breakpoint: Callback for adjusting the address of a
+ *       breakpoint.  Used by AVR to handle a gdb mis-feature with
+ *       its Harvard architecture split code and data.
  * @gdb_num_core_regs: Number of core registers accessible to GDB.
  * @gdb_core_xml_file: File name for core registers GDB XML description.
  * @gdb_stop_before_watchpoint: Indicates whether GDB expects the CPU to stop
@@ -137,6 +140,7 @@ struct CPUClass {
     void (*set_pc)(CPUState *cpu, vaddr value);
     int (*gdb_read_register)(CPUState *cpu, GByteArray *buf, int reg);
     int (*gdb_write_register)(CPUState *cpu, uint8_t *buf, int reg);
+    vaddr (*gdb_adjust_breakpoint)(CPUState *cpu, vaddr addr);
 
     const char *gdb_core_xml_file;
     gchar * (*gdb_arch_name)(CPUState *cpu);
diff --git a/cpu.c b/cpu.c
index 83059537d7..91d9e38acb 100644
--- a/cpu.c
+++ b/cpu.c
@@ -267,8 +267,13 @@ static void breakpoint_invalidate(CPUState *cpu, target_ulong pc)
 int cpu_breakpoint_insert(CPUState *cpu, vaddr pc, int flags,
                           CPUBreakpoint **breakpoint)
 {
+    CPUClass *cc = CPU_GET_CLASS(cpu);
     CPUBreakpoint *bp;
 
+    if (cc->gdb_adjust_breakpoint) {
+        pc = cc->gdb_adjust_breakpoint(cpu, pc);
+    }
+
     bp = g_malloc(sizeof(*bp));
 
     bp->pc = pc;
@@ -294,8 +299,13 @@ int cpu_breakpoint_insert(CPUState *cpu, vaddr pc, int flags,
 /* Remove a specific breakpoint.  */
 int cpu_breakpoint_remove(CPUState *cpu, vaddr pc, int flags)
 {
+    CPUClass *cc = CPU_GET_CLASS(cpu);
     CPUBreakpoint *bp;
 
+    if (cc->gdb_adjust_breakpoint) {
+        pc = cc->gdb_adjust_breakpoint(cpu, pc);
+    }
+
     QTAILQ_FOREACH(bp, &cpu->breakpoints, entry) {
         if (bp->pc == pc && bp->flags == flags) {
             cpu_breakpoint_remove_by_ref(cpu, bp);
-- 
2.25.1

Re: [PATCH for-6.1 v6 11/17] hw/core: Introduce CPUClass.gdb_adjust_breakpoint

[Peter Maydell]

On Tue, 20 Jul 2021 at 20:54, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> This will allow a breakpoint hack to move out of AVR's translator.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

> diff --git a/cpu.c b/cpu.c
> index 83059537d7..91d9e38acb 100644
> --- a/cpu.c
> +++ b/cpu.c
> @@ -267,8 +267,13 @@ static void breakpoint_invalidate(CPUState *cpu, target_ulong pc)
>  int cpu_breakpoint_insert(CPUState *cpu, vaddr pc, int flags,
>                            CPUBreakpoint **breakpoint)
>  {
> +    CPUClass *cc = CPU_GET_CLASS(cpu);
>      CPUBreakpoint *bp;
>
> +    if (cc->gdb_adjust_breakpoint) {
> +        pc = cc->gdb_adjust_breakpoint(cpu, pc);
> +    }
> +
>      bp = g_malloc(sizeof(*bp));
>
>      bp->pc = pc;
> @@ -294,8 +299,13 @@ int cpu_breakpoint_insert(CPUState *cpu, vaddr pc, int flags,
>  /* Remove a specific breakpoint.  */
>  int cpu_breakpoint_remove(CPUState *cpu, vaddr pc, int flags)
>  {
> +    CPUClass *cc = CPU_GET_CLASS(cpu);
>      CPUBreakpoint *bp;
>
> +    if (cc->gdb_adjust_breakpoint) {
> +        pc = cc->gdb_adjust_breakpoint(cpu, pc);
> +    }
> +
>      QTAILQ_FOREACH(bp, &cpu->breakpoints, entry) {
>          if (bp->pc == pc && bp->flags == flags) {
>              cpu_breakpoint_remove_by_ref(cpu, bp);
> --

So previously for AVR we would have considered the bp at 0x100
and the one at 0x800100 as distinct (in the sense that the only way
the gdb remote protocol distinguishes breakpoints is by "what address",
and these have different addresses). After this change, they won't
be distinct, because if you set a bp at 0x100 and 0x800100 and then
try to remove the one at 0x100 we might remove the 0x800100 one,
because we're storing only the adjusted-address, not the one gdb used.

This might not matter in practice...

-- PMM

Re: [PATCH for-6.1 v6 11/17] hw/core: Introduce CPUClass.gdb_adjust_breakpoint

[Richard Henderson]

On 7/20/21 10:56 AM, Peter Maydell wrote:
> On Tue, 20 Jul 2021 at 20:54, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> This will allow a breakpoint hack to move out of AVR's translator.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> 
>> diff --git a/cpu.c b/cpu.c
>> index 83059537d7..91d9e38acb 100644
>> --- a/cpu.c
>> +++ b/cpu.c
>> @@ -267,8 +267,13 @@ static void breakpoint_invalidate(CPUState *cpu, target_ulong pc)
>>   int cpu_breakpoint_insert(CPUState *cpu, vaddr pc, int flags,
>>                             CPUBreakpoint **breakpoint)
>>   {
>> +    CPUClass *cc = CPU_GET_CLASS(cpu);
>>       CPUBreakpoint *bp;
>>
>> +    if (cc->gdb_adjust_breakpoint) {
>> +        pc = cc->gdb_adjust_breakpoint(cpu, pc);
>> +    }
>> +
>>       bp = g_malloc(sizeof(*bp));
>>
>>       bp->pc = pc;
>> @@ -294,8 +299,13 @@ int cpu_breakpoint_insert(CPUState *cpu, vaddr pc, int flags,
>>   /* Remove a specific breakpoint.  */
>>   int cpu_breakpoint_remove(CPUState *cpu, vaddr pc, int flags)
>>   {
>> +    CPUClass *cc = CPU_GET_CLASS(cpu);
>>       CPUBreakpoint *bp;
>>
>> +    if (cc->gdb_adjust_breakpoint) {
>> +        pc = cc->gdb_adjust_breakpoint(cpu, pc);
>> +    }
>> +
>>       QTAILQ_FOREACH(bp, &cpu->breakpoints, entry) {
>>           if (bp->pc == pc && bp->flags == flags) {
>>               cpu_breakpoint_remove_by_ref(cpu, bp);
>> --
> 
> So previously for AVR we would have considered the bp at 0x100
> and the one at 0x800100 as distinct (in the sense that the only way
> the gdb remote protocol distinguishes breakpoints is by "what address",
> and these have different addresses). After this change, they won't
> be distinct, because if you set a bp at 0x100 and 0x800100 and then
> try to remove the one at 0x100 we might remove the 0x800100 one,
> because we're storing only the adjusted-address, not the one gdb used.
> 
> This might not matter in practice...

I don't think it will matter.

Currently, if it sets both 0x100 and 0x800100, then we'll record two breakpoints, and with 
either we'll raise EXCP_DEBUG when pc == 0x100.

Afterward, we'll have two CPUBreakpoint structures that both contain 0x100, and when pc == 
0x100 we'll raise EXCP_DEBUG.  If gdb removes the breakpoint at 0x800100, we'll remove one 
of the two CPUBreakpoint.  But we'll still stop at 0x100, as expected.  When it removes 
the breakpoint at 0x100, both CPUBreakpoint structures will be gone.

In principal, gdb could now add a breakpoint at 0x800100 and remove it with 0x100, where 
it could not before.  But I don't expect that to happen.  If we reported any kind of 
status to gdb re the breakpoint insertion or removal (e.g. bp not found), then it might 
matter, but we don't.

Practically, this is working around what I'd call a gdb bug wrt avr.  Which may even have 
been fixed -- I haven't looked.


r~

Re: [PATCH for-6.1 v6 11/17] hw/core: Introduce CPUClass.gdb_adjust_breakpoint

[Philippe Mathieu-Daudé]

On 7/20/21 11:08 PM, Richard Henderson wrote:
> On 7/20/21 10:56 AM, Peter Maydell wrote:
>> On Tue, 20 Jul 2021 at 20:54, Richard Henderson
>> <richard.henderson@linaro.org> wrote:
>>>
>>> This will allow a breakpoint hack to move out of AVR's translator.
>>>
>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>>
>>> diff --git a/cpu.c b/cpu.c
>>> index 83059537d7..91d9e38acb 100644
>>> --- a/cpu.c
>>> +++ b/cpu.c
>>> @@ -267,8 +267,13 @@ static void breakpoint_invalidate(CPUState *cpu,
>>> target_ulong pc)
>>>   int cpu_breakpoint_insert(CPUState *cpu, vaddr pc, int flags,
>>>                             CPUBreakpoint **breakpoint)
>>>   {
>>> +    CPUClass *cc = CPU_GET_CLASS(cpu);
>>>       CPUBreakpoint *bp;
>>>
>>> +    if (cc->gdb_adjust_breakpoint) {
>>> +        pc = cc->gdb_adjust_breakpoint(cpu, pc);
>>> +    }
>>> +
>>>       bp = g_malloc(sizeof(*bp));
>>>
>>>       bp->pc = pc;
>>> @@ -294,8 +299,13 @@ int cpu_breakpoint_insert(CPUState *cpu, vaddr
>>> pc, int flags,
>>>   /* Remove a specific breakpoint.  */
>>>   int cpu_breakpoint_remove(CPUState *cpu, vaddr pc, int flags)
>>>   {
>>> +    CPUClass *cc = CPU_GET_CLASS(cpu);
>>>       CPUBreakpoint *bp;
>>>
>>> +    if (cc->gdb_adjust_breakpoint) {
>>> +        pc = cc->gdb_adjust_breakpoint(cpu, pc);
>>> +    }
>>> +
>>>       QTAILQ_FOREACH(bp, &cpu->breakpoints, entry) {
>>>           if (bp->pc == pc && bp->flags == flags) {
>>>               cpu_breakpoint_remove_by_ref(cpu, bp);
>>> -- 
>>
>> So previously for AVR we would have considered the bp at 0x100
>> and the one at 0x800100 as distinct (in the sense that the only way
>> the gdb remote protocol distinguishes breakpoints is by "what address",
>> and these have different addresses). After this change, they won't
>> be distinct, because if you set a bp at 0x100 and 0x800100 and then
>> try to remove the one at 0x100 we might remove the 0x800100 one,
>> because we're storing only the adjusted-address, not the one gdb used.
>>
>> This might not matter in practice...
> 
> I don't think it will matter.
> 
> Currently, if it sets both 0x100 and 0x800100, then we'll record two
> breakpoints, and with either we'll raise EXCP_DEBUG when pc == 0x100.
> 
> Afterward, we'll have two CPUBreakpoint structures that both contain
> 0x100, and when pc == 0x100 we'll raise EXCP_DEBUG.  If gdb removes the
> breakpoint at 0x800100, we'll remove one of the two CPUBreakpoint.  But
> we'll still stop at 0x100, as expected.  When it removes the breakpoint
> at 0x100, both CPUBreakpoint structures will be gone.
> 
> In principal, gdb could now add a breakpoint at 0x800100 and remove it
> with 0x100, where it could not before.  But I don't expect that to
> happen.  If we reported any kind of status to gdb re the breakpoint
> insertion or removal (e.g. bp not found), then it might matter, but we
> don't.
> 
> Practically, this is working around what I'd call a gdb bug wrt avr. 
> Which may even have been fixed -- I haven't looked.

This is not a bug but a feature to deal with the Harvard architecture.
QEMU AVR model is based on GCC sources so uses the same "feature".

The AVR core has 2 address spaces: "CODE" and "DATA". An address space
is always zero-based (so both are). To avoid having to deal with
relocation of symbols from different AS but having same address, the
DATA space is mapped at 0x800000 (bit 23 is "virtual" as inexistant
- masked - from the CODE AS).

The core can not execute from DATA, so CPUBreakpoint can only be
triggered from CODE.

I once implemented different AS but switched to smth else :/
It was working but for some reason I couldn't remove the
OFFSET_DATA / OFFSET_CODE definitions, I don't remember &
should respin... See
https://gitlab.com/philmd/qemu/-/compare/avr_gsoc_v1a...avr_gsoc_v1b

Extract of the patches to show the idea:

diff --git a/target/avr/cpu.h b/target/avr/cpu.h
+/* Indexes used when registering address spaces with
cpu_address_space_init */
+typedef enum AVRASIdx {
+    AVRASIdx_CODE = 0,
+    AVRASIdx_DATA = 1,
+} AVRASIdx;

diff --git a/target/avr/cpu.c b/target/avr/cpu.c
@@ -96,6 +98,13 @@ static void avr_cpu_realizefn(DeviceState *dev, Error
**errp)
         error_propagate(errp, local_err);
         return;
     }
+
+    cs->num_ases = 2;
+    cpu_address_space_init(cs, AVRASIdx_CODE, "cpu-program-bus",
+                           get_program_memory());
+    cpu_address_space_init(cs, AVRASIdx_DATA, "cpu-data-bus",
+                           get_data_memory());
+
     qemu_init_vcpu(cs);
     cpu_reset(cs);

diff --git a/target/avr/helper.c b/target/avr/helper.c
-/*
- * This function implements IN instruction
- *
- * It does the following
- * a.  if an IO register belongs to CPU, its value is read and returned
- * b.  otherwise io address is translated to mem address and physical
memory
- *     is read.
- * c.  it caches the value for sake of SBI, SBIC, SBIS & CBI implementation
- *
- */
-target_ulong helper_inb(CPUAVRState *env, uint32_t port)
+static uint8_t data_read(CPUAVRState *env, uint32_t addr)
 {
-    target_ulong data = 0;
+    CPUState *cs;
+    AddressSpace *as;
+    uint8_t data = 0;

-    switch (port) {
+    switch (addr) {
+    case 0x00 ... 0x1f:
+        /* CPU registers */
+        data = env->r[addr];
+        break;
     case 0x38: /* RAMPD */
-        data = 0xff & (env->rampD >> 16);
+        /* FIXME check available feature? */
+        data = env->rampD >> 16;
         break;
     case 0x39: /* RAMPX */
-        data = 0xff & (env->rampX >> 16);
+        data = env->rampX >> 16;
         break;
     case 0x3a: /* RAMPY */
-        data = 0xff & (env->rampY >> 16);
+        data = env->rampY >> 16;
         break;
     case 0x3b: /* RAMPZ */
-        data = 0xff & (env->rampZ >> 16);
+        data = env->rampZ >> 16;
         break;
     case 0x3c: /* EIND */
-        data = 0xff & (env->eind >> 16);
+        data = env->eind >> 16;
         break;
     case 0x3d: /* SPL */
         data = env->sp & 0x00ff;
@@ -232,12 +230,30 @@ target_ulong helper_inb(CPUAVRState *env, uint32_t
port)
         break;
     default:
         /* not a special register, pass to normal memory access */
-        cpu_physical_memory_read(OFFSET_IO_REGISTERS + port, &data, 1);
+        cs = env_cpu(env);
+        as = cpu_get_address_space(cs, AVRASIdx_DATA);
+        data = address_space_ldub(as, addr, MEMTXATTRS_UNSPECIFIED, NULL);
     }
+    trace_avr_data_read(addr, data);

     return data;
 }

+/*
+ * This function implements IN instruction
+ *
+ * It does the following
+ * a.  if an IO register belongs to CPU, its value is read and returned
+ * b.  otherwise io address is translated to mem address and physical
memory
+ *     is read.
+ * c.  it caches the value for sake of SBI, SBIC, SBIS & CBI implementation
+ *
+ */
+target_ulong helper_inb(CPUAVRState *env, uint32_t port)
+{
+    return data_read(env, NUMBER_OF_CPU_REGISTERS + port);
+}

@@ -299,21 +315,9 @@ void helper_outb(CPUAVRState *env, uint32_t port,
uint32_t data)
  */
 target_ulong helper_fullrd(CPUAVRState *env, uint32_t addr)
 {
-    uint8_t data;
-
     env->fullacc = false;

-    if (addr < NUMBER_OF_CPU_REGISTERS) {
-        /* CPU registers */
-        data = env->r[addr];
-    } else if (addr < NUMBER_OF_CPU_REGISTERS + NUMBER_OF_IO_REGISTERS) {
-        /* IO registers */
-        data = helper_inb(env, addr - NUMBER_OF_CPU_REGISTERS);
-    } else {
-        /* memory */
-        cpu_physical_memory_read(OFFSET_DATA + addr, &data, 1);
-    }
-    return data;
+    return data_read(env, addr);
 }

Re: [PATCH for-6.1 v6 11/17] hw/core: Introduce CPUClass.gdb_adjust_breakpoint

[Philippe Mathieu-Daudé]

On 7/20/21 11:53 PM, Philippe Mathieu-Daudé wrote:
> On 7/20/21 11:08 PM, Richard Henderson wrote:
>> On 7/20/21 10:56 AM, Peter Maydell wrote:
>>> On Tue, 20 Jul 2021 at 20:54, Richard Henderson
>>> <richard.henderson@linaro.org> wrote:
>>>>
>>>> This will allow a breakpoint hack to move out of AVR's translator.
>>>>
>>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>>>
>>>> diff --git a/cpu.c b/cpu.c
>>>> index 83059537d7..91d9e38acb 100644
>>>> --- a/cpu.c
>>>> +++ b/cpu.c
>>>> @@ -267,8 +267,13 @@ static void breakpoint_invalidate(CPUState *cpu,
>>>> target_ulong pc)
>>>>   int cpu_breakpoint_insert(CPUState *cpu, vaddr pc, int flags,
>>>>                             CPUBreakpoint **breakpoint)
>>>>   {
>>>> +    CPUClass *cc = CPU_GET_CLASS(cpu);
>>>>       CPUBreakpoint *bp;
>>>>
>>>> +    if (cc->gdb_adjust_breakpoint) {
>>>> +        pc = cc->gdb_adjust_breakpoint(cpu, pc);
>>>> +    }
>>>> +
>>>>       bp = g_malloc(sizeof(*bp));
>>>>
>>>>       bp->pc = pc;
>>>> @@ -294,8 +299,13 @@ int cpu_breakpoint_insert(CPUState *cpu, vaddr
>>>> pc, int flags,
>>>>   /* Remove a specific breakpoint.  */
>>>>   int cpu_breakpoint_remove(CPUState *cpu, vaddr pc, int flags)
>>>>   {
>>>> +    CPUClass *cc = CPU_GET_CLASS(cpu);
>>>>       CPUBreakpoint *bp;
>>>>
>>>> +    if (cc->gdb_adjust_breakpoint) {
>>>> +        pc = cc->gdb_adjust_breakpoint(cpu, pc);
>>>> +    }
>>>> +
>>>>       QTAILQ_FOREACH(bp, &cpu->breakpoints, entry) {
>>>>           if (bp->pc == pc && bp->flags == flags) {
>>>>               cpu_breakpoint_remove_by_ref(cpu, bp);
>>>> -- 
>>>
>>> So previously for AVR we would have considered the bp at 0x100
>>> and the one at 0x800100 as distinct (in the sense that the only way
>>> the gdb remote protocol distinguishes breakpoints is by "what address",
>>> and these have different addresses). After this change, they won't
>>> be distinct, because if you set a bp at 0x100 and 0x800100 and then
>>> try to remove the one at 0x100 we might remove the 0x800100 one,
>>> because we're storing only the adjusted-address, not the one gdb used.

Yes. Looks good.

>>>
>>> This might not matter in practice...
>>
>> I don't think it will matter.
>>
>> Currently, if it sets both 0x100 and 0x800100, then we'll record two
>> breakpoints, and with either we'll raise EXCP_DEBUG when pc == 0x100.
>>
>> Afterward, we'll have two CPUBreakpoint structures that both contain
>> 0x100, and when pc == 0x100 we'll raise EXCP_DEBUG.  If gdb removes the
>> breakpoint at 0x800100, we'll remove one of the two CPUBreakpoint.  But
>> we'll still stop at 0x100, as expected.  When it removes the breakpoint
>> at 0x100, both CPUBreakpoint structures will be gone.
>>
>> In principal, gdb could now add a breakpoint at 0x800100 and remove it
>> with 0x100, where it could not before.  But I don't expect that to
>> happen.  If we reported any kind of status to gdb re the breakpoint
>> insertion or removal (e.g. bp not found), then it might matter, but we
>> don't.

IIUC QEMU model is "hardware breakpoint". I don't know how gdb deals
if user add both soft/hard bp. Neither do I know how many soft/hard
bp are "annouced" via gdbstub monitor to gdb (Alex?). Amusingly
gdb-xml/avr-cpu.xml announces itself as a riscv cpu:

<feature name="org.gnu.gdb.riscv.cpu">

Maybe because there is no official XML declaration for AVR?
https://sourceware.org/gdb/current/onlinedocs/gdb/Standard-Target-Features.html#Standard-Target-Features

>> Practically, this is working around what I'd call a gdb bug wrt avr. 
>> Which may even have been fixed -- I haven't looked.

I agree this won't matter much (and this patch makes it cleaner) so:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH for-6.1 v6 11/17] hw/core: Introduce CPUClass.gdb_adjust_breakpoint

[Alex Bennée]

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

> On 7/20/21 11:53 PM, Philippe Mathieu-Daudé wrote:
>> On 7/20/21 11:08 PM, Richard Henderson wrote:
>>> On 7/20/21 10:56 AM, Peter Maydell wrote:
>>>> On Tue, 20 Jul 2021 at 20:54, Richard Henderson
>>>> <richard.henderson@linaro.org> wrote:
>>>>>
>>>>> This will allow a breakpoint hack to move out of AVR's translator.
>>>>>
>>>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>>>>
<snip>
>>>> So previously for AVR we would have considered the bp at 0x100
>>>> and the one at 0x800100 as distinct (in the sense that the only way
>>>> the gdb remote protocol distinguishes breakpoints is by "what address",
>>>> and these have different addresses). After this change, they won't
>>>> be distinct, because if you set a bp at 0x100 and 0x800100 and then
>>>> try to remove the one at 0x100 we might remove the 0x800100 one,
>>>> because we're storing only the adjusted-address, not the one gdb used.
>
> Yes. Looks good.
>
>>>>
>>>> This might not matter in practice...
>>>
>>> I don't think it will matter.
>>>
>>> Currently, if it sets both 0x100 and 0x800100, then we'll record two
>>> breakpoints, and with either we'll raise EXCP_DEBUG when pc == 0x100.
>>>
>>> Afterward, we'll have two CPUBreakpoint structures that both contain
>>> 0x100, and when pc == 0x100 we'll raise EXCP_DEBUG.  If gdb removes the
>>> breakpoint at 0x800100, we'll remove one of the two CPUBreakpoint.  But
>>> we'll still stop at 0x100, as expected.  When it removes the breakpoint
>>> at 0x100, both CPUBreakpoint structures will be gone.
>>>
>>> In principal, gdb could now add a breakpoint at 0x800100 and remove it
>>> with 0x100, where it could not before.  But I don't expect that to
>>> happen.  If we reported any kind of status to gdb re the breakpoint
>>> insertion or removal (e.g. bp not found), then it might matter, but we
>>> don't.
>
> IIUC QEMU model is "hardware breakpoint". I don't know how gdb deals
> if user add both soft/hard bp. Neither do I know how many soft/hard
> bp are "annouced" via gdbstub monitor to gdb (Alex?).

The gdbstub treats both the same and I don't think gdb cares about the
limit until we tell it we can't insert a breakpoint (which we won't
because breakpoints are infinite).

-- 
Alex Bennée

Re: [PATCH for-6.1 v6 11/17] hw/core: Introduce CPUClass.gdb_adjust_breakpoint

[Richard Henderson]

On 7/20/21 11:53 AM, Philippe Mathieu-Daudé wrote:
>> Practically, this is working around what I'd call a gdb bug wrt avr.
>> Which may even have been fixed -- I haven't looked.
> 
> This is not a bug but a feature to deal with the Harvard architecture.
> QEMU AVR model is based on GCC sources so uses the same "feature".
> 
> The AVR core has 2 address spaces: "CODE" and "DATA". An address space
> is always zero-based (so both are). To avoid having to deal with
> relocation of symbols from different AS but having same address, the
> DATA space is mapped at 0x800000 (bit 23 is "virtual" as inexistant
> - masked - from the CODE AS).
> 
> The core can not execute from DATA, so CPUBreakpoint can only be
> triggered from CODE.

I know all this.  It begs the question why gdb would ever *ask* for a CODE breakpoint in 
DATA space.


r~

[PATCH for-6.1 v6 12/17] target/avr: Implement gdb_adjust_breakpoint

[Richard Henderson]

Ensure at registration that all breakpoints are in
code space, not data space.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/avr/cpu.h       |  1 +
 target/avr/cpu.c       |  1 +
 target/avr/gdbstub.c   | 13 +++++++++++++
 target/avr/translate.c | 14 --------------
 4 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/target/avr/cpu.h b/target/avr/cpu.h
index d148e8c75a..93e3faa0a9 100644
--- a/target/avr/cpu.h
+++ b/target/avr/cpu.h
@@ -162,6 +162,7 @@ hwaddr avr_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
 int avr_cpu_gdb_read_register(CPUState *cpu, GByteArray *buf, int reg);
 int avr_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
 int avr_print_insn(bfd_vma addr, disassemble_info *info);
+vaddr avr_cpu_gdb_adjust_breakpoint(CPUState *cpu, vaddr addr);
 
 static inline int avr_feature(CPUAVRState *env, AVRFeature feature)
 {
diff --git a/target/avr/cpu.c b/target/avr/cpu.c
index 57e3fab4a0..ea14175ca5 100644
--- a/target/avr/cpu.c
+++ b/target/avr/cpu.c
@@ -223,6 +223,7 @@ static void avr_cpu_class_init(ObjectClass *oc, void *data)
     cc->disas_set_info = avr_cpu_disas_set_info;
     cc->gdb_read_register = avr_cpu_gdb_read_register;
     cc->gdb_write_register = avr_cpu_gdb_write_register;
+    cc->gdb_adjust_breakpoint = avr_cpu_gdb_adjust_breakpoint;
     cc->gdb_num_core_regs = 35;
     cc->gdb_core_xml_file = "avr-cpu.xml";
     cc->tcg_ops = &avr_tcg_ops;
diff --git a/target/avr/gdbstub.c b/target/avr/gdbstub.c
index c28ed67efe..1c1b908c92 100644
--- a/target/avr/gdbstub.c
+++ b/target/avr/gdbstub.c
@@ -82,3 +82,16 @@ int avr_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
 
     return 0;
 }
+
+vaddr avr_cpu_gdb_adjust_breakpoint(CPUState *cpu, vaddr addr)
+{
+    /*
+     * This is due to some strange GDB behavior
+     * Let's assume main has address 0x100:
+     * b main   - sets breakpoint at address 0x00000100 (code)
+     * b *0x100 - sets breakpoint at address 0x00800100 (data)
+     *
+     * Force all breakpoints into code space.
+     */
+    return addr % OFFSET_DATA;
+}
diff --git a/target/avr/translate.c b/target/avr/translate.c
index 8237a03c23..f7202a646b 100644
--- a/target/avr/translate.c
+++ b/target/avr/translate.c
@@ -2958,20 +2958,6 @@ static void avr_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs)
     DisasContext *ctx = container_of(dcbase, DisasContext, base);
     TCGLabel *skip_label = NULL;
 
-    /*
-     * This is due to some strange GDB behavior
-     * Let's assume main has address 0x100:
-     * b main   - sets breakpoint at address 0x00000100 (code)
-     * b *0x100 - sets breakpoint at address 0x00800100 (data)
-     *
-     * The translator driver has already taken care of the code pointer.
-     */
-    if (!ctx->base.singlestep_enabled &&
-        cpu_breakpoint_test(cs, OFFSET_DATA + ctx->base.pc_next, BP_ANY)) {
-        gen_breakpoint(ctx);
-        return;
-    }
-
     /* Conditionally skip the next instruction, if indicated.  */
     if (ctx->skip_cond != TCG_COND_NEVER) {
         skip_label = gen_new_label();
-- 
2.25.1

Re: [PATCH for-6.1 v6 12/17] target/avr: Implement gdb_adjust_breakpoint

[Philippe Mathieu-Daudé]

On 7/20/21 9:54 PM, Richard Henderson wrote:
> Ensure at registration that all breakpoints are in
> code space, not data space.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/avr/cpu.h       |  1 +
>  target/avr/cpu.c       |  1 +
>  target/avr/gdbstub.c   | 13 +++++++++++++
>  target/avr/translate.c | 14 --------------
>  4 files changed, 15 insertions(+), 14 deletions(-)
> 
> diff --git a/target/avr/cpu.h b/target/avr/cpu.h
> index d148e8c75a..93e3faa0a9 100644
> --- a/target/avr/cpu.h
> +++ b/target/avr/cpu.h
> @@ -162,6 +162,7 @@ hwaddr avr_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
>  int avr_cpu_gdb_read_register(CPUState *cpu, GByteArray *buf, int reg);
>  int avr_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
>  int avr_print_insn(bfd_vma addr, disassemble_info *info);
> +vaddr avr_cpu_gdb_adjust_breakpoint(CPUState *cpu, vaddr addr);
>  
>  static inline int avr_feature(CPUAVRState *env, AVRFeature feature)
>  {
> diff --git a/target/avr/cpu.c b/target/avr/cpu.c
> index 57e3fab4a0..ea14175ca5 100644
> --- a/target/avr/cpu.c
> +++ b/target/avr/cpu.c
> @@ -223,6 +223,7 @@ static void avr_cpu_class_init(ObjectClass *oc, void *data)
>      cc->disas_set_info = avr_cpu_disas_set_info;
>      cc->gdb_read_register = avr_cpu_gdb_read_register;
>      cc->gdb_write_register = avr_cpu_gdb_write_register;
> +    cc->gdb_adjust_breakpoint = avr_cpu_gdb_adjust_breakpoint;
>      cc->gdb_num_core_regs = 35;
>      cc->gdb_core_xml_file = "avr-cpu.xml";
>      cc->tcg_ops = &avr_tcg_ops;
> diff --git a/target/avr/gdbstub.c b/target/avr/gdbstub.c
> index c28ed67efe..1c1b908c92 100644
> --- a/target/avr/gdbstub.c
> +++ b/target/avr/gdbstub.c
> @@ -82,3 +82,16 @@ int avr_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
>  
>      return 0;
>  }
> +
> +vaddr avr_cpu_gdb_adjust_breakpoint(CPUState *cpu, vaddr addr)
> +{
> +    /*
> +     * This is due to some strange GDB behavior
> +     * Let's assume main has address 0x100:
> +     * b main   - sets breakpoint at address 0x00000100 (code)

I'd say hardware breakpoint is used here (using the Breakpoint
Unit via JTAG),

> +     * b *0x100 - sets breakpoint at address 0x00800100 (data)

while software breakpoint is used here (insert a BREAK instruction
at that address).

> +     *
> +     * Force all breakpoints into code space.
> +     */
> +    return addr % OFFSET_DATA;
> +}

From ATmega640 DS:

The Break Point Unit implements Break on Change of Program Flow,
Single Step Break, two Program Memory Break Points, and two combined
Break Points. Together, the four Break Points can be configured as
either:
  • 4 single Program Memory Break Points

  • 3 Single Program Memory Break Points
    + 1 single Data Memory Break Point

  • 2 single Program Memory Break Points
    + 2 single Data Memory Break Points

  • 2 single Program Memory Break Points
    + 1 Program Memory Break Point with mask (“range Break Point”)

  • 2 single Program Memory Break Points
    + 1 Data Memory Break Point with mask (“range Break Point”)

A debugger, like the AVR Studio, may however use one or more of
these resources for its internal purpose, leaving less flexibility
to the end-user.

[...]

All necessary execution commands are available in AVR Studio, both
on source level and on disassembly level.
The user can execute the program, single step through the code either
by tracing into or stepping over functions, step out of functions,
place the cursor on a statement and execute until the statement is
reached, stop the execution, and reset the execution target.
In addition, the user can have an unlimited number of code Break
Points (using the BREAK instruction) and up to two data memory Break
Points, alternatively combined as a mask (range) Break Point.

I wish we didn't have to add gdb_adjust_breakpoint() but we can
remove it later, so for this patch:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

[PATCH for-6.1 v6 13/17] accel/tcg: Merge tb_find into its only caller

[Richard Henderson]

We are going to want two things:
(1) check for breakpoints will want to break out of the loop here,
(2) cflags can only be calculated with pc in hand.

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 accel/tcg/cpu-exec.c | 83 ++++++++++++++++++++++----------------------
 1 file changed, 41 insertions(+), 42 deletions(-)

diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index 5bb099174f..cde7069eb7 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -500,41 +500,6 @@ static inline void tb_add_jump(TranslationBlock *tb, int n,
     return;
 }
 
-static inline TranslationBlock *tb_find(CPUState *cpu,
-                                        TranslationBlock *last_tb,
-                                        int tb_exit, uint32_t cflags)
-{
-    CPUArchState *env = (CPUArchState *)cpu->env_ptr;
-    TranslationBlock *tb;
-    target_ulong cs_base, pc;
-    uint32_t flags;
-
-    cpu_get_tb_cpu_state(env, &pc, &cs_base, &flags);
-
-    tb = tb_lookup(cpu, pc, cs_base, flags, cflags);
-    if (tb == NULL) {
-        mmap_lock();
-        tb = tb_gen_code(cpu, pc, cs_base, flags, cflags);
-        mmap_unlock();
-        /* We add the TB in the virtual pc hash table for the fast lookup */
-        qatomic_set(&cpu->tb_jmp_cache[tb_jmp_cache_hash_func(pc)], tb);
-    }
-#ifndef CONFIG_USER_ONLY
-    /* We don't take care of direct jumps when address mapping changes in
-     * system emulation. So it's not safe to make a direct jump to a TB
-     * spanning two pages because the mapping for the second page can change.
-     */
-    if (tb->page_addr[1] != -1) {
-        last_tb = NULL;
-    }
-#endif
-    /* See if we can patch the calling TB. */
-    if (last_tb) {
-        tb_add_jump(last_tb, tb_exit, tb);
-    }
-    return tb;
-}
-
 static inline bool cpu_handle_halt(CPUState *cpu)
 {
     if (cpu->halted) {
@@ -868,22 +833,56 @@ int cpu_exec(CPUState *cpu)
         int tb_exit = 0;
 
         while (!cpu_handle_interrupt(cpu, &last_tb)) {
-            uint32_t cflags = cpu->cflags_next_tb;
             TranslationBlock *tb;
+            target_ulong cs_base, pc;
+            uint32_t flags, cflags;
 
-            /* When requested, use an exact setting for cflags for the next
-               execution.  This is used for icount, precise smc, and stop-
-               after-access watchpoints.  Since this request should never
-               have CF_INVALID set, -1 is a convenient invalid value that
-               does not require tcg headers for cpu_common_reset.  */
+            /*
+             * When requested, use an exact setting for cflags for the next
+             * execution.  This is used for icount, precise smc, and stop-
+             * after-access watchpoints.  Since this request should never
+             * have CF_INVALID set, -1 is a convenient invalid value that
+             * does not require tcg headers for cpu_common_reset.
+             */
+            cflags = cpu->cflags_next_tb;
             if (cflags == -1) {
                 cflags = curr_cflags(cpu);
             } else {
                 cpu->cflags_next_tb = -1;
             }
 
-            tb = tb_find(cpu, last_tb, tb_exit, cflags);
+            cpu_get_tb_cpu_state(cpu->env_ptr, &pc, &cs_base, &flags);
+
+            tb = tb_lookup(cpu, pc, cs_base, flags, cflags);
+            if (tb == NULL) {
+                mmap_lock();
+                tb = tb_gen_code(cpu, pc, cs_base, flags, cflags);
+                mmap_unlock();
+                /*
+                 * We add the TB in the virtual pc hash table
+                 * for the fast lookup
+                 */
+                qatomic_set(&cpu->tb_jmp_cache[tb_jmp_cache_hash_func(pc)], tb);
+            }
+
+#ifndef CONFIG_USER_ONLY
+            /*
+             * We don't take care of direct jumps when address mapping
+             * changes in system emulation.  So it's not safe to make a
+             * direct jump to a TB spanning two pages because the mapping
+             * for the second page can change.
+             */
+            if (tb->page_addr[1] != -1) {
+                last_tb = NULL;
+            }
+#endif
+            /* See if we can patch the calling TB. */
+            if (last_tb) {
+                tb_add_jump(last_tb, tb_exit, tb);
+            }
+
             cpu_loop_exec_tb(cpu, tb, &last_tb, &tb_exit);
+
             /* Try to align the host and virtual clocks
                if the guest is in advance */
             align_clocks(&sc, cpu);
-- 
2.25.1

[PATCH for-6.1 v6 14/17] accel/tcg: Move breakpoint recognition outside translation

[Richard Henderson]

Trigger breakpoints before beginning translation of a TB
that would begin with a BP.  Thus we never generate code
for the BP at all.

Single-step instructions within a page containing a BP so
that we are sure to check each insn for the BP as above.

We no longer need to flush any TBs when changing BPs.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/286
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/404
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/489
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 accel/tcg/cpu-exec.c   | 91 ++++++++++++++++++++++++++++++++++++++++--
 accel/tcg/translator.c | 24 +----------
 cpu.c                  | 20 ----------
 3 files changed, 89 insertions(+), 46 deletions(-)

diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index cde7069eb7..5cc6363f4c 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -222,6 +222,76 @@ static inline void log_cpu_exec(target_ulong pc, CPUState *cpu,
     }
 }
 
+static bool check_for_breakpoints(CPUState *cpu, target_ulong pc,
+                                  uint32_t *cflags)
+{
+    CPUBreakpoint *bp;
+    bool match_page = false;
+
+    if (likely(QTAILQ_EMPTY(&cpu->breakpoints))) {
+        return false;
+    }
+
+    /*
+     * Singlestep overrides breakpoints.
+     * This requirement is visible in the record-replay tests, where
+     * we would fail to make forward progress in reverse-continue.
+     *
+     * TODO: gdb singlestep should only override gdb breakpoints,
+     * so that one could (gdb) singlestep into the guest kernel's
+     * architectural breakpoint handler.
+     */
+    if (cpu->singlestep_enabled) {
+        return false;
+    }
+
+    QTAILQ_FOREACH(bp, &cpu->breakpoints, entry) {
+        /*
+         * If we have an exact pc match, trigger the breakpoint.
+         * Otherwise, note matches within the page.
+         */
+        if (pc == bp->pc) {
+            bool match_bp = false;
+
+            if (bp->flags & BP_GDB) {
+                match_bp = true;
+            } else if (bp->flags & BP_CPU) {
+#ifdef CONFIG_USER_ONLY
+                g_assert_not_reached();
+#else
+                CPUClass *cc = CPU_GET_CLASS(cpu);
+                assert(cc->tcg_ops->debug_check_breakpoint);
+                match_bp = cc->tcg_ops->debug_check_breakpoint(cpu);
+#endif
+            }
+
+            if (match_bp) {
+                cpu->exception_index = EXCP_DEBUG;
+                return true;
+            }
+        } else if (((pc ^ bp->pc) & TARGET_PAGE_MASK) == 0) {
+            match_page = true;
+        }
+    }
+
+    /*
+     * Within the same page as a breakpoint, single-step,
+     * returning to helper_lookup_tb_ptr after each insn looking
+     * for the actual breakpoint.
+     *
+     * TODO: Perhaps better to record all of the TBs associated
+     * with a given virtual page that contains a breakpoint, and
+     * then invalidate them when a new overlapping breakpoint is
+     * set on the page.  Non-overlapping TBs would not be
+     * invalidated, nor would any TB need to be invalidated as
+     * breakpoints are removed.
+     */
+    if (match_page) {
+        *cflags = (*cflags & ~CF_COUNT_MASK) | CF_NO_GOTO_TB | 1;
+    }
+    return false;
+}
+
 /**
  * helper_lookup_tb_ptr: quick check for next tb
  * @env: current cpu state
@@ -235,11 +305,16 @@ const void *HELPER(lookup_tb_ptr)(CPUArchState *env)
     CPUState *cpu = env_cpu(env);
     TranslationBlock *tb;
     target_ulong cs_base, pc;
-    uint32_t flags;
+    uint32_t flags, cflags;
 
     cpu_get_tb_cpu_state(env, &pc, &cs_base, &flags);
 
-    tb = tb_lookup(cpu, pc, cs_base, flags, curr_cflags(cpu));
+    cflags = curr_cflags(cpu);
+    if (check_for_breakpoints(cpu, pc, &cflags)) {
+        cpu_loop_exit(cpu);
+    }
+
+    tb = tb_lookup(cpu, pc, cs_base, flags, cflags);
     if (tb == NULL) {
         return tcg_code_gen_epilogue;
     }
@@ -346,6 +421,12 @@ void cpu_exec_step_atomic(CPUState *cpu)
         cflags &= ~CF_PARALLEL;
         /* After 1 insn, return and release the exclusive lock. */
         cflags |= CF_NO_GOTO_TB | CF_NO_GOTO_PTR | 1;
+        /*
+         * No need to check_for_breakpoints here.
+         * We only arrive in cpu_exec_step_atomic after beginning execution
+         * of an insn that includes an atomic operation we can't handle.
+         * Any breakpoint for this insn will have been recognized earlier.
+         */
 
         tb = tb_lookup(cpu, pc, cs_base, flags, cflags);
         if (tb == NULL) {
@@ -837,6 +918,8 @@ int cpu_exec(CPUState *cpu)
             target_ulong cs_base, pc;
             uint32_t flags, cflags;
 
+            cpu_get_tb_cpu_state(cpu->env_ptr, &pc, &cs_base, &flags);
+
             /*
              * When requested, use an exact setting for cflags for the next
              * execution.  This is used for icount, precise smc, and stop-
@@ -851,7 +934,9 @@ int cpu_exec(CPUState *cpu)
                 cpu->cflags_next_tb = -1;
             }
 
-            cpu_get_tb_cpu_state(cpu->env_ptr, &pc, &cs_base, &flags);
+            if (check_for_breakpoints(cpu, pc, &cflags)) {
+                break;
+            }
 
             tb = tb_lookup(cpu, pc, cs_base, flags, cflags);
             if (tb == NULL) {
diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c
index a59eb7c11b..4f3728c278 100644
--- a/accel/tcg/translator.c
+++ b/accel/tcg/translator.c
@@ -50,7 +50,6 @@ bool translator_use_goto_tb(DisasContextBase *db, target_ulong dest)
 void translator_loop(const TranslatorOps *ops, DisasContextBase *db,
                      CPUState *cpu, TranslationBlock *tb, int max_insns)
 {
-    int bp_insn = 0;
     bool plugin_enabled;
 
     /* Initialize DisasContext */
@@ -85,27 +84,6 @@ void translator_loop(const TranslatorOps *ops, DisasContextBase *db,
             plugin_gen_insn_start(cpu, db);
         }
 
-        /* Pass breakpoint hits to target for further processing */
-        if (!db->singlestep_enabled
-            && unlikely(!QTAILQ_EMPTY(&cpu->breakpoints))) {
-            CPUBreakpoint *bp;
-            QTAILQ_FOREACH(bp, &cpu->breakpoints, entry) {
-                if (bp->pc == db->pc_next) {
-                    if (ops->breakpoint_check(db, cpu, bp)) {
-                        bp_insn = 1;
-                        break;
-                    }
-                }
-            }
-            /* The breakpoint_check hook may use DISAS_TOO_MANY to indicate
-               that only one more instruction is to be executed.  Otherwise
-               it should use DISAS_NORETURN when generating an exception,
-               but may use a DISAS_TARGET_* value for Something Else.  */
-            if (db->is_jmp > DISAS_TOO_MANY) {
-                break;
-            }
-        }
-
         /* Disassemble one instruction.  The translate_insn hook should
            update db->pc_next and db->is_jmp to indicate what should be
            done next -- either exiting this loop or locate the start of
@@ -144,7 +122,7 @@ void translator_loop(const TranslatorOps *ops, DisasContextBase *db,
 
     /* Emit code to exit the TB, as indicated by db->is_jmp.  */
     ops->tb_stop(db, cpu);
-    gen_tb_end(db->tb, db->num_insns - bp_insn);
+    gen_tb_end(db->tb, db->num_insns);
 
     if (plugin_enabled) {
         plugin_gen_tb_end(cpu);
diff --git a/cpu.c b/cpu.c
index 91d9e38acb..d6ae5ae581 100644
--- a/cpu.c
+++ b/cpu.c
@@ -225,11 +225,6 @@ void tb_invalidate_phys_addr(target_ulong addr)
     tb_invalidate_phys_page_range(addr, addr + 1);
     mmap_unlock();
 }
-
-static void breakpoint_invalidate(CPUState *cpu, target_ulong pc)
-{
-    tb_invalidate_phys_addr(pc);
-}
 #else
 void tb_invalidate_phys_addr(AddressSpace *as, hwaddr addr, MemTxAttrs attrs)
 {
@@ -250,17 +245,6 @@ void tb_invalidate_phys_addr(AddressSpace *as, hwaddr addr, MemTxAttrs attrs)
     ram_addr = memory_region_get_ram_addr(mr) + addr;
     tb_invalidate_phys_page_range(ram_addr, ram_addr + 1);
 }
-
-static void breakpoint_invalidate(CPUState *cpu, target_ulong pc)
-{
-    /*
-     * There may not be a virtual to physical translation for the pc
-     * right now, but there may exist cached TB for this pc.
-     * Flush the whole TB cache to force re-translation of such TBs.
-     * This is heavyweight, but we're debugging anyway.
-     */
-    tb_flush(cpu);
-}
 #endif
 
 /* Add a breakpoint.  */
@@ -286,8 +270,6 @@ int cpu_breakpoint_insert(CPUState *cpu, vaddr pc, int flags,
         QTAILQ_INSERT_TAIL(&cpu->breakpoints, bp, entry);
     }
 
-    breakpoint_invalidate(cpu, pc);
-
     if (breakpoint) {
         *breakpoint = bp;
     }
@@ -320,8 +302,6 @@ void cpu_breakpoint_remove_by_ref(CPUState *cpu, CPUBreakpoint *bp)
 {
     QTAILQ_REMOVE(&cpu->breakpoints, bp, entry);
 
-    breakpoint_invalidate(cpu, bp->pc);
-
     trace_breakpoint_remove(cpu->cpu_index, bp->pc, bp->flags);
     g_free(bp);
 }
-- 
2.25.1

Re: [PATCH for-6.1 v6 14/17] accel/tcg: Move breakpoint recognition outside translation

[Philippe Mathieu-Daudé]

Hi,

On 20/7/21 21:54, Richard Henderson wrote:
> Trigger breakpoints before beginning translation of a TB
> that would begin with a BP.  Thus we never generate code
> for the BP at all.
> 
> Single-step instructions within a page containing a BP so
> that we are sure to check each insn for the BP as above.
> 
> We no longer need to flush any TBs when changing BPs.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/286
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/404
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/489
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   accel/tcg/cpu-exec.c   | 91 ++++++++++++++++++++++++++++++++++++++++--
>   accel/tcg/translator.c | 24 +----------
>   cpu.c                  | 20 ----------
>   3 files changed, 89 insertions(+), 46 deletions(-)


> diff --git a/cpu.c b/cpu.c
> index 91d9e38acb..d6ae5ae581 100644
> --- a/cpu.c
> +++ b/cpu.c
> @@ -225,11 +225,6 @@ void tb_invalidate_phys_addr(target_ulong addr)
>       tb_invalidate_phys_page_range(addr, addr + 1);
>       mmap_unlock();
>   }
> -
> -static void breakpoint_invalidate(CPUState *cpu, target_ulong pc)
> -{
> -    tb_invalidate_phys_addr(pc);
> -}

This patch removed the last use of tb_invalidate_phys_addr() in
user emulation:

   void tb_invalidate_phys_addr(hwaddr addr)
   {
       mmap_lock();
       tb_invalidate_phys_page(addr);
       mmap_unlock();
   }

Do we still need it?

(In sysemu there is a single use in Xtensa tb_invalidate_virtual_addr).

>   #else
>   void tb_invalidate_phys_addr(AddressSpace *as, hwaddr addr, MemTxAttrs attrs)
>   {
> @@ -250,17 +245,6 @@ void tb_invalidate_phys_addr(AddressSpace *as, hwaddr addr, MemTxAttrs attrs)
>       ram_addr = memory_region_get_ram_addr(mr) + addr;
>       tb_invalidate_phys_page_range(ram_addr, ram_addr + 1);
>   }
> -
> -static void breakpoint_invalidate(CPUState *cpu, target_ulong pc)
> -{
> -    /*
> -     * There may not be a virtual to physical translation for the pc
> -     * right now, but there may exist cached TB for this pc.
> -     * Flush the whole TB cache to force re-translation of such TBs.
> -     * This is heavyweight, but we're debugging anyway.
> -     */
> -    tb_flush(cpu);
> -}
>   #endif

Re: [PATCH for-6.1 v6 14/17] accel/tcg: Move breakpoint recognition outside translation

[Richard Henderson]

On 11/28/23 05:08, Philippe Mathieu-Daudé wrote:
> Hi,
> 
> On 20/7/21 21:54, Richard Henderson wrote:
>> Trigger breakpoints before beginning translation of a TB
>> that would begin with a BP.  Thus we never generate code
>> for the BP at all.
>>
>> Single-step instructions within a page containing a BP so
>> that we are sure to check each insn for the BP as above.
>>
>> We no longer need to flush any TBs when changing BPs.
>>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/286
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/404
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/489
>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>   accel/tcg/cpu-exec.c   | 91 ++++++++++++++++++++++++++++++++++++++++--
>>   accel/tcg/translator.c | 24 +----------
>>   cpu.c                  | 20 ----------
>>   3 files changed, 89 insertions(+), 46 deletions(-)
> 
> 
>> diff --git a/cpu.c b/cpu.c
>> index 91d9e38acb..d6ae5ae581 100644
>> --- a/cpu.c
>> +++ b/cpu.c
>> @@ -225,11 +225,6 @@ void tb_invalidate_phys_addr(target_ulong addr)
>>       tb_invalidate_phys_page_range(addr, addr + 1);
>>       mmap_unlock();
>>   }
>> -
>> -static void breakpoint_invalidate(CPUState *cpu, target_ulong pc)
>> -{
>> -    tb_invalidate_phys_addr(pc);
>> -}
> 
> This patch removed the last use of tb_invalidate_phys_addr() in
> user emulation:
> 
>    void tb_invalidate_phys_addr(hwaddr addr)
>    {
>        mmap_lock();
>        tb_invalidate_phys_page(addr);
>        mmap_unlock();
>    }
> 
> Do we still need it?

Probably not.

> (In sysemu there is a single use in Xtensa tb_invalidate_virtual_addr).

I suspect that should be migrated to use the common HW breakpoint support.


r~

Re: [PATCH for-6.1 v6 14/17] accel/tcg: Move breakpoint recognition outside translation

[Max Filippov]

On Tue, Nov 28, 2023 at 10:06 AM Richard Henderson
<richard.henderson@linaro.org> wrote:
> On 11/28/23 05:08, Philippe Mathieu-Daudé wrote:
> > (In sysemu there is a single use in Xtensa tb_invalidate_virtual_addr).
>
> I suspect that should be migrated to use the common HW breakpoint support.

I'm taking a look.

-- 
Thanks.
-- Max

[PATCH for-6.1 v6 15/17] accel/tcg: Remove TranslatorOps.breakpoint_check

[Richard Henderson]

The hook is now unused, with breakpoints checked outside translation.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/exec/translator.h     | 11 -----------
 target/arm/helper.h           |  2 --
 target/alpha/translate.c      | 16 ----------------
 target/arm/debug_helper.c     |  7 -------
 target/arm/translate-a64.c    | 25 -------------------------
 target/arm/translate.c        | 29 -----------------------------
 target/avr/translate.c        | 18 ------------------
 target/cris/translate.c       | 20 --------------------
 target/hexagon/translate.c    | 17 -----------------
 target/hppa/translate.c       | 11 -----------
 target/i386/tcg/translate.c   | 28 ----------------------------
 target/m68k/translate.c       | 18 ------------------
 target/microblaze/translate.c | 18 ------------------
 target/mips/tcg/translate.c   | 19 -------------------
 target/nios2/translate.c      | 27 ---------------------------
 target/openrisc/translate.c   | 17 -----------------
 target/ppc/translate.c        | 18 ------------------
 target/riscv/translate.c      | 17 -----------------
 target/rx/translate.c         | 14 --------------
 target/s390x/tcg/translate.c  | 24 ------------------------
 target/sh4/translate.c        | 18 ------------------
 target/sparc/translate.c      | 17 -----------------
 target/tricore/translate.c    | 16 ----------------
 target/xtensa/translate.c     | 17 -----------------
 24 files changed, 424 deletions(-)

diff --git a/include/exec/translator.h b/include/exec/translator.h
index dd9c06d40d..d318803267 100644
--- a/include/exec/translator.h
+++ b/include/exec/translator.h
@@ -89,15 +89,6 @@ typedef struct DisasContextBase {
  * @insn_start:
  *      Emit the tcg_gen_insn_start opcode.
  *
- * @breakpoint_check:
- *      When called, the breakpoint has already been checked to match the PC,
- *      but the target may decide the breakpoint missed the address
- *      (e.g., due to conditions encoded in their flags).  Return true to
- *      indicate that the breakpoint did hit, in which case no more breakpoints
- *      are checked.  If the breakpoint did hit, emit any code required to
- *      signal the exception, and set db->is_jmp as necessary to terminate
- *      the main loop.
- *
  * @translate_insn:
  *      Disassemble one instruction and set db->pc_next for the start
  *      of the following instruction.  Set db->is_jmp as necessary to
@@ -113,8 +104,6 @@ typedef struct TranslatorOps {
     void (*init_disas_context)(DisasContextBase *db, CPUState *cpu);
     void (*tb_start)(DisasContextBase *db, CPUState *cpu);
     void (*insn_start)(DisasContextBase *db, CPUState *cpu);
-    bool (*breakpoint_check)(DisasContextBase *db, CPUState *cpu,
-                             const CPUBreakpoint *bp);
     void (*translate_insn)(DisasContextBase *db, CPUState *cpu);
     void (*tb_stop)(DisasContextBase *db, CPUState *cpu);
     void (*disas_log)(const DisasContextBase *db, CPUState *cpu);
diff --git a/target/arm/helper.h b/target/arm/helper.h
index db87d7d537..248569b0cd 100644
--- a/target/arm/helper.h
+++ b/target/arm/helper.h
@@ -54,8 +54,6 @@ DEF_HELPER_1(yield, void, env)
 DEF_HELPER_1(pre_hvc, void, env)
 DEF_HELPER_2(pre_smc, void, env, i32)
 
-DEF_HELPER_1(check_breakpoints, void, env)
-
 DEF_HELPER_3(cpsr_write, void, env, i32, i32)
 DEF_HELPER_2(cpsr_write_eret, void, env, i32)
 DEF_HELPER_1(cpsr_read, i32, env)
diff --git a/target/alpha/translate.c b/target/alpha/translate.c
index 949ba6ffde..de6c0a8439 100644
--- a/target/alpha/translate.c
+++ b/target/alpha/translate.c
@@ -2967,21 +2967,6 @@ static void alpha_tr_insn_start(DisasContextBase *dcbase, CPUState *cpu)
     tcg_gen_insn_start(dcbase->pc_next);
 }
 
-static bool alpha_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cpu,
-                                      const CPUBreakpoint *bp)
-{
-    DisasContext *ctx = container_of(dcbase, DisasContext, base);
-
-    ctx->base.is_jmp = gen_excp(ctx, EXCP_DEBUG, 0);
-
-    /* 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.  */
-    ctx->base.pc_next += 4;
-    return true;
-}
-
 static void alpha_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
 {
     DisasContext *ctx = container_of(dcbase, DisasContext, base);
@@ -3040,7 +3025,6 @@ static const TranslatorOps alpha_tr_ops = {
     .init_disas_context = alpha_tr_init_disas_context,
     .tb_start           = alpha_tr_tb_start,
     .insn_start         = alpha_tr_insn_start,
-    .breakpoint_check   = alpha_tr_breakpoint_check,
     .translate_insn     = alpha_tr_translate_insn,
     .tb_stop            = alpha_tr_tb_stop,
     .disas_log          = alpha_tr_disas_log,
diff --git a/target/arm/debug_helper.c b/target/arm/debug_helper.c
index 4a0c479527..2983e36dd3 100644
--- a/target/arm/debug_helper.c
+++ b/target/arm/debug_helper.c
@@ -239,13 +239,6 @@ bool arm_debug_check_breakpoint(CPUState *cs)
     return false;
 }
 
-void HELPER(check_breakpoints)(CPUARMState *env)
-{
-    if (arm_debug_check_breakpoint(env_cpu(env))) {
-        HELPER(exception_internal(env, EXCP_DEBUG));
-    }
-}
-
 bool arm_debug_check_watchpoint(CPUState *cs, CPUWatchpoint *wp)
 {
     /*
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index ca11a5fecd..422e2ac0c9 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -14844,30 +14844,6 @@ static void aarch64_tr_insn_start(DisasContextBase *dcbase, CPUState *cpu)
     dc->insn_start = tcg_last_op();
 }
 
-static bool aarch64_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cpu,
-                                        const CPUBreakpoint *bp)
-{
-    DisasContext *dc = container_of(dcbase, DisasContext, base);
-
-    if (bp->flags & BP_CPU) {
-        gen_a64_set_pc_im(dc->base.pc_next);
-        gen_helper_check_breakpoints(cpu_env);
-        /* End the TB early; it likely won't be executed */
-        dc->base.is_jmp = DISAS_TOO_MANY;
-    } else {
-        gen_exception_internal_insn(dc, dc->base.pc_next, 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->base.pc_next += 4;
-        dc->base.is_jmp = DISAS_NORETURN;
-    }
-
-    return true;
-}
-
 static void aarch64_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
 {
     DisasContext *dc = container_of(dcbase, DisasContext, base);
@@ -14982,7 +14958,6 @@ const TranslatorOps aarch64_translator_ops = {
     .init_disas_context = aarch64_tr_init_disas_context,
     .tb_start           = aarch64_tr_tb_start,
     .insn_start         = aarch64_tr_insn_start,
-    .breakpoint_check   = aarch64_tr_breakpoint_check,
     .translate_insn     = aarch64_tr_translate_insn,
     .tb_stop            = aarch64_tr_tb_stop,
     .disas_log          = aarch64_tr_disas_log,
diff --git a/target/arm/translate.c b/target/arm/translate.c
index e1a8152598..351afa43a2 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -9438,33 +9438,6 @@ static void arm_tr_insn_start(DisasContextBase *dcbase, CPUState *cpu)
     dc->insn_start = tcg_last_op();
 }
 
-static bool arm_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cpu,
-                                    const CPUBreakpoint *bp)
-{
-    DisasContext *dc = container_of(dcbase, DisasContext, base);
-
-    if (bp->flags & BP_CPU) {
-        gen_set_condexec(dc);
-        gen_set_pc_im(dc, dc->base.pc_next);
-        gen_helper_check_breakpoints(cpu_env);
-        /* End the TB early; it's likely not going to be executed */
-        dc->base.is_jmp = DISAS_TOO_MANY;
-    } else {
-        gen_exception_internal_insn(dc, dc->base.pc_next, 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.  */
-        /* TODO: Advance PC by correct instruction length to
-         * avoid disassembler error messages */
-        dc->base.pc_next += 2;
-        dc->base.is_jmp = DISAS_NORETURN;
-    }
-
-    return true;
-}
-
 static bool arm_pre_translate_insn(DisasContext *dc)
 {
 #ifdef CONFIG_USER_ONLY
@@ -9827,7 +9800,6 @@ static const TranslatorOps arm_translator_ops = {
     .init_disas_context = arm_tr_init_disas_context,
     .tb_start           = arm_tr_tb_start,
     .insn_start         = arm_tr_insn_start,
-    .breakpoint_check   = arm_tr_breakpoint_check,
     .translate_insn     = arm_tr_translate_insn,
     .tb_stop            = arm_tr_tb_stop,
     .disas_log          = arm_tr_disas_log,
@@ -9837,7 +9809,6 @@ static const TranslatorOps thumb_translator_ops = {
     .init_disas_context = arm_tr_init_disas_context,
     .tb_start           = arm_tr_tb_start,
     .insn_start         = arm_tr_insn_start,
-    .breakpoint_check   = arm_tr_breakpoint_check,
     .translate_insn     = thumb_tr_translate_insn,
     .tb_stop            = arm_tr_tb_stop,
     .disas_log          = arm_tr_disas_log,
diff --git a/target/avr/translate.c b/target/avr/translate.c
index f7202a646b..1111e08b83 100644
--- a/target/avr/translate.c
+++ b/target/avr/translate.c
@@ -2900,14 +2900,6 @@ static bool canonicalize_skip(DisasContext *ctx)
     return true;
 }
 
-static void gen_breakpoint(DisasContext *ctx)
-{
-    canonicalize_skip(ctx);
-    tcg_gen_movi_tl(cpu_pc, ctx->npc);
-    gen_helper_debug(cpu_env);
-    ctx->base.is_jmp = DISAS_NORETURN;
-}
-
 static void avr_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
 {
     DisasContext *ctx = container_of(dcbase, DisasContext, base);
@@ -2944,15 +2936,6 @@ static void avr_tr_insn_start(DisasContextBase *dcbase, CPUState *cs)
     tcg_gen_insn_start(ctx->npc);
 }
 
-static bool avr_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cs,
-                                    const CPUBreakpoint *bp)
-{
-    DisasContext *ctx = container_of(dcbase, DisasContext, base);
-
-    gen_breakpoint(ctx);
-    return true;
-}
-
 static void avr_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs)
 {
     DisasContext *ctx = container_of(dcbase, DisasContext, base);
@@ -3055,7 +3038,6 @@ static const TranslatorOps avr_tr_ops = {
     .init_disas_context = avr_tr_init_disas_context,
     .tb_start           = avr_tr_tb_start,
     .insn_start         = avr_tr_insn_start,
-    .breakpoint_check   = avr_tr_breakpoint_check,
     .translate_insn     = avr_tr_translate_insn,
     .tb_stop            = avr_tr_tb_stop,
     .disas_log          = avr_tr_disas_log,
diff --git a/target/cris/translate.c b/target/cris/translate.c
index 9258c13e9f..a84b753349 100644
--- a/target/cris/translate.c
+++ b/target/cris/translate.c
@@ -3118,25 +3118,6 @@ static void cris_tr_insn_start(DisasContextBase *dcbase, CPUState *cpu)
     tcg_gen_insn_start(dc->delayed_branch == 1 ? dc->ppc | 1 : dc->pc);
 }
 
-static bool cris_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cpu,
-                                     const CPUBreakpoint *bp)
-{
-    DisasContext *dc = container_of(dcbase, DisasContext, base);
-
-    cris_evaluate_flags(dc);
-    tcg_gen_movi_tl(env_pc, dc->pc);
-    t_gen_raise_exception(EXCP_DEBUG);
-    dc->base.is_jmp = DISAS_NORETURN;
-    /*
-     * 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 += 2;
-    return true;
-}
-
 static void cris_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs)
 {
     DisasContext *dc = container_of(dcbase, DisasContext, base);
@@ -3315,7 +3296,6 @@ static const TranslatorOps cris_tr_ops = {
     .init_disas_context = cris_tr_init_disas_context,
     .tb_start           = cris_tr_tb_start,
     .insn_start         = cris_tr_insn_start,
-    .breakpoint_check   = cris_tr_breakpoint_check,
     .translate_insn     = cris_tr_translate_insn,
     .tb_stop            = cris_tr_tb_stop,
     .disas_log          = cris_tr_disas_log,
diff --git a/target/hexagon/translate.c b/target/hexagon/translate.c
index b23d36adf5..54fdcaa5e8 100644
--- a/target/hexagon/translate.c
+++ b/target/hexagon/translate.c
@@ -540,22 +540,6 @@ static void hexagon_tr_insn_start(DisasContextBase *dcbase, CPUState *cpu)
     tcg_gen_insn_start(ctx->base.pc_next);
 }
 
-static bool hexagon_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cpu,
-                                        const CPUBreakpoint *bp)
-{
-    DisasContext *ctx = container_of(dcbase, DisasContext, base);
-
-    gen_exception_end_tb(ctx, 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.
-     */
-    ctx->base.pc_next += 4;
-    return true;
-}
-
 static bool pkt_crosses_page(CPUHexagonState *env, DisasContext *ctx)
 {
     target_ulong page_start = ctx->base.pc_first & TARGET_PAGE_MASK;
@@ -631,7 +615,6 @@ static const TranslatorOps hexagon_tr_ops = {
     .init_disas_context = hexagon_tr_init_disas_context,
     .tb_start           = hexagon_tr_tb_start,
     .insn_start         = hexagon_tr_insn_start,
-    .breakpoint_check   = hexagon_tr_breakpoint_check,
     .translate_insn     = hexagon_tr_translate_packet,
     .tb_stop            = hexagon_tr_tb_stop,
     .disas_log          = hexagon_tr_disas_log,
diff --git a/target/hppa/translate.c b/target/hppa/translate.c
index 2552747138..b18150ef8d 100644
--- a/target/hppa/translate.c
+++ b/target/hppa/translate.c
@@ -4159,16 +4159,6 @@ static void hppa_tr_insn_start(DisasContextBase *dcbase, CPUState *cs)
     tcg_gen_insn_start(ctx->iaoq_f, ctx->iaoq_b);
 }
 
-static bool hppa_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cs,
-                                      const CPUBreakpoint *bp)
-{
-    DisasContext *ctx = container_of(dcbase, DisasContext, base);
-
-    gen_excp(ctx, EXCP_DEBUG);
-    ctx->base.pc_next += 4;
-    return true;
-}
-
 static void hppa_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs)
 {
     DisasContext *ctx = container_of(dcbase, DisasContext, base);
@@ -4330,7 +4320,6 @@ static const TranslatorOps hppa_tr_ops = {
     .init_disas_context = hppa_tr_init_disas_context,
     .tb_start           = hppa_tr_tb_start,
     .insn_start         = hppa_tr_insn_start,
-    .breakpoint_check   = hppa_tr_breakpoint_check,
     .translate_insn     = hppa_tr_translate_insn,
     .tb_stop            = hppa_tr_tb_stop,
     .disas_log          = hppa_tr_disas_log,
diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
index 8520d5a1e2..aacb605eee 100644
--- a/target/i386/tcg/translate.c
+++ b/target/i386/tcg/translate.c
@@ -2604,14 +2604,6 @@ static void gen_interrupt(DisasContext *s, int intno,
     s->base.is_jmp = DISAS_NORETURN;
 }
 
-static void gen_debug(DisasContext *s)
-{
-    gen_update_cc_op(s);
-    gen_jmp_im(s, s->base.pc_next - s->cs_base);
-    gen_helper_debug(cpu_env);
-    s->base.is_jmp = DISAS_NORETURN;
-}
-
 static void gen_set_hflag(DisasContext *s, uint32_t mask)
 {
     if ((s->flags & mask) == 0) {
@@ -8635,25 +8627,6 @@ static void i386_tr_insn_start(DisasContextBase *dcbase, CPUState *cpu)
     tcg_gen_insn_start(dc->base.pc_next, dc->cc_op);
 }
 
-static bool i386_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cpu,
-                                     const CPUBreakpoint *bp)
-{
-    DisasContext *dc = container_of(dcbase, DisasContext, base);
-    /* If RF is set, suppress an internally generated breakpoint.  */
-    int flags = dc->base.tb->flags & HF_RF_MASK ? BP_GDB : BP_ANY;
-    if (bp->flags & flags) {
-        gen_debug(dc);
-        /* 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 generic logic setting tb->size later does the right thing.  */
-        dc->base.pc_next += 1;
-        return true;
-    } else {
-        return false;
-    }
-}
-
 static void i386_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
 {
     DisasContext *dc = container_of(dcbase, DisasContext, base);
@@ -8721,7 +8694,6 @@ static const TranslatorOps i386_tr_ops = {
     .init_disas_context = i386_tr_init_disas_context,
     .tb_start           = i386_tr_tb_start,
     .insn_start         = i386_tr_insn_start,
-    .breakpoint_check   = i386_tr_breakpoint_check,
     .translate_insn     = i386_tr_translate_insn,
     .tb_stop            = i386_tr_tb_stop,
     .disas_log          = i386_tr_disas_log,
diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index 1fee04b8dd..c34d9aed61 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -6208,23 +6208,6 @@ static void m68k_tr_insn_start(DisasContextBase *dcbase, CPUState *cpu)
     tcg_gen_insn_start(dc->base.pc_next, dc->cc_op);
 }
 
-static bool m68k_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cpu,
-                                     const CPUBreakpoint *bp)
-{
-    DisasContext *dc = container_of(dcbase, DisasContext, base);
-
-    gen_exception(dc, dc->base.pc_next, 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->base.pc_next += 2;
-
-    return true;
-}
-
 static void m68k_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
 {
     DisasContext *dc = container_of(dcbase, DisasContext, base);
@@ -6310,7 +6293,6 @@ static const TranslatorOps m68k_tr_ops = {
     .init_disas_context = m68k_tr_init_disas_context,
     .tb_start           = m68k_tr_tb_start,
     .insn_start         = m68k_tr_insn_start,
-    .breakpoint_check   = m68k_tr_breakpoint_check,
     .translate_insn     = m68k_tr_translate_insn,
     .tb_stop            = m68k_tr_tb_stop,
     .disas_log          = m68k_tr_disas_log,
diff --git a/target/microblaze/translate.c b/target/microblaze/translate.c
index c68a84a219..a14ffed784 100644
--- a/target/microblaze/translate.c
+++ b/target/microblaze/translate.c
@@ -1673,23 +1673,6 @@ static void mb_tr_insn_start(DisasContextBase *dcb, CPUState *cs)
     dc->insn_start = tcg_last_op();
 }
 
-static bool mb_tr_breakpoint_check(DisasContextBase *dcb, CPUState *cs,
-                                   const CPUBreakpoint *bp)
-{
-    DisasContext *dc = container_of(dcb, DisasContext, base);
-
-    gen_raise_exception_sync(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->base.pc_next += 4;
-    return true;
-}
-
 static void mb_tr_translate_insn(DisasContextBase *dcb, CPUState *cs)
 {
     DisasContext *dc = container_of(dcb, DisasContext, base);
@@ -1854,7 +1837,6 @@ static const TranslatorOps mb_tr_ops = {
     .init_disas_context = mb_tr_init_disas_context,
     .tb_start           = mb_tr_tb_start,
     .insn_start         = mb_tr_insn_start,
-    .breakpoint_check   = mb_tr_breakpoint_check,
     .translate_insn     = mb_tr_translate_insn,
     .tb_stop            = mb_tr_tb_stop,
     .disas_log          = mb_tr_disas_log,
diff --git a/target/mips/tcg/translate.c b/target/mips/tcg/translate.c
index fd980ea966..5b03545f09 100644
--- a/target/mips/tcg/translate.c
+++ b/target/mips/tcg/translate.c
@@ -16178,24 +16178,6 @@ static void mips_tr_insn_start(DisasContextBase *dcbase, CPUState *cs)
                        ctx->btarget);
 }
 
-static bool mips_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cs,
-                                     const CPUBreakpoint *bp)
-{
-    DisasContext *ctx = container_of(dcbase, DisasContext, base);
-
-    save_cpu_state(ctx, 1);
-    ctx->base.is_jmp = DISAS_NORETURN;
-    gen_helper_raise_exception_debug(cpu_env);
-    /*
-     * 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.
-     */
-    ctx->base.pc_next += 4;
-    return true;
-}
-
 static void mips_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs)
 {
     CPUMIPSState *env = cs->env_ptr;
@@ -16303,7 +16285,6 @@ static const TranslatorOps mips_tr_ops = {
     .init_disas_context = mips_tr_init_disas_context,
     .tb_start           = mips_tr_tb_start,
     .insn_start         = mips_tr_insn_start,
-    .breakpoint_check   = mips_tr_breakpoint_check,
     .translate_insn     = mips_tr_translate_insn,
     .tb_stop            = mips_tr_tb_stop,
     .disas_log          = mips_tr_disas_log,
diff --git a/target/nios2/translate.c b/target/nios2/translate.c
index 17742cebc7..08d7ac5398 100644
--- a/target/nios2/translate.c
+++ b/target/nios2/translate.c
@@ -744,16 +744,6 @@ static const char * const regnames[] = {
 
 #include "exec/gen-icount.h"
 
-static void gen_exception(DisasContext *dc, uint32_t excp)
-{
-    TCGv_i32 tmp = tcg_const_i32(excp);
-
-    tcg_gen_movi_tl(cpu_R[R_PC], dc->pc);
-    gen_helper_raise_exception(cpu_env, tmp);
-    tcg_temp_free_i32(tmp);
-    dc->base.is_jmp = DISAS_NORETURN;
-}
-
 /* generate intermediate code for basic block 'tb'.  */
 static void nios2_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
 {
@@ -777,22 +767,6 @@ static void nios2_tr_insn_start(DisasContextBase *dcbase, CPUState *cs)
     tcg_gen_insn_start(dcbase->pc_next);
 }
 
-static bool nios2_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cs,
-                                      const CPUBreakpoint *bp)
-{
-    DisasContext *dc = container_of(dcbase, DisasContext, base);
-
-    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->base.pc_next += 4;
-    return true;
-}
-
 static void nios2_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs)
 {
     DisasContext *dc = container_of(dcbase, DisasContext, base);
@@ -870,7 +844,6 @@ 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,
diff --git a/target/openrisc/translate.c b/target/openrisc/translate.c
index 059da48475..d6ea536744 100644
--- a/target/openrisc/translate.c
+++ b/target/openrisc/translate.c
@@ -1609,22 +1609,6 @@ static void openrisc_tr_insn_start(DisasContextBase *dcbase, CPUState *cs)
                        | (dc->base.num_insns > 1 ? 2 : 0));
 }
 
-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_NORETURN;
-    /* 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);
@@ -1727,7 +1711,6 @@ 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,
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 0a55cb7181..171b216e17 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -8565,23 +8565,6 @@ static void ppc_tr_insn_start(DisasContextBase *dcbase, CPUState *cs)
     tcg_gen_insn_start(dcbase->pc_next);
 }
 
-static bool ppc_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cs,
-                                    const CPUBreakpoint *bp)
-{
-    DisasContext *ctx = container_of(dcbase, DisasContext, base);
-
-    gen_update_nip(ctx, ctx->base.pc_next);
-    gen_debug_exception(ctx);
-    /*
-     * 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.
-     */
-    ctx->base.pc_next += 4;
-    return true;
-}
-
 static bool is_prefix_insn(DisasContext *ctx, uint32_t insn)
 {
     REQUIRE_INSNS_FLAGS2(ctx, ISA310);
@@ -8710,7 +8693,6 @@ static const TranslatorOps ppc_tr_ops = {
     .init_disas_context = ppc_tr_init_disas_context,
     .tb_start           = ppc_tr_tb_start,
     .insn_start         = ppc_tr_insn_start,
-    .breakpoint_check   = ppc_tr_breakpoint_check,
     .translate_insn     = ppc_tr_translate_insn,
     .tb_stop            = ppc_tr_tb_stop,
     .disas_log          = ppc_tr_disas_log,
diff --git a/target/riscv/translate.c b/target/riscv/translate.c
index deda0c8a44..6983be5723 100644
--- a/target/riscv/translate.c
+++ b/target/riscv/translate.c
@@ -961,22 +961,6 @@ static void riscv_tr_insn_start(DisasContextBase *dcbase, CPUState *cpu)
     tcg_gen_insn_start(ctx->base.pc_next);
 }
 
-static bool riscv_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cpu,
-                                      const CPUBreakpoint *bp)
-{
-    DisasContext *ctx = container_of(dcbase, DisasContext, base);
-
-    tcg_gen_movi_tl(cpu_pc, ctx->base.pc_next);
-    ctx->base.is_jmp = DISAS_NORETURN;
-    gen_exception_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.  */
-    ctx->base.pc_next += 4;
-    return true;
-}
-
 static void riscv_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
 {
     DisasContext *ctx = container_of(dcbase, DisasContext, base);
@@ -1029,7 +1013,6 @@ static const TranslatorOps riscv_tr_ops = {
     .init_disas_context = riscv_tr_init_disas_context,
     .tb_start           = riscv_tr_tb_start,
     .insn_start         = riscv_tr_insn_start,
-    .breakpoint_check   = riscv_tr_breakpoint_check,
     .translate_insn     = riscv_tr_translate_insn,
     .tb_stop            = riscv_tr_tb_stop,
     .disas_log          = riscv_tr_disas_log,
diff --git a/target/rx/translate.c b/target/rx/translate.c
index 23a626438a..a3cf720455 100644
--- a/target/rx/translate.c
+++ b/target/rx/translate.c
@@ -2309,19 +2309,6 @@ static void rx_tr_insn_start(DisasContextBase *dcbase, CPUState *cs)
     tcg_gen_insn_start(ctx->base.pc_next);
 }
 
-static bool rx_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cs,
-                                    const CPUBreakpoint *bp)
-{
-    DisasContext *ctx = container_of(dcbase, DisasContext, base);
-
-    /* We have hit a breakpoint - make sure PC is up-to-date */
-    tcg_gen_movi_i32(cpu_pc, ctx->base.pc_next);
-    gen_helper_debug(cpu_env);
-    ctx->base.is_jmp = DISAS_NORETURN;
-    ctx->base.pc_next += 1;
-    return true;
-}
-
 static void rx_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs)
 {
     DisasContext *ctx = container_of(dcbase, DisasContext, base);
@@ -2373,7 +2360,6 @@ static const TranslatorOps rx_tr_ops = {
     .init_disas_context = rx_tr_init_disas_context,
     .tb_start           = rx_tr_tb_start,
     .insn_start         = rx_tr_insn_start,
-    .breakpoint_check   = rx_tr_breakpoint_check,
     .translate_insn     = rx_tr_translate_insn,
     .tb_stop            = rx_tr_tb_stop,
     .disas_log          = rx_tr_disas_log,
diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index 92fa7656c2..0632b0374b 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -6552,29 +6552,6 @@ static void s390x_tr_insn_start(DisasContextBase *dcbase, CPUState *cs)
 {
 }
 
-static bool s390x_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cs,
-                                      const CPUBreakpoint *bp)
-{
-    DisasContext *dc = container_of(dcbase, DisasContext, base);
-
-    /*
-     * Emit an insn_start to accompany the breakpoint exception.
-     * The ILEN value is a dummy, since this does not result in
-     * an s390x exception, but an internal qemu exception which
-     * brings us back to interact with the gdbstub.
-     */
-    tcg_gen_insn_start(dc->base.pc_next, dc->cc_op, 2);
-
-    dc->base.is_jmp = DISAS_PC_STALE;
-    dc->do_debug = true;
-    /* 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 does the right thing.  */
-    dc->base.pc_next += 2;
-    return true;
-}
-
 static void s390x_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs)
 {
     CPUS390XState *env = cs->env_ptr;
@@ -6642,7 +6619,6 @@ static const TranslatorOps s390x_tr_ops = {
     .init_disas_context = s390x_tr_init_disas_context,
     .tb_start           = s390x_tr_tb_start,
     .insn_start         = s390x_tr_insn_start,
-    .breakpoint_check   = s390x_tr_breakpoint_check,
     .translate_insn     = s390x_tr_translate_insn,
     .tb_stop            = s390x_tr_tb_stop,
     .disas_log          = s390x_tr_disas_log,
diff --git a/target/sh4/translate.c b/target/sh4/translate.c
index 40898e2393..8704fea1ca 100644
--- a/target/sh4/translate.c
+++ b/target/sh4/translate.c
@@ -2289,23 +2289,6 @@ static void sh4_tr_insn_start(DisasContextBase *dcbase, CPUState *cs)
     tcg_gen_insn_start(ctx->base.pc_next, ctx->envflags);
 }
 
-static bool sh4_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cs,
-                                    const CPUBreakpoint *bp)
-{
-    DisasContext *ctx = container_of(dcbase, DisasContext, base);
-
-    /* We have hit a breakpoint - make sure PC is up-to-date */
-    gen_save_cpu_state(ctx, true);
-    gen_helper_debug(cpu_env);
-    ctx->base.is_jmp = DISAS_NORETURN;
-    /* 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.  */
-    ctx->base.pc_next += 2;
-    return true;
-}
-
 static void sh4_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs)
 {
     CPUSH4State *env = cs->env_ptr;
@@ -2369,7 +2352,6 @@ static const TranslatorOps sh4_tr_ops = {
     .init_disas_context = sh4_tr_init_disas_context,
     .tb_start           = sh4_tr_tb_start,
     .insn_start         = sh4_tr_insn_start,
-    .breakpoint_check   = sh4_tr_breakpoint_check,
     .translate_insn     = sh4_tr_translate_insn,
     .tb_stop            = sh4_tr_tb_stop,
     .disas_log          = sh4_tr_disas_log,
diff --git a/target/sparc/translate.c b/target/sparc/translate.c
index e530cb4aa8..11de5a4963 100644
--- a/target/sparc/translate.c
+++ b/target/sparc/translate.c
@@ -5854,22 +5854,6 @@ static void sparc_tr_insn_start(DisasContextBase *dcbase, CPUState *cs)
     }
 }
 
-static bool sparc_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cs,
-                                      const CPUBreakpoint *bp)
-{
-    DisasContext *dc = container_of(dcbase, DisasContext, base);
-
-    if (dc->pc != dc->base.pc_first) {
-        save_state(dc);
-    }
-    gen_helper_debug(cpu_env);
-    tcg_gen_exit_tb(NULL, 0);
-    dc->base.is_jmp = DISAS_NORETURN;
-    /* update pc_next so that the current instruction is included in tb->size */
-    dc->base.pc_next += 4;
-    return true;
-}
-
 static void sparc_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs)
 {
     DisasContext *dc = container_of(dcbase, DisasContext, base);
@@ -5932,7 +5916,6 @@ static const TranslatorOps sparc_tr_ops = {
     .init_disas_context = sparc_tr_init_disas_context,
     .tb_start           = sparc_tr_tb_start,
     .insn_start         = sparc_tr_insn_start,
-    .breakpoint_check   = sparc_tr_breakpoint_check,
     .translate_insn     = sparc_tr_translate_insn,
     .tb_stop            = sparc_tr_tb_stop,
     .disas_log          = sparc_tr_disas_log,
diff --git a/target/tricore/translate.c b/target/tricore/translate.c
index 865020754d..a0cc0f1cb3 100644
--- a/target/tricore/translate.c
+++ b/target/tricore/translate.c
@@ -8810,21 +8810,6 @@ static void tricore_tr_insn_start(DisasContextBase *dcbase, CPUState *cpu)
     tcg_gen_insn_start(ctx->base.pc_next);
 }
 
-static bool tricore_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cpu,
-                                      const CPUBreakpoint *bp)
-{
-    DisasContext *ctx = container_of(dcbase, DisasContext, base);
-    generate_qemu_excp(ctx, 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.
-     */
-    ctx->base.pc_next += 4;
-    return true;
-}
-
 static bool insn_crosses_page(CPUTriCoreState *env, DisasContext *ctx)
 {
     /*
@@ -8898,7 +8883,6 @@ static const TranslatorOps tricore_tr_ops = {
     .init_disas_context = tricore_tr_init_disas_context,
     .tb_start           = tricore_tr_tb_start,
     .insn_start         = tricore_tr_insn_start,
-    .breakpoint_check   = tricore_tr_breakpoint_check,
     .translate_insn     = tricore_tr_translate_insn,
     .tb_stop            = tricore_tr_tb_stop,
     .disas_log          = tricore_tr_disas_log,
diff --git a/target/xtensa/translate.c b/target/xtensa/translate.c
index 7094cfcf1d..20399d6a04 100644
--- a/target/xtensa/translate.c
+++ b/target/xtensa/translate.c
@@ -1232,22 +1232,6 @@ static void xtensa_tr_insn_start(DisasContextBase *dcbase, CPUState *cpu)
     tcg_gen_insn_start(dcbase->pc_next);
 }
 
-static bool xtensa_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cpu,
-                                       const CPUBreakpoint *bp)
-{
-    DisasContext *dc = container_of(dcbase, DisasContext, base);
-
-    tcg_gen_movi_i32(cpu_pc, dc->base.pc_next);
-    gen_exception(dc, EXCP_DEBUG);
-    dc->base.is_jmp = DISAS_NORETURN;
-    /* 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 += 2;
-    return true;
-}
-
 static void xtensa_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
 {
     DisasContext *dc = container_of(dcbase, DisasContext, base);
@@ -1330,7 +1314,6 @@ static const TranslatorOps xtensa_translator_ops = {
     .init_disas_context = xtensa_tr_init_disas_context,
     .tb_start           = xtensa_tr_tb_start,
     .insn_start         = xtensa_tr_insn_start,
-    .breakpoint_check   = xtensa_tr_breakpoint_check,
     .translate_insn     = xtensa_tr_translate_insn,
     .tb_stop            = xtensa_tr_tb_stop,
     .disas_log          = xtensa_tr_disas_log,
-- 
2.25.1

Re: [PATCH for-6.1 v6 15/17] accel/tcg: Remove TranslatorOps.breakpoint_check

[Peter Maydell]

On Tue, 20 Jul 2021 at 20:55, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> The hook is now unused, with breakpoints checked outside translation.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  include/exec/translator.h     | 11 -----------
>  target/arm/helper.h           |  2 --
>  target/alpha/translate.c      | 16 ----------------
>  target/arm/debug_helper.c     |  7 -------
>  target/arm/translate-a64.c    | 25 -------------------------
>  target/arm/translate.c        | 29 -----------------------------
>  target/avr/translate.c        | 18 ------------------
>  target/cris/translate.c       | 20 --------------------
>  target/hexagon/translate.c    | 17 -----------------
>  target/hppa/translate.c       | 11 -----------
>  target/i386/tcg/translate.c   | 28 ----------------------------
>  target/m68k/translate.c       | 18 ------------------
>  target/microblaze/translate.c | 18 ------------------
>  target/mips/tcg/translate.c   | 19 -------------------
>  target/nios2/translate.c      | 27 ---------------------------
>  target/openrisc/translate.c   | 17 -----------------
>  target/ppc/translate.c        | 18 ------------------
>  target/riscv/translate.c      | 17 -----------------
>  target/rx/translate.c         | 14 --------------
>  target/s390x/tcg/translate.c  | 24 ------------------------
>  target/sh4/translate.c        | 18 ------------------
>  target/sparc/translate.c      | 17 -----------------
>  target/tricore/translate.c    | 16 ----------------
>  target/xtensa/translate.c     | 17 -----------------
>  24 files changed, 424 deletions(-)

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

thanks
-- PMM

Re: [PATCH for-6.1 v6 15/17] accel/tcg: Remove TranslatorOps.breakpoint_check

[Philippe Mathieu-Daudé]

On 7/20/21 9:54 PM, Richard Henderson wrote:
> The hook is now unused, with breakpoints checked outside translation.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  include/exec/translator.h     | 11 -----------
>  target/arm/helper.h           |  2 --
>  target/alpha/translate.c      | 16 ----------------
>  target/arm/debug_helper.c     |  7 -------
>  target/arm/translate-a64.c    | 25 -------------------------
>  target/arm/translate.c        | 29 -----------------------------
>  target/avr/translate.c        | 18 ------------------
>  target/cris/translate.c       | 20 --------------------
>  target/hexagon/translate.c    | 17 -----------------
>  target/hppa/translate.c       | 11 -----------
>  target/i386/tcg/translate.c   | 28 ----------------------------
>  target/m68k/translate.c       | 18 ------------------
>  target/microblaze/translate.c | 18 ------------------
>  target/mips/tcg/translate.c   | 19 -------------------
>  target/nios2/translate.c      | 27 ---------------------------
>  target/openrisc/translate.c   | 17 -----------------
>  target/ppc/translate.c        | 18 ------------------
>  target/riscv/translate.c      | 17 -----------------
>  target/rx/translate.c         | 14 --------------
>  target/s390x/tcg/translate.c  | 24 ------------------------
>  target/sh4/translate.c        | 18 ------------------
>  target/sparc/translate.c      | 17 -----------------
>  target/tricore/translate.c    | 16 ----------------
>  target/xtensa/translate.c     | 17 -----------------
>  24 files changed, 424 deletions(-)

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

[PATCH for-6.1 v6 16/17] accel/tcg: Hoist tb_cflags to a local in translator_loop

[Richard Henderson]

The access internal to tb_cflags() is atomic.
Avoid re-reading it as such for the multiple uses.

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 accel/tcg/translator.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c
index 4f3728c278..b45337f3ba 100644
--- a/accel/tcg/translator.c
+++ b/accel/tcg/translator.c
@@ -50,6 +50,7 @@ bool translator_use_goto_tb(DisasContextBase *db, target_ulong dest)
 void translator_loop(const TranslatorOps *ops, DisasContextBase *db,
                      CPUState *cpu, TranslationBlock *tb, int max_insns)
 {
+    uint32_t cflags = tb_cflags(tb);
     bool plugin_enabled;
 
     /* Initialize DisasContext */
@@ -72,8 +73,7 @@ void translator_loop(const TranslatorOps *ops, DisasContextBase *db,
     ops->tb_start(db, cpu);
     tcg_debug_assert(db->is_jmp == DISAS_NEXT);  /* no early exit */
 
-    plugin_enabled = plugin_gen_tb_start(cpu, tb,
-                                         tb_cflags(db->tb) & CF_MEMI_ONLY);
+    plugin_enabled = plugin_gen_tb_start(cpu, tb, cflags & CF_MEMI_ONLY);
 
     while (true) {
         db->num_insns++;
@@ -88,14 +88,13 @@ void translator_loop(const TranslatorOps *ops, DisasContextBase *db,
            update db->pc_next and db->is_jmp to indicate what should be
            done next -- either exiting this loop or locate the start of
            the next instruction.  */
-        if (db->num_insns == db->max_insns
-            && (tb_cflags(db->tb) & CF_LAST_IO)) {
+        if (db->num_insns == db->max_insns && (cflags & CF_LAST_IO)) {
             /* Accept I/O on the last instruction.  */
             gen_io_start();
             ops->translate_insn(db, cpu);
         } else {
             /* we should only see CF_MEMI_ONLY for io_recompile */
-            tcg_debug_assert(!(tb_cflags(db->tb) & CF_MEMI_ONLY));
+            tcg_debug_assert(!(cflags & CF_MEMI_ONLY));
             ops->translate_insn(db, cpu);
         }
 
-- 
2.25.1

[PATCH for-6.1 v6 17/17] accel/tcg: Record singlestep_enabled in tb->cflags

[Richard Henderson]

Set CF_SINGLE_STEP when single-stepping is enabled.
This avoids the need to flush all tb's when turning
single-stepping on or off.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/exec/exec-all.h   | 1 +
 accel/tcg/cpu-exec.c      | 7 ++++++-
 accel/tcg/translate-all.c | 4 ----
 accel/tcg/translator.c    | 7 +------
 cpu.c                     | 4 ----
 5 files changed, 8 insertions(+), 15 deletions(-)

diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index 6873cce8df..5d1b6d80fb 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -497,6 +497,7 @@ struct TranslationBlock {
 #define CF_COUNT_MASK    0x000001ff
 #define CF_NO_GOTO_TB    0x00000200 /* Do not chain with goto_tb */
 #define CF_NO_GOTO_PTR   0x00000400 /* Do not chain with goto_ptr */
+#define CF_SINGLE_STEP   0x00000800 /* gdbstub single-step in effect */
 #define CF_LAST_IO       0x00008000 /* Last insn may be an IO access.  */
 #define CF_MEMI_ONLY     0x00010000 /* Only instrument memory ops */
 #define CF_USE_ICOUNT    0x00020000
diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index 5cc6363f4c..fc895cf51e 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -150,10 +150,15 @@ uint32_t curr_cflags(CPUState *cpu)
     uint32_t cflags = cpu->tcg_cflags;
 
     /*
+     * Record gdb single-step.  We should be exiting the TB by raising
+     * EXCP_DEBUG, but to simplify other tests, disable chaining too.
+     *
      * For singlestep and -d nochain, suppress goto_tb so that
      * we can log -d cpu,exec after every TB.
      */
-    if (singlestep) {
+    if (unlikely(cpu->singlestep_enabled)) {
+        cflags |= CF_NO_GOTO_TB | CF_NO_GOTO_PTR | CF_SINGLE_STEP | 1;
+    } else if (singlestep) {
         cflags |= CF_NO_GOTO_TB | 1;
     } else if (qemu_loglevel_mask(CPU_LOG_TB_NOCHAIN)) {
         cflags |= CF_NO_GOTO_TB;
diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index bf82c15aab..bbfcfb698c 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -1432,10 +1432,6 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
     }
     QEMU_BUILD_BUG_ON(CF_COUNT_MASK + 1 != TCG_MAX_INSNS);
 
-    if (cpu->singlestep_enabled) {
-        max_insns = 1;
-    }
-
  buffer_overflow:
     tb = tcg_tb_alloc(tcg_ctx);
     if (unlikely(!tb)) {
diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c
index b45337f3ba..c53a7f8e44 100644
--- a/accel/tcg/translator.c
+++ b/accel/tcg/translator.c
@@ -38,11 +38,6 @@ bool translator_use_goto_tb(DisasContextBase *db, target_ulong dest)
         return false;
     }
 
-    /* Suppress goto_tb in the case of single-steping.  */
-    if (db->singlestep_enabled) {
-        return false;
-    }
-
     /* Check for the dest on the same page as the start of the TB.  */
     return ((db->pc_first ^ dest) & TARGET_PAGE_MASK) == 0;
 }
@@ -60,7 +55,7 @@ void translator_loop(const TranslatorOps *ops, DisasContextBase *db,
     db->is_jmp = DISAS_NEXT;
     db->num_insns = 0;
     db->max_insns = max_insns;
-    db->singlestep_enabled = cpu->singlestep_enabled;
+    db->singlestep_enabled = cflags & CF_SINGLE_STEP;
 
     ops->init_disas_context(db, cpu);
     tcg_debug_assert(db->is_jmp == DISAS_NEXT);  /* no early exit */
diff --git a/cpu.c b/cpu.c
index d6ae5ae581..e1799a15bc 100644
--- a/cpu.c
+++ b/cpu.c
@@ -326,10 +326,6 @@ void cpu_single_step(CPUState *cpu, int enabled)
         cpu->singlestep_enabled = enabled;
         if (kvm_enabled()) {
             kvm_update_guest_debug(cpu, 0);
-        } else {
-            /* must flush all the translated code to avoid inconsistencies */
-            /* XXX: only flush what is necessary */
-            tb_flush(cpu);
         }
         trace_breakpoint_singlestep(cpu->cpu_index, enabled);
     }
-- 
2.25.1

Re: [PATCH for-6.1 v6 17/17] accel/tcg: Record singlestep_enabled in tb->cflags

[Peter Maydell]

On Tue, 20 Jul 2021 at 20:55, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Set CF_SINGLE_STEP when single-stepping is enabled.
> This avoids the need to flush all tb's when turning
> single-stepping on or off.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

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

thanks
-- PMM

Re: [PATCH for-6.1 v6 17/17] accel/tcg: Record singlestep_enabled in tb->cflags

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> Set CF_SINGLE_STEP when single-stepping is enabled.
> This avoids the need to flush all tb's when turning
> single-stepping on or off.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  include/exec/exec-all.h   | 1 +
>  accel/tcg/cpu-exec.c      | 7 ++++++-
>  accel/tcg/translate-all.c | 4 ----
>  accel/tcg/translator.c    | 7 +------
>  cpu.c                     | 4 ----
>  5 files changed, 8 insertions(+), 15 deletions(-)
>
> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
> index 6873cce8df..5d1b6d80fb 100644
> --- a/include/exec/exec-all.h
> +++ b/include/exec/exec-all.h
> @@ -497,6 +497,7 @@ struct TranslationBlock {
>  #define CF_COUNT_MASK    0x000001ff
>  #define CF_NO_GOTO_TB    0x00000200 /* Do not chain with goto_tb */
>  #define CF_NO_GOTO_PTR   0x00000400 /* Do not chain with goto_ptr */
> +#define CF_SINGLE_STEP   0x00000800 /* gdbstub single-step in effect */
>  #define CF_LAST_IO       0x00008000 /* Last insn may be an IO access.  */
>  #define CF_MEMI_ONLY     0x00010000 /* Only instrument memory ops */
>  #define CF_USE_ICOUNT    0x00020000
> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> index 5cc6363f4c..fc895cf51e 100644
> --- a/accel/tcg/cpu-exec.c
> +++ b/accel/tcg/cpu-exec.c
> @@ -150,10 +150,15 @@ uint32_t curr_cflags(CPUState *cpu)
>      uint32_t cflags = cpu->tcg_cflags;
>  
>      /*
> +     * Record gdb single-step.  We should be exiting the TB by raising
> +     * EXCP_DEBUG, but to simplify other tests, disable chaining too.
> +     *
>       * For singlestep and -d nochain, suppress goto_tb so that
>       * we can log -d cpu,exec after every TB.
>       */
> -    if (singlestep) {
> +    if (unlikely(cpu->singlestep_enabled)) {
> +        cflags |= CF_NO_GOTO_TB | CF_NO_GOTO_PTR | CF_SINGLE_STEP |
>      1;

What does CF_SINGLE_STEP achieve that isn't already handled by having:

  cflags |= CF_NO_GOTO_TB | CF_NO_GOTO_PTR | 1;

(btw did we mask CF_COUNT_MASK somewhere else?). Because surely the
CF_COUNT is part of cflags so limits the TB's that could be returned
anyway?


> +    } else if (singlestep) {
>          cflags |= CF_NO_GOTO_TB | 1;
>      } else if (qemu_loglevel_mask(CPU_LOG_TB_NOCHAIN)) {
>          cflags |= CF_NO_GOTO_TB;
> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> index bf82c15aab..bbfcfb698c 100644
> --- a/accel/tcg/translate-all.c
> +++ b/accel/tcg/translate-all.c
> @@ -1432,10 +1432,6 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
>      }
>      QEMU_BUILD_BUG_ON(CF_COUNT_MASK + 1 != TCG_MAX_INSNS);
>  
> -    if (cpu->singlestep_enabled) {
> -        max_insns = 1;
> -    }
> -
>   buffer_overflow:
>      tb = tcg_tb_alloc(tcg_ctx);
>      if (unlikely(!tb)) {
> diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c
> index b45337f3ba..c53a7f8e44 100644
> --- a/accel/tcg/translator.c
> +++ b/accel/tcg/translator.c
> @@ -38,11 +38,6 @@ bool translator_use_goto_tb(DisasContextBase *db, target_ulong dest)
>          return false;
>      }
>  
> -    /* Suppress goto_tb in the case of single-steping.  */
> -    if (db->singlestep_enabled) {
> -        return false;
> -    }
> -
>      /* Check for the dest on the same page as the start of the TB.  */
>      return ((db->pc_first ^ dest) & TARGET_PAGE_MASK) == 0;
>  }
> @@ -60,7 +55,7 @@ void translator_loop(const TranslatorOps *ops, DisasContextBase *db,
>      db->is_jmp = DISAS_NEXT;
>      db->num_insns = 0;
>      db->max_insns = max_insns;
> -    db->singlestep_enabled = cpu->singlestep_enabled;
> +    db->singlestep_enabled = cflags & CF_SINGLE_STEP;
>  
>      ops->init_disas_context(db, cpu);
>      tcg_debug_assert(db->is_jmp == DISAS_NEXT);  /* no early exit */
> diff --git a/cpu.c b/cpu.c
> index d6ae5ae581..e1799a15bc 100644
> --- a/cpu.c
> +++ b/cpu.c
> @@ -326,10 +326,6 @@ void cpu_single_step(CPUState *cpu, int enabled)
>          cpu->singlestep_enabled = enabled;
>          if (kvm_enabled()) {
>              kvm_update_guest_debug(cpu, 0);
> -        } else {
> -            /* must flush all the translated code to avoid inconsistencies */
> -            /* XXX: only flush what is necessary */
> -            tb_flush(cpu);
>          }
>          trace_breakpoint_singlestep(cpu->cpu_index, enabled);
>      }


-- 
Alex Bennée

Re: [PATCH for-6.1 v6 17/17] accel/tcg: Record singlestep_enabled in tb->cflags

[Richard Henderson]

On 7/21/21 12:38 AM, Alex Bennée wrote:
> 
> Richard Henderson <richard.henderson@linaro.org> writes:
> 
>> Set CF_SINGLE_STEP when single-stepping is enabled.
>> This avoids the need to flush all tb's when turning
>> single-stepping on or off.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>   include/exec/exec-all.h   | 1 +
>>   accel/tcg/cpu-exec.c      | 7 ++++++-
>>   accel/tcg/translate-all.c | 4 ----
>>   accel/tcg/translator.c    | 7 +------
>>   cpu.c                     | 4 ----
>>   5 files changed, 8 insertions(+), 15 deletions(-)
>>
>> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
>> index 6873cce8df..5d1b6d80fb 100644
>> --- a/include/exec/exec-all.h
>> +++ b/include/exec/exec-all.h
>> @@ -497,6 +497,7 @@ struct TranslationBlock {
>>   #define CF_COUNT_MASK    0x000001ff
>>   #define CF_NO_GOTO_TB    0x00000200 /* Do not chain with goto_tb */
>>   #define CF_NO_GOTO_PTR   0x00000400 /* Do not chain with goto_ptr */
>> +#define CF_SINGLE_STEP   0x00000800 /* gdbstub single-step in effect */
>>   #define CF_LAST_IO       0x00008000 /* Last insn may be an IO access.  */
>>   #define CF_MEMI_ONLY     0x00010000 /* Only instrument memory ops */
>>   #define CF_USE_ICOUNT    0x00020000
>> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
>> index 5cc6363f4c..fc895cf51e 100644
>> --- a/accel/tcg/cpu-exec.c
>> +++ b/accel/tcg/cpu-exec.c
>> @@ -150,10 +150,15 @@ uint32_t curr_cflags(CPUState *cpu)
>>       uint32_t cflags = cpu->tcg_cflags;
>>   
>>       /*
>> +     * Record gdb single-step.  We should be exiting the TB by raising
>> +     * EXCP_DEBUG, but to simplify other tests, disable chaining too.
>> +     *
>>        * For singlestep and -d nochain, suppress goto_tb so that
>>        * we can log -d cpu,exec after every TB.
>>        */
>> -    if (singlestep) {
>> +    if (unlikely(cpu->singlestep_enabled)) {
>> +        cflags |= CF_NO_GOTO_TB | CF_NO_GOTO_PTR | CF_SINGLE_STEP |
>>       1;
> 
> What does CF_SINGLE_STEP achieve that isn't already handled by having:
> 
>    cflags |= CF_NO_GOTO_TB | CF_NO_GOTO_PTR | 1;

It sets DisasContextBase.singlestep_enabled.

With only this patch set, we still check that and emit EXCP_DEBUG at the end of every TB. 
  After the 6.2 singlestep cleanup, we still have one reference to 
DisasContextBase.singlestep_enabled in target/mips for the branch delay slot thing that we 
discussed on IRC yesterday.

> 
> (btw did we mask CF_COUNT_MASK somewhere else?). Because surely the
> CF_COUNT is part of cflags so limits the TB's that could be returned
> anyway?

Here in curr_cflags(), CF_COUNT_MASK begins at zero.


r~

Re: [PATCH for-6.1 v6 17/17] accel/tcg: Record singlestep_enabled in tb->cflags

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> On 7/21/21 12:38 AM, Alex Bennée wrote:
>> Richard Henderson <richard.henderson@linaro.org> writes:
>> 
>>> Set CF_SINGLE_STEP when single-stepping is enabled.
>>> This avoids the need to flush all tb's when turning
>>> single-stepping on or off.
>>>
>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>>> ---
>>>   include/exec/exec-all.h   | 1 +
>>>   accel/tcg/cpu-exec.c      | 7 ++++++-
>>>   accel/tcg/translate-all.c | 4 ----
>>>   accel/tcg/translator.c    | 7 +------
>>>   cpu.c                     | 4 ----
>>>   5 files changed, 8 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
>>> index 6873cce8df..5d1b6d80fb 100644
>>> --- a/include/exec/exec-all.h
>>> +++ b/include/exec/exec-all.h
>>> @@ -497,6 +497,7 @@ struct TranslationBlock {
>>>   #define CF_COUNT_MASK    0x000001ff
>>>   #define CF_NO_GOTO_TB    0x00000200 /* Do not chain with goto_tb */
>>>   #define CF_NO_GOTO_PTR   0x00000400 /* Do not chain with goto_ptr */
>>> +#define CF_SINGLE_STEP   0x00000800 /* gdbstub single-step in effect */
>>>   #define CF_LAST_IO       0x00008000 /* Last insn may be an IO access.  */
>>>   #define CF_MEMI_ONLY     0x00010000 /* Only instrument memory ops */
>>>   #define CF_USE_ICOUNT    0x00020000
>>> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
>>> index 5cc6363f4c..fc895cf51e 100644
>>> --- a/accel/tcg/cpu-exec.c
>>> +++ b/accel/tcg/cpu-exec.c
>>> @@ -150,10 +150,15 @@ uint32_t curr_cflags(CPUState *cpu)
>>>       uint32_t cflags = cpu->tcg_cflags;
>>>         /*
>>> +     * Record gdb single-step.  We should be exiting the TB by raising
>>> +     * EXCP_DEBUG, but to simplify other tests, disable chaining too.
>>> +     *
>>>        * For singlestep and -d nochain, suppress goto_tb so that
>>>        * we can log -d cpu,exec after every TB.
>>>        */
>>> -    if (singlestep) {
>>> +    if (unlikely(cpu->singlestep_enabled)) {
>>> +        cflags |= CF_NO_GOTO_TB | CF_NO_GOTO_PTR | CF_SINGLE_STEP |
>>>       1;
>> What does CF_SINGLE_STEP achieve that isn't already handled by
>> having:
>>    cflags |= CF_NO_GOTO_TB | CF_NO_GOTO_PTR | 1;
>
> It sets DisasContextBase.singlestep_enabled.

Ahh fair enough... I was only thinking of the effect on stored and
looked up translations. I guess we still have bits we can rob if we need
to until the day we expand cflags and flags to full 64 bit values.

> With only this patch set, we still check that and emit EXCP_DEBUG at
> the end of every TB.   After the 6.2 singlestep cleanup, we still have
> one reference to DisasContextBase.singlestep_enabled in target/mips
> for the branch delay slot thing that we discussed on IRC yesterday.
>
>> (btw did we mask CF_COUNT_MASK somewhere else?). Because surely the
>> CF_COUNT is part of cflags so limits the TB's that could be returned
>> anyway?
>
> Here in curr_cflags(), CF_COUNT_MASK begins at zero.

OK:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée

Re: [PATCH for-6.1 v6 00/17] tcg: breakpoint reorg

[Mark Cave-Ayland]

On 20/07/2021 20:54, Richard Henderson wrote:

> This is fixing #404 ("windows xp boot takes much longer...")
> and several other similar reports.
> 
> Changes for v6:
>    * Reinstate accidental loss of singlestep overriding breakpoint check.
>      Shows up in the record-replay avocado tests failing to make progress.
>    * Add CPUState.gdb_adjust_breakpoint hook for AVR weirdness.
> 
> Changes for v5:
>    * Include missing hunk in tb_gen_code, as noted in reply to v4.
>    * Remove helper_check_breakpoints from target/arm/.
>    * Reorg cflags_for_breakpoints into check_for_breakpoints;
>      reorg cpu_exec to use a break instead of a longjmp.
>    * Move singlestep_enabled check from cflags_for_breakpoints
>      to curr_cflags, which makes cpu_exec_step_atomic cleaner.
> 
> Changes for v4:
>    * Issue breakpoints directly from cflags_for_breakpoints.
>      Do not generate code for a TB beginning with a BP at all.
>    * Drop the problematic TranslatorOps.breakpoint_check hook entirely.
> 
> Changes for v3:
>    * Map CF_COUNT_MASK == 0 -> TCG_MAX_INSNS.
>    * Split out *_breakpoint_check fixes for avr, mips, riscv.
> 
> Changes for v2:
>    * All prerequisites and 7 of the patches from v1 with are merged.
> 
> Patches lacking review:
>    11-hw-core-Introduce-CPUClass.gdb_adjust_breakpoint.patch
>    12-target-avr-Implement-gdb_adjust_breakpoint.patch
>    15-accel-tcg-Remove-TranslatorOps.breakpoint_check.patch
>    17-accel-tcg-Record-singlestep_enabled-in-tb-cflags.patch
> 
> 
> r~
> 
> 
> Richard Henderson (17):
>    accel/tcg: Reduce CF_COUNT_MASK to match TCG_MAX_INSNS
>    accel/tcg: Move curr_cflags into cpu-exec.c
>    target/alpha: Drop goto_tb path in gen_call_pal
>    accel/tcg: Add CF_NO_GOTO_TB and CF_NO_GOTO_PTR
>    accel/tcg: Drop CF_NO_GOTO_PTR from -d nochain
>    accel/tcg: Handle -singlestep in curr_cflags
>    accel/tcg: Use CF_NO_GOTO_{TB, PTR} in cpu_exec_step_atomic
>    hw/core: Introduce TCGCPUOps.debug_check_breakpoint
>    target/arm: Implement debug_check_breakpoint
>    target/i386: Implement debug_check_breakpoint
>    hw/core: Introduce CPUClass.gdb_adjust_breakpoint
>    target/avr: Implement gdb_adjust_breakpoint
>    accel/tcg: Merge tb_find into its only caller
>    accel/tcg: Move breakpoint recognition outside translation
>    accel/tcg: Remove TranslatorOps.breakpoint_check
>    accel/tcg: Hoist tb_cflags to a local in translator_loop
>    accel/tcg: Record singlestep_enabled in tb->cflags
> 
>   include/exec/exec-all.h       |  24 ++--
>   include/exec/translator.h     |  11 --
>   include/hw/core/cpu.h         |   4 +
>   include/hw/core/tcg-cpu-ops.h |   6 +
>   target/arm/helper.h           |   2 -
>   target/arm/internals.h        |   3 +
>   target/avr/cpu.h              |   1 +
>   accel/tcg/cpu-exec.c          | 205 ++++++++++++++++++++++++++--------
>   accel/tcg/translate-all.c     |   7 +-
>   accel/tcg/translator.c        |  39 ++-----
>   cpu.c                         |  34 ++----
>   target/alpha/translate.c      |  31 +----
>   target/arm/cpu.c              |   1 +
>   target/arm/cpu_tcg.c          |   1 +
>   target/arm/debug_helper.c     |  12 +-
>   target/arm/translate-a64.c    |  25 -----
>   target/arm/translate.c        |  29 -----
>   target/avr/cpu.c              |   1 +
>   target/avr/gdbstub.c          |  13 +++
>   target/avr/translate.c        |  32 ------
>   target/cris/translate.c       |  20 ----
>   target/hexagon/translate.c    |  17 ---
>   target/hppa/translate.c       |  11 --
>   target/i386/tcg/tcg-cpu.c     |  12 ++
>   target/i386/tcg/translate.c   |  28 -----
>   target/m68k/translate.c       |  18 ---
>   target/microblaze/translate.c |  18 ---
>   target/mips/tcg/translate.c   |  19 ----
>   target/nios2/translate.c      |  27 -----
>   target/openrisc/translate.c   |  17 ---
>   target/ppc/translate.c        |  18 ---
>   target/riscv/translate.c      |  17 ---
>   target/rx/translate.c         |  14 ---
>   target/s390x/tcg/translate.c  |  24 ----
>   target/sh4/translate.c        |  18 ---
>   target/sparc/translate.c      |  17 ---
>   target/tricore/translate.c    |  16 ---
>   target/xtensa/translate.c     |  17 ---
>   tcg/tcg-op.c                  |  28 ++---
>   39 files changed, 248 insertions(+), 589 deletions(-)

I spent a bit of time this evening testing this patchset with some typical OpenBIOS 
debugging patterns across SPARC32, SPARC64 and PPC and didn't spot any regressions, 
and the WinXP image still boots in 25s so:

Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

I did find the slow-down noticeable when testing a few OpenBIOS breakpoints e.g. a 
breakpoint on OpenBIOS SPARC64's init_memory() upped the boot time to the Forth 
prompt from 9s to 30s even though it hits only once early in boot (presumably due to 
its proximity to a hot routine). Having said that, the changes look like a good 
improvement and patch 14 suggests that there could be further optimisations to be 
made in future, so in general this feels like a net win.


ATB,

Mark.