* Markus Armbruster (arm...@redhat.com) wrote:
> Philippe Mathieu-Daudé <f4...@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-ayl...@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-ayl...@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 <dgilb...@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 / dgilb...@redhat.com / Manchester, UK
