Re: [PATCH 08/11] mos6522: add "info via" HMP command for debugging

[Mark Cave-Ayland]

On 08/02/2022 11:38, Daniel P. Berrangé wrote:

> On Thu, Jan 27, 2022 at 08:54:02PM +0000, Mark Cave-Ayland wrote:
> > This displays detailed information about the device registers and timers to aid
> > debugging problems with timers and interrupts.
> >
> > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > ---
> >   hmp-commands-info.hx | 12 ++++++
> >   hw/misc/mos6522.c    | 92 ++++++++++++++++++++++++++++++++++++++++++++
> >   2 files changed, 104 insertions(+)
> >
> > diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> > index e90f20a107..4e714e79a2 100644
> > --- a/hmp-commands-info.hx
> > +++ b/hmp-commands-info.hx
> > @@ -879,3 +879,15 @@ SRST
> >     ``info sgx``
> >       Show intel SGX information.
> >   ERST
> > +
> > +    {
> > +        .name       = "via",
> > +        .args_type  = "",
> > +        .params     = "",
> > +        .help       = "show guest 6522 VIA devices",
> > +    },
> > +
> > +SRST
> > +  ``info via``
> > +    Show guest 6522 VIA devices.
> > +ERST
> > diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
> > index aaae195d63..cfa6a9c44b 100644
> > --- a/hw/misc/mos6522.c
> > +++ b/hw/misc/mos6522.c
> > @@ -30,6 +30,8 @@
> >   #include "hw/misc/mos6522.h"
> >   #include "hw/qdev-properties.h"
> >   #include "migration/vmstate.h"
> > +#include "monitor/monitor.h"
> > +#include "qapi/type-helpers.h"
> >   #include "qemu/timer.h"
> >   #include "qemu/cutils.h"
> >   #include "qemu/log.h"
> > @@ -415,6 +417,95 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t 
> > val, unsigned size)
> >       }
> >   }
> >   
> > +static int qmp_x_query_via_foreach(Object *obj, void *opaque)
>
>
> > +
> > +static HumanReadableText *qmp_x_query_via(Error **errp)
> > +{
> > +    g_autoptr(GString) buf = g_string_new("");
> > +
> > +    object_child_foreach_recursive(object_get_root(),
> > +                                   qmp_x_query_via_foreach, buf);
> > +
> > +    return human_readable_text_from_str(buf);
> > +}
>
> This provides a code handler for a QMP command which is good,
> but doesn't ever define the QMP command in the qapi/ schema.

First of all, thank you for writing the docs at 
https://www.qemu.org/docs/master/devel/writing-monitor-commands.html#writing-a-debugging-aid-returning-unstructured-text 
which were really useful when writing this patch.

I was under the impression that monitor_register_hmp_info_hrt() does all the magic 
here i.e. it declares the underlying QMP command with an x- prefix and effectively 
encapsulates the text field in a way that says "this is an unreliable text opaque for 
humans"?

If a qapi/ schema is needed could you explain what it should look like for this 
example and where it should go? Looking at the existing .json files I can't 
immediately see one which is the right place for this to live.

> >   static const MemoryRegionOps mos6522_ops = {
> >       .read = mos6522_read,
> >       .write = mos6522_write,
> > @@ -547,6 +638,7 @@ static const TypeInfo mos6522_type_info = {
> >   static void mos6522_register_types(void)
> >   {
> >       type_register_static(&mos6522_type_info);
> > +    monitor_register_hmp_info_hrt("via", qmp_x_query_via);
>
> This only registers the HMP counterpart.
>
> The idea of the HumanReadableText handler is that it is calling
> a QMP command that is exposed to apps.
>
> >   }
>
> Regards,
> Daniel


ATB,

Mark.