| Message ID | 20240610150758.2827-1-philmd@linaro.org |
|---|---|
| Headers | show |
| Series | hw/misc/mos6522: Do not open-code hmp_info_human_readable_text() | expand |
On 11/06/2024 06:49, Markus Armbruster wrote: > Philippe Mathieu-Daudé <philmd@linaro.org> writes: > >> Officialise the QMP command, use the existing >> hmp_info_human_readable_text() helper. > > I'm not sure "officialise" is a word :) > > Taking a step back... "info via" and its new QMP counterpart > x-query-mos6522-devices dump device state. I understand why examining > device state via monitor can be useful for debugging. However, we have > more than 2000 devices in the tree. Clearly, we don't want 2000 device > state queries. Not even 100. Could we have more generic means instead? > > We could use QOM (read-only) properties to expose device state. > > If we use one QOM property per "thing", examining device state becomes > quite tedious. Also, you'd have to stop the guest to get a consistent > view, and adding lots of QOM properties bloats the code. > > If we use a single, object-valued property for the entire state, we get > to define the objects in QAPI. Differently tedious, and bloats the > generated code. > > We could use a single string-valued property. Too much of an abuse of > QOM? > > We could add an optional "dump state for debugging" method to QOM, and > have a single query command that calls it if present. > > Thoughts? I agree that there should be a better way of doing things here. The aim of the original "info via" series was to allow the command to be contained completely within mos6522.c, but unfortunately due to the way that qemu-options.hx works then you end up with #ifdef-fery or stubs to make all configuration combinations work. As you point out ideally there should be a way for a QOM object to dynamically register its own monitor commands, which I think should help with this. IIRC in the original thread Daniel or David proposed a new "debug" monitor command such that a device could register its own debug <foo> commands either via DeviceClass or a function called during realize that would return a HumanReadableText via QMP. In terms of "info via" it is only used by developers for the 68k and PPC Mac machines so if it were to change from "info via" to "debug via" I don't see there would be a big problem with this. ATB, Mark.
On Tue, 11 Jun 2024 at 09:11, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote: > > On 11/06/2024 06:49, Markus Armbruster wrote: > > > Philippe Mathieu-Daudé <philmd@linaro.org> writes: > > > >> Officialise the QMP command, use the existing > >> hmp_info_human_readable_text() helper. > > > > I'm not sure "officialise" is a word :) > > > > Taking a step back... "info via" and its new QMP counterpart > > x-query-mos6522-devices dump device state. I understand why examining > > device state via monitor can be useful for debugging. However, we have > > more than 2000 devices in the tree. Clearly, we don't want 2000 device > > state queries. Not even 100. Could we have more generic means instead? > > > > We could use QOM (read-only) properties to expose device state. > > > > If we use one QOM property per "thing", examining device state becomes > > quite tedious. Also, you'd have to stop the guest to get a consistent > > view, and adding lots of QOM properties bloats the code. > > > > If we use a single, object-valued property for the entire state, we get > > to define the objects in QAPI. Differently tedious, and bloats the > > generated code. > > > > We could use a single string-valued property. Too much of an abuse of > > QOM? > > > > We could add an optional "dump state for debugging" method to QOM, and > > have a single query command that calls it if present. > > > > Thoughts? > > I agree that there should be a better way of doing things here. The aim of the > original "info via" series was to allow the command to be contained completely within > mos6522.c, but unfortunately due to the way that qemu-options.hx works then you end > up with #ifdef-fery or stubs to make all configuration combinations work. > > As you point out ideally there should be a way for a QOM object to dynamically > register its own monitor commands, which I think should help with this. > > IIRC in the original thread Daniel or David proposed a new "debug" monitor command > such that a device could register its own debug <foo> commands either via DeviceClass > or a function called during realize that would return a HumanReadableText via QMP. This is starting to sound like OOP: A Monitor interface defines monitor commands, and QOM type classes can implement/define their own. A Debug interface would do this.
On Tue, Jun 11, 2024 at 07:49:12AM +0200, Markus Armbruster wrote: > Philippe Mathieu-Daudé <philmd@linaro.org> writes: > > > Officialise the QMP command, use the existing > > hmp_info_human_readable_text() helper. > > I'm not sure "officialise" is a word :) > > Taking a step back... "info via" and its new QMP counterpart > x-query-mos6522-devices dump device state. I understand why examining > device state via monitor can be useful for debugging. However, we have > more than 2000 devices in the tree. Clearly, we don't want 2000 device > state queries. Not even 100. Could we have more generic means instead? > > We could use QOM (read-only) properties to expose device state. > > If we use one QOM property per "thing", examining device state becomes > quite tedious. Also, you'd have to stop the guest to get a consistent > view, and adding lots of QOM properties bloats the code. > > If we use a single, object-valued property for the entire state, we get > to define the objects in QAPI. Differently tedious, and bloats the > generated code. > > We could use a single string-valued property. Too much of an abuse of > QOM? Yeah, I'd suggest we just keep it dumb and free form, adding a callback like this to the QOM base class: HumanReadableText (*debug_state)(Error **errp); With regards, Daniel
On 11/06/2024 07:58, Manos Pitsidianakis wrote: > On Tue, 11 Jun 2024 at 09:11, Mark Cave-Ayland > <mark.cave-ayland@ilande.co.uk> wrote: >> >> On 11/06/2024 06:49, Markus Armbruster wrote: >> >>> Philippe Mathieu-Daudé <philmd@linaro.org> writes: >>> >>>> Officialise the QMP command, use the existing >>>> hmp_info_human_readable_text() helper. >>> >>> I'm not sure "officialise" is a word :) >>> >>> Taking a step back... "info via" and its new QMP counterpart >>> x-query-mos6522-devices dump device state. I understand why examining >>> device state via monitor can be useful for debugging. However, we have >>> more than 2000 devices in the tree. Clearly, we don't want 2000 device >>> state queries. Not even 100. Could we have more generic means instead? >>> >>> We could use QOM (read-only) properties to expose device state. >>> >>> If we use one QOM property per "thing", examining device state becomes >>> quite tedious. Also, you'd have to stop the guest to get a consistent >>> view, and adding lots of QOM properties bloats the code. >>> >>> If we use a single, object-valued property for the entire state, we get >>> to define the objects in QAPI. Differently tedious, and bloats the >>> generated code. >>> >>> We could use a single string-valued property. Too much of an abuse of >>> QOM? >>> >>> We could add an optional "dump state for debugging" method to QOM, and >>> have a single query command that calls it if present. >>> >>> Thoughts? >> >> I agree that there should be a better way of doing things here. The aim of the >> original "info via" series was to allow the command to be contained completely within >> mos6522.c, but unfortunately due to the way that qemu-options.hx works then you end >> up with #ifdef-fery or stubs to make all configuration combinations work. >> >> As you point out ideally there should be a way for a QOM object to dynamically >> register its own monitor commands, which I think should help with this. >> >> IIRC in the original thread Daniel or David proposed a new "debug" monitor command >> such that a device could register its own debug <foo> commands either via DeviceClass >> or a function called during realize that would return a HumanReadableText via QMP. > > This is starting to sound like OOP: A Monitor interface defines > monitor commands, and QOM type classes can implement/define their own. > A Debug interface would do this. I like this idea: having a QOM interface that objects can implement to register their own monitor commands feels like a good solution. ATB, Mark.
On 11/06/2024 08:57, Daniel P. Berrangé wrote: > On Tue, Jun 11, 2024 at 07:49:12AM +0200, Markus Armbruster wrote: >> Philippe Mathieu-Daudé <philmd@linaro.org> writes: >> >>> Officialise the QMP command, use the existing >>> hmp_info_human_readable_text() helper. >> >> I'm not sure "officialise" is a word :) >> >> Taking a step back... "info via" and its new QMP counterpart >> x-query-mos6522-devices dump device state. I understand why examining >> device state via monitor can be useful for debugging. However, we have >> more than 2000 devices in the tree. Clearly, we don't want 2000 device >> state queries. Not even 100. Could we have more generic means instead? >> >> We could use QOM (read-only) properties to expose device state. >> >> If we use one QOM property per "thing", examining device state becomes >> quite tedious. Also, you'd have to stop the guest to get a consistent >> view, and adding lots of QOM properties bloats the code. >> >> If we use a single, object-valued property for the entire state, we get >> to define the objects in QAPI. Differently tedious, and bloats the >> generated code. >> >> We could use a single string-valued property. Too much of an abuse of >> QOM? > > Yeah, I'd suggest we just keep it dumb and free form, adding a > callback like this to the QOM base class: > > HumanReadableText (*debug_state)(Error **errp); I think that's a little bit too restrictive: certainly I can see the need for allowing parameters for the output to be customised, and there may also be cases where a device may want to register more than one debug (or monitor) command. Currently I quite like Manos' solution since it allows a MonitorInterface to be defined, and that could have separate methods for registering both "info" and "debug" commands. Longer term this could allow for e.g. TYPE_TCG_ACCEL to register "info jit" and devices can add whatever debug monitor commands they need. ATB, Mark.
On Tue, 11 Jun 2024 at 06:50, Markus Armbruster <armbru@redhat.com> wrote: > > Philippe Mathieu-Daudé <philmd@linaro.org> writes: > > > Officialise the QMP command, use the existing > > hmp_info_human_readable_text() helper. > > I'm not sure "officialise" is a word :) > > Taking a step back... "info via" and its new QMP counterpart > x-query-mos6522-devices dump device state. I understand why examining > device state via monitor can be useful for debugging. However, we have > more than 2000 devices in the tree. Clearly, we don't want 2000 device > state queries. Not even 100. Could we have more generic means instead? > > We could use QOM (read-only) properties to expose device state. > > If we use one QOM property per "thing", examining device state becomes > quite tedious. Also, you'd have to stop the guest to get a consistent > view, and adding lots of QOM properties bloats the code. > > If we use a single, object-valued property for the entire state, we get > to define the objects in QAPI. Differently tedious, and bloats the > generated code. We already have a machine readable mandatory-for-every-device representation of its entire state -- it's the vmstate struct. Admittedly this is sometimes a bit different from the guest-facing view of a device and we don't machine-record the field names... -- PMM
On Tue, Jun 11, 2024 at 09:13:00AM +0100, Mark Cave-Ayland wrote: > On 11/06/2024 08:57, Daniel P. Berrangé wrote: > > > On Tue, Jun 11, 2024 at 07:49:12AM +0200, Markus Armbruster wrote: > > > Philippe Mathieu-Daudé <philmd@linaro.org> writes: > > > > > > > Officialise the QMP command, use the existing > > > > hmp_info_human_readable_text() helper. > > > > > > I'm not sure "officialise" is a word :) > > > > > > Taking a step back... "info via" and its new QMP counterpart > > > x-query-mos6522-devices dump device state. I understand why examining > > > device state via monitor can be useful for debugging. However, we have > > > more than 2000 devices in the tree. Clearly, we don't want 2000 device > > > state queries. Not even 100. Could we have more generic means instead? > > > > > > We could use QOM (read-only) properties to expose device state. > > > > > > If we use one QOM property per "thing", examining device state becomes > > > quite tedious. Also, you'd have to stop the guest to get a consistent > > > view, and adding lots of QOM properties bloats the code. > > > > > > If we use a single, object-valued property for the entire state, we get > > > to define the objects in QAPI. Differently tedious, and bloats the > > > generated code. > > > > > > We could use a single string-valued property. Too much of an abuse of > > > QOM? > > > > Yeah, I'd suggest we just keep it dumb and free form, adding a > > callback like this to the QOM base class: > > > > HumanReadableText (*debug_state)(Error **errp); > > I think that's a little bit too restrictive: certainly I can see the need > for allowing parameters for the output to be customised, and there may also > be cases where a device may want to register more than one debug (or > monitor) command. > > Currently I quite like Manos' solution since it allows a MonitorInterface to > be defined, and that could have separate methods for registering both "info" > and "debug" commands. Longer term this could allow for e.g. TYPE_TCG_ACCEL > to register "info jit" and devices can add whatever debug monitor commands > they need. The downside is that this wires the monitor APIs into the internals of the devices, instead of having internals work exclusively in terms of QAPI types. With regards, Daniel
* Peter Maydell (peter.maydell@linaro.org) wrote: > On Tue, 11 Jun 2024 at 06:50, Markus Armbruster <armbru@redhat.com> wrote: > > > > Philippe Mathieu-Daudé <philmd@linaro.org> writes: > > > > > Officialise the QMP command, use the existing > > > hmp_info_human_readable_text() helper. > > > > I'm not sure "officialise" is a word :) > > > > Taking a step back... "info via" and its new QMP counterpart > > x-query-mos6522-devices dump device state. I understand why examining > > device state via monitor can be useful for debugging. However, we have > > more than 2000 devices in the tree. Clearly, we don't want 2000 device > > state queries. Not even 100. Could we have more generic means instead? > > > > We could use QOM (read-only) properties to expose device state. > > > > If we use one QOM property per "thing", examining device state becomes > > quite tedious. Also, you'd have to stop the guest to get a consistent > > view, and adding lots of QOM properties bloats the code. > > > > If we use a single, object-valued property for the entire state, we get > > to define the objects in QAPI. Differently tedious, and bloats the > > generated code. > > We already have a machine readable mandatory-for-every-device > representation of its entire state -- it's the vmstate struct. > Admittedly this is sometimes a bit different from the guest-facing > view of a device and we don't machine-record the field names... vmstate might not contain everything needed for debug; devices do a lot of fiddling to create a consistent vmstate, and there may be more that someone debugging wants (e.g. fd's or host memory addresses). It's also hopelessly cryptic. From an HMP point, a 'info debug <DEVICENAME>' seems good to me, possibly with some options as to whether to recurse or perhaps add flags to 'info qtree' to also call it. Dave > -- PMM