[PATCH v3 00/19] monitor: explicitly permit QMP commands to be added for all use cases

[PATCH v3 00/19] monitor: explicitly permit QMP commands to be added for all use cases

[Daniel P. Berrangé]

Previous postings:

  v1: https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg02295.html
  v2: https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg03703.html

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.

The way to get there is to stop adding commands to HMP only. All
commands must be implemented using QMP and any HMP equivalent be
a shim around the QMP implemetation. 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.

The series then converts the following HMP commands to be QMP shims.

    info opcount
    info jit
    info irq
    info lapic
    info cmma
    info skeys
    info ramblock
    info rdma
    info usb
    info numa
    info profile
    info roms

A full conversion would also enable HMP to be emulated entirely
outside QEMU. This could be interesting if we introduce a new QEMU
system emulator binary which is legacy free and 100% controlled
via QMP, as it would let us provide HMP backcompat around it
without the burden of HMP being integrated directly.

There are still many HMP commands with no QMP counterpart after
this series.

 - A few are not relevant to port as they directly
   reflect HMP functionality (help, info history).
 - A few are sort of available in QMP but look quite
   different (drive_add vs blockdev_add)
 - A few are complicated. "info usbhost" is a dynamically
   loaded HMP command inside a loadable module and we
   don't have a way to dynamically register QMP handlers
   at runtime.
 - Most are just tedious gruntwork.

Changed in v3:

 - Temporarily spun off the 'info registers' and 'info tlb'
   conversions. These required 30+ patches across all the
   targets and was making this series too large and conflict
   prone and spanning too many subsystems.

   I'll re-submit those two later

 - Pull in a fix for the 'info lapic' command

 - Misc improvements to the documentation after reviews

 - Add helper for GString -> HumanReadableText conversion

Changed in v2:

 - Improved documentation in response to feedback
 - Finished "info registers" conversion on all targets
 - Got a bit carried away and converted many many more
   commands

Daniel P. Berrangé (18):
  docs/devel: rename file for writing monitor commands
  docs/devel: tweak headings in monitor command docs
  docs/devel: document expectations for QAPI data modelling for QMP
  docs/devel: add example of command returning unstructured text
  docs/devel: document expectations for HMP commands in the future
  monitor: remove 'info ioapic' HMP command
  qapi: introduce x-query-roms QMP command
  qapi: introduce x-query-profile QMP command
  qapi: introduce x-query-numa QMP command
  qapi: introduce x-query-usb QMP command
  qapi: introduce x-query-rdma QMP command
  qapi: introduce x-query-ramblock QMP command
  qapi: introduce x-query-skeys QMP command
  qapi: introduce x-query-cmma QMP command
  qapi: introduce x-query-lapic QMP command
  qapi: introduce x-query-irq QMP command
  qapi: introduce x-query-jit QMP command
  qapi: introduce x-query-opcount QMP command

Dongli Zhang (1):
  hmp: synchronize cpu state for lapic info

 accel/tcg/cpu-exec.c                          |  51 +++++-
 accel/tcg/hmp.c                               |  24 ++-
 accel/tcg/translate-all.c                     |  84 ++++-----
 docs/devel/index.rst                          |   2 +-
 ...mands.rst => writing-monitor-commands.rst} | 134 ++++++++++++++-
 hmp-commands-info.hx                          |  15 --
 hw/core/cpu-common.c                          |   7 +
 hw/core/loader.c                              |  53 ++++--
 hw/core/machine-hmp-cmds.c                    |  33 +---
 hw/core/machine-qmp-cmds.c                    |  40 +++++
 hw/rdma/rdma_rm.c                             | 104 +++++------
 hw/rdma/rdma_rm.h                             |   2 +-
 hw/rdma/vmw/pvrdma_main.c                     |  31 ++--
 hw/s390x/s390-skeys.c                         |  35 +++-
 hw/s390x/s390-stattrib.c                      |  56 ++++--
 hw/usb/bus.c                                  |  36 +++-
 include/exec/cpu-all.h                        |   6 +-
 include/exec/ramlist.h                        |   2 +-
 include/hw/core/cpu.h                         |  10 ++
 include/hw/rdma/rdma.h                        |   2 +-
 include/monitor/hmp-target.h                  |   1 -
 include/qapi/type-helpers.h                   |  14 ++
 include/tcg/tcg.h                             |   4 +-
 monitor/hmp-cmds.c                            |  81 +++------
 monitor/misc.c                                |  30 +---
 monitor/qmp-cmds.c                            | 116 +++++++++++++
 qapi/common.json                              |  11 ++
 qapi/machine-target.json                      |  47 +++++
 qapi/machine.json                             | 110 ++++++++++++
 qapi/meson.build                              |   3 +
 qapi/qapi-type-helpers.c                      |  23 +++
 scripts/qapi/commands.py                      |   1 +
 softmmu/physmem.c                             |  19 ++-
 stubs/usb-dev-stub.c                          |   8 +
 target/i386/cpu-dump.c                        | 161 +++++++++---------
 target/i386/cpu.h                             |   4 +-
 target/i386/monitor.c                         |  50 ++++--
 tcg/tcg.c                                     |  98 ++++++-----
 tests/qtest/qmp-cmd-test.c                    |   8 +
 39 files changed, 1063 insertions(+), 453 deletions(-)
 rename docs/devel/{writing-qmp-commands.rst => writing-monitor-commands.rst} (78%)
 create mode 100644 include/qapi/type-helpers.h
 create mode 100644 qapi/qapi-type-helpers.c

-- 
2.31.1

[PATCH v3 01/19] docs/devel: rename file for writing monitor commands

[Daniel P. Berrangé]

The file already covers writing HMP commands, in addition to
the QMP commands, so it deserves a more general name.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 docs/devel/index.rst                                        | 2 +-
 ...riting-qmp-commands.rst => writing-monitor-commands.rst} | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)
 rename docs/devel/{writing-qmp-commands.rst => writing-monitor-commands.rst} (99%)

diff --git a/docs/devel/index.rst b/docs/devel/index.rst
index f95df10b3e..7c25177c5d 100644
--- a/docs/devel/index.rst
+++ b/docs/devel/index.rst
@@ -44,4 +44,4 @@ modifying QEMU's source code.
    ebpf_rss
    vfio-migration
    qapi-code-gen
-   writing-qmp-commands
+   writing-monitor-commands
diff --git a/docs/devel/writing-qmp-commands.rst b/docs/devel/writing-monitor-commands.rst
similarity index 99%
rename from docs/devel/writing-qmp-commands.rst
rename to docs/devel/writing-monitor-commands.rst
index 6a10a06c48..4a4c051624 100644
--- a/docs/devel/writing-qmp-commands.rst
+++ b/docs/devel/writing-monitor-commands.rst
@@ -1,8 +1,8 @@
-How to write QMP commands using the QAPI framework
-==================================================
+How to write monitor commands
+=============================
 
 This document is a step-by-step guide on how to write new QMP commands using
-the QAPI framework. It also shows how to implement new style HMP commands.
+the QAPI framework and HMP commands.
 
 This document doesn't discuss QMP protocol level details, nor does it dive
 into the QAPI framework implementation.
-- 
2.31.1

Re: [PATCH v3 01/19] docs/devel: rename file for writing monitor commands

[Eric Blake]

On Thu, Sep 30, 2021 at 02:23:31PM +0100, Daniel P. Berrangé wrote:
> The file already covers writing HMP commands, in addition to
> the QMP commands, so it deserves a more general name.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  docs/devel/index.rst                                        | 2 +-
>  ...riting-qmp-commands.rst => writing-monitor-commands.rst} | 6 +++---
>  2 files changed, 4 insertions(+), 4 deletions(-)
>  rename docs/devel/{writing-qmp-commands.rst => writing-monitor-commands.rst} (99%)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

[PATCH v3 02/19] docs/devel: tweak headings in monitor command docs

[Daniel P. Berrangé]

The new headings reflect the intended structure of the document and will
better suit additions that follow.

Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 docs/devel/writing-monitor-commands.rst | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/docs/devel/writing-monitor-commands.rst b/docs/devel/writing-monitor-commands.rst
index 4a4c051624..a973c48f66 100644
--- a/docs/devel/writing-monitor-commands.rst
+++ b/docs/devel/writing-monitor-commands.rst
@@ -85,8 +85,8 @@ any data". Now you're ready to enter the QMP example commands as explained in
 the following sections.
 
 
-Writing a command that doesn't return data
-------------------------------------------
+Writing a simple command: hello-world
+-------------------------------------
 
 That's the most simple QMP command that can be written. Usually, this kind of
 command carries some meaningful action in QEMU but here it will just print
@@ -340,8 +340,8 @@ Please, check the "-monitor" command-line option to know how to open a user
 monitor.
 
 
-Writing a command that returns data
------------------------------------
+Writing more complex commands
+-----------------------------
 
 A QMP command is capable of returning any data the QAPI supports like integers,
 strings, booleans, enumerations and user defined types.
-- 
2.31.1

[PATCH v3 03/19] docs/devel: document expectations for QAPI data modelling for QMP

[Daniel P. Berrangé]

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.

Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 docs/devel/writing-monitor-commands.rst | 27 +++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/docs/devel/writing-monitor-commands.rst b/docs/devel/writing-monitor-commands.rst
index a973c48f66..0f3b751dab 100644
--- a/docs/devel/writing-monitor-commands.rst
+++ b/docs/devel/writing-monitor-commands.rst
@@ -350,6 +350,33 @@ 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, then 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 ad hoc 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 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

Re: [PATCH v3 03/19] docs/devel: document expectations for QAPI data modelling for QMP

[Dr. David Alan Gilbert]

* Daniel P. Berrangé (berrange@redhat.com) 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.
> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  docs/devel/writing-monitor-commands.rst | 27 +++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/docs/devel/writing-monitor-commands.rst b/docs/devel/writing-monitor-commands.rst
> index a973c48f66..0f3b751dab 100644
> --- a/docs/devel/writing-monitor-commands.rst
> +++ b/docs/devel/writing-monitor-commands.rst
> @@ -350,6 +350,33 @@ 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, then 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 ad hoc 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 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.
> +

Are they required to do it this way - or are they allowed to define x-
qapi types and do the formatting in the HMP code?
For example, a lot of the info commands produce lists of data,
you can imagine some of them could add x- types for each list entry.

Dave

>  User Defined Types
>  ~~~~~~~~~~~~~~~~~~
>  
> -- 
> 2.31.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v3 03/19] docs/devel: document expectations for QAPI data modelling for QMP

[Daniel P. Berrangé]

On Mon, Oct 04, 2021 at 01:13:00PM +0100, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrangé (berrange@redhat.com) 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.
> > 
> > Reviewed-by: Markus Armbruster <armbru@redhat.com>
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  docs/devel/writing-monitor-commands.rst | 27 +++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> > 
> > diff --git a/docs/devel/writing-monitor-commands.rst b/docs/devel/writing-monitor-commands.rst
> > index a973c48f66..0f3b751dab 100644
> > --- a/docs/devel/writing-monitor-commands.rst
> > +++ b/docs/devel/writing-monitor-commands.rst
> > @@ -350,6 +350,33 @@ 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, then 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 ad hoc 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 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.
> > +
> 
> Are they required to do it this way - or are they allowed to define x-
> qapi types and do the formatting in the HMP code?
> For example, a lot of the info commands produce lists of data,
> you can imagine some of them could add x- types for each list entry.

If it makes sense for the corresponding QMP command to return more
explicitly structured data than HumanReadableText, but still using an
x- prefixed data type, then I'd say that is fine. That is an
incrementally better step towards the ideal of a fully stable QMP
command + data type. It'll be a judgement call by whomever designs
it as to how much time to invest in the QAPI side of things, depending
on how likely it is that machines want to interpret the strucutred
data.

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 :|

Re: [PATCH v3 03/19] docs/devel: document expectations for QAPI data modelling for QMP

[Markus Armbruster]

This clashes with my "[PATCH v2 0/9] Configurable policy for handling
unstable interfaces", which replaces "you must give unstable stuff names
starting with 'x-'" by "you must give unstable stuff feature flag
'unstable' (and may give it a name starting with 'x-')".

Re: [PATCH v3 03/19] docs/devel: document expectations for QAPI data modelling for QMP

[Daniel P. Berrangé]

On Thu, Oct 28, 2021 at 04:31:40PM +0200, Markus Armbruster wrote:
> This clashes with my "[PATCH v2 0/9] Configurable policy for handling
> unstable interfaces", which replaces "you must give unstable stuff names
> starting with 'x-'" by "you must give unstable stuff feature flag
> 'unstable' (and may give it a name starting with 'x-')".

If your series goes in first, I'll update this to take account of it,
but for now I'll leave it as is.

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 :|

Re: [PATCH v3 03/19] docs/devel: document expectations for QAPI data modelling for QMP

[Daniel P. Berrangé]

On Thu, Oct 28, 2021 at 04:24:31PM +0100, Daniel P. Berrangé wrote:
> On Thu, Oct 28, 2021 at 04:31:40PM +0200, Markus Armbruster wrote:
> > This clashes with my "[PATCH v2 0/9] Configurable policy for handling
> > unstable interfaces", which replaces "you must give unstable stuff names
> > starting with 'x-'" by "you must give unstable stuff feature flag
> > 'unstable' (and may give it a name starting with 'x-')".
> 
> If your series goes in first, I'll update this to take account of it,
> but for now I'll leave it as is.

This patch has now merged to git master.

If you re-post your series feel free to update the new docs, or let
me know if you want me to do it afterwards.

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 :|

Re: [PATCH v3 03/19] docs/devel: document expectations for QAPI data modelling for QMP

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Thu, Oct 28, 2021 at 04:24:31PM +0100, Daniel P. Berrangé wrote:
>> On Thu, Oct 28, 2021 at 04:31:40PM +0200, Markus Armbruster wrote:
>> > This clashes with my "[PATCH v2 0/9] Configurable policy for handling
>> > unstable interfaces", which replaces "you must give unstable stuff names
>> > starting with 'x-'" by "you must give unstable stuff feature flag
>> > 'unstable' (and may give it a name starting with 'x-')".
>> 
>> If your series goes in first, I'll update this to take account of it,
>> but for now I'll leave it as is.
>
> This patch has now merged to git master.

Merge commit e86e00a2493, Nov 3.

> If you re-post your series feel free to update the new docs, or let
> me know if you want me to do it afterwards.

The latter, because my series went in before yours, in merge commit
dd61b91c080, Oct 29 :)

Re: [PATCH v3 03/19] docs/devel: document expectations for QAPI data modelling for QMP

[Daniel P. Berrangé]

On Thu, Nov 04, 2021 at 06:43:23AM +0100, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Thu, Oct 28, 2021 at 04:24:31PM +0100, Daniel P. Berrangé wrote:
> >> On Thu, Oct 28, 2021 at 04:31:40PM +0200, Markus Armbruster wrote:
> >> > This clashes with my "[PATCH v2 0/9] Configurable policy for handling
> >> > unstable interfaces", which replaces "you must give unstable stuff names
> >> > starting with 'x-'" by "you must give unstable stuff feature flag
> >> > 'unstable' (and may give it a name starting with 'x-')".
> >> 
> >> If your series goes in first, I'll update this to take account of it,
> >> but for now I'll leave it as is.
> >
> > This patch has now merged to git master.
> 
> Merge commit e86e00a2493, Nov 3.
> 
> > If you re-post your series feel free to update the new docs, or let
> > me know if you want me to do it afterwards.
> 
> The latter, because my series went in before yours, in merge commit
> dd61b91c080, Oct 29 :)

Sigh, I'm blind ! Dunno how i missed that.

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 :|

Re: [PATCH v3 03/19] docs/devel: document expectations for QAPI data modelling for QMP

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Thu, Nov 04, 2021 at 06:43:23AM +0100, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>> > On Thu, Oct 28, 2021 at 04:24:31PM +0100, Daniel P. Berrangé wrote:
>> >> On Thu, Oct 28, 2021 at 04:31:40PM +0200, Markus Armbruster wrote:
>> >> > This clashes with my "[PATCH v2 0/9] Configurable policy for handling
>> >> > unstable interfaces", which replaces "you must give unstable stuff names
>> >> > starting with 'x-'" by "you must give unstable stuff feature flag
>> >> > 'unstable' (and may give it a name starting with 'x-')".
>> >> 
>> >> If your series goes in first, I'll update this to take account of it,
>> >> but for now I'll leave it as is.
>> >
>> > This patch has now merged to git master.
>> 
>> Merge commit e86e00a2493, Nov 3.
>> 
>> > If you re-post your series feel free to update the new docs, or let
>> > me know if you want me to do it afterwards.
>> 
>> The latter, because my series went in before yours, in merge commit
>> dd61b91c080, Oct 29 :)
>
> Sigh, I'm blind ! Dunno how i missed that.

I can do it for you (it's such a simple patch), but I'd rather not do it
in addition to you :)

Re: [PATCH v3 03/19] docs/devel: document expectations for QAPI data modelling for QMP

[Daniel P. Berrangé]

On Tue, Nov 09, 2021 at 07:39:29AM +0100, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Thu, Nov 04, 2021 at 06:43:23AM +0100, Markus Armbruster wrote:
> >> Daniel P. Berrangé <berrange@redhat.com> writes:
> >> 
> >> > On Thu, Oct 28, 2021 at 04:24:31PM +0100, Daniel P. Berrangé wrote:
> >> >> On Thu, Oct 28, 2021 at 04:31:40PM +0200, Markus Armbruster wrote:
> >> >> > This clashes with my "[PATCH v2 0/9] Configurable policy for handling
> >> >> > unstable interfaces", which replaces "you must give unstable stuff names
> >> >> > starting with 'x-'" by "you must give unstable stuff feature flag
> >> >> > 'unstable' (and may give it a name starting with 'x-')".
> >> >> 
> >> >> If your series goes in first, I'll update this to take account of it,
> >> >> but for now I'll leave it as is.
> >> >
> >> > This patch has now merged to git master.
> >> 
> >> Merge commit e86e00a2493, Nov 3.
> >> 
> >> > If you re-post your series feel free to update the new docs, or let
> >> > me know if you want me to do it afterwards.
> >> 
> >> The latter, because my series went in before yours, in merge commit
> >> dd61b91c080, Oct 29 :)
> >
> > Sigh, I'm blind ! Dunno how i missed that.
> 
> I can do it for you (it's such a simple patch), but I'd rather not do it
> in addition to you :)

I've not started yet, so I'll let you do it since you probably have a
better idea of the preferred language to describe it. I'll promise my
review services in return.

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 :|

Re: [PATCH v3 03/19] docs/devel: document expectations for QAPI data modelling for QMP

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Tue, Nov 09, 2021 at 07:39:29AM +0100, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>> > On Thu, Nov 04, 2021 at 06:43:23AM +0100, Markus Armbruster wrote:
>> >> Daniel P. Berrangé <berrange@redhat.com> writes:
>> >> 
>> >> > On Thu, Oct 28, 2021 at 04:24:31PM +0100, Daniel P. Berrangé wrote:
>> >> >> On Thu, Oct 28, 2021 at 04:31:40PM +0200, Markus Armbruster wrote:
>> >> >> > This clashes with my "[PATCH v2 0/9] Configurable policy for handling
>> >> >> > unstable interfaces", which replaces "you must give unstable stuff names
>> >> >> > starting with 'x-'" by "you must give unstable stuff feature flag
>> >> >> > 'unstable' (and may give it a name starting with 'x-')".
>> >> >> 
>> >> >> If your series goes in first, I'll update this to take account of it,
>> >> >> but for now I'll leave it as is.
>> >> >
>> >> > This patch has now merged to git master.
>> >> 
>> >> Merge commit e86e00a2493, Nov 3.
>> >> 
>> >> > If you re-post your series feel free to update the new docs, or let
>> >> > me know if you want me to do it afterwards.
>> >> 
>> >> The latter, because my series went in before yours, in merge commit
>> >> dd61b91c080, Oct 29 :)
>> >
>> > Sigh, I'm blind ! Dunno how i missed that.
>> 
>> I can do it for you (it's such a simple patch), but I'd rather not do it
>> in addition to you :)
>
> I've not started yet, so I'll let you do it since you probably have a
> better idea of the preferred language to describe it. I'll promise my
> review services in return.

[PATCH] qapi: Belatedly mark unstable QMP parts with feature 'unstable'

[PATCH v3 04/19] docs/devel: add example of command returning unstructured text

[Daniel P. Berrangé]

This illustrates how to add a QMP command returning unstructured text,
following the guidelines added in the previous patch. The example uses
a simplified version of 'info roms'.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 docs/devel/writing-monitor-commands.rst | 87 ++++++++++++++++++++++++-
 1 file changed, 86 insertions(+), 1 deletion(-)

diff --git a/docs/devel/writing-monitor-commands.rst b/docs/devel/writing-monitor-commands.rst
index 0f3b751dab..82a382d700 100644
--- a/docs/devel/writing-monitor-commands.rst
+++ b/docs/devel/writing-monitor-commands.rst
@@ -375,7 +375,9 @@ best practices. An example where this approach is taken is the QMP
 command "x-query-registers". This returns a 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.
+intended to be consumed as an opaque string by machines. Refer to the
+`Writing a debugging aid returning unstructured text`_ section for
+an illustration.
 
 User Defined Types
 ~~~~~~~~~~~~~~~~~~
@@ -647,3 +649,86 @@ has to traverse the list, it's shown below for reference::
 
      qapi_free_TimerAlarmMethodList(method_list);
  }
+
+Writing a debugging aid returning unstructured text
+---------------------------------------------------
+
+As discussed in section `Modelling data in QAPI`_, it is required that
+commands expecting machine usage be using fine-grained QAPI data types.
+The exception to this rule applies when the command is solely intended
+as a debugging aid and allows for returning unstructured text. This is
+commonly needed for query commands that report aspects of QEMU's
+internal state that are useful to human operators.
+
+In this example we will consider a simplified variant of the HMP
+command ``info roms``. Following the earlier rules, this command will
+need to live under the ``x-`` name prefix, so its QMP implementation
+will be called ``x-query-roms``. It will have no parameters and will
+return a single text string::
+
+ { 'struct': 'HumanReadableText',
+   'data': { 'human-readable-text': 'str' } }
+
+ { 'command': 'x-query-roms',
+   'returns': 'HumanReadableText' }
+
+The ``HumanReadableText`` struct is intended to be used for all
+commands, under the ``x-`` name prefix that are returning unstructured
+text targetted at humans. It should never be used for commands outside
+the ``x-`` name prefix, as those should be using structured QAPI types.
+
+Implementing the QMP command
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+The QMP implementation will typically involve creating a ``GString``
+object and printing formatted data into it::
+
+ HumanReadableText *qmp_x_query_roms(Error **errp)
+ {
+     g_autoptr(GString) buf = g_string_new("");
+     Rom *rom;
+
+     QTAILQ_FOREACH(rom, &roms, next) {
+        g_string_append_printf("%s size=0x%06zx name=\"%s\"\n",
+                               memory_region_name(rom->mr),
+                               rom->romsize,
+                               rom->name);
+     }
+
+     return human_readable_text_from_str(buf);
+ }
+
+
+Implementing the HMP command
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+Now that the QMP command is in place, we can also make it available in
+the human monitor (HMP) as shown in previous examples. The HMP
+implementations will all look fairly similar, as all they need do is
+invoke the QMP command and then print the resulting text or error
+message. Here's the implementation of the "info roms" HMP command::
+
+ void hmp_info_roms(Monitor *mon, const QDict *qdict)
+ {
+     Error err = NULL;
+     g_autoptr(HumanReadableText) info = qmp_x_query_roms(&err);
+
+     if (err) {
+         error_report_err(err);
+         return;
+     }
+     monitor_printf(mon, "%s\n", info->human_readable_text);
+ }
+
+Also, you have to add the function's prototype to the hmp.h file.
+
+There's one last step to actually make the command available to
+monitor users, we should add it to the hmp-commands-info.hx file::
+
+    {
+        .name       = "roms",
+        .args_type  = "",
+        .params     = "",
+        .help       = "show roms",
+        .cmd        = hmp_info_roms,
+    },
-- 
2.31.1

Re: [PATCH v3 04/19] docs/devel: add example of command returning unstructured text

[Eric Blake]

On Thu, Sep 30, 2021 at 02:23:34PM +0100, Daniel P. Berrangé wrote:
> This illustrates how to add a QMP command returning unstructured text,
> following the guidelines added in the previous patch. The example uses
> a simplified version of 'info roms'.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  docs/devel/writing-monitor-commands.rst | 87 ++++++++++++++++++++++++-
>  1 file changed, 86 insertions(+), 1 deletion(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v3 04/19] docs/devel: add example of command returning unstructured text

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> This illustrates how to add a QMP command returning unstructured text,
> following the guidelines added in the previous patch. The example uses
> a simplified version of 'info roms'.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  docs/devel/writing-monitor-commands.rst | 87 ++++++++++++++++++++++++-
>  1 file changed, 86 insertions(+), 1 deletion(-)
>
> diff --git a/docs/devel/writing-monitor-commands.rst b/docs/devel/writing-monitor-commands.rst
> index 0f3b751dab..82a382d700 100644
> --- a/docs/devel/writing-monitor-commands.rst
> +++ b/docs/devel/writing-monitor-commands.rst
> @@ -375,7 +375,9 @@ best practices. An example where this approach is taken is the QMP
>  command "x-query-registers". This returns a 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.
> +intended to be consumed as an opaque string by machines. Refer to the
> +`Writing a debugging aid returning unstructured text`_ section for
> +an illustration.
>  
>  User Defined Types
>  ~~~~~~~~~~~~~~~~~~
> @@ -647,3 +649,86 @@ has to traverse the list, it's shown below for reference::
>  
>       qapi_free_TimerAlarmMethodList(method_list);
>   }
> +
> +Writing a debugging aid returning unstructured text
> +---------------------------------------------------
> +
> +As discussed in section `Modelling data in QAPI`_, it is required that
> +commands expecting machine usage be using fine-grained QAPI data types.
> +The exception to this rule applies when the command is solely intended
> +as a debugging aid and allows for returning unstructured text. This is
> +commonly needed for query commands that report aspects of QEMU's
> +internal state that are useful to human operators.
> +
> +In this example we will consider a simplified variant of the HMP
> +command ``info roms``. Following the earlier rules, this command will
> +need to live under the ``x-`` name prefix, so its QMP implementation
> +will be called ``x-query-roms``. It will have no parameters and will
> +return a single text string::
> +
> + { 'struct': 'HumanReadableText',
> +   'data': { 'human-readable-text': 'str' } }
> +
> + { 'command': 'x-query-roms',
> +   'returns': 'HumanReadableText' }
> +
> +The ``HumanReadableText`` struct is intended to be used for all
> +commands, under the ``x-`` name prefix that are returning unstructured
> +text targetted at humans. It should never be used for commands outside
> +the ``x-`` name prefix, as those should be using structured QAPI types.

This clashes with my "[PATCH v2 0/9] Configurable policy for handling
unstable interfaces", which replaces "you must give unstable stuff names
starting with 'x-'" by "you must give unstable stuff feature flag
'unstable' (and may give it a name starting with 'x-')".

> +
> +Implementing the QMP command
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +The QMP implementation will typically involve creating a ``GString``
> +object and printing formatted data into it::
> +
> + HumanReadableText *qmp_x_query_roms(Error **errp)
> + {
> +     g_autoptr(GString) buf = g_string_new("");
> +     Rom *rom;
> +
> +     QTAILQ_FOREACH(rom, &roms, next) {
> +        g_string_append_printf("%s size=0x%06zx name=\"%s\"\n",
> +                               memory_region_name(rom->mr),
> +                               rom->romsize,
> +                               rom->name);
> +     }
> +
> +     return human_readable_text_from_str(buf);
> + }
> +
> +
> +Implementing the HMP command
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +Now that the QMP command is in place, we can also make it available in
> +the human monitor (HMP) as shown in previous examples. The HMP
> +implementations will all look fairly similar, as all they need do is
> +invoke the QMP command and then print the resulting text or error
> +message. Here's the implementation of the "info roms" HMP command::
> +
> + void hmp_info_roms(Monitor *mon, const QDict *qdict)
> + {
> +     Error err = NULL;
> +     g_autoptr(HumanReadableText) info = qmp_x_query_roms(&err);
> +
> +     if (err) {
> +         error_report_err(err);
> +         return;
> +     }

There's hmp_handle_error().

If it returned whether there's an error, we could write

        if (hmp_handle_error(err)) {
            return;
        }

Of course, qmp_x_query_roms() can't fail, so we could just as well do

        g_autoptr(HumanReadableText) info = qmp_x_query_roms(&error_abort);

Okay as long as we show how to report errors in HMP commands
*somewhere*, but the use of &error_abort needs explaining.  Not sure
that's an improvement here.

Aside: the existing examples show questionable error handling.  The
first one uses error_get_pretty() instead of hmp_handle_error().  The
other two throw away the error they get from the QMP command, and report
"Could not query ..." instead, which is a bit of an anti-pattern.

> +     monitor_printf(mon, "%s\n", info->human_readable_text);

Sure you want to print an extra newline?

If not, then consider

        monitor_puts(mon, info->human_readable_text);

> + }
> +
> +Also, you have to add the function's prototype to the hmp.h file.
> +
> +There's one last step to actually make the command available to
> +monitor users, we should add it to the hmp-commands-info.hx file::
> +
> +    {
> +        .name       = "roms",
> +        .args_type  = "",
> +        .params     = "",
> +        .help       = "show roms",
> +        .cmd        = hmp_info_roms,
> +    },

Re: [PATCH v3 04/19] docs/devel: add example of command returning unstructured text

[Daniel P. Berrangé]

On Thu, Oct 28, 2021 at 04:47:14PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > This illustrates how to add a QMP command returning unstructured text,
> > following the guidelines added in the previous patch. The example uses
> > a simplified version of 'info roms'.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  docs/devel/writing-monitor-commands.rst | 87 ++++++++++++++++++++++++-
> >  1 file changed, 86 insertions(+), 1 deletion(-)

> > +Implementing the HMP command
> > +~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > +
> > +Now that the QMP command is in place, we can also make it available in
> > +the human monitor (HMP) as shown in previous examples. The HMP
> > +implementations will all look fairly similar, as all they need do is
> > +invoke the QMP command and then print the resulting text or error
> > +message. Here's the implementation of the "info roms" HMP command::
> > +
> > + void hmp_info_roms(Monitor *mon, const QDict *qdict)
> > + {
> > +     Error err = NULL;
> > +     g_autoptr(HumanReadableText) info = qmp_x_query_roms(&err);
> > +
> > +     if (err) {
> > +         error_report_err(err);
> > +         return;
> > +     }
> 
> There's hmp_handle_error().
> 
> If it returned whether there's an error, we could write
> 
>         if (hmp_handle_error(err)) {
>             return;
>         }

I'll add a return value to hmp_handle_error and update existing
callers where appropriate

> 
> Of course, qmp_x_query_roms() can't fail, so we could just as well do
> 
>         g_autoptr(HumanReadableText) info = qmp_x_query_roms(&error_abort);
> 
> Okay as long as we show how to report errors in HMP commands
> *somewhere*, but the use of &error_abort needs explaining.  Not sure
> that's an improvement here.

For the sake of illustration I think I'll stick with hmp_handle_error
and not &error_abort.  In fact following Dave's feedback, I've added
a helper to provide the HMP implementation with no code required in
the no-arg case.

> Aside: the existing examples show questionable error handling.  The
> first one uses error_get_pretty() instead of hmp_handle_error().  The
> other two throw away the error they get from the QMP command, and report
> "Could not query ..." instead, which is a bit of an anti-pattern.

I'll fix those examples

> 
> > +     monitor_printf(mon, "%s\n", info->human_readable_text);
> 
> Sure you want to print an extra newline?

Opps, mistake.

> If not, then consider
> 
>         monitor_puts(mon, info->human_readable_text);

I'd prefer "%s", since it avoids danger of the human readable
text possibly containing a '%' sign that trips up printf.


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 :|

Re: [PATCH v3 04/19] docs/devel: add example of command returning unstructured text

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Thu, Oct 28, 2021 at 04:47:14PM +0200, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>> > This illustrates how to add a QMP command returning unstructured text,
>> > following the guidelines added in the previous patch. The example uses
>> > a simplified version of 'info roms'.
>> >
>> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>> > ---
>> >  docs/devel/writing-monitor-commands.rst | 87 ++++++++++++++++++++++++-
>> >  1 file changed, 86 insertions(+), 1 deletion(-)
>
>> > +Implementing the HMP command
>> > +~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> > +
>> > +Now that the QMP command is in place, we can also make it available in
>> > +the human monitor (HMP) as shown in previous examples. The HMP
>> > +implementations will all look fairly similar, as all they need do is
>> > +invoke the QMP command and then print the resulting text or error
>> > +message. Here's the implementation of the "info roms" HMP command::
>> > +
>> > + void hmp_info_roms(Monitor *mon, const QDict *qdict)
>> > + {
>> > +     Error err = NULL;
>> > +     g_autoptr(HumanReadableText) info = qmp_x_query_roms(&err);
>> > +
>> > +     if (err) {
>> > +         error_report_err(err);
>> > +         return;
>> > +     }
>> 
>> There's hmp_handle_error().
>> 
>> If it returned whether there's an error, we could write
>> 
>>         if (hmp_handle_error(err)) {
>>             return;
>>         }
>
> I'll add a return value to hmp_handle_error and update existing
> callers where appropriate
>
>> 
>> Of course, qmp_x_query_roms() can't fail, so we could just as well do
>> 
>>         g_autoptr(HumanReadableText) info = qmp_x_query_roms(&error_abort);
>> 
>> Okay as long as we show how to report errors in HMP commands
>> *somewhere*, but the use of &error_abort needs explaining.  Not sure
>> that's an improvement here.
>
> For the sake of illustration I think I'll stick with hmp_handle_error
> and not &error_abort.  In fact following Dave's feedback, I've added
> a helper to provide the HMP implementation with no code required in
> the no-arg case.
>
>> Aside: the existing examples show questionable error handling.  The
>> first one uses error_get_pretty() instead of hmp_handle_error().  The
>> other two throw away the error they get from the QMP command, and report
>> "Could not query ..." instead, which is a bit of an anti-pattern.
>
> I'll fix those examples

Thanks!

>> 
>> > +     monitor_printf(mon, "%s\n", info->human_readable_text);
>> 
>> Sure you want to print an extra newline?
>
> Opps, mistake.
>
>> If not, then consider
>> 
>>         monitor_puts(mon, info->human_readable_text);
>
> I'd prefer "%s", since it avoids danger of the human readable
> text possibly containing a '%' sign that trips up printf.

Read that again: "puts" :)

[PATCH v3 05/19] docs/devel: document expectations for HMP commands in the future

[Daniel P. Berrangé]

We no longer wish to have commands implemented in HMP only. All commands
should start with a QMP implementation and the HMP merely be a shim
around this. To reduce the burden of implementing QMP commands where
there is low expectation of machine usage, requirements for QAPI
modelling are relaxed provided the command is under the "x-" name
prefix.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 docs/devel/writing-monitor-commands.rst | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/docs/devel/writing-monitor-commands.rst b/docs/devel/writing-monitor-commands.rst
index 82a382d700..8fb855e192 100644
--- a/docs/devel/writing-monitor-commands.rst
+++ b/docs/devel/writing-monitor-commands.rst
@@ -11,6 +11,14 @@ For an in-depth introduction to the QAPI framework, please refer to
 docs/devel/qapi-code-gen.txt. For documentation about the QMP protocol,
 start with docs/interop/qmp-intro.txt.
 
+New commands may be implemented in QMP only.  New HMP commands should be
+implemented on top of QMP.  The typical HMP command wraps around an
+equivalent QMP command, but HMP convenience commands built from QMP
+building blocks are also fine.  The long term goal is to make all
+existing HMP commands conform to this, to fully isolate HMP from the
+internals of QEMU. Refer to the `Writing a debugging aid returning
+unstructured text`_ section for further guidance on commands that
+would have traditionally been HMP only.
 
 Overview
 --------
-- 
2.31.1

Re: [PATCH v3 05/19] docs/devel: document expectations for HMP commands in the future

[Eric Blake]

On Thu, Sep 30, 2021 at 02:23:35PM +0100, Daniel P. Berrangé wrote:
> We no longer wish to have commands implemented in HMP only. All commands
> should start with a QMP implementation and the HMP merely be a shim
> around this. To reduce the burden of implementing QMP commands where
> there is low expectation of machine usage, requirements for QAPI
> modelling are relaxed provided the command is under the "x-" name
> prefix.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  docs/devel/writing-monitor-commands.rst | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v3 05/19] docs/devel: document expectations for HMP commands in the future

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> We no longer wish to have commands implemented in HMP only. All commands
> should start with a QMP implementation and the HMP merely be a shim
> around this. To reduce the burden of implementing QMP commands where
> there is low expectation of machine usage, requirements for QAPI
> modelling are relaxed provided the command is under the "x-" name
> prefix.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  docs/devel/writing-monitor-commands.rst | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/docs/devel/writing-monitor-commands.rst b/docs/devel/writing-monitor-commands.rst
> index 82a382d700..8fb855e192 100644
> --- a/docs/devel/writing-monitor-commands.rst
> +++ b/docs/devel/writing-monitor-commands.rst
> @@ -11,6 +11,14 @@ For an in-depth introduction to the QAPI framework, please refer to
>  docs/devel/qapi-code-gen.txt. For documentation about the QMP protocol,
>  start with docs/interop/qmp-intro.txt.
>  
> +New commands may be implemented in QMP only.  New HMP commands should be
> +implemented on top of QMP.  The typical HMP command wraps around an
> +equivalent QMP command, but HMP convenience commands built from QMP
> +building blocks are also fine.  The long term goal is to make all
> +existing HMP commands conform to this, to fully isolate HMP from the
> +internals of QEMU. Refer to the `Writing a debugging aid returning
> +unstructured text`_ section for further guidance on commands that
> +would have traditionally been HMP only.
>  
>  Overview
>  --------

Reviewed-by: Markus Armbruster <armbru@redhat.com>

[PATCH v3 06/19] monitor: remove 'info ioapic' HMP command

[Daniel P. Berrangé]

This command was turned into a no-op four years ago in

  commit 0c8465440d50c18a7bb13d0a866748f0593e193a
  Author: Peter Xu <peterx@redhat.com>
  Date:   Fri Dec 29 15:31:04 2017 +0800

    hmp: obsolete "info ioapic"

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 hmp-commands-info.hx         | 15 ---------------
 include/monitor/hmp-target.h |  1 -
 target/i386/monitor.c        |  6 ------
 3 files changed, 22 deletions(-)

diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index 27206ac049..f8312342cd 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -127,21 +127,6 @@ SRST
     Show local APIC state
 ERST
 
-#if defined(TARGET_I386)
-    {
-        .name       = "ioapic",
-        .args_type  = "",
-        .params     = "",
-        .help       = "show io apic state",
-        .cmd        = hmp_info_io_apic,
-    },
-#endif
-
-SRST
-  ``info ioapic``
-    Show io APIC state
-ERST
-
     {
         .name       = "cpus",
         .args_type  = "",
diff --git a/include/monitor/hmp-target.h b/include/monitor/hmp-target.h
index 60fc92722a..df79ad3355 100644
--- a/include/monitor/hmp-target.h
+++ b/include/monitor/hmp-target.h
@@ -48,6 +48,5 @@ void hmp_info_mem(Monitor *mon, const QDict *qdict);
 void hmp_info_tlb(Monitor *mon, const QDict *qdict);
 void hmp_mce(Monitor *mon, const QDict *qdict);
 void hmp_info_local_apic(Monitor *mon, const QDict *qdict);
-void hmp_info_io_apic(Monitor *mon, const QDict *qdict);
 
 #endif /* MONITOR_HMP_TARGET_H */
diff --git a/target/i386/monitor.c b/target/i386/monitor.c
index 119211f0b0..19468c4e85 100644
--- a/target/i386/monitor.c
+++ b/target/i386/monitor.c
@@ -669,12 +669,6 @@ void hmp_info_local_apic(Monitor *mon, const QDict *qdict)
     x86_cpu_dump_local_apic_state(cs, CPU_DUMP_FPU);
 }
 
