[PATCH 0/2] hw/misc/mos6522: Do not open-code hmp_info_human_readable_text()

[PATCH 0/2] hw/misc/mos6522: Do not open-code hmp_info_human_readable_text()

[Philippe Mathieu-Daudé]

Officialise the QMP command, use the existing
hmp_info_human_readable_text() helper.

Philippe Mathieu-Daudé (2):
  hw/misc/mos6522: Expose x-query-mos6522-devices QMP command
  hw/misc/mos6522: Do not open-code hmp_info_human_readable_text()

 MAINTAINERS                  |  2 +-
 qapi/machine.json            | 17 +++++++++++++++++
 include/hw/misc/mos6522.h    |  2 --
 include/monitor/hmp-target.h |  1 -
 hw/misc/mos6522-stubs.c      | 18 ++++++++++++++++++
 hw/misc/mos6522.c            | 16 ++--------------
 hmp-commands-info.hx         |  2 +-
 hw/misc/meson.build          |  3 ++-
 8 files changed, 41 insertions(+), 20 deletions(-)
 create mode 100644 hw/misc/mos6522-stubs.c

-- 
2.41.0

[PATCH 1/2] hw/misc/mos6522: Expose x-query-mos6522-devices QMP command

[Philippe Mathieu-Daudé]

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

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 MAINTAINERS             |  2 +-
 qapi/machine.json       | 17 +++++++++++++++++
 hw/misc/mos6522-stubs.c | 18 ++++++++++++++++++
 hw/misc/mos6522.c       |  5 +++--
 hw/misc/meson.build     |  3 ++-
 5 files changed, 41 insertions(+), 4 deletions(-)
 create mode 100644 hw/misc/mos6522-stubs.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 951556224a..e86638c68c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1453,7 +1453,7 @@ F: hw/ppc/mac_newworld.c
 F: hw/pci-host/uninorth.c
 F: hw/pci-bridge/dec.[hc]
 F: hw/misc/macio/
-F: hw/misc/mos6522.c
+F: hw/misc/mos6522*.c
 F: hw/nvram/mac_nvram.c
 F: hw/ppc/fw_cfg.c
 F: hw/input/adb*
diff --git a/qapi/machine.json b/qapi/machine.json
index 1283d14493..a82b8dd39d 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1865,6 +1865,23 @@
   'data': { 'filename': 'str' },
   'if': 'CONFIG_FDT' }
 
+##
+# @x-query-mos6522-devices:
+#
+# Query information on MOS6522 VIA devices
+#
+# Features:
+#
+# @unstable: This command is meant for debugging.
+#
+# Returns: MOS6522 VIA devices information
+#
+# Since: 9.1
+##
+{ 'command': 'x-query-mos6522-devices',
+  'returns': 'HumanReadableText',
+  'features': [ 'unstable' ]}
+
 ##
 # @x-query-interrupt-controllers:
 #
