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

[Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>]

On 08/02/2022 10:29, 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.

Unfortunately that doesn't seem to work: whilst the target CONFIG_* defines are 
declared when processing hmp-commands-info.hx it seems the the device CONFIG_* 
defines are missing?

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

... which I suspect is a workaround for only the target CONFIG_* defines being declared.

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


ATB,

Mark.