[PATCH v2 00/10] Deprecate/rename singlestep command line option, monitor interfaces
The command line option '-singlestep' and its HMP equivalent the 'singlestep' command are very confusingly named, because they have nothing to do with single-stepping the guest (either via the gdb stub or by emulation of guest CPU architectural debug facilities). What they actually do is put TCG into a mode where it puts only one guest instruction into each translation block. This is useful for some circumstances such as when you want the -d debug logging to be easier to interpret, or if you have a finicky guest binary that wants to see interrupts delivered at something other than the end of a basic block. The confusing name is made worse by the fact that our documentation for these is so minimal as to be useless for telling users what they really do. This series: * changes the command line interface: for user-mode emulators, the new option is '-one-insn-per-tb', and for system mode emulators it is a TCG accel property '-accel tcg,one-insn-per-tb=on' * updates all the places which currently directly touch the 'singlestep' global variable to instead get the current accelerator and query/set the QOM property * documents that the old -singlestep option is deprecated * adds a new HMP command 'one-insn-per-tb', and deprecates the old 'singlestep' command. (Strictly we don't need to deprecate HMP commands, but I'd already written the code for v1 of this series and it's a minor user convenience.) * makes the HMP 'info status' output report "one insn per TB" instead of "single step mode" * adds a new 'one-insn-per-tb' member to the QMP StatusInfo type, and deprecates the old 'singlestep' field Notes: * I hope I have got the QMP changes and deprecation right, but that's probably the bit in most need of review from an expert * There's an argument for just dropping the reporting of one-insn-per-tb in QMP StatusInfo at all, except that (a) it's hard to know if anybody's really using it (b) then the info isn't reported to HMP 'info status', which wouldn't line up with HMP having a mechanism to get/set the value * I have written patch 3 on the assumption that curr_cflags() is not such a hot codepath that we can't afford to have a QOM cast macro in it; the alternative would be to keep it using a global variable but make the global be restricted to accel/tcg/internals.h. RTH: opinions welcome... * Still haven't tested on bsd-user, but the patch is identical to the linux-user change thanks -- PMM Peter Maydell (10): make one-insn-per-tb an accel option softmmu: Don't use 'singlestep' global in QMP and HMP commands tcg: Use one-insn-per-tb accelerator property in curr_cflags() linux-user: Add '-one-insn-per-tb' option equivalent to '-singlestep' bsd-user: Add '-one-insn-per-tb' option equivalent to '-singlestep' Document that -singlestep command line option is deprecated hmp: Add 'one-insn-per-tb' command equivalent to 'singlestep' hmp: Report 'one-insn-per-tb', not 'single step mode', in 'info status' output qapi/run-state.json: Fix missing newline at end of file hmp: Deprecate 'singlestep' member of StatusInfo docs/about/deprecated.rst | 35 +++++++++++++++++++++++++++++++++++ docs/interop/qmp-intro.txt | 1 + docs/user/main.rst | 14 ++++++++++++-- qapi/run-state.json | 19 +++++++++++++++---- accel/tcg/internal.h | 16 ++++++++++++++++ include/exec/cpu-common.h | 3 --- include/monitor/hmp.h | 2 +- accel/tcg/cpu-exec.c | 5 +++-- accel/tcg/tcg-all.c | 32 ++++++++++++++++++-------------- bsd-user/main.c | 14 +++++++++----- linux-user/main.c | 18 ++++++++++++------ softmmu/globals.c | 1 - softmmu/runstate-hmp-cmds.c | 22 ++++++++++++++++++---- softmmu/runstate.c | 12 +++++++++++- softmmu/vl.c | 17 +++++++++++++++-- tests/qtest/test-hmp.c | 1 + hmp-commands.hx | 25 +++++++++++++++++++++---- qemu-options.hx | 12 ++++++++++-- tcg/tci/README | 2 +- 19 files changed, 199 insertions(+), 52 deletions(-) -- 2.34.1
This commit adds 'one-insn-per-tb' as a property on the TCG accelerator object, so you can enable it with -accel tcg,one-insn-per-tb=on It has the same behaviour as the existing '-singlestep' command line option. We use a different name because 'singlestep' has always been a confusing choice, because it doesn't have anything to do with single-stepping the CPU. What it does do is force TCG emulation to put one guest instruction in each TB, which can be useful in some situations (such as analysing debug logs). The existing '-singlestep' commandline options are decoupled from the global 'singlestep' variable and instead now are syntactic sugar for setting the accel property. (These can then go away after a deprecation period.) The global variable remains for the moment as: * what the TCG code looks at to change its behaviour * what HMP and QMP use to query and set the behaviour In the following commits we'll clean those up to not directly look at the global variable. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- accel/tcg/tcg-all.c | 21 +++++++++++++++++++++ bsd-user/main.c | 8 ++++++-- linux-user/main.c | 8 ++++++-- softmmu/vl.c | 17 +++++++++++++++-- qemu-options.hx | 7 +++++++ 5 files changed, 55 insertions(+), 6 deletions(-) diff --git a/accel/tcg/tcg-all.c b/accel/tcg/tcg-all.c index 5dab1ae9dd3..fcf361c8db6 100644 --- a/accel/tcg/tcg-all.c +++ b/accel/tcg/tcg-all.c @@ -42,6 +42,7 @@ struct TCGState { AccelState parent_obj; bool mttcg_enabled; + bool one_insn_per_tb; int splitwx_enabled; unsigned long tb_size; }; @@ -208,6 +209,20 @@ static void tcg_set_splitwx(Object *obj, bool value, Error **errp) s->splitwx_enabled = value; } +static bool tcg_get_one_insn_per_tb(Object *obj, Error **errp) +{ + TCGState *s = TCG_STATE(obj); + return s->one_insn_per_tb; +} + +static void tcg_set_one_insn_per_tb(Object *obj, bool value, Error **errp) +{ + TCGState *s = TCG_STATE(obj); + s->one_insn_per_tb = value; + /* For the moment, set the global also: this changes the behaviour */ + singlestep = value; +} + static int tcg_gdbstub_supported_sstep_flags(void) { /* @@ -245,6 +260,12 @@ static void tcg_accel_class_init(ObjectClass *oc, void *data) tcg_get_splitwx, tcg_set_splitwx); object_class_property_set_description(oc, "split-wx", "Map jit pages into separate RW and RX regions"); + + object_class_property_add_bool(oc, "one-insn-per-tb", + tcg_get_one_insn_per_tb, + tcg_set_one_insn_per_tb); + object_class_property_set_description(oc, "one-insn-per-tb", + "Only put one guest insn in each translation block"); } static const TypeInfo tcg_accel_type = { diff --git a/bsd-user/main.c b/bsd-user/main.c index babc3b009b6..09b84da190c 100644 --- a/bsd-user/main.c +++ b/bsd-user/main.c @@ -50,6 +50,7 @@ #include "target_arch_cpu.h" int singlestep; +static bool opt_one_insn_per_tb; uintptr_t guest_base; bool have_guest_base; /* @@ -386,7 +387,7 @@ int main(int argc, char **argv) } else if (!strcmp(r, "seed")) { seed_optarg = optarg; } else if (!strcmp(r, "singlestep")) { - singlestep = 1; + opt_one_insn_per_tb = true; } else if (!strcmp(r, "strace")) { do_strace = 1; } else if (!strcmp(r, "trace")) { @@ -444,9 +445,12 @@ int main(int argc, char **argv) /* init tcg before creating CPUs and to get qemu_host_page_size */ { - AccelClass *ac = ACCEL_GET_CLASS(current_accel()); + AccelState *accel = current_accel(); + AccelClass *ac = ACCEL_GET_CLASS(accel); accel_init_interfaces(ac); + object_property_set_bool(OBJECT(accel), "one-insn-per-tb", + opt_one_insn_per_tb, &error_abort); ac->init_machine(NULL); } cpu = cpu_create(cpu_type); diff --git a/linux-user/main.c b/linux-user/main.c index fe03293516a..489694ad654 100644 --- a/linux-user/main.c +++ b/linux-user/main.c @@ -69,6 +69,7 @@ char *exec_path; char real_exec_path[PATH_MAX]; int singlestep; +static bool opt_one_insn_per_tb; static const char *argv0; static const char *gdbstub; static envlist_t *envlist; @@ -411,7 +412,7 @@ static void handle_arg_reserved_va(const char *arg) static void handle_arg_singlestep(const char *arg) { - singlestep = 1; + opt_one_insn_per_tb = true; } static void handle_arg_strace(const char *arg) @@ -777,9 +778,12 @@ int main(int argc, char **argv, char **envp) /* init tcg before creating CPUs and to get qemu_host_page_size */ { - AccelClass *ac = ACCEL_GET_CLASS(current_accel()); + AccelState *accel = current_accel(); + AccelClass *ac = ACCEL_GET_CLASS(accel); accel_init_interfaces(ac); + object_property_set_bool(OBJECT(accel), "one-insn-per-tb", + opt_one_insn_per_tb, &error_abort); ac->init_machine(NULL); } cpu = cpu_create(cpu_type); diff --git a/softmmu/vl.c b/softmmu/vl.c index ea20b23e4c8..492b5fe65e6 100644 --- a/softmmu/vl.c +++ b/softmmu/vl.c @@ -182,6 +182,7 @@ static const char *log_file; static bool list_data_dirs; static const char *qtest_chrdev; static const char *qtest_log; +static bool opt_one_insn_per_tb; static int has_defaults = 1; static int default_serial = 1; @@ -2220,7 +2221,19 @@ static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp) qemu_opt_foreach(opts, accelerator_set_property, accel, &error_fatal); - + /* + * If legacy -singlestep option is set, honour it for TCG and + * silently ignore for any other accelerator (which is how this + * option has always behaved). + */ + if (opt_one_insn_per_tb) { + /* + * This will always succeed for TCG, and we want to ignore + * the error from trying to set a nonexistent property + * on any other accelerator. + */ + object_property_set_bool(OBJECT(accel), "one-insn-per-tb", true, NULL); + } ret = accel_init_machine(accel, current_machine); if (ret < 0) { if (!qtest_with_kvm || ret != -ENOENT) { @@ -2955,7 +2968,7 @@ void qemu_init(int argc, char **argv) qdict_put_str(machine_opts_dict, "firmware", optarg); break; case QEMU_OPTION_singlestep: - singlestep = 1; + opt_one_insn_per_tb = true; break; case QEMU_OPTION_S: autostart = 0; diff --git a/qemu-options.hx b/qemu-options.hx index 59bdf67a2c5..1dffd36884e 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -182,6 +182,7 @@ DEF("accel", HAS_ARG, QEMU_OPTION_accel, " igd-passthru=on|off (enable Xen integrated Intel graphics passthrough, default=off)\n" " kernel-irqchip=on|off|split controls accelerated irqchip support (default=on)\n" " kvm-shadow-mem=size of KVM shadow MMU in bytes\n" + " one-insn-per-tb=on|off (one guest instruction per TCG translation block)\n" " split-wx=on|off (enable TCG split w^x mapping)\n" " tb-size=n (TCG translation block cache size)\n" " dirty-ring-size=n (KVM dirty ring GFN count, default 0)\n" @@ -210,6 +211,12 @@ SRST ``kvm-shadow-mem=size`` Defines the size of the KVM shadow MMU. + ``one-insn-per-tb=on|off`` + Makes the TCG accelerator put only one guest instruction into + each translation block. This slows down emulation a lot, but + can be useful in some situations, such as when trying to analyse + the logs produced by the ``-d`` option. + ``split-wx=on|off`` Controls the use of split w^x mapping for the TCG code generation buffer. Some operating systems require this to be enabled, and in -- 2.34.1
On 4/3/23 07:46, Peter Maydell wrote:
This commit adds 'one-insn-per-tb' as a property on the TCG accelerator object, so you can enable it with -accel tcg,one-insn-per-tb=on
It has the same behaviour as the existing '-singlestep' command line option. We use a different name because 'singlestep' has always been a confusing choice, because it doesn't have anything to do with single-stepping the CPU. What it does do is force TCG emulation to put one guest instruction in each TB, which can be useful in some situations (such as analysing debug logs).
The existing '-singlestep' commandline options are decoupled from the global 'singlestep' variable and instead now are syntactic sugar for setting the accel property. (These can then go away after a deprecation period.)
The global variable remains for the moment as: * what the TCG code looks at to change its behaviour * what HMP and QMP use to query and set the behaviour
In the following commits we'll clean those up to not directly look at the global variable.
Signed-off-by: Peter Maydell<peter.maydell@linaro.org> --- accel/tcg/tcg-all.c | 21 +++++++++++++++++++++ bsd-user/main.c | 8 ++++++-- linux-user/main.c | 8 ++++++-- softmmu/vl.c | 17 +++++++++++++++-- qemu-options.hx | 7 +++++++ 5 files changed, 55 insertions(+), 6 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
The HMP 'singlestep' command, the QMP 'query-status' command and the HMP 'info status' command (which is just wrapping the QMP command implementation) look at the 'singlestep' global variable. Make them access the new TCG accelerator 'one-insn-per-tb' property instead. This leaves the HMP and QMP command/field names and output strings unchanged; we will clean that up later. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- softmmu/runstate-hmp-cmds.c | 18 ++++++++++++++++-- softmmu/runstate.c | 10 +++++++++- 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/softmmu/runstate-hmp-cmds.c b/softmmu/runstate-hmp-cmds.c index d55a7d4db89..127521a483a 100644 --- a/softmmu/runstate-hmp-cmds.c +++ b/softmmu/runstate-hmp-cmds.c @@ -20,6 +20,7 @@ #include "qapi/error.h" #include "qapi/qapi-commands-run-state.h" #include "qapi/qmp/qdict.h" +#include "qemu/accel.h" void hmp_info_status(Monitor *mon, const QDict *qdict) { @@ -43,13 +44,26 @@ void hmp_info_status(Monitor *mon, const QDict *qdict) void hmp_singlestep(Monitor *mon, const QDict *qdict) { const char *option = qdict_get_try_str(qdict, "option"); + AccelState *accel = current_accel(); + bool newval; + + if (!object_property_find(OBJECT(accel), "one-insn-per-tb")) { + monitor_printf(mon, + "This accelerator does not support setting one-insn-per-tb\n"); + return; + } + if (!option || !strcmp(option, "on")) { - singlestep = 1; + newval = true; } else if (!strcmp(option, "off")) { - singlestep = 0; + newval = false; } else { monitor_printf(mon, "unexpected option %s\n", option); + return; } + /* If the property exists then setting it can never fail */ + object_property_set_bool(OBJECT(accel), "one-insn-per-tb", + newval, &error_abort); } void hmp_watchdog_action(Monitor *mon, const QDict *qdict) diff --git a/softmmu/runstate.c b/softmmu/runstate.c index d1e04586dbc..2f2396c819e 100644 --- a/softmmu/runstate.c +++ b/softmmu/runstate.c @@ -40,6 +40,7 @@ #include "qapi/error.h" #include "qapi/qapi-commands-run-state.h" #include "qapi/qapi-events-run-state.h" +#include "qemu/accel.h" #include "qemu/error-report.h" #include "qemu/job.h" #include "qemu/log.h" @@ -234,9 +235,16 @@ bool runstate_needs_reset(void) StatusInfo *qmp_query_status(Error **errp) { StatusInfo *info = g_malloc0(sizeof(*info)); + AccelState *accel = current_accel(); + /* + * We ignore errors, which will happen if the accelerator + * is not TCG. "singlestep" is meaningless for other accelerators, + * so we will set the StatusInfo field to false for those. + */ + info->singlestep = object_property_get_bool(OBJECT(accel), + "one-insn-per-tb", NULL); info->running = runstate_is_running(); - info->singlestep = singlestep; info->status = current_run_state; return info; -- 2.34.1
On 4/3/23 07:46, Peter Maydell wrote:
The HMP 'singlestep' command, the QMP 'query-status' command and the HMP 'info status' command (which is just wrapping the QMP command implementation) look at the 'singlestep' global variable. Make them access the new TCG accelerator 'one-insn-per-tb' property instead.
This leaves the HMP and QMP command/field names and output strings unchanged; we will clean that up later.
Signed-off-by: Peter Maydell<peter.maydell@linaro.org> --- softmmu/runstate-hmp-cmds.c | 18 ++++++++++++++++-- softmmu/runstate.c | 10 +++++++++- 2 files changed, 25 insertions(+), 3 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
Change curr_cflags() to look at the one-insn-per-tb accelerator property instead of the old singlestep global variable. Since this is the final remaining use of the global, we can delete it entirely. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- This is the "clean" way of doing it. I dunno how much of a hot path curr_cflags is; if it's really critical we could have a global that's private to accel/tcg/internals.h I guess. --- accel/tcg/internal.h | 16 ++++++++++++++++ include/exec/cpu-common.h | 3 --- accel/tcg/cpu-exec.c | 5 +++-- accel/tcg/tcg-all.c | 17 ----------------- bsd-user/main.c | 1 - linux-user/main.c | 1 - softmmu/globals.c | 1 - 7 files changed, 19 insertions(+), 25 deletions(-) diff --git a/accel/tcg/internal.h b/accel/tcg/internal.h index 96f198b28b4..6ea5f7a295f 100644 --- a/accel/tcg/internal.h +++ b/accel/tcg/internal.h @@ -10,6 +10,22 @@ #define ACCEL_TCG_INTERNAL_H #include "exec/exec-all.h" +#include "qemu/accel.h" + +struct TCGState { + AccelState parent_obj; + + bool mttcg_enabled; + bool one_insn_per_tb; + int splitwx_enabled; + unsigned long tb_size; +}; +typedef struct TCGState TCGState; + +#define TYPE_TCG_ACCEL ACCEL_CLASS_NAME("tcg") + +DECLARE_INSTANCE_CHECKER(TCGState, TCG_STATE, + TYPE_TCG_ACCEL) /* * Access to the various translations structures need to be serialised diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h index 6feaa40ca7b..609a29a5dc2 100644 --- a/include/exec/cpu-common.h +++ b/include/exec/cpu-common.h @@ -162,9 +162,6 @@ int ram_block_discard_range(RAMBlock *rb, uint64_t start, size_t length); int cpu_memory_rw_debug(CPUState *cpu, vaddr addr, void *ptr, size_t len, bool is_write); -/* vl.c */ -extern int singlestep; - void list_cpus(const char *optarg); #endif /* CPU_COMMON_H */ diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c index c815f2dbfdf..1ed3878b6b7 100644 --- a/accel/tcg/cpu-exec.c +++ b/accel/tcg/cpu-exec.c @@ -149,17 +149,18 @@ static void init_delay_params(SyncClocks *sc, const CPUState *cpu) uint32_t curr_cflags(CPUState *cpu) { uint32_t cflags = cpu->tcg_cflags; + TCGState *tcgstate = TCG_STATE(current_accel()); /* * Record gdb single-step. We should be exiting the TB by raising * EXCP_DEBUG, but to simplify other tests, disable chaining too. * - * For singlestep and -d nochain, suppress goto_tb so that + * For one-insn-per-tb and -d nochain, suppress goto_tb so that * we can log -d cpu,exec after every TB. */ if (unlikely(cpu->singlestep_enabled)) { cflags |= CF_NO_GOTO_TB | CF_NO_GOTO_PTR | CF_SINGLE_STEP | 1; - } else if (singlestep) { + } else if (tcgstate->one_insn_per_tb) { cflags |= CF_NO_GOTO_TB | 1; } else if (qemu_loglevel_mask(CPU_LOG_TB_NOCHAIN)) { cflags |= CF_NO_GOTO_TB; diff --git a/accel/tcg/tcg-all.c b/accel/tcg/tcg-all.c index fcf361c8db6..7c4f9f34d39 100644 --- a/accel/tcg/tcg-all.c +++ b/accel/tcg/tcg-all.c @@ -38,21 +38,6 @@ #endif #include "internal.h" -struct TCGState { - AccelState parent_obj; - - bool mttcg_enabled; - bool one_insn_per_tb; - int splitwx_enabled; - unsigned long tb_size; -}; -typedef struct TCGState TCGState; - -#define TYPE_TCG_ACCEL ACCEL_CLASS_NAME("tcg") - -DECLARE_INSTANCE_CHECKER(TCGState, TCG_STATE, - TYPE_TCG_ACCEL) - /* * We default to false if we know other options have been enabled * which are currently incompatible with MTTCG. Otherwise when each @@ -219,8 +204,6 @@ static void tcg_set_one_insn_per_tb(Object *obj, bool value, Error **errp) { TCGState *s = TCG_STATE(obj); s->one_insn_per_tb = value; - /* For the moment, set the global also: this changes the behaviour */ - singlestep = value; } static int tcg_gdbstub_supported_sstep_flags(void) diff --git a/bsd-user/main.c b/bsd-user/main.c index 09b84da190c..a9e5a127d38 100644 --- a/bsd-user/main.c +++ b/bsd-user/main.c @@ -49,7 +49,6 @@ #include "host-os.h" #include "target_arch_cpu.h" -int singlestep; static bool opt_one_insn_per_tb; uintptr_t guest_base; bool have_guest_base; diff --git a/linux-user/main.c b/linux-user/main.c index 489694ad654..c7020b413bc 100644 --- a/linux-user/main.c +++ b/linux-user/main.c @@ -68,7 +68,6 @@ char *exec_path; char real_exec_path[PATH_MAX]; -int singlestep; static bool opt_one_insn_per_tb; static const char *argv0; static const char *gdbstub; diff --git a/softmmu/globals.c b/softmmu/globals.c index 39678aa8c58..e83b5428d12 100644 --- a/softmmu/globals.c +++ b/softmmu/globals.c @@ -43,7 +43,6 @@ int vga_interface_type = VGA_NONE; bool vga_interface_created; Chardev *parallel_hds[MAX_PARALLEL_PORTS]; int win2k_install_hack; -int singlestep; int fd_bootchk = 1; int graphic_rotate; QEMUOptionRom option_rom[MAX_OPTION_ROMS]; -- 2.34.1
On 4/3/23 07:46, Peter Maydell wrote:
uint32_t curr_cflags(CPUState *cpu) { uint32_t cflags = cpu->tcg_cflags; + TCGState *tcgstate = TCG_STATE(current_accel());
As mentioned against the cover, this is a very hot path. We should try for something less expensive. Perhaps as simple as return cpu->tcg_cflags | tcg_cflags_global; where cpu->tcg_cflags is updated with cpu->singlestep_enabled. r~
On Mon, 3 Apr 2023 at 19:33, Richard Henderson <richard.henderson@linaro.org> wrote:
On 4/3/23 07:46, Peter Maydell wrote:
uint32_t curr_cflags(CPUState *cpu) { uint32_t cflags = cpu->tcg_cflags; + TCGState *tcgstate = TCG_STATE(current_accel());
As mentioned against the cover, this is a very hot path.
We should try for something less expensive. Perhaps as simple as
return cpu->tcg_cflags | tcg_cflags_global;
where cpu->tcg_cflags is updated with cpu->singlestep_enabled.
I feel like that introduces atomicity issues. If I'm reading the code right, curr_cflags() is called without any kind of lock held. At the moment we get away with this because 'singlestep' is an int and is always going to be atomically updated. If we make tcg_cflags_global a value which might have multiple bits set or not set I'm not entirely sure what the right way is to handle the reads and writes of it. I think we can assume we have the iothread lock at any point where we want to change either 'singlestep' or the 'nochain' option, at least. Any suggestions? I'm not very familiar with the qemu atomic primitives... thanks -- PMM
On 4/13/23 18:24, Peter Maydell wrote:
On Mon, 3 Apr 2023 at 19:33, Richard Henderson <richard.henderson@linaro.org> wrote:
On 4/3/23 07:46, Peter Maydell wrote:
uint32_t curr_cflags(CPUState *cpu) { uint32_t cflags = cpu->tcg_cflags; + TCGState *tcgstate = TCG_STATE(current_accel());
As mentioned against the cover, this is a very hot path.
We should try for something less expensive. Perhaps as simple as
return cpu->tcg_cflags | tcg_cflags_global;
where cpu->tcg_cflags is updated with cpu->singlestep_enabled.
I feel like that introduces atomicity issues. If I'm reading the code right, curr_cflags() is called without any kind of lock held. At the moment we get away with this because 'singlestep' is an int and is always going to be atomically updated. If we make tcg_cflags_global a value which might have multiple bits set or not set I'm not entirely sure what the right way is to handle the reads and writes of it.
qatomic_read() here, will dtrt for no tearing on the read. (Not that we should have expected one anyway, for uint32_t.)
I think we can assume we have the iothread lock at any point where we want to change either 'singlestep' or the 'nochain' option, at least.
Indeed, it can only be changed by the monitor, under user control, so even without a lock there's no real race there. Using qatomic_set(&global, new_value) is sufficient to match the qatomic_read() for no tearing. Concurrent threads will see the old value or the new value, but not garbage, which is just fine. We probably need to kick all cpus, so that they come out of long-running TB chains to see the new value and re-translate. r~
The '-singlestep' option is confusing, because it doesn't actually have anything to do with single-stepping the CPU. What it does do is force TCG emulation to put one guest instruction in each TB, which can be useful in some situations. Create a new command line argument -one-insn-per-tb, so we can document that -singlestep is just a deprecated synonym for it, and eventually perhaps drop it. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- docs/user/main.rst | 7 ++++++- linux-user/main.c | 9 ++++++--- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/docs/user/main.rst b/docs/user/main.rst index 6f2ffa080f7..f9ac701f4b1 100644 --- a/docs/user/main.rst +++ b/docs/user/main.rst @@ -93,8 +93,13 @@ Debug options: ``-g port`` Wait gdb connection to port +``-one-insn-per-tb`` + Run the emulation with one guest instruction per translation block. + This slows down emulation a lot, but can be useful in some situations, + such as when trying to analyse the logs produced by the ``-d`` option. + ``-singlestep`` - Run the emulation in single step mode. + This is a deprecated synonym for the ``-one-insn-per-tb`` option. Environment variables: diff --git a/linux-user/main.c b/linux-user/main.c index c7020b413bc..5defe5a6db8 100644 --- a/linux-user/main.c +++ b/linux-user/main.c @@ -409,7 +409,7 @@ static void handle_arg_reserved_va(const char *arg) reserved_va = val ? val - 1 : 0; } -static void handle_arg_singlestep(const char *arg) +static void handle_arg_one_insn_per_tb(const char *arg) { opt_one_insn_per_tb = true; } @@ -500,8 +500,11 @@ static const struct qemu_argument arg_table[] = { "logfile", "write logs to 'logfile' (default stderr)"}, {"p", "QEMU_PAGESIZE", true, handle_arg_pagesize, "pagesize", "set the host page size to 'pagesize'"}, - {"singlestep", "QEMU_SINGLESTEP", false, handle_arg_singlestep, - "", "run in singlestep mode"}, + {"one-insn-per-tb", + "QEMU_ONE_INSN_PER_TB", false, handle_arg_one_insn_per_tb, + "", "run with one guest instruction per emulated TB"}, + {"singlestep", "QEMU_SINGLESTEP", false, handle_arg_one_insn_per_tb, + "", "deprecated synonym for -one-insn-per-tb"}, {"strace", "QEMU_STRACE", false, handle_arg_strace, "", "log system calls"}, {"seed", "QEMU_RAND_SEED", true, handle_arg_seed, -- 2.34.1
On 4/3/23 07:46, Peter Maydell wrote:
The '-singlestep' option is confusing, because it doesn't actually have anything to do with single-stepping the CPU. What it does do is force TCG emulation to put one guest instruction in each TB, which can be useful in some situations.
Create a new command line argument -one-insn-per-tb, so we can document that -singlestep is just a deprecated synonym for it, and eventually perhaps drop it.
Signed-off-by: Peter Maydell<peter.maydell@linaro.org> --- docs/user/main.rst | 7 ++++++- linux-user/main.c | 9 ++++++--- 2 files changed, 12 insertions(+), 4 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On Mon, Apr 3, 2023 at 12:35 PM Richard Henderson < richard.henderson@linaro.org> wrote:
On 4/3/23 07:46, Peter Maydell wrote:
The '-singlestep' option is confusing, because it doesn't actually have anything to do with single-stepping the CPU. What it does do is force TCG emulation to put one guest instruction in each TB, which can be useful in some situations.
Create a new command line argument -one-insn-per-tb, so we can document that -singlestep is just a deprecated synonym for it, and eventually perhaps drop it.
Signed-off-by: Peter Maydell<peter.maydell@linaro.org> --- docs/user/main.rst | 7 ++++++- linux-user/main.c | 9 ++++++--- 2 files changed, 12 insertions(+), 4 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Warner Losh <imp@bsdimp.com>
r~
The '-singlestep' option is confusing, because it doesn't actually have anything to do with single-stepping the CPU. What it does do is force TCG emulation to put one guest instruction in each TB, which can be useful in some situations. Create a new command line argument -one-insn-per-tb, so we can document that -singlestep is just a deprecated synonym for it, and eventually perhaps drop it. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- NB: not even compile tested! --- docs/user/main.rst | 7 ++++++- bsd-user/main.c | 5 +++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/docs/user/main.rst b/docs/user/main.rst index f9ac701f4b1..f4786353965 100644 --- a/docs/user/main.rst +++ b/docs/user/main.rst @@ -247,5 +247,10 @@ Debug options: ``-p pagesize`` Act as if the host page size was 'pagesize' bytes +``-one-insn-per-tb`` + Run the emulation with one guest instruction per translation block. + This slows down emulation a lot, but can be useful in some situations, + such as when trying to analyse the logs produced by the ``-d`` option. + ``-singlestep`` - Run the emulation in single step mode. + This is a deprecated synonym for the ``-one-insn-per-tb`` option. diff --git a/bsd-user/main.c b/bsd-user/main.c index a9e5a127d38..9d604a670b7 100644 --- a/bsd-user/main.c +++ b/bsd-user/main.c @@ -162,7 +162,8 @@ static void usage(void) "-d item1[,...] enable logging of specified items\n" " (use '-d help' for a list of log items)\n" "-D logfile write logs to 'logfile' (default stderr)\n" - "-singlestep always run in singlestep mode\n" + "-one-insn-per-tb run with one guest instruction per emulated TB\n" + "-singlestep deprecated synonym for -one-insn-per-tb\n" "-strace log system calls\n" "-trace [[enable=]<pattern>][,events=<file>][,file=<file>]\n" " specify tracing options\n" @@ -385,7 +386,7 @@ int main(int argc, char **argv) (void) envlist_unsetenv(envlist, "LD_PRELOAD"); } else if (!strcmp(r, "seed")) { seed_optarg = optarg; - } else if (!strcmp(r, "singlestep")) { + } else if (!strcmp(r, "singlestep") || !strcmp(r, "one-insn-per-tb") { opt_one_insn_per_tb = true; } else if (!strcmp(r, "strace")) { do_strace = 1; -- 2.34.1
On 4/3/23 07:46, Peter Maydell wrote:
The '-singlestep' option is confusing, because it doesn't actually have anything to do with single-stepping the CPU. What it does do is force TCG emulation to put one guest instruction in each TB, which can be useful in some situations.
Create a new command line argument -one-insn-per-tb, so we can document that -singlestep is just a deprecated synonym for it, and eventually perhaps drop it.
Signed-off-by: Peter Maydell<peter.maydell@linaro.org> --- NB: not even compile tested! --- docs/user/main.rst | 7 ++++++- bsd-user/main.c | 5 +++-- 2 files changed, 9 insertions(+), 3 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On Mon, Apr 3, 2023 at 8:46 AM Peter Maydell <peter.maydell@linaro.org> wrote:
The '-singlestep' option is confusing, because it doesn't actually have anything to do with single-stepping the CPU. What it does do is force TCG emulation to put one guest instruction in each TB, which can be useful in some situations.
Create a new command line argument -one-insn-per-tb, so we can document that -singlestep is just a deprecated synonym for it, and eventually perhaps drop it.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- NB: not even compile tested! ---
It looks good in theory. It may even compile. If ti does: Reviewed-by: Warner Losh <imp@bsdimp.com>
docs/user/main.rst | 7 ++++++- bsd-user/main.c | 5 +++-- 2 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/docs/user/main.rst b/docs/user/main.rst index f9ac701f4b1..f4786353965 100644 --- a/docs/user/main.rst +++ b/docs/user/main.rst @@ -247,5 +247,10 @@ Debug options: ``-p pagesize`` Act as if the host page size was 'pagesize' bytes
+``-one-insn-per-tb`` + Run the emulation with one guest instruction per translation block. + This slows down emulation a lot, but can be useful in some situations, + such as when trying to analyse the logs produced by the ``-d`` option. + ``-singlestep`` - Run the emulation in single step mode. + This is a deprecated synonym for the ``-one-insn-per-tb`` option. diff --git a/bsd-user/main.c b/bsd-user/main.c index a9e5a127d38..9d604a670b7 100644 --- a/bsd-user/main.c +++ b/bsd-user/main.c @@ -162,7 +162,8 @@ static void usage(void) "-d item1[,...] enable logging of specified items\n" " (use '-d help' for a list of log items)\n" "-D logfile write logs to 'logfile' (default stderr)\n" - "-singlestep always run in singlestep mode\n" + "-one-insn-per-tb run with one guest instruction per emulated TB\n" + "-singlestep deprecated synonym for -one-insn-per-tb\n" "-strace log system calls\n" "-trace [[enable=]<pattern>][,events=<file>][,file=<file>]\n" " specify tracing options\n" @@ -385,7 +386,7 @@ int main(int argc, char **argv) (void) envlist_unsetenv(envlist, "LD_PRELOAD"); } else if (!strcmp(r, "seed")) { seed_optarg = optarg; - } else if (!strcmp(r, "singlestep")) { + } else if (!strcmp(r, "singlestep") || !strcmp(r, "one-insn-per-tb") { opt_one_insn_per_tb = true; } else if (!strcmp(r, "strace")) { do_strace = 1; -- 2.34.1
Document that the -singlestep command line option is now deprecated, as it is replaced by either the TCG accelerator property 'one-insn-per-tb' for system emulation or the new '-one-insn-per-tb' option for usermode emulation, and remove the only use of the deprecated syntax from a README. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- docs/about/deprecated.rst | 16 ++++++++++++++++ qemu-options.hx | 5 +++-- tcg/tci/README | 2 +- 3 files changed, 20 insertions(+), 3 deletions(-) diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index 1ca9dc33d61..3c62671dac1 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -111,6 +111,22 @@ Use ``-machine acpi=off`` instead. The HAXM project has been retired (see https://github.com/intel/haxm#status). Use "whpx" (on Windows) or "hvf" (on macOS) instead. +``-singlestep`` (since 8.1) +''''''''''''''''''''''''''' + +The ``-singlestep`` option has been turned into an accelerator property, +and given a name that better reflects what it actually does. +Use ``-accel tcg,one-insn-per-tb=on`` instead. + +User-mode emulator command line arguments +----------------------------------------- + +``-singlestep`` (since 8.1) +''''''''''''''''''''''''''' + +The ``-singlestep`` option has been given a name that better reflects +what it actually does. For both linux-user and bsd-user, use the +new ``-one-insn-per-tb`` option instead. QEMU Machine Protocol (QMP) commands ------------------------------------ diff --git a/qemu-options.hx b/qemu-options.hx index 1dffd36884e..6a59e997497 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -4159,10 +4159,11 @@ SRST ERST DEF("singlestep", 0, QEMU_OPTION_singlestep, \ - "-singlestep always run in singlestep mode\n", QEMU_ARCH_ALL) + "-singlestep deprecated synonym for -accel tcg,one-insn-per-tb=on\n", QEMU_ARCH_ALL) SRST ``-singlestep`` - Run the emulation in single step mode. + This is a deprecated synonym for the TCG accelerator property + ``one-insn-per-tb``. ERST DEF("preconfig", 0, QEMU_OPTION_preconfig, \ diff --git a/tcg/tci/README b/tcg/tci/README index f72a40a395a..4a8b5b54018 100644 --- a/tcg/tci/README +++ b/tcg/tci/README @@ -49,7 +49,7 @@ The only difference from running QEMU with TCI to running without TCI should be speed. Especially during development of TCI, it was very useful to compare runs with and without TCI. Create /tmp/qemu.log by - qemu-system-i386 -d in_asm,op_opt,cpu -D /tmp/qemu.log -singlestep + qemu-system-i386 -d in_asm,op_opt,cpu -D /tmp/qemu.log -accel tcg,one-insn-per-tb=on once with interpreter and once without interpreter and compare the resulting qemu.log files. This is also useful to see the effects of additional -- 2.34.1
On 4/3/23 07:46, Peter Maydell wrote:
Document that the -singlestep command line option is now deprecated, as it is replaced by either the TCG accelerator property 'one-insn-per-tb' for system emulation or the new '-one-insn-per-tb' option for usermode emulation, and remove the only use of the deprecated syntax from a README.
Signed-off-by: Peter Maydell<peter.maydell@linaro.org> --- docs/about/deprecated.rst | 16 ++++++++++++++++ qemu-options.hx | 5 +++-- tcg/tci/README | 2 +- 3 files changed, 20 insertions(+), 3 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
The 'singlestep' HMP command is confusing, because it doesn't actually have anything to do with single-stepping the CPU. What it does do is force TCG emulation to put one guest instruction in each TB, which can be useful in some situations. Create a new HMP command 'one-insn-per-tb', so we can document that 'singlestep' is just a deprecated synonym for it, and eventually perhaps drop it. We aren't obliged to do deprecate-and-drop for HMP commands, but it's easy enough to do so, so we do. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- docs/about/deprecated.rst | 9 +++++++++ include/monitor/hmp.h | 2 +- softmmu/runstate-hmp-cmds.c | 2 +- tests/qtest/test-hmp.c | 1 + hmp-commands.hx | 25 +++++++++++++++++++++---- 5 files changed, 33 insertions(+), 6 deletions(-) diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index 3c62671dac1..6f5e689aa45 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -199,6 +199,15 @@ accepted incorrect commands will return an error. Users should make sure that all arguments passed to ``device_add`` are consistent with the documented property types. +Human Monitor Protocol (HMP) commands +------------------------------------- + +``singlestep`` (since 8.1) +'''''''''''''''''''''''''' + +The ``singlestep`` command has been replaced by the ``one-insn-per-tb`` +command, which has the same behaviour but a less misleading name. + Host Architectures ------------------ diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h index fdb69b7f9ca..13f9a2dedb8 100644 --- a/include/monitor/hmp.h +++ b/include/monitor/hmp.h @@ -158,7 +158,7 @@ void hmp_info_vcpu_dirty_limit(Monitor *mon, const QDict *qdict); void hmp_human_readable_text_helper(Monitor *mon, HumanReadableText *(*qmp_handler)(Error **)); void hmp_info_stats(Monitor *mon, const QDict *qdict); -void hmp_singlestep(Monitor *mon, const QDict *qdict); +void hmp_one_insn_per_tb(Monitor *mon, const QDict *qdict); void hmp_watchdog_action(Monitor *mon, const QDict *qdict); void hmp_pcie_aer_inject_error(Monitor *mon, const QDict *qdict); void hmp_info_capture(Monitor *mon, const QDict *qdict); diff --git a/softmmu/runstate-hmp-cmds.c b/softmmu/runstate-hmp-cmds.c index 127521a483a..76d1399ed85 100644 --- a/softmmu/runstate-hmp-cmds.c +++ b/softmmu/runstate-hmp-cmds.c @@ -41,7 +41,7 @@ void hmp_info_status(Monitor *mon, const QDict *qdict) qapi_free_StatusInfo(info); } -void hmp_singlestep(Monitor *mon, const QDict *qdict) +void hmp_one_insn_per_tb(Monitor *mon, const QDict *qdict) { const char *option = qdict_get_try_str(qdict, "option"); AccelState *accel = current_accel(); diff --git a/tests/qtest/test-hmp.c b/tests/qtest/test-hmp.c index b4a920df898..cb3530df722 100644 --- a/tests/qtest/test-hmp.c +++ b/tests/qtest/test-hmp.c @@ -64,6 +64,7 @@ static const char *hmp_cmds[] = { "screendump /dev/null", "sendkey x", "singlestep on", + "one-insn-per-tb on", "wavcapture /dev/null", "stopcapture 0", "sum 0 512", diff --git a/hmp-commands.hx b/hmp-commands.hx index bb85ee1d267..9afbb54a515 100644 --- a/hmp-commands.hx +++ b/hmp-commands.hx @@ -378,18 +378,35 @@ SRST only *tag* as parameter. ERST + { + .name = "one-insn-per-tb", + .args_type = "option:s?", + .params = "[on|off]", + .help = "run emulation with one guest instruction per translation block", + .cmd = hmp_one_insn_per_tb, + }, + +SRST +``one-insn-per-tb [off]`` + Run the emulation with one guest instruction per translation block. + This slows down emulation a lot, but can be useful in some situations, + such as when trying to analyse the logs produced by the ``-d`` option. + This only has an effect when using TCG, not with KVM or other accelerators. + + If called with option off, the emulation returns to normal mode. +ERST + { .name = "singlestep", .args_type = "option:s?", .params = "[on|off]", - .help = "run emulation in singlestep mode or switch to normal mode", - .cmd = hmp_singlestep, + .help = "deprecated synonym for one-insn-per-tb", + .cmd = hmp_one_insn_per_tb, }, SRST ``singlestep [off]`` - Run the emulation in single step mode. - If called with option off, the emulation returns to normal mode. + This is a deprecated synonym for the one-insn-per-tb command. ERST { -- 2.34.1
* Peter Maydell (peter.maydell@linaro.org) wrote:
The 'singlestep' HMP command is confusing, because it doesn't actually have anything to do with single-stepping the CPU. What it does do is force TCG emulation to put one guest instruction in each TB, which can be useful in some situations.
Create a new HMP command 'one-insn-per-tb', so we can document that 'singlestep' is just a deprecated synonym for it, and eventually perhaps drop it.
We aren't obliged to do deprecate-and-drop for HMP commands, but it's easy enough to do so, so we do.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- docs/about/deprecated.rst | 9 +++++++++ include/monitor/hmp.h | 2 +- softmmu/runstate-hmp-cmds.c | 2 +- tests/qtest/test-hmp.c | 1 + hmp-commands.hx | 25 +++++++++++++++++++++---- 5 files changed, 33 insertions(+), 6 deletions(-)
diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index 3c62671dac1..6f5e689aa45 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -199,6 +199,15 @@ accepted incorrect commands will return an error. Users should make sure that all arguments passed to ``device_add`` are consistent with the documented property types.
+Human Monitor Protocol (HMP) commands +------------------------------------- + +``singlestep`` (since 8.1) +'''''''''''''''''''''''''' + +The ``singlestep`` command has been replaced by the ``one-insn-per-tb`` +command, which has the same behaviour but a less misleading name. + Host Architectures ------------------
diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h index fdb69b7f9ca..13f9a2dedb8 100644 --- a/include/monitor/hmp.h +++ b/include/monitor/hmp.h @@ -158,7 +158,7 @@ void hmp_info_vcpu_dirty_limit(Monitor *mon, const QDict *qdict); void hmp_human_readable_text_helper(Monitor *mon, HumanReadableText *(*qmp_handler)(Error **)); void hmp_info_stats(Monitor *mon, const QDict *qdict); -void hmp_singlestep(Monitor *mon, const QDict *qdict); +void hmp_one_insn_per_tb(Monitor *mon, const QDict *qdict); void hmp_watchdog_action(Monitor *mon, const QDict *qdict); void hmp_pcie_aer_inject_error(Monitor *mon, const QDict *qdict); void hmp_info_capture(Monitor *mon, const QDict *qdict); diff --git a/softmmu/runstate-hmp-cmds.c b/softmmu/runstate-hmp-cmds.c index 127521a483a..76d1399ed85 100644 --- a/softmmu/runstate-hmp-cmds.c +++ b/softmmu/runstate-hmp-cmds.c @@ -41,7 +41,7 @@ void hmp_info_status(Monitor *mon, const QDict *qdict) qapi_free_StatusInfo(info); }
-void hmp_singlestep(Monitor *mon, const QDict *qdict) +void hmp_one_insn_per_tb(Monitor *mon, const QDict *qdict) { const char *option = qdict_get_try_str(qdict, "option"); AccelState *accel = current_accel(); diff --git a/tests/qtest/test-hmp.c b/tests/qtest/test-hmp.c index b4a920df898..cb3530df722 100644 --- a/tests/qtest/test-hmp.c +++ b/tests/qtest/test-hmp.c @@ -64,6 +64,7 @@ static const char *hmp_cmds[] = { "screendump /dev/null", "sendkey x", "singlestep on", + "one-insn-per-tb on",
OK, it wouldn't be bad if this list got a bit back into near alphabetic order. Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
"wavcapture /dev/null", "stopcapture 0", "sum 0 512", diff --git a/hmp-commands.hx b/hmp-commands.hx index bb85ee1d267..9afbb54a515 100644 --- a/hmp-commands.hx +++ b/hmp-commands.hx @@ -378,18 +378,35 @@ SRST only *tag* as parameter. ERST
+ { + .name = "one-insn-per-tb", + .args_type = "option:s?", + .params = "[on|off]", + .help = "run emulation with one guest instruction per translation block", + .cmd = hmp_one_insn_per_tb, + }, + +SRST +``one-insn-per-tb [off]`` + Run the emulation with one guest instruction per translation block. + This slows down emulation a lot, but can be useful in some situations, + such as when trying to analyse the logs produced by the ``-d`` option. + This only has an effect when using TCG, not with KVM or other accelerators. + + If called with option off, the emulation returns to normal mode. +ERST + { .name = "singlestep", .args_type = "option:s?", .params = "[on|off]", - .help = "run emulation in singlestep mode or switch to normal mode", - .cmd = hmp_singlestep, + .help = "deprecated synonym for one-insn-per-tb", + .cmd = hmp_one_insn_per_tb, },
SRST ``singlestep [off]`` - Run the emulation in single step mode. - If called with option off, the emulation returns to normal mode. + This is a deprecated synonym for the one-insn-per-tb command. ERST
{ -- 2.34.1
-- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 4/3/23 07:46, Peter Maydell wrote:
The 'singlestep' HMP command is confusing, because it doesn't actually have anything to do with single-stepping the CPU. What it does do is force TCG emulation to put one guest instruction in each TB, which can be useful in some situations.
Create a new HMP command 'one-insn-per-tb', so we can document that 'singlestep' is just a deprecated synonym for it, and eventually perhaps drop it.
We aren't obliged to do deprecate-and-drop for HMP commands, but it's easy enough to do so, so we do.
Signed-off-by: Peter Maydell<peter.maydell@linaro.org> --- docs/about/deprecated.rst | 9 +++++++++ include/monitor/hmp.h | 2 +- softmmu/runstate-hmp-cmds.c | 2 +- tests/qtest/test-hmp.c | 1 + hmp-commands.hx | 25 +++++++++++++++++++++---- 5 files changed, 33 insertions(+), 6 deletions(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
The HMP 'info status' output includes "(single step mode)" when we are running with TCG one-insn-per-tb enabled. Change this text to "(one insn per TB)" to match the new command line option names. We don't need to have a deprecation/transition plan for this, because we make no guarantees about stability of HMP output. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- softmmu/runstate-hmp-cmds.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/softmmu/runstate-hmp-cmds.c b/softmmu/runstate-hmp-cmds.c index 76d1399ed85..20facb055f0 100644 --- a/softmmu/runstate-hmp-cmds.c +++ b/softmmu/runstate-hmp-cmds.c @@ -30,7 +30,7 @@ void hmp_info_status(Monitor *mon, const QDict *qdict) monitor_printf(mon, "VM status: %s%s", info->running ? "running" : "paused", - info->singlestep ? " (single step mode)" : ""); + info->singlestep ? " (one insn per TB)" : ""); if (!info->running && info->status != RUN_STATE_PAUSED) { monitor_printf(mon, " (%s)", RunState_str(info->status)); -- 2.34.1
On 4/3/23 07:46, Peter Maydell wrote:
The HMP 'info status' output includes "(single step mode)" when we are running with TCG one-insn-per-tb enabled. Change this text to "(one insn per TB)" to match the new command line option names.
We don't need to have a deprecation/transition plan for this, because we make no guarantees about stability of HMP output.
Signed-off-by: Peter Maydell<peter.maydell@linaro.org> --- softmmu/runstate-hmp-cmds.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
The run-state.json file is missing a trailing newline; add it. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- Noticed this because my editor wanted to add the newline when I touched the file for the following patch... --- qapi/run-state.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qapi/run-state.json b/qapi/run-state.json index 419c188dd1a..9d34afa39e0 100644 --- a/qapi/run-state.json +++ b/qapi/run-state.json @@ -663,4 +663,4 @@ # Since: 7.2 ## { 'enum': 'NotifyVmexitOption', - 'data': [ 'run', 'internal-error', 'disable' ] } \ No newline at end of file + 'data': [ 'run', 'internal-error', 'disable' ] } -- 2.34.1
On 4/3/23 07:46, Peter Maydell wrote:
The run-state.json file is missing a trailing newline; add it.
Signed-off-by: Peter Maydell<peter.maydell@linaro.org> --- Noticed this because my editor wanted to add the newline when I touched the file for the following patch... --- qapi/run-state.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
The 'singlestep' member of StatusInfo has never done what the QMP documentation claims it does. What it actually reports is whether TCG is working in "one guest instruction per translation block" mode. Create a new 'one-insn-per-tb' member whose name matches what the field actually does and the new command line options. Deprecate the old 'singlestep' field. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- docs/about/deprecated.rst | 10 ++++++++++ docs/interop/qmp-intro.txt | 1 + qapi/run-state.json | 17 ++++++++++++++--- softmmu/runstate-hmp-cmds.c | 2 +- softmmu/runstate.c | 6 ++++-- 5 files changed, 30 insertions(+), 6 deletions(-) diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index 6f5e689aa45..dd36becdf3b 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -199,6 +199,16 @@ accepted incorrect commands will return an error. Users should make sure that all arguments passed to ``device_add`` are consistent with the documented property types. +``StatusInfo`` member ``singlestep`` (since 8.1) +'''''''''''''''''''''''''''''''''''''''''''''''' + +The ``singlestep`` member of the ``StatusInfo`` returned from +the ``query-status`` command is deprecated, because its name +is confusing and it never did what the documentation claimed +or what its name suggests. Use the ``one-insn-per-tb`` +member instead, which reports the same information the old +``singlestep`` member did but under a clearer name. + Human Monitor Protocol (HMP) commands ------------------------------------- diff --git a/docs/interop/qmp-intro.txt b/docs/interop/qmp-intro.txt index 1c745a7af04..b22916b23df 100644 --- a/docs/interop/qmp-intro.txt +++ b/docs/interop/qmp-intro.txt @@ -73,6 +73,7 @@ Escape character is '^]'. { "execute": "query-status" } { "return": { + "one-insn-per-tb": false, "status": "prelaunch", "singlestep": false, "running": false diff --git a/qapi/run-state.json b/qapi/run-state.json index 9d34afa39e0..1de8c5c55d0 100644 --- a/qapi/run-state.json +++ b/qapi/run-state.json @@ -104,16 +104,27 @@ # # @running: true if all VCPUs are runnable, false if not runnable # -# @singlestep: true if VCPUs are in single-step mode +# @one-insn-per-tb: true if using TCG with one guest instruction +# per translation block +# +# @singlestep: deprecated synonym for @one-insn-per-tb # # @status: the virtual machine @RunState # +# Features: +# @deprecated: Member 'singlestep' is deprecated. Use @one-insn-per-tb instead. +# # Since: 0.14 # -# Notes: @singlestep is enabled through the GDB stub +# Notes: @one-insn-per-tb is enabled on the command line with +# '-accel tcg,one-insn-per-tb=on', or with the HMP +# 'one-insn-per-tb' command. ## { 'struct': 'StatusInfo', - 'data': {'running': 'bool', 'singlestep': 'bool', 'status': 'RunState'} } + 'data': {'running': 'bool', + 'singlestep': { 'type': 'bool', 'features': [ 'deprecated' ]}, + 'one-insn-per-tb': 'bool', + 'status': 'RunState'} } ## # @query-status: diff --git a/softmmu/runstate-hmp-cmds.c b/softmmu/runstate-hmp-cmds.c index 20facb055f0..404d331b523 100644 --- a/softmmu/runstate-hmp-cmds.c +++ b/softmmu/runstate-hmp-cmds.c @@ -30,7 +30,7 @@ void hmp_info_status(Monitor *mon, const QDict *qdict) monitor_printf(mon, "VM status: %s%s", info->running ? "running" : "paused", - info->singlestep ? " (one insn per TB)" : ""); + info->one_insn_per_tb ? " (one insn per TB)" : ""); if (!info->running && info->status != RUN_STATE_PAUSED) { monitor_printf(mon, " (%s)", RunState_str(info->status)); diff --git a/softmmu/runstate.c b/softmmu/runstate.c index 2f2396c819e..a93e74c41ca 100644 --- a/softmmu/runstate.c +++ b/softmmu/runstate.c @@ -239,11 +239,13 @@ StatusInfo *qmp_query_status(Error **errp) /* * We ignore errors, which will happen if the accelerator - * is not TCG. "singlestep" is meaningless for other accelerators, + * is not TCG. "one-insn-per-tb" is meaningless for other accelerators, * so we will set the StatusInfo field to false for those. */ - info->singlestep = object_property_get_bool(OBJECT(accel), + info->one_insn_per_tb = object_property_get_bool(OBJECT(accel), "one-insn-per-tb", NULL); + /* Deprecated member with same meaning as one-insn-per-tb */ + info->singlestep = info->one_insn_per_tb; info->running = runstate_is_running(); info->status = current_run_state; -- 2.34.1
In the title: "qmp:" Peter Maydell <peter.maydell@linaro.org> writes:
The 'singlestep' member of StatusInfo has never done what the QMP documentation claims it does. What it actually reports is whether TCG is working in "one guest instruction per translation block" mode.
Create a new 'one-insn-per-tb' member whose name matches what the field actually does and the new command line options. Deprecate the old 'singlestep' field.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- docs/about/deprecated.rst | 10 ++++++++++ docs/interop/qmp-intro.txt | 1 + qapi/run-state.json | 17 ++++++++++++++--- softmmu/runstate-hmp-cmds.c | 2 +- softmmu/runstate.c | 6 ++++-- 5 files changed, 30 insertions(+), 6 deletions(-)
diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index 6f5e689aa45..dd36becdf3b 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -199,6 +199,16 @@ accepted incorrect commands will return an error. Users should make sure that all arguments passed to ``device_add`` are consistent with the documented property types.
+``StatusInfo`` member ``singlestep`` (since 8.1) +'''''''''''''''''''''''''''''''''''''''''''''''' + +The ``singlestep`` member of the ``StatusInfo`` returned from +the ``query-status`` command is deprecated, because its name +is confusing and it never did what the documentation claimed +or what its name suggests. Use the ``one-insn-per-tb`` +member instead, which reports the same information the old +``singlestep`` member did but under a clearer name. + Human Monitor Protocol (HMP) commands -------------------------------------
diff --git a/docs/interop/qmp-intro.txt b/docs/interop/qmp-intro.txt index 1c745a7af04..b22916b23df 100644 --- a/docs/interop/qmp-intro.txt +++ b/docs/interop/qmp-intro.txt @@ -73,6 +73,7 @@ Escape character is '^]'. { "execute": "query-status" } { "return": { + "one-insn-per-tb": false, "status": "prelaunch", "singlestep": false, "running": false diff --git a/qapi/run-state.json b/qapi/run-state.json index 9d34afa39e0..1de8c5c55d0 100644 --- a/qapi/run-state.json +++ b/qapi/run-state.json @@ -104,16 +104,27 @@ # # @running: true if all VCPUs are runnable, false if not runnable # -# @singlestep: true if VCPUs are in single-step mode +# @one-insn-per-tb: true if using TCG with one guest instruction +# per translation block +# +# @singlestep: deprecated synonym for @one-insn-per-tb # # @status: the virtual machine @RunState # +# Features: +# @deprecated: Member 'singlestep' is deprecated. Use @one-insn-per-tb instead.
Wrap this line, please.
+# # Since: 0.14 # -# Notes: @singlestep is enabled through the GDB stub +# Notes: @one-insn-per-tb is enabled on the command line with +# '-accel tcg,one-insn-per-tb=on', or with the HMP +# 'one-insn-per-tb' command. ##
Hmm. We report it in query-status, which means it's relevant to QMP clients. We provide the command to control it only in HMP, which means it's not relevant to QMP clients. Why is reading it relevant to QMP clients, but not writing? Use cases for reading it via QMP query-status? Have you considered tacking feature 'unstable' to it?
{ 'struct': 'StatusInfo', - 'data': {'running': 'bool', 'singlestep': 'bool', 'status': 'RunState'} } + 'data': {'running': 'bool', + 'singlestep': { 'type': 'bool', 'features': [ 'deprecated' ]}, + 'one-insn-per-tb': 'bool', + 'status': 'RunState'} }
## # @query-status:
I see a bunch of query-status results in tests/qemu-iotests/{183,234,262,280}.out. Do they need an update? [...]
On Tue, 4 Apr 2023 at 09:25, Markus Armbruster <armbru@redhat.com> wrote:
In the title: "qmp:"
Peter Maydell <peter.maydell@linaro.org> writes:
diff --git a/qapi/run-state.json b/qapi/run-state.json index 9d34afa39e0..1de8c5c55d0 100644 --- a/qapi/run-state.json +++ b/qapi/run-state.json @@ -104,16 +104,27 @@ # # @running: true if all VCPUs are runnable, false if not runnable # -# @singlestep: true if VCPUs are in single-step mode +# @one-insn-per-tb: true if using TCG with one guest instruction +# per translation block +# +# @singlestep: deprecated synonym for @one-insn-per-tb # # @status: the virtual machine @RunState # +# Features: +# @deprecated: Member 'singlestep' is deprecated. Use @one-insn-per-tb instead.
Wrap this line, please.
+# # Since: 0.14 # -# Notes: @singlestep is enabled through the GDB stub +# Notes: @one-insn-per-tb is enabled on the command line with +# '-accel tcg,one-insn-per-tb=on', or with the HMP +# 'one-insn-per-tb' command. ##
Hmm. We report it in query-status, which means it's relevant to QMP clients. We provide the command to control it only in HMP, which means it's not relevant to QMP clients.
Why is reading it relevant to QMP clients, but not writing?
I suspect that neither is very relevant to QMP clients, but I thought we had a requirement that HMP interfaces went via QMP ones ? If not, we could just make the HMP query interface directly look at the TCG property, the way the write interface does. I don't want to add a QMP interface for writing it unless there's somebody who actually has a use case for it.
Use cases for reading it via QMP query-status?
Have you considered tacking feature 'unstable' to it?
Nope, because I don't know anything about what that does :-)
{ 'struct': 'StatusInfo', - 'data': {'running': 'bool', 'singlestep': 'bool', 'status': 'RunState'} } + 'data': {'running': 'bool', + 'singlestep': { 'type': 'bool', 'features': [ 'deprecated' ]}, + 'one-insn-per-tb': 'bool', + 'status': 'RunState'} }
## # @query-status:
I see a bunch of query-status results in tests/qemu-iotests/{183,234,262,280}.out. Do they need an update?
"make check" passed, so I guess not, unless those don't run in "make check"... Do those .out files need exact matching output, or can they be written to say "we don't care about what value this field has or whether it exists" ? thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes:
On Tue, 4 Apr 2023 at 09:25, Markus Armbruster <armbru@redhat.com> wrote:
In the title: "qmp:"
Peter Maydell <peter.maydell@linaro.org> writes:
diff --git a/qapi/run-state.json b/qapi/run-state.json index 9d34afa39e0..1de8c5c55d0 100644 --- a/qapi/run-state.json +++ b/qapi/run-state.json @@ -104,16 +104,27 @@ # # @running: true if all VCPUs are runnable, false if not runnable # -# @singlestep: true if VCPUs are in single-step mode +# @one-insn-per-tb: true if using TCG with one guest instruction +# per translation block +# +# @singlestep: deprecated synonym for @one-insn-per-tb # # @status: the virtual machine @RunState # +# Features: +# @deprecated: Member 'singlestep' is deprecated. Use @one-insn-per-tb instead.
Wrap this line, please.
+# # Since: 0.14 # -# Notes: @singlestep is enabled through the GDB stub +# Notes: @one-insn-per-tb is enabled on the command line with +# '-accel tcg,one-insn-per-tb=on', or with the HMP +# 'one-insn-per-tb' command. ##
Hmm. We report it in query-status, which means it's relevant to QMP clients. We provide the command to control it only in HMP, which means it's not relevant to QMP clients.
Why is reading it relevant to QMP clients, but not writing?
I suspect that neither is very relevant to QMP clients, but I thought we had a requirement that HMP interfaces went via QMP ones ?
Kind of. Here's my current boilerplate on the subject: HMP commands without a QMP equivalent are okay if their functionality makes no sense in QMP, or is of use only for human users. Example for "makes no sense in QMP": setting the current CPU, because a QMP monitor doesn't have a current CPU. Examples for "is of use only for human users": HMP command "help", the integrated pocket calculator. Debugging commands are kind of borderline. Debugging is commonly a human activity, where HMP is just fine. However, humans create tools to assist with their activities, and then QMP is useful. While I wouldn't encourage HMP-only for the debugging use case, I wouldn't veto it. When adding an HMP-only command, explain why it is HMP-only in the commit message.
If not, we could just make the HMP query interface directly look at the TCG property, the way the write interface does.
How useful is it HMP?
I don't want to add a QMP interface for writing it unless there's somebody who actually has a use case for it.
Use cases for reading it via QMP query-status?
Have you considered tacking feature 'unstable' to it?
Nope, because I don't know anything about what that does :-)
docs/devel/qapi-code-gen.rst explains: Special features ~~~~~~~~~~~~~~~~ Feature "deprecated" marks a command, event, enum value, or struct member as deprecated. It is not supported elsewhere so far. Interfaces so marked may be withdrawn in future releases in accordance with QEMU's deprecation policy. Feature "unstable" marks a command, event, enum value, or struct member as unstable. It is not supported elsewhere so far. Interfaces so marked may be withdrawn or changed incompatibly in future releases. We use "unstable" for debugging aids, testing aids, experiments / unfinished work.
{ 'struct': 'StatusInfo', - 'data': {'running': 'bool', 'singlestep': 'bool', 'status': 'RunState'} } + 'data': {'running': 'bool', + 'singlestep': { 'type': 'bool', 'features': [ 'deprecated' ]}, + 'one-insn-per-tb': 'bool', + 'status': 'RunState'} }
## # @query-status:
I see a bunch of query-status results in tests/qemu-iotests/{183,234,262,280}.out. Do they need an update?
"make check" passed, so I guess not, unless those don't run in "make check"...
Plenty of iotests don't run in "make check". Try $ tests/qemu-iotests/check -qcow2 183 234 262 280
Do those .out files need exact matching output, or can they be written to say "we don't care about what value this field has or whether it exists" ?
If (hazy) memory serves, there's some normalization. I doubt it'll affect this member, i.e. you need to put it there.
On Tue, 4 Apr 2023 at 14:25, Markus Armbruster <armbru@redhat.com> wrote:
Peter Maydell <peter.maydell@linaro.org> writes:
On Tue, 4 Apr 2023 at 09:25, Markus Armbruster <armbru@redhat.com> wrote:
Hmm. We report it in query-status, which means it's relevant to QMP clients. We provide the command to control it only in HMP, which means it's not relevant to QMP clients.
Why is reading it relevant to QMP clients, but not writing?
I suspect that neither is very relevant to QMP clients, but I thought we had a requirement that HMP interfaces went via QMP ones ?
Kind of. Here's my current boilerplate on the subject:
HMP commands without a QMP equivalent are okay if their functionality makes no sense in QMP, or is of use only for human users.
Example for "makes no sense in QMP": setting the current CPU, because a QMP monitor doesn't have a current CPU.
Examples for "is of use only for human users": HMP command "help", the integrated pocket calculator.
Debugging commands are kind of borderline. Debugging is commonly a human activity, where HMP is just fine. However, humans create tools to assist with their activities, and then QMP is useful. While I wouldn't encourage HMP-only for the debugging use case, I wouldn't veto it.
When adding an HMP-only command, explain why it is HMP-only in the commit message.
If not, we could just make the HMP query interface directly look at the TCG property, the way the write interface does.
How useful is it HMP?
Well, as usual, we have no idea if anybody really uses any feature. I've never used it myself but I have a vague recollection of reading list mail once from somebody who used it. You can construct theoretical scenarios where it might be nice (eg "boot guest OS quickly and then turn on the one-insn-per-tb mode once you get to the point of interest"), I guess. These theoretical scenarios are equally valid (or esoteric) whether you're trying to control QEMU via QMP or HMP. I think on balance I would go for: * remove (ie deprecate-and-drop) 'singlestep' from the QMP struct, rather than merely renaming it * if anybody comes along and says they want to do this via QMP, implement Paolo's idea of putting the accelerator object somewhere they can get at it and use qom-get/qom-set on it [My guess is this is very unlikely: nobody's complained so far that QMP doesn't permit setting 'singlestep'; and wanting read without write seems even more marginal.] * keep the HMP commands, but have both read and write directly talk to the accel object. I favour splitting the 'read' part out into its own 'info one-insn-per-tb', for consistency (then 'info status' matches the QMP query-status) In particular, the fact that messing with this obscure debug functionality requires updating the reference-output for a bunch of io tests that have no interest at all in it rather suggests that even if we did want to expose this to QMP that the query-status command is the wrong place to do it. thanks -- PMM
On 4/4/23 16:24, Peter Maydell wrote:
I think on balance I would go for: * remove (ie deprecate-and-drop) 'singlestep' from the QMP struct, rather than merely renaming it * if anybody comes along and says they want to do this via QMP, implement Paolo's idea of putting the accelerator object somewhere they can get at it and use qom-get/qom-set on it [My guess is this is very unlikely: nobody's complained so far that QMP doesn't permit setting 'singlestep'; and wanting read without write seems even more marginal.] * keep the HMP commands, but have both read and write directly talk to the accel object. I favour splitting the 'read' part out into its own 'info one-insn-per-tb', for consistency (then 'info status' matches the QMP query-status)
I think the read part could be added to 'info jit'. Paolo
* Peter Maydell (peter.maydell@linaro.org) wrote:
On Tue, 4 Apr 2023 at 14:25, Markus Armbruster <armbru@redhat.com> wrote:
Peter Maydell <peter.maydell@linaro.org> writes:
On Tue, 4 Apr 2023 at 09:25, Markus Armbruster <armbru@redhat.com> wrote:
Hmm. We report it in query-status, which means it's relevant to QMP clients. We provide the command to control it only in HMP, which means it's not relevant to QMP clients.
Why is reading it relevant to QMP clients, but not writing?
I suspect that neither is very relevant to QMP clients, but I thought we had a requirement that HMP interfaces went via QMP ones ?
Kind of. Here's my current boilerplate on the subject:
HMP commands without a QMP equivalent are okay if their functionality makes no sense in QMP, or is of use only for human users.
Example for "makes no sense in QMP": setting the current CPU, because a QMP monitor doesn't have a current CPU.
Examples for "is of use only for human users": HMP command "help", the integrated pocket calculator.
Debugging commands are kind of borderline. Debugging is commonly a human activity, where HMP is just fine. However, humans create tools to assist with their activities, and then QMP is useful. While I wouldn't encourage HMP-only for the debugging use case, I wouldn't veto it.
When adding an HMP-only command, explain why it is HMP-only in the commit message.
If not, we could just make the HMP query interface directly look at the TCG property, the way the write interface does.
How useful is it HMP?
Well, as usual, we have no idea if anybody really uses any feature. I've never used it myself but I have a vague recollection of reading list mail once from somebody who used it. You can construct theoretical scenarios where it might be nice (eg "boot guest OS quickly and then turn on the one-insn-per-tb mode once you get to the point of interest"), I guess. These theoretical scenarios are equally valid (or esoteric) whether you're trying to control QEMU via QMP or HMP.
I think on balance I would go for: * remove (ie deprecate-and-drop) 'singlestep' from the QMP struct, rather than merely renaming it * if anybody comes along and says they want to do this via QMP, implement Paolo's idea of putting the accelerator object somewhere they can get at it and use qom-get/qom-set on it [My guess is this is very unlikely: nobody's complained so far that QMP doesn't permit setting 'singlestep'; and wanting read without write seems even more marginal.] * keep the HMP commands, but have both read and write directly talk to the accel object. I favour splitting the 'read' part out into its own 'info one-insn-per-tb', for consistency (then 'info status' matches the QMP query-status)
If it's pretty obscure, then the qom-set/get is fine; as long as there is a way to do it, then just make sure in the commit message you say what the replacement command is. Dave
In particular, the fact that messing with this obscure debug functionality requires updating the reference-output for a bunch of io tests that have no interest at all in it rather suggests that even if we did want to expose this to QMP that the query-status command is the wrong place to do it.
thanks -- PMM
-- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Wed, 5 Apr 2023 at 15:56, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:
* Peter Maydell (peter.maydell@linaro.org) wrote:
I think on balance I would go for: * remove (ie deprecate-and-drop) 'singlestep' from the QMP struct, rather than merely renaming it * if anybody comes along and says they want to do this via QMP, implement Paolo's idea of putting the accelerator object somewhere they can get at it and use qom-get/qom-set on it [My guess is this is very unlikely: nobody's complained so far that QMP doesn't permit setting 'singlestep'; and wanting read without write seems even more marginal.] * keep the HMP commands, but have both read and write directly talk to the accel object. I favour splitting the 'read' part out into its own 'info one-insn-per-tb', for consistency (then 'info status' matches the QMP query-status)
If it's pretty obscure, then the qom-set/get is fine; as long as there is a way to do it, then just make sure in the commit message you say what the replacement command is
The point is that there isn't a replacement way to do it *right now*, but that we have a sketch of how we'd do it if anybody showed up and really cared about it. I think the chances of that happening are quite close to zero, so I don't want to do the work to actually implement the mechanism on spec... -- PMM
* Peter Maydell (peter.maydell@linaro.org) wrote:
On Wed, 5 Apr 2023 at 15:56, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:
* Peter Maydell (peter.maydell@linaro.org) wrote:
I think on balance I would go for: * remove (ie deprecate-and-drop) 'singlestep' from the QMP struct, rather than merely renaming it * if anybody comes along and says they want to do this via QMP, implement Paolo's idea of putting the accelerator object somewhere they can get at it and use qom-get/qom-set on it [My guess is this is very unlikely: nobody's complained so far that QMP doesn't permit setting 'singlestep'; and wanting read without write seems even more marginal.] * keep the HMP commands, but have both read and write directly talk to the accel object. I favour splitting the 'read' part out into its own 'info one-insn-per-tb', for consistency (then 'info status' matches the QMP query-status)
If it's pretty obscure, then the qom-set/get is fine; as long as there is a way to do it, then just make sure in the commit message you say what the replacement command is
The point is that there isn't a replacement way to do it *right now*, but that we have a sketch of how we'd do it if anybody showed up and really cared about it. I think the chances of that happening are quite close to zero, so I don't want to do the work to actually implement the mechanism on spec...
Sure, then just drop it. Dave
-- PMM
-- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 4/4/23 11:17, Peter Maydell wrote:
I don't want to add a QMP interface for writing it unless there's somebody who actually has a use case for it.
We could place the accelerator at a well-known QOM path such as /machine/accel, and then qom-set can already be used to implement such an interface. Not that you have to do it---just an argument against adding a QMP version of singlestep/one-insn-per-tb. Paolo
On 4/3/23 07:46, Peter Maydell wrote:
* I have written patch 3 on the assumption that curr_cflags() is not such a hot codepath that we can't afford to have a QOM cast macro in it; the alternative would be to keep it using a global variable but make the global be restricted to accel/tcg/internals.h. RTH: opinions welcome...
curr_cflags() is quite hot, called from lookup_tb_ptr every time we time we end a chain of directly linked TBs. You'll see lookup_tb_ptr near the top of any tcg profile. With a global variable, it might be worth combining with CPU_LOG_TB_NOCHAIN, recomputing the global if either option changes. r~
participants (6)
-
Dr. David Alan Gilbert -
Markus Armbruster -
Paolo Bonzini -
Peter Maydell -
Richard Henderson -
Warner Losh