Re: [PATCH v4 5/9] accel/tcg: give the TB jump cache a second base pointer for generated code

[Richard Henderson <richard.henderson@linaro.org>]

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.

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

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

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.

Conveniently, you've already shown how to adjust tcg_cflags from 
gdbstub.  :-)


r~