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

[Dr. David Alan Gilbert]

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

Dave


> 
> On to "how to do it", part 1.
> 
> Most of the time, the command handler is declared with the command in
> hmp-commands{,-info}.hx, possibly with compile-time conditionals.
> 
> But it can also be left null there, and set with monitor_register_hmp()
> or monitor_register_hmp_info_hrt().  This is intended for modules; see
> commit f0e48cbd791^..bca6eb34f03.
> 
> Aside: can modules be unloaded?  If yes, we better zap the handler
> then.
> 
> The proposed "info via" uses monitor_register_hmp_info_hrt().  No
> objection from me, requires David's ACK.
> 
> 
> "How to do it", part 2, in reply to Philippe's remark.
> 
> Ideally, HMP commands wrap around QMP commands, but we accept exceptions
> for certain use cases where the wrapping is more trouble than it's
> worth, with justification.  I've explained this several times, and I'm
> happy to dig up a reference or explain it again if there's a need.
> 
> Justifying an exception is bothersome, too.  Daniel Berrangé recently
> created a way to reduce the wrapping trouble (merge commit
> e86e00a2493).  The proposed "info via" makes use of it.
> 
> >> (patch below for context)
> >> thanks
> >> -- PMM
> >> 
> >>>
> >>> 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
> 
> [...]
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK