| Message ID | 20230206171359.1327671-1-peter.maydell@linaro.org |
|---|---|
| Headers | show |
| Series | Deprecate/rename singlestep command line option | expand |
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~
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
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, 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
* 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
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.
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(-)