On Thu, Sep 17, 2026 at 5:33 AM Richard Henderson <[email protected]> wrote: > > From: Matt Turner <[email protected]> > > The next patch dispatches a goto_jc by probing the TB jump cache from > generated code. That probe cannot check everything helper_lookup_tb_ptr() > checks, and the one that matters is breakpoints: check_for_breakpoints() > raises EXCP_DEBUG on an exact pc match and selects CF_BP_PAGE cflags for the > rest of the page, and inserting a breakpoint deliberately invalidates no TB. > > The probe compares the destination's cflags against the cflags of the block > doing the dispatching, and only takes the destination when they are equal. > So a cflag is all that is needed. Add CF_NO_GOTO_JC, set it in > CPUState::tcg_cflags while cpu->breakpoints is non-empty, and blocks > translated from then on both decline to dispatch inline themselves -- the > next patch makes them emit the plain helper call -- and are unreachable from > blocks that do, because their cflags no longer match. > > The two ends of the flag are cpu_breakpoint_insert() and > cpu_breakpoint_remove_by_ref(), which are the only places the list changes. > Both already run either on the CPU's own thread or with it stopped, or reach > another CPU exactly as cpu_single_step() does, which is where the previous > patch put the same kind of update. > > That leaves blocks translated before the breakpoint was inserted, which are > still live and still chain to each other. They do so on the old cflags, so > inline dispatch among them keeps working until the vCPU reaches its main > loop, which then looks up with the new cflags and translates afresh. In > system mode gdb inserts breakpoints with the vCPUs stopped, so there is no > window at all. In user mode the window is the one goto_tb chaining already > has: a chained direct jump consults nothing either, and is not broken by > inserting a breakpoint. > > Nothing reads CF_NO_GOTO_JC yet; the next patch does. > > Signed-off-by: Matt Turner <[email protected]> > Signed-off-by: Richard Henderson <[email protected]> > Message-ID: <[email protected]> > --- > include/exec/translation-block.h | 5 +++-- > accel/tcg/cpu-exec-common.c | 16 ++++++++++++++-- > cpu-common.c | 25 ++++++++++++++++++++++++- > 3 files changed, 41 insertions(+), 5 deletions(-) > > diff --git a/include/exec/translation-block.h > b/include/exec/translation-block.h > index 40cc6990318..f3410dbcb93 100644 > --- a/include/exec/translation-block.h > +++ b/include/exec/translation-block.h > @@ -76,14 +76,15 @@ 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_MEMI_ONLY 0x00001000 /* Only instrument memory ops */ > +#define CF_NO_GOTO_JC 0x00000800 /* Do not dispatch via the inline probe > */ > +#define CF_SINGLE_STEP 0x00001000 /* gdbstub single-step in effect */ > #define CF_USE_ICOUNT 0x00002000 > #define CF_INVALID 0x00004000 /* TB is stale. Set with @jmp_lock held > */ > #define CF_PARALLEL 0x00008000 /* Generate code for a parallel context > */ > #define CF_NOIRQ 0x00010000 /* Generate an uninterruptible TB */ > #define CF_PCREL 0x00020000 /* Opcodes in TB are PC-relative */ > #define CF_BP_PAGE 0x00040000 /* Breakpoint present in code page */ > +#define CF_MEMI_ONLY 0x00080000 /* Only instrument memory ops */ > #define CF_CLUSTER_MASK 0xff000000 /* Top 8 bits are cluster ID */ > #define CF_CLUSTER_SHIFT 24 > > diff --git a/accel/tcg/cpu-exec-common.c b/accel/tcg/cpu-exec-common.c > index 9f3517f36b6..9f9555e34f6 100644 > --- a/accel/tcg/cpu-exec-common.c > +++ b/accel/tcg/cpu-exec-common.c > @@ -38,10 +38,11 @@ void tcg_cflags_set(CPUState *cpu, uint32_t flags) > > /* > * The bits of CPUState::tcg_cflags that tcg_cflags_set() never sets, because > - * they are derived from gdb single-step, one-insn-per-tb and -d nochain. > + * they are derived from gdb single-step, one-insn-per-tb and -d nochain, > + * or from various cpu parameters. > */ > #define CF_DERIVED (CF_COUNT_MASK | CF_NO_GOTO_TB | CF_NO_GOTO_PTR | \ > - CF_SINGLE_STEP) > + CF_NO_GOTO_JC | CF_SINGLE_STEP) > > void tcg_update_cflags(CPUState *cpu) > { > @@ -62,6 +63,17 @@ void tcg_update_cflags(CPUState *cpu) > cflags |= CF_NO_GOTO_TB; > } > > + /* > + * A block that dispatches through the jump cache inline does not consult > + * cpu->breakpoints, and inserting a breakpoint deliberately invalidates > + * nothing. Give blocks translated while one is set a distinct cflags, > so > + * that they neither dispatch inline themselves nor are reached by a > block > + * that does, and check_for_breakpoints() gets to run on every dispatch. > + */ > + if (unlikely(!QTAILQ_EMPTY(&cpu->breakpoints))) { > + cflags |= CF_NO_GOTO_JC; > + } > + > cpu->tcg_cflags = cflags; > } > > diff --git a/cpu-common.c b/cpu-common.c > index adb76b3a786..909f8d059b4 100644 > --- a/cpu-common.c > +++ b/cpu-common.c > @@ -22,6 +22,7 @@ > #include "exec/cpu-common.h" > #include "hw/core/cpu.h" > #include "qemu/lockable.h" > +#include "system/tcg.h" > #include "trace/trace-root.h" > > QemuMutex qemu_cpu_list_lock; > @@ -429,6 +430,11 @@ int cpu_breakpoint_insert(CPUState *cpu, vaddr pc, int > flags, > *breakpoint = bp; > } > > + /* The first breakpoint takes the CPU off the inline dispatch path. */ > + if (tcg_enabled()) { > + tcg_update_cflags(cpu); > + } > + > trace_breakpoint_insert(cpu->cpu_index, pc, flags); > return 0; > } > @@ -452,7 +458,7 @@ int cpu_breakpoint_remove(CPUState *cpu, vaddr pc, int > flags) > } > > /* Remove a specific breakpoint by reference. */ > -void cpu_breakpoint_remove_by_ref(CPUState *cpu, CPUBreakpoint *bp) > +static void cpu_breakpoint_remove_by_ref_int(CPUState *cpu, CPUBreakpoint > *bp) > { > QTAILQ_REMOVE(&cpu->breakpoints, bp, entry); > > @@ -460,14 +466,31 @@ void cpu_breakpoint_remove_by_ref(CPUState *cpu, > CPUBreakpoint *bp) > g_free(bp); > } > > +void cpu_breakpoint_remove_by_ref(CPUState *cpu, CPUBreakpoint *bp) > +{ > + cpu_breakpoint_remove_by_ref_int(cpu, bp); > + > + /* The last breakpoint puts the CPU back on the inline dispatch path. */ > + if (tcg_enabled()) { > + tcg_update_cflags(cpu); > + } > +} > + > /* Remove all matching breakpoints. */ > void cpu_breakpoint_remove_all(CPUState *cpu, int mask) > { > CPUBreakpoint *bp, *next; > + bool removed = false; > > QTAILQ_FOREACH_SAFE(bp, &cpu->breakpoints, entry, next) { > if (bp->flags & mask) { > cpu_breakpoint_remove_by_ref(cpu, bp); > + removed = true; > } > } > + > + /* The last breakpoint puts the CPU back on the inline dispatch path. */ > + if (tcg_enabled() && removed) { > + tcg_update_cflags(cpu); > + } > }
The loop still calls the public cpu_breakpoint_remove_by_ref(), which now calls tcg_update_cflags() itself, so this updates the cflags once per removed breakpoint and then once more at the end. I assume the loop was meant to call cpu_breakpoint_remove_by_ref_int()?
