[PATCH v2 00/10] Deprecate/rename singlestep command line option, monitor interfaces

[PATCH v2 00/10] Deprecate/rename singlestep command line option, monitor interfaces

[Peter Maydell]

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

[PATCH v2 01/10] make one-insn-per-tb an accel option

[Peter Maydell]

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

Re: [PATCH v2 01/10] make one-insn-per-tb an accel option

[Richard Henderson]

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~

[PATCH v2 02/10] softmmu: Don't use 'singlestep' global in QMP and HMP commands

[Peter Maydell]

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

Re: [PATCH v2 02/10] softmmu: Don't use 'singlestep' global in QMP and HMP commands

[Richard Henderson]

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~

[PATCH v2 03/10] tcg: Use one-insn-per-tb accelerator property in curr_cflags()

[Peter Maydell]

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

Re: [PATCH v2 03/10] tcg: Use one-insn-per-tb accelerator property in curr_cflags()

[Richard Henderson]

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~

Re: [PATCH v2 03/10] tcg: Use one-insn-per-tb accelerator property in curr_cflags()

[Peter Maydell]

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

Re: [PATCH v2 03/10] tcg: Use one-insn-per-tb accelerator property in curr_cflags()

[Richard Henderson]

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~

[PATCH v2 04/10] linux-user: Add '-one-insn-per-tb' option equivalent to '-singlestep'

[Peter Maydell]

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

Re: [PATCH v2 04/10] linux-user: Add '-one-insn-per-tb' option equivalent to '-singlestep'

[Richard Henderson]

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~

Re: [PATCH v2 04/10] linux-user: Add '-one-insn-per-tb' option equivalent to '-singlestep'

[Warner Losh]

[-- Attachment #1: Type: text/plain, Size: 909 bytes --]

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~
>

[-- Attachment #2: Type: text/html, Size: 1665 bytes --]

[PATCH v2 05/10] bsd-user: Add '-one-insn-per-tb' option equivalent to '-singlestep'

[Peter Maydell]

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

Re: [PATCH v2 05/10] bsd-user: Add '-one-insn-per-tb' option equivalent to '-singlestep'

[Richard Henderson]

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~

Re: [PATCH v2 05/10] bsd-user: Add '-one-insn-per-tb' option equivalent to '-singlestep'

[Warner Losh]

[-- Attachment #1: Type: text/plain, Size: 2846 bytes --]

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
>
>

[-- Attachment #2: Type: text/html, Size: 3952 bytes --]

[PATCH v2 06/10] Document that -singlestep command line option is deprecated

[Peter Maydell]

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

Re: [PATCH v2 06/10] Document that -singlestep command line option is deprecated

[Richard Henderson]

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~

[PATCH v2 07/10] hmp: Add 'one-insn-per-tb' command equivalent to 'singlestep'

[Peter Maydell]

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

Re: [PATCH v2 07/10] hmp: Add 'one-insn-per-tb' command equivalent to 'singlestep'

[Dr. David Alan Gilbert]

* 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

Re: [PATCH v2 07/10] hmp: Add 'one-insn-per-tb' command equivalent to 'singlestep'

[Richard Henderson]

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~

[PATCH v2 08/10] hmp: Report 'one-insn-per-tb', not 'single step mode', in 'info status' output

[Peter Maydell]

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

Re: [PATCH v2 08/10] hmp: Report 'one-insn-per-tb', not 'single step mode', in 'info status' output

[Richard Henderson]

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~

[PATCH v2 09/10] qapi/run-state.json: Fix missing newline at end of file

[Peter Maydell]

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

Re: [PATCH v2 09/10] qapi/run-state.json: Fix missing newline at end of file

[Richard Henderson]

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~

[PATCH v2 10/10] hmp: Deprecate 'singlestep' member of StatusInfo

[Peter Maydell]

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

Re: [PATCH v2 10/10] hmp: Deprecate 'singlestep' member of StatusInfo

[Markus Armbruster]

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?

[...]

Re: [PATCH v2 10/10] hmp: Deprecate 'singlestep' member of StatusInfo

[Peter Maydell]

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

Re: [PATCH v2 10/10] hmp: Deprecate 'singlestep' member of StatusInfo

[Markus Armbruster]

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.

Re: [PATCH v2 10/10] hmp: Deprecate 'singlestep' member of StatusInfo

[Peter Maydell]

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

Re: [PATCH v2 10/10] hmp: Deprecate 'singlestep' member of StatusInfo

[Paolo Bonzini]

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

Re: [PATCH v2 10/10] hmp: Deprecate 'singlestep' member of StatusInfo

[Dr. David Alan Gilbert]

* 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

Re: [PATCH v2 10/10] hmp: Deprecate 'singlestep' member of StatusInfo

[Peter Maydell]

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

Re: [PATCH v2 10/10] hmp: Deprecate 'singlestep' member of StatusInfo

[Dr. David Alan Gilbert]

* 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

Re: [PATCH v2 10/10] hmp: Deprecate 'singlestep' member of StatusInfo

[Paolo Bonzini]

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

Re: [PATCH v2 00/10] Deprecate/rename singlestep command line option, monitor interfaces

[Richard Henderson]

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~