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

[Daniel P . Berrangé]

On Tue, Feb 08, 2022 at 10:29:09AM +0000, Dr. David Alan Gilbert wrote:
> * Markus Armbruster (armbru@redhat.com) wrote:
> > Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
> > 
> > > On 7/2/22 20:34, Peter Maydell wrote:
> > >> On Thu, 27 Jan 2022 at 21:03, Mark Cave-Ayland
> > >> <mark.cave-ayland@ilande.co.uk> 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(+)
> > >> 
> > >> I'm not sure how keen we are on adding new device-specific
> > >> HMP info commands, but it's not my area of expertise. Markus ?
> > >
> > > HMP is David :)
> > 
> > Yes.
> 
> So let me start with an:
> 
> Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> (If it's useful info for the author of the device, then I'm happy for
> HMP to have that), but then - (moving the reply around a bit):
> 
> 
> > Should this be conditional on the targets where we actually link the
> > device, like info skeys?
> > 
> 
> Yes, I think so; it's a reasonably old/obscure device, there's no reason
> everyone having it built in.
> 
> > >                 IIRC it is OK as long as HMP is a QMP wrapper.
> > 
> > That's "how to do it", and I'll get back to it in a jiffie, but Peter
> > was wondering about the "whether to do it".
> > 
> > Most HMP commands are always there.
> > 
> > We have a few specific to compile-time configurable features: TCG, VNC,
> > Spice, Slirp, Linux.  Does not apply here.
> > 
> > We have a few specific to targets, such as dump-skeys and info skeys for
> > s390.  Target-specific is not quite the same as device-specific.
> > 
> > We have no device-specific commands so far.  However, dump-skeys and
> > info skeys appear to be about the skeys *device*, not the s390 target.
> > Perhaps any s390 target has such a device?  I don't know.  My point is
> > we already have device-specific commands, they're just masquerading as
> > target-specific commands.
> 
> Yeh we've got info lapic/ioapic as well.
> 
> > The proposed device-specific command uses a mechanism originally made
> > for modules instead (more on that below).
> > 
> > I think we should make up our minds which way we want device-specific
> > commands done, then do *all* of them that way.
> 
> I think device specific commands make sense, but I think it would
> probably be better if we had an 'info dev $name' and that a method on
> the device rather than registering each one separately.
> I'd assume that this would be a QMP level thing that got unwrapped at
> HMP.
> 
> But that's not a problem for this contribution; someone else can figure
> that out later.

Actually I think this would solve a problem with this contribution.
This patch implements a QMP command but never registers it, so it
isn't actually accessible via QMP. It only registers the HMP wrapper
which rather defeats the point of doing it via the QMP HumanReadableText
approach.

I'm guessing this was done because we don't have ability to dynamically
register QMP commands at runtime.  I don't know how hard/easy it is
to address this, and perhaps we don't even want to.  It was a problem
for me when previously converting HMP info commands to QMP and I
didn't get a solution, so didn't convert anything where the command
impl was dynamically registered at runtime. This basically excluded
converting devices that have been split into loadable modules.

If we had a general  'info dev-debug' (HMP) and  'x-query-dev-debug'
commands, then we could side-step the QMP limitation, as both thue
HMP and QMP comamnds could be statically registered. The devices
then only need to register  a callback at runtime which should be
easier to deal 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 :|