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

[Markus Armbruster <armbru@redhat.com>]

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.

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

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.


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

Should this be conditional on the targets where we actually link the
device, like info skeys?

[...]