For v2, all prerequisites and 7 of the patches from v1 with

reviews are now upstream.

Mark Cave-Ayland reported success with WinXP with v1, with

this patch set being even faster than b55f54bc~1. Which was

a bit of a surprise, but I'll take it. It means that it's

probably not worth making the breakpoint detection scheme

any more complicated.

I'd still like some more feedback. Given this is fixing a

regression from qemu 5.2 I feel comfortable delaying this

past soft freeze, but not past hard freeze on the 20th.

Richard Henderson (10):

accel/tcg: Use CF_NO_GOTO_{TB,PTR} in cpu_exec_step_atomic

accel/tcg: Move cflags lookup into tb_find

accel/tcg: Adjust interface of TranslatorOps.breakpoint_check

accel/tcg: Encode breakpoint info into tb->cflags

include/exec/exec-all.h | 30 +++++---

include/exec/translator.h | 17 +++--

accel/tcg/cpu-exec.c | 130 ++++++++++++++++++++++++++++------

accel/tcg/translator.c | 79 ++++++++++++++-------

cpu.c | 24 -------

target/alpha/translate.c | 12 +---

target/arm/translate-a64.c | 14 ++--

target/arm/translate.c | 20 +++---

target/avr/translate.c | 6 +-

target/cris/translate.c | 14 ++--

target/hexagon/translate.c | 13 +---

target/hppa/translate.c | 7 +-

target/i386/tcg/translate.c | 15 ++--

target/m68k/translate.c | 14 +---

target/microblaze/translate.c | 14 +---

target/mips/tcg/translate.c | 14 ++--

target/nios2/translate.c | 13 +---

target/openrisc/translate.c | 11 +--

target/ppc/translate.c | 13 +---

target/riscv/translate.c | 11 +--

target/rx/translate.c | 8 +--

target/s390x/translate.c | 12 ++--

target/sh4/translate.c | 12 ++--

target/sparc/translate.c | 9 ++-

target/tricore/translate.c | 13 +---

target/xtensa/translate.c | 12 ++--

tcg/tcg-op.c | 28 ++++----

28 files changed, 280 insertions(+), 292 deletions(-)

max_insns = CF_COUNT_MASK;

We will shortly require the guest pc for computing cflags,

so move the choice just after cpu_get_tb_cpu_state.

accel/tcg/cpu-exec.c | 34 +++++++++++++++++-----------------

1 file changed, 17 insertions(+), 17 deletions(-)

static inline TranslationBlock *tb_find(CPUState *cpu,

TranslationBlock *last_tb,

+ int tb_exit)

CPUArchState *env = (CPUArchState *)cpu->env_ptr;

TranslationBlock *tb;

target_ulong cs_base, pc;

- uint32_t flags;

+ uint32_t flags, cflags;

