[RFC PATCH 0/5] Deprecate/rename singlestep command line option

[RFC PATCH 0/5] Deprecate/rename singlestep command line option

[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:
 * renames the 'singlestep' global variable to 'one_insn_per_tb'
 * Adds new '-one-insn-per-tb' command line options and a
   HMP 'one-insn-per-tb' command
 * Documents the '-singlestep' options and 'singlestep'
   HMP command as deprecated synonyms for the new ones

It does not do anything about the other place where we surface
'singlestep', which is in the QMP StatusInfo object returned by the
'query-status' command.  This is incorrectly documented as "true if
VCPUs are in single-step mode" and "singlestep is enabled through
the GDB stub", because what it's actually returning is the
one-insn-per-tb state.

Things I didn't bother with because this is only an RFC but
will do if it turns into a non-RFC patchset:
 * test the bsd-user changes :-)
 * add text to deprecated.rst

So, questions:

(1) is this worth bothering with at all? We could just
    name our global variable etc better, and document what
    -singlestep actually does, and not bother with the new
    names for the options/commands.
(2) if we do do it, do we retain the old name indefinitely,
    or actively put it on the deprecate-and-drop list?
(3) what should we do about the HMP StatusInfo object?
    I'm not sure how we handle compatibility for HMP.

thanks
-- PMM


Peter Maydell (5):
  Rename the singlestep global variable to one_insn_per_tb
  linux-user: Add '-one-insn-per-tb' option equivalent to '-singlestep'
  bsd-user: Add '-one-insn-per-tb' option equivalent to '-singlestep'
  softmmu: Add '-one-insn-per-tb' option equivalent to '-singlestep'
  hmp: Add 'one-insn-per-tb' command equivalent to 'singlestep'

 docs/user/main.rst          | 14 ++++++++++++--
 include/exec/cpu-common.h   |  2 +-
 include/monitor/hmp.h       |  2 +-
 accel/tcg/cpu-exec.c        |  4 ++--
 bsd-user/main.c             |  9 +++++----
 linux-user/main.c           | 13 ++++++++-----
 softmmu/globals.c           |  2 +-
 softmmu/runstate-hmp-cmds.c |  6 +++---
 softmmu/runstate.c          |  2 +-
 softmmu/vl.c                |  3 ++-
 tests/qtest/test-hmp.c      |  1 +
 hmp-commands.hx             | 25 +++++++++++++++++++++----
 qemu-options.hx             | 14 ++++++++++++--
 tcg/tci/README              |  2 +-
 14 files changed, 71 insertions(+), 28 deletions(-)

-- 
2.34.1

[RFC PATCH 1/5] Rename the singlestep global variable to one_insn_per_tb

[Peter Maydell]

The 'singlestep' global variable is badly misnamed, because it has
nothing to do with single-stepping the emulation either via the gdb
stub or by emulation of architectural debug facilities.  Instead what
it does is force TCG to put only one instruction into each TB.
Rename it to one_insn_per_tb, so that it reflects what it does better
and is easier to search for in the codebase.

This misnaming is also present in user-facing interfaces: the command
line option '-singlestep', the HMP 'singlestep' command, and the QMP
StatusInfo object.  Those are harder to update because we need to
retain backwards compatibility.  Subsequent patches will add
better-named aliases so we can eventually deprecate-and-drop the old,
bad name.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/exec/cpu-common.h   | 2 +-
 accel/tcg/cpu-exec.c        | 4 ++--
 bsd-user/main.c             | 4 ++--
 linux-user/main.c           | 4 ++--
 softmmu/globals.c           | 2 +-
 softmmu/runstate-hmp-cmds.c | 4 ++--
 softmmu/runstate.c          | 2 +-
 softmmu/vl.c                | 2 +-
 8 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
index 6feaa40ca7b..f2592a1967f 100644
--- a/include/exec/cpu-common.h
+++ b/include/exec/cpu-common.h
@@ -163,7 +163,7 @@ int cpu_memory_rw_debug(CPUState *cpu, vaddr addr,
                         void *ptr, size_t len, bool is_write);
 
 /* vl.c */
-extern int singlestep;
+extern int one_insn_per_tb;
 
 void list_cpus(const char *optarg);
 
diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index 9c857eeb077..08a65f8d506 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -157,12 +157,12 @@ uint32_t curr_cflags(CPUState *cpu)
      * 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 (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/bsd-user/main.c b/bsd-user/main.c
index 6f09180d654..a8de6906ed5 100644
--- a/bsd-user/main.c
+++ b/bsd-user/main.c
@@ -50,7 +50,7 @@
 #include "host-os.h"
 #include "target_arch_cpu.h"
 
-int singlestep;
+int one_insn_per_tb;
 uintptr_t guest_base;
 bool have_guest_base;
 /*
@@ -391,7 +391,7 @@ int main(int argc, char **argv)
         } else if (!strcmp(r, "seed")) {
             seed_optarg = optarg;
         } else if (!strcmp(r, "singlestep")) {
-            singlestep = 1;
+            one_insn_per_tb = 1;
         } else if (!strcmp(r, "strace")) {
             do_strace = 1;
         } else if (!strcmp(r, "trace")) {
diff --git a/linux-user/main.c b/linux-user/main.c
index 4290651c3cf..99bcd542b42 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -66,7 +66,7 @@
 
 char *exec_path;
 
-int singlestep;
+int one_insn_per_tb;
 static const char *argv0;
 static const char *gdbstub;
 static envlist_t *envlist;
@@ -397,7 +397,7 @@ static void handle_arg_reserved_va(const char *arg)
 
 static void handle_arg_singlestep(const char *arg)
 {
-    singlestep = 1;
+    one_insn_per_tb = 1;
 }
 
 static void handle_arg_strace(const char *arg)
diff --git a/softmmu/globals.c b/softmmu/globals.c
index 527edbefdd0..f46df89d2db 100644
--- a/softmmu/globals.c
+++ b/softmmu/globals.c
@@ -43,7 +43,7 @@ int vga_interface_type = VGA_NONE;
 bool vga_interface_created;
 Chardev *parallel_hds[MAX_PARALLEL_PORTS];
 int win2k_install_hack;
-int singlestep;
+int one_insn_per_tb;
 int fd_bootchk = 1;
 int graphic_rotate;
 QEMUOptionRom option_rom[MAX_OPTION_ROMS];
diff --git a/softmmu/runstate-hmp-cmds.c b/softmmu/runstate-hmp-cmds.c
index d55a7d4db89..29c9a038863 100644
--- a/softmmu/runstate-hmp-cmds.c
+++ b/softmmu/runstate-hmp-cmds.c
@@ -44,9 +44,9 @@ void hmp_singlestep(Monitor *mon, const QDict *qdict)
 {
     const char *option = qdict_get_try_str(qdict, "option");
     if (!option || !strcmp(option, "on")) {
-        singlestep = 1;
+        one_insn_per_tb = 1;
     } else if (!strcmp(option, "off")) {
-        singlestep = 0;
+        one_insn_per_tb = 0;
     } else {
         monitor_printf(mon, "unexpected option %s\n", option);
     }
diff --git a/softmmu/runstate.c b/softmmu/runstate.c
index cab9f6fc075..8272aef43b4 100644
--- a/softmmu/runstate.c
+++ b/softmmu/runstate.c
@@ -236,7 +236,7 @@ StatusInfo *qmp_query_status(Error **errp)
     StatusInfo *info = g_malloc0(sizeof(*info));
 
     info->running = runstate_is_running();
-    info->singlestep = singlestep;
+    info->singlestep = one_insn_per_tb;
     info->status = current_run_state;
 
     return info;
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 9177d95d4ec..dbe5124b5e7 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2957,7 +2957,7 @@ void qemu_init(int argc, char **argv)
                 qdict_put_str(machine_opts_dict, "firmware", optarg);
                 break;
             case QEMU_OPTION_singlestep:
-                singlestep = 1;
+                one_insn_per_tb = 1;
                 break;
             case QEMU_OPTION_S:
                 autostart = 0;
-- 
2.34.1

[RFC PATCH 2/5] 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 99bcd542b42..41089e51cea 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -395,7 +395,7 @@ static void handle_arg_reserved_va(const char *arg)
     }
 }
 
-static void handle_arg_singlestep(const char *arg)
+static void handle_arg_one_insn_per_tb(const char *arg)
 {
     one_insn_per_tb = 1;
 }
@@ -486,8 +486,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

[RFC PATCH 3/5] 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 a8de6906ed5..8ede40614ec 100644
--- a/bsd-user/main.c
+++ b/bsd-user/main.c
@@ -167,7 +167,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"
@@ -390,7 +391,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") {
             one_insn_per_tb = 1;
         } else if (!strcmp(r, "strace")) {
             do_strace = 1;
-- 
2.34.1

[RFC PATCH 4/5] softmmu: 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>
---
 softmmu/vl.c    |  1 +
 qemu-options.hx | 14 ++++++++++++--
 tcg/tci/README  |  2 +-
 3 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/softmmu/vl.c b/softmmu/vl.c
index dbe5124b5e7..61335ec7bc0 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2957,6 +2957,7 @@ void qemu_init(int argc, char **argv)
                 qdict_put_str(machine_opts_dict, "firmware", optarg);
                 break;
             case QEMU_OPTION_singlestep:
+            case QEMU_OPTION_one_insn_per_tb:
                 one_insn_per_tb = 1;
                 break;
             case QEMU_OPTION_S:
diff --git a/qemu-options.hx b/qemu-options.hx
index 88e93c61031..184f8cc36d0 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -4158,11 +4158,21 @@ SRST
     from a script.
 ERST
 
+DEF("one-insn-per-tb", 0, QEMU_OPTION_one_insn_per_tb, \
+    "-one-insn-per-tb     run with one guest instruction per emulated TB\n", QEMU_ARCH_ALL)
+SRST
+``-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.
+    This only has an effect when using TCG, not with KVM or other accelerators.
+ERST
+
 DEF("singlestep", 0, QEMU_OPTION_singlestep, \
-    "-singlestep     always run in singlestep mode\n", QEMU_ARCH_ALL)
+    "-singlestep     deprecated synonym for -one-insn-per-tb\n", QEMU_ARCH_ALL)
 SRST
 ``-singlestep``
-    Run the emulation in single step mode.
+    This is a deprecated synonym for the -one-insn-per-tb option.
 ERST
 
 DEF("preconfig", 0, QEMU_OPTION_preconfig, \
diff --git a/tcg/tci/README b/tcg/tci/README
index f72a40a395a..751558f2892 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 -one-insn-per-tb
 
 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

[RFC PATCH 5/5] 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.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/monitor/hmp.h       |  2 +-
 softmmu/runstate-hmp-cmds.c |  2 +-
 tests/qtest/test-hmp.c      |  1 +
 hmp-commands.hx             | 25 +++++++++++++++++++++----
 4 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
index 2220f14fc98..e66c8c63799 100644
--- a/include/monitor/hmp.h
+++ b/include/monitor/hmp.h
@@ -156,7 +156,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 29c9a038863..da218aa4b86 100644
--- a/softmmu/runstate-hmp-cmds.c
+++ b/softmmu/runstate-hmp-cmds.c
@@ -40,7 +40,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");
     if (!option || !strcmp(option, "on")) {
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 fbb5daf09be..88933567845 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: [RFC PATCH 0/5] Deprecate/rename singlestep command line option

[Richard Henderson]

On 2/6/23 07:13, Peter Maydell wrote:
> So, questions:
> 
> (1) is this worth bothering with at all? We could just
>      name our global variable etc better, and document what
>      -singlestep actually does, and not bother with the new
>      names for the options/commands.
> (2) if we do do it, do we retain the old name indefinitely,
>      or actively put it on the deprecate-and-drop list?
> (3) what should we do about the HMP StatusInfo object?
>      I'm not sure how we handle compatibility for HMP.

I was thinking that a better place to put this is within -d, akin to nochain.

I would deprecate and drop.  I dunno what to do about HMP.


r~

Re: [RFC PATCH 0/5] Deprecate/rename singlestep command line option

[Thomas Huth]

On 06/02/2023 18.13, Peter Maydell wrote:
> 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:
>   * renames the 'singlestep' global variable to 'one_insn_per_tb'
>   * Adds new '-one-insn-per-tb' command line options and a

Please no new "top level" command line options like this! It's related to 
TCG, so this should IMHO become a parameter of the "-accel tcg" option.

  Thomas

Re: [RFC PATCH 1/5] Rename the singlestep global variable to one_insn_per_tb

[Thomas Huth]

On 06/02/2023 18.13, Peter Maydell wrote:
> The 'singlestep' global variable is badly misnamed, because it has
> nothing to do with single-stepping the emulation either via the gdb
> stub or by emulation of architectural debug facilities.  Instead what
> it does is force TCG to put only one instruction into each TB.
> Rename it to one_insn_per_tb, so that it reflects what it does better
> and is easier to search for in the codebase.
> 
> This misnaming is also present in user-facing interfaces: the command
> line option '-singlestep', the HMP 'singlestep' command, and the QMP
> StatusInfo object.  Those are harder to update because we need to
> retain backwards compatibility.  Subsequent patches will add
> better-named aliases so we can eventually deprecate-and-drop the old,
> bad name.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   include/exec/cpu-common.h   | 2 +-
>   accel/tcg/cpu-exec.c        | 4 ++--
>   bsd-user/main.c             | 4 ++--
>   linux-user/main.c           | 4 ++--
>   softmmu/globals.c           | 2 +-
>   softmmu/runstate-hmp-cmds.c | 4 ++--
>   softmmu/runstate.c          | 2 +-
>   softmmu/vl.c                | 2 +-
>   8 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
> index 6feaa40ca7b..f2592a1967f 100644
> --- a/include/exec/cpu-common.h
> +++ b/include/exec/cpu-common.h
> @@ -163,7 +163,7 @@ int cpu_memory_rw_debug(CPUState *cpu, vaddr addr,
>                           void *ptr, size_t len, bool is_write);
>   
>   /* vl.c */
> -extern int singlestep;
> +extern int one_insn_per_tb;

IMHO the global variable should completely go away - this should get a 
property of the tcg accel instead.

  Thomas

Re: [RFC PATCH 0/5] Deprecate/rename singlestep command line option

[Peter Maydell]

On Mon, 6 Feb 2023 at 20:18, Thomas Huth <thuth@redhat.com> wrote:
>
> On 06/02/2023 18.13, Peter Maydell wrote:
> > 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:
> >   * renames the 'singlestep' global variable to 'one_insn_per_tb'
> >   * Adds new '-one-insn-per-tb' command line options and a
>
> Please no new "top level" command line options like this! It's related to
> TCG, so this should IMHO become a parameter of the "-accel tcg" option.

That makes sense (and is probably an argument for taking
the deprecate-and-drop step). Is there an equivalent to
"accel suboptions" for HMP commands, or does that just
stay a top-level command ?

(For the user-mode binaries it'll stay a top level option
because those are all we have there.)

thanks
-- PMM

Re: [RFC PATCH 0/5] Deprecate/rename singlestep command line option

[Thomas Huth]

On 07/02/2023 12.01, Peter Maydell wrote:
> On Mon, 6 Feb 2023 at 20:18, Thomas Huth <thuth@redhat.com> wrote:
>>
>> On 06/02/2023 18.13, Peter Maydell wrote:
>>> 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:
>>>    * renames the 'singlestep' global variable to 'one_insn_per_tb'
>>>    * Adds new '-one-insn-per-tb' command line options and a
>>
>> Please no new "top level" command line options like this! It's related to
>> TCG, so this should IMHO become a parameter of the "-accel tcg" option.
> 
> That makes sense (and is probably an argument for taking
> the deprecate-and-drop step). Is there an equivalent to
> "accel suboptions" for HMP commands, or does that just
> stay a top-level command ?

I'm not aware of an "accel" HMP command, so I guess it has to stay top 
level. Or you could introduce a new "accel" command now, so we have 
something we can use in the future for other related HMP commands, too?

> (For the user-mode binaries it'll stay a top level option
> because those are all we have there.)

Ack, that's right.

  Thomas

Re: [RFC PATCH 0/5] Deprecate/rename singlestep command line option

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> 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:
>  * renames the 'singlestep' global variable to 'one_insn_per_tb'
>  * Adds new '-one-insn-per-tb' command line options and a
>    HMP 'one-insn-per-tb' command
>  * Documents the '-singlestep' options and 'singlestep'
>    HMP command as deprecated synonyms for the new ones
>
> It does not do anything about the other place where we surface
> 'singlestep', which is in the QMP StatusInfo object returned by the
> 'query-status' command.  This is incorrectly documented as "true if
> VCPUs are in single-step mode" and "singlestep is enabled through
> the GDB stub", because what it's actually returning is the
> one-insn-per-tb state.
>
> Things I didn't bother with because this is only an RFC but
> will do if it turns into a non-RFC patchset:
>  * test the bsd-user changes :-)
>  * add text to deprecated.rst
>
> So, questions:
>
> (1) is this worth bothering with at all? We could just
>     name our global variable etc better, and document what
>     -singlestep actually does, and not bother with the new
>     names for the options/commands.