-void hmp_info_io_apic(Monitor *mon, const QDict *qdict)
-{
-    monitor_printf(mon, "This command is obsolete and will be "
-                   "removed soon. Please use 'info pic' instead.\n");
-}
-
 SevInfo *qmp_query_sev(Error **errp)
 {
     SevInfo *info;
-- 
2.31.1

[PATCH v3 07/19] qapi: introduce x-query-roms QMP command

[Daniel P. Berrangé]

This is a counterpart to the HMP "info roms" 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.
The existing HMP command is rewritten to call the QMP command.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 hw/core/loader.c            | 53 +++++++++++++++++++++++++------------
 hw/core/machine-qmp-cmds.c  |  1 +
 include/qapi/type-helpers.h | 14 ++++++++++
 qapi/common.json            | 11 ++++++++
 qapi/machine.json           | 12 +++++++++
 qapi/meson.build            |  3 +++
 qapi/qapi-type-helpers.c    | 23 ++++++++++++++++
 7 files changed, 100 insertions(+), 17 deletions(-)
 create mode 100644 include/qapi/type-helpers.h
 create mode 100644 qapi/qapi-type-helpers.c

diff --git a/hw/core/loader.c b/hw/core/loader.c
index c623318b73..5ebdca3087 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -46,6 +46,8 @@
 #include "qemu-common.h"
 #include "qemu/datadir.h"
 #include "qapi/error.h"
+#include "qapi/qapi-commands-machine.h"
+#include "qapi/type-helpers.h"
 #include "trace.h"
 #include "hw/hw.h"
 #include "disas/disas.h"
@@ -1472,32 +1474,49 @@ void *rom_ptr_for_as(AddressSpace *as, hwaddr addr, size_t size)
     return cbdata.rom;
 }
 
-void hmp_info_roms(Monitor *mon, const QDict *qdict)
+HumanReadableText *qmp_x_query_roms(Error **errp)
 {
     Rom *rom;
+    g_autoptr(GString) buf = g_string_new("");
 
     QTAILQ_FOREACH(rom, &roms, next) {
         if (rom->mr) {
-            monitor_printf(mon, "%s"
-                           " size=0x%06zx name=\"%s\"\n",
-                           memory_region_name(rom->mr),
-                           rom->romsize,
-                           rom->name);
+            g_string_append_printf(buf, "%s"
+                                   " size=0x%06zx name=\"%s\"\n",
+                                   memory_region_name(rom->mr),
+                                   rom->romsize,
+                                   rom->name);
         } else if (!rom->fw_file) {
-            monitor_printf(mon, "addr=" TARGET_FMT_plx
-                           " size=0x%06zx mem=%s name=\"%s\"\n",
-                           rom->addr, rom->romsize,
-                           rom->isrom ? "rom" : "ram",
-                           rom->name);
+            g_string_append_printf(buf, "addr=" TARGET_FMT_plx
+                                   " size=0x%06zx mem=%s name=\"%s\"\n",
+                                   rom->addr, rom->romsize,
+                                   rom->isrom ? "rom" : "ram",
+                                   rom->name);
         } else {
-            monitor_printf(mon, "fw=%s/%s"
-                           " size=0x%06zx name=\"%s\"\n",
-                           rom->fw_dir,
-                           rom->fw_file,
-                           rom->romsize,
-                           rom->name);
+            g_string_append_printf(buf, "fw=%s/%s"
+                                   " size=0x%06zx name=\"%s\"\n",
+                                   rom->fw_dir,
+                                   rom->fw_file,
+                                   rom->romsize,
+                                   rom->name);
         }
     }
+
+    return human_readable_text_from_str(buf);
+}
+
+
+void hmp_info_roms(Monitor *mon, const QDict *qdict)
+{
+    Error *err = NULL;
+    g_autoptr(HumanReadableText) info = qmp_x_query_roms(&err);
+
+    if (err) {
+        error_report_err(err);
+        return;
+    }
+
+    monitor_printf(mon, "%s", info->human_readable_text);
 }
 
 typedef enum HexRecord HexRecord;
diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
index 216fdfaf3a..76f2b84d81 100644
--- a/hw/core/machine-qmp-cmds.c
+++ b/hw/core/machine-qmp-cmds.c
@@ -15,6 +15,7 @@
 #include "qapi/qmp/qerror.h"
 #include "qapi/qmp/qobject.h"
 #include "qapi/qobject-input-visitor.h"
+#include "qapi/type-helpers.h"
 #include "qemu/main-loop.h"
 #include "qom/qom-qobject.h"
 #include "sysemu/hostmem.h"
diff --git a/include/qapi/type-helpers.h b/include/qapi/type-helpers.h
new file mode 100644
index 0000000000..be1f181526
--- /dev/null
+++ b/include/qapi/type-helpers.h
@@ -0,0 +1,14 @@
+/*
+ * QAPI common helper functions
+ *
+ * This file provides helper functions related to types defined
+ * in the QAPI schema.
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#include "qapi/qapi-types-common.h"
+
+HumanReadableText *human_readable_text_from_str(GString *str);
diff --git a/qapi/common.json b/qapi/common.json
index 7c976296f0..412cc4f5ae 100644
--- a/qapi/common.json
+++ b/qapi/common.json
@@ -197,3 +197,14 @@
 { 'enum': 'GrabToggleKeys',
   'data': [ 'ctrl-ctrl', 'alt-alt', 'shift-shift','meta-meta', 'scrolllock',
             'ctrl-scrolllock' ] }
+
+##
+# @HumanReadableText:
+#
+# @human-readable-text: Formatted output intended for humans.
+#
+# Since: 6.2
+#
+##
+{ 'struct': 'HumanReadableText',
+  'data': { 'human-readable-text': 'str' } }
diff --git a/qapi/machine.json b/qapi/machine.json
index 32d47f4e35..4c18904521 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1346,3 +1346,15 @@
      '*cores': 'int',
      '*threads': 'int',
      '*maxcpus': 'int' } }
+
+##
+# @x-query-roms:
+#
+# Query information on the registered ROMS
+#
+# Returns: registered ROMs
+#
+# Since: 6.2
+##
+{ 'command': 'x-query-roms',
+  'returns': 'HumanReadableText' }
diff --git a/qapi/meson.build b/qapi/meson.build
index c356a385e3..c0c49c15e4 100644
--- a/qapi/meson.build
+++ b/qapi/meson.build
@@ -10,6 +10,9 @@ util_ss.add(files(
   'string-input-visitor.c',
   'string-output-visitor.c',
 ))
+if have_system
+  util_ss.add(files('qapi-type-helpers.c'))
+endif
 if have_system or have_tools
   util_ss.add(files(
     'qmp-dispatch.c',
diff --git a/qapi/qapi-type-helpers.c b/qapi/qapi-type-helpers.c
new file mode 100644
index 0000000000..f76b34f647
--- /dev/null
+++ b/qapi/qapi-type-helpers.c
@@ -0,0 +1,23 @@
+/*
+ * QAPI common helper functions
+ *
+ * This file provides helper functions related to types defined
+ * in the QAPI schema.
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "qapi/type-helpers.h"
+
+HumanReadableText *human_readable_text_from_str(GString *str)
+{
+    HumanReadableText *ret = g_new0(HumanReadableText, 1);
+
+    ret->human_readable_text = g_steal_pointer(&str->str);
+
+    return ret;
+}
-- 
2.31.1

Re: [PATCH v3 07/19] qapi: introduce x-query-roms QMP command

[Dr. David Alan Gilbert]

* Daniel P. Berrangé (berrange@redhat.com) wrote:
> This is a counterpart to the HMP "info roms" 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.
> The existing HMP command is rewritten to call the QMP command.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  hw/core/loader.c            | 53 +++++++++++++++++++++++++------------
>  hw/core/machine-qmp-cmds.c  |  1 +
>  include/qapi/type-helpers.h | 14 ++++++++++
>  qapi/common.json            | 11 ++++++++
>  qapi/machine.json           | 12 +++++++++
>  qapi/meson.build            |  3 +++
>  qapi/qapi-type-helpers.c    | 23 ++++++++++++++++
>  7 files changed, 100 insertions(+), 17 deletions(-)
>  create mode 100644 include/qapi/type-helpers.h
>  create mode 100644 qapi/qapi-type-helpers.c
> 
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index c623318b73..5ebdca3087 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -46,6 +46,8 @@
>  #include "qemu-common.h"
>  #include "qemu/datadir.h"
>  #include "qapi/error.h"
> +#include "qapi/qapi-commands-machine.h"
> +#include "qapi/type-helpers.h"
>  #include "trace.h"
>  #include "hw/hw.h"
>  #include "disas/disas.h"
> @@ -1472,32 +1474,49 @@ void *rom_ptr_for_as(AddressSpace *as, hwaddr addr, size_t size)
>      return cbdata.rom;
>  }
>  
> -void hmp_info_roms(Monitor *mon, const QDict *qdict)
> +HumanReadableText *qmp_x_query_roms(Error **errp)
>  {
>      Rom *rom;
> +    g_autoptr(GString) buf = g_string_new("");
>  
>      QTAILQ_FOREACH(rom, &roms, next) {
>          if (rom->mr) {
> -            monitor_printf(mon, "%s"
> -                           " size=0x%06zx name=\"%s\"\n",
> -                           memory_region_name(rom->mr),
> -                           rom->romsize,
> -                           rom->name);
> +            g_string_append_printf(buf, "%s"
> +                                   " size=0x%06zx name=\"%s\"\n",
> +                                   memory_region_name(rom->mr),
> +                                   rom->romsize,
> +                                   rom->name);
>          } else if (!rom->fw_file) {
> -            monitor_printf(mon, "addr=" TARGET_FMT_plx
> -                           " size=0x%06zx mem=%s name=\"%s\"\n",
> -                           rom->addr, rom->romsize,
> -                           rom->isrom ? "rom" : "ram",
> -                           rom->name);
> +            g_string_append_printf(buf, "addr=" TARGET_FMT_plx
> +                                   " size=0x%06zx mem=%s name=\"%s\"\n",
> +                                   rom->addr, rom->romsize,
> +                                   rom->isrom ? "rom" : "ram",
> +                                   rom->name);
>          } else {
> -            monitor_printf(mon, "fw=%s/%s"
> -                           " size=0x%06zx name=\"%s\"\n",
> -                           rom->fw_dir,
> -                           rom->fw_file,
> -                           rom->romsize,
> -                           rom->name);
> +            g_string_append_printf(buf, "fw=%s/%s"
> +                                   " size=0x%06zx name=\"%s\"\n",
> +                                   rom->fw_dir,
> +                                   rom->fw_file,
> +                                   rom->romsize,
> +                                   rom->name);
>          }
>      }
> +
> +    return human_readable_text_from_str(buf);
> +}
> +
> +
> +void hmp_info_roms(Monitor *mon, const QDict *qdict)
> +{
> +    Error *err = NULL;
> +    g_autoptr(HumanReadableText) info = qmp_x_query_roms(&err);
> +
> +    if (err) {
> +        error_report_err(err);
> +        return;
> +    }
> +
> +    monitor_printf(mon, "%s", info->human_readable_text);

This is getting copied in each one of these; it looks like you need
either:
  a) A helper function like:
       void hmp_info_from_qmp(Monitor *mon, HumanReadableText *(void)func) 
       {
           ...
       }

  b) Or teach the hmp parser to do the calls?

Dave

>  }
>  
>  typedef enum HexRecord HexRecord;
> diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
> index 216fdfaf3a..76f2b84d81 100644
> --- a/hw/core/machine-qmp-cmds.c
> +++ b/hw/core/machine-qmp-cmds.c
> @@ -15,6 +15,7 @@
>  #include "qapi/qmp/qerror.h"
>  #include "qapi/qmp/qobject.h"
>  #include "qapi/qobject-input-visitor.h"
> +#include "qapi/type-helpers.h"
>  #include "qemu/main-loop.h"
>  #include "qom/qom-qobject.h"
>  #include "sysemu/hostmem.h"
> diff --git a/include/qapi/type-helpers.h b/include/qapi/type-helpers.h
> new file mode 100644
> index 0000000000..be1f181526
> --- /dev/null
> +++ b/include/qapi/type-helpers.h
> @@ -0,0 +1,14 @@
> +/*
> + * QAPI common helper functions
> + *
> + * This file provides helper functions related to types defined
> + * in the QAPI schema.
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
> + * See the COPYING.LIB file in the top-level directory.
> + *
> + */
> +
> +#include "qapi/qapi-types-common.h"
> +
> +HumanReadableText *human_readable_text_from_str(GString *str);
> diff --git a/qapi/common.json b/qapi/common.json
> index 7c976296f0..412cc4f5ae 100644
> --- a/qapi/common.json
> +++ b/qapi/common.json
> @@ -197,3 +197,14 @@
>  { 'enum': 'GrabToggleKeys',
>    'data': [ 'ctrl-ctrl', 'alt-alt', 'shift-shift','meta-meta', 'scrolllock',
>              'ctrl-scrolllock' ] }
> +
> +##
> +# @HumanReadableText:
> +#
> +# @human-readable-text: Formatted output intended for humans.
> +#
> +# Since: 6.2
> +#
> +##
> +{ 'struct': 'HumanReadableText',
> +  'data': { 'human-readable-text': 'str' } }
> diff --git a/qapi/machine.json b/qapi/machine.json
> index 32d47f4e35..4c18904521 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -1346,3 +1346,15 @@
>       '*cores': 'int',
>       '*threads': 'int',
>       '*maxcpus': 'int' } }
> +
> +##
> +# @x-query-roms:
> +#
> +# Query information on the registered ROMS
> +#
> +# Returns: registered ROMs
> +#
> +# Since: 6.2
> +##
> +{ 'command': 'x-query-roms',
> +  'returns': 'HumanReadableText' }
> diff --git a/qapi/meson.build b/qapi/meson.build
> index c356a385e3..c0c49c15e4 100644
> --- a/qapi/meson.build
> +++ b/qapi/meson.build
> @@ -10,6 +10,9 @@ util_ss.add(files(
>    'string-input-visitor.c',
>    'string-output-visitor.c',
>  ))
> +if have_system
> +  util_ss.add(files('qapi-type-helpers.c'))
> +endif
>  if have_system or have_tools
>    util_ss.add(files(
>      'qmp-dispatch.c',
> diff --git a/qapi/qapi-type-helpers.c b/qapi/qapi-type-helpers.c
> new file mode 100644
> index 0000000000..f76b34f647
> --- /dev/null
> +++ b/qapi/qapi-type-helpers.c
> @@ -0,0 +1,23 @@
> +/*
> + * QAPI common helper functions
> + *
> + * This file provides helper functions related to types defined
> + * in the QAPI schema.
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
> + * See the COPYING.LIB file in the top-level directory.
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "qapi/type-helpers.h"
> +
> +HumanReadableText *human_readable_text_from_str(GString *str)
> +{
> +    HumanReadableText *ret = g_new0(HumanReadableText, 1);
> +
> +    ret->human_readable_text = g_steal_pointer(&str->str);
> +
> +    return ret;
> +}
> -- 
> 2.31.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v3 07/19] qapi: introduce x-query-roms QMP command

[Daniel P. Berrangé]

On Mon, Oct 04, 2021 at 01:32:14PM +0100, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrangé (berrange@redhat.com) wrote:
> > This is a counterpart to the HMP "info roms" 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.
> > The existing HMP command is rewritten to call the QMP command.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  hw/core/loader.c            | 53 +++++++++++++++++++++++++------------
> >  hw/core/machine-qmp-cmds.c  |  1 +
> >  include/qapi/type-helpers.h | 14 ++++++++++
> >  qapi/common.json            | 11 ++++++++
> >  qapi/machine.json           | 12 +++++++++
> >  qapi/meson.build            |  3 +++
> >  qapi/qapi-type-helpers.c    | 23 ++++++++++++++++
> >  7 files changed, 100 insertions(+), 17 deletions(-)
> >  create mode 100644 include/qapi/type-helpers.h
> >  create mode 100644 qapi/qapi-type-helpers.c
> > 
> > diff --git a/hw/core/loader.c b/hw/core/loader.c
> > index c623318b73..5ebdca3087 100644
> > --- a/hw/core/loader.c
> > +++ b/hw/core/loader.c
> > @@ -46,6 +46,8 @@
> >  #include "qemu-common.h"
> >  #include "qemu/datadir.h"
> >  #include "qapi/error.h"
> > +#include "qapi/qapi-commands-machine.h"
> > +#include "qapi/type-helpers.h"
> >  #include "trace.h"
> >  #include "hw/hw.h"
> >  #include "disas/disas.h"
> > @@ -1472,32 +1474,49 @@ void *rom_ptr_for_as(AddressSpace *as, hwaddr addr, size_t size)
> >      return cbdata.rom;
> >  }
> >  
> > -void hmp_info_roms(Monitor *mon, const QDict *qdict)
> > +HumanReadableText *qmp_x_query_roms(Error **errp)
> >  {
> >      Rom *rom;
> > +    g_autoptr(GString) buf = g_string_new("");
> >  
> >      QTAILQ_FOREACH(rom, &roms, next) {
> >          if (rom->mr) {
> > -            monitor_printf(mon, "%s"
> > -                           " size=0x%06zx name=\"%s\"\n",
> > -                           memory_region_name(rom->mr),
> > -                           rom->romsize,
> > -                           rom->name);
> > +            g_string_append_printf(buf, "%s"
> > +                                   " size=0x%06zx name=\"%s\"\n",
> > +                                   memory_region_name(rom->mr),
> > +                                   rom->romsize,
> > +                                   rom->name);
> >          } else if (!rom->fw_file) {
> > -            monitor_printf(mon, "addr=" TARGET_FMT_plx
> > -                           " size=0x%06zx mem=%s name=\"%s\"\n",
> > -                           rom->addr, rom->romsize,
> > -                           rom->isrom ? "rom" : "ram",
> > -                           rom->name);
> > +            g_string_append_printf(buf, "addr=" TARGET_FMT_plx
> > +                                   " size=0x%06zx mem=%s name=\"%s\"\n",
> > +                                   rom->addr, rom->romsize,
> > +                                   rom->isrom ? "rom" : "ram",
> > +                                   rom->name);
> >          } else {
> > -            monitor_printf(mon, "fw=%s/%s"
> > -                           " size=0x%06zx name=\"%s\"\n",
> > -                           rom->fw_dir,
> > -                           rom->fw_file,
> > -                           rom->romsize,
> > -                           rom->name);
> > +            g_string_append_printf(buf, "fw=%s/%s"
> > +                                   " size=0x%06zx name=\"%s\"\n",
> > +                                   rom->fw_dir,
> > +                                   rom->fw_file,
> > +                                   rom->romsize,
> > +                                   rom->name);
> >          }
> >      }
> > +
> > +    return human_readable_text_from_str(buf);
> > +}
> > +
> > +
> > +void hmp_info_roms(Monitor *mon, const QDict *qdict)
> > +{
> > +    Error *err = NULL;
> > +    g_autoptr(HumanReadableText) info = qmp_x_query_roms(&err);
> > +
> > +    if (err) {
> > +        error_report_err(err);
> > +        return;
> > +    }
> > +
> > +    monitor_printf(mon, "%s", info->human_readable_text);
> 
> This is getting copied in each one of these; it looks like you need
> either:
>   a) A helper function like:
>        void hmp_info_from_qmp(Monitor *mon, HumanReadableText *(void)func) 
>        {
>            ...
>        }
> 
>   b) Or teach the hmp parser to do the calls?


This pattern is repeated in many, but not all, or the handlers in
this series, because I started with the easy cases of no-arg 'info'
commands. There's a few exceptions such as commands which drive off
the currently selected CPU state.  I'm not convince it is worth
adding specials to the hmp parser, since it will only be used for
a subset of the commands lng term. A helper function though could
do the job, since I've already introduced a helper for the QMP
side converting GString to HumanReadableText


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 :|

[PATCH v3 08/19] qapi: introduce x-query-profile QMP command

[Daniel P. Berrangé]

This is a counterpart to the HMP "info profile" 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.
The existing HMP command is rewritten to call the QMP command.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 monitor/misc.c             | 30 ++++++++----------------------
 monitor/qmp-cmds.c         | 32 ++++++++++++++++++++++++++++++++
 qapi/machine.json          | 12 ++++++++++++
 tests/qtest/qmp-cmd-test.c |  3 +++
 4 files changed, 55 insertions(+), 22 deletions(-)

diff --git a/monitor/misc.c b/monitor/misc.c
index ffe7966870..5aebfaa986 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"
@@ -929,32 +930,17 @@ static void hmp_info_mtree(Monitor *mon, const QDict *qdict)
     mtree_info(flatview, dispatch_tree, owner, disabled);
 }
 
-#ifdef CONFIG_PROFILER
-
-int64_t dev_time;
-
 static void hmp_info_profile(Monitor *mon, const QDict *qdict)
 {
-    static int64_t last_cpu_exec_time;
-    int64_t cpu_exec_time;
-    int64_t delta;
-
-    cpu_exec_time = tcg_cpu_exec_time();
-    delta = cpu_exec_time - last_cpu_exec_time;
+    Error *err = NULL;
+    g_autoptr(HumanReadableText) info = qmp_x_query_profile(&err);
 
-    monitor_printf(mon, "async time  %" PRId64 " (%0.3f)\n",
-                   dev_time, dev_time / (double)NANOSECONDS_PER_SECOND);
-    monitor_printf(mon, "qemu time   %" PRId64 " (%0.3f)\n",
-                   delta, delta / (double)NANOSECONDS_PER_SECOND);
-    last_cpu_exec_time = cpu_exec_time;
-    dev_time = 0;
-}
-#else
-static void hmp_info_profile(Monitor *mon, const QDict *qdict)
-{
-    monitor_printf(mon, "Internal profiler not compiled\n");
+    if (err) {
+        error_report_err(err);
+        return;
+    }
+    monitor_printf(mon, "%s", info->human_readable_text);
 }
-#endif
 
 /* Capture support */
 static QLIST_HEAD (capture_list_head, CaptureState) capture_head;
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index 5c0d5e116b..6122ad18b6 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -36,6 +36,7 @@
 #include "qapi/qapi-commands-machine.h"
 #include "qapi/qapi-commands-misc.h"
 #include "qapi/qapi-commands-ui.h"
+#include "qapi/type-helpers.h"
 #include "qapi/qmp/qerror.h"
 #include "hw/mem/memory-device.h"
 #include "hw/acpi/acpi_dev_interface.h"
@@ -350,3 +351,34 @@ void qmp_display_reload(DisplayReloadOptions *arg, Error **errp)
         abort();
     }
 }
