* [PATCH 0/5] Stop adding HMP-only commands, allow QMP for all @ 2021-09-08 10:37 Daniel P. Berrangé 2021-09-08 10:37 ` [PATCH 1/5] docs/devel: document expectations for QAPI data modelling for QMP Daniel P. Berrangé ` (6 more replies) 0 siblings, 7 replies; 28+ messages in thread From: Daniel P. Berrangé @ 2021-09-08 10:37 UTC (permalink / raw) To: qemu-devel Cc: Daniel P. Berrangé, Eduardo Habkost, Dr. David Alan Gilbert, Markus Armbruster, Eric Blake We are still adding HMP commands without any QMP counterparts. This is done because there are a reasonable number of scenarios where the cost of designing a QAPI data type for the command is not justified. This has the downside, however, that we will never be able to fully isolate the monitor code from the remainder of QEMU internals. It is desirable to be able to get to a point where subsystems in QEMU are exclusively implemented using QAPI types and never need to have any knowledge of the monitor APIs. The way to get there is to stop adding commands to HMP only. All commands must be implemented using QMP, and any HMP implementation be a shim around the QMP implementation. We don't want to compromise our supportability of QMP long term though. This series proposes that we relax our requirements around fine grained QAPI data design, but with the caveat that any command taking this design approach is mandated to use the 'x-' name prefix. This tradeoff should be suitable for any commands we have been adding exclusively to HMP in recent times, and thus mean we have mandate QMP support for all new commands going forward. This series illustrates the concept by converting the "info registers" HMP to invoke a new 'x-query-registers' QMP command. Note that only the i386 CPU target is converted to work with this new approach, so this series needs to be considered incomplete. If we go forward with this idea, then a subsequent version of this series would need to obviously convert all other CPU targets. After doing that conversion the only use of qemu_fprintf() would be the disas.c file. Remaining uses of qemu_fprintf and qemu_printf could be tackled in a similar way and eventually eliminate the need for any of these printf wrappers in QEMU. NB: I added docs to devel/writing-qmp-commands.rst about the two design approaches to QMP. I didn't see another good place to put an explicit note that we will not add any more HMP-only commands. Obviously HMP/QMP maintainers control this in their reviews of patches, and maybe that's sufficient ? NB: if we take this approach we'll want to figure out how many HMP-only commands we actually have left and then perhaps have a task to track their conversion to QMP. This could possibly be a useful task for newbies if we make it clear that they wouldn't be required to undertake complex QAPI modelling in doing this conversion. Daniel P. Berrangé (5): docs/devel: document expectations for QAPI data modelling for QMP hw/core: introduce 'format_state' callback to replace 'dump_state' target/i386: convert to use format_state instead of dump_state qapi: introduce x-query-registers QMP command monitor: rewrite 'info registers' in terms of 'x-query-registers' docs/devel/writing-qmp-commands.rst | 25 +++ hw/core/cpu-common.c | 15 ++ hw/core/machine-qmp-cmds.c | 28 +++ include/hw/core/cpu.h | 13 +- monitor/misc.c | 25 ++- qapi/machine.json | 37 ++++ target/i386/cpu-dump.c | 325 +++++++++++++++------------- target/i386/cpu.c | 2 +- target/i386/cpu.h | 2 +- 9 files changed, 307 insertions(+), 165 deletions(-) -- 2.31.1 ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 1/5] docs/devel: document expectations for QAPI data modelling for QMP 2021-09-08 10:37 [PATCH 0/5] Stop adding HMP-only commands, allow QMP for all Daniel P. Berrangé @ 2021-09-08 10:37 ` Daniel P. Berrangé 2021-09-08 17:42 ` Eric Blake 2021-09-09 9:33 ` Markus Armbruster 2021-09-08 10:37 ` [PATCH 2/5] hw/core: introduce 'format_state' callback to replace 'dump_state' Daniel P. Berrangé ` (5 subsequent siblings) 6 siblings, 2 replies; 28+ messages in thread From: Daniel P. Berrangé @ 2021-09-08 10:37 UTC (permalink / raw) To: qemu-devel Cc: Daniel P. Berrangé, Eduardo Habkost, Dr. David Alan Gilbert, Markus Armbruster, Eric Blake Traditionally we have required that newly added QMP commands will model any returned data using fine grained QAPI types. This is good for commands that are intended to be consumed by machines, where clear data representation is very important. Commands that don't satisfy this have generally been added to HMP only. In effect the decision of whether to add a new command to QMP vs HMP has been used as a proxy for the decision of whether the cost of designing a fine grained QAPI type is justified by the potential benefits. As a result the commands present in QMP and HMP are non-overlapping sets, although HMP comamnds can be accessed indirectly via the QMP command 'human-monitor-command'. One of the downsides of 'human-monitor-command' is that the QEMU monitor APIs remain tied into various internal parts of the QEMU code. For example any exclusively HMP command will need to use 'monitor_printf' to get data out. It would be desirable to be able to fully isolate the monitor implementation from QEMU internals, however, this is only possible if all commands are exclusively based on QAPI with direct QMP exposure. The way to achieve this desired end goal is to finese the requirements for QMP command design. For cases where the output of a command is only intended for human consumption, it is reasonable to want to simplify the implementation by returning a plain string containing formatted data instead of designing a fine grained QAPI data type. This can be permitted if-and-only-if the command is exposed under the 'x-' name prefix. This indicates that the command data format is liable to future change and that it is not following QAPI design best practice. The poster child example for this would be the 'info registers' HMP command which returns printf formatted data representing CPU state. This information varies enourmously across target architectures and changes relatively frequently as new CPU features are implemented. It is there as debugging data for human operators, and any machine usage would treat it as an opaque blob. It is thus reasonable to expose this in QMP as 'x-query-registers' returning a 'str' field. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- docs/devel/writing-qmp-commands.rst | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/docs/devel/writing-qmp-commands.rst b/docs/devel/writing-qmp-commands.rst index 6a10a06c48..d032daa62d 100644 --- a/docs/devel/writing-qmp-commands.rst +++ b/docs/devel/writing-qmp-commands.rst @@ -350,6 +350,31 @@ In this section we will focus on user defined types. Please, check the QAPI documentation for information about the other types. +Modelling data in QAPI +~~~~~~~~~~~~~~~~~~~~~~ + +For a QMP command that to be considered stable and supported long term there +is a requirement returned data should be explicitly modelled using fine grained +QAPI types. As a general guide, a caller of the QMP command should never need +to parse individual returned data fields. If a field appears to need parsing, +them it should be split into separate fields corresponding to each distinct +data item. This should be the common case for any new QMP command that is +intended to be used by machines, as opposed to exclusively human operators. + +Some QMP commands, however, are only intended as adhoc debugging aids for human +operators. While they may return large amounts of formatted data, it is not +expected that machines will need to parse the result. The overhead of defining +a fine grained QAPI type for the data may not be justified by the potential +benefit. In such cases, it is permitted to have a command return a simple string +that contains formatted data, however, it is mandatory for the command to use +the 'x-' name prefix. This indicates that the command is not guaranteed to be +long term stable / liable to change in future and is not following QAPI design +best practices. An example where this approach is taken is the QMP command +"x-query-registers". This returns a printf formatted dump of the architecture +specific CPU state. The way the data is formatted varies across QEMU targets, +is liable to change over time, and is only intended to be consumed as an opaque +string by machines. + User Defined Types ~~~~~~~~~~~~~~~~~~ -- 2.31.1 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 1/5] docs/devel: document expectations for QAPI data modelling for QMP 2021-09-08 10:37 ` [PATCH 1/5] docs/devel: document expectations for QAPI data modelling for QMP Daniel P. Berrangé @ 2021-09-08 17:42 ` Eric Blake 2021-09-09 9:33 ` Markus Armbruster 1 sibling, 0 replies; 28+ messages in thread From: Eric Blake @ 2021-09-08 17:42 UTC (permalink / raw) To: Daniel P. Berrangé Cc: Markus Armbruster, Eduardo Habkost, qemu-devel, Dr. David Alan Gilbert On Wed, Sep 08, 2021 at 11:37:07AM +0100, Daniel P. Berrangé wrote: > Traditionally we have required that newly added QMP commands will model > any returned data using fine grained QAPI types. This is good for > commands that are intended to be consumed by machines, where clear data > representation is very important. Commands that don't satisfy this have > generally been added to HMP only. > > In effect the decision of whether to add a new command to QMP vs HMP has > been used as a proxy for the decision of whether the cost of designing a > fine grained QAPI type is justified by the potential benefits. > > As a result the commands present in QMP and HMP are non-overlapping > sets, although HMP comamnds can be accessed indirectly via the QMP > command 'human-monitor-command'. > > One of the downsides of 'human-monitor-command' is that the QEMU monitor > APIs remain tied into various internal parts of the QEMU code. For > example any exclusively HMP command will need to use 'monitor_printf' > to get data out. It would be desirable to be able to fully isolate the > monitor implementation from QEMU internals, however, this is only > possible if all commands are exclusively based on QAPI with direct > QMP exposure. > > The way to achieve this desired end goal is to finese the requirements > for QMP command design. For cases where the output of a command is only > intended for human consumption, it is reasonable to want to simplify > the implementation by returning a plain string containing formatted > data instead of designing a fine grained QAPI data type. This can be > permitted if-and-only-if the command is exposed under the 'x-' name > prefix. This indicates that the command data format is liable to > future change and that it is not following QAPI design best practice. > > The poster child example for this would be the 'info registers' HMP > command which returns printf formatted data representing CPU state. > This information varies enourmously across target architectures and > changes relatively frequently as new CPU features are implemented. > It is there as debugging data for human operators, and any machine > usage would treat it as an opaque blob. It is thus reasonable to > expose this in QMP as 'x-query-registers' returning a 'str' field. > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > --- > docs/devel/writing-qmp-commands.rst | 25 +++++++++++++++++++++++++ > 1 file changed, 25 insertions(+) > > diff --git a/docs/devel/writing-qmp-commands.rst b/docs/devel/writing-qmp-commands.rst > index 6a10a06c48..d032daa62d 100644 > --- a/docs/devel/writing-qmp-commands.rst > +++ b/docs/devel/writing-qmp-commands.rst > @@ -350,6 +350,31 @@ In this section we will focus on user defined types. Please, check the QAPI > documentation for information about the other types. > > > +Modelling data in QAPI > +~~~~~~~~~~~~~~~~~~~~~~ > + > +For a QMP command that to be considered stable and supported long term there > +is a requirement returned data should be explicitly modelled using fine grained > +QAPI types. As a general guide, a caller of the QMP command should never need > +to parse individual returned data fields. If a field appears to need parsing, > +them it should be split into separate fields corresponding to each distinct then > +data item. This should be the common case for any new QMP command that is > +intended to be used by machines, as opposed to exclusively human operators. > + > +Some QMP commands, however, are only intended as adhoc debugging aids for human ad hoc > +operators. While they may return large amounts of formatted data, it is not > +expected that machines will need to parse the result. The overhead of defining > +a fine grained QAPI type for the data may not be justified by the potential > +benefit. In such cases, it is permitted to have a command return a simple string > +that contains formatted data, however, it is mandatory for the command to use > +the 'x-' name prefix. This indicates that the command is not guaranteed to be > +long term stable / liable to change in future and is not following QAPI design > +best practices. An example where this approach is taken is the QMP command > +"x-query-registers". This returns a printf formatted dump of the architecture > +specific CPU state. The way the data is formatted varies across QEMU targets, > +is liable to change over time, and is only intended to be consumed as an opaque > +string by machines. > + > User Defined Types > ~~~~~~~~~~~~~~~~~~ > > -- > 2.31.1 > Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/5] docs/devel: document expectations for QAPI data modelling for QMP 2021-09-08 10:37 ` [PATCH 1/5] docs/devel: document expectations for QAPI data modelling for QMP Daniel P. Berrangé 2021-09-08 17:42 ` Eric Blake @ 2021-09-09 9:33 ` Markus Armbruster 2021-09-10 12:46 ` Daniel P. Berrangé 1 sibling, 1 reply; 28+ messages in thread From: Markus Armbruster @ 2021-09-09 9:33 UTC (permalink / raw) To: Daniel P. Berrangé Cc: Eduardo Habkost, Eric Blake, qemu-devel, Dr. David Alan Gilbert Daniel P. Berrangé <berrange@redhat.com> writes: > Traditionally we have required that newly added QMP commands will model > any returned data using fine grained QAPI types. This is good for > commands that are intended to be consumed by machines, where clear data > representation is very important. Commands that don't satisfy this have > generally been added to HMP only. > > In effect the decision of whether to add a new command to QMP vs HMP has > been used as a proxy for the decision of whether the cost of designing a > fine grained QAPI type is justified by the potential benefits. > > As a result the commands present in QMP and HMP are non-overlapping > sets, although HMP comamnds can be accessed indirectly via the QMP > command 'human-monitor-command'. > > One of the downsides of 'human-monitor-command' is that the QEMU monitor > APIs remain tied into various internal parts of the QEMU code. For > example any exclusively HMP command will need to use 'monitor_printf' > to get data out. It would be desirable to be able to fully isolate the > monitor implementation from QEMU internals, however, this is only > possible if all commands are exclusively based on QAPI with direct > QMP exposure. > > The way to achieve this desired end goal is to finese the requirements > for QMP command design. For cases where the output of a command is only > intended for human consumption, it is reasonable to want to simplify > the implementation by returning a plain string containing formatted > data instead of designing a fine grained QAPI data type. This can be > permitted if-and-only-if the command is exposed under the 'x-' name > prefix. This indicates that the command data format is liable to > future change and that it is not following QAPI design best practice. > > The poster child example for this would be the 'info registers' HMP > command which returns printf formatted data representing CPU state. > This information varies enourmously across target architectures and > changes relatively frequently as new CPU features are implemented. > It is there as debugging data for human operators, and any machine > usage would treat it as an opaque blob. It is thus reasonable to > expose this in QMP as 'x-query-registers' returning a 'str' field. > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > --- > docs/devel/writing-qmp-commands.rst | 25 +++++++++++++++++++++++++ > 1 file changed, 25 insertions(+) > > diff --git a/docs/devel/writing-qmp-commands.rst b/docs/devel/writing-qmp-commands.rst > index 6a10a06c48..d032daa62d 100644 > --- a/docs/devel/writing-qmp-commands.rst > +++ b/docs/devel/writing-qmp-commands.rst > @@ -350,6 +350,31 @@ In this section we will focus on user defined types. Please, check the QAPI > documentation for information about the other types. > > > +Modelling data in QAPI > +~~~~~~~~~~~~~~~~~~~~~~ Outline now: How to write QMP commands using the QAPI framework Overview Testing Writing a command that doesn't return data Arguments Errors Command Documentation Implementing the HMP command Writing a command that returns data --> Modelling data in QAPI User Defined Types The HMP command Returning Lists Awkward. I guess you wanted it next to "User Defined Types", which makes some sense. Perhaps minor tweaks (headlines, maybe a bit of text as well) would suffice: How to write QMP commands using the QAPI framework Overview Testing Writing a simple command: hello-world Arguments Errors Command Documentation Implementing the HMP command Writing more complex commands Modelling data in QAPI User Defined Types The HMP command Returning Lists > + > +For a QMP command that to be considered stable and supported long term there "term, there" > +is a requirement returned data should be explicitly modelled using fine grained "fine-grained", I think. > +QAPI types. As a general guide, a caller of the QMP command should never need > +to parse individual returned data fields. If a field appears to need parsing, > +them it should be split into separate fields corresponding to each distinct > +data item. This should be the common case for any new QMP command that is > +intended to be used by machines, as opposed to exclusively human operators. > + > +Some QMP commands, however, are only intended as adhoc debugging aids for human > +operators. While they may return large amounts of formatted data, it is not > +expected that machines will need to parse the result. The overhead of defining > +a fine grained QAPI type for the data may not be justified by the potential > +benefit. In such cases, it is permitted to have a command return a simple string There are many existing long lines in this file, so I'm not flagging yours, except for this one, because it increases the maximum. > +that contains formatted data, however, it is mandatory for the command to use > +the 'x-' name prefix. This indicates that the command is not guaranteed to be > +long term stable / liable to change in future and is not following QAPI design > +best practices. An example where this approach is taken is the QMP command > +"x-query-registers". This returns a printf formatted dump of the architecture Drop "printf". > +specific CPU state. The way the data is formatted varies across QEMU targets, > +is liable to change over time, and is only intended to be consumed as an opaque > +string by machines. > + > User Defined Types > ~~~~~~~~~~~~~~~~~~ This file is a tutorial. It teaches command writing by examples[*]. I think it should also teach this new class of "ad hoc" QMP commands the same way. A section "Writing a debugging aid returning unstructured text" could go at the very end. [*] Bit-rotten in places, but that's life for docs. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/5] docs/devel: document expectations for QAPI data modelling for QMP 2021-09-09 9:33 ` Markus Armbruster @ 2021-09-10 12:46 ` Daniel P. Berrangé 2021-09-10 13:45 ` Markus Armbruster 0 siblings, 1 reply; 28+ messages in thread From: Daniel P. Berrangé @ 2021-09-10 12:46 UTC (permalink / raw) To: Markus Armbruster Cc: Eduardo Habkost, Eric Blake, qemu-devel, Dr. David Alan Gilbert On Thu, Sep 09, 2021 at 11:33:20AM +0200, Markus Armbruster wrote: > Daniel P. Berrangé <berrange@redhat.com> writes: > > > Traditionally we have required that newly added QMP commands will model > > any returned data using fine grained QAPI types. This is good for > > commands that are intended to be consumed by machines, where clear data > > representation is very important. Commands that don't satisfy this have > > generally been added to HMP only. > > > > In effect the decision of whether to add a new command to QMP vs HMP has > > been used as a proxy for the decision of whether the cost of designing a > > fine grained QAPI type is justified by the potential benefits. > > > > As a result the commands present in QMP and HMP are non-overlapping > > sets, although HMP comamnds can be accessed indirectly via the QMP > > command 'human-monitor-command'. > > > > One of the downsides of 'human-monitor-command' is that the QEMU monitor > > APIs remain tied into various internal parts of the QEMU code. For > > example any exclusively HMP command will need to use 'monitor_printf' > > to get data out. It would be desirable to be able to fully isolate the > > monitor implementation from QEMU internals, however, this is only > > possible if all commands are exclusively based on QAPI with direct > > QMP exposure. > > > > The way to achieve this desired end goal is to finese the requirements > > for QMP command design. For cases where the output of a command is only > > intended for human consumption, it is reasonable to want to simplify > > the implementation by returning a plain string containing formatted > > data instead of designing a fine grained QAPI data type. This can be > > permitted if-and-only-if the command is exposed under the 'x-' name > > prefix. This indicates that the command data format is liable to > > future change and that it is not following QAPI design best practice. > > > > The poster child example for this would be the 'info registers' HMP > > command which returns printf formatted data representing CPU state. > > This information varies enourmously across target architectures and > > changes relatively frequently as new CPU features are implemented. > > It is there as debugging data for human operators, and any machine > > usage would treat it as an opaque blob. It is thus reasonable to > > expose this in QMP as 'x-query-registers' returning a 'str' field. > > > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > > --- > > docs/devel/writing-qmp-commands.rst | 25 +++++++++++++++++++++++++ > > 1 file changed, 25 insertions(+) > > +QAPI types. As a general guide, a caller of the QMP command should never need > > +to parse individual returned data fields. If a field appears to need parsing, > > +them it should be split into separate fields corresponding to each distinct > > +data item. This should be the common case for any new QMP command that is > > +intended to be used by machines, as opposed to exclusively human operators. > > + > > +Some QMP commands, however, are only intended as adhoc debugging aids for human > > +operators. While they may return large amounts of formatted data, it is not > > +expected that machines will need to parse the result. The overhead of defining > > +a fine grained QAPI type for the data may not be justified by the potential > > +benefit. In such cases, it is permitted to have a command return a simple string > > There are many existing long lines in this file, so I'm not flagging > yours, except for this one, because it increases the maximum. This line is at exactly 80 characters so checkstyle is happy with it. We don't have any requirement for a differenet line limit for docs afair ? Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/5] docs/devel: document expectations for QAPI data modelling for QMP 2021-09-10 12:46 ` Daniel P. Berrangé @ 2021-09-10 13:45 ` Markus Armbruster 2021-09-10 13:52 ` Daniel P. Berrangé 0 siblings, 1 reply; 28+ messages in thread From: Markus Armbruster @ 2021-09-10 13:45 UTC (permalink / raw) To: Daniel P. Berrangé Cc: Eduardo Habkost, Eric Blake, qemu-devel, Dr. David Alan Gilbert Daniel P. Berrangé <berrange@redhat.com> writes: > On Thu, Sep 09, 2021 at 11:33:20AM +0200, Markus Armbruster wrote: [...] >> There are many existing long lines in this file, so I'm not flagging >> yours, except for this one, because it increases the maximum. > > This line is at exactly 80 characters so checkstyle is happy with it. > We don't have any requirement for a differenet line limit for docs > afair ? We don't. I'm asking for a favour. Humans tend to have trouble following long lines with our eyes (I sure do). Typographic manuals suggest to limit columns to roughly 60 characters for exactly that reason[1]. 70 is a reasonable compromise between legibility and "writability". For code, we use 80-90, because there the actual width should still be fine due to indentation. checkpatch.pl doesn't differentiate betwen code and prose. This file is already hard to read for me. Please consider not making it harder. [1] https://en.wikipedia.org/wiki/Column_(typography)#Typographic_style ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/5] docs/devel: document expectations for QAPI data modelling for QMP 2021-09-10 13:45 ` Markus Armbruster @ 2021-09-10 13:52 ` Daniel P. Berrangé 0 siblings, 0 replies; 28+ messages in thread From: Daniel P. Berrangé @ 2021-09-10 13:52 UTC (permalink / raw) To: Markus Armbruster Cc: Eduardo Habkost, Eric Blake, qemu-devel, Dr. David Alan Gilbert On Fri, Sep 10, 2021 at 03:45:11PM +0200, Markus Armbruster wrote: > Daniel P. Berrangé <berrange@redhat.com> writes: > > > On Thu, Sep 09, 2021 at 11:33:20AM +0200, Markus Armbruster wrote: > > [...] > > >> There are many existing long lines in this file, so I'm not flagging > >> yours, except for this one, because it increases the maximum. > > > > This line is at exactly 80 characters so checkstyle is happy with it. > > We don't have any requirement for a differenet line limit for docs > > afair ? > > We don't. I'm asking for a favour. > > Humans tend to have trouble following long lines with our eyes (I sure > do). Typographic manuals suggest to limit columns to roughly 60 > characters for exactly that reason[1]. > > 70 is a reasonable compromise between legibility and "writability". For > code, we use 80-90, because there the actual width should still be fine > due to indentation. > > checkpatch.pl doesn't differentiate betwen code and prose. > > This file is already hard to read for me. Please consider not making it > harder. Ok, no worries, I just didn't understand the rationale. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 2/5] hw/core: introduce 'format_state' callback to replace 'dump_state' 2021-09-08 10:37 [PATCH 0/5] Stop adding HMP-only commands, allow QMP for all Daniel P. Berrangé 2021-09-08 10:37 ` [PATCH 1/5] docs/devel: document expectations for QAPI data modelling for QMP Daniel P. Berrangé @ 2021-09-08 10:37 ` Daniel P. Berrangé 2021-09-08 10:37 ` [PATCH 3/5] target/i386: convert to use format_state instead of dump_state Daniel P. Berrangé ` (4 subsequent siblings) 6 siblings, 0 replies; 28+ messages in thread From: Daniel P. Berrangé @ 2021-09-08 10:37 UTC (permalink / raw) To: qemu-devel Cc: Daniel P. Berrangé, Eduardo Habkost, Dr. David Alan Gilbert, Markus Armbruster, Eric Blake The 'dump_state' callback assumes it will be outputting to a FILE object. This is fine for HMP, but not so useful for QMP. Introduce a new 'format_state' callback that returns a formatted GString instead. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- hw/core/cpu-common.c | 15 +++++++++++++++ include/hw/core/cpu.h | 13 ++++++++++++- 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c index e2f5a64604..c2cd33a817 100644 --- a/hw/core/cpu-common.c +++ b/hw/core/cpu-common.c @@ -106,6 +106,21 @@ void cpu_dump_state(CPUState *cpu, FILE *f, int flags) if (cc->dump_state) { cpu_synchronize_state(cpu); cc->dump_state(cpu, f, flags); + } else if (cc->format_state) { + g_autoptr(GString) buf = g_string_new(""); + cpu_synchronize_state(cpu); + cc->format_state(cpu, buf, flags); + qemu_fprintf(f, "%s", buf->str); + } +} + +void cpu_format_state(CPUState *cpu, GString *buf, int flags) +{ + CPUClass *cc = CPU_GET_CLASS(cpu); + + if (cc->format_state) { + cpu_synchronize_state(cpu); + cc->format_state(cpu, buf, flags); } } diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h index bc864564ce..1599ef9df3 100644 --- a/include/hw/core/cpu.h +++ b/include/hw/core/cpu.h @@ -91,7 +91,8 @@ struct SysemuCPUOps; * @reset_dump_flags: #CPUDumpFlags to use for reset logging. * @has_work: Callback for checking if there is work to do. * @memory_rw_debug: Callback for GDB memory access. - * @dump_state: Callback for dumping state. + * @dump_state: Callback for dumping state. Deprecated, use @format_state. + * @format_state: Callback for formatting state. * @get_arch_id: Callback for getting architecture-dependent CPU ID. * @set_pc: Callback for setting the Program Counter register. This * should have the semantics used by the target architecture when @@ -136,6 +137,7 @@ struct CPUClass { int (*memory_rw_debug)(CPUState *cpu, vaddr addr, uint8_t *buf, int len, bool is_write); void (*dump_state)(CPUState *cpu, FILE *, int flags); + void (*format_state)(CPUState *cpu, GString *buf, int flags); int64_t (*get_arch_id)(CPUState *cpu); void (*set_pc)(CPUState *cpu, vaddr value); int (*gdb_read_register)(CPUState *cpu, GByteArray *buf, int reg); @@ -537,6 +539,15 @@ enum CPUDumpFlags { */ void cpu_dump_state(CPUState *cpu, FILE *f, int flags); +/** + * cpu_format_state: + * @cpu: The CPU whose state is to be formatted. + * @buf: buffer to format state into + * + * Formats the CPU state. + */ +void cpu_format_state(CPUState *cpu, GString *buf, int flags); + #ifndef CONFIG_USER_ONLY /** * cpu_get_phys_page_attrs_debug: -- 2.31.1 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 3/5] target/i386: convert to use format_state instead of dump_state 2021-09-08 10:37 [PATCH 0/5] Stop adding HMP-only commands, allow QMP for all Daniel P. Berrangé 2021-09-08 10:37 ` [PATCH 1/5] docs/devel: document expectations for QAPI data modelling for QMP Daniel P. Berrangé 2021-09-08 10:37 ` [PATCH 2/5] hw/core: introduce 'format_state' callback to replace 'dump_state' Daniel P. Berrangé @ 2021-09-08 10:37 ` Daniel P. Berrangé 2021-09-08 12:17 ` Ján Tomko 2021-09-08 18:05 ` Eric Blake 2021-09-08 10:37 ` [PATCH 4/5] qapi: introduce x-query-registers QMP command Daniel P. Berrangé ` (3 subsequent siblings) 6 siblings, 2 replies; 28+ messages in thread From: Daniel P. Berrangé @ 2021-09-08 10:37 UTC (permalink / raw) To: qemu-devel Cc: Daniel P. Berrangé, Eduardo Habkost, Dr. David Alan Gilbert, Markus Armbruster, Eric Blake Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- target/i386/cpu-dump.c | 325 ++++++++++++++++++++++------------------- target/i386/cpu.c | 2 +- target/i386/cpu.h | 2 +- 3 files changed, 174 insertions(+), 155 deletions(-) diff --git a/target/i386/cpu-dump.c b/target/i386/cpu-dump.c index 02b635a52c..8e19485a20 100644 --- a/target/i386/cpu-dump.c +++ b/target/i386/cpu-dump.c @@ -94,41 +94,45 @@ static const char *cc_op_str[CC_OP_NB] = { }; static void -cpu_x86_dump_seg_cache(CPUX86State *env, FILE *f, +cpu_x86_dump_seg_cache(CPUX86State *env, GString *buf, const char *name, struct SegmentCache *sc) { #ifdef TARGET_X86_64 if (env->hflags & HF_CS64_MASK) { - qemu_fprintf(f, "%-3s=%04x %016" PRIx64 " %08x %08x", name, - sc->selector, sc->base, sc->limit, - sc->flags & 0x00ffff00); + g_string_append_printf(buf, "%-3s=%04x %016" PRIx64 " %08x %08x", name, + sc->selector, sc->base, sc->limit, + sc->flags & 0x00ffff00); } else #endif { - qemu_fprintf(f, "%-3s=%04x %08x %08x %08x", name, sc->selector, - (uint32_t)sc->base, sc->limit, - sc->flags & 0x00ffff00); + g_string_append_printf(buf, "%-3s=%04x %08x %08x %08x", + name, sc->selector, + (uint32_t)sc->base, sc->limit, + sc->flags & 0x00ffff00); } if (!(env->hflags & HF_PE_MASK) || !(sc->flags & DESC_P_MASK)) goto done; - qemu_fprintf(f, " DPL=%d ", + g_string_append_printf(buf, " DPL=%d ", (sc->flags & DESC_DPL_MASK) >> DESC_DPL_SHIFT); if (sc->flags & DESC_S_MASK) { if (sc->flags & DESC_CS_MASK) { - qemu_fprintf(f, (sc->flags & DESC_L_MASK) ? "CS64" : - ((sc->flags & DESC_B_MASK) ? "CS32" : "CS16")); - qemu_fprintf(f, " [%c%c", (sc->flags & DESC_C_MASK) ? 'C' : '-', - (sc->flags & DESC_R_MASK) ? 'R' : '-'); + g_string_append_printf(buf, (sc->flags & DESC_L_MASK) ? "CS64" : + ((sc->flags & DESC_B_MASK) ? "CS32" : "CS16")); + g_string_append_printf(buf, " [%c%c", + (sc->flags & DESC_C_MASK) ? 'C' : '-', + (sc->flags & DESC_R_MASK) ? 'R' : '-'); } else { - qemu_fprintf(f, (sc->flags & DESC_B_MASK + g_string_append_printf(buf, (sc->flags & DESC_B_MASK || env->hflags & HF_LMA_MASK) ? "DS " : "DS16"); - qemu_fprintf(f, " [%c%c", (sc->flags & DESC_E_MASK) ? 'E' : '-', - (sc->flags & DESC_W_MASK) ? 'W' : '-'); + g_string_append_printf(buf, " [%c%c", + (sc->flags & DESC_E_MASK) ? 'E' : '-', + (sc->flags & DESC_W_MASK) ? 'W' : '-'); } - qemu_fprintf(f, "%c]", (sc->flags & DESC_A_MASK) ? 'A' : '-'); + g_string_append_printf(buf, "%c]", + (sc->flags & DESC_A_MASK) ? 'A' : '-'); } else { static const char *sys_type_name[2][16] = { { /* 32 bit mode */ @@ -144,12 +148,12 @@ cpu_x86_dump_seg_cache(CPUX86State *env, FILE *f, "Reserved", "IntGate64", "TrapGate64" } }; - qemu_fprintf(f, "%s", - sys_type_name[(env->hflags & HF_LMA_MASK) ? 1 : 0] - [(sc->flags & DESC_TYPE_MASK) >> DESC_TYPE_SHIFT]); + g_string_append_printf(buf, "%s", + sys_type_name[(env->hflags & HF_LMA_MASK) ? 1 : 0] + [(sc->flags & DESC_TYPE_MASK) >> DESC_TYPE_SHIFT]); } done: - qemu_fprintf(f, "\n"); + g_string_append_printf(buf, "\n"); } #ifndef CONFIG_USER_ONLY @@ -344,7 +348,7 @@ void x86_cpu_dump_local_apic_state(CPUState *cs, int flags) #define DUMP_CODE_BYTES_TOTAL 50 #define DUMP_CODE_BYTES_BACKWARD 20 -void x86_cpu_dump_state(CPUState *cs, FILE *f, int flags) +void x86_cpu_format_state(CPUState *cs, GString *buf, int flags) { X86CPU *cpu = X86_CPU(cs); CPUX86State *env = &cpu->env; @@ -355,107 +359,116 @@ void x86_cpu_dump_state(CPUState *cs, FILE *f, int flags) eflags = cpu_compute_eflags(env); #ifdef TARGET_X86_64 if (env->hflags & HF_CS64_MASK) { - qemu_fprintf(f, "RAX=%016" PRIx64 " RBX=%016" PRIx64 " RCX=%016" PRIx64 " RDX=%016" PRIx64 "\n" - "RSI=%016" PRIx64 " RDI=%016" PRIx64 " RBP=%016" PRIx64 " RSP=%016" PRIx64 "\n" - "R8 =%016" PRIx64 " R9 =%016" PRIx64 " R10=%016" PRIx64 " R11=%016" PRIx64 "\n" - "R12=%016" PRIx64 " R13=%016" PRIx64 " R14=%016" PRIx64 " R15=%016" PRIx64 "\n" - "RIP=%016" PRIx64 " RFL=%08x [%c%c%c%c%c%c%c] CPL=%d II=%d A20=%d SMM=%d HLT=%d\n", - env->regs[R_EAX], - env->regs[R_EBX], - env->regs[R_ECX], - env->regs[R_EDX], - env->regs[R_ESI], - env->regs[R_EDI], - env->regs[R_EBP], - env->regs[R_ESP], - env->regs[8], - env->regs[9], - env->regs[10], - env->regs[11], - env->regs[12], - env->regs[13], - env->regs[14], - env->regs[15], - env->eip, eflags, - eflags & DF_MASK ? 'D' : '-', - eflags & CC_O ? 'O' : '-', - eflags & CC_S ? 'S' : '-', - eflags & CC_Z ? 'Z' : '-', - eflags & CC_A ? 'A' : '-', - eflags & CC_P ? 'P' : '-', - eflags & CC_C ? 'C' : '-', - env->hflags & HF_CPL_MASK, - (env->hflags >> HF_INHIBIT_IRQ_SHIFT) & 1, - (env->a20_mask >> 20) & 1, - (env->hflags >> HF_SMM_SHIFT) & 1, - cs->halted); + g_string_append_printf(buf, "RAX=%016" PRIx64 " RBX=%016" PRIx64 + " RCX=%016" PRIx64 " RDX=%016" PRIx64 "\n" + "RSI=%016" PRIx64 " RDI=%016" PRIx64 + " RBP=%016" PRIx64 " RSP=%016" PRIx64 "\n" + "R8 =%016" PRIx64 " R9 =%016" PRIx64 + " R10=%016" PRIx64 " R11=%016" PRIx64 "\n" + "R12=%016" PRIx64 " R13=%016" PRIx64 + " R14=%016" PRIx64 " R15=%016" PRIx64 "\n" + "RIP=%016" PRIx64 " RFL=%08x [%c%c%c%c%c%c%c] " + "CPL=%d II=%d A20=%d SMM=%d HLT=%d\n", + env->regs[R_EAX], + env->regs[R_EBX], + env->regs[R_ECX], + env->regs[R_EDX], + env->regs[R_ESI], + env->regs[R_EDI], + env->regs[R_EBP], + env->regs[R_ESP], + env->regs[8], + env->regs[9], + env->regs[10], + env->regs[11], + env->regs[12], + env->regs[13], + env->regs[14], + env->regs[15], + env->eip, eflags, + eflags & DF_MASK ? 'D' : '-', + eflags & CC_O ? 'O' : '-', + eflags & CC_S ? 'S' : '-', + eflags & CC_Z ? 'Z' : '-', + eflags & CC_A ? 'A' : '-', + eflags & CC_P ? 'P' : '-', + eflags & CC_C ? 'C' : '-', + env->hflags & HF_CPL_MASK, + (env->hflags >> HF_INHIBIT_IRQ_SHIFT) & 1, + (env->a20_mask >> 20) & 1, + (env->hflags >> HF_SMM_SHIFT) & 1, + cs->halted); } else #endif { - qemu_fprintf(f, "EAX=%08x EBX=%08x ECX=%08x EDX=%08x\n" - "ESI=%08x EDI=%08x EBP=%08x ESP=%08x\n" - "EIP=%08x EFL=%08x [%c%c%c%c%c%c%c] CPL=%d II=%d A20=%d SMM=%d HLT=%d\n", - (uint32_t)env->regs[R_EAX], - (uint32_t)env->regs[R_EBX], - (uint32_t)env->regs[R_ECX], - (uint32_t)env->regs[R_EDX], - (uint32_t)env->regs[R_ESI], - (uint32_t)env->regs[R_EDI], - (uint32_t)env->regs[R_EBP], - (uint32_t)env->regs[R_ESP], - (uint32_t)env->eip, eflags, - eflags & DF_MASK ? 'D' : '-', - eflags & CC_O ? 'O' : '-', - eflags & CC_S ? 'S' : '-', - eflags & CC_Z ? 'Z' : '-', - eflags & CC_A ? 'A' : '-', - eflags & CC_P ? 'P' : '-', - eflags & CC_C ? 'C' : '-', - env->hflags & HF_CPL_MASK, - (env->hflags >> HF_INHIBIT_IRQ_SHIFT) & 1, - (env->a20_mask >> 20) & 1, - (env->hflags >> HF_SMM_SHIFT) & 1, - cs->halted); + g_string_append_printf(buf, "EAX=%08x EBX=%08x ECX=%08x EDX=%08x\n" + "ESI=%08x EDI=%08x EBP=%08x ESP=%08x\n" + "EIP=%08x EFL=%08x [%c%c%c%c%c%c%c] " + "CPL=%d II=%d A20=%d SMM=%d HLT=%d\n", + (uint32_t)env->regs[R_EAX], + (uint32_t)env->regs[R_EBX], + (uint32_t)env->regs[R_ECX], + (uint32_t)env->regs[R_EDX], + (uint32_t)env->regs[R_ESI], + (uint32_t)env->regs[R_EDI], + (uint32_t)env->regs[R_EBP], + (uint32_t)env->regs[R_ESP], + (uint32_t)env->eip, eflags, + eflags & DF_MASK ? 'D' : '-', + eflags & CC_O ? 'O' : '-', + eflags & CC_S ? 'S' : '-', + eflags & CC_Z ? 'Z' : '-', + eflags & CC_A ? 'A' : '-', + eflags & CC_P ? 'P' : '-', + eflags & CC_C ? 'C' : '-', + env->hflags & HF_CPL_MASK, + (env->hflags >> HF_INHIBIT_IRQ_SHIFT) & 1, + (env->a20_mask >> 20) & 1, + (env->hflags >> HF_SMM_SHIFT) & 1, + cs->halted); } for(i = 0; i < 6; i++) { - cpu_x86_dump_seg_cache(env, f, seg_name[i], &env->segs[i]); + cpu_x86_dump_seg_cache(env, buf, seg_name[i], &env->segs[i]); } - cpu_x86_dump_seg_cache(env, f, "LDT", &env->ldt); - cpu_x86_dump_seg_cache(env, f, "TR", &env->tr); + cpu_x86_dump_seg_cache(env, buf, "LDT", &env->ldt); + cpu_x86_dump_seg_cache(env, buf, "TR", &env->tr); #ifdef TARGET_X86_64 if (env->hflags & HF_LMA_MASK) { - qemu_fprintf(f, "GDT= %016" PRIx64 " %08x\n", - env->gdt.base, env->gdt.limit); - qemu_fprintf(f, "IDT= %016" PRIx64 " %08x\n", - env->idt.base, env->idt.limit); - qemu_fprintf(f, "CR0=%08x CR2=%016" PRIx64 " CR3=%016" PRIx64 " CR4=%08x\n", - (uint32_t)env->cr[0], - env->cr[2], - env->cr[3], - (uint32_t)env->cr[4]); + g_string_append_printf(buf, "GDT= %016" PRIx64 " %08x\n", + env->gdt.base, env->gdt.limit); + g_string_append_printf(buf, "IDT= %016" PRIx64 " %08x\n", + env->idt.base, env->idt.limit); + g_string_append_printf(buf, "CR0=%08x CR2=%016" PRIx64 + " CR3=%016" PRIx64 " CR4=%08x\n", + (uint32_t)env->cr[0], + env->cr[2], + env->cr[3], + (uint32_t)env->cr[4]); for(i = 0; i < 4; i++) - qemu_fprintf(f, "DR%d=%016" PRIx64 " ", i, env->dr[i]); - qemu_fprintf(f, "\nDR6=%016" PRIx64 " DR7=%016" PRIx64 "\n", - env->dr[6], env->dr[7]); + g_string_append_printf(buf, "DR%d=%016" PRIx64 " ", i, env->dr[i]); + g_string_append_printf(buf, "\nDR6=%016" PRIx64 " DR7=%016" PRIx64 "\n", + env->dr[6], env->dr[7]); } else #endif { - qemu_fprintf(f, "GDT= %08x %08x\n", - (uint32_t)env->gdt.base, env->gdt.limit); - qemu_fprintf(f, "IDT= %08x %08x\n", - (uint32_t)env->idt.base, env->idt.limit); - qemu_fprintf(f, "CR0=%08x CR2=%08x CR3=%08x CR4=%08x\n", - (uint32_t)env->cr[0], - (uint32_t)env->cr[2], - (uint32_t)env->cr[3], - (uint32_t)env->cr[4]); + g_string_append_printf(buf, "GDT= %08x %08x\n", + (uint32_t)env->gdt.base, env->gdt.limit); + g_string_append_printf(buf, "IDT= %08x %08x\n", + (uint32_t)env->idt.base, env->idt.limit); + g_string_append_printf(buf, "CR0=%08x CR2=%08x CR3=%08x CR4=%08x\n", + (uint32_t)env->cr[0], + (uint32_t)env->cr[2], + (uint32_t)env->cr[3], + (uint32_t)env->cr[4]); for(i = 0; i < 4; i++) { - qemu_fprintf(f, "DR%d=" TARGET_FMT_lx " ", i, env->dr[i]); + g_string_append_printf(buf, "DR%d=" TARGET_FMT_lx + " ", i, env->dr[i]); } - qemu_fprintf(f, "\nDR6=" TARGET_FMT_lx " DR7=" TARGET_FMT_lx "\n", - env->dr[6], env->dr[7]); + g_string_append_printf(buf, "\nDR6=" TARGET_FMT_lx + " DR7=" TARGET_FMT_lx "\n", + env->dr[6], env->dr[7]); } if (flags & CPU_DUMP_CCOP) { if ((unsigned)env->cc_op < CC_OP_NB) @@ -464,18 +477,19 @@ void x86_cpu_dump_state(CPUState *cs, FILE *f, int flags) snprintf(cc_op_name, sizeof(cc_op_name), "[%d]", env->cc_op); #ifdef TARGET_X86_64 if (env->hflags & HF_CS64_MASK) { - qemu_fprintf(f, "CCS=%016" PRIx64 " CCD=%016" PRIx64 " CCO=%-8s\n", - env->cc_src, env->cc_dst, - cc_op_name); + g_string_append_printf(buf, "CCS=%016" PRIx64 + " CCD=%016" PRIx64 " CCO=%-8s\n", + env->cc_src, env->cc_dst, + cc_op_name); } else #endif { - qemu_fprintf(f, "CCS=%08x CCD=%08x CCO=%-8s\n", - (uint32_t)env->cc_src, (uint32_t)env->cc_dst, - cc_op_name); + g_string_append_printf(buf, "CCS=%08x CCD=%08x CCO=%-8s\n", + (uint32_t)env->cc_src, (uint32_t)env->cc_dst, + cc_op_name); } } - qemu_fprintf(f, "EFER=%016" PRIx64 "\n", env->efer); + g_string_append_printf(buf, "EFER=%016" PRIx64 "\n", env->efer); if (flags & CPU_DUMP_FPU) { int fptag; const uint64_t avx512_mask = XSTATE_OPMASK_MASK | \ @@ -488,64 +502,68 @@ void x86_cpu_dump_state(CPUState *cs, FILE *f, int flags) fptag |= ((!env->fptags[i]) << i); } update_mxcsr_from_sse_status(env); - qemu_fprintf(f, "FCW=%04x FSW=%04x [ST=%d] FTW=%02x MXCSR=%08x\n", - env->fpuc, - (env->fpus & ~0x3800) | (env->fpstt & 0x7) << 11, - env->fpstt, - fptag, - env->mxcsr); + g_string_append_printf( + buf, "FCW=%04x FSW=%04x [ST=%d] FTW=%02x MXCSR=%08x\n", + env->fpuc, + (env->fpus & ~0x3800) | (env->fpstt & 0x7) << 11, + env->fpstt, + fptag, + env->mxcsr); for(i=0;i<8;i++) { CPU_LDoubleU u; u.d = env->fpregs[i].d; - qemu_fprintf(f, "FPR%d=%016" PRIx64 " %04x", - i, u.l.lower, u.l.upper); + g_string_append_printf(buf, "FPR%d=%016" PRIx64 " %04x", + i, u.l.lower, u.l.upper); if ((i & 1) == 1) - qemu_fprintf(f, "\n"); + g_string_append_printf(buf, "\n"); else - qemu_fprintf(f, " "); + g_string_append_printf(buf, " "); } - if ((env->xcr0 & avx512_mask) == avx512_mask) { /* XSAVE enabled AVX512 */ for (i = 0; i < NB_OPMASK_REGS; i++) { - qemu_fprintf(f, "Opmask%02d=%016"PRIx64"%s", i, - env->opmask_regs[i], ((i & 3) == 3) ? "\n" : " "); + g_string_append_printf(buf, "Opmask%02d=%016"PRIx64"%s", i, + env->opmask_regs[i], + ((i & 3) == 3) ? "\n" : " "); } nb = (env->hflags & HF_CS64_MASK) ? 32 : 8; for (i = 0; i < nb; i++) { - qemu_fprintf(f, "ZMM%02d=%016"PRIx64" %016"PRIx64" %016"PRIx64 - " %016"PRIx64" %016"PRIx64" %016"PRIx64 - " %016"PRIx64" %016"PRIx64"\n", - i, - env->xmm_regs[i].ZMM_Q(7), - env->xmm_regs[i].ZMM_Q(6), - env->xmm_regs[i].ZMM_Q(5), - env->xmm_regs[i].ZMM_Q(4), - env->xmm_regs[i].ZMM_Q(3), - env->xmm_regs[i].ZMM_Q(2), - env->xmm_regs[i].ZMM_Q(1), - env->xmm_regs[i].ZMM_Q(0)); + g_string_append_printf(buf, "ZMM%02d=%016"PRIx64 + " %016"PRIx64" %016"PRIx64 + " %016"PRIx64" %016"PRIx64" %016"PRIx64 + " %016"PRIx64" %016"PRIx64"\n", + i, + env->xmm_regs[i].ZMM_Q(7), + env->xmm_regs[i].ZMM_Q(6), + env->xmm_regs[i].ZMM_Q(5), + env->xmm_regs[i].ZMM_Q(4), + env->xmm_regs[i].ZMM_Q(3), + env->xmm_regs[i].ZMM_Q(2), + env->xmm_regs[i].ZMM_Q(1), + env->xmm_regs[i].ZMM_Q(0)); } } else if ((env->xcr0 & avx_mask) == avx_mask) { /* XSAVE enabled AVX */ nb = env->hflags & HF_CS64_MASK ? 16 : 8; for (i = 0; i < nb; i++) { - qemu_fprintf(f, "YMM%02d=%016"PRIx64" %016"PRIx64" %016"PRIx64 - " %016"PRIx64"\n", i, - env->xmm_regs[i].ZMM_Q(3), - env->xmm_regs[i].ZMM_Q(2), - env->xmm_regs[i].ZMM_Q(1), - env->xmm_regs[i].ZMM_Q(0)); + g_string_append_printf(buf, "YMM%02d=%016"PRIx64 + " %016"PRIx64" %016"PRIx64 + " %016"PRIx64"\n", i, + env->xmm_regs[i].ZMM_Q(3), + env->xmm_regs[i].ZMM_Q(2), + env->xmm_regs[i].ZMM_Q(1), + env->xmm_regs[i].ZMM_Q(0)); } } else { /* SSE and below cases */ nb = env->hflags & HF_CS64_MASK ? 16 : 8; for (i = 0; i < nb; i++) { - qemu_fprintf(f, "XMM%02d=%016"PRIx64" %016"PRIx64"%s", - i, - env->xmm_regs[i].ZMM_Q(1), - env->xmm_regs[i].ZMM_Q(0), - (i & 1) ? "\n" : " "); + g_string_append_printf(buf, + "XMM%02d=%016"PRIx64" %016"PRIx64"%s", + i, + env->xmm_regs[i].ZMM_Q(1), + env->xmm_regs[i].ZMM_Q(0), + (i & 1) ? "\n" : " "); } } } @@ -555,16 +573,17 @@ void x86_cpu_dump_state(CPUState *cs, FILE *f, int flags) uint8_t code; char codestr[3]; - qemu_fprintf(f, "Code="); + g_string_append_printf(buf, "Code="); for (i = 0; i < DUMP_CODE_BYTES_TOTAL; i++) { if (cpu_memory_rw_debug(cs, base - offs + i, &code, 1, 0) == 0) { snprintf(codestr, sizeof(codestr), "%02x", code); } else { snprintf(codestr, sizeof(codestr), "??"); } - qemu_fprintf(f, "%s%s%s%s", i > 0 ? " " : "", - i == offs ? "<" : "", codestr, i == offs ? ">" : ""); + g_string_append_printf(buf, "%s%s%s%s", i > 0 ? " " : "", + i == offs ? "<" : "", codestr, + i == offs ? ">" : ""); } - qemu_fprintf(f, "\n"); + g_string_append_printf(buf, "\n"); } } diff --git a/target/i386/cpu.c b/target/i386/cpu.c index 97e250e876..f31a304abb 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -6757,7 +6757,7 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data) cc->class_by_name = x86_cpu_class_by_name; cc->parse_features = x86_cpu_parse_featurestr; cc->has_work = x86_cpu_has_work; - cc->dump_state = x86_cpu_dump_state; + cc->format_state = x86_cpu_format_state; cc->set_pc = x86_cpu_set_pc; cc->gdb_read_register = x86_cpu_gdb_read_register; cc->gdb_write_register = x86_cpu_gdb_write_register; diff --git a/target/i386/cpu.h b/target/i386/cpu.h index 6c50d3ab4f..01ca4e715d 100644 --- a/target/i386/cpu.h +++ b/target/i386/cpu.h @@ -1821,7 +1821,7 @@ int x86_cpu_write_elf32_qemunote(WriteCoreDumpFunction f, CPUState *cpu, void x86_cpu_get_memory_mapping(CPUState *cpu, MemoryMappingList *list, Error **errp); -void x86_cpu_dump_state(CPUState *cs, FILE *f, int flags); +void x86_cpu_format_state(CPUState *cs, GString *buf, int flags); hwaddr x86_cpu_get_phys_page_attrs_debug(CPUState *cpu, vaddr addr, MemTxAttrs *attrs); -- 2.31.1 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 3/5] target/i386: convert to use format_state instead of dump_state 2021-09-08 10:37 ` [PATCH 3/5] target/i386: convert to use format_state instead of dump_state Daniel P. Berrangé @ 2021-09-08 12:17 ` Ján Tomko 2021-09-08 18:05 ` Eric Blake 1 sibling, 0 replies; 28+ messages in thread From: Ján Tomko @ 2021-09-08 12:17 UTC (permalink / raw) To: Daniel P. Berrangé Cc: Markus Armbruster, Eric Blake, qemu-devel, Dr. David Alan Gilbert, Eduardo Habkost [-- Attachment #1: Type: text/plain, Size: 2302 bytes --] On a Wednesday in 2021, Daniel P. Berrangé wrote: >Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> >--- > target/i386/cpu-dump.c | 325 ++++++++++++++++++++++------------------- > target/i386/cpu.c | 2 +- > target/i386/cpu.h | 2 +- > 3 files changed, 174 insertions(+), 155 deletions(-) > >diff --git a/target/i386/cpu-dump.c b/target/i386/cpu-dump.c >index 02b635a52c..8e19485a20 100644 >--- a/target/i386/cpu-dump.c >+++ b/target/i386/cpu-dump.c [...] >@@ -355,107 +359,116 @@ void x86_cpu_dump_state(CPUState *cs, FILE *f, int flags) [...] > { >- qemu_fprintf(f, "GDT= %08x %08x\n", >- (uint32_t)env->gdt.base, env->gdt.limit); >- qemu_fprintf(f, "IDT= %08x %08x\n", >- (uint32_t)env->idt.base, env->idt.limit); >- qemu_fprintf(f, "CR0=%08x CR2=%08x CR3=%08x CR4=%08x\n", >- (uint32_t)env->cr[0], >- (uint32_t)env->cr[2], >- (uint32_t)env->cr[3], >- (uint32_t)env->cr[4]); >+ g_string_append_printf(buf, "GDT= %08x %08x\n", >+ (uint32_t)env->gdt.base, env->gdt.limit); >+ g_string_append_printf(buf, "IDT= %08x %08x\n", >+ (uint32_t)env->idt.base, env->idt.limit); >+ g_string_append_printf(buf, "CR0=%08x CR2=%08x CR3=%08x CR4=%08x\n", >+ (uint32_t)env->cr[0], >+ (uint32_t)env->cr[2], >+ (uint32_t)env->cr[3], >+ (uint32_t)env->cr[4]); > for(i = 0; i < 4; i++) { >- qemu_fprintf(f, "DR%d=" TARGET_FMT_lx " ", i, env->dr[i]); >+ g_string_append_printf(buf, "DR%d=" TARGET_FMT_lx >+ " ", i, env->dr[i]); The formatting string can comfortably fit on the first line here. Jano > } >- qemu_fprintf(f, "\nDR6=" TARGET_FMT_lx " DR7=" TARGET_FMT_lx "\n", >- env->dr[6], env->dr[7]); >+ g_string_append_printf(buf, "\nDR6=" TARGET_FMT_lx >+ " DR7=" TARGET_FMT_lx "\n", >+ env->dr[6], env->dr[7]); [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/5] target/i386: convert to use format_state instead of dump_state 2021-09-08 10:37 ` [PATCH 3/5] target/i386: convert to use format_state instead of dump_state Daniel P. Berrangé 2021-09-08 12:17 ` Ján Tomko @ 2021-09-08 18:05 ` Eric Blake 2021-09-08 22:06 ` Daniel P. Berrangé 1 sibling, 1 reply; 28+ messages in thread From: Eric Blake @ 2021-09-08 18:05 UTC (permalink / raw) To: Daniel P. Berrangé Cc: Markus Armbruster, Eduardo Habkost, qemu-devel, Dr. David Alan Gilbert On Wed, Sep 08, 2021 at 11:37:09AM +0100, Daniel P. Berrangé wrote: > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > --- > target/i386/cpu-dump.c | 325 ++++++++++++++++++++++------------------- > target/i386/cpu.c | 2 +- > target/i386/cpu.h | 2 +- > 3 files changed, 174 insertions(+), 155 deletions(-) > > diff --git a/target/i386/cpu-dump.c b/target/i386/cpu-dump.c > index 02b635a52c..8e19485a20 100644 > --- a/target/i386/cpu-dump.c > +++ b/target/i386/cpu-dump.c > @@ -94,41 +94,45 @@ static const char *cc_op_str[CC_OP_NB] = { > }; > > static void > -cpu_x86_dump_seg_cache(CPUX86State *env, FILE *f, > +cpu_x86_dump_seg_cache(CPUX86State *env, GString *buf, > const char *name, struct SegmentCache *sc) > { > #ifdef TARGET_X86_64 > if (env->hflags & HF_CS64_MASK) { > - qemu_fprintf(f, "%-3s=%04x %016" PRIx64 " %08x %08x", name, > - sc->selector, sc->base, sc->limit, > - sc->flags & 0x00ffff00); > + g_string_append_printf(buf, "%-3s=%04x %016" PRIx64 " %08x %08x", name, > + sc->selector, sc->base, sc->limit, > + sc->flags & 0x00ffff00); Did you consider using open_memstream() to get a FILE* that can then be passed into these callbacks unchanged, rather than rewriting all the callbacks to a new signature? Then again, I like the GString signature better than FILE*, even if it makes for longer lines. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/5] target/i386: convert to use format_state instead of dump_state 2021-09-08 18:05 ` Eric Blake @ 2021-09-08 22:06 ` Daniel P. Berrangé 2021-09-09 9:55 ` Daniel P. Berrangé 0 siblings, 1 reply; 28+ messages in thread From: Daniel P. Berrangé @ 2021-09-08 22:06 UTC (permalink / raw) To: Eric Blake Cc: Markus Armbruster, Eduardo Habkost, qemu-devel, Dr. David Alan Gilbert On Wed, Sep 08, 2021 at 01:05:13PM -0500, Eric Blake wrote: > On Wed, Sep 08, 2021 at 11:37:09AM +0100, Daniel P. Berrangé wrote: > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > > --- > > target/i386/cpu-dump.c | 325 ++++++++++++++++++++++------------------- > > target/i386/cpu.c | 2 +- > > target/i386/cpu.h | 2 +- > > 3 files changed, 174 insertions(+), 155 deletions(-) > > > > diff --git a/target/i386/cpu-dump.c b/target/i386/cpu-dump.c > > index 02b635a52c..8e19485a20 100644 > > --- a/target/i386/cpu-dump.c > > +++ b/target/i386/cpu-dump.c > > @@ -94,41 +94,45 @@ static const char *cc_op_str[CC_OP_NB] = { > > }; > > > > static void > > -cpu_x86_dump_seg_cache(CPUX86State *env, FILE *f, > > +cpu_x86_dump_seg_cache(CPUX86State *env, GString *buf, > > const char *name, struct SegmentCache *sc) > > { > > #ifdef TARGET_X86_64 > > if (env->hflags & HF_CS64_MASK) { > > - qemu_fprintf(f, "%-3s=%04x %016" PRIx64 " %08x %08x", name, > > - sc->selector, sc->base, sc->limit, > > - sc->flags & 0x00ffff00); > > + g_string_append_printf(buf, "%-3s=%04x %016" PRIx64 " %08x %08x", name, > > + sc->selector, sc->base, sc->limit, > > + sc->flags & 0x00ffff00); > > Did you consider using open_memstream() to get a FILE* that can then > be passed into these callbacks unchanged, rather than rewriting all > the callbacks to a new signature? That is certainly an option, but it wouldn't eliminate the need to do a rewrite. I would still want to replace qemu_fprintf with fprintf in that scenario. It is desirable to be able to eliminate the QEMU specific printf wrappers which only exist because they need to treat a NULL FILE* object as an indication to output to the HMP chardev. Admittedly that would result in shorter lines than today. > Then again, I like the GString signature better than FILE*, even if it > makes for longer lines. Yes, the verbosity is not ideal. I like the GString API as a general purpose API for formatting text output to a buffer overall. I don't feel too strongly either way though, as long as we get to a place where we eliminate the custom QEMU printf wrappers that integrate with the monitor APIs. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/5] target/i386: convert to use format_state instead of dump_state 2021-09-08 22:06 ` Daniel P. Berrangé @ 2021-09-09 9:55 ` Daniel P. Berrangé 0 siblings, 0 replies; 28+ messages in thread From: Daniel P. Berrangé @ 2021-09-09 9:55 UTC (permalink / raw) To: Eric Blake, Markus Armbruster, Eduardo Habkost, qemu-devel, Dr. David Alan Gilbert On Wed, Sep 08, 2021 at 11:06:33PM +0100, Daniel P. Berrangé wrote: > On Wed, Sep 08, 2021 at 01:05:13PM -0500, Eric Blake wrote: > > On Wed, Sep 08, 2021 at 11:37:09AM +0100, Daniel P. Berrangé wrote: > > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > > > --- > > > target/i386/cpu-dump.c | 325 ++++++++++++++++++++++------------------- > > > target/i386/cpu.c | 2 +- > > > target/i386/cpu.h | 2 +- > > > 3 files changed, 174 insertions(+), 155 deletions(-) > > > > > > diff --git a/target/i386/cpu-dump.c b/target/i386/cpu-dump.c > > > index 02b635a52c..8e19485a20 100644 > > > --- a/target/i386/cpu-dump.c > > > +++ b/target/i386/cpu-dump.c > > > @@ -94,41 +94,45 @@ static const char *cc_op_str[CC_OP_NB] = { > > > }; > > > > > > static void > > > -cpu_x86_dump_seg_cache(CPUX86State *env, FILE *f, > > > +cpu_x86_dump_seg_cache(CPUX86State *env, GString *buf, > > > const char *name, struct SegmentCache *sc) > > > { > > > #ifdef TARGET_X86_64 > > > if (env->hflags & HF_CS64_MASK) { > > > - qemu_fprintf(f, "%-3s=%04x %016" PRIx64 " %08x %08x", name, > > > - sc->selector, sc->base, sc->limit, > > > - sc->flags & 0x00ffff00); > > > + g_string_append_printf(buf, "%-3s=%04x %016" PRIx64 " %08x %08x", name, > > > + sc->selector, sc->base, sc->limit, > > > + sc->flags & 0x00ffff00); > > > > Did you consider using open_memstream() to get a FILE* that can then > > be passed into these callbacks unchanged, rather than rewriting all > > the callbacks to a new signature? > > That is certainly an option, but it wouldn't eliminate the need to do > a rewrite. I would still want to replace qemu_fprintf with fprintf in > that scenario. It is desirable to be able to eliminate the QEMU > specific printf wrappers which only exist because they need to treat > a NULL FILE* object as an indication to output to the HMP chardev. > Admittedly that would result in shorter lines than today. > > > Then again, I like the GString signature better than FILE*, even if it > > makes for longer lines. > > Yes, the verbosity is not ideal. I like the GString API as a general > purpose API for formatting text output to a buffer overall. > > I don't feel too strongly either way though, as long as we get to a place > where we eliminate the custom QEMU printf wrappers that integrate with > the monitor APIs. I forgot that open_memstream is not portable. The portable alternative is fmemopen but that needs to know the buffer size upfront which is too unpleasant to use. So GString is the better portable option. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 4/5] qapi: introduce x-query-registers QMP command 2021-09-08 10:37 [PATCH 0/5] Stop adding HMP-only commands, allow QMP for all Daniel P. Berrangé ` (2 preceding siblings ...) 2021-09-08 10:37 ` [PATCH 3/5] target/i386: convert to use format_state instead of dump_state Daniel P. Berrangé @ 2021-09-08 10:37 ` Daniel P. Berrangé 2021-09-08 18:06 ` Eric Blake 2021-09-09 9:05 ` Markus Armbruster 2021-09-08 10:37 ` [PATCH 5/5] monitor: rewrite 'info registers' in terms of 'x-query-registers' Daniel P. Berrangé ` (2 subsequent siblings) 6 siblings, 2 replies; 28+ messages in thread From: Daniel P. Berrangé @ 2021-09-08 10:37 UTC (permalink / raw) To: qemu-devel Cc: Daniel P. Berrangé, Eduardo Habkost, Dr. David Alan Gilbert, Markus Armbruster, Eric Blake This is a counterpart to the HMP "info registers" command. It is being added with an "x-" prefix because this QMP command is intended as an adhoc debugging tool and will thus not be modelled in QAPI as fully structured data, nor will it have long term guaranteed stability. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- hw/core/machine-qmp-cmds.c | 28 ++++++++++++++++++++++++++++ qapi/machine.json | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+) diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c index 216fdfaf3a..0d9943ff60 100644 --- a/hw/core/machine-qmp-cmds.c +++ b/hw/core/machine-qmp-cmds.c @@ -204,3 +204,31 @@ MemdevList *qmp_query_memdev(Error **errp) object_child_foreach(obj, query_memdev, &list); return list; } + +RegisterInfo *qmp_x_query_registers(bool has_cpu, int64_t cpu, Error **errp) +{ + RegisterInfo *info = g_new0(RegisterInfo, 1); + g_autoptr(GString) buf = g_string_new(""); + CPUState *cs = NULL, *tmp; + + if (has_cpu) { + CPU_FOREACH(tmp) { + if (cpu == tmp->cpu_index) { + cs = tmp; + } + } + if (!cs) { + error_setg(errp, "CPU %"PRId64" not available", cpu); + return NULL; + } + cpu_format_state(cs, buf, CPU_DUMP_FPU); + } else { + CPU_FOREACH(cs) { + g_string_append_printf(buf, "\nCPU#%d\n", cs->cpu_index); + cpu_format_state(cs, buf, CPU_DUMP_FPU); + } + } + + info->state = g_steal_pointer(&buf->str); + return info; +} diff --git a/qapi/machine.json b/qapi/machine.json index 157712f006..27b922f2ce 100644 --- a/qapi/machine.json +++ b/qapi/machine.json @@ -1312,3 +1312,40 @@ '*cores': 'int', '*threads': 'int', '*maxcpus': 'int' } } + +## +# @RegisterParams: +# +# Information about the CPU to query state of +# +# @cpu: the CPU number to query. If omitted, queries all CPUs +# +# Since: 6.2.0 +# +## +{ 'struct': 'RegisterParams', 'data': {'*cpu': 'int' } } + +## +# @RegisterInfo: +# +# Information about the CPU state +# +# @state: the CPU state in an architecture specific format +# +# Since: 6.2.0 +# +## +{ 'struct': 'RegisterInfo', 'data': {'state': 'str' } } + +## +# @x-query-registers: +# +# Return information on the CPU registers +# +# Returns: the CPU state +# +# Since: 6.2.0 +## +{ 'command': 'x-query-registers', + 'data': 'RegisterParams', + 'returns': 'RegisterInfo' } -- 2.31.1 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 4/5] qapi: introduce x-query-registers QMP command 2021-09-08 10:37 ` [PATCH 4/5] qapi: introduce x-query-registers QMP command Daniel P. Berrangé @ 2021-09-08 18:06 ` Eric Blake 2021-09-09 9:05 ` Markus Armbruster 1 sibling, 0 replies; 28+ messages in thread From: Eric Blake @ 2021-09-08 18:06 UTC (permalink / raw) To: Daniel P. Berrangé Cc: Markus Armbruster, Eduardo Habkost, qemu-devel, Dr. David Alan Gilbert On Wed, Sep 08, 2021 at 11:37:10AM +0100, Daniel P. Berrangé wrote: > This is a counterpart to the HMP "info registers" command. It is being > added with an "x-" prefix because this QMP command is intended as an > adhoc debugging tool and will thus not be modelled in QAPI as fully ad hoc > structured data, nor will it have long term guaranteed stability. > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > --- > hw/core/machine-qmp-cmds.c | 28 ++++++++++++++++++++++++++++ > qapi/machine.json | 37 +++++++++++++++++++++++++++++++++++++ > 2 files changed, 65 insertions(+) > > +++ b/qapi/machine.json > @@ -1312,3 +1312,40 @@ > '*cores': 'int', > '*threads': 'int', > '*maxcpus': 'int' } } > + > +## > +# @RegisterParams: > +# > +# Information about the CPU to query state of > +# > +# @cpu: the CPU number to query. If omitted, queries all CPUs > +# > +# Since: 6.2.0 Consensus seems to be "6.2", not "6.2.0". -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 4/5] qapi: introduce x-query-registers QMP command 2021-09-08 10:37 ` [PATCH 4/5] qapi: introduce x-query-registers QMP command Daniel P. Berrangé 2021-09-08 18:06 ` Eric Blake @ 2021-09-09 9:05 ` Markus Armbruster 2021-09-09 10:01 ` Daniel P. Berrangé 1 sibling, 1 reply; 28+ messages in thread From: Markus Armbruster @ 2021-09-09 9:05 UTC (permalink / raw) To: Daniel P. Berrangé Cc: Eduardo Habkost, Eric Blake, qemu-devel, Dr. David Alan Gilbert Daniel P. Berrangé <berrange@redhat.com> writes: > This is a counterpart to the HMP "info registers" command. It is being > added with an "x-" prefix because this QMP command is intended as an > adhoc debugging tool and will thus not be modelled in QAPI as fully > structured data, nor will it have long term guaranteed stability. > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > --- > hw/core/machine-qmp-cmds.c | 28 ++++++++++++++++++++++++++++ > qapi/machine.json | 37 +++++++++++++++++++++++++++++++++++++ > 2 files changed, 65 insertions(+) > > diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c > index 216fdfaf3a..0d9943ff60 100644 > --- a/hw/core/machine-qmp-cmds.c > +++ b/hw/core/machine-qmp-cmds.c > @@ -204,3 +204,31 @@ MemdevList *qmp_query_memdev(Error **errp) > object_child_foreach(obj, query_memdev, &list); > return list; > } > + > +RegisterInfo *qmp_x_query_registers(bool has_cpu, int64_t cpu, Error **errp) > +{ > + RegisterInfo *info = g_new0(RegisterInfo, 1); > + g_autoptr(GString) buf = g_string_new(""); > + CPUState *cs = NULL, *tmp; > + > + if (has_cpu) { > + CPU_FOREACH(tmp) { > + if (cpu == tmp->cpu_index) { > + cs = tmp; > + } > + } > + if (!cs) { > + error_setg(errp, "CPU %"PRId64" not available", cpu); > + return NULL; > + } > + cpu_format_state(cs, buf, CPU_DUMP_FPU); > + } else { > + CPU_FOREACH(cs) { > + g_string_append_printf(buf, "\nCPU#%d\n", cs->cpu_index); > + cpu_format_state(cs, buf, CPU_DUMP_FPU); > + } > + } > + > + info->state = g_steal_pointer(&buf->str); > + return info; > +} > diff --git a/qapi/machine.json b/qapi/machine.json > index 157712f006..27b922f2ce 100644 > --- a/qapi/machine.json > +++ b/qapi/machine.json > @@ -1312,3 +1312,40 @@ > '*cores': 'int', > '*threads': 'int', > '*maxcpus': 'int' } } > + > +## > +# @RegisterParams: > +# > +# Information about the CPU to query state of > +# > +# @cpu: the CPU number to query. If omitted, queries all CPUs > +# > +# Since: 6.2.0 > +# > +## > +{ 'struct': 'RegisterParams', 'data': {'*cpu': 'int' } } > + > +## > +# @RegisterInfo: > +# > +# Information about the CPU state > +# > +# @state: the CPU state in an architecture specific format > +# > +# Since: 6.2.0 > +# > +## > +{ 'struct': 'RegisterInfo', 'data': {'state': 'str' } } > + > +## > +# @x-query-registers: > +# > +# Return information on the CPU registers > +# > +# Returns: the CPU state > +# > +# Since: 6.2.0 > +## > +{ 'command': 'x-query-registers', > + 'data': 'RegisterParams', Unless you have further uses of RegisterParams in mind, use an implicit type: 'data': { '*cpu': 'int' } } > + 'returns': 'RegisterInfo' } I'd like us to adopt a convention for commands returning formatted text for human consumption. Like so: 'returns': 'HumanReadableText' } with the obvious ## # @HumanReadableText: # # @human-readable-text: Formatted output intended for humans. # # Since: 6.2.0 ## { 'struct': 'HumanReadableText', 'data': { 'human-readable-text': 'str' } } When the output needs explaining, do that in the command's doc string. I figure ## # @x-query-registers: # # Returns information about the CPU state # # Returns: CPU state in an architecture-specific format # # Since: 6.2.0 ## would do in this case. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 4/5] qapi: introduce x-query-registers QMP command 2021-09-09 9:05 ` Markus Armbruster @ 2021-09-09 10:01 ` Daniel P. Berrangé 0 siblings, 0 replies; 28+ messages in thread From: Daniel P. Berrangé @ 2021-09-09 10:01 UTC (permalink / raw) To: Markus Armbruster Cc: Eduardo Habkost, Eric Blake, qemu-devel, Dr. David Alan Gilbert On Thu, Sep 09, 2021 at 11:05:20AM +0200, Markus Armbruster wrote: > Daniel P. Berrangé <berrange@redhat.com> writes: > > > This is a counterpart to the HMP "info registers" command. It is being > > added with an "x-" prefix because this QMP command is intended as an > > adhoc debugging tool and will thus not be modelled in QAPI as fully > > structured data, nor will it have long term guaranteed stability. > > > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > > --- > > hw/core/machine-qmp-cmds.c | 28 ++++++++++++++++++++++++++++ > > qapi/machine.json | 37 +++++++++++++++++++++++++++++++++++++ > > 2 files changed, 65 insertions(+) > > > > diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c > > index 216fdfaf3a..0d9943ff60 100644 > > --- a/hw/core/machine-qmp-cmds.c > > +++ b/hw/core/machine-qmp-cmds.c > > @@ -204,3 +204,31 @@ MemdevList *qmp_query_memdev(Error **errp) > > object_child_foreach(obj, query_memdev, &list); > > return list; > > } > > + > > +RegisterInfo *qmp_x_query_registers(bool has_cpu, int64_t cpu, Error **errp) > > +{ > > + RegisterInfo *info = g_new0(RegisterInfo, 1); > > + g_autoptr(GString) buf = g_string_new(""); > > + CPUState *cs = NULL, *tmp; > > + > > + if (has_cpu) { > > + CPU_FOREACH(tmp) { > > + if (cpu == tmp->cpu_index) { > > + cs = tmp; > > + } > > + } > > + if (!cs) { > > + error_setg(errp, "CPU %"PRId64" not available", cpu); > > + return NULL; > > + } > > + cpu_format_state(cs, buf, CPU_DUMP_FPU); > > + } else { > > + CPU_FOREACH(cs) { > > + g_string_append_printf(buf, "\nCPU#%d\n", cs->cpu_index); > > + cpu_format_state(cs, buf, CPU_DUMP_FPU); > > + } > > + } > > + > > + info->state = g_steal_pointer(&buf->str); > > + return info; > > +} > > diff --git a/qapi/machine.json b/qapi/machine.json > > index 157712f006..27b922f2ce 100644 > > --- a/qapi/machine.json > > +++ b/qapi/machine.json > > @@ -1312,3 +1312,40 @@ > > '*cores': 'int', > > '*threads': 'int', > > '*maxcpus': 'int' } } > > + > > +## > > +# @RegisterParams: > > +# > > +# Information about the CPU to query state of > > +# > > +# @cpu: the CPU number to query. If omitted, queries all CPUs > > +# > > +# Since: 6.2.0 > > +# > > +## > > +{ 'struct': 'RegisterParams', 'data': {'*cpu': 'int' } } > > + > > +## > > +# @RegisterInfo: > > +# > > +# Information about the CPU state > > +# > > +# @state: the CPU state in an architecture specific format > > +# > > +# Since: 6.2.0 > > +# > > +## > > +{ 'struct': 'RegisterInfo', 'data': {'state': 'str' } } > > + > > +## > > +# @x-query-registers: > > +# > > +# Return information on the CPU registers > > +# > > +# Returns: the CPU state > > +# > > +# Since: 6.2.0 > > +## > > +{ 'command': 'x-query-registers', > > + 'data': 'RegisterParams', > > Unless you have further uses of RegisterParams in mind, use an implicit > type: > > 'data': { '*cpu': 'int' } } No further usage, so will do this. > > > + 'returns': 'RegisterInfo' } > > I'd like us to adopt a convention for commands returning formatted text > for human consumption. Like so: > > 'returns': 'HumanReadableText' } > > with the obvious > > ## > # @HumanReadableText: > # > # @human-readable-text: Formatted output intended for humans. > # > # Since: 6.2.0 > ## > { 'struct': 'HumanReadableText', > 'data': { 'human-readable-text': 'str' } } Ah yes that's a nice idea that will apply easily for many/most of the "info xxxx" commands without current QMP equivs. > When the output needs explaining, do that in the command's doc string. > I figure > > ## > # @x-query-registers: > # > # Returns information about the CPU state > # > # Returns: CPU state in an architecture-specific format > # > # Since: 6.2.0 > ## > > would do in this case. Yep. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 5/5] monitor: rewrite 'info registers' in terms of 'x-query-registers' 2021-09-08 10:37 [PATCH 0/5] Stop adding HMP-only commands, allow QMP for all Daniel P. Berrangé ` (3 preceding siblings ...) 2021-09-08 10:37 ` [PATCH 4/5] qapi: introduce x-query-registers QMP command Daniel P. Berrangé @ 2021-09-08 10:37 ` Daniel P. Berrangé 2021-09-08 11:01 ` Philippe Mathieu-Daudé 2021-09-08 12:18 ` [PATCH 0/5] Stop adding HMP-only commands, allow QMP for all Ján Tomko 2021-09-08 15:09 ` Markus Armbruster 6 siblings, 1 reply; 28+ messages in thread From: Daniel P. Berrangé @ 2021-09-08 10:37 UTC (permalink / raw) To: qemu-devel Cc: Daniel P. Berrangé, Eduardo Habkost, Dr. David Alan Gilbert, Markus Armbruster, Eric Blake Now that we have a QMP command 'x-query-registers', the HMP counterpart 'info registers' can be refactored to call the former. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- monitor/misc.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/monitor/misc.c b/monitor/misc.c index ffe7966870..f0b94c3084 100644 --- a/monitor/misc.c +++ b/monitor/misc.c @@ -67,6 +67,7 @@ #include "block/block-hmp-cmds.h" #include "qapi/qapi-commands-char.h" #include "qapi/qapi-commands-control.h" +#include "qapi/qapi-commands-machine.h" #include "qapi/qapi-commands-migration.h" #include "qapi/qapi-commands-misc.h" #include "qapi/qapi-commands-qom.h" @@ -301,23 +302,29 @@ int monitor_get_cpu_index(Monitor *mon) static void hmp_info_registers(Monitor *mon, const QDict *qdict) { bool all_cpus = qdict_get_try_bool(qdict, "cpustate_all", false); - CPUState *cs; + bool has_cpu = !all_cpus; + int64_t cpu = 0; + Error *local_err = NULL; + g_autoptr(RegisterInfo) info = NULL; - if (all_cpus) { - CPU_FOREACH(cs) { - monitor_printf(mon, "\nCPU#%d\n", cs->cpu_index); - cpu_dump_state(cs, NULL, CPU_DUMP_FPU); - } - } else { - cs = mon_get_cpu(mon); + if (has_cpu) { + CPUState *cs = mon_get_cpu(mon); if (!cs) { monitor_printf(mon, "No CPU available\n"); return; } - cpu_dump_state(cs, NULL, CPU_DUMP_FPU); + cpu = cs->cpu_index; + } + + info = qmp_x_query_registers(has_cpu, cpu, &local_err); + if (!info) { + error_report_err(local_err); + return; } + + monitor_printf(mon, "%s", info->state); } static void hmp_info_sync_profile(Monitor *mon, const QDict *qdict) -- 2.31.1 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 5/5] monitor: rewrite 'info registers' in terms of 'x-query-registers' 2021-09-08 10:37 ` [PATCH 5/5] monitor: rewrite 'info registers' in terms of 'x-query-registers' Daniel P. Berrangé @ 2021-09-08 11:01 ` Philippe Mathieu-Daudé 2021-09-08 11:02 ` Daniel P. Berrangé 0 siblings, 1 reply; 28+ messages in thread From: Philippe Mathieu-Daudé @ 2021-09-08 11:01 UTC (permalink / raw) To: Daniel P. Berrangé, qemu-devel Cc: Markus Armbruster, Eric Blake, Eduardo Habkost, Dr. David Alan Gilbert On 9/8/21 12:37 PM, Daniel P. Berrangé wrote: > Now that we have a QMP command 'x-query-registers', the HMP counterpart > 'info registers' can be refactored to call the former. > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > --- > monitor/misc.c | 25 ++++++++++++++++--------- > 1 file changed, 16 insertions(+), 9 deletions(-) > > diff --git a/monitor/misc.c b/monitor/misc.c > index ffe7966870..f0b94c3084 100644 > --- a/monitor/misc.c > +++ b/monitor/misc.c > @@ -67,6 +67,7 @@ > #include "block/block-hmp-cmds.h" > #include "qapi/qapi-commands-char.h" > #include "qapi/qapi-commands-control.h" > +#include "qapi/qapi-commands-machine.h" > #include "qapi/qapi-commands-migration.h" > #include "qapi/qapi-commands-misc.h" > #include "qapi/qapi-commands-qom.h" > @@ -301,23 +302,29 @@ int monitor_get_cpu_index(Monitor *mon) > static void hmp_info_registers(Monitor *mon, const QDict *qdict) > { > bool all_cpus = qdict_get_try_bool(qdict, "cpustate_all", false); > - CPUState *cs; > + bool has_cpu = !all_cpus; > + int64_t cpu = 0; > + Error *local_err = NULL; > + g_autoptr(RegisterInfo) info = NULL; > > - if (all_cpus) { > - CPU_FOREACH(cs) { > - monitor_printf(mon, "\nCPU#%d\n", cs->cpu_index); > - cpu_dump_state(cs, NULL, CPU_DUMP_FPU); And once all targets are converted we can remove cpu_dump_state(). ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 5/5] monitor: rewrite 'info registers' in terms of 'x-query-registers' 2021-09-08 11:01 ` Philippe Mathieu-Daudé @ 2021-09-08 11:02 ` Daniel P. Berrangé 0 siblings, 0 replies; 28+ messages in thread From: Daniel P. Berrangé @ 2021-09-08 11:02 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Markus Armbruster, Eric Blake, qemu-devel, Dr. David Alan Gilbert, Eduardo Habkost On Wed, Sep 08, 2021 at 01:01:21PM +0200, Philippe Mathieu-Daudé wrote: > On 9/8/21 12:37 PM, Daniel P. Berrangé wrote: > > Now that we have a QMP command 'x-query-registers', the HMP counterpart > > 'info registers' can be refactored to call the former. > > > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > > --- > > monitor/misc.c | 25 ++++++++++++++++--------- > > 1 file changed, 16 insertions(+), 9 deletions(-) > > > > diff --git a/monitor/misc.c b/monitor/misc.c > > index ffe7966870..f0b94c3084 100644 > > --- a/monitor/misc.c > > +++ b/monitor/misc.c > > @@ -67,6 +67,7 @@ > > #include "block/block-hmp-cmds.h" > > #include "qapi/qapi-commands-char.h" > > #include "qapi/qapi-commands-control.h" > > +#include "qapi/qapi-commands-machine.h" > > #include "qapi/qapi-commands-migration.h" > > #include "qapi/qapi-commands-misc.h" > > #include "qapi/qapi-commands-qom.h" > > @@ -301,23 +302,29 @@ int monitor_get_cpu_index(Monitor *mon) > > static void hmp_info_registers(Monitor *mon, const QDict *qdict) > > { > > bool all_cpus = qdict_get_try_bool(qdict, "cpustate_all", false); > > - CPUState *cs; > > + bool has_cpu = !all_cpus; > > + int64_t cpu = 0; > > + Error *local_err = NULL; > > + g_autoptr(RegisterInfo) info = NULL; > > > > - if (all_cpus) { > > - CPU_FOREACH(cs) { > > - monitor_printf(mon, "\nCPU#%d\n", cs->cpu_index); > > - cpu_dump_state(cs, NULL, CPU_DUMP_FPU); > > And once all targets are converted we can remove cpu_dump_state(). Yes, if we take approach in this series, then there would be another 20-ish patches inserted before this one, to convert all the other targets, and eliminate the cpu_dump_state method / callback. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 0/5] Stop adding HMP-only commands, allow QMP for all 2021-09-08 10:37 [PATCH 0/5] Stop adding HMP-only commands, allow QMP for all Daniel P. Berrangé ` (4 preceding siblings ...) 2021-09-08 10:37 ` [PATCH 5/5] monitor: rewrite 'info registers' in terms of 'x-query-registers' Daniel P. Berrangé @ 2021-09-08 12:18 ` Ján Tomko 2021-09-08 15:09 ` Markus Armbruster 6 siblings, 0 replies; 28+ messages in thread From: Ján Tomko @ 2021-09-08 12:18 UTC (permalink / raw) To: Daniel P. Berrangé Cc: Markus Armbruster, Eric Blake, qemu-devel, Dr. David Alan Gilbert, Eduardo Habkost [-- Attachment #1: Type: text/plain, Size: 3479 bytes --] On a Wednesday in 2021, Daniel P. Berrangé wrote: >We are still adding HMP commands without any QMP counterparts. This is >done because there are a reasonable number of scenarios where the cost >of designing a QAPI data type for the command is not justified. > >This has the downside, however, that we will never be able to fully >isolate the monitor code from the remainder of QEMU internals. It is >desirable to be able to get to a point where subsystems in QEMU are >exclusively implemented using QAPI types and never need to have any >knowledge of the monitor APIs. > >The way to get there is to stop adding commands to HMP only. All >commands must be implemented using QMP, and any HMP implementation >be a shim around the QMP implementation. > >We don't want to compromise our supportability of QMP long term though. > >This series proposes that we relax our requirements around fine grained >QAPI data design, but with the caveat that any command taking this >design approach is mandated to use the 'x-' name prefix. > >This tradeoff should be suitable for any commands we have been adding >exclusively to HMP in recent times, and thus mean we have mandate QMP >support for all new commands going forward. > >This series illustrates the concept by converting the "info registers" >HMP to invoke a new 'x-query-registers' QMP command. Note that only >the i386 CPU target is converted to work with this new approach, so >this series needs to be considered incomplete. If we go forward with >this idea, then a subsequent version of this series would need to >obviously convert all other CPU targets. > >After doing that conversion the only use of qemu_fprintf() would be >the disas.c file. Remaining uses of qemu_fprintf and qemu_printf >could be tackled in a similar way and eventually eliminate the need >for any of these printf wrappers in QEMU. > >NB: I added docs to devel/writing-qmp-commands.rst about the two >design approaches to QMP. I didn't see another good place to put >an explicit note that we will not add any more HMP-only commands. >Obviously HMP/QMP maintainers control this in their reviews of >patches, and maybe that's sufficient ? > >NB: if we take this approach we'll want to figure out how many >HMP-only commands we actually have left and then perhaps have >a task to track their conversion to QMP. This could possibly >be a useful task for newbies if we make it clear that they >wouldn't be required to undertake complex QAPI modelling in >doing this conversion. > >Daniel P. Berrangé (5): > docs/devel: document expectations for QAPI data modelling for QMP > hw/core: introduce 'format_state' callback to replace 'dump_state' > target/i386: convert to use format_state instead of dump_state > qapi: introduce x-query-registers QMP command > monitor: rewrite 'info registers' in terms of 'x-query-registers' > > docs/devel/writing-qmp-commands.rst | 25 +++ > hw/core/cpu-common.c | 15 ++ > hw/core/machine-qmp-cmds.c | 28 +++ > include/hw/core/cpu.h | 13 +- > monitor/misc.c | 25 ++- > qapi/machine.json | 37 ++++ > target/i386/cpu-dump.c | 325 +++++++++++++++------------- > target/i386/cpu.c | 2 +- > target/i386/cpu.h | 2 +- > 9 files changed, 307 insertions(+), 165 deletions(-) > Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 0/5] Stop adding HMP-only commands, allow QMP for all 2021-09-08 10:37 [PATCH 0/5] Stop adding HMP-only commands, allow QMP for all Daniel P. Berrangé ` (5 preceding siblings ...) 2021-09-08 12:18 ` [PATCH 0/5] Stop adding HMP-only commands, allow QMP for all Ján Tomko @ 2021-09-08 15:09 ` Markus Armbruster 2021-09-08 15:24 ` Daniel P. Berrangé ` (2 more replies) 6 siblings, 3 replies; 28+ messages in thread From: Markus Armbruster @ 2021-09-08 15:09 UTC (permalink / raw) To: Daniel P. Berrangé Cc: Eduardo Habkost, Markus Armbruster, qemu-devel, Eric Blake, Dr. David Alan Gilbert Daniel P. Berrangé <berrange@redhat.com> writes: > We are still adding HMP commands without any QMP counterparts. This is > done because there are a reasonable number of scenarios where the cost > of designing a QAPI data type for the command is not justified. > > This has the downside, however, that we will never be able to fully > isolate the monitor code from the remainder of QEMU internals. It is > desirable to be able to get to a point where subsystems in QEMU are > exclusively implemented using QAPI types and never need to have any > knowledge of the monitor APIs. > > The way to get there is to stop adding commands to HMP only. All > commands must be implemented using QMP, and any HMP implementation > be a shim around the QMP implementation. > > We don't want to compromise our supportability of QMP long term though. > > This series proposes that we relax our requirements around fine grained > QAPI data design, Specifics? QMP command returns a string, HMP wrapper prints that string? > but with the caveat that any command taking this > design approach is mandated to use the 'x-' name prefix. > > This tradeoff should be suitable for any commands we have been adding > exclusively to HMP in recent times, and thus mean we have mandate QMP > support for all new commands going forward. > > This series illustrates the concept by converting the "info registers" > HMP to invoke a new 'x-query-registers' QMP command. Note that only > the i386 CPU target is converted to work with this new approach, so > this series needs to be considered incomplete. If we go forward with > this idea, then a subsequent version of this series would need to > obviously convert all other CPU targets. > > After doing that conversion the only use of qemu_fprintf() would be > the disas.c file. Remaining uses of qemu_fprintf and qemu_printf > could be tackled in a similar way and eventually eliminate the need > for any of these printf wrappers in QEMU. > > NB: I added docs to devel/writing-qmp-commands.rst about the two > design approaches to QMP. I didn't see another good place to put > an explicit note that we will not add any more HMP-only commands. > Obviously HMP/QMP maintainers control this in their reviews of > patches, and maybe that's sufficient ? We could add devel/writing-hmp-commands.rst, or go with a single document devel/writing-monitor-commands.rst. > NB: if we take this approach we'll want to figure out how many > HMP-only commands we actually have left and then perhaps have > HMP-only commands we actually have left Yes. For many HMP commands, a QMP commands with the same name exists, and the former is probably a wrapper around the latter. Same for HMP "info FOO" and QMP query-FOO. HMP commands without such a match: boot_set change commit cpu delvm drive_add drive_del exit_preconfig gdbserver gpa2hpa gpa2hva gva2gpa help hostfwd_add hostfwd_remove i info info capture info cmma info cpus info history info ioapic info irq info jit info lapic info mem info mtree info network info numa info opcount info pic info profile info qdm info qom-tree info qtree info ramblock info rdma info registers info roms info skeys info snapshots info sync-profile info tlb info trace-events info usb info usbhost info usernet loadvm log logfile mce migrate_set_capability migrate_set_parameter migration_mode mouse_button mouse_move mouse_set nmi o pcie_aer_inject_error print qemu-io savevm sendkey singlestep snapshot_blkdev snapshot_blkdev_internal snapshot_delete_blkdev_internal stopcapture sum sync-profile trace-event trace-file watchdog_action wavcapture x xp This is 77 out of 170 HMP commands. I was hoping for fewer. > and then perhaps have > a task to track their conversion to QMP. This could possibly > be a useful task for newbies if we make it clear that they > wouldn't be required to undertake complex QAPI modelling in > doing this conversion. Yes. Thanks for tackling this! ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 0/5] Stop adding HMP-only commands, allow QMP for all 2021-09-08 15:09 ` Markus Armbruster @ 2021-09-08 15:24 ` Daniel P. Berrangé 2021-09-09 4:48 ` Markus Armbruster 2021-09-08 16:15 ` Philippe Mathieu-Daudé 2021-09-09 6:15 ` Paolo Bonzini 2 siblings, 1 reply; 28+ messages in thread From: Daniel P. Berrangé @ 2021-09-08 15:24 UTC (permalink / raw) To: Markus Armbruster Cc: Eduardo Habkost, Eric Blake, qemu-devel, Dr. David Alan Gilbert On Wed, Sep 08, 2021 at 05:09:13PM +0200, Markus Armbruster wrote: > Daniel P. Berrangé <berrange@redhat.com> writes: > > > We are still adding HMP commands without any QMP counterparts. This is > > done because there are a reasonable number of scenarios where the cost > > of designing a QAPI data type for the command is not justified. > > > > This has the downside, however, that we will never be able to fully > > isolate the monitor code from the remainder of QEMU internals. It is > > desirable to be able to get to a point where subsystems in QEMU are > > exclusively implemented using QAPI types and never need to have any > > knowledge of the monitor APIs. > > > > The way to get there is to stop adding commands to HMP only. All > > commands must be implemented using QMP, and any HMP implementation > > be a shim around the QMP implementation. > > > > We don't want to compromise our supportability of QMP long term though. > > > > This series proposes that we relax our requirements around fine grained > > QAPI data design, > > Specifics? QMP command returns a string, HMP wrapper prints that > string? yes, a command returning a single opaque printf formatted string would be the common case. At a more general POV though, the JSON doc received by the client should be usable "as received", immediately after JSON de-serialization without needing any further custom parsing on top. ie if a value needs to be parsed by the client, then it must be split into multiple distinct values in the QAPI data type design to remove the need for parsing by the client. If a command's design violates that, then it must remain under the "x-" prefix. "info registers" is a example because we're printf formatting each register value into a opaque string. Any client needing a specific register value would have to scanf parse this string. The justification for not representing each register as a distinct QAPI field is that we don't think machines genuinely need to parse this, as its just adhoc human operator debug info. So we take the easy option and just printf to a string and put it under "x-" prefix > For many HMP commands, a QMP commands with the same name exists, and the > former is probably a wrapper around the latter. Same for HMP "info FOO" > and QMP query-FOO. > > HMP commands without such a match: > > boot_set > change > commit > cpu > delvm > drive_add > drive_del > exit_preconfig > gdbserver > gpa2hpa > gpa2hva > gva2gpa > help > hostfwd_add > hostfwd_remove > i > info > info capture > info cmma > info cpus > info history > info ioapic > info irq > info jit > info lapic > info mem > info mtree > info network > info numa > info opcount > info pic > info profile > info qdm > info qom-tree > info qtree > info ramblock > info rdma > info registers > info roms > info skeys > info snapshots > info sync-profile > info tlb > info trace-events > info usb > info usbhost > info usernet > loadvm > log > logfile > mce > migrate_set_capability > migrate_set_parameter > migration_mode > mouse_button > mouse_move > mouse_set > nmi > o > pcie_aer_inject_error > print > qemu-io > savevm > sendkey > singlestep > snapshot_blkdev > snapshot_blkdev_internal > snapshot_delete_blkdev_internal > stopcapture > sum > sync-profile > trace-event > trace-file > watchdog_action > wavcapture > x > xp > > This is 77 out of 170 HMP commands. I was hoping for fewer. Wow, yes, I'm surprised ! A few of those do indeed have QMP equivalents, but with slight differences eg savevm -> snapshot-save, but clearly we have a big list regardless Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 0/5] Stop adding HMP-only commands, allow QMP for all 2021-09-08 15:24 ` Daniel P. Berrangé @ 2021-09-09 4:48 ` Markus Armbruster 2021-09-09 8:13 ` Markus Armbruster 2021-09-09 8:40 ` Daniel P. Berrangé 0 siblings, 2 replies; 28+ messages in thread From: Markus Armbruster @ 2021-09-09 4:48 UTC (permalink / raw) To: Daniel P. Berrangé Cc: Eduardo Habkost, Eric Blake, qemu-devel, Dr. David Alan Gilbert Daniel P. Berrangé <berrange@redhat.com> writes: > On Wed, Sep 08, 2021 at 05:09:13PM +0200, Markus Armbruster wrote: >> Daniel P. Berrangé <berrange@redhat.com> writes: >> >> > We are still adding HMP commands without any QMP counterparts. This is >> > done because there are a reasonable number of scenarios where the cost >> > of designing a QAPI data type for the command is not justified. >> > >> > This has the downside, however, that we will never be able to fully >> > isolate the monitor code from the remainder of QEMU internals. It is >> > desirable to be able to get to a point where subsystems in QEMU are >> > exclusively implemented using QAPI types and never need to have any >> > knowledge of the monitor APIs. >> > >> > The way to get there is to stop adding commands to HMP only. All >> > commands must be implemented using QMP, and any HMP implementation >> > be a shim around the QMP implementation. >> > >> > We don't want to compromise our supportability of QMP long term though. >> > >> > This series proposes that we relax our requirements around fine grained >> > QAPI data design, >> >> Specifics? QMP command returns a string, HMP wrapper prints that >> string? > > yes, a command returning a single opaque printf formatted string would > be the common case. At a more general POV though, the JSON doc received > by the client should be usable "as received", immediately after JSON > de-serialization without needing any further custom parsing on top. > > ie if a value needs to be parsed by the client, then it must be split > into multiple distinct values in the QAPI data type design to remove > the need for parsing by the client. Yes, that's QMP doctrine. > If a command's design violates that, then it must remain under the > "x-" prefix. "info registers" is a example because we're printf > formatting each register value into a opaque string. Any client > needing a specific register value would have to scanf parse this > string. The justification for not representing each register as > a distinct QAPI field is that we don't think machines genuinely > need to parse this, as its just adhoc human operator debug info. > So we take the easy option and just printf to a string and put > it under "x-" prefix Got it. Limitations: 1. If we convert a long-running HMP command to this technique, we print its output only after it completed its work. We also end up with a long-running QMP command, which is bad, because it stops the main loop and makes the QMP monitor unresponsive (except for OOB commands, if the client is careful). The former can be mitigated with 'coroutine': true. The latter can't. 2. We can't prompt for input. The only current use I can see is HMP "change vnc passwd" prompting for a password. Except you currently have to say "change vnc passwd wtf" to get it to prompt (suspect logic error in commit cfb5387a1de). [...] ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 0/5] Stop adding HMP-only commands, allow QMP for all 2021-09-09 4:48 ` Markus Armbruster @ 2021-09-09 8:13 ` Markus Armbruster 2021-09-09 8:40 ` Daniel P. Berrangé 1 sibling, 0 replies; 28+ messages in thread From: Markus Armbruster @ 2021-09-09 8:13 UTC (permalink / raw) To: Daniel P. Berrangé Cc: Eduardo Habkost, Eric Blake, qemu-devel, Dr. David Alan Gilbert Markus Armbruster <armbru@redhat.com> writes: [...] > Limitations: > > 1. If we convert a long-running HMP command to this technique, we print > its output only after it completed its work. We also end up with a > long-running QMP command, which is bad, because it stops the main > loop and makes the QMP monitor unresponsive (except for OOB commands, > if the client is careful). The former can be mitigated with > 'coroutine': true. The latter can't. > > 2. We can't prompt for input. > > The only current use I can see is HMP "change vnc passwd" prompting > for a password. Except you currently have to say "change vnc passwd > wtf" to get it to prompt (suspect logic error in commit cfb5387a1de). Subject: [PATCH 1/2] hmp: Unbreak "change vnc" Message-Id: <20210909081219.308065-2-armbru@redhat.com> > > > [...] ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 0/5] Stop adding HMP-only commands, allow QMP for all 2021-09-09 4:48 ` Markus Armbruster 2021-09-09 8:13 ` Markus Armbruster @ 2021-09-09 8:40 ` Daniel P. Berrangé 1 sibling, 0 replies; 28+ messages in thread From: Daniel P. Berrangé @ 2021-09-09 8:40 UTC (permalink / raw) To: Markus Armbruster Cc: Eduardo Habkost, Eric Blake, qemu-devel, Dr. David Alan Gilbert On Thu, Sep 09, 2021 at 06:48:21AM +0200, Markus Armbruster wrote: > Daniel P. Berrangé <berrange@redhat.com> writes: > > > On Wed, Sep 08, 2021 at 05:09:13PM +0200, Markus Armbruster wrote: > >> Daniel P. Berrangé <berrange@redhat.com> writes: > >> > >> > We are still adding HMP commands without any QMP counterparts. This is > >> > done because there are a reasonable number of scenarios where the cost > >> > of designing a QAPI data type for the command is not justified. > >> > > >> > This has the downside, however, that we will never be able to fully > >> > isolate the monitor code from the remainder of QEMU internals. It is > >> > desirable to be able to get to a point where subsystems in QEMU are > >> > exclusively implemented using QAPI types and never need to have any > >> > knowledge of the monitor APIs. > >> > > >> > The way to get there is to stop adding commands to HMP only. All > >> > commands must be implemented using QMP, and any HMP implementation > >> > be a shim around the QMP implementation. > >> > > >> > We don't want to compromise our supportability of QMP long term though. > >> > > >> > This series proposes that we relax our requirements around fine grained > >> > QAPI data design, > >> > >> Specifics? QMP command returns a string, HMP wrapper prints that > >> string? > > > > yes, a command returning a single opaque printf formatted string would > > be the common case. At a more general POV though, the JSON doc received > > by the client should be usable "as received", immediately after JSON > > de-serialization without needing any further custom parsing on top. > > > > ie if a value needs to be parsed by the client, then it must be split > > into multiple distinct values in the QAPI data type design to remove > > the need for parsing by the client. > > Yes, that's QMP doctrine. > > > If a command's design violates that, then it must remain under the > > "x-" prefix. "info registers" is a example because we're printf > > formatting each register value into a opaque string. Any client > > needing a specific register value would have to scanf parse this > > string. The justification for not representing each register as > > a distinct QAPI field is that we don't think machines genuinely > > need to parse this, as its just adhoc human operator debug info. > > So we take the easy option and just printf to a string and put > > it under "x-" prefix > > Got it. > > Limitations: > > 1. If we convert a long-running HMP command to this technique, we print > its output only after it completed its work. We also end up with a > long-running QMP command, which is bad, because it stops the main > loop and makes the QMP monitor unresponsive (except for OOB commands, > if the client is careful). The former can be mitigated with > 'coroutine': true. The latter can't. I'm essentially ignoring this problem because it already exists in QMP, because anyone needing a HMP-only command today will be running it via the QMP 'human-monitor-command' and thus have the behaviour you describe. > 2. We can't prompt for input. > > The only current use I can see is HMP "change vnc passwd" prompting > for a password. Except you currently have to say "change vnc passwd > wtf" to get it to prompt (suspect logic error in commit cfb5387a1de). Prompting just needs to be killed off. The VNC code supports the 'secret' object type now, in the same way as the rest of the QEMU codebase that needs passwords. So we just need a way to tell VNC the QOM ID of a new "secret" instance instead. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 0/5] Stop adding HMP-only commands, allow QMP for all 2021-09-08 15:09 ` Markus Armbruster 2021-09-08 15:24 ` Daniel P. Berrangé @ 2021-09-08 16:15 ` Philippe Mathieu-Daudé 2021-09-09 6:15 ` Paolo Bonzini 2 siblings, 0 replies; 28+ messages in thread From: Philippe Mathieu-Daudé @ 2021-09-08 16:15 UTC (permalink / raw) To: Markus Armbruster, Daniel P. Berrangé Cc: Eric Blake, Eduardo Habkost, Dr. David Alan Gilbert, qemu-devel On 9/8/21 5:09 PM, Markus Armbruster wrote: > Daniel P. Berrangé <berrange@redhat.com> writes: > >> We are still adding HMP commands without any QMP counterparts. This is >> done because there are a reasonable number of scenarios where the cost >> of designing a QAPI data type for the command is not justified. >> >> This has the downside, however, that we will never be able to fully >> isolate the monitor code from the remainder of QEMU internals. It is >> desirable to be able to get to a point where subsystems in QEMU are >> exclusively implemented using QAPI types and never need to have any >> knowledge of the monitor APIs. >> >> The way to get there is to stop adding commands to HMP only. All >> commands must be implemented using QMP, and any HMP implementation >> be a shim around the QMP implementation. >> >> We don't want to compromise our supportability of QMP long term though. >> >> This series proposes that we relax our requirements around fine grained >> QAPI data design, > > Specifics? QMP command returns a string, HMP wrapper prints that > string? > >> but with the caveat that any command taking this >> design approach is mandated to use the 'x-' name prefix. >> >> This tradeoff should be suitable for any commands we have been adding >> exclusively to HMP in recent times, and thus mean we have mandate QMP >> support for all new commands going forward. >> >> This series illustrates the concept by converting the "info registers" >> HMP to invoke a new 'x-query-registers' QMP command. Note that only >> the i386 CPU target is converted to work with this new approach, so >> this series needs to be considered incomplete. If we go forward with >> this idea, then a subsequent version of this series would need to >> obviously convert all other CPU targets. >> >> After doing that conversion the only use of qemu_fprintf() would be >> the disas.c file. Remaining uses of qemu_fprintf and qemu_printf >> could be tackled in a similar way and eventually eliminate the need >> for any of these printf wrappers in QEMU. >> >> NB: I added docs to devel/writing-qmp-commands.rst about the two >> design approaches to QMP. I didn't see another good place to put >> an explicit note that we will not add any more HMP-only commands. >> Obviously HMP/QMP maintainers control this in their reviews of >> patches, and maybe that's sufficient ? > > We could add devel/writing-hmp-commands.rst, or go with a single > document devel/writing-monitor-commands.rst. > >> NB: if we take this approach we'll want to figure out how many >> HMP-only commands we actually have left and then perhaps have >> HMP-only commands we actually have left > > Yes. > > For many HMP commands, a QMP commands with the same name exists, and the > former is probably a wrapper around the latter. Same for HMP "info FOO" > and QMP query-FOO. > > HMP commands without such a match: (1) Handy HMP commands while debugging: - help - info * - i/o - loadvm/savevm - trace-event/trace-file - wavcapture/stopcapture - xp Eventually also: - hostfwd_add/hostfwd_remove - log - logfile - print - sendkey (2) I suppose these are pre-QMP and wonder about their usefulness in the monitor (I'd certainly use a QMP equivalent to test). - migrate_set_capability - migrate_set_parameter - migration_mode - mouse_button - mouse_move - mouse_set - nmi - pcie_aer_inject_error - exit_preconfig - singlestep - snapshot_blkdev_internal - snapshot_delete_blkdev_internal (3) I don't use them, I expect them to belong in either (1) or (2). > boot_set > change > commit > cpu > delvm > drive_add > drive_del > gdbserver > gpa2hpa > gpa2hva > gva2gpa > mce > qemu-io > snapshot_blkdev > sum > sync-profile > watchdog_action > > This is 77 out of 170 HMP commands. I was hoping for fewer. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 0/5] Stop adding HMP-only commands, allow QMP for all 2021-09-08 15:09 ` Markus Armbruster 2021-09-08 15:24 ` Daniel P. Berrangé 2021-09-08 16:15 ` Philippe Mathieu-Daudé @ 2021-09-09 6:15 ` Paolo Bonzini 2 siblings, 0 replies; 28+ messages in thread From: Paolo Bonzini @ 2021-09-09 6:15 UTC (permalink / raw) To: Markus Armbruster, Daniel P. Berrangé Cc: Eric Blake, Eduardo Habkost, Dr. David Alan Gilbert, qemu-devel On 08/09/21 17:09, Markus Armbruster wrote: > This is 77 out of 170 HMP commands. I was hoping for fewer. There are several that are not quite 1:1, but still not HMP-specific. > exit_preconfig This is x-exit-preconfig. > migrate_set_capability > migrate_set_parameter These are migrate-set-{capabilities,parameters} > mouse_button > mouse_move > mouse_set > sendkey These are input-send-event, but they are not implemented in terms of it. > nmi This is inject-nmi. > loadvm > savevm These are snapshot-{load,save}. > watchdog_action This is set-action, but it is not implemented in terms of it. Paolo >> and then perhaps have >> a task to track their conversion to QMP. This could possibly >> be a useful task for newbies if we make it clear that they >> wouldn't be required to undertake complex QAPI modelling in >> doing this conversion. > > Yes. > > Thanks for tackling this! > > ^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2021-09-10 13:53 UTC | newest] Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-08 10:37 [PATCH 0/5] Stop adding HMP-only commands, allow QMP for all Daniel P. Berrangé 2021-09-08 10:37 ` [PATCH 1/5] docs/devel: document expectations for QAPI data modelling for QMP Daniel P. Berrangé 2021-09-08 17:42 ` Eric Blake 2021-09-09 9:33 ` Markus Armbruster 2021-09-10 12:46 ` Daniel P. Berrangé 2021-09-10 13:45 ` Markus Armbruster 2021-09-10 13:52 ` Daniel P. Berrangé 2021-09-08 10:37 ` [PATCH 2/5] hw/core: introduce 'format_state' callback to replace 'dump_state' Daniel P. Berrangé 2021-09-08 10:37 ` [PATCH 3/5] target/i386: convert to use format_state instead of dump_state Daniel P. Berrangé 2021-09-08 12:17 ` Ján Tomko 2021-09-08 18:05 ` Eric Blake 2021-09-08 22:06 ` Daniel P. Berrangé 2021-09-09 9:55 ` Daniel P. Berrangé 2021-09-08 10:37 ` [PATCH 4/5] qapi: introduce x-query-registers QMP command Daniel P. Berrangé 2021-09-08 18:06 ` Eric Blake 2021-09-09 9:05 ` Markus Armbruster 2021-09-09 10:01 ` Daniel P. Berrangé 2021-09-08 10:37 ` [PATCH 5/5] monitor: rewrite 'info registers' in terms of 'x-query-registers' Daniel P. Berrangé 2021-09-08 11:01 ` Philippe Mathieu-Daudé 2021-09-08 11:02 ` Daniel P. Berrangé 2021-09-08 12:18 ` [PATCH 0/5] Stop adding HMP-only commands, allow QMP for all Ján Tomko 2021-09-08 15:09 ` Markus Armbruster 2021-09-08 15:24 ` Daniel P. Berrangé 2021-09-09 4:48 ` Markus Armbruster 2021-09-09 8:13 ` Markus Armbruster 2021-09-09 8:40 ` Daniel P. Berrangé 2021-09-08 16:15 ` Philippe Mathieu-Daudé 2021-09-09 6:15 ` Paolo Bonzini
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).