The feature is kind of esoteric.  Rather weak excuse for not fixing bad
UI, in my opinion.  Weaker still since you already did a good part of
the actual work.

> (2) if we do do it, do we retain the old name indefinitely,
>     or actively put it on the deprecate-and-drop list?

By "the old name", you mean the CLI option name, right?

I'd prefer deprecate and drop.

> (3) what should we do about the HMP StatusInfo object?
>     I'm not sure how we handle compatibility for HMP.

Uh, you mean *QMP*, don't you?

As you wrote above, StatusInfo is returned by query-status, which is a
stable interface.  Changes to members therefore require the usual
deprecation grace period.  We'd add a new member with a sane name, and
deprecate the old one.

The matching HMP command is "info status".  It shows member @singlestep
as " (single step mode)".  Changing that is fine; HMP is not a stable
interface.

Re: [RFC PATCH 0/5] Deprecate/rename singlestep command line option

[Warner Losh]

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

On Tue, Feb 7, 2023 at 8:56 AM Markus Armbruster <armbru@redhat.com> wrote:

> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > 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:
> >  * renames the 'singlestep' global variable to 'one_insn_per_tb'
> >  * Adds new '-one-insn-per-tb' command line options and a
> >    HMP 'one-insn-per-tb' command
> >  * Documents the '-singlestep' options and 'singlestep'
> >    HMP command as deprecated synonyms for the new ones
> >
> > It does not do anything about the other place where we surface
> > 'singlestep', which is in the QMP StatusInfo object returned by the
> > 'query-status' command.  This is incorrectly documented as "true if
> > VCPUs are in single-step mode" and "singlestep is enabled through
> > the GDB stub", because what it's actually returning is the
> > one-insn-per-tb state.
> >
> > Things I didn't bother with because this is only an RFC but
> > will do if it turns into a non-RFC patchset:
> >  * test the bsd-user changes :-)
> >  * add text to deprecated.rst
> >
> > So, questions:
> >
> > (1) is this worth bothering with at all? We could just
> >     name our global variable etc better, and document what
> >     -singlestep actually does, and not bother with the new
> >     names for the options/commands.
>
> The feature is kind of esoteric.  Rather weak excuse for not fixing bad
> UI, in my opinion.  Weaker still since you already did a good part of
> the actual work.
>
> > (2) if we do do it, do we retain the old name indefinitely,
> >     or actively put it on the deprecate-and-drop list?
>
> By "the old name", you mean the CLI option name, right?
>
> I'd prefer deprecate and drop.
>
> > (3) what should we do about the HMP StatusInfo object?
> >     I'm not sure how we handle compatibility for HMP.
>
> Uh, you mean *QMP*, don't you?
>
> As you wrote above, StatusInfo is returned by query-status, which is a
> stable interface.  Changes to members therefore require the usual
> deprecation grace period.  We'd add a new member with a sane name, and
> deprecate the old one.
>
> The matching HMP command is "info status".  It shows member @singlestep
> as " (single step mode)".  Changing that is fine; HMP is not a stable
> interface.
>

