On Thu, Sep 17, 2026 at 4:26 AM Richard Henderson
<[email protected]> wrote:
>
> From: Matt Turner <[email protected]>
Thanks for reworking this; routing the system-mode update through
update_guest_debug is much nicer than the version I posted.
Three optional nits inline. None of those need a respin on my account.
> curr_cflags() is called once per TB dispatch, from helper_lookup_tb_ptr()
> and from the cpu_exec() loop. It recomputes the same value every time:
>
> uint32_t cflags = cpu->tcg_cflags;
> if (unlikely(cpu_single_stepping(cpu))) { ... }
> else if (qatomic_read(&one_insn_per_tb)) { ... }
> else if (qemu_loglevel_mask(CPU_LOG_TB_NOCHAIN)) { ... }
>
> That is three loads and three branches on the hottest path in the
> interpreter, for state that changes only when gdb enables single-step,
> when one-insn-per-tb is toggled, or when the log mask changes.
>
> None of the three has to be sampled at dispatch time. Fold each into
> CPUState::tcg_cflags where it changes and curr_cflags() becomes a single
> load of a field that TB lookup has to read anyway.
>
> The derived bits -- CF_COUNT_MASK, CF_NO_GOTO_TB, CF_NO_GOTO_PTR and
> CF_SINGLE_STEP -- are never set by tcg_cflags_set(), so tcg_update_cflags()
> can recompute them in place without disturbing the rest, and conversely
> tcg_cflags_set() ORs in its bits without disturbing them.
>
> There are three places to call it:
>
> - tcg_exec_realizefn(), so that a CPU created after the command line has
> been parsed starts out with the right value. This covers user-only,
> where tcg_cpu_init_cflags() is not reached. linux-user's cpu_copy()
> copies tcg_cflags wholesale, so a cloned thread inherits it.
>
> - cpu_single_step(), which changes one CPU. gdb is the only caller that
> matters; in system mode it runs with the vCPUs stopped, and in user mode
> gdb_continue_partial() can reach a thread that is still running, because
> gdb_handlesig() stops only the thread that trapped. That is exactly the
> plain cross-thread store to another CPU's CPUState that
> cpu->singlestep_flags already was, read back by that CPU through
> cpu_single_stepping() in curr_cflags(). This patch changes which field
> carries it, not who writes it or how.
>
> - hmp_one_insn_per_tb() and hmp_log(), which change every CPU while the
> vCPUs are running, so the update is queued with async_run_on_cpu() and
> each CPU writes its own cflags from its own thread. The command line
> spellings of those two settings need nothing: they are parsed before
> any CPU is realized, so tcg_exec_realizefn() picks them up.
>
> Measured with qemu-alpha running an emulated alpha gcc 16.2.0 compiling
> the SQLite 3.45.1 amalgamation (255k lines, -O2) on an x86-64 host, in a
> build configured with --enable-lto:
>
> before: 1,646,994,254,249 instructions
> after: 1,562,204,796,597 instructions -5.15%
>
> That workload issues 8.4 billion dispatches, so the per-call saving is
> small but the aggregate is not. The emulated compiler produces
> byte-identical output before and after.
>
> Wall clock does not move: 133.19s to 132.58s, a 0.46% difference against a
> run-to-run spread larger than that. The removed work is a few predictable
> loads and branches that the host executes largely in parallel with the
> surrounding dispatch, so this patch is worth taking for the instruction
> count and for what it enables, not for a time saving that can be measured
> on its own.
>
> v4: Update the cflags from the HMP handlers for 'log' and 'one-insn-per-tb'
> rather than from qemu_set_log_internal() and the accelerator property
> setter. Those are the paths that reach a running vCPU, and the monitor
> is the only thing that does. Suggested by Richard Henderson.
>
> v4: Queue the per-CPU update with async_run_on_cpu() rather than
> async_safe_run_on_cpu(). Halting the other vCPUs buys nothing: the
> queued work already runs on the owning CPU's own thread. Suggested by
> Alex Bennee, who also asked whether there are cross-vCPU updates of
> tcg_cflags at all. With this change the monitor path has none: the
> only remaining writer from another thread is cpu_single_step(), above,
> which is neither new nor made worse here.
>
> v4: Move the stub to accel/stubs/, which is where the other accelerator
> stubs live.
>
> Signed-off-by: Matt Turner <[email protected]>
> Reviewed-by: Richard Henderson <[email protected]>
> Signed-off-by: Richard Henderson <[email protected]>
> Message-ID: <[email protected]>
> ---
> accel/tcg/internal-common.h | 11 +++++++++--
> include/system/tcg.h | 12 ++++++++++++
> accel/tcg/cpu-exec-common.c | 33 ++++++++++++++++++++++++++++++---
> accel/tcg/cpu-exec.c | 3 +++
> accel/tcg/tcg-accel-ops.c | 2 ++
> cpu-target.c | 5 ++++-
> monitor/hmp-cmds.c | 7 +++++++
> system/runstate-hmp-cmds.c | 18 ++++++++++++------
> 8 files changed, 79 insertions(+), 12 deletions(-)
>
> diff --git a/accel/tcg/internal-common.h b/accel/tcg/internal-common.h
> index 9e7be2d78df..853d1b51eeb 100644
> --- a/accel/tcg/internal-common.h
> +++ b/accel/tcg/internal-common.h
> @@ -69,8 +69,15 @@ void tlb_destroy(CPUState *cpu);
> bool tcg_exec_realizefn(CPUState *cpu, Error **errp);
> void tcg_exec_unrealizefn(CPUState *cpu);
>
> -/* current cflags for hashing/comparison */
> -uint32_t curr_cflags(CPUState *cpu);
> +/*
> + * Current cflags for hashing/comparison. Everything that feeds into the
> + * value is folded into CPUState::tcg_cflags when it changes, by
> + * tcg_update_cflags(), so that TB dispatch only has to load it.
> + */
> +static inline uint32_t curr_cflags(CPUState *cpu)
> +{
> + return cpu->tcg_cflags;
> +}
>
> void tb_check_watchpoint(CPUState *cpu, uintptr_t retaddr);
>
> diff --git a/include/system/tcg.h b/include/system/tcg.h
> index 7622dcea302..2c2dbc753b3 100644
> --- a/include/system/tcg.h
> +++ b/include/system/tcg.h
> @@ -17,6 +17,18 @@ extern bool tcg_allowed;
> #define tcg_enabled() 0
> #endif
>
> +/*
> + * Recompute the parts of CPUState::tcg_cflags that TB dispatch consumes but
> + * tcg_cflags_set() does not provide: gdb single-step, one-insn-per-tb and
> + * the CPU_LOG_TB_NOCHAIN log flag. Call whenever one of those changes.
> + *
> + * tcg_update_cflags() updates one CPU and must be called from that CPU's
> + * thread, or with it stopped. tcg_update_all_cflags() updates every CPU
> + * and is safe to call from the monitor while the vCPUs run.
> + */
> +void tcg_update_cflags(CPUState *cpu);
> +void tcg_update_all_cflags(void);
> +
> /**
> * qemu_tcg_mttcg_enabled:
> * Check whether we are running MultiThread TCG or not.
> diff --git a/accel/tcg/cpu-exec-common.c b/accel/tcg/cpu-exec-common.c
> index 44e84344f3b..9f3517f36b6 100644
> --- a/accel/tcg/cpu-exec-common.c
> +++ b/accel/tcg/cpu-exec-common.c
> @@ -36,9 +36,16 @@ void tcg_cflags_set(CPUState *cpu, uint32_t flags)
> cpu->tcg_cflags |= flags;
> }
>
> -uint32_t curr_cflags(CPUState *cpu)
> +/*
> + * 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.
> + */
tcg_cflags_set() could assert(!(flags & CF_DERIVED)).
> +#define CF_DERIVED (CF_COUNT_MASK | CF_NO_GOTO_TB | CF_NO_GOTO_PTR | \
> + CF_SINGLE_STEP)
> +
> +void tcg_update_cflags(CPUState *cpu)
> {
> - uint32_t cflags = cpu->tcg_cflags;
> + uint32_t cflags = cpu->tcg_cflags & ~CF_DERIVED;
>
> /*
> * Record gdb single-step. We should be exiting the TB by raising
> @@ -55,7 +62,27 @@ uint32_t curr_cflags(CPUState *cpu)
> cflags |= CF_NO_GOTO_TB;
> }
>
> - return cflags;
> + cpu->tcg_cflags = cflags;
> +}
> +
> +static void tcg_update_cflags_work(CPUState *cpu, run_on_cpu_data data)
> +{
> + tcg_update_cflags(cpu);
> +}
> +
> +void tcg_update_all_cflags(void)
> +{
> + CPUState *cpu;
> +
Could assert(bql_locked()) here, since CPU_FOREACH plus
async_run_on_cpu() requires it.
> + /*
> + * one-insn-per-tb and -d nochain can both be changed from the monitor
> + * while the vCPUs are running. Queue the update onto each CPU rather
> + * than writing tcg_cflags from here, so that the field is only ever
> + * written by the CPU that owns it.
> + */
> + CPU_FOREACH(cpu) {
> + async_run_on_cpu(cpu, tcg_update_cflags_work, RUN_ON_CPU_NULL);
> + }
> }
>
> /* exit the current TB, but without causing any exception to be raised */
> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> index 46b723cb734..5226cfb7650 100644
> --- a/accel/tcg/cpu-exec.c
> +++ b/accel/tcg/cpu-exec.c
> @@ -1071,6 +1071,9 @@ bool tcg_exec_realizefn(CPUState *cpu, Error **errp)
> tcg_target_initialized = true;
> }
>
> + /* Pick up one-insn-per-tb and -d nochain from the command line. */
> + tcg_update_cflags(cpu);
> +
> cpu->tb_jmp_cache = g_new0(CPUJumpCache, 1);
> tlb_init(cpu);
> #ifndef CONFIG_USER_ONLY
> diff --git a/accel/tcg/tcg-accel-ops.c b/accel/tcg/tcg-accel-ops.c
> index 9c3d2214162..2e50df20c18 100644
> --- a/accel/tcg/tcg-accel-ops.c
> +++ b/accel/tcg/tcg-accel-ops.c
> @@ -48,6 +48,7 @@
> #include "tcg-accel-ops-mttcg.h"
> #include "tcg-accel-ops-rr.h"
> #include "tcg-accel-ops-icount.h"
> +#include "internal-common.h"
tcg_update_cflags() is declared in "system/tcg.h", which this file
already includes. The new include looks unnecessary.
>
> /* common functionality among all TCG variants */
>
> @@ -222,6 +223,7 @@ static void tcg_accel_ops_init(AccelClass *ac)
> ops->insert_gdbstub_breakpoint = tcg_insert_gdbstub_breakpoint;
> ops->remove_gdbstub_breakpoint = tcg_remove_gdbstub_breakpoint;
> ops->remove_all_gdbstub_breakpoints = tcg_remove_all_gdbstub_breakpoints;
> + ops->update_guest_debug = tcg_update_cflags;
> }
>
> static void tcg_accel_ops_class_init(ObjectClass *oc, const void *data)
> diff --git a/cpu-target.c b/cpu-target.c
> index 4783845c9bf..9bd1605e831 100644
> --- a/cpu-target.c
> +++ b/cpu-target.c
> @@ -24,6 +24,7 @@
> #include "exec/replay-core.h"
> #include "exec/log.h"
> #include "hw/core/cpu.h"
> +#include "system/tcg.h"
> #include "trace/trace-root.h"
>
> /* enable or disable single step mode. EXCP_DEBUG is returned by the
> @@ -35,7 +36,9 @@ void cpu_single_step(CPUState *cpu, unsigned flags)
> cpu->singlestep_flags, flags);
> cpu->singlestep_flags = flags;
>
> -#if !defined(CONFIG_USER_ONLY)
> +#ifdef CONFIG_USER_ONLY
> + tcg_update_cflags(cpu);
> +#else
> const AccelOpsClass *ops = cpus_get_accel();
> if (ops->update_guest_debug) {
> ops->update_guest_debug(cpu);
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index 91701ddf331..9b1b94f8ea9 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -40,6 +40,7 @@
> #include "system/hw_accel.h"
> #include "system/memory.h"
> #include "system/system.h"
> +#include "system/tcg.h"
> #include "disas/disas.h"
>
> /* Please update hmp-commands.hx when adding or changing commands */
> @@ -340,6 +341,12 @@ void hmp_log(MonitorHMP *hmp, const QDict *qdict)
>
> if (!qemu_set_log(mask, &err)) {
> error_report_err(err);
> + return;
> + }
> +
> + /* CPU_LOG_TB_NOCHAIN feeds into the per-CPU cflags. */
> + if (tcg_enabled()) {
> + tcg_update_all_cflags();
> }
> }
>
> diff --git a/system/runstate-hmp-cmds.c b/system/runstate-hmp-cmds.c
> index ad70b53f8ab..eb34d14d062 100644
> --- a/system/runstate-hmp-cmds.c
> +++ b/system/runstate-hmp-cmds.c
> @@ -22,6 +22,7 @@
> #include "qapi/qapi-commands-run-state.h"
> #include "qobject/qdict.h"
> #include "qemu/accel.h"
> +#include "system/tcg.h"
>
> void hmp_info_status(MonitorHMP *hmp, const QDict *qdict)
> {
> @@ -43,16 +44,17 @@ void hmp_info_status(MonitorHMP *hmp, const QDict *qdict)
>
> void hmp_one_insn_per_tb(MonitorHMP *hmp, const QDict *qdict)
> {
> - const char *option = qdict_get_try_str(qdict, "option");
> - AccelState *accel = current_accel();
> + const char *option;
> + AccelState *accel;
> bool newval;
>
> - if (!object_property_find(OBJECT(accel), "one-insn-per-tb")) {
> - monitor_hmp_printf(hmp,
> - "This accelerator does not support setting
> one-insn-per-tb\n");
> + if (!tcg_enabled()) {
> + monitor_hmp_printf(hmp, "This accelerator does not support "
> + "setting one-insn-per-tb\n");
> return;
> }
>
> + option = qdict_get_try_str(qdict, "option");
> if (!option || !strcmp(option, "on")) {
> newval = true;
> } else if (!strcmp(option, "off")) {
> @@ -61,9 +63,13 @@ void hmp_one_insn_per_tb(MonitorHMP *hmp, const QDict
> *qdict)
> monitor_hmp_printf(hmp, "unexpected option %s\n", option);
> return;
> }
> - /* If the property exists then setting it can never fail */
> +
> + accel = current_accel();
> object_property_set_bool(OBJECT(accel), "one-insn-per-tb",
> newval, &error_abort);
> +
> + /* one-insn-per-tb feeds into the per-CPU cflags. */
> + tcg_update_all_cflags();
> }
>
> void hmp_watchdog_action(MonitorHMP *hmp, const QDict *qdict)
> --
> 2.53.0
>