cpu_get_tb_cpu_state(env, &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-

+ * 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_lookup(cpu, pc, cs_base, flags, cflags);

if (tb == NULL) {

mmap_lock();

- TranslationBlock *tb;

-

- if (cflags == -1) {

- cflags = curr_cflags(cpu);

- } else {

- cpu->cflags_next_tb = -1;

- }

-

+ TranslationBlock *tb = tb_find(cpu, last_tb, tb_exit);

Having this data in cflags means that hashing takes care

of selecting a TB with or without exceptions built in.

Which means that we no longer need to flush all TBs.

This does require that we single-step while we're within a page

that contains a breakpoint, so it's not yet ideal, but should be

an improvement over some corner-case slowdowns.

include/exec/exec-all.h | 7 ++++

accel/tcg/cpu-exec.c | 68 ++++++++++++++++++++++++++++++-

accel/tcg/translate-all.c | 4 --

accel/tcg/translator.c | 85 +++++++++++++++++++++------------------

cpu.c | 24 -----------

5 files changed, 119 insertions(+), 69 deletions(-)

diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h

index XXXXXXX..XXXXXXX 100644

--- a/include/exec/exec-all.h

+++ b/include/exec/exec-all.h

@@ -XXX,XX +XXX,XX @@ struct TranslationBlock {

#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_BP_MASK 0x00300000 /* See below */

+#define CF_BP_SHIFT 20

#define CF_CLUSTER_MASK 0xff000000 /* Top 8 bits are cluster ID */

#define CF_CLUSTER_SHIFT 24

+#define CF_BP_NONE (0 << CF_BP_SHIFT) /* TB does not interact with BPs */

+#define CF_BP_SSTEP (1 << CF_BP_SHIFT) /* gdbstub single-step in effect */

+#define CF_BP_GDB (2 << CF_BP_SHIFT) /* gdbstub breakpoint at tb->pc */

+#define CF_BP_CPU (3 << CF_BP_SHIFT) /* arch breakpoint at tb->pc */

+

/* Per-vCPU dynamic tracing state used to generate this TB */

uint32_t trace_vcpu_dstate;

+static uint32_t cflags_for_breakpoints(CPUState *cpu, target_ulong pc,

+ uint32_t cflags)

+ uint32_t bflags = 0;

+

+ if (unlikely(cpu->singlestep_enabled)) {

+ bflags = CF_BP_SSTEP;

+ } else {

+ bool match_page = false;

+ CPUBreakpoint *bp;

+

+ QTAILQ_FOREACH(bp, &cpu->breakpoints, entry) {

+ /* Note exact pc matches */

+ if (pc == bp->pc) {

+ if (bp->flags & BP_GDB) {

+ bflags = CF_BP_GDB;

+ break;

+ }

+ if (bp->flags & BP_CPU) {

+ bflags = CF_BP_CPU;

+ break;

+ }

+ /* Note page matches */

+ if (((pc ^ bp->pc) & TARGET_PAGE_MASK) == 0) {

+ match_page = true;

+ * Reduce the TB to one insn.

+ * In the case of a BP hit, we will be raising an exception anyway.

+ * In the case of a page hit, return to helper_lookup_tb_ptr after

+ * every insn to look for the breakpoint.

+ return (cflags & ~CF_COUNT_MASK) | 1 | bflags;

+ cflags = cflags_for_breakpoints(cpu, pc, curr_cflags(cpu));

+ /* Raise any post-instruction debug exceptions. */

+ cflags = cflags_for_breakpoints(cpu, pc, cflags);

@@ -XXX,XX +XXX,XX @@ static inline TranslationBlock *tb_find(CPUState *cpu,

} else {

cpu->cflags_next_tb = -1;

}

+ cflags = cflags_for_breakpoints(cpu, pc, cflags);

tb = tb_lookup(cpu, pc, cs_base, flags, cflags);

if (tb == NULL) {

diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c

index XXXXXXX..XXXXXXX 100644

--- a/accel/tcg/translate-all.c

+++ b/accel/tcg/translate-all.c

@@ -XXX,XX +XXX,XX @@ 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)) {

@@ -XXX,XX +XXX,XX @@ 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) {

+ /* Suppress goto_tb if requested, or required by breakpoints. */

+ if (tb_cflags(db->tb) & (CF_NO_GOTO_TB | CF_BP_MASK)) {

return false;

}

@@ -XXX,XX +XXX,XX @@ void translator_loop(const TranslatorOps *ops, DisasContextBase *db,

uint32_t cflags = tb_cflags(tb);

+ int bp_flags = 0;

db->is_jmp = DISAS_NEXT;

db->num_insns = 0;

db->max_insns = max_insns;

- db->singlestep_enabled = cpu->singlestep_enabled;

+ db->singlestep_enabled = false;

+

+ switch (cflags & CF_BP_MASK) {

+ case CF_BP_NONE:

+ break;

+ case CF_BP_SSTEP:

+ db->singlestep_enabled = true;

+ break;

+ case CF_BP_GDB:

+ bp_flags = BP_GDB;

+ break;

+ case CF_BP_CPU:

+ bp_flags = BP_CPU;

+ break;

+ default:

+ g_assert_not_reached();

+ }

ops->init_disas_context(db, cpu);

tcg_debug_assert(db->is_jmp == DISAS_NEXT); /* no early exit */

@@ -XXX,XX +XXX,XX @@ void translator_loop(const TranslatorOps *ops, DisasContextBase *db,

/* Pass breakpoint hits to target for further processing */

- int len = ops->breakpoint_check(db, cpu, bp->flags);

+ if (unlikely(bp_flags)) {

+ int len = ops->breakpoint_check(db, cpu, bp_flags);

- /*

- * 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) {

- /*

- * 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.

- */

- tcg_debug_assert(len > 0);

- db->pc_next += len;

+ /*

+ * When there is a bp hit, we're going to execute a maximum

+ * of one instruction. The breakpoint_check hook may use

+ * DISAS_NEXT or DISAS_TOO_MANY to indicate that the current

+ * instruction should be translated. Anything else is taken

+ * to mean that an exception has been raised and that zero

+ * instructions will be executed.

+ */

+ if (db->is_jmp > DISAS_TOO_MANY) {

+ /*

+ * 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.

+ */

+ tcg_debug_assert(len > 0);

+ db->pc_next += len;

- /*

- * The breakpoint definitely hit, so decrement the

- * number of instructions completed for icount.

- */

- db->num_insns--;

- goto done;

+ /*

+ * The breakpoint definitely hit, so decrement the

+ * number of instructions completed for icount.

+ */

+ db->num_insns--;

+ goto done;

}

}

We don't need the whole CPUBreakpoint structure in the check,

only the flags. Return the instruction length to consolidate

the adjustment of db->pc_next.

include/exec/translator.h | 17 +++++++++------

accel/tcg/translator.c | 40 ++++++++++++++++++++++++-----------

target/alpha/translate.c | 12 +++--------

target/arm/translate-a64.c | 14 ++++--------

target/arm/translate.c | 20 +++++++-----------

target/avr/translate.c | 6 +++---

target/cris/translate.c | 14 ++++--------

target/hexagon/translate.c | 13 +++---------

target/hppa/translate.c | 7 +++---

target/i386/tcg/translate.c | 15 ++++---------

target/m68k/translate.c | 14 +++---------

target/microblaze/translate.c | 14 +++---------

target/mips/tcg/translate.c | 14 ++++--------

target/nios2/translate.c | 13 +++---------

target/openrisc/translate.c | 11 +++-------

target/ppc/translate.c | 13 +++---------

target/riscv/translate.c | 11 +++-------

target/rx/translate.c | 8 +++----

target/s390x/translate.c | 12 ++++-------

target/sh4/translate.c | 12 ++++-------

target/sparc/translate.c | 9 ++++----

target/tricore/translate.c | 13 +++---------

target/xtensa/translate.c | 12 ++++-------

23 files changed, 115 insertions(+), 199 deletions(-)

* @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), in which case

+ * db->is_jmp may be left as DISAS_NEXT or DISAS_TOO_MANY to indicate

+ * that the insn should be translated. Anything other than those two

+ * will be taken to indicate an exception has been raised, but in most

+ * cases db->is_jmp should be set to DISAS_NORETURN.

+ *

+ * Return the minimum instruction size that should be applied to the TB.

+ * The size of any TB cannot be zero, as that breaks the math used to

+ * invalidate TBs.

*

diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c

index XXXXXXX..XXXXXXX 100644

--- a/accel/tcg/translator.c

+++ b/accel/tcg/translator.c

@@ -XXX,XX +XXX,XX @@ 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 */

@@ -XXX,XX +XXX,XX @@ void translator_loop(const TranslatorOps *ops, DisasContextBase *db,

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;

+ int len = ops->breakpoint_check(db, cpu, bp->flags);

+

+ /*

+ * 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) {

+ /*

+ * 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.

+ */

+ tcg_debug_assert(len > 0);

+ db->pc_next += len;

+

+ /*

+ * The breakpoint definitely hit, so decrement the

+ * number of instructions completed for icount.

+ */

+ db->num_insns--;

+ goto done;

}

}

}

- /* 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

@@ -XXX,XX +XXX,XX @@ void translator_loop(const TranslatorOps *ops, DisasContextBase *db,

}

}

+ done:

/* 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);

+static int alpha_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cpu,

+ int bp_flags)

{

DisasContext *ctx = container_of(dcbase, DisasContext, base);

ctx->base.is_jmp = gen_excp(ctx, EXCP_DEBUG, 0);

+ return 4; /* minimum instruction length */

}

+static int aarch64_tr_breakpoint_check(DisasContextBase *dcbase,

+ CPUState *cpu, int bp_flags)

{

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);

dc->base.is_jmp = DISAS_NORETURN;

}

- return true;

+ return 4; /* minimum instruction length */

}

+static int arm_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cpu,

+ int bp_flags)

{

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);

@@ -XXX,XX +XXX,XX @@ static bool arm_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cpu,

dc->base.is_jmp = DISAS_TOO_MANY;

} else {

gen_exception_internal_insn(dc, dc->base.pc_next, EXCP_DEBUG);

dc->base.is_jmp = DISAS_NORETURN;

}

- return true;

+ /*

+ * TODO: Advance PC by correct instruction length to avoid disassembler

+ * error messages. In the meantime, minimum instruction length.

+ */

+ return 2;

}

+static int avr_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cs,

+ int bp_flags)