We don't use this feature on bsd-user, so I'm happy to follow whatever is
decided
here. I can test things once the suggestion in this and other threads have
played out
and there's another RFC set of patches.

Warner

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

Re: [RFC PATCH 1/5] Rename the singlestep global variable to one_insn_per_tb

[Peter Maydell]

On Mon, 6 Feb 2023 at 20:20, Thomas Huth <thuth@redhat.com> wrote:
>
> On 06/02/2023 18.13, Peter Maydell wrote:
> > The 'singlestep' global variable is badly misnamed, because it has
> > nothing to do with single-stepping the emulation either via the gdb
> > stub or by emulation of architectural debug facilities.  Instead what
> > it does is force TCG to put only one instruction into each TB.
> > Rename it to one_insn_per_tb, so that it reflects what it does better
> > and is easier to search for in the codebase.
> >
> > This misnaming is also present in user-facing interfaces: the command
> > line option '-singlestep', the HMP 'singlestep' command, and the QMP
> > StatusInfo object.  Those are harder to update because we need to
> > retain backwards compatibility.  Subsequent patches will add
> > better-named aliases so we can eventually deprecate-and-drop the old,
> > bad name.
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >   include/exec/cpu-common.h   | 2 +-
> >   accel/tcg/cpu-exec.c        | 4 ++--
> >   bsd-user/main.c             | 4 ++--
> >   linux-user/main.c           | 4 ++--
> >   softmmu/globals.c           | 2 +-
> >   softmmu/runstate-hmp-cmds.c | 4 ++--
> >   softmmu/runstate.c          | 2 +-
> >   softmmu/vl.c                | 2 +-
> >   8 files changed, 12 insertions(+), 12 deletions(-)
> >
> > diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
> > index 6feaa40ca7b..f2592a1967f 100644
> > --- a/include/exec/cpu-common.h
> > +++ b/include/exec/cpu-common.h
> > @@ -163,7 +163,7 @@ int cpu_memory_rw_debug(CPUState *cpu, vaddr addr,
> >                           void *ptr, size_t len, bool is_write);
> >
> >   /* vl.c */
> > -extern int singlestep;
> > +extern int one_insn_per_tb;
>
> IMHO the global variable should completely go away - this should get a
> property of the tcg accel instead.

