mbox series

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

Message ID 20230417164041.684562-1-peter.maydell@linaro.org
Headers show
Series Deprecate/rename singlestep command line option, monitor interfaces | expand

Message

Peter Maydell April 17, 2023, 4:40 p.m. UTC 
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
   (except the one internal to TCG itself in curr_cflags())
 * 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.)
 * moves the 'is one-insn-per-tb on?' info from 'info status'
   to 'info jit'
 * deprecates the 'singlestep' member of the QMP StatusInfo
   type, with no replacement. (We have a sketch of a design
   of how we might provide this in QMP if we need to, but
   I'm pretty sure nobody using QMP is actually using the
   info in the 'singlestep' field, especially since it's
   always been wrongly documented and there's no write
   interface, only a read one.)

Changes v2->v3:
 * curr_cflags() is a hot path, so use a global variable
   so it doesn't have to fetch the current accelerator object.
   (NB: I haven't done the tcg_global_cflags thing suggested
   in review of v2; justification in the below-the-fold part
   of the patch 3 commit message notes.)
 * put the new line in test-hmp.c in its proper alphabetical
   order place in patch 8
 * in patch 10 don't provide a replacement field in the
   QMP StatusInfo type, just deprecate the old one
 * I finally tested that bsd-user compiles, and fixed the
   missing-close-bracket error in that patch :-)

Patches still needing review: 3, 7, 10

thanks
-- PMM

Peter Maydell (10):
  make one-insn-per-tb an accel option
  softmmu: Don't use 'singlestep' global in QMP and HMP commands
  accel/tcg: Use one_insn_per_tb global instead of old singlestep global
  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
  accel/tcg: Report one-insn-per-tb in 'info jit', not 'info status'
  hmp: Add 'one-insn-per-tb' command equivalent to 'singlestep'
  qapi/run-state.json: Fix missing newline at end of file
  hmp: Deprecate 'singlestep' member of StatusInfo

 docs/about/deprecated.rst   | 39 +++++++++++++++++++++++++++++++++++++
 docs/user/main.rst          | 14 +++++++++++--
 qapi/run-state.json         | 16 +++++++++++----
 accel/tcg/internal.h        |  2 ++
 include/exec/cpu-common.h   |  2 --
 include/monitor/hmp.h       |  2 +-
 accel/tcg/cpu-exec.c        |  2 +-
 accel/tcg/monitor.c         | 14 +++++++++++++
 accel/tcg/tcg-all.c         | 23 ++++++++++++++++++++++
 bsd-user/main.c             | 14 ++++++++-----
 linux-user/main.c           | 18 +++++++++++------
 softmmu/globals.c           |  1 -
 softmmu/runstate-hmp-cmds.c | 25 ++++++++++++++++++------
 softmmu/runstate.c          | 10 +++++++++-
 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, 201 insertions(+), 38 deletions(-)

Comments

Peter Maydell May 2, 2023, 10:22 a.m. UTC | #1 
On Mon, 17 Apr 2023 at 17:40, Peter Maydell <peter.maydell@linaro.org> 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.

I'm going to take this series via target-arm.next since
I'm doing a pullreq anyway.

thanks
-- PMM
Markus Armbruster May 2, 2023, 10:43 a.m. UTC | #2 
Peter Maydell <peter.maydell@linaro.org> writes:

> On Mon, 17 Apr 2023 at 17:40, Peter Maydell <peter.maydell@linaro.org> 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.
>
> I'm going to take this series via target-arm.next since
> I'm doing a pullreq anyway.

Yes, please!