On Thu, Aug 27, 2026 at 4:03 PM Richard Henderson
<[email protected]> wrote:
>
> On 8/26/26 22:02, Matt Turner wrote:
> > +/*
> > + * The inline jump cache probe reads cpu->tb_jmp_cache_probe and takes the
> > + * slow path when the entry it finds has a NULL tb. Pointing the probe at
> > a
> > + * region that is all zeroes therefore forces every indirect dispatch into
> > + * helper_lookup_tb_ptr(), which does the full lookup the inline probe only
> > + * approximates. The real jump cache is untouched, so no contents are lost
> > + * and recovery is a single store.
> > + *
> > + * Only ever read from, and only one entry per dispatch, so one shared
> > + * zero-filled cache is enough for every CPU. Not const: that would put a
> > + * megabyte of zeroes in .rodata and so in the binary, where .bss costs
> > + * nothing on disk and only faults in the handful of pages a poisoned run
> > + * happens to probe.
> > + */
> > +static CPUJumpCache tb_jmp_cache_poison;
>
> Hmm. Maybe we should mmap it at startup then, because while we're not
> supposed to be writing into this, it would be nice to enforce that.
Done, also in 8/9. It's a page-aligned qemu_memalign() + mprotect at
startup instead of a .bss object, which also removes the "1MB never
written" note from that patch's RFC list. I added qemu_mprotect_ro()
next to the existing _rw/_rwx/_none forms for it. A stray store into
a page every vCPU dispatches through is worth trapping rather than
debugging.
> > +/*
> > + * Poison @cpu's probe, from any thread. Called when a breakpoint is
> > + * inserted, which is what makes the poison take effect at the dispatch
> > + * after the insert rather than whenever @cpu next reaches its main loop:
> > + * a vCPU chaining indirectly need never reach it, and would run past a
> > + * breakpoint another thread had just set.
> > + *
> > + * A plain store is enough. The value only ever costs a slow path that is
> > + * correct on its own, and the generated code re-reads the base on every
> > + * dispatch. Un-poisoning is tcg_cpu_sync_jmp_cache()'s job.
> > + */
> > +void tcg_cpu_poison_jmp_cache(CPUState *cpu)
> > +{
> > + if (qatomic_read(&cpu->tb_jmp_cache_probe) != NULL) {
> > + qatomic_set(&cpu->tb_jmp_cache_probe, &tb_jmp_cache_poison);
> > + }
> > +}
>
> Why do we need to check for NULL?
Fixed, see below.
> > +
> > +/*
> > + * Called from the main loop, which is the only context that can establish
> > + * that no reason to be poisoned is left. Cheap enough to call every time
> > + * round: the common case is a load, a compare and no store at all.
> > + */
> > +void tcg_cpu_sync_jmp_cache(CPUState *cpu)
> > +{
> > + CPUJumpCache *want;
> > +
> > + if (qatomic_read(&cpu->tb_jmp_cache_probe) == NULL) {
> > + return; /* not realized, or already unrealized */
> > + }
>
> If this function is only called by the main loop, we shouldn't have to
> deal with either unrealized state.
>
> > +
> > + want = tcg_cpu_may_dispatch(cpu)
> > + ? cpu->tb_jmp_cache
> > + : &tb_jmp_cache_poison;
> > +
> > + if (qatomic_read(&cpu->tb_jmp_cache_probe) != want) {
> > + qatomic_set(&cpu->tb_jmp_cache_probe, want);
> > +
> > + if (want == cpu->tb_jmp_cache) {
> > + /*
> > + * Un-poisoning races a concurrent tcg_cpu_poison_jmp_cache():
> > + * the reason may have appeared after tcg_cpu_may_dispatch()
> > read
> > + * it, and the poison may have landed before the store above.
> > + * Order that store against the re-read below, so that the race
> > + * is lost in the safe direction.
> > + */
> > + smp_mb();
> > + if (!tcg_cpu_may_dispatch(cpu)) {
> > + tcg_cpu_poison_jmp_cache(cpu);
>
> Why do we need to check for breakpoints twice?
> I don't think I understand this race.
Fixed, see below.
> I'm not familiar with how gdbstub interacts with user threads, but this
> feels overly complicated. Up to and including needing to poison the
> jump cache just for adding a breakpoint.
>
> I suspect what we need is to add a CF_NO_GOTO_JC flag that suppresses
> the inline jump cache, which is set whenever any breakpoint exists,
> which falls back to the helper, which checks for breakpoints.
You're right, and this is much better. v5 replaces this patch with
"accel/tcg: add CF_NO_GOTO_JC, set while a breakpoint is present".
What makes the cflag sufficient rather than merely convenient: the
inline probe already compares the destination TB's cflags against the
constant cflags of the block doing the dispatching, and only takes the
destination when they're equal. So a block translated while a
breakpoint exists neither dispatches inline itself nor can be reached
by a block that does. tcg_update_cflags() sets the flag while
cpu->breakpoints is non-empty, and
cpu_breakpoint_insert()/cpu_breakpoint_remove_by_ref() call it -- the
only two places the list changes, both of which already run on the
CPU's own thread or with it stopped, or reach another CPU exactly the
way cpu_single_step() does.
That deletes the entire mechanism you were objecting to. The
cross-thread poison from breakpoint insertion, the un-poison, and the
double breakpoint check are all gone, so the questions above mostly
answer themselves.