diff --git a/hw/misc/mos6522-stubs.c b/hw/misc/mos6522-stubs.c
new file mode 100644
index 0000000000..c953f01a16
--- /dev/null
+++ b/hw/misc/mos6522-stubs.c
@@ -0,0 +1,18 @@
+/*
+ * QEMU MOS6522 VIA stubs
+ *
+ * SPDX-FileContributor: Philippe Mathieu-Daudé <philmd@linaro.org>
+ * SPDX-FileCopyrightText: 2024 Linaro Ltd.
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "qapi/qapi-commands-machine.h"
+
+HumanReadableText *qmp_x_query_mos6522_devices(Error **errp)
+{
+    error_setg(errp, "Support for MOS6522 VIA devices not built-in");
+
+    return NULL;
+}
diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index 515f62e687..b1bb7f54f0 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -31,6 +31,7 @@
 #include "migration/vmstate.h"
 #include "monitor/monitor.h"
 #include "monitor/hmp.h"
+#include "qapi/qapi-commands-machine.h"
 #include "qapi/type-helpers.h"
 #include "qemu/timer.h"
 #include "qemu/cutils.h"
@@ -576,7 +577,7 @@ static int qmp_x_query_via_foreach(Object *obj, void *opaque)
     return 0;
 }
 
-static HumanReadableText *qmp_x_query_via(Error **errp)
+HumanReadableText *qmp_x_query_mos6522_devices(Error **errp)
 {
     g_autoptr(GString) buf = g_string_new("");
 
@@ -589,7 +590,7 @@ static HumanReadableText *qmp_x_query_via(Error **errp)
 void hmp_info_via(Monitor *mon, const QDict *qdict)
 {
     Error *err = NULL;
-    g_autoptr(HumanReadableText) info = qmp_x_query_via(&err);
+    g_autoptr(HumanReadableText) info = qmp_x_query_mos6522_devices(&err);
 
     if (hmp_handle_error(mon, err)) {
         return;
diff --git a/hw/misc/meson.build b/hw/misc/meson.build
index 86596a3888..9fa0e98794 100644
--- a/hw/misc/meson.build
+++ b/hw/misc/meson.build
@@ -18,7 +18,8 @@ system_ss.add(when: 'CONFIG_ARM11SCU', if_true: files('arm11scu.c'))
 system_ss.add(when: 'CONFIG_ARM_V7M', if_true: files('armv7m_ras.c'))
 
 # Mac devices
-system_ss.add(when: 'CONFIG_MOS6522', if_true: files('mos6522.c'))
+system_ss.add(when: 'CONFIG_MOS6522', if_true: files('mos6522.c'),
+                                      if_false: files('mos6522-stubs.c'))
 system_ss.add(when: 'CONFIG_DJMEMC', if_true: files('djmemc.c'))
 system_ss.add(when: 'CONFIG_IOSB', if_true: files('iosb.c'))
 
-- 
2.41.0

Re: [PATCH 1/2] hw/misc/mos6522: Expose x-query-mos6522-devices QMP command

[Daniel P. Berrangé]

On Mon, Jun 10, 2024 at 05:07:57PM +0200, Philippe Mathieu-Daudé wrote:
> This is a counterpart to the HMP "info via" command. It is being
> added with an "x-" prefix because this QMP command is intended as an
> adhoc debugging tool and will thus not be modelled in QAPI as fully
> structured data, nor will it have long term guaranteed stability.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  MAINTAINERS             |  2 +-
>  qapi/machine.json       | 17 +++++++++++++++++
>  hw/misc/mos6522-stubs.c | 18 ++++++++++++++++++
>  hw/misc/mos6522.c       |  5 +++--
>  hw/misc/meson.build     |  3 ++-
>  5 files changed, 41 insertions(+), 4 deletions(-)
>  create mode 100644 hw/misc/mos6522-stubs.c

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

With 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 1/2] hw/misc/mos6522: Expose x-query-mos6522-devices QMP command

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> This is a counterpart to the HMP "info via" command. It is being
> added with an "x-" prefix because this QMP command is intended as an
> adhoc debugging tool and will thus not be modelled in QAPI as fully
> structured data, nor will it have long term guaranteed stability.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  MAINTAINERS             |  2 +-
>  qapi/machine.json       | 17 +++++++++++++++++
>  hw/misc/mos6522-stubs.c | 18 ++++++++++++++++++
>  hw/misc/mos6522.c       |  5 +++--
>  hw/misc/meson.build     |  3 ++-
>  5 files changed, 41 insertions(+), 4 deletions(-)
>  create mode 100644 hw/misc/mos6522-stubs.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 951556224a..e86638c68c 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1453,7 +1453,7 @@ F: hw/ppc/mac_newworld.c
>  F: hw/pci-host/uninorth.c
>  F: hw/pci-bridge/dec.[hc]
>  F: hw/misc/macio/
> -F: hw/misc/mos6522.c
> +F: hw/misc/mos6522*.c
>  F: hw/nvram/mac_nvram.c
>  F: hw/ppc/fw_cfg.c
>  F: hw/input/adb*
> diff --git a/qapi/machine.json b/qapi/machine.json
> index 1283d14493..a82b8dd39d 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json

I figure you pick machine.json because it already serves as grabbag of
vaguely device-specific queries like x-query-usb.  misc-target.json is
another grabbag.

> @@ -1865,6 +1865,23 @@
>    'data': { 'filename': 'str' },
>    'if': 'CONFIG_FDT' }
>  
> +##
> +# @x-query-mos6522-devices:
> +#
> +# Query information on MOS6522 VIA devices
> +#
> +# Features:
> +#
> +# @unstable: This command is meant for debugging.
> +#
> +# Returns: MOS6522 VIA devices information
> +#
> +# Since: 9.1
> +##
> +{ 'command': 'x-query-mos6522-devices',
> +  'returns': 'HumanReadableText',
> +  'features': [ 'unstable' ]}
> +
>  ##
>  # @x-query-interrupt-controllers:
>  #

HMP "info via" is compile-time conditional on CONFIG_MOS6522.

Its new QMP counterpart x-query-mos6522-devices is unconditional.
Can you explain why?

Possibly related:

commit 409e9f7131e55e74eb09e65535779e311df5ebf5
Author: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Date:   Sat Mar 5 15:09:53 2022 +0000

    mos6522: add "info via" HMP command for debugging
    
    This displays detailed information about the device registers and timers to aid
    debugging problems with timers and interrupts.
    
--> Currently the QAPI generators for HumanReadableText don't work correctly if
--> used in qapi/target-misc.json when a non-specified target is built, so for
--> now manually add a hmp_info_via() wrapper until direct support for per-device
--> HMP/QMP commands is implemented.
    
    Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
    Reviewed-by: Laurent Vivier <laurent@vivier.eu>
    Message-Id: <20220305150957.5053-9-mark.cave-ayland@ilande.co.uk>
    Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

[...]

[PATCH 2/2] hw/misc/mos6522: Do not open-code hmp_info_human_readable_text()

[Philippe Mathieu-Daudé]

Register the command 'info via' using HMPCommand::cmd_info_hrt(),
so it is processed using the generic hmp_info_human_readable_text().

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/misc/mos6522.h    |  2 --
 include/monitor/hmp-target.h |  1 -
 hw/misc/mos6522.c            | 13 -------------
 hmp-commands-info.hx         |  2 +-
 4 files changed, 1 insertion(+), 17 deletions(-)

diff --git a/include/hw/misc/mos6522.h b/include/hw/misc/mos6522.h
index fba45668ab..a54fe063ac 100644
--- a/include/hw/misc/mos6522.h
+++ b/include/hw/misc/mos6522.h
@@ -172,6 +172,4 @@ extern const VMStateDescription vmstate_mos6522;
 uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned size);
 void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size);
 
-void hmp_info_via(Monitor *mon, const QDict *qdict);
-
 #endif /* MOS6522_H */