I agree in principle, but this might be tricky because of the
user-mode emulators. I'll have a look.

-- PMM

Re: [RFC PATCH 0/5] Deprecate/rename singlestep command line option

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
> 
> > 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:
> >  * renames the 'singlestep' global variable to 'one_insn_per_tb'
> >  * Adds new '-one-insn-per-tb' command line options and a
> >    HMP 'one-insn-per-tb' command
> >  * Documents the '-singlestep' options and 'singlestep'
> >    HMP command as deprecated synonyms for the new ones
> >
> > It does not do anything about the other place where we surface
> > 'singlestep', which is in the QMP StatusInfo object returned by the
> > 'query-status' command.  This is incorrectly documented as "true if
> > VCPUs are in single-step mode" and "singlestep is enabled through
> > the GDB stub", because what it's actually returning is the
> > one-insn-per-tb state.
> >
> > Things I didn't bother with because this is only an RFC but
> > will do if it turns into a non-RFC patchset:
> >  * test the bsd-user changes :-)
> >  * add text to deprecated.rst
> >
> > So, questions:
> >
> > (1) is this worth bothering with at all? We could just
> >     name our global variable etc better, and document what
> >     -singlestep actually does, and not bother with the new
> >     names for the options/commands.
> 
> The feature is kind of esoteric.  Rather weak excuse for not fixing bad
> UI, in my opinion.  Weaker still since you already did a good part of
> the actual work.
> 
> > (2) if we do do it, do we retain the old name indefinitely,
> >     or actively put it on the deprecate-and-drop list?
> 
> By "the old name", you mean the CLI option name, right?
> 
> I'd prefer deprecate and drop.
> 
> > (3) what should we do about the HMP StatusInfo object?
> >     I'm not sure how we handle compatibility for HMP.
> 
> Uh, you mean *QMP*, don't you?
> 
> As you wrote above, StatusInfo is returned by query-status, which is a
> stable interface.  Changes to members therefore require the usual
> deprecation grace period.  We'd add a new member with a sane name, and
> deprecate the old one.
> 
> The matching HMP command is "info status".  It shows member @singlestep
> as " (single step mode)".  Changing that is fine; HMP is not a stable
> interface.