+
+#ifdef CONFIG_PROFILER
+
+int64_t dev_time;
+
+HumanReadableText *qmp_x_query_profile(Error **errp)
+{
+    g_autoptr(GString) buf = g_string_new("");
+    static int64_t last_cpu_exec_time;
+    int64_t cpu_exec_time;
+    int64_t delta;
+
+    cpu_exec_time = tcg_cpu_exec_time();
+    delta = cpu_exec_time - last_cpu_exec_time;
+
+    g_string_append_printf(buf, "async time  %" PRId64 " (%0.3f)\n",
+                           dev_time, dev_time / (double)NANOSECONDS_PER_SECOND);
+    g_string_append_printf(buf, "qemu time   %" PRId64 " (%0.3f)\n",
+                           delta, delta / (double)NANOSECONDS_PER_SECOND);
+    last_cpu_exec_time = cpu_exec_time;
+    dev_time = 0;
+
+    return human_readable_text_from_str(buf);
+}
+#else
+HumanReadableText *qmp_x_query_profile(Error **errp)
+{
+    error_setg(errp, "Internal profiler not compiled");
+    return NULL;
+}
+#endif
diff --git a/qapi/machine.json b/qapi/machine.json
index 4c18904521..db1bb9454e 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1347,6 +1347,18 @@
      '*threads': 'int',
      '*maxcpus': 'int' } }
 
+##
+# @x-query-profile:
+#
+# Query TCG profiling information
+#
+# Returns: profile information
+#
+# Since: 6.2
+##
+{ 'command': 'x-query-profile',
+  'returns': 'HumanReadableText' }
+
 ##
 # @x-query-roms:
 #
diff --git a/tests/qtest/qmp-cmd-test.c b/tests/qtest/qmp-cmd-test.c
index c98b78d033..fbd7ac10fb 100644
--- a/tests/qtest/qmp-cmd-test.c
+++ b/tests/qtest/qmp-cmd-test.c
@@ -46,6 +46,9 @@ static int query_error_class(const char *cmd)
         { "query-balloon", ERROR_CLASS_DEVICE_NOT_ACTIVE },
         { "query-hotpluggable-cpus", ERROR_CLASS_GENERIC_ERROR },
         { "query-vm-generation-id", ERROR_CLASS_GENERIC_ERROR },
+#ifndef CONFIG_PROFILER
+        { "x-query-profile", ERROR_CLASS_GENERIC_ERROR },
+#endif
         { NULL, -1 }
     };
     int i;
-- 
2.31.1

[PATCH v3 09/19] qapi: introduce x-query-numa QMP command

[Daniel P. Berrangé]

This is a counterpart to the HMP "info numa" 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.
The existing HMP command is rewritten to call the QMP command.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 hw/core/machine-hmp-cmds.c | 33 +++++---------------------------
 hw/core/machine-qmp-cmds.c | 39 ++++++++++++++++++++++++++++++++++++++
 qapi/machine.json          | 12 ++++++++++++
 3 files changed, 56 insertions(+), 28 deletions(-)

diff --git a/hw/core/machine-hmp-cmds.c b/hw/core/machine-hmp-cmds.c
index 76b22b00d6..cfa01c6933 100644
--- a/hw/core/machine-hmp-cmds.c
+++ b/hw/core/machine-hmp-cmds.c
@@ -134,35 +134,12 @@ void hmp_info_memdev(Monitor *mon, const QDict *qdict)
 
 void hmp_info_numa(Monitor *mon, const QDict *qdict)
 {
-    int i, nb_numa_nodes;
-    NumaNodeMem *node_mem;
-    CpuInfoFastList *cpu_list, *cpu;
-    MachineState *ms = MACHINE(qdev_get_machine());
+    Error *err = NULL;
+    g_autoptr(HumanReadableText) info = qmp_x_query_numa(&err);
 
-    nb_numa_nodes = ms->numa_state ? ms->numa_state->num_nodes : 0;
-    monitor_printf(mon, "%d nodes\n", nb_numa_nodes);
-    if (!nb_numa_nodes) {
+    if (err) {
+        error_report_err(err);
         return;
     }
-
-    cpu_list = qmp_query_cpus_fast(&error_abort);
-    node_mem = g_new0(NumaNodeMem, nb_numa_nodes);
-
-    query_numa_node_mem(node_mem, ms);
-    for (i = 0; i < nb_numa_nodes; i++) {
-        monitor_printf(mon, "node %d cpus:", i);
-        for (cpu = cpu_list; cpu; cpu = cpu->next) {
-            if (cpu->value->has_props && cpu->value->props->has_node_id &&
-                cpu->value->props->node_id == i) {
-                monitor_printf(mon, " %" PRIi64, cpu->value->cpu_index);
-            }
-        }
-        monitor_printf(mon, "\n");
-        monitor_printf(mon, "node %d size: %" PRId64 " MB\n", i,
-                       node_mem[i].node_mem >> 20);
-        monitor_printf(mon, "node %d plugged: %" PRId64 " MB\n", i,
-                       node_mem[i].node_plugged_mem >> 20);
-    }
-    qapi_free_CpuInfoFastList(cpu_list);
-    g_free(node_mem);
+    monitor_printf(mon, "%s", info->human_readable_text);
 }
diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
index 76f2b84d81..4f4ab30f8c 100644
--- a/hw/core/machine-qmp-cmds.c
+++ b/hw/core/machine-qmp-cmds.c
@@ -205,3 +205,42 @@ MemdevList *qmp_query_memdev(Error **errp)
     object_child_foreach(obj, query_memdev, &list);
     return list;
 }
+
+HumanReadableText *qmp_x_query_numa(Error **errp)
+{
+    g_autoptr(GString) buf = g_string_new("");
+    int i, nb_numa_nodes;
+    NumaNodeMem *node_mem;
+    CpuInfoFastList *cpu_list, *cpu;
+    MachineState *ms = MACHINE(qdev_get_machine());
+
+    nb_numa_nodes = ms->numa_state ? ms->numa_state->num_nodes : 0;
+    g_string_append_printf(buf, "%d nodes\n", nb_numa_nodes);
+    if (!nb_numa_nodes) {
+        goto done;
+    }
+
+    cpu_list = qmp_query_cpus_fast(&error_abort);
+    node_mem = g_new0(NumaNodeMem, nb_numa_nodes);
+
+    query_numa_node_mem(node_mem, ms);
+    for (i = 0; i < nb_numa_nodes; i++) {
+        g_string_append_printf(buf, "node %d cpus:", i);
+        for (cpu = cpu_list; cpu; cpu = cpu->next) {
+            if (cpu->value->has_props && cpu->value->props->has_node_id &&
+                cpu->value->props->node_id == i) {
+                g_string_append_printf(buf, " %" PRIi64, cpu->value->cpu_index);
+            }
+        }
+        g_string_append_printf(buf, "\n");
+        g_string_append_printf(buf, "node %d size: %" PRId64 " MB\n", i,
+                               node_mem[i].node_mem >> 20);
+        g_string_append_printf(buf, "node %d plugged: %" PRId64 " MB\n", i,
+                               node_mem[i].node_plugged_mem >> 20);
+    }
+    qapi_free_CpuInfoFastList(cpu_list);
+    g_free(node_mem);
+
+ done:
+    return human_readable_text_from_str(buf);
+}
diff --git a/qapi/machine.json b/qapi/machine.json
index db1bb9454e..c69a7ceef6 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1347,6 +1347,18 @@
      '*threads': 'int',
      '*maxcpus': 'int' } }
 
+##
+# @x-query-numa:
+#
+# Query NUMA topology information
+#
+# Returns: topology information
+#
+# Since: 6.2
+##
+{ 'command': 'x-query-numa',
+  'returns': 'HumanReadableText' }
+
 ##
 # @x-query-profile:
 #
-- 
2.31.1

[PATCH v3 10/19] qapi: introduce x-query-usb QMP command

[Daniel P. Berrangé]

This is a counterpart to the HMP "info usb" 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.
The existing HMP command is rewritten to call the QMP command.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 hw/usb/bus.c               | 36 +++++++++++++++++++++++++++---------
 qapi/machine.json          | 12 ++++++++++++
 stubs/usb-dev-stub.c       |  8 ++++++++
 tests/qtest/qmp-cmd-test.c |  2 ++
 4 files changed, 49 insertions(+), 9 deletions(-)

diff --git a/hw/usb/bus.c b/hw/usb/bus.c
index 07083349f5..17096e9e95 100644
--- a/hw/usb/bus.c
+++ b/hw/usb/bus.c
@@ -2,6 +2,8 @@
 #include "hw/qdev-properties.h"
 #include "hw/usb.h"
 #include "qapi/error.h"
+#include "qapi/qapi-commands-machine.h"
+#include "qapi/type-helpers.h"
 #include "qemu/error-report.h"
 #include "qemu/module.h"
 #include "sysemu/sysemu.h"
@@ -631,15 +633,16 @@ static char *usb_get_fw_dev_path(DeviceState *qdev)
     return fw_path;
 }
 
-void hmp_info_usb(Monitor *mon, const QDict *qdict)
+HumanReadableText *qmp_x_query_usb(Error **errp)
 {
+    g_autoptr(GString) buf = g_string_new("");
     USBBus *bus;
     USBDevice *dev;
     USBPort *port;
 
     if (QTAILQ_EMPTY(&busses)) {
-        monitor_printf(mon, "USB support not enabled\n");
-        return;
+        error_setg(errp, "USB support not enabled");
+        return NULL;
     }
 
     QTAILQ_FOREACH(bus, &busses, next) {
@@ -647,14 +650,29 @@ void hmp_info_usb(Monitor *mon, const QDict *qdict)
             dev = port->dev;
             if (!dev)
                 continue;
-            monitor_printf(mon, "  Device %d.%d, Port %s, Speed %s Mb/s, "
-                           "Product %s%s%s\n",
-                           bus->busnr, dev->addr, port->path,
-                           usb_speed(dev->speed), dev->product_desc,
-                           dev->qdev.id ? ", ID: " : "",
-                           dev->qdev.id ?: "");
+            g_string_append_printf(buf,
+                                   "  Device %d.%d, Port %s, Speed %s Mb/s, "
+                                   "Product %s%s%s\n",
+                                   bus->busnr, dev->addr, port->path,
+                                   usb_speed(dev->speed), dev->product_desc,
+                                   dev->qdev.id ? ", ID: " : "",
+                                   dev->qdev.id ?: "");
         }
     }
+
+    return human_readable_text_from_str(buf);
+}
+
+void hmp_info_usb(Monitor *mon, const QDict *qdict)
+{
+    Error *err = NULL;
+    g_autoptr(HumanReadableText) info = qmp_x_query_usb(&err);
+
+    if (err) {
+        error_report_err(err);
+        return;
+    }
+    monitor_printf(mon, "%s", info->human_readable_text);
 }
 
 /* handle legacy -usbdevice cmd line option */
diff --git a/qapi/machine.json b/qapi/machine.json
index c69a7ceef6..7450fa3ac1 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1382,3 +1382,15 @@
 ##
 { 'command': 'x-query-roms',
   'returns': 'HumanReadableText' }
+
+##
+# @x-query-usb:
+#
+# Query information on the USB devices
+#
+# Returns: USB device information
+#
+# Since: 6.2
+##
+{ 'command': 'x-query-usb',
+  'returns': 'HumanReadableText' }
diff --git a/stubs/usb-dev-stub.c b/stubs/usb-dev-stub.c
index b1adeeb454..aa557692b7 100644
--- a/stubs/usb-dev-stub.c
+++ b/stubs/usb-dev-stub.c
@@ -8,6 +8,8 @@
 
 #include "qemu/osdep.h"
 #include "qemu/error-report.h"
+#include "qapi/error.h"
+#include "qapi/qapi-commands-machine.h"
 #include "sysemu/sysemu.h"
 #include "monitor/monitor.h"
 #include "hw/usb.h"
@@ -19,6 +21,12 @@ USBDevice *usbdevice_create(const char *driver)
     return NULL;
 }
 
+HumanReadableText *qmp_x_query_usb(Error **errp)
+{
+    error_setg(errp, "Support for USB devices not built-in");
+    return NULL;
+}
+
 void hmp_info_usb(Monitor *mon, const QDict *qdict)
 {
     monitor_printf(mon, "Support for USB devices not built-in\n");
diff --git a/tests/qtest/qmp-cmd-test.c b/tests/qtest/qmp-cmd-test.c
index fbd7ac10fb..15875a14c6 100644
--- a/tests/qtest/qmp-cmd-test.c
+++ b/tests/qtest/qmp-cmd-test.c
@@ -49,6 +49,8 @@ static int query_error_class(const char *cmd)
 #ifndef CONFIG_PROFILER
         { "x-query-profile", ERROR_CLASS_GENERIC_ERROR },
 #endif
+        /* Only valid with a USB bus added */
+        { "x-query-usb", ERROR_CLASS_GENERIC_ERROR },
         { NULL, -1 }
     };
     int i;
-- 
2.31.1

[PATCH v3 11/19] qapi: introduce x-query-rdma QMP command

[Daniel P. Berrangé]

This is a counterpart to the HMP "info rdma" 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.
The existing HMP command is rewritten to call the QMP command.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 hw/rdma/rdma_rm.c         | 104 +++++++++++++++++++-------------------
 hw/rdma/rdma_rm.h         |   2 +-
 hw/rdma/vmw/pvrdma_main.c |  31 ++++++------
 include/hw/rdma/rdma.h    |   2 +-
 monitor/hmp-cmds.c        |  29 +++--------
 monitor/qmp-cmds.c        |  32 ++++++++++++
 qapi/machine.json         |  12 +++++
 7 files changed, 122 insertions(+), 90 deletions(-)

diff --git a/hw/rdma/rdma_rm.c b/hw/rdma/rdma_rm.c
index 49141d4074..cfd85de3e6 100644
--- a/hw/rdma/rdma_rm.c
+++ b/hw/rdma/rdma_rm.c
@@ -27,58 +27,58 @@
 #define PG_DIR_SZ { TARGET_PAGE_SIZE / sizeof(__u64) }
 #define PG_TBL_SZ { TARGET_PAGE_SIZE / sizeof(__u64) }
 