diff --git a/include/monitor/hmp-target.h b/include/monitor/hmp-target.h
index b679aaebbf..9b46fec84a 100644
--- a/include/monitor/hmp-target.h
+++ b/include/monitor/hmp-target.h
@@ -53,7 +53,6 @@ void hmp_mce(Monitor *mon, const QDict *qdict);
 void hmp_info_local_apic(Monitor *mon, const QDict *qdict);
 void hmp_info_sev(Monitor *mon, const QDict *qdict);
 void hmp_info_sgx(Monitor *mon, const QDict *qdict);
-void hmp_info_via(Monitor *mon, const QDict *qdict);
 void hmp_memory_dump(Monitor *mon, const QDict *qdict);
 void hmp_physical_memory_dump(Monitor *mon, const QDict *qdict);
 void hmp_info_registers(Monitor *mon, const QDict *qdict);
diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index b1bb7f54f0..afa343dd27 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -29,8 +29,6 @@
 #include "hw/misc/mos6522.h"
 #include "hw/qdev-properties.h"
 #include "migration/vmstate.h"
-#include "monitor/monitor.h"
-#include "monitor/hmp.h"
 #include "qapi/qapi-commands-machine.h"
 #include "qapi/type-helpers.h"
 #include "qemu/timer.h"
@@ -587,17 +585,6 @@ HumanReadableText *qmp_x_query_mos6522_devices(Error **errp)
     return human_readable_text_from_str(buf);
 }
 
-void hmp_info_via(Monitor *mon, const QDict *qdict)
-{
-    Error *err = NULL;
-    g_autoptr(HumanReadableText) info = qmp_x_query_mos6522_devices(&err);
-
-    if (hmp_handle_error(mon, err)) {
-        return;
-    }
-    monitor_puts(mon, info->human_readable_text);
-}
-
 static const MemoryRegionOps mos6522_ops = {
     .read = mos6522_read,
     .write = mos6522_write,
diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index cfd4ad5651..a24c217d89 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -873,7 +873,7 @@ ERST
         .args_type    = "",
         .params       = "",
         .help         = "show guest mos6522 VIA devices",
-        .cmd          = hmp_info_via,
+        .cmd_info_hrt = qmp_x_query_mos6522_devices,
     },
 #endif
 
-- 
2.41.0

Re: [PATCH 2/2] hw/misc/mos6522: Do not open-code hmp_info_human_readable_text()

[Daniel P. Berrangé]

On Mon, Jun 10, 2024 at 05:07:58PM +0200, Philippe Mathieu-Daudé wrote:
> Register the command 'info via' using HMPCommand::cmd_info_hrt(),
> so it is processed using the generic hmp_info_human_readable_text().
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  include/hw/misc/mos6522.h    |  2 --
>  include/monitor/hmp-target.h |  1 -
>  hw/misc/mos6522.c            | 13 -------------
>  hmp-commands-info.hx         |  2 +-
>  4 files changed, 1 insertion(+), 17 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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

Examining device state via monitor for debugging (was: [PATCH 0/2] hw/misc/mos6522: Do not open-code hmp_info_human_readable_text())

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> Officialise the QMP command, use the existing
> hmp_info_human_readable_text() helper.

I'm not sure "officialise" is a word :)

Taking a step back...  "info via" and its new QMP counterpart
x-query-mos6522-devices dump device state.  I understand why examining
device state via monitor can be useful for debugging.  However, we have
more than 2000 devices in the tree.  Clearly, we don't want 2000 device
state queries.  Not even 100.  Could we have more generic means instead?

We could use QOM (read-only) properties to expose device state.

If we use one QOM property per "thing", examining device state becomes
quite tedious.  Also, you'd have to stop the guest to get a consistent
view, and adding lots of QOM properties bloats the code.

If we use a single, object-valued property for the entire state, we get
to define the objects in QAPI.  Differently tedious, and bloats the
generated code.

We could use a single string-valued property.  Too much of an abuse of
QOM?

We could add an optional "dump state for debugging" method to QOM, and
have a single query command that calls it if present.

Thoughts?

Re: Examining device state via monitor for debugging

[Mark Cave-Ayland]

On 11/06/2024 06:49, Markus Armbruster wrote:

> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
> 
>> Officialise the QMP command, use the existing
>> hmp_info_human_readable_text() helper.
> 
> I'm not sure "officialise" is a word :)
> 
> Taking a step back...  "info via" and its new QMP counterpart
> x-query-mos6522-devices dump device state.  I understand why examining
> device state via monitor can be useful for debugging.  However, we have
> more than 2000 devices in the tree.  Clearly, we don't want 2000 device
> state queries.  Not even 100.  Could we have more generic means instead?
> 
> We could use QOM (read-only) properties to expose device state.
> 
> If we use one QOM property per "thing", examining device state becomes
> quite tedious.  Also, you'd have to stop the guest to get a consistent
> view, and adding lots of QOM properties bloats the code.
> 
> If we use a single, object-valued property for the entire state, we get
> to define the objects in QAPI.  Differently tedious, and bloats the
> generated code.
> 
> We could use a single string-valued property.  Too much of an abuse of
> QOM?
> 
> We could add an optional "dump state for debugging" method to QOM, and
> have a single query command that calls it if present.
> 
> Thoughts?

I agree that there should be a better way of doing things here. The aim of the 
original "info via" series was to allow the command to be contained completely within 
mos6522.c, but unfortunately due to the way that qemu-options.hx works then you end 
up with #ifdef-fery or stubs to make all configuration combinations work.

As you point out ideally there should be a way for a QOM object to dynamically 
register its own monitor commands, which I think should help with this.

IIRC in the original thread Daniel or David proposed a new "debug" monitor command 
such that a device could register its own debug <foo> commands either via DeviceClass 
or a function called during realize that would return a HumanReadableText via QMP.

In terms of "info via" it is only used by developers for the 68k and PPC Mac machines 
so if it were to change from "info via" to "debug via" I don't see there would be a 
big problem with this.


ATB,

Mark.

Re: Examining device state via monitor for debugging

[Manos Pitsidianakis]

On Tue, 11 Jun 2024 at 09:11, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> On 11/06/2024 06:49, Markus Armbruster wrote:
>
> > Philippe Mathieu-Daudé <philmd@linaro.org> writes:
> >
> >> Officialise the QMP command, use the existing
> >> hmp_info_human_readable_text() helper.
> >
> > I'm not sure "officialise" is a word :)
> >
> > Taking a step back...  "info via" and its new QMP counterpart
> > x-query-mos6522-devices dump device state.  I understand why examining
> > device state via monitor can be useful for debugging.  However, we have
> > more than 2000 devices in the tree.  Clearly, we don't want 2000 device
> > state queries.  Not even 100.  Could we have more generic means instead?
> >
> > We could use QOM (read-only) properties to expose device state.
> >
> > If we use one QOM property per "thing", examining device state becomes
> > quite tedious.  Also, you'd have to stop the guest to get a consistent
> > view, and adding lots of QOM properties bloats the code.
> >
> > If we use a single, object-valued property for the entire state, we get
> > to define the objects in QAPI.  Differently tedious, and bloats the
> > generated code.
> >
> > We could use a single string-valued property.  Too much of an abuse of
> > QOM?
> >
> > We could add an optional "dump state for debugging" method to QOM, and
> > have a single query command that calls it if present.
> >
> > Thoughts?
>
> I agree that there should be a better way of doing things here. The aim of the
> original "info via" series was to allow the command to be contained completely within
> mos6522.c, but unfortunately due to the way that qemu-options.hx works then you end
> up with #ifdef-fery or stubs to make all configuration combinations work.
>
> As you point out ideally there should be a way for a QOM object to dynamically
> register its own monitor commands, which I think should help with this.
>
> IIRC in the original thread Daniel or David proposed a new "debug" monitor command
> such that a device could register its own debug <foo> commands either via DeviceClass
> or a function called during realize that would return a HumanReadableText via QMP.

This is starting to sound like OOP: A Monitor interface defines
monitor commands, and QOM type classes can implement/define their own.
A Debug interface would do this.

Re: Examining device state via monitor for debugging

[Mark Cave-Ayland]

On 11/06/2024 07:58, Manos Pitsidianakis wrote:

> On Tue, 11 Jun 2024 at 09:11, Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk> wrote:
>>
>> On 11/06/2024 06:49, Markus Armbruster wrote:
>>
>>> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
>>>
>>>> Officialise the QMP command, use the existing
>>>> hmp_info_human_readable_text() helper.
>>>
>>> I'm not sure "officialise" is a word :)
>>>
>>> Taking a step back...  "info via" and its new QMP counterpart
>>> x-query-mos6522-devices dump device state.  I understand why examining
>>> device state via monitor can be useful for debugging.  However, we have
>>> more than 2000 devices in the tree.  Clearly, we don't want 2000 device
>>> state queries.  Not even 100.  Could we have more generic means instead?
>>>
>>> We could use QOM (read-only) properties to expose device state.
>>>
>>> If we use one QOM property per "thing", examining device state becomes
>>> quite tedious.  Also, you'd have to stop the guest to get a consistent
>>> view, and adding lots of QOM properties bloats the code.
>>>
>>> If we use a single, object-valued property for the entire state, we get
>>> to define the objects in QAPI.  Differently tedious, and bloats the
>>> generated code.
>>>
>>> We could use a single string-valued property.  Too much of an abuse of
>>> QOM?
>>>
>>> We could add an optional "dump state for debugging" method to QOM, and
>>> have a single query command that calls it if present.
>>>
>>> Thoughts?
>>
>> I agree that there should be a better way of doing things here. The aim of the
>> original "info via" series was to allow the command to be contained completely within
>> mos6522.c, but unfortunately due to the way that qemu-options.hx works then you end
>> up with #ifdef-fery or stubs to make all configuration combinations work.
>>
>> As you point out ideally there should be a way for a QOM object to dynamically
>> register its own monitor commands, which I think should help with this.
>>
>> IIRC in the original thread Daniel or David proposed a new "debug" monitor command
>> such that a device could register its own debug <foo> commands either via DeviceClass
>> or a function called during realize that would return a HumanReadableText via QMP.
> 
> This is starting to sound like OOP: A Monitor interface defines
> monitor commands, and QOM type classes can implement/define their own.
> A Debug interface would do this.

I like this idea: having a QOM interface that objects can implement to register their 
own monitor commands feels like a good solution.


ATB,

Mark.

Re: Examining device state via monitor for debugging (was: [PATCH 0/2] hw/misc/mos6522: Do not open-code hmp_info_human_readable_text())

[Daniel P. Berrangé]

On Tue, Jun 11, 2024 at 07:49:12AM +0200, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
> 
> > Officialise the QMP command, use the existing
> > hmp_info_human_readable_text() helper.
> 
> I'm not sure "officialise" is a word :)
> 
> Taking a step back...  "info via" and its new QMP counterpart
> x-query-mos6522-devices dump device state.  I understand why examining
> device state via monitor can be useful for debugging.  However, we have
> more than 2000 devices in the tree.  Clearly, we don't want 2000 device
> state queries.  Not even 100.  Could we have more generic means instead?
> 
> We could use QOM (read-only) properties to expose device state.
> 
> If we use one QOM property per "thing", examining device state becomes
> quite tedious.  Also, you'd have to stop the guest to get a consistent
> view, and adding lots of QOM properties bloats the code.
> 
> If we use a single, object-valued property for the entire state, we get
> to define the objects in QAPI.  Differently tedious, and bloats the
> generated code.
> 
> We could use a single string-valued property.  Too much of an abuse of
> QOM?

Yeah, I'd suggest we just keep it dumb and free form, adding a
callback like this to the QOM base class:

  HumanReadableText (*debug_state)(Error **errp);

With 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: Examining device state via monitor for debugging

[Mark Cave-Ayland]

On 11/06/2024 08:57, Daniel P. Berrangé wrote:

> On Tue, Jun 11, 2024 at 07:49:12AM +0200, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
>>
>>> Officialise the QMP command, use the existing
>>> hmp_info_human_readable_text() helper.
>>
>> I'm not sure "officialise" is a word :)
>>
>> Taking a step back...  "info via" and its new QMP counterpart
>> x-query-mos6522-devices dump device state.  I understand why examining
>> device state via monitor can be useful for debugging.  However, we have
>> more than 2000 devices in the tree.  Clearly, we don't want 2000 device
>> state queries.  Not even 100.  Could we have more generic means instead?
>>
>> We could use QOM (read-only) properties to expose device state.
>>
>> If we use one QOM property per "thing", examining device state becomes
>> quite tedious.  Also, you'd have to stop the guest to get a consistent
>> view, and adding lots of QOM properties bloats the code.
>>
>> If we use a single, object-valued property for the entire state, we get
>> to define the objects in QAPI.  Differently tedious, and bloats the
>> generated code.
>>
>> We could use a single string-valued property.  Too much of an abuse of
>> QOM?
> 
> Yeah, I'd suggest we just keep it dumb and free form, adding a
> callback like this to the QOM base class:
> 
>    HumanReadableText (*debug_state)(Error **errp);

I think that's a little bit too restrictive: certainly I can see the need for 
allowing parameters for the output to be customised, and there may also be cases 
where a device may want to register more than one debug (or monitor) command.

Currently I quite like Manos' solution since it allows a MonitorInterface to be 
defined, and that could have separate methods for registering both "info" and "debug" 
commands. Longer term this could allow for e.g. TYPE_TCG_ACCEL to register "info jit" 
and devices can add whatever debug monitor commands they need.


ATB,

Mark.

Re: Examining device state via monitor for debugging

[Daniel P. Berrangé]

On Tue, Jun 11, 2024 at 09:13:00AM +0100, Mark Cave-Ayland wrote:
> On 11/06/2024 08:57, Daniel P. Berrangé wrote:
> 
> > On Tue, Jun 11, 2024 at 07:49:12AM +0200, Markus Armbruster wrote:
> > > Philippe Mathieu-Daudé <philmd@linaro.org> writes:
> > > 
> > > > Officialise the QMP command, use the existing
> > > > hmp_info_human_readable_text() helper.
> > > 
> > > I'm not sure "officialise" is a word :)
> > > 
> > > Taking a step back...  "info via" and its new QMP counterpart
> > > x-query-mos6522-devices dump device state.  I understand why examining
> > > device state via monitor can be useful for debugging.  However, we have
> > > more than 2000 devices in the tree.  Clearly, we don't want 2000 device
> > > state queries.  Not even 100.  Could we have more generic means instead?
> > > 
> > > We could use QOM (read-only) properties to expose device state.
> > > 
> > > If we use one QOM property per "thing", examining device state becomes
> > > quite tedious.  Also, you'd have to stop the guest to get a consistent
> > > view, and adding lots of QOM properties bloats the code.
> > > 
> > > If we use a single, object-valued property for the entire state, we get
> > > to define the objects in QAPI.  Differently tedious, and bloats the
> > > generated code.
> > > 
> > > We could use a single string-valued property.  Too much of an abuse of
> > > QOM?
> > 
> > Yeah, I'd suggest we just keep it dumb and free form, adding a
> > callback like this to the QOM base class:
> > 
> >    HumanReadableText (*debug_state)(Error **errp);
> 
> I think that's a little bit too restrictive: certainly I can see the need
> for allowing parameters for the output to be customised, and there may also
> be cases where a device may want to register more than one debug (or
> monitor) command.
> 
> Currently I quite like Manos' solution since it allows a MonitorInterface to
> be defined, and that could have separate methods for registering both "info"
> and "debug" commands. Longer term this could allow for e.g. TYPE_TCG_ACCEL
> to register "info jit" and devices can add whatever debug monitor commands
> they need.

The downside is that this wires the monitor APIs into the internals of the
devices, instead of having internals work exclusively in terms of QAPI
types.

With 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: Examining device state via monitor for debugging (was: [PATCH 0/2] hw/misc/mos6522: Do not open-code hmp_info_human_readable_text())

[Peter Maydell]

On Tue, 11 Jun 2024 at 06:50, Markus Armbruster <armbru@redhat.com> wrote:
>
> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
>
> > Officialise the QMP command, use the existing
> > hmp_info_human_readable_text() helper.
>
> I'm not sure "officialise" is a word :)
>
> Taking a step back...  "info via" and its new QMP counterpart
> x-query-mos6522-devices dump device state.  I understand why examining
> device state via monitor can be useful for debugging.  However, we have
> more than 2000 devices in the tree.  Clearly, we don't want 2000 device
> state queries.  Not even 100.  Could we have more generic means instead?
>
> We could use QOM (read-only) properties to expose device state.
>
> If we use one QOM property per "thing", examining device state becomes
> quite tedious.  Also, you'd have to stop the guest to get a consistent
> view, and adding lots of QOM properties bloats the code.
>
> If we use a single, object-valued property for the entire state, we get
> to define the objects in QAPI.  Differently tedious, and bloats the
> generated code.

We already have a machine readable mandatory-for-every-device
representation of its entire state -- it's the vmstate struct.
Admittedly this is sometimes a bit different from the guest-facing
view of a device and we don't machine-record the field names...

-- PMM

Re: Examining device state via monitor for debugging (was: [PATCH 0/2] hw/misc/mos6522: Do not open-code hmp_info_human_readable_text())

[Dr. David Alan Gilbert]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> On Tue, 11 Jun 2024 at 06:50, Markus Armbruster <armbru@redhat.com> wrote:
> >
> > Philippe Mathieu-Daudé <philmd@linaro.org> writes:
> >
> > > Officialise the QMP command, use the existing
> > > hmp_info_human_readable_text() helper.
> >
> > I'm not sure "officialise" is a word :)
> >
> > Taking a step back...  "info via" and its new QMP counterpart
> > x-query-mos6522-devices dump device state.  I understand why examining
> > device state via monitor can be useful for debugging.  However, we have
> > more than 2000 devices in the tree.  Clearly, we don't want 2000 device
> > state queries.  Not even 100.  Could we have more generic means instead?
> >
> > We could use QOM (read-only) properties to expose device state.
> >
> > If we use one QOM property per "thing", examining device state becomes
> > quite tedious.  Also, you'd have to stop the guest to get a consistent
> > view, and adding lots of QOM properties bloats the code.
> >
> > If we use a single, object-valued property for the entire state, we get
> > to define the objects in QAPI.  Differently tedious, and bloats the
> > generated code.
> 
> We already have a machine readable mandatory-for-every-device
> representation of its entire state -- it's the vmstate struct.
> Admittedly this is sometimes a bit different from the guest-facing
> view of a device and we don't machine-record the field names...

vmstate might not contain everything needed for debug; devices
do a lot of fiddling to create a consistent vmstate, and there may
be more that someone debugging wants (e.g. fd's or host memory addresses).
It's also hopelessly cryptic.

From an HMP point, a 'info debug <DEVICENAME>' seems good to me,
possibly with some options as to whether to recurse or perhaps
add flags to 'info qtree' to also call it.

Dave




> -- PMM
-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
\        dave @ treblig.org |                               | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/