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

[Daniel P . Berrangé]

On Tue, Feb 08, 2022 at 12:39:04PM +0000, Mark Cave-Ayland wrote:
> 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 
> > > o 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"?

The monitor_register_hmp_info_hrt only does the HMP glue side, and
that's only needed if you must dynamically register the HMP command.
For statically registered commands set '.cmd_info_hrt' directly in
the hml-commands-info.hx for the HMP side.

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

Take a look in qapi/machine.json for anyof the 'x-query-XXXX' commands
there. The QAPI bit is fairly simple. 

if you want to see an illustration of what's different from a previous
pure HMP impl, look at:

  commit dd98234c059e6bdb05a52998270df6d3d990332e
  Author: Daniel P. Berrangé <berrange@redhat.com>
  Date:   Wed Sep 8 10:35:43 2021 +0100

    qapi: introduce x-query-roms QMP command



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