-void rdma_dump_device_counters(Monitor *mon, RdmaDeviceResources *dev_res)
-{
-    monitor_printf(mon, "\ttx               : %" PRId64 "\n",
-                   dev_res->stats.tx);
-    monitor_printf(mon, "\ttx_len           : %" PRId64 "\n",
-                   dev_res->stats.tx_len);
-    monitor_printf(mon, "\ttx_err           : %" PRId64 "\n",
-                   dev_res->stats.tx_err);
-    monitor_printf(mon, "\trx_bufs          : %" PRId64 "\n",
-                   dev_res->stats.rx_bufs);
-    monitor_printf(mon, "\trx_srq           : %" PRId64 "\n",
-                   dev_res->stats.rx_srq);
-    monitor_printf(mon, "\trx_bufs_len      : %" PRId64 "\n",
-                   dev_res->stats.rx_bufs_len);
-    monitor_printf(mon, "\trx_bufs_err      : %" PRId64 "\n",
-                   dev_res->stats.rx_bufs_err);
-    monitor_printf(mon, "\tcomps            : %" PRId64 "\n",
-                   dev_res->stats.completions);
-    monitor_printf(mon, "\tmissing_comps    : %" PRId32 "\n",
-                   dev_res->stats.missing_cqe);
-    monitor_printf(mon, "\tpoll_cq (bk)     : %" PRId64 "\n",
-                   dev_res->stats.poll_cq_from_bk);
-    monitor_printf(mon, "\tpoll_cq_ppoll_to : %" PRId64 "\n",
-                   dev_res->stats.poll_cq_ppoll_to);
-    monitor_printf(mon, "\tpoll_cq (fe)     : %" PRId64 "\n",
-                   dev_res->stats.poll_cq_from_guest);
-    monitor_printf(mon, "\tpoll_cq_empty    : %" PRId64 "\n",
-                   dev_res->stats.poll_cq_from_guest_empty);
-    monitor_printf(mon, "\tmad_tx           : %" PRId64 "\n",
-                   dev_res->stats.mad_tx);
-    monitor_printf(mon, "\tmad_tx_err       : %" PRId64 "\n",
-                   dev_res->stats.mad_tx_err);
-    monitor_printf(mon, "\tmad_rx           : %" PRId64 "\n",
-                   dev_res->stats.mad_rx);
-    monitor_printf(mon, "\tmad_rx_err       : %" PRId64 "\n",
-                   dev_res->stats.mad_rx_err);
-    monitor_printf(mon, "\tmad_rx_bufs      : %" PRId64 "\n",
-                   dev_res->stats.mad_rx_bufs);
-    monitor_printf(mon, "\tmad_rx_bufs_err  : %" PRId64 "\n",
-                   dev_res->stats.mad_rx_bufs_err);
-    monitor_printf(mon, "\tPDs              : %" PRId32 "\n",
-                   dev_res->pd_tbl.used);
-    monitor_printf(mon, "\tMRs              : %" PRId32 "\n",
-                   dev_res->mr_tbl.used);
-    monitor_printf(mon, "\tUCs              : %" PRId32 "\n",
-                   dev_res->uc_tbl.used);
-    monitor_printf(mon, "\tQPs              : %" PRId32 "\n",
-                   dev_res->qp_tbl.used);
-    monitor_printf(mon, "\tCQs              : %" PRId32 "\n",
-                   dev_res->cq_tbl.used);
-    monitor_printf(mon, "\tCEQ_CTXs         : %" PRId32 "\n",
-                   dev_res->cqe_ctx_tbl.used);
+void rdma_format_device_counters(RdmaDeviceResources *dev_res, GString *buf)
+{
+    g_string_append_printf(buf, "\ttx               : %" PRId64 "\n",
+                           dev_res->stats.tx);
+    g_string_append_printf(buf, "\ttx_len           : %" PRId64 "\n",
+                           dev_res->stats.tx_len);
+    g_string_append_printf(buf, "\ttx_err           : %" PRId64 "\n",
+                           dev_res->stats.tx_err);
+    g_string_append_printf(buf, "\trx_bufs          : %" PRId64 "\n",
+                           dev_res->stats.rx_bufs);
+    g_string_append_printf(buf, "\trx_srq           : %" PRId64 "\n",
+                           dev_res->stats.rx_srq);
+    g_string_append_printf(buf, "\trx_bufs_len      : %" PRId64 "\n",
+                           dev_res->stats.rx_bufs_len);
+    g_string_append_printf(buf, "\trx_bufs_err      : %" PRId64 "\n",
+                           dev_res->stats.rx_bufs_err);
+    g_string_append_printf(buf, "\tcomps            : %" PRId64 "\n",
+                           dev_res->stats.completions);
+    g_string_append_printf(buf, "\tmissing_comps    : %" PRId32 "\n",
+                           dev_res->stats.missing_cqe);
+    g_string_append_printf(buf, "\tpoll_cq (bk)     : %" PRId64 "\n",
+                           dev_res->stats.poll_cq_from_bk);
+    g_string_append_printf(buf, "\tpoll_cq_ppoll_to : %" PRId64 "\n",
+                           dev_res->stats.poll_cq_ppoll_to);
+    g_string_append_printf(buf, "\tpoll_cq (fe)     : %" PRId64 "\n",
+                           dev_res->stats.poll_cq_from_guest);
+    g_string_append_printf(buf, "\tpoll_cq_empty    : %" PRId64 "\n",
+                           dev_res->stats.poll_cq_from_guest_empty);
+    g_string_append_printf(buf, "\tmad_tx           : %" PRId64 "\n",
+                           dev_res->stats.mad_tx);
+    g_string_append_printf(buf, "\tmad_tx_err       : %" PRId64 "\n",
+                           dev_res->stats.mad_tx_err);
+    g_string_append_printf(buf, "\tmad_rx           : %" PRId64 "\n",
+                           dev_res->stats.mad_rx);
+    g_string_append_printf(buf, "\tmad_rx_err       : %" PRId64 "\n",
+                           dev_res->stats.mad_rx_err);
+    g_string_append_printf(buf, "\tmad_rx_bufs      : %" PRId64 "\n",
+                           dev_res->stats.mad_rx_bufs);
+    g_string_append_printf(buf, "\tmad_rx_bufs_err  : %" PRId64 "\n",
+                           dev_res->stats.mad_rx_bufs_err);
+    g_string_append_printf(buf, "\tPDs              : %" PRId32 "\n",
+                           dev_res->pd_tbl.used);
+    g_string_append_printf(buf, "\tMRs              : %" PRId32 "\n",
+                           dev_res->mr_tbl.used);
+    g_string_append_printf(buf, "\tUCs              : %" PRId32 "\n",
+                           dev_res->uc_tbl.used);
+    g_string_append_printf(buf, "\tQPs              : %" PRId32 "\n",
+                           dev_res->qp_tbl.used);
+    g_string_append_printf(buf, "\tCQs              : %" PRId32 "\n",
+                           dev_res->cq_tbl.used);
+    g_string_append_printf(buf, "\tCEQ_CTXs         : %" PRId32 "\n",
+                           dev_res->cqe_ctx_tbl.used);
 }
 
 static inline void res_tbl_init(const char *name, RdmaRmResTbl *tbl,
diff --git a/hw/rdma/rdma_rm.h b/hw/rdma/rdma_rm.h
index e8639909cd..d69a917795 100644
--- a/hw/rdma/rdma_rm.h
+++ b/hw/rdma/rdma_rm.h
@@ -92,6 +92,6 @@ static inline union ibv_gid *rdma_rm_get_gid(RdmaDeviceResources *dev_res,
 {
     return &dev_res->port.gid_tbl[sgid_idx].gid;
 }
-void rdma_dump_device_counters(Monitor *mon, RdmaDeviceResources *dev_res);
+void rdma_format_device_counters(RdmaDeviceResources *dev_res, GString *buf);
 
 #endif
diff --git a/hw/rdma/vmw/pvrdma_main.c b/hw/rdma/vmw/pvrdma_main.c
index 7c0c3551a8..91206dbb8e 100644
--- a/hw/rdma/vmw/pvrdma_main.c
+++ b/hw/rdma/vmw/pvrdma_main.c
@@ -58,24 +58,25 @@ static Property pvrdma_dev_properties[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
-static void pvrdma_print_statistics(Monitor *mon, RdmaProvider *obj)
+static void pvrdma_format_statistics(RdmaProvider *obj, GString *buf)
 {
     PVRDMADev *dev = PVRDMA_DEV(obj);
     PCIDevice *pdev = PCI_DEVICE(dev);
 
-    monitor_printf(mon, "%s, %x.%x\n", pdev->name, PCI_SLOT(pdev->devfn),
-                   PCI_FUNC(pdev->devfn));
-    monitor_printf(mon, "\tcommands         : %" PRId64 "\n",
-                   dev->stats.commands);
-    monitor_printf(mon, "\tregs_reads       : %" PRId64 "\n",
-                   dev->stats.regs_reads);
-    monitor_printf(mon, "\tregs_writes      : %" PRId64 "\n",
-                   dev->stats.regs_writes);
-    monitor_printf(mon, "\tuar_writes       : %" PRId64 "\n",
-                   dev->stats.uar_writes);
-    monitor_printf(mon, "\tinterrupts       : %" PRId64 "\n",
-                   dev->stats.interrupts);
-    rdma_dump_device_counters(mon, &dev->rdma_dev_res);
+    g_string_append_printf(buf, "%s, %x.%x\n",
+                           pdev->name, PCI_SLOT(pdev->devfn),
+                           PCI_FUNC(pdev->devfn));
+    g_string_append_printf(buf, "\tcommands         : %" PRId64 "\n",
+                           dev->stats.commands);
+    g_string_append_printf(buf, "\tregs_reads       : %" PRId64 "\n",
+                           dev->stats.regs_reads);
+    g_string_append_printf(buf, "\tregs_writes      : %" PRId64 "\n",
+                           dev->stats.regs_writes);
+    g_string_append_printf(buf, "\tuar_writes       : %" PRId64 "\n",
+                           dev->stats.uar_writes);
+    g_string_append_printf(buf, "\tinterrupts       : %" PRId64 "\n",
+                           dev->stats.interrupts);
+    rdma_format_device_counters(&dev->rdma_dev_res, buf);
 }
 
 static void free_dev_ring(PCIDevice *pci_dev, PvrdmaRing *ring,
@@ -699,7 +700,7 @@ static void pvrdma_class_init(ObjectClass *klass, void *data)
     device_class_set_props(dc, pvrdma_dev_properties);
     set_bit(DEVICE_CATEGORY_NETWORK, dc->categories);
 
-    ir->print_statistics = pvrdma_print_statistics;
+    ir->format_statistics = pvrdma_format_statistics;
 }
 
 static const TypeInfo pvrdma_info = {
diff --git a/include/hw/rdma/rdma.h b/include/hw/rdma/rdma.h
index e77e43a170..80b2e531c4 100644
--- a/include/hw/rdma/rdma.h
+++ b/include/hw/rdma/rdma.h
@@ -31,7 +31,7 @@ typedef struct RdmaProvider RdmaProvider;
 struct RdmaProviderClass {
     InterfaceClass parent;
 
-    void (*print_statistics)(Monitor *mon, RdmaProvider *obj);
+    void (*format_statistics)(RdmaProvider *obj, GString *buf);
 };
 
 #endif
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index b5e71d9e6f..ebc093b3d9 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -54,7 +54,6 @@
 #include "qemu/error-report.h"
 #include "exec/ramlist.h"
 #include "hw/intc/intc.h"
-#include "hw/rdma/rdma.h"
 #include "migration/snapshot.h"
 #include "migration/misc.h"
 
@@ -850,30 +849,18 @@ void hmp_info_pic(Monitor *mon, const QDict *qdict)
                                    hmp_info_pic_foreach, mon);
 }
 
-static int hmp_info_rdma_foreach(Object *obj, void *opaque)
+void hmp_info_rdma(Monitor *mon, const QDict *qdict)
 {
-    RdmaProvider *rdma;
-    RdmaProviderClass *k;
-    Monitor *mon = opaque;
+    Error *err = NULL;
+    g_autoptr(HumanReadableText) info = NULL;
 
-    if (object_dynamic_cast(obj, INTERFACE_RDMA_PROVIDER)) {
-        rdma = RDMA_PROVIDER(obj);
-        k = RDMA_PROVIDER_GET_CLASS(obj);
-        if (k->print_statistics) {
-            k->print_statistics(mon, rdma);
-        } else {
-            monitor_printf(mon, "RDMA statistics not available for %s.\n",
-                           object_get_typename(obj));
-        }
+    info = qmp_x_query_rdma(&err);
+    if (err) {
+        error_report_err(err);
+        return;
     }
 
-    return 0;
-}
-
-void hmp_info_rdma(Monitor *mon, const QDict *qdict)
-{
-    object_child_foreach_recursive(object_get_root(),
-                                   hmp_info_rdma_foreach, mon);
+    monitor_printf(mon, "%s", info->human_readable_text);
 }
 
 void hmp_info_pci(Monitor *mon, const QDict *qdict)
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index 6122ad18b6..0a9ba7595c 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -40,6 +40,7 @@
 #include "qapi/qmp/qerror.h"
 #include "hw/mem/memory-device.h"
 #include "hw/acpi/acpi_dev_interface.h"
+#include "hw/rdma/rdma.h"
 
 NameInfo *qmp_query_name(Error **errp)
 {
@@ -382,3 +383,34 @@ HumanReadableText *qmp_x_query_profile(Error **errp)
     return NULL;
 }
 #endif
+
+static int qmp_x_query_rdma_foreach(Object *obj, void *opaque)
+{
+    RdmaProvider *rdma;
+    RdmaProviderClass *k;
+    GString *buf = opaque;
+
+    if (object_dynamic_cast(obj, INTERFACE_RDMA_PROVIDER)) {
+        rdma = RDMA_PROVIDER(obj);
+        k = RDMA_PROVIDER_GET_CLASS(obj);
+        if (k->format_statistics) {
+            k->format_statistics(rdma, buf);
+        } else {
+            g_string_append_printf(buf,
+                                   "RDMA statistics not available for %s.\n",
+                                   object_get_typename(obj));
+        }
+    }
+
+    return 0;
+}
+
+HumanReadableText *qmp_x_query_rdma(Error **errp)
+{
+    g_autoptr(GString) buf = g_string_new("");
+
+    object_child_foreach_recursive(object_get_root(),
+                                   qmp_x_query_rdma_foreach, buf);
+
+    return human_readable_text_from_str(buf);
+}
diff --git a/qapi/machine.json b/qapi/machine.json
index 7450fa3ac1..fbe4100443 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1371,6 +1371,18 @@
 { 'command': 'x-query-profile',
   'returns': 'HumanReadableText' }
 
+##
+# @x-query-rdma:
+#
+# Query RDMA state
+#
+# Returns: RDMA state
+#
+# Since: 6.2
+##
+{ 'command': 'x-query-rdma',
+  'returns': 'HumanReadableText' }
+
 ##
 # @x-query-roms:
 #
-- 
2.31.1

[PATCH v3 12/19] qapi: introduce x-query-ramblock QMP command

[Daniel P. Berrangé]

This is a counterpart to the HMP "info ramblock" 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.
The existing HMP command is rewritten to call the QMP command.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 include/exec/ramlist.h |  2 +-
 monitor/hmp-cmds.c     | 12 ++++++++++--
 monitor/qmp-cmds.c     |  8 ++++++++
 qapi/machine.json      | 12 ++++++++++++
 softmmu/physmem.c      | 19 +++++++++++--------
 5 files changed, 42 insertions(+), 11 deletions(-)

diff --git a/include/exec/ramlist.h b/include/exec/ramlist.h
index ece6497ee2..2ad2a81acc 100644
--- a/include/exec/ramlist.h
+++ b/include/exec/ramlist.h
@@ -80,6 +80,6 @@ void ram_block_notify_add(void *host, size_t size, size_t max_size);
 void ram_block_notify_remove(void *host, size_t size, size_t max_size);
 void ram_block_notify_resize(void *host, size_t old_size, size_t new_size);
 
-void ram_block_dump(Monitor *mon);
+GString *ram_block_format(void);
 
 #endif /* RAMLIST_H */
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index ebc093b3d9..2d09a01599 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -52,7 +52,6 @@
 #include "ui/console.h"
 #include "qemu/cutils.h"
 #include "qemu/error-report.h"
-#include "exec/ramlist.h"
 #include "hw/intc/intc.h"
 #include "migration/snapshot.h"
 #include "migration/misc.h"
@@ -2188,7 +2187,16 @@ void hmp_rocker_of_dpa_groups(Monitor *mon, const QDict *qdict)
 
 void hmp_info_ramblock(Monitor *mon, const QDict *qdict)
 {
-    ram_block_dump(mon);
+    Error *err = NULL;
+    g_autoptr(HumanReadableText) info = NULL;
+
+    info = qmp_x_query_ramblock(&err);
+    if (err) {
+        error_report_err(err);
+        return;
+    }
+
+    monitor_printf(mon, "%s", info->human_readable_text);
 }
 
 void hmp_info_vm_generation_id(Monitor *mon, const QDict *qdict)
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index 0a9ba7595c..a9766fa38d 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -38,6 +38,7 @@
 #include "qapi/qapi-commands-ui.h"
 #include "qapi/type-helpers.h"
 #include "qapi/qmp/qerror.h"
+#include "exec/ramlist.h"
 #include "hw/mem/memory-device.h"
 #include "hw/acpi/acpi_dev_interface.h"
 #include "hw/rdma/rdma.h"
@@ -414,3 +415,10 @@ HumanReadableText *qmp_x_query_rdma(Error **errp)
 
     return human_readable_text_from_str(buf);
 }
+
+HumanReadableText *qmp_x_query_ramblock(Error **errp)
+{
+    g_autoptr(GString) buf = ram_block_format();
+
+    return human_readable_text_from_str(buf);
+}
diff --git a/qapi/machine.json b/qapi/machine.json
index fbe4100443..340ab57f60 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1371,6 +1371,18 @@
 { 'command': 'x-query-profile',
   'returns': 'HumanReadableText' }
 
+##
+# @x-query-ramblock:
+#
+# Query system ramblock information
+#
+# Returns: system ramblock information
+#
+# Since: 6.2
+##
+{ 'command': 'x-query-ramblock',
+  'returns': 'HumanReadableText' }
+
 ##
 # @x-query-rdma:
 #
diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index 23e77cb771..bc4a8354d3 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -1298,23 +1298,26 @@ void qemu_mutex_unlock_ramlist(void)
     qemu_mutex_unlock(&ram_list.mutex);
 }
 
-void ram_block_dump(Monitor *mon)
+GString *ram_block_format(void)
 {
     RAMBlock *block;
     char *psize;
+    GString *buf = g_string_new("");
 
     RCU_READ_LOCK_GUARD();
-    monitor_printf(mon, "%24s %8s  %18s %18s %18s\n",
-                   "Block Name", "PSize", "Offset", "Used", "Total");
+    g_string_append_printf(buf, "%24s %8s  %18s %18s %18s\n",
+                           "Block Name", "PSize", "Offset", "Used", "Total");
     RAMBLOCK_FOREACH(block) {
         psize = size_to_str(block->page_size);
-        monitor_printf(mon, "%24s %8s  0x%016" PRIx64 " 0x%016" PRIx64
-                       " 0x%016" PRIx64 "\n", block->idstr, psize,
-                       (uint64_t)block->offset,
-                       (uint64_t)block->used_length,
-                       (uint64_t)block->max_length);
+        g_string_append_printf(buf, "%24s %8s  0x%016" PRIx64 " 0x%016" PRIx64
+                               " 0x%016" PRIx64 "\n", block->idstr, psize,
+                               (uint64_t)block->offset,
+                               (uint64_t)block->used_length,
+                               (uint64_t)block->max_length);
         g_free(psize);
     }
+
+    return buf;
 }
 
 #ifdef __linux__
-- 
2.31.1

[PATCH v3 13/19] qapi: introduce x-query-skeys QMP command

[Daniel P. Berrangé]

This is a counterpart to the HMP "info skeys" 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.
The existing HMP command is rewritten to call the QMP command.

Including 'common.json' into 'machine-target.json' created a little
problem because the static marshalling method for HumanReadableText
is generated unconditionally. It is only used, however, conditionally
on certain target architectures.

To deal with this we change the QAPI code generator to simply mark
all static marshalling functions with G_GNUC_UNSED to hide the
compiler warning.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 hw/s390x/s390-skeys.c    | 35 +++++++++++++++++++++++++++--------
 qapi/machine-target.json | 17 +++++++++++++++++
 scripts/qapi/commands.py |  1 +
 3 files changed, 45 insertions(+), 8 deletions(-)

diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
index 5024faf411..62eff8c88b 100644
--- a/hw/s390x/s390-skeys.c
+++ b/hw/s390x/s390-skeys.c
@@ -15,6 +15,8 @@
 #include "hw/s390x/storage-keys.h"
 #include "qapi/error.h"
 #include "qapi/qapi-commands-misc-target.h"
+#include "qapi/qapi-commands-machine-target.h"
+#include "qapi/type-helpers.h"
 #include "qapi/qmp/qdict.h"
 #include "qemu/error-report.h"
 #include "sysemu/memory_mapping.h"
@@ -73,34 +75,51 @@ static void write_keys(FILE *f, uint8_t *keys, uint64_t startgfn,
     }
 }
 
-void hmp_info_skeys(Monitor *mon, const QDict *qdict)
+HumanReadableText *qmp_x_query_skeys(int64_t addr, Error **errp)
 {
+    g_autoptr(GString) buf = g_string_new("");
     S390SKeysState *ss = s390_get_skeys_device();
     S390SKeysClass *skeyclass = S390_SKEYS_GET_CLASS(ss);
-    uint64_t addr = qdict_get_int(qdict, "addr");
     uint8_t key;
     int r;
 
     /* Quick check to see if guest is using storage keys*/
     if (!skeyclass->skeys_are_enabled(ss)) {
-        monitor_printf(mon, "Error: This guest is not using storage keys\n");
-        return;
+        error_setg(errp, "this guest is not using storage keys");
+        return NULL;
     }
 
     if (!address_space_access_valid(&address_space_memory,
                                     addr & TARGET_PAGE_MASK, TARGET_PAGE_SIZE,
                                     false, MEMTXATTRS_UNSPECIFIED)) {
-        monitor_printf(mon, "Error: The given address is not valid\n");
-        return;
+        error_setg(errp, "the given address is not valid");
+        return NULL;
     }
 
     r = skeyclass->get_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key);
     if (r < 0) {
-        monitor_printf(mon, "Error: %s\n", strerror(-r));
+        error_setg_errno(errp, r, "unable to query storage keys");
+        return NULL;
+    }
+
+    g_string_append_printf(buf, "  key: 0x%X\n", key);
+
+    return human_readable_text_from_str(buf);
+}
+
+void hmp_info_skeys(Monitor *mon, const QDict *qdict)
+{
+    Error *err = NULL;
+    g_autoptr(HumanReadableText) info = NULL;
+    uint64_t addr = qdict_get_int(qdict, "addr");
+
+    info = qmp_x_query_skeys(addr, &err);
+    if (err) {
+        error_report_err(err);
         return;
     }
 
-    monitor_printf(mon, "  key: 0x%X\n", key);
+    monitor_printf(mon, "%s", info->human_readable_text);
 }
 
 void hmp_dump_skeys(Monitor *mon, const QDict *qdict)
diff --git a/qapi/machine-target.json b/qapi/machine-target.json
index f5ec4bc172..7d4e73462f 100644
--- a/qapi/machine-target.json
+++ b/qapi/machine-target.json
@@ -4,6 +4,8 @@
 # This work is licensed under the terms of the GNU GPL, version 2 or later.
 # See the COPYING file in the top-level directory.
 
+{ 'include': 'common.json' }
+
 ##
 # @CpuModelInfo:
 #
@@ -341,3 +343,18 @@
                    'TARGET_I386',
                    'TARGET_S390X',
                    'TARGET_MIPS' ] } }
+
+
+##
+# @x-query-skeys:
+#
+# Query the value of a storage key
+#
+# Returns: storage key value
+#
+# Since: 6.2
+##
+{ 'command': 'x-query-skeys',
+  'data': { 'addr': 'int' },
+  'returns': 'HumanReadableText',
+  'if': 'TARGET_S390X' }
diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index 3654825968..01d8d1ea2c 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -91,6 +91,7 @@ def gen_call(name: str,
 def gen_marshal_output(ret_type: QAPISchemaType) -> str:
     return mcgen('''
 
+G_GNUC_UNUSED
 static void qmp_marshal_output_%(c_name)s(%(c_type)s ret_in,
                                 QObject **ret_out, Error **errp)
 {
-- 
2.31.1

Re: [PATCH v3 13/19] qapi: introduce x-query-skeys QMP command

[Thomas Huth]

On 30/09/2021 15.23, Daniel P. Berrangé wrote:
> This is a counterpart to the HMP "info skeys" 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.
> The existing HMP command is rewritten to call the QMP command.
> 
> Including 'common.json' into 'machine-target.json' created a little
> problem because the static marshalling method for HumanReadableText
> is generated unconditionally. It is only used, however, conditionally
> on certain target architectures.
> 
> To deal with this we change the QAPI code generator to simply mark
> all static marshalling functions with G_GNUC_UNSED to hide the
> compiler warning.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
...
> +void hmp_info_skeys(Monitor *mon, const QDict *qdict)
> +{
> +    Error *err = NULL;
> +    g_autoptr(HumanReadableText) info = NULL;
> +    uint64_t addr = qdict_get_int(qdict, "addr");
> +
> +    info = qmp_x_query_skeys(addr, &err);
> +    if (err) {
> +        error_report_err(err);

Shouldn't that rather be:

            monitor_printf(mon, "%s\n", error_get_pretty(err));

or something similar?

>           return;
>       }
>   
> -    monitor_printf(mon, "  key: 0x%X\n", key);
> +    monitor_printf(mon, "%s", info->human_readable_text);
>   }

Apart the question above, patch looks fine to me.

  Thomas

Re: [PATCH v3 13/19] qapi: introduce x-query-skeys QMP command

[Daniel P. Berrangé]

On Tue, Oct 12, 2021 at 09:12:23AM +0200, Thomas Huth wrote:
> On 30/09/2021 15.23, Daniel P. Berrangé wrote:
> > This is a counterpart to the HMP "info skeys" 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.
> > The existing HMP command is rewritten to call the QMP command.
> > 
> > Including 'common.json' into 'machine-target.json' created a little
> > problem because the static marshalling method for HumanReadableText
> > is generated unconditionally. It is only used, however, conditionally
> > on certain target architectures.
> > 
> > To deal with this we change the QAPI code generator to simply mark
> > all static marshalling functions with G_GNUC_UNSED to hide the
> > compiler warning.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> ...
> > +void hmp_info_skeys(Monitor *mon, const QDict *qdict)
> > +{
> > +    Error *err = NULL;
> > +    g_autoptr(HumanReadableText) info = NULL;
> > +    uint64_t addr = qdict_get_int(qdict, "addr");
> > +
> > +    info = qmp_x_query_skeys(addr, &err);
> > +    if (err) {
> > +        error_report_err(err);
> 
> Shouldn't that rather be:
> 
>            monitor_printf(mon, "%s\n", error_get_pretty(err));
> 
> or something similar?

error_report_err gets (eventually) hooked into monitor_printf() if
current monitor in the thread is non-NULL

> 
> >           return;
> >       }
> > -    monitor_printf(mon, "  key: 0x%X\n", key);
> > +    monitor_printf(mon, "%s", info->human_readable_text);
> >   }
> 
> Apart the question above, patch looks fine to me.
> 
>  Thomas
> 

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 :|

[PATCH v3 14/19] qapi: introduce x-query-cmma QMP command

[Daniel P. Berrangé]

This is a counterpart to the HMP "info cmma" 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.
The existing HMP command is rewritten to call the QMP command.

This command is unable to use the pre-existing HumanReadableText,
because if 'common.json' is included into 'machine-target.json'
the static marshalling method for HumanReadableText will be reported
as unused by the compiler on all architectures except s390x.

Possible options were

 1 Support 'if' conditionals on 'include' statements in QAPI
 2 Add further commands to 'machine-target.json' that use
   HumanReadableText, such that it has at least one usage
   on all architecture targets.
 3 Duplicate HumanReadableText as TargetHumanReadableText
   adding conditions

This patch takes option (3) in the belief that we will eventually
get to a point where option (2) happens, and TargetHumanReadableText
can be removed again.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 hw/s390x/s390-stattrib.c | 56 +++++++++++++++++++++++++++-------------
 qapi/machine-target.json | 14 ++++++++++
 2 files changed, 52 insertions(+), 18 deletions(-)

diff --git a/hw/s390x/s390-stattrib.c b/hw/s390x/s390-stattrib.c
index 9eda1c3b2a..d8f5867a5f 100644
--- a/hw/s390x/s390-stattrib.c
+++ b/hw/s390x/s390-stattrib.c
@@ -17,6 +17,8 @@
 #include "qemu/error-report.h"
 #include "exec/ram_addr.h"
 #include "qapi/error.h"
+#include "qapi/qapi-commands-machine-target.h"
+#include "qapi/type-helpers.h"
 #include "qapi/qmp/qdict.h"
 
 /* 512KiB cover 2GB of guest memory */
@@ -67,41 +69,59 @@ void hmp_migrationmode(Monitor *mon, const QDict *qdict)
     }
 }
 
-void hmp_info_cmma(Monitor *mon, const QDict *qdict)
+HumanReadableText *qmp_x_query_cmma(int64_t addr,
+                                    bool has_count,
+                                    int64_t count,
+                                    Error **errp)
 {
+    g_autoptr(GString) buf = g_string_new("");
     S390StAttribState *sas = s390_get_stattrib_device();
     S390StAttribClass *sac = S390_STATTRIB_GET_CLASS(sas);
-    uint64_t addr = qdict_get_int(qdict, "addr");
-    uint64_t buflen = qdict_get_try_int(qdict, "count", 8);
-    uint8_t *vals;
+    g_autofree uint8_t *vals = NULL;
     int cx, len;
 
-    vals = g_try_malloc(buflen);
+    vals = g_try_malloc(count);
     if (!vals) {
-        monitor_printf(mon, "Error: %s\n", strerror(errno));
-        return;
+        error_setg_errno(errp, errno, "cannot allocate CMMA attribute values");
+        return NULL;
     }
 
-    len = sac->peek_stattr(sas, addr / TARGET_PAGE_SIZE, buflen, vals);
+    len = sac->peek_stattr(sas, addr / TARGET_PAGE_SIZE, count, vals);
     if (len < 0) {
-        monitor_printf(mon, "Error: %s", strerror(-len));
-        goto out;
+        error_setg_errno(errp, -len, "cannot peek at CMMA attribute values");
+        return NULL;
     }
 
-    monitor_printf(mon, "  CMMA attributes, "
-                   "pages %" PRIu64 "+%d (0x%" PRIx64 "):\n",
-                   addr / TARGET_PAGE_SIZE, len, addr & ~TARGET_PAGE_MASK);
+    g_string_append_printf(buf, "  CMMA attributes, "
+                           "pages %" PRIu64 "+%d (0x%" PRIx64 "):\n",
+                           addr / TARGET_PAGE_SIZE, len,
+                           addr & ~TARGET_PAGE_MASK);
     for (cx = 0; cx < len; cx++) {
         if (cx % 8 == 7) {
-            monitor_printf(mon, "%02x\n", vals[cx]);
+            g_string_append_printf(buf, "%02x\n", vals[cx]);
         } else {
-            monitor_printf(mon, "%02x", vals[cx]);
+            g_string_append_printf(buf, "%02x", vals[cx]);
         }
     }
-    monitor_printf(mon, "\n");
+    g_string_append_printf(buf, "\n");
+
+    return human_readable_text_from_str(buf);
+}
+
+void hmp_info_cmma(Monitor *mon, const QDict *qdict)
+{
+    Error *err = NULL;
+    g_autoptr(HumanReadableText) info = NULL;
+    uint64_t addr = qdict_get_int(qdict, "addr");
+    uint64_t count = qdict_get_try_int(qdict, "count", 8);
+
+    info = qmp_x_query_cmma(addr, true, count, &err);
+    if (err) {
+        error_report_err(err);
+        return;
+    }
 
-out:
-    g_free(vals);
+    monitor_printf(mon, "%s", info->human_readable_text);
 }
 
 /* Migration support: */
diff --git a/qapi/machine-target.json b/qapi/machine-target.json
index 7d4e73462f..19e44987af 100644
--- a/qapi/machine-target.json
+++ b/qapi/machine-target.json
@@ -345,6 +345,20 @@
                    'TARGET_MIPS' ] } }
 
 
+##
+# @x-query-cmma:
+#
+# Query the values of the CMMA storage attributes for a range of pages
+#
+# Returns: CMMA storage attribute values
+#
+# Since: 6.2
+##
+{ 'command': 'x-query-cmma',
+  'data': { 'addr': 'int', '*count': 'int' },
+  'returns': 'HumanReadableText',
+  'if': 'TARGET_S390X' }
+
 ##
 # @x-query-skeys:
 #
-- 
2.31.1

[PATCH v3 15/19] hmp: synchronize cpu state for lapic info

[Daniel P. Berrangé]

From: Dongli Zhang <dongli.zhang@oracle.com>

While the default "info lapic" always synchronizes cpu state ...

mon_get_cpu()
-> mon_get_cpu_sync(mon, true)
   -> cpu_synchronize_state(cpu)
      -> ioctl KVM_GET_LAPIC (taking KVM as example)

... the cpu state is not synchronized when the apic-id is available as
argument.

The cpu state should be synchronized when apic-id is available. Otherwise
the "info lapic <apic-id>" always returns stale data.

Cc: Joe Jin <joe.jin@oracle.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
---
 target/i386/monitor.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/target/i386/monitor.c b/target/i386/monitor.c
index 19468c4e85..2a41eac7fd 100644
--- a/target/i386/monitor.c
+++ b/target/i386/monitor.c
@@ -28,6 +28,7 @@
 #include "monitor/hmp-target.h"
 #include "monitor/hmp.h"
 #include "qapi/qmp/qdict.h"
+#include "sysemu/hw_accel.h"
 #include "sysemu/kvm.h"
 #include "sysemu/sev.h"
 #include "qapi/error.h"
@@ -656,7 +657,11 @@ void hmp_info_local_apic(Monitor *mon, const QDict *qdict)
 
     if (qdict_haskey(qdict, "apic-id")) {
         int id = qdict_get_try_int(qdict, "apic-id", 0);
+
         cs = cpu_by_arch_id(id);
+        if (cs) {
+            cpu_synchronize_state(cs);
+        }
     } else {
         cs = mon_get_cpu(mon);
     }
-- 
2.31.1

[PATCH v3 16/19] qapi: introduce x-query-lapic QMP command

[Daniel P. Berrangé]

This is a counterpart to the HMP "info lapic" 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.
The existing HMP command is rewritten to call the QMP command.

This command is unable to use the pre-existing HumanReadableText,
because if 'common.json' is included into 'machine-target.json'
the static marshalling method for HumanReadableText will be reported
as unused by the compiler on all architectures except s390x.

Possible options were

 1 Support 'if' conditionals on 'include' statements in QAPI
 2 Add further commands to 'machine-target.json' that use
   HumanReadableText, such that it has at least one usage
   on all architecture targets.
 3 Duplicate HumanReadableText as TargetHumanReadableText
   adding conditions

This patch takes option (3) in the belief that we will eventually
get to a point where option (2) happens, and TargetHumanReadableText
can be removed again.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 hw/core/cpu-common.c     |   7 ++
 include/hw/core/cpu.h    |  10 +++
 qapi/machine-target.json |  16 ++++
 target/i386/cpu-dump.c   | 161 ++++++++++++++++++++-------------------
 target/i386/cpu.h        |   4 +-
 target/i386/monitor.c    |  49 +++++++++---
 6 files changed, 157 insertions(+), 90 deletions(-)

diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
index e2f5a64604..ac95cc1935 100644
--- a/hw/core/cpu-common.c
+++ b/hw/core/cpu-common.c
@@ -49,6 +49,13 @@ CPUState *cpu_by_arch_id(int64_t id)
     return NULL;
 }
 
+int64_t cpu_get_arch_id(CPUState *cpu)
+{
+    CPUClass *cc = CPU_GET_CLASS(cpu);
+
+    return cc->get_arch_id(cpu);
+}
+
 bool cpu_exists(int64_t id)
 {
     return !!cpu_by_arch_id(id);
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index bc864564ce..a8740d1d63 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -769,6 +769,16 @@ bool cpu_exists(int64_t id);
  */
 CPUState *cpu_by_arch_id(int64_t id);
 
+/**
+ * cpu_get_arch_id:
+ * @cpu: the CPU to query
+ *
+ * Get the guest exposed CPU ID for @cpu
+ *
+ * Returns: The guest exposed CPU ID
+ */
+int64_t cpu_get_arch_id(CPUState *cpu);
+
 /**
  * cpu_interrupt:
  * @cpu: The CPU to set an interrupt on.
diff --git a/qapi/machine-target.json b/qapi/machine-target.json
index 19e44987af..43fdfb2c89 100644
--- a/qapi/machine-target.json
+++ b/qapi/machine-target.json
@@ -359,6 +359,22 @@
   'returns': 'HumanReadableText',
   'if': 'TARGET_S390X' }
 
+##
+# @x-query-lapic:
+#
+# @apic-id: the local APIC ID to report
+#
+# Query local APIC state.
+#
+# Returns: local APIC state
+#
+# Since: 6.2
+##
+{ 'command': 'x-query-lapic',
+  'data': { 'apic-id': 'int' },
+  'returns': 'HumanReadableText',
+  'if': 'TARGET_I386' }
+
 ##
 # @x-query-skeys:
 #
diff --git a/target/i386/cpu-dump.c b/target/i386/cpu-dump.c
index 02b635a52c..7c5536db25 100644
--- a/target/i386/cpu-dump.c
+++ b/target/i386/cpu-dump.c
@@ -20,6 +20,7 @@
 #include "qemu/osdep.h"
 #include "cpu.h"
 #include "qemu/qemu-print.h"
+#include "qapi/error.h"
 #ifndef CONFIG_USER_ONLY
 #include "hw/i386/apic_internal.h"
 #endif
@@ -172,24 +173,26 @@ static inline const char *dm2str(uint32_t dm)
     return str[dm];
 }
 
-static void dump_apic_lvt(const char *name, uint32_t lvt, bool is_timer)
+static void format_apic_lvt(const char *name, uint32_t lvt, bool is_timer,
+                            GString *buf)
 {
     uint32_t dm = (lvt & APIC_LVT_DELIV_MOD) >> APIC_LVT_DELIV_MOD_SHIFT;
-    qemu_printf("%s\t 0x%08x %s %-5s %-6s %-7s %-12s %-6s",
-                name, lvt,
-                lvt & APIC_LVT_INT_POLARITY ? "active-lo" : "active-hi",
-                lvt & APIC_LVT_LEVEL_TRIGGER ? "level" : "edge",
-                lvt & APIC_LVT_MASKED ? "masked" : "",
-                lvt & APIC_LVT_DELIV_STS ? "pending" : "",
-                !is_timer ?
-                    "" : lvt & APIC_LVT_TIMER_PERIODIC ?
-                            "periodic" : lvt & APIC_LVT_TIMER_TSCDEADLINE ?
-                                            "tsc-deadline" : "one-shot",
+    g_string_append_printf(buf, "%s\t 0x%08x %s %-5s %-6s %-7s %-12s %-6s",
+                           name, lvt,
+                           lvt & APIC_LVT_INT_POLARITY ?
+                           "active-lo" : "active-hi",
+                           lvt & APIC_LVT_LEVEL_TRIGGER ? "level" : "edge",
+                           lvt & APIC_LVT_MASKED ? "masked" : "",
+                           lvt & APIC_LVT_DELIV_STS ? "pending" : "",
+                           !is_timer ?
+                           "" : lvt & APIC_LVT_TIMER_PERIODIC ?
+                           "periodic" : lvt & APIC_LVT_TIMER_TSCDEADLINE ?
+                           "tsc-deadline" : "one-shot",
                 dm2str(dm));
     if (dm != APIC_DM_NMI) {
-        qemu_printf(" (vec %u)\n", lvt & APIC_VECTOR_MASK);
+        g_string_append_printf(buf, " (vec %u)\n", lvt & APIC_VECTOR_MASK);
     } else {
-        qemu_printf("\n");
+        g_string_append_printf(buf, "\n");
     }
 }
 
@@ -221,7 +224,7 @@ static inline void mask2str(char *str, uint32_t val, uint8_t size)
 
 #define MAX_LOGICAL_APIC_ID_MASK_SIZE 16
 
-static void dump_apic_icr(APICCommonState *s, CPUX86State *env)
+static void format_apic_icr(APICCommonState *s, CPUX86State *env, GString *buf)
 {
     uint32_t icr = s->icr[0], icr2 = s->icr[1];
     uint8_t dest_shorthand = \
@@ -231,16 +234,16 @@ static void dump_apic_icr(APICCommonState *s, CPUX86State *env)
     uint32_t dest_field;
     bool x2apic;
 
-    qemu_printf("ICR\t 0x%08x %s %s %s %s\n",
-                icr,
-                logical_mod ? "logical" : "physical",
-                icr & APIC_ICR_TRIGGER_MOD ? "level" : "edge",
-                icr & APIC_ICR_LEVEL ? "assert" : "de-assert",
-                shorthand2str(dest_shorthand));
+    g_string_append_printf(buf, "ICR\t 0x%08x %s %s %s %s\n",
+                           icr,
+                           logical_mod ? "logical" : "physical",
+                           icr & APIC_ICR_TRIGGER_MOD ? "level" : "edge",
+                           icr & APIC_ICR_LEVEL ? "assert" : "de-assert",
+                           shorthand2str(dest_shorthand));
 
-    qemu_printf("ICR2\t 0x%08x", icr2);
+    g_string_append_printf(buf, "ICR2\t 0x%08x", icr2);
     if (dest_shorthand != 0) {
-        qemu_printf("\n");
+        g_string_append_printf(buf, "\n");
         return;
     }
     x2apic = env->features[FEAT_1_ECX] & CPUID_EXT_X2APIC;
@@ -248,96 +251,100 @@ static void dump_apic_icr(APICCommonState *s, CPUX86State *env)
 
     if (!logical_mod) {
         if (x2apic) {
-            qemu_printf(" cpu %u (X2APIC ID)\n", dest_field);
+            g_string_append_printf(buf, " cpu %u (X2APIC ID)\n", dest_field);
         } else {
-            qemu_printf(" cpu %u (APIC ID)\n",
-                        dest_field & APIC_LOGDEST_XAPIC_ID);
+            g_string_append_printf(buf, " cpu %u (APIC ID)\n",
+                                   dest_field & APIC_LOGDEST_XAPIC_ID);
         }
         return;
     }
 
     if (s->dest_mode == 0xf) { /* flat mode */
         mask2str(apic_id_str, icr2 >> APIC_ICR_DEST_SHIFT, 8);
-        qemu_printf(" mask %s (APIC ID)\n", apic_id_str);
+        g_string_append_printf(buf, " mask %s (APIC ID)\n", apic_id_str);
     } else if (s->dest_mode == 0) { /* cluster mode */
         if (x2apic) {
             mask2str(apic_id_str, dest_field & APIC_LOGDEST_X2APIC_ID, 16);
-            qemu_printf(" cluster %u mask %s (X2APIC ID)\n",
-                        dest_field >> APIC_LOGDEST_X2APIC_SHIFT, apic_id_str);
+            g_string_append_printf(buf, " cluster %u mask %s (X2APIC ID)\n",
+                                   dest_field >> APIC_LOGDEST_X2APIC_SHIFT,
+                                   apic_id_str);
         } else {
             mask2str(apic_id_str, dest_field & APIC_LOGDEST_XAPIC_ID, 4);
-            qemu_printf(" cluster %u mask %s (APIC ID)\n",
-                        dest_field >> APIC_LOGDEST_XAPIC_SHIFT, apic_id_str);
+            g_string_append_printf(buf, " cluster %u mask %s (APIC ID)\n",
+                                   dest_field >> APIC_LOGDEST_XAPIC_SHIFT,
+                                   apic_id_str);
         }
     }
 }
 
-static void dump_apic_interrupt(const char *name, uint32_t *ireg_tab,
-                                uint32_t *tmr_tab)
+static void format_apic_interrupt(const char *name, uint32_t *ireg_tab,
+                                  uint32_t *tmr_tab, GString *buf)
 {
     int i, empty = true;
 
-    qemu_printf("%s\t ", name);
+    g_string_append_printf(buf, "%s\t ", name);
     for (i = 0; i < 256; i++) {
         if (apic_get_bit(ireg_tab, i)) {
-            qemu_printf("%u%s ", i,
-                        apic_get_bit(tmr_tab, i) ? "(level)" : "");
+            g_string_append_printf(buf, "%u%s ", i,
+                                   apic_get_bit(tmr_tab, i) ? "(level)" : "");
             empty = false;
         }
     }
-    qemu_printf("%s\n", empty ? "(none)" : "");
+    g_string_append_printf(buf, "%s\n", empty ? "(none)" : "");
 }
 
-void x86_cpu_dump_local_apic_state(CPUState *cs, int flags)
+GString *x86_cpu_format_local_apic_state(CPUState *cs, int flags, Error **errp)
 {
+    g_autoptr(GString) buf = g_string_new("");
     X86CPU *cpu = X86_CPU(cs);
     APICCommonState *s = APIC_COMMON(cpu->apic_state);
     if (!s) {
-        qemu_printf("local apic state not available\n");
-        return;
+        error_setg(errp, "local apic state not available");
+        return NULL;
     }
     uint32_t *lvt = s->lvt;
 
-    qemu_printf("dumping local APIC state for CPU %-2u\n\n",
-                CPU(cpu)->cpu_index);
-    dump_apic_lvt("LVT0", lvt[APIC_LVT_LINT0], false);
-    dump_apic_lvt("LVT1", lvt[APIC_LVT_LINT1], false);
-    dump_apic_lvt("LVTPC", lvt[APIC_LVT_PERFORM], false);
-    dump_apic_lvt("LVTERR", lvt[APIC_LVT_ERROR], false);
-    dump_apic_lvt("LVTTHMR", lvt[APIC_LVT_THERMAL], false);
-    dump_apic_lvt("LVTT", lvt[APIC_LVT_TIMER], true);
-
-    qemu_printf("Timer\t DCR=0x%x (divide by %u) initial_count = %u"
-                " current_count = %u\n",
-                s->divide_conf & APIC_DCR_MASK,
-                divider_conf(s->divide_conf),
-                s->initial_count, apic_get_current_count(s));
-
-    qemu_printf("SPIV\t 0x%08x APIC %s, focus=%s, spurious vec %u\n",
-                s->spurious_vec,
-                s->spurious_vec & APIC_SPURIO_ENABLED ? "enabled" : "disabled",
-                s->spurious_vec & APIC_SPURIO_FOCUS ? "on" : "off",
-                s->spurious_vec & APIC_VECTOR_MASK);
-
-    dump_apic_icr(s, &cpu->env);
-
-    qemu_printf("ESR\t 0x%08x\n", s->esr);
-
-    dump_apic_interrupt("ISR", s->isr, s->tmr);
-    dump_apic_interrupt("IRR", s->irr, s->tmr);
-
-    qemu_printf("\nAPR 0x%02x TPR 0x%02x DFR 0x%02x LDR 0x%02x",
-                s->arb_id, s->tpr, s->dest_mode, s->log_dest);
+    g_string_append_printf(buf, "dumping local APIC state for CPU %-2u\n\n",
+                           CPU(cpu)->cpu_index);
+    format_apic_lvt("LVT0", lvt[APIC_LVT_LINT0], false, buf);
+    format_apic_lvt("LVT1", lvt[APIC_LVT_LINT1], false, buf);
+    format_apic_lvt("LVTPC", lvt[APIC_LVT_PERFORM], false, buf);
+    format_apic_lvt("LVTERR", lvt[APIC_LVT_ERROR], false, buf);
+    format_apic_lvt("LVTTHMR", lvt[APIC_LVT_THERMAL], false, buf);
+    format_apic_lvt("LVTT", lvt[APIC_LVT_TIMER], true, buf);
+
+    g_string_append_printf(buf,
+                           "Timer\t DCR=0x%x (divide by %u) initial_count = %u"
+                           " current_count = %u\n",
+                           s->divide_conf & APIC_DCR_MASK,
+                           divider_conf(s->divide_conf),
+                           s->initial_count, apic_get_current_count(s));
+
+    g_string_append_printf(buf,
+                           "SPIV\t 0x%08x APIC %s, focus=%s, spurious vec %u\n",
+                           s->spurious_vec,
+                           s->spurious_vec & APIC_SPURIO_ENABLED ?
+                           "enabled" : "disabled",
+                           s->spurious_vec & APIC_SPURIO_FOCUS ? "on" : "off",
+                           s->spurious_vec & APIC_VECTOR_MASK);
+
+    format_apic_icr(s, &cpu->env, buf);
+
+    g_string_append_printf(buf, "ESR\t 0x%08x\n", s->esr);
+
+    format_apic_interrupt("ISR", s->isr, s->tmr, buf);
+    format_apic_interrupt("IRR", s->irr, s->tmr, buf);
+
+    g_string_append_printf(buf, "\nAPR 0x%02x TPR 0x%02x DFR 0x%02x LDR 0x%02x",
+                           s->arb_id, s->tpr, s->dest_mode, s->log_dest);
     if (s->dest_mode == 0) {
-        qemu_printf("(cluster %u: id %u)",
-                    s->log_dest >> APIC_LOGDEST_XAPIC_SHIFT,
-                    s->log_dest & APIC_LOGDEST_XAPIC_ID);
+        g_string_append_printf(buf, "(cluster %u: id %u)",
+                               s->log_dest >> APIC_LOGDEST_XAPIC_SHIFT,
+                               s->log_dest & APIC_LOGDEST_XAPIC_ID);
     }
-    qemu_printf(" PPR 0x%02x\n", apic_get_ppr(s));
-}
-#else
-void x86_cpu_dump_local_apic_state(CPUState *cs, int flags)
-{
+    g_string_append_printf(buf, " PPR 0x%02x\n", apic_get_ppr(s));
+
+    return g_steal_pointer(&buf);
 }
 #endif /* !CONFIG_USER_ONLY */
 
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index c2954c71ea..3620752f74 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -2197,8 +2197,10 @@ void x86_cpu_set_default_version(X86CPUVersion version);
 #define APIC_DEFAULT_ADDRESS 0xfee00000
 #define APIC_SPACE_SIZE      0x100000
 
+#ifndef CONFIG_USER_ONLY
 /* cpu-dump.c */
-void x86_cpu_dump_local_apic_state(CPUState *cs, int flags);
+GString *x86_cpu_format_local_apic_state(CPUState *cs, int flags, Error **errp);
+#endif /* !CONFIG_USER_ONLY */
 
 /* cpu.c */
 bool cpu_is_bsp(X86CPU *cpu);
diff --git a/target/i386/monitor.c b/target/i386/monitor.c
index 2a41eac7fd..b48ce3eaee 100644
--- a/target/i386/monitor.c
+++ b/target/i386/monitor.c
@@ -34,7 +34,9 @@
 #include "qapi/error.h"
 #include "sev_i386.h"
 #include "qapi/qapi-commands-misc-target.h"
+#include "qapi/qapi-commands-machine-target.h"
 #include "qapi/qapi-commands-misc.h"
+#include "qapi/type-helpers.h"
 #include "hw/i386/pc.h"
 
 /* Perform linear address sign extension */
@@ -651,27 +653,50 @@ const MonitorDef *target_monitor_defs(void)
     return monitor_defs;
 }
 
+HumanReadableText *qmp_x_query_lapic(int64_t apicid,
+                                     Error **errp)
+{
+    g_autoptr(GString) buf = NULL;
+    CPUState *cs = cpu_by_arch_id(apicid);
+
+    if (!cs) {
+        error_setg(errp, "No CPU with APIC ID %" PRId64 " available", apicid);
+        return NULL;
+    }
+    cpu_synchronize_state(cs);
+
+    buf = x86_cpu_format_local_apic_state(cs, CPU_DUMP_FPU, errp);
+    if (!buf) {
+        return NULL;
+    }
+
+    return human_readable_text_from_str(buf);
+}
+
 void hmp_info_local_apic(Monitor *mon, const QDict *qdict)
 {
-    CPUState *cs;
+    Error *err = NULL;
+    g_autoptr(HumanReadableText) info = NULL;
+    int64_t apicid;
 
     if (qdict_haskey(qdict, "apic-id")) {
-        int id = qdict_get_try_int(qdict, "apic-id", 0);
-
-        cs = cpu_by_arch_id(id);
-        if (cs) {
-            cpu_synchronize_state(cs);
-        }
+        apicid = qdict_get_try_int(qdict, "apic-id", 0);
     } else {
-        cs = mon_get_cpu(mon);
+        CPUState *cs = mon_get_cpu(mon);
+        if (!cs) {
+            monitor_printf(mon, "No CPU available\n");
+            return;
+        }
+        apicid = cpu_get_arch_id(cs);
     }
 
-
-    if (!cs) {
-        monitor_printf(mon, "No CPU available\n");
+    info = qmp_x_query_lapic(apicid, &err);
+    if (err) {
+        error_report_err(err);
         return;
     }
-    x86_cpu_dump_local_apic_state(cs, CPU_DUMP_FPU);
+
+    monitor_printf(mon, "%s", info->human_readable_text);
 }
 
 SevInfo *qmp_query_sev(Error **errp)
-- 
2.31.1

[PATCH v3 17/19] qapi: introduce x-query-irq QMP command

[Daniel P. Berrangé]

This is a counterpart to the HMP "info irq" 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.
The existing HMP command is rewritten to call the QMP command.

This command is unable to use the pre-existing HumanReadableText,
because if 'common.json' is included into 'machine-target.json'
the static marshalling method for HumanReadableText will be reported
as unused by the compiler on all architectures except s390x.

Possible options were

 1 Support 'if' conditionals on 'include' statements in QAPI
 2 Add further commands to 'machine-target.json' that use
   HumanReadableText, such that it has at least one usage
   on all architecture targets.
 3 Duplicate HumanReadableText as TargetHumanReadableText
   adding conditions

This patch takes option (3) in the belief that we will eventually
get to a point where option (2) happens, and TargetHumanReadableText
can be removed again.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 monitor/hmp-cmds.c | 40 ++++++++--------------------------------
 monitor/qmp-cmds.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 qapi/machine.json  | 12 ++++++++++++
 3 files changed, 64 insertions(+), 32 deletions(-)

diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 2d09a01599..2df1136e21 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -784,42 +784,18 @@ static void hmp_info_pci_device(Monitor *mon, const PciDeviceInfo *dev)
     }
 }
 
-static int hmp_info_irq_foreach(Object *obj, void *opaque)
+void hmp_info_irq(Monitor *mon, const QDict *qdict)
 {
-    InterruptStatsProvider *intc;
-    InterruptStatsProviderClass *k;
-    Monitor *mon = opaque;
+    Error *err = NULL;
+    g_autoptr(HumanReadableText) info = NULL;
 
-    if (object_dynamic_cast(obj, TYPE_INTERRUPT_STATS_PROVIDER)) {
-        intc = INTERRUPT_STATS_PROVIDER(obj);
-        k = INTERRUPT_STATS_PROVIDER_GET_CLASS(obj);
-        uint64_t *irq_counts;
-        unsigned int nb_irqs, i;
-        if (k->get_statistics &&
-            k->get_statistics(intc, &irq_counts, &nb_irqs)) {
-            if (nb_irqs > 0) {
-                monitor_printf(mon, "IRQ statistics for %s:\n",
-                               object_get_typename(obj));
-                for (i = 0; i < nb_irqs; i++) {
-                    if (irq_counts[i] > 0) {
-                        monitor_printf(mon, "%2d: %" PRId64 "\n", i,
-                                       irq_counts[i]);
-                    }
-                }
-            }
-        } else {
-            monitor_printf(mon, "IRQ statistics not available for %s.\n",
-                           object_get_typename(obj));
-        }
+    info = qmp_x_query_irq(&err);
+    if (err) {
+        error_report_err(err);
+        return;
     }
 
-    return 0;
-}
-
-void hmp_info_irq(Monitor *mon, const QDict *qdict)
-{
-    object_child_foreach_recursive(object_get_root(),
-                                   hmp_info_irq_foreach, mon);
+    monitor_printf(mon, "%s", info->human_readable_text);
 }
 
 static int hmp_info_pic_foreach(Object *obj, void *opaque)
diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index a9766fa38d..343353e27a 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -41,6 +41,7 @@
 #include "exec/ramlist.h"
 #include "hw/mem/memory-device.h"
 #include "hw/acpi/acpi_dev_interface.h"
+#include "hw/intc/intc.h"
 #include "hw/rdma/rdma.h"
 
 NameInfo *qmp_query_name(Error **errp)
@@ -422,3 +423,46 @@ HumanReadableText *qmp_x_query_ramblock(Error **errp)
 
     return human_readable_text_from_str(buf);
 }
+
+static int qmp_x_query_irq_foreach(Object *obj, void *opaque)
+{
+    InterruptStatsProvider *intc;
+    InterruptStatsProviderClass *k;
+    GString *buf = opaque;
+
+    if (object_dynamic_cast(obj, TYPE_INTERRUPT_STATS_PROVIDER)) {
+        intc = INTERRUPT_STATS_PROVIDER(obj);
+        k = INTERRUPT_STATS_PROVIDER_GET_CLASS(obj);
+        uint64_t *irq_counts;
+        unsigned int nb_irqs, i;
+        if (k->get_statistics &&
+            k->get_statistics(intc, &irq_counts, &nb_irqs)) {
+            if (nb_irqs > 0) {
+                g_string_append_printf(buf, "IRQ statistics for %s:\n",
+                                       object_get_typename(obj));
+                for (i = 0; i < nb_irqs; i++) {
+                    if (irq_counts[i] > 0) {
+                        g_string_append_printf(buf, "%2d: %" PRId64 "\n", i,
+                                               irq_counts[i]);
+                    }
+                }
+            }
+        } else {
+            g_string_append_printf(buf,
+                                   "IRQ statistics not available for %s.\n",
+                                   object_get_typename(obj));
+        }
+    }
+
+    return 0;
+}
+
+HumanReadableText *qmp_x_query_irq(Error **errp)
+{
+    g_autoptr(GString) buf = g_string_new("");
+
+    object_child_foreach_recursive(object_get_root(),
+                                   qmp_x_query_irq_foreach, buf);
+
+    return human_readable_text_from_str(buf);
+}
diff --git a/qapi/machine.json b/qapi/machine.json
index 340ab57f60..31eafe2fc4 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1347,6 +1347,18 @@
      '*threads': 'int',
      '*maxcpus': 'int' } }
 
+##
+# @x-query-irq:
+#
+# Query interrupt statistics
+#
+# Returns: interrupt statistics
+#
+# Since: 6.2
+##
+{ 'command': 'x-query-irq',
+  'returns': 'HumanReadableText' }
+
 ##
 # @x-query-numa:
 #
-- 
2.31.1

[PATCH v3 18/19] qapi: introduce x-query-jit QMP command

[Daniel P. Berrangé]

This is a counterpart to the HMP "info jit" command. It is being
added with an "x-" prefix because this QMP command is intended as an
ad hoc debugging tool and will thus not be modelled in QAPI as fully
structured data, nor will it have long term guaranteed stability.
The existing HMP command is rewritten to call the QMP command.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 accel/tcg/cpu-exec.c       | 37 ++++++++++++----
 accel/tcg/hmp.c            | 13 ++++--
 accel/tcg/translate-all.c  | 80 ++++++++++++++++++----------------
 include/exec/cpu-all.h     |  4 +-
 include/tcg/tcg.h          |  2 +-
 qapi/machine.json          | 13 ++++++
 tcg/tcg.c                  | 88 ++++++++++++++++++++------------------
 tests/qtest/qmp-cmd-test.c |  2 +
 8 files changed, 146 insertions(+), 93 deletions(-)

diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index 5fd1ed3422..84e10e0f49 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -20,6 +20,9 @@
 #include "qemu/osdep.h"
 #include "qemu-common.h"
 #include "qemu/qemu-print.h"
+#include "qapi/error.h"
+#include "qapi/qapi-commands-machine.h"
+#include "qapi/type-helpers.h"
 #include "hw/core/tcg-cpu-ops.h"
 #include "trace.h"
 #include "disas/disas.h"
@@ -38,6 +41,7 @@
 #include "exec/cpu-all.h"
 #include "sysemu/cpu-timers.h"
 #include "sysemu/replay.h"
+#include "sysemu/tcg.h"
 #include "exec/helper-proto.h"
 #include "tb-hash.h"
 #include "tb-context.h"
@@ -1017,23 +1021,38 @@ void tcg_exec_unrealizefn(CPUState *cpu)
 
 #ifndef CONFIG_USER_ONLY
 
-void dump_drift_info(void)
+void dump_drift_info(GString *buf)
 {
     if (!icount_enabled()) {
         return;
     }
 
-    qemu_printf("Host - Guest clock  %"PRIi64" ms\n",
-                (cpu_get_clock() - icount_get()) / SCALE_MS);
+    g_string_append_printf(buf, "Host - Guest clock  %"PRIi64" ms\n",
+                           (cpu_get_clock() - icount_get()) / SCALE_MS);
     if (icount_align_option) {
-        qemu_printf("Max guest delay     %"PRIi64" ms\n",
-                    -max_delay / SCALE_MS);
-        qemu_printf("Max guest advance   %"PRIi64" ms\n",
-                    max_advance / SCALE_MS);
+        g_string_append_printf(buf, "Max guest delay     %"PRIi64" ms\n",
+                               -max_delay / SCALE_MS);
+        g_string_append_printf(buf, "Max guest advance   %"PRIi64" ms\n",
+                               max_advance / SCALE_MS);
     } else {
-        qemu_printf("Max guest delay     NA\n");
-        qemu_printf("Max guest advance   NA\n");
+        g_string_append_printf(buf, "Max guest delay     NA\n");
+        g_string_append_printf(buf, "Max guest advance   NA\n");
     }
 }
 
+HumanReadableText *qmp_x_query_jit(Error **errp)
+{
+    g_autoptr(GString) buf = g_string_new("");
+
+    if (!tcg_enabled()) {
+        error_setg(errp, "JIT information is only available with accel=tcg");
+        return NULL;
+    }
+
+    dump_exec_info(buf);
+    dump_drift_info(buf);
+
+    return human_readable_text_from_str(buf);
+}
+
 #endif /* !CONFIG_USER_ONLY */
diff --git a/accel/tcg/hmp.c b/accel/tcg/hmp.c
index a6e72fdb3e..9d7bcd9185 100644
--- a/accel/tcg/hmp.c
+++ b/accel/tcg/hmp.c
@@ -1,18 +1,23 @@
 #include "qemu/osdep.h"
 #include "qemu/error-report.h"
+#include "qapi/error.h"
+#include "qapi/qapi-commands-machine.h"
 #include "exec/exec-all.h"
 #include "monitor/monitor.h"
 #include "sysemu/tcg.h"
 
 static void hmp_info_jit(Monitor *mon, const QDict *qdict)
 {
-    if (!tcg_enabled()) {
-        error_report("JIT information is only available with accel=tcg");
+    Error *err = NULL;
+    g_autoptr(HumanReadableText) info = NULL;
+
+    info = qmp_x_query_jit(&err);
+    if (err) {
+        error_report_err(err);
         return;
     }
 
-    dump_exec_info();
-    dump_drift_info();
+    monitor_printf(mon, "%s", info->human_readable_text);
 }
 
 static void hmp_info_opcount(Monitor *mon, const QDict *qdict)
diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index fb9ebfad9e..8f17a91858 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -1991,7 +1991,7 @@ void cpu_io_recompile(CPUState *cpu, uintptr_t retaddr)
     cpu_loop_exit_noexc(cpu);
 }
 
-static void print_qht_statistics(struct qht_stats hst)
+static void print_qht_statistics(struct qht_stats hst, GString *buf)
 {
     uint32_t hgram_opts;
     size_t hgram_bins;
@@ -2000,9 +2000,11 @@ static void print_qht_statistics(struct qht_stats hst)
     if (!hst.head_buckets) {
         return;
     }
-    qemu_printf("TB hash buckets     %zu/%zu (%0.2f%% head buckets used)\n",
-                hst.used_head_buckets, hst.head_buckets,
-                (double)hst.used_head_buckets / hst.head_buckets * 100);
+    g_string_append_printf(buf, "TB hash buckets     %zu/%zu "
+                           "(%0.2f%% head buckets used)\n",
+                           hst.used_head_buckets, hst.head_buckets,
+                           (double)hst.used_head_buckets /
+                           hst.head_buckets * 100);
 
     hgram_opts =  QDIST_PR_BORDER | QDIST_PR_LABELS;
     hgram_opts |= QDIST_PR_100X   | QDIST_PR_PERCENT;
@@ -2010,8 +2012,9 @@ static void print_qht_statistics(struct qht_stats hst)
         hgram_opts |= QDIST_PR_NODECIMAL;
     }
     hgram = qdist_pr(&hst.occupancy, 10, hgram_opts);
-    qemu_printf("TB hash occupancy   %0.2f%% avg chain occ. Histogram: %s\n",
-                qdist_avg(&hst.occupancy) * 100, hgram);
+    g_string_append_printf(buf, "TB hash occupancy   %0.2f%% avg chain occ. "
+                           "Histogram: %s\n",
+                           qdist_avg(&hst.occupancy) * 100, hgram);
     g_free(hgram);
 
     hgram_opts = QDIST_PR_BORDER | QDIST_PR_LABELS;
@@ -2023,8 +2026,9 @@ static void print_qht_statistics(struct qht_stats hst)
         hgram_opts |= QDIST_PR_NODECIMAL | QDIST_PR_NOBINRANGE;
     }
     hgram = qdist_pr(&hst.chain, hgram_bins, hgram_opts);
-    qemu_printf("TB hash avg chain   %0.3f buckets. Histogram: %s\n",
-                qdist_avg(&hst.chain), hgram);
+    g_string_append_printf(buf, "TB hash avg chain   %0.3f buckets. "
+                           "Histogram: %s\n",
+                           qdist_avg(&hst.chain), hgram);
     g_free(hgram);
 }
 
@@ -2061,7 +2065,7 @@ static gboolean tb_tree_stats_iter(gpointer key, gpointer value, gpointer data)
     return false;
 }
 
-void dump_exec_info(void)
+void dump_exec_info(GString *buf)
 {
     struct tb_tree_stats tst = {};
     struct qht_stats hst;
@@ -2070,44 +2074,48 @@ void dump_exec_info(void)
     tcg_tb_foreach(tb_tree_stats_iter, &tst);
     nb_tbs = tst.nb_tbs;
     /* XXX: avoid using doubles ? */
-    qemu_printf("Translation buffer state:\n");
+    g_string_append_printf(buf, "Translation buffer state:\n");
     /*
      * Report total code size including the padding and TB structs;
      * otherwise users might think "-accel tcg,tb-size" is not honoured.
      * For avg host size we use the precise numbers from tb_tree_stats though.
      */
-    qemu_printf("gen code size       %zu/%zu\n",
-                tcg_code_size(), tcg_code_capacity());
-    qemu_printf("TB count            %zu\n", nb_tbs);
-    qemu_printf("TB avg target size  %zu max=%zu bytes\n",
-                nb_tbs ? tst.target_size / nb_tbs : 0,
-                tst.max_target_size);
-    qemu_printf("TB avg host size    %zu bytes (expansion ratio: %0.1f)\n",
-                nb_tbs ? tst.host_size / nb_tbs : 0,
-                tst.target_size ? (double)tst.host_size / tst.target_size : 0);
-    qemu_printf("cross page TB count %zu (%zu%%)\n", tst.cross_page,
-                nb_tbs ? (tst.cross_page * 100) / nb_tbs : 0);
-    qemu_printf("direct jump count   %zu (%zu%%) (2 jumps=%zu %zu%%)\n",
-                tst.direct_jmp_count,
-                nb_tbs ? (tst.direct_jmp_count * 100) / nb_tbs : 0,
-                tst.direct_jmp2_count,
-                nb_tbs ? (tst.direct_jmp2_count * 100) / nb_tbs : 0);
+    g_string_append_printf(buf, "gen code size       %zu/%zu\n",
+                           tcg_code_size(), tcg_code_capacity());
+    g_string_append_printf(buf, "TB count            %zu\n", nb_tbs);
+    g_string_append_printf(buf, "TB avg target size  %zu max=%zu bytes\n",
+                           nb_tbs ? tst.target_size / nb_tbs : 0,
+                           tst.max_target_size);
+    g_string_append_printf(buf, "TB avg host size    %zu bytes "
+                           "(expansion ratio: %0.1f)\n",
+                           nb_tbs ? tst.host_size / nb_tbs : 0,
+                           tst.target_size ?
+                           (double)tst.host_size / tst.target_size : 0);
+    g_string_append_printf(buf, "cross page TB count %zu (%zu%%)\n",
+                           tst.cross_page,
+                           nb_tbs ? (tst.cross_page * 100) / nb_tbs : 0);
+    g_string_append_printf(buf, "direct jump count   %zu (%zu%%) "
+                           "(2 jumps=%zu %zu%%)\n",
+                           tst.direct_jmp_count,
+                           nb_tbs ? (tst.direct_jmp_count * 100) / nb_tbs : 0,
+                           tst.direct_jmp2_count,
+                           nb_tbs ? (tst.direct_jmp2_count * 100) / nb_tbs : 0);
 
     qht_statistics_init(&tb_ctx.htable, &hst);
-    print_qht_statistics(hst);
+    print_qht_statistics(hst, buf);
     qht_statistics_destroy(&hst);
 
-    qemu_printf("\nStatistics:\n");
-    qemu_printf("TB flush count      %u\n",
-                qatomic_read(&tb_ctx.tb_flush_count));
-    qemu_printf("TB invalidate count %u\n",
-                qatomic_read(&tb_ctx.tb_phys_invalidate_count));
+    g_string_append_printf(buf, "\nStatistics:\n");
+    g_string_append_printf(buf, "TB flush count      %u\n",
+                           qatomic_read(&tb_ctx.tb_flush_count));
+    g_string_append_printf(buf, "TB invalidate count %u\n",
+                           qatomic_read(&tb_ctx.tb_phys_invalidate_count));
 
     tlb_flush_counts(&flush_full, &flush_part, &flush_elide);
-    qemu_printf("TLB full flushes    %zu\n", flush_full);
-    qemu_printf("TLB partial flushes %zu\n", flush_part);
-    qemu_printf("TLB elided flushes  %zu\n", flush_elide);
-    tcg_dump_info();
+    g_string_append_printf(buf, "TLB full flushes    %zu\n", flush_full);
+    g_string_append_printf(buf, "TLB partial flushes %zu\n", flush_part);
+    g_string_append_printf(buf, "TLB elided flushes  %zu\n", flush_elide);
+    tcg_dump_info(buf);
 }
 
 void dump_opcount_info(void)
diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
index 32cfb634c6..d92f6fa7a9 100644
--- a/include/exec/cpu-all.h
+++ b/include/exec/cpu-all.h
@@ -429,9 +429,9 @@ static inline bool tlb_hit(target_ulong tlb_addr, target_ulong addr)
 
 #ifdef CONFIG_TCG
 /* accel/tcg/cpu-exec.c */
-void dump_drift_info(void);
+void dump_drift_info(GString *buf);
 /* accel/tcg/translate-all.c */
-void dump_exec_info(void);
+void dump_exec_info(GString *buf);
 void dump_opcount_info(void);
 #endif /* CONFIG_TCG */
 
diff --git a/include/tcg/tcg.h b/include/tcg/tcg.h
index 44ccd86f3e..1b56e382b7 100644
--- a/include/tcg/tcg.h
+++ b/include/tcg/tcg.h
@@ -943,7 +943,7 @@ int tcg_check_temp_count(void);
 #endif
 
 int64_t tcg_cpu_exec_time(void);
-void tcg_dump_info(void);
+void tcg_dump_info(GString *buf);
 void tcg_dump_op_count(void);
 
 #define TCG_CT_CONST  1 /* any constant of register size */
diff --git a/qapi/machine.json b/qapi/machine.json
index 31eafe2fc4..568348309c 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1359,6 +1359,19 @@
 { 'command': 'x-query-irq',
   'returns': 'HumanReadableText' }
 
+##
+# @x-query-jit:
+#
+# Query TCG compiler statistics
+#
+# Returns: TCG compiler statistics
+#
+# Since: 6.2
+##
+{ 'command': 'x-query-jit',
+  'returns': 'HumanReadableText',
+  'if': 'CONFIG_TCG' }
+
 ##
 # @x-query-numa:
 #
diff --git a/tcg/tcg.c b/tcg/tcg.c
index 4142d42d77..7fcdfd9c0f 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -4382,7 +4382,7 @@ int tcg_gen_code(TCGContext *s, TranslationBlock *tb)
 }
 
 #ifdef CONFIG_PROFILER
-void tcg_dump_info(void)
+void tcg_dump_info(GString *buf)
 {
     TCGProfile prof = {};
     const TCGProfile *s;
@@ -4396,53 +4396,59 @@ void tcg_dump_info(void)
     tb_div_count = tb_count ? tb_count : 1;
     tot = s->interm_time + s->code_time;
 
-    qemu_printf("JIT cycles          %" PRId64 " (%0.3f s at 2.4 GHz)\n",
-                tot, tot / 2.4e9);
-    qemu_printf("translated TBs      %" PRId64 " (aborted=%" PRId64
-                " %0.1f%%)\n",
-                tb_count, s->tb_count1 - tb_count,
-                (double)(s->tb_count1 - s->tb_count)
-                / (s->tb_count1 ? s->tb_count1 : 1) * 100.0);
-    qemu_printf("avg ops/TB          %0.1f max=%d\n",
-                (double)s->op_count / tb_div_count, s->op_count_max);
-    qemu_printf("deleted ops/TB      %0.2f\n",
-                (double)s->del_op_count / tb_div_count);
-    qemu_printf("avg temps/TB        %0.2f max=%d\n",
-                (double)s->temp_count / tb_div_count, s->temp_count_max);
-    qemu_printf("avg host code/TB    %0.1f\n",
-                (double)s->code_out_len / tb_div_count);
-    qemu_printf("avg search data/TB  %0.1f\n",
-                (double)s->search_out_len / tb_div_count);
+    g_string_append_printf(buf, "JIT cycles          %" PRId64
+                           " (%0.3f s at 2.4 GHz)\n",
+                           tot, tot / 2.4e9);
+    g_string_append_printf(buf, "translated TBs      %" PRId64
+                           " (aborted=%" PRId64 " %0.1f%%)\n",
+                           tb_count, s->tb_count1 - tb_count,
+                           (double)(s->tb_count1 - s->tb_count)
+                           / (s->tb_count1 ? s->tb_count1 : 1) * 100.0);
+    g_string_append_printf(buf, "avg ops/TB          %0.1f max=%d\n",
+                           (double)s->op_count / tb_div_count, s->op_count_max);
+    g_string_append_printf(buf, "deleted ops/TB      %0.2f\n",
+                           (double)s->del_op_count / tb_div_count);
+    g_string_append_printf(buf, "avg temps/TB        %0.2f max=%d\n",
+                           (double)s->temp_count / tb_div_count,
+                           s->temp_count_max);
+    g_string_append_printf(buf, "avg host code/TB    %0.1f\n",
+                           (double)s->code_out_len / tb_div_count);
+    g_string_append_printf(buf, "avg search data/TB  %0.1f\n",
+                           (double)s->search_out_len / tb_div_count);
     
-    qemu_printf("cycles/op           %0.1f\n",
-                s->op_count ? (double)tot / s->op_count : 0);
-    qemu_printf("cycles/in byte      %0.1f\n",
-                s->code_in_len ? (double)tot / s->code_in_len : 0);
-    qemu_printf("cycles/out byte     %0.1f\n",
-                s->code_out_len ? (double)tot / s->code_out_len : 0);
-    qemu_printf("cycles/search byte     %0.1f\n",
-                s->search_out_len ? (double)tot / s->search_out_len : 0);
+    g_string_append_printf(buf, "cycles/op           %0.1f\n",
+                           s->op_count ? (double)tot / s->op_count : 0);
+    g_string_append_printf(buf, "cycles/in byte      %0.1f\n",
+                           s->code_in_len ? (double)tot / s->code_in_len : 0);
+    g_string_append_printf(buf, "cycles/out byte     %0.1f\n",
+                           s->code_out_len ? (double)tot / s->code_out_len : 0);
+    g_string_append_printf(buf, "cycles/search byte     %0.1f\n",
+                           s->search_out_len ?
+                           (double)tot / s->search_out_len : 0);
     if (tot == 0) {
         tot = 1;
     }
-    qemu_printf("  gen_interm time   %0.1f%%\n",
-                (double)s->interm_time / tot * 100.0);
-    qemu_printf("  gen_code time     %0.1f%%\n",
-                (double)s->code_time / tot * 100.0);
-    qemu_printf("optim./code time    %0.1f%%\n",
-                (double)s->opt_time / (s->code_time ? s->code_time : 1)
-                * 100.0);
-    qemu_printf("liveness/code time  %0.1f%%\n",
-                (double)s->la_time / (s->code_time ? s->code_time : 1) * 100.0);
-    qemu_printf("cpu_restore count   %" PRId64 "\n",
-                s->restore_count);
-    qemu_printf("  avg cycles        %0.1f\n",
-                s->restore_count ? (double)s->restore_time / s->restore_count : 0);
+    g_string_append_printf(buf, "  gen_interm time   %0.1f%%\n",
+                           (double)s->interm_time / tot * 100.0);
+    g_string_append_printf(buf, "  gen_code time     %0.1f%%\n",
+                           (double)s->code_time / tot * 100.0);
+    g_string_append_printf(buf, "optim./code time    %0.1f%%\n",
+                           (double)s->opt_time / (s->code_time ?
+                                                  s->code_time : 1)
+                           * 100.0);
+    g_string_append_printf(buf, "liveness/code time  %0.1f%%\n",
+                           (double)s->la_time / (s->code_time ?
+                                                 s->code_time : 1) * 100.0);
+    g_string_append_printf(buf, "cpu_restore count   %" PRId64 "\n",
+                           s->restore_count);
+    g_string_append_printf(buf, "  avg cycles        %0.1f\n",
+                           s->restore_count ?
+                           (double)s->restore_time / s->restore_count : 0);
 }
 #else
-void tcg_dump_info(void)
+void tcg_dump_info(GString *buf)
 {
-    qemu_printf("[TCG profiler not compiled]\n");
+    g_string_append_printf(buf, "[TCG profiler not compiled]\n");
 }
 #endif
 
diff --git a/tests/qtest/qmp-cmd-test.c b/tests/qtest/qmp-cmd-test.c
index 15875a14c6..6aa628691a 100644
--- a/tests/qtest/qmp-cmd-test.c
+++ b/tests/qtest/qmp-cmd-test.c
@@ -51,6 +51,8 @@ static int query_error_class(const char *cmd)
 #endif
         /* Only valid with a USB bus added */
         { "x-query-usb", ERROR_CLASS_GENERIC_ERROR },
+        /* Only valid with accel=tcg */
+        { "x-query-jit", ERROR_CLASS_GENERIC_ERROR },
         { NULL, -1 }
     };
     int i;
-- 
2.31.1

[PATCH v3 19/19] qapi: introduce x-query-opcount QMP command

[Daniel P. Berrangé]

This is a counterpart to the HMP "info opcount" command. It is being
added with an "x-" prefix because this QMP command is intended as an
ad hoc debugging tool and will thus not be modelled in QAPI as fully
structured data, nor will it have long term guaranteed stability.
The existing HMP command is rewritten to call the QMP command.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 accel/tcg/cpu-exec.c       | 14 ++++++++++++++
 accel/tcg/hmp.c            | 11 ++++++++++-
 accel/tcg/translate-all.c  |  4 ++--
 include/exec/cpu-all.h     |  2 +-
 include/tcg/tcg.h          |  2 +-
 qapi/machine.json          | 13 +++++++++++++
 tcg/tcg.c                  | 10 +++++-----
 tests/qtest/qmp-cmd-test.c |  1 +
 8 files changed, 47 insertions(+), 10 deletions(-)

diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index 84e10e0f49..623120d246 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -1055,4 +1055,18 @@ HumanReadableText *qmp_x_query_jit(Error **errp)
     return human_readable_text_from_str(buf);
 }
 
+HumanReadableText *qmp_x_query_opcount(Error **errp)
+{
+    g_autoptr(GString) buf = g_string_new("");
+
+    if (!tcg_enabled()) {
+        error_setg(errp, "JIT information is only available with accel=tcg");
+        return NULL;
+    }
+
+    dump_opcount_info(buf);
+
+    return human_readable_text_from_str(buf);
+}
+
 #endif /* !CONFIG_USER_ONLY */
diff --git a/accel/tcg/hmp.c b/accel/tcg/hmp.c
index 9d7bcd9185..9b049d1e76 100644
--- a/accel/tcg/hmp.c
+++ b/accel/tcg/hmp.c
@@ -22,7 +22,16 @@ static void hmp_info_jit(Monitor *mon, const QDict *qdict)
 
 static void hmp_info_opcount(Monitor *mon, const QDict *qdict)
 {
-    dump_opcount_info();
+    Error *err = NULL;
+    g_autoptr(HumanReadableText) info = NULL;
+
+    info = qmp_x_query_opcount(&err);
+    if (err) {
+        error_report_err(err);
+        return;
+    }
+
+    monitor_printf(mon, "%s", info->human_readable_text);
 }
 
 static void hmp_tcg_register(void)
diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index 8f17a91858..bd0bb81d08 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -2118,9 +2118,9 @@ void dump_exec_info(GString *buf)
     tcg_dump_info(buf);
 }
 
-void dump_opcount_info(void)
+void dump_opcount_info(GString *buf)
 {
-    tcg_dump_op_count();
+    tcg_dump_op_count(buf);
 }
 
 #else /* CONFIG_USER_ONLY */
diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
index d92f6fa7a9..3c8e24292b 100644
--- a/include/exec/cpu-all.h
+++ b/include/exec/cpu-all.h
@@ -432,7 +432,7 @@ static inline bool tlb_hit(target_ulong tlb_addr, target_ulong addr)
 void dump_drift_info(GString *buf);
 /* accel/tcg/translate-all.c */
 void dump_exec_info(GString *buf);
-void dump_opcount_info(void);
+void dump_opcount_info(GString *buf);
 #endif /* CONFIG_TCG */
 
 #endif /* !CONFIG_USER_ONLY */
diff --git a/include/tcg/tcg.h b/include/tcg/tcg.h
index 1b56e382b7..cc7f59070b 100644
--- a/include/tcg/tcg.h
+++ b/include/tcg/tcg.h
@@ -944,7 +944,7 @@ int tcg_check_temp_count(void);
 
 int64_t tcg_cpu_exec_time(void);
 void tcg_dump_info(GString *buf);
-void tcg_dump_op_count(void);
+void tcg_dump_op_count(GString *buf);
 
 #define TCG_CT_CONST  1 /* any constant of register size */
 
diff --git a/qapi/machine.json b/qapi/machine.json
index 568348309c..c735436c47 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1384,6 +1384,19 @@
 { 'command': 'x-query-numa',
   'returns': 'HumanReadableText' }
 
+##
+# @x-query-opcount:
+#
+# Query TCG opcode counters
+#
+# Returns: TCG opcode counters
+#
+# Since: 6.2
+##
+{ 'command': 'x-query-opcount',
+  'returns': 'HumanReadableText',
+  'if': 'CONFIG_TCG' }
+
 ##
 # @x-query-profile:
 #
diff --git a/tcg/tcg.c b/tcg/tcg.c
index 7fcdfd9c0f..d9653990af 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -4115,15 +4115,15 @@ static void tcg_profile_snapshot_table(TCGProfile *prof)
     tcg_profile_snapshot(prof, false, true);
 }
 
-void tcg_dump_op_count(void)
+void tcg_dump_op_count(GString *buf)
 {
     TCGProfile prof = {};
     int i;
 
     tcg_profile_snapshot_table(&prof);
     for (i = 0; i < NB_OPS; i++) {
-        qemu_printf("%s %" PRId64 "\n", tcg_op_defs[i].name,
-                    prof.table_op_count[i]);
+        g_string_append_printf(buf, "%s %" PRId64 "\n", tcg_op_defs[i].name,
+                               prof.table_op_count[i]);
     }
 }
 
@@ -4142,9 +4142,9 @@ int64_t tcg_cpu_exec_time(void)
     return ret;
 }
 #else
-void tcg_dump_op_count(void)
+void tcg_dump_op_count(GString *buf)
 {
-    qemu_printf("[TCG profiler not compiled]\n");
+    g_string_append_printf(buf, "[TCG profiler not compiled]\n");
 }
 
 int64_t tcg_cpu_exec_time(void)
diff --git a/tests/qtest/qmp-cmd-test.c b/tests/qtest/qmp-cmd-test.c
index 6aa628691a..251a14ddf7 100644
--- a/tests/qtest/qmp-cmd-test.c
+++ b/tests/qtest/qmp-cmd-test.c
@@ -53,6 +53,7 @@ static int query_error_class(const char *cmd)
         { "x-query-usb", ERROR_CLASS_GENERIC_ERROR },
         /* Only valid with accel=tcg */
         { "x-query-jit", ERROR_CLASS_GENERIC_ERROR },
+        { "x-query-opcount", ERROR_CLASS_GENERIC_ERROR },
         { NULL, -1 }
     };
     int i;
-- 
2.31.1