Right, and similarly you don't need to keep the old 'singlestep' command
around; you can just rename.

Dave

-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [RFC PATCH 0/5] Deprecate/rename singlestep command line option

[Peter Maydell]

On Tue, 7 Feb 2023 at 15:56, Markus Armbruster <armbru@redhat.com> wrote:
> > (3) what should we do about the HMP StatusInfo object?
> >     I'm not sure how we handle compatibility for HMP.
>
> Uh, you mean *QMP*, don't you?

Yes.

> As you wrote above, StatusInfo is returned by query-status, which is a
> stable interface.  Changes to members therefore require the usual
> deprecation grace period.  We'd add a new member with a sane name, and
> deprecate the old one.

Question: is it worth creating a new 'one-insn-per-tb' member at all,
or should we instead just make the 'singlestep' member optional and
then stop emitting it (and as a corollary stop reporting it in
HMP 'info status')? It seems kind of weird that we surface this
specific slightly esoteric accelerator property and only this one
in the StatusInfo struct.

-- PMM

Re: [RFC PATCH 0/5] Deprecate/rename singlestep command line option

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Tue, 7 Feb 2023 at 15:56, Markus Armbruster <armbru@redhat.com> wrote:
>> > (3) what should we do about the HMP StatusInfo object?
>> >     I'm not sure how we handle compatibility for HMP.
>>
>> Uh, you mean *QMP*, don't you?
>
> Yes.
>
>> As you wrote above, StatusInfo is returned by query-status, which is a
>> stable interface.  Changes to members therefore require the usual
>> deprecation grace period.  We'd add a new member with a sane name, and
>> deprecate the old one.
>
> Question: is it worth creating a new 'one-insn-per-tb' member at all,
> or should we instead just make the 'singlestep' member optional and
> then stop emitting it (and as a corollary stop reporting it in
> HMP 'info status')? It seems kind of weird that we surface this
> specific slightly esoteric accelerator property and only this one
> in the StatusInfo struct.

Making a mandatory member of QMP output optional is an incompatible
change.  Not necessary here.

If you think the value is not worth reporting, deprecate @singlestep in
QMP, and drop it after a grace period.  You can drop it from HMP right
away, or together with QMP.