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

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.

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

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.

* 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

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

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

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

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

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~