| Message ID | 20230403144637.2949366-11-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Deprecate/rename singlestep command line option, monitor interfaces | expand |
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? [...]
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
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.
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
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
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
* 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 >
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
* 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 >
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;
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(-)