| Message ID | 20220127205405.23499-9-mark.cave-ayland@ilande.co.uk |
|---|---|
| State | New |
| Headers | show |
| Series | mos6522: switch to gpios, add control line edge-triggering and extra debugging | expand |
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 ? (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 > diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c > index aaae195d63..cfa6a9c44b 100644 > --- a/hw/misc/mos6522.c > +++ b/hw/misc/mos6522.c > @@ -30,6 +30,8 @@ > #include "hw/misc/mos6522.h" > #include "hw/qdev-properties.h" > #include "migration/vmstate.h" > +#include "monitor/monitor.h" > +#include "qapi/type-helpers.h" > #include "qemu/timer.h" > #include "qemu/cutils.h" > #include "qemu/log.h" > @@ -415,6 +417,95 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) > } > } > > +static int qmp_x_query_via_foreach(Object *obj, void *opaque) > +{ > + GString *buf = opaque; > + > + if (object_dynamic_cast(obj, TYPE_MOS6522)) { > + MOS6522State *s = MOS6522(obj); > + int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > + uint16_t t1counter = get_counter(s, &s->timers[0]); > + uint16_t t2counter = get_counter(s, &s->timers[1]); > + > + g_string_append_printf(buf, "%s:\n", object_get_typename(obj)); > + > + g_string_append_printf(buf, " Registers:\n"); > + g_string_append_printf(buf, " %-*s: 0x%x\n", 4, > + mos6522_reg_names[0], s->b); > + g_string_append_printf(buf, " %-*s: 0x%x\n", 4, > + mos6522_reg_names[1], s->a); > + g_string_append_printf(buf, " %-*s: 0x%x\n", 4, > + mos6522_reg_names[2], s->dirb); > + g_string_append_printf(buf, " %-*s: 0x%x\n", 4, > + mos6522_reg_names[3], s->dira); > + g_string_append_printf(buf, " %-*s: 0x%x\n", 4, > + mos6522_reg_names[4], t1counter & 0xff); > + g_string_append_printf(buf, " %-*s: 0x%x\n", 4, > + mos6522_reg_names[5], t1counter >> 8); > + g_string_append_printf(buf, " %-*s: 0x%x\n", 4, > + mos6522_reg_names[6], > + s->timers[0].latch & 0xff); > + g_string_append_printf(buf, " %-*s: 0x%x\n", 4, > + mos6522_reg_names[7], > + s->timers[0].latch >> 8); > + g_string_append_printf(buf, " %-*s: 0x%x\n", 4, > + mos6522_reg_names[8], t2counter & 0xff); > + g_string_append_printf(buf, " %-*s: 0x%x\n", 4, > + mos6522_reg_names[9], t2counter >> 8); > + g_string_append_printf(buf, " %-*s: 0x%x\n", 4, > + mos6522_reg_names[10], s->sr); > + g_string_append_printf(buf, " %-*s: 0x%x\n", 4, > + mos6522_reg_names[11], s->acr); > + g_string_append_printf(buf, " %-*s: 0x%x\n", 4, > + mos6522_reg_names[12], s->pcr); > + g_string_append_printf(buf, " %-*s: 0x%x\n", 4, > + mos6522_reg_names[13], s->ifr); > + g_string_append_printf(buf, " %-*s: 0x%x\n", 4, > + mos6522_reg_names[14], s->ier); > + > + g_string_append_printf(buf, " Timers:\n"); > + g_string_append_printf(buf, " Using current time now(ns)=%"PRId64 > + "\n", now); > + g_string_append_printf(buf, " T1 freq(hz)=%"PRId64 > + " mode=%s" > + " counter=0x%x" > + " latch=0x%x\n" > + " load_time(ns)=%"PRId64 > + " next_irq_time(ns)=%"PRId64 "\n", > + s->timers[0].frequency, > + ((s->acr & T1MODE) == T1MODE_CONT) ? "continuous" > + : "one-shot", > + t1counter, > + s->timers[0].latch, > + s->timers[0].load_time, > + get_next_irq_time(s, &s->timers[0], now)); > + g_string_append_printf(buf, " T2 freq(hz)=%"PRId64 > + " mode=%s" > + " counter=0x%x" > + " latch=0x%x\n" > + " load_time(ns)=%"PRId64 > + " next_irq_time(ns)=%"PRId64 "\n", > + s->timers[1].frequency, > + "one-shot", > + t2counter, > + s->timers[1].latch, > + s->timers[1].load_time, > + get_next_irq_time(s, &s->timers[1], now)); > + } > + > + return 0; > +} > + > +static HumanReadableText *qmp_x_query_via(Error **errp) > +{ > + g_autoptr(GString) buf = g_string_new(""); > + > + object_child_foreach_recursive(object_get_root(), > + qmp_x_query_via_foreach, buf); > + > + return human_readable_text_from_str(buf); > +} > + > static const MemoryRegionOps mos6522_ops = { > .read = mos6522_read, > .write = mos6522_write, > @@ -547,6 +638,7 @@ static const TypeInfo mos6522_type_info = { > static void mos6522_register_types(void) > { > type_register_static(&mos6522_type_info); > + monitor_register_hmp_info_hrt("via", qmp_x_query_via); > } > > type_init(mos6522_register_types) > -- > 2.20.1
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? [...]
* 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 > > [...] >
On Thu, Jan 27, 2022 at 08:54:02PM +0000, Mark Cave-Ayland 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(+) > > 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 > diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c > index aaae195d63..cfa6a9c44b 100644 > --- a/hw/misc/mos6522.c > +++ b/hw/misc/mos6522.c > @@ -30,6 +30,8 @@ > #include "hw/misc/mos6522.h" > #include "hw/qdev-properties.h" > #include "migration/vmstate.h" > +#include "monitor/monitor.h" > +#include "qapi/type-helpers.h" > #include "qemu/timer.h" > #include "qemu/cutils.h" > #include "qemu/log.h" > @@ -415,6 +417,95 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) > } > } > > +static int qmp_x_query_via_foreach(Object *obj, void *opaque) > + > +static HumanReadableText *qmp_x_query_via(Error **errp) > +{ > + g_autoptr(GString) buf = g_string_new(""); > + > + object_child_foreach_recursive(object_get_root(), > + qmp_x_query_via_foreach, buf); > + > + return human_readable_text_from_str(buf); > +} This provides a code handler for a QMP command which is good, but doesn't ever define the QMP command in the qapi/ schema. > static const MemoryRegionOps mos6522_ops = { > .read = mos6522_read, > .write = mos6522_write, > @@ -547,6 +638,7 @@ static const TypeInfo mos6522_type_info = { > static void mos6522_register_types(void) > { > type_register_static(&mos6522_type_info); > + monitor_register_hmp_info_hrt("via", qmp_x_query_via); This only registers the HMP counterpart. The idea of the HumanReadableText handler is that it is calling a QMP command that is exposed to apps. > } Regards, Daniel
On Tue, Feb 08, 2022 at 10:29:09AM +0000, 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. > > > > 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. Actually I think this would solve a problem with this contribution. This patch implements a QMP command but never registers it, so it isn't actually accessible via QMP. It only registers the HMP wrapper which rather defeats the point of doing it via the QMP HumanReadableText approach. I'm guessing this was done because we don't have ability to dynamically register QMP commands at runtime. I don't know how hard/easy it is to address this, and perhaps we don't even want to. It was a problem for me when previously converting HMP info commands to QMP and I didn't get a solution, so didn't convert anything where the command impl was dynamically registered at runtime. This basically excluded converting devices that have been split into loadable modules. If we had a general 'info dev-debug' (HMP) and 'x-query-dev-debug' commands, then we could side-step the QMP limitation, as both thue HMP and QMP comamnds could be statically registered. The devices then only need to register a callback at runtime which should be easier to deal with. Regards, Daniel
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.
On 08/02/2022 11:38, Daniel P. Berrangé wrote: > On Thu, Jan 27, 2022 at 08:54:02PM +0000, Mark Cave-Ayland 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(+) >> >> 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 >> diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c >> index aaae195d63..cfa6a9c44b 100644 >> --- a/hw/misc/mos6522.c >> +++ b/hw/misc/mos6522.c >> @@ -30,6 +30,8 @@ >> #include "hw/misc/mos6522.h" >> #include "hw/qdev-properties.h" >> #include "migration/vmstate.h" >> +#include "monitor/monitor.h" >> +#include "qapi/type-helpers.h" >> #include "qemu/timer.h" >> #include "qemu/cutils.h" >> #include "qemu/log.h" >> @@ -415,6 +417,95 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) >> } >> } >> >> +static int qmp_x_query_via_foreach(Object *obj, void *opaque) > > >> + >> +static HumanReadableText *qmp_x_query_via(Error **errp) >> +{ >> + g_autoptr(GString) buf = g_string_new(""); >> + >> + object_child_foreach_recursive(object_get_root(), >> + qmp_x_query_via_foreach, buf); >> + >> + return human_readable_text_from_str(buf); >> +} > > This provides a code handler for a QMP command which is good, > but doesn't ever define the QMP command in the qapi/ schema. First of all, thank you for writing the docs at https://www.qemu.org/docs/master/devel/writing-monitor-commands.html#writing-a-debugging-aid-returning-unstructured-text which were really useful when writing this patch. I was under the impression that monitor_register_hmp_info_hrt() does all the magic here i.e. it declares the underlying QMP command with an x- prefix and effectively encapsulates the text field in a way that says "this is an unreliable text opaque for humans"? If a qapi/ schema is needed could you explain what it should look like for this example and where it should go? Looking at the existing .json files I can't immediately see one which is the right place for this to live. >> static const MemoryRegionOps mos6522_ops = { >> .read = mos6522_read, >> .write = mos6522_write, >> @@ -547,6 +638,7 @@ static const TypeInfo mos6522_type_info = { >> static void mos6522_register_types(void) >> { >> type_register_static(&mos6522_type_info); >> + monitor_register_hmp_info_hrt("via", qmp_x_query_via); > > This only registers the HMP counterpart. > > The idea of the HumanReadableText handler is that it is calling > a QMP command that is exposed to apps. > >> } > > Regards, > Daniel ATB, Mark.
On 08/02/2022 11:52, Daniel P. Berrangé wrote: (cut) >>> 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. > > Actually I think this would solve a problem with this contribution. > This patch implements a QMP command but never registers it, so it > isn't actually accessible via QMP. It only registers the HMP wrapper > which rather defeats the point of doing it via the QMP HumanReadableText > approach. > > I'm guessing this was done because we don't have ability to dynamically > register QMP commands at runtime. I don't know how hard/easy it is > to address this, and perhaps we don't even want to. It was a problem > for me when previously converting HMP info commands to QMP and I > didn't get a solution, so didn't convert anything where the command > impl was dynamically registered at runtime. This basically excluded > converting devices that have been split into loadable modules. > > If we had a general 'info dev-debug' (HMP) and 'x-query-dev-debug' > commands, then we could side-step the QMP limitation, as both thue > HMP and QMP comamnds could be statically registered. The devices > then only need to register a callback at runtime which should be > easier to deal with. That could work, or even just a "debug via" without using "info" for brevity. You could even add the callback to DeviceClass so that the registration takes place as part of class_init() using a function such as device_class_register_debug("name", callback). ATB, Mark.
On Tue, Feb 08, 2022 at 12:39:04PM +0000, Mark Cave-Ayland wrote: > On 08/02/2022 11:38, Daniel P. Berrangé wrote: > > > On Thu, Jan 27, 2022 at 08:54:02PM +0000, Mark Cave-Ayland wrote: > > > This displays detailed information about the device registers and timers o 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(+) > > > > > > 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 > > > diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c > > > index aaae195d63..cfa6a9c44b 100644 > > > --- a/hw/misc/mos6522.c > > > +++ b/hw/misc/mos6522.c > > > @@ -30,6 +30,8 @@ > > > #include "hw/misc/mos6522.h" > > > #include "hw/qdev-properties.h" > > > #include "migration/vmstate.h" > > > +#include "monitor/monitor.h" > > > +#include "qapi/type-helpers.h" > > > #include "qemu/timer.h" > > > #include "qemu/cutils.h" > > > #include "qemu/log.h" > > > @@ -415,6 +417,95 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) > > > } > > > } > > > +static int qmp_x_query_via_foreach(Object *obj, void *opaque) > > > > > > > + > > > +static HumanReadableText *qmp_x_query_via(Error **errp) > > > +{ > > > + g_autoptr(GString) buf = g_string_new(""); > > > + > > > + object_child_foreach_recursive(object_get_root(), > > > + qmp_x_query_via_foreach, buf); > > > + > > > + return human_readable_text_from_str(buf); > > > +} > > > > This provides a code handler for a QMP command which is good, > > but doesn't ever define the QMP command in the qapi/ schema. > > First of all, thank you for writing the docs at https://www.qemu.org/docs/master/devel/writing-monitor-commands.html#writing-a-debugging-aid-returning-unstructured-text > which were really useful when writing this patch. > > I was under the impression that monitor_register_hmp_info_hrt() does all the > magic here i.e. it declares the underlying QMP command with an x- prefix and > effectively encapsulates the text field in a way that says "this is an > unreliable text opaque for humans"? The monitor_register_hmp_info_hrt only does the HMP glue side, and that's only needed if you must dynamically register the HMP command. For statically registered commands set '.cmd_info_hrt' directly in the hml-commands-info.hx for the HMP side. > If a qapi/ schema is needed could you explain what it should look like for > this example and where it should go? Looking at the existing .json files I > can't immediately see one which is the right place for this to live. Take a look in qapi/machine.json for anyof the 'x-query-XXXX' commands there. The QAPI bit is fairly simple. if you want to see an illustration of what's different from a previous pure HMP impl, look at: commit dd98234c059e6bdb05a52998270df6d3d990332e Author: Daniel P. Berrangé <berrange@redhat.com> Date: Wed Sep 8 10:35:43 2021 +0100 qapi: introduce x-query-roms QMP command Regards, Daniel
* Mark Cave-Ayland (mark.cave-ayland@ilande.co.uk) wrote: > On 08/02/2022 11:52, Daniel P. Berrangé wrote: > > (cut) > > > > > 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. > > > > Actually I think this would solve a problem with this contribution. > > This patch implements a QMP command but never registers it, so it > > isn't actually accessible via QMP. It only registers the HMP wrapper > > which rather defeats the point of doing it via the QMP HumanReadableText > > approach. > > > > I'm guessing this was done because we don't have ability to dynamically > > register QMP commands at runtime. I don't know how hard/easy it is > > to address this, and perhaps we don't even want to. It was a problem > > for me when previously converting HMP info commands to QMP and I > > didn't get a solution, so didn't convert anything where the command > > impl was dynamically registered at runtime. This basically excluded > > converting devices that have been split into loadable modules. > > > > If we had a general 'info dev-debug' (HMP) and 'x-query-dev-debug' > > commands, then we could side-step the QMP limitation, as both thue > > HMP and QMP comamnds could be statically registered. The devices > > then only need to register a callback at runtime which should be > > easier to deal with. > > That could work, or even just a "debug via" without using "info" for brevity. > > You could even add the callback to DeviceClass so that the registration > takes place as part of class_init() using a function such as > device_class_register_debug("name", callback). Yes, that was what I was imagining Dave > > ATB, > > Mark. >
* Mark Cave-Ayland (mark.cave-ayland@ilande.co.uk) wrote: > 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? I'd be happy to take it down to the target level. Dave > > > > 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. >
On 08/02/2022 12:49, Daniel P. Berrangé wrote: >> I was under the impression that monitor_register_hmp_info_hrt() does all the >> magic here i.e. it declares the underlying QMP command with an x- prefix and >> effectively encapsulates the text field in a way that says "this is an >> unreliable text opaque for humans"? > > The monitor_register_hmp_info_hrt only does the HMP glue side, and > that's only needed if you must dynamically register the HMP command. > For statically registered commands set '.cmd_info_hrt' directly in > the hml-commands-info.hx for the HMP side. > >> If a qapi/ schema is needed could you explain what it should look like for >> this example and where it should go? Looking at the existing .json files I >> can't immediately see one which is the right place for this to live. > > Take a look in qapi/machine.json for anyof the 'x-query-XXXX' commands > there. The QAPI bit is fairly simple. > > if you want to see an illustration of what's different from a previous > pure HMP impl, look at: > > commit dd98234c059e6bdb05a52998270df6d3d990332e > Author: Daniel P. Berrangé <berrange@redhat.com> > Date: Wed Sep 8 10:35:43 2021 +0100 > > qapi: introduce x-query-roms QMP command I see, thanks for the reference. So qapi/machine.json would be the right place to declare the QMP part even for a specific device? Even this approach still wouldn't work in its current form though, since as mentioned in my previous email it seems that only the target CONFIG_* defines and not the device CONFIG_* defines are present when processing hmp-commands-info.hx. ATB, Mark.
On Tue, Feb 08, 2022 at 01:06:59PM +0000, Mark Cave-Ayland wrote: > On 08/02/2022 12:49, Daniel P. Berrangé wrote: > > > > I was under the impression that monitor_register_hmp_info_hrt() does all the > > > magic here i.e. it declares the underlying QMP command with an x- prefix and > > > effectively encapsulates the text field in a way that says "this is an > > > unreliable text opaque for humans"? > > > > The monitor_register_hmp_info_hrt only does the HMP glue side, and > > that's only needed if you must dynamically register the HMP command. > > For statically registered commands set '.cmd_info_hrt' directly in > > the hml-commands-info.hx for the HMP side. > > > > > If a qapi/ schema is needed could you explain what it should look like for > > > this example and where it should go? Looking at the existing .json files I > > > can't immediately see one which is the right place for this to live. > > > > Take a look in qapi/machine.json for anyof the 'x-query-XXXX' commands > > there. The QAPI bit is fairly simple. > > > > if you want to see an illustration of what's different from a previous > > pure HMP impl, look at: > > > > commit dd98234c059e6bdb05a52998270df6d3d990332e > > Author: Daniel P. Berrangé <berrange@redhat.com> > > Date: Wed Sep 8 10:35:43 2021 +0100 > > > > qapi: introduce x-query-roms QMP command > > I see, thanks for the reference. So qapi/machine.json would be the right > place to declare the QMP part even for a specific device? > > Even this approach still wouldn't work in its current form though, since as > mentioned in my previous email it seems that only the target CONFIG_* > defines and not the device CONFIG_* defines are present when processing > hmp-commands-info.hx. Yeah, that's where the pain comes in. While QAPI schema can be made conditional on a few CONFIG_* parameters - basically those derived from global configure time options, it is impossible for this to be with with target specific options like the device CONFIG_* defines. This is why I suggested in my othuer reply that it would need to be done with a generic 'info dev-debug' / 'x-query-dev-debug' command that can be registered unconditionally, and then individual devices plug into it. Regards, Daniel
Daniel P. Berrangé <berrange@redhat.com> writes: > On Tue, Feb 08, 2022 at 10:29:09AM +0000, 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. >> >> > > 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. > > Actually I think this would solve a problem with this contribution. > This patch implements a QMP command but never registers it, so it > isn't actually accessible via QMP. It only registers the HMP wrapper > which rather defeats the point of doing it via the QMP HumanReadableText > approach. > > I'm guessing this was done because we don't have ability to dynamically > register QMP commands at runtime. Correct. I pointed this out in review, actually: Message-ID: <87tuk1k0de.fsf@dusky.pond.sub.org> https://lists.gnu.org/archive/html/qemu-devel/2021-08/msg01236.html Gerd replied "when going forward with building more code modular we might need this for qmp too at some point". > I don't know how hard/easy it is > to address this, and perhaps we don't even want to. Depends on requirements. Say all we want is "command fails unless the module is loaded" (or whatever the condition is). Should be fairly easy. But if we want introspection to reflect the state of things, that's harder. > It was a problem > for me when previously converting HMP info commands to QMP and I > didn't get a solution, so didn't convert anything where the command > impl was dynamically registered at runtime. This basically excluded > converting devices that have been split into loadable modules. Understand; you series was big enough as it was. > If we had a general 'info dev-debug' (HMP) and 'x-query-dev-debug' > commands, then we could side-step the QMP limitation, as both thue > HMP and QMP comamnds could be statically registered. The devices > then only need to register a callback at runtime which should be > easier to deal with. I like it.
On 08/02/2022 13:10, Daniel P. Berrangé wrote: > On Tue, Feb 08, 2022 at 01:06:59PM +0000, Mark Cave-Ayland wrote: >> On 08/02/2022 12:49, Daniel P. Berrangé wrote: >> >>>> I was under the impression that monitor_register_hmp_info_hrt() does all the >>>> magic here i.e. it declares the underlying QMP command with an x- prefix and >>>> effectively encapsulates the text field in a way that says "this is an >>>> unreliable text opaque for humans"? >>> >>> The monitor_register_hmp_info_hrt only does the HMP glue side, and >>> that's only needed if you must dynamically register the HMP command. >>> For statically registered commands set '.cmd_info_hrt' directly in >>> the hml-commands-info.hx for the HMP side. >>> >>>> If a qapi/ schema is needed could you explain what it should look like for >>>> this example and where it should go? Looking at the existing .json files I >>>> can't immediately see one which is the right place for this to live. >>> >>> Take a look in qapi/machine.json for anyof the 'x-query-XXXX' commands >>> there. The QAPI bit is fairly simple. >>> >>> if you want to see an illustration of what's different from a previous >>> pure HMP impl, look at: >>> >>> commit dd98234c059e6bdb05a52998270df6d3d990332e >>> Author: Daniel P. Berrangé <berrange@redhat.com> >>> Date: Wed Sep 8 10:35:43 2021 +0100 >>> >>> qapi: introduce x-query-roms QMP command >> >> I see, thanks for the reference. So qapi/machine.json would be the right >> place to declare the QMP part even for a specific device? >> >> Even this approach still wouldn't work in its current form though, since as >> mentioned in my previous email it seems that only the target CONFIG_* >> defines and not the device CONFIG_* defines are present when processing >> hmp-commands-info.hx. > > Yeah, that's where the pain comes in. While QAPI schema can be made > conditional on a few CONFIG_* parameters - basically those derived > from global configure time options, it is impossible for this to be > with with target specific options like the device CONFIG_* defines. > > This is why I suggested in my othuer reply that it would need to be > done with a generic 'info dev-debug' / 'x-query-dev-debug' command > that can be registered unconditionally, and then individual devices > plug into it. After some more experiments this afternoon I still seem to be falling through the gaps on this one. This is based upon my understanding that all new HMP commands should use a QMP HumanReadableText implementation and the new command should be restricted according to target. Currently I am working with this change to hmp-commands-info.hx and qapi/misc-target.json: diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx index 7e6bd30395..aac86d5473 100644 --- a/hmp-commands-info.hx +++ b/hmp-commands-info.hx @@ -880,15 +880,15 @@ SRST Show intel SGX information. ERST #if defined(TARGET_M68K) || defined(TARGET_PPC) { .name = "via", .args_type = "", .params = "", .help = "show guest mos6522 VIA devices", .cmd_info_hrt = qmp_x_query_via, }, diff --git a/qapi/misc-target.json b/qapi/misc-target.json index 4bc45d2474..72bf71888e 100644 --- a/qapi/misc-target.json +++ b/qapi/misc-target.json @@ -2,6 +2,8 @@ # vim: filetype=python # +{ 'include': 'common.json' } + ## # @RTC_CHANGE: # @@ -424,3 +426,19 @@ # ## { 'command': 'query-sgx-capabilities', 'returns': 'SGXInfo', 'if': 'TARGET_I386' } + +## +# @x-query-via: +# +# Query information on the registered mos6522 VIAs +# +# Features: +# @unstable: This command is meant for debugging. +# +# Returns: internal mos6522 information +# +# Since: 7.0 +## +{ 'command': 'x-query-via', + 'returns': 'HumanReadableText', + 'features': [ 'unstable' ], 'if': { 'any': [ 'TARGET_M68K', 'TARGET_PPC' ] } } The main problem with trying to restrict the new command to a target (or targets) is that the autogenerated qapi/qapi-commands-misc-target.h QAPI header cannot be included in a device header such as mos6522.h without getting poison errors like below (which does actually make sense): In file included from ./qapi/qapi-commands-misc-target.h:17, from /home/build/src/qemu/git/qemu/include/hw/misc/mos6522.h:35, from ../hw/misc/mos6522.c:30: ./qapi/qapi-types-misc-target.h:19:13: error: attempt to use poisoned "TARGET_ALPHA" I can work around that by declaring the prototype for qmp_x_query_via() manually i.e.: diff --git a/include/hw/misc/mos6522.h b/include/hw/misc/mos6522.h index 9c21da2ddd..9677293ad0 100644 --- a/include/hw/misc/mos6522.h +++ b/include/hw/misc/mos6522.h @@ -30,7 +30,7 @@ #include "exec/memory.h" #include "hw/sysbus.h" #include "hw/input/adb.h" +#include "qapi/qapi-commands-common.h" #include "qom/object.h" /* Bits in ACR */ @@ -156,4 +156,6 @@ extern const VMStateDescription vmstate_mos6522; uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned size); void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size); +HumanReadableText *qmp_x_query_via(Error **errp); + #endif /* MOS6522_H */ This works fine for targets where CONFIG_MOS6522 is defined, but if I try a target such as x86_64 where the device isn't used then I hit another compilation error: qapi/qapi-commands-misc-target.c:598:13: error: ‘qmp_marshal_output_HumanReadableText’ defined but not used [-Werror=unused-function] static void qmp_marshal_output_HumanReadableText(HumanReadableText *ret_in, ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ cc1: all warnings being treated as errors Looking at the generated qapi/qapi-commands-misc-target.c file in question I see this: static void qmp_marshal_output_HumanReadableText(HumanReadableText *ret_in, QObject **ret_out, Error **errp) { Visitor *v; v = qobject_output_visitor_new_qmp(ret_out); if (visit_type_HumanReadableText(v, "unused", &ret_in, errp)) { visit_complete(v, ret_out); } visit_free(v); v = qapi_dealloc_visitor_new(); visit_type_HumanReadableText(v, "unused", &ret_in, NULL); visit_free(v); } #if defined(TARGET_M68K) || defined(TARGET_PPC) void qmp_marshal_x_query_via(QDict *args, QObject **ret, Error **errp) { ... ... } #endif i.e. qmp_marshal_output_HumanReadableText() isn't protected by the #if TARGET guards and since HumanReadableText is only used by the new qmp_x_query_via() functionality then the compiler complains and aborts the compilation. Possibly this is an error in the QAPI generator for types hidden behind commands using "if"? Otherwise I'm not sure what is the best way to proceed, so I'd be grateful for some further pointers. ATB, Mark.
+Thomas On 20/2/22 18:18, Mark Cave-Ayland wrote: > On 08/02/2022 13:10, Daniel P. Berrangé wrote: > >> On Tue, Feb 08, 2022 at 01:06:59PM +0000, Mark Cave-Ayland wrote: >>> On 08/02/2022 12:49, Daniel P. Berrangé wrote: >>> >>>>> I was under the impression that monitor_register_hmp_info_hrt() >>>>> does all the >>>>> magic here i.e. it declares the underlying QMP command with an x- >>>>> prefix and >>>>> effectively encapsulates the text field in a way that says "this is an >>>>> unreliable text opaque for humans"? >>>> >>>> The monitor_register_hmp_info_hrt only does the HMP glue side, and >>>> that's only needed if you must dynamically register the HMP command. >>>> For statically registered commands set '.cmd_info_hrt' directly in >>>> the hml-commands-info.hx for the HMP side. >>>> >>>>> If a qapi/ schema is needed could you explain what it should look >>>>> like for >>>>> this example and where it should go? Looking at the existing .json >>>>> files I >>>>> can't immediately see one which is the right place for this to live. >>>> >>>> Take a look in qapi/machine.json for anyof the 'x-query-XXXX' commands >>>> there. The QAPI bit is fairly simple. >>>> >>>> if you want to see an illustration of what's different from a previous >>>> pure HMP impl, look at: >>>> >>>> commit dd98234c059e6bdb05a52998270df6d3d990332e >>>> Author: Daniel P. Berrangé <berrange@redhat.com> >>>> Date: Wed Sep 8 10:35:43 2021 +0100 >>>> >>>> qapi: introduce x-query-roms QMP command >>> >>> I see, thanks for the reference. So qapi/machine.json would be the right >>> place to declare the QMP part even for a specific device? >>> >>> Even this approach still wouldn't work in its current form though, >>> since as >>> mentioned in my previous email it seems that only the target CONFIG_* >>> defines and not the device CONFIG_* defines are present when processing >>> hmp-commands-info.hx. >> >> Yeah, that's where the pain comes in. While QAPI schema can be made >> conditional on a few CONFIG_* parameters - basically those derived >> from global configure time options, it is impossible for this to be >> with with target specific options like the device CONFIG_* defines. >> >> This is why I suggested in my othuer reply that it would need to be >> done with a generic 'info dev-debug' / 'x-query-dev-debug' command >> that can be registered unconditionally, and then individual devices >> plug into it. > > After some more experiments this afternoon I still seem to be falling > through the gaps on this one. This is based upon my understanding that > all new HMP commands should use a QMP HumanReadableText implementation > and the new command should be restricted according to target. > > Currently I am working with this change to hmp-commands-info.hx and > qapi/misc-target.json: > > > diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx > index 7e6bd30395..aac86d5473 100644 > --- a/hmp-commands-info.hx > +++ b/hmp-commands-info.hx > @@ -880,15 +880,15 @@ SRST > Show intel SGX information. > ERST > > #if defined(TARGET_M68K) || defined(TARGET_PPC) > { > .name = "via", > .args_type = "", > .params = "", > .help = "show guest mos6522 VIA devices", > .cmd_info_hrt = qmp_x_query_via, > }, > diff --git a/qapi/misc-target.json b/qapi/misc-target.json > index 4bc45d2474..72bf71888e 100644 > --- a/qapi/misc-target.json > +++ b/qapi/misc-target.json > @@ -2,6 +2,8 @@ > # vim: filetype=python > # > > +{ 'include': 'common.json' } > + > ## > # @RTC_CHANGE: > # > @@ -424,3 +426,19 @@ > # > ## > { 'command': 'query-sgx-capabilities', 'returns': 'SGXInfo', 'if': > 'TARGET_I386' } > + > +## > +# @x-query-via: > +# > +# Query information on the registered mos6522 VIAs > +# > +# Features: > +# @unstable: This command is meant for debugging. > +# > +# Returns: internal mos6522 information > +# > +# Since: 7.0 > +## > +{ 'command': 'x-query-via', > + 'returns': 'HumanReadableText', > + 'features': [ 'unstable' ], 'if': { 'any': [ 'TARGET_M68K', > 'TARGET_PPC' ] } } > > > The main problem with trying to restrict the new command to a target (or > targets) is that the autogenerated qapi/qapi-commands-misc-target.h QAPI > header cannot be included in a device header such as mos6522.h without > getting poison errors like below (which does actually make sense): > > > In file included from ./qapi/qapi-commands-misc-target.h:17, > from > /home/build/src/qemu/git/qemu/include/hw/misc/mos6522.h:35, > from ../hw/misc/mos6522.c:30: > ./qapi/qapi-types-misc-target.h:19:13: error: attempt to use poisoned > "TARGET_ALPHA" Can be kludged by making this device target-specific: -- >8 -- diff --git a/hw/misc/meson.build b/hw/misc/meson.build index 6dcbe044f3..65837b1dfe 100644 --- a/hw/misc/meson.build +++ b/hw/misc/meson.build @@ -23 +23 @@ softmmu_ss.add(when: 'CONFIG_ARM_V7M', if_true: files('armv7m_ras.c')) -softmmu_ss.add(when: 'CONFIG_MOS6522', if_true: files('mos6522.c')) +specific_ss.add(when: 'CONFIG_MOS6522', if_true: files('mos6522.c')) --- I'd rather keep devices generic, but sometime we can't. In this case VIA is only used on m68k so it could be acceptable. > I can work around that by declaring the prototype for qmp_x_query_via() > manually i.e.: > > > diff --git a/include/hw/misc/mos6522.h b/include/hw/misc/mos6522.h > index 9c21da2ddd..9677293ad0 100644 > --- a/include/hw/misc/mos6522.h > +++ b/include/hw/misc/mos6522.h > @@ -30,7 +30,7 @@ > #include "exec/memory.h" > #include "hw/sysbus.h" > #include "hw/input/adb.h" > +#include "qapi/qapi-commands-common.h" > #include "qom/object.h" > > /* Bits in ACR */ > @@ -156,4 +156,6 @@ extern const VMStateDescription vmstate_mos6522; > uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned size); > void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned > size); > > +HumanReadableText *qmp_x_query_via(Error **errp); > + > #endif /* MOS6522_H */ > > > This works fine for targets where CONFIG_MOS6522 is defined, but if I > try a target such as x86_64 where the device isn't used then I hit > another compilation error: > > > qapi/qapi-commands-misc-target.c:598:13: error: > ‘qmp_marshal_output_HumanReadableText’ defined but not used > [-Werror=unused-function] > static void qmp_marshal_output_HumanReadableText(HumanReadableText > *ret_in, > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > cc1: all warnings being treated as errors > > > Looking at the generated qapi/qapi-commands-misc-target.c file in > question I see this: > > > static void qmp_marshal_output_HumanReadableText(HumanReadableText *ret_in, > QObject **ret_out, Error **errp) > { > Visitor *v; > > v = qobject_output_visitor_new_qmp(ret_out); > if (visit_type_HumanReadableText(v, "unused", &ret_in, errp)) { > visit_complete(v, ret_out); > } > visit_free(v); > v = qapi_dealloc_visitor_new(); > visit_type_HumanReadableText(v, "unused", &ret_in, NULL); > visit_free(v); > } > > #if defined(TARGET_M68K) || defined(TARGET_PPC) > void qmp_marshal_x_query_via(QDict *args, QObject **ret, Error **errp) > { > ... > ... > } > #endif > > i.e. qmp_marshal_output_HumanReadableText() isn't protected by the #if > TARGET guards and since HumanReadableText is only used by the new > qmp_x_query_via() functionality then the compiler complains and aborts > the compilation. > > Possibly this is an error in the QAPI generator for types hidden behind > commands using "if"? Otherwise I'm not sure what is the best way to > proceed, so I'd be grateful for some further pointers. > > > ATB, > > Mark. >
On Sun, Feb 20, 2022 at 05:18:33PM +0000, Mark Cave-Ayland wrote: > On 08/02/2022 13:10, Daniel P. Berrangé wrote: > > > On Tue, Feb 08, 2022 at 01:06:59PM +0000, Mark Cave-Ayland wrote: > > > On 08/02/2022 12:49, Daniel P. Berrangé wrote: > > > > > > > > I was under the impression that monitor_register_hmp_info_hrt() does all the > > > > > magic here i.e. it declares the underlying QMP command with an x- prefix and > > > > > effectively encapsulates the text field in a way that says "this is an > > > > > unreliable text opaque for humans"? > > > > > > > > The monitor_register_hmp_info_hrt only does the HMP glue side, and > > > > that's only needed if you must dynamically register the HMP command. > > > > For statically registered commands set '.cmd_info_hrt' directly in > > > > the hml-commands-info.hx for the HMP side. > > > > > > > > > If a qapi/ schema is needed could you explain what it should look like for > > > > > this example and where it should go? Looking at the existing .json files I > > > > > can't immediately see one which is the right place for this to live. > > > > > > > > Take a look in qapi/machine.json for anyof the 'x-query-XXXX' commands > > > > there. The QAPI bit is fairly simple. > > > > > > > > if you want to see an illustration of what's different from a previous > > > > pure HMP impl, look at: > > > > > > > > commit dd98234c059e6bdb05a52998270df6d3d990332e > > > > Author: Daniel P. Berrangé <berrange@redhat.com> > > > > Date: Wed Sep 8 10:35:43 2021 +0100 > > > > > > > > qapi: introduce x-query-roms QMP command > > > > > > I see, thanks for the reference. So qapi/machine.json would be the right > > > place to declare the QMP part even for a specific device? > > > > > > Even this approach still wouldn't work in its current form though, since as > > > mentioned in my previous email it seems that only the target CONFIG_* > > > defines and not the device CONFIG_* defines are present when processing > > > hmp-commands-info.hx. > > > > Yeah, that's where the pain comes in. While QAPI schema can be made > > conditional on a few CONFIG_* parameters - basically those derived > > from global configure time options, it is impossible for this to be > > with with target specific options like the device CONFIG_* defines. > > > > This is why I suggested in my othuer reply that it would need to be > > done with a generic 'info dev-debug' / 'x-query-dev-debug' command > > that can be registered unconditionally, and then individual devices > > plug into it. > > After some more experiments this afternoon I still seem to be falling > through the gaps on this one. This is based upon my understanding that all > new HMP commands should use a QMP HumanReadableText implementation and the > new command should be restricted according to target. > > Currently I am working with this change to hmp-commands-info.hx and > qapi/misc-target.json: [snip] > i.e. qmp_marshal_output_HumanReadableText() isn't protected by the #if > TARGET guards and since HumanReadableText is only used by the new > qmp_x_query_via() functionality then the compiler complains and aborts the > compilation. > > Possibly this is an error in the QAPI generator for types hidden behind > commands using "if"? Otherwise I'm not sure what is the best way to proceed, > so I'd be grateful for some further pointers. Yes, this is pretty much what I expect and exactly what I hit with other target specific commands. That's why I suggested something like a general 'x-device-debug' command that is NOT conditionalized in QAPI, against which dev impls can register a callback to provide detailed reporting, instead of a device type specific command. Regards, Daniel
On 21/02/2022 12:20, Philippe Mathieu-Daudé wrote: > +Thomas > > On 20/2/22 18:18, Mark Cave-Ayland wrote: >> On 08/02/2022 13:10, Daniel P. Berrangé wrote: >> >>> On Tue, Feb 08, 2022 at 01:06:59PM +0000, Mark Cave-Ayland wrote: >>>> On 08/02/2022 12:49, Daniel P. Berrangé wrote: >>>> >>>>>> I was under the impression that monitor_register_hmp_info_hrt() does all the >>>>>> magic here i.e. it declares the underlying QMP command with an x- prefix and >>>>>> effectively encapsulates the text field in a way that says "this is an >>>>>> unreliable text opaque for humans"? >>>>> >>>>> The monitor_register_hmp_info_hrt only does the HMP glue side, and >>>>> that's only needed if you must dynamically register the HMP command. >>>>> For statically registered commands set '.cmd_info_hrt' directly in >>>>> the hml-commands-info.hx for the HMP side. >>>>> >>>>>> If a qapi/ schema is needed could you explain what it should look like for >>>>>> this example and where it should go? Looking at the existing .json files I >>>>>> can't immediately see one which is the right place for this to live. >>>>> >>>>> Take a look in qapi/machine.json for anyof the 'x-query-XXXX' commands >>>>> there. The QAPI bit is fairly simple. >>>>> >>>>> if you want to see an illustration of what's different from a previous >>>>> pure HMP impl, look at: >>>>> >>>>> commit dd98234c059e6bdb05a52998270df6d3d990332e >>>>> Author: Daniel P. Berrangé <berrange@redhat.com> >>>>> Date: Wed Sep 8 10:35:43 2021 +0100 >>>>> >>>>> qapi: introduce x-query-roms QMP command >>>> >>>> I see, thanks for the reference. So qapi/machine.json would be the right >>>> place to declare the QMP part even for a specific device? >>>> >>>> Even this approach still wouldn't work in its current form though, since as >>>> mentioned in my previous email it seems that only the target CONFIG_* >>>> defines and not the device CONFIG_* defines are present when processing >>>> hmp-commands-info.hx. >>> >>> Yeah, that's where the pain comes in. While QAPI schema can be made >>> conditional on a few CONFIG_* parameters - basically those derived >>> from global configure time options, it is impossible for this to be >>> with with target specific options like the device CONFIG_* defines. >>> >>> This is why I suggested in my othuer reply that it would need to be >>> done with a generic 'info dev-debug' / 'x-query-dev-debug' command >>> that can be registered unconditionally, and then individual devices >>> plug into it. >> >> After some more experiments this afternoon I still seem to be falling through the >> gaps on this one. This is based upon my understanding that all new HMP commands >> should use a QMP HumanReadableText implementation and the new command should be >> restricted according to target. >> >> Currently I am working with this change to hmp-commands-info.hx and >> qapi/misc-target.json: >> >> >> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx >> index 7e6bd30395..aac86d5473 100644 >> --- a/hmp-commands-info.hx >> +++ b/hmp-commands-info.hx >> @@ -880,15 +880,15 @@ SRST >> Show intel SGX information. >> ERST >> >> #if defined(TARGET_M68K) || defined(TARGET_PPC) >> { >> .name = "via", >> .args_type = "", >> .params = "", >> .help = "show guest mos6522 VIA devices", >> .cmd_info_hrt = qmp_x_query_via, >> }, >> diff --git a/qapi/misc-target.json b/qapi/misc-target.json >> index 4bc45d2474..72bf71888e 100644 >> --- a/qapi/misc-target.json >> +++ b/qapi/misc-target.json >> @@ -2,6 +2,8 @@ >> # vim: filetype=python >> # >> >> +{ 'include': 'common.json' } >> + >> ## >> # @RTC_CHANGE: >> # >> @@ -424,3 +426,19 @@ >> # >> ## >> { 'command': 'query-sgx-capabilities', 'returns': 'SGXInfo', 'if': 'TARGET_I386' } >> + >> +## >> +# @x-query-via: >> +# >> +# Query information on the registered mos6522 VIAs >> +# >> +# Features: >> +# @unstable: This command is meant for debugging. >> +# >> +# Returns: internal mos6522 information >> +# >> +# Since: 7.0 >> +## >> +{ 'command': 'x-query-via', >> + 'returns': 'HumanReadableText', >> + 'features': [ 'unstable' ], 'if': { 'any': [ 'TARGET_M68K', 'TARGET_PPC' ] } } >> >> >> The main problem with trying to restrict the new command to a target (or targets) >> is that the autogenerated qapi/qapi-commands-misc-target.h QAPI header cannot be >> included in a device header such as mos6522.h without getting poison errors like >> below (which does actually make sense): >> >> >> In file included from ./qapi/qapi-commands-misc-target.h:17, >> from /home/build/src/qemu/git/qemu/include/hw/misc/mos6522.h:35, >> from ../hw/misc/mos6522.c:30: >> ./qapi/qapi-types-misc-target.h:19:13: error: attempt to use poisoned "TARGET_ALPHA" > > Can be kludged by making this device target-specific: > > -- >8 -- > diff --git a/hw/misc/meson.build b/hw/misc/meson.build > index 6dcbe044f3..65837b1dfe 100644 > --- a/hw/misc/meson.build > +++ b/hw/misc/meson.build > @@ -23 +23 @@ softmmu_ss.add(when: 'CONFIG_ARM_V7M', if_true: files('armv7m_ras.c')) > -softmmu_ss.add(when: 'CONFIG_MOS6522', if_true: files('mos6522.c')) > +specific_ss.add(when: 'CONFIG_MOS6522', if_true: files('mos6522.c')) > --- > > I'd rather keep devices generic, but sometime we can't. In this case > VIA is only used on m68k so it could be acceptable. That's not quite true though, it's also used for the Mac machines. From what I can see everyone agrees that there needs to be a better way to do this, but it requires someone with time and interest to implement the suggested changes. ATB, Mark.
On 21/02/2022 17:11, Daniel P. Berrangé wrote: > On Sun, Feb 20, 2022 at 05:18:33PM +0000, Mark Cave-Ayland wrote: >> On 08/02/2022 13:10, Daniel P. Berrangé wrote: >> >>> On Tue, Feb 08, 2022 at 01:06:59PM +0000, Mark Cave-Ayland wrote: >>>> On 08/02/2022 12:49, Daniel P. Berrangé wrote: >>>> >>>>>> I was under the impression that monitor_register_hmp_info_hrt() does all the >>>>>> magic here i.e. it declares the underlying QMP command with an x- prefix and >>>>>> effectively encapsulates the text field in a way that says "this is an >>>>>> unreliable text opaque for humans"? >>>>> >>>>> The monitor_register_hmp_info_hrt only does the HMP glue side, and >>>>> that's only needed if you must dynamically register the HMP command. >>>>> For statically registered commands set '.cmd_info_hrt' directly in >>>>> the hml-commands-info.hx for the HMP side. >>>>> >>>>>> If a qapi/ schema is needed could you explain what it should look like for >>>>>> this example and where it should go? Looking at the existing .json files I >>>>>> can't immediately see one which is the right place for this to live. >>>>> >>>>> Take a look in qapi/machine.json for anyof the 'x-query-XXXX' commands >>>>> there. The QAPI bit is fairly simple. >>>>> >>>>> if you want to see an illustration of what's different from a previous >>>>> pure HMP impl, look at: >>>>> >>>>> commit dd98234c059e6bdb05a52998270df6d3d990332e >>>>> Author: Daniel P. Berrangé <berrange@redhat.com> >>>>> Date: Wed Sep 8 10:35:43 2021 +0100 >>>>> >>>>> qapi: introduce x-query-roms QMP command >>>> >>>> I see, thanks for the reference. So qapi/machine.json would be the right >>>> place to declare the QMP part even for a specific device? >>>> >>>> Even this approach still wouldn't work in its current form though, since as >>>> mentioned in my previous email it seems that only the target CONFIG_* >>>> defines and not the device CONFIG_* defines are present when processing >>>> hmp-commands-info.hx. >>> >>> Yeah, that's where the pain comes in. While QAPI schema can be made >>> conditional on a few CONFIG_* parameters - basically those derived >>> from global configure time options, it is impossible for this to be >>> with with target specific options like the device CONFIG_* defines. >>> >>> This is why I suggested in my othuer reply that it would need to be >>> done with a generic 'info dev-debug' / 'x-query-dev-debug' command >>> that can be registered unconditionally, and then individual devices >>> plug into it. >> >> After some more experiments this afternoon I still seem to be falling >> through the gaps on this one. This is based upon my understanding that all >> new HMP commands should use a QMP HumanReadableText implementation and the >> new command should be restricted according to target. >> >> Currently I am working with this change to hmp-commands-info.hx and >> qapi/misc-target.json: > > [snip] > >> i.e. qmp_marshal_output_HumanReadableText() isn't protected by the #if >> TARGET guards and since HumanReadableText is only used by the new >> qmp_x_query_via() functionality then the compiler complains and aborts the >> compilation. >> >> Possibly this is an error in the QAPI generator for types hidden behind >> commands using "if"? Otherwise I'm not sure what is the best way to proceed, >> so I'd be grateful for some further pointers. > > Yes, this is pretty much what I expect and exactly what I hit with > other target specific commands. > > That's why I suggested something like a general 'x-device-debug' command > that is NOT conditionalized in QAPI, against which dev impls can register > a callback to provide detailed reporting, instead of a device type specific > command. Ah so this is a known issue with this approach then. David mentioned earlier in the thread that he'd be okay with a HMP command if it was useful and restricted to the required targets, so would it be okay to add "info via" for now as just a (non-QMP wrapped) HMP info command if I can get that to work? ATB, Mark.
* Mark Cave-Ayland (mark.cave-ayland@ilande.co.uk) wrote: > On 21/02/2022 17:11, Daniel P. Berrangé wrote: > > > On Sun, Feb 20, 2022 at 05:18:33PM +0000, Mark Cave-Ayland wrote: > > > On 08/02/2022 13:10, Daniel P. Berrangé wrote: > > > > > > > On Tue, Feb 08, 2022 at 01:06:59PM +0000, Mark Cave-Ayland wrote: > > > > > On 08/02/2022 12:49, Daniel P. Berrangé wrote: > > > > > > > > > > > > I was under the impression that monitor_register_hmp_info_hrt() does all the > > > > > > > magic here i.e. it declares the underlying QMP command with an x- prefix and > > > > > > > effectively encapsulates the text field in a way that says "this is an > > > > > > > unreliable text opaque for humans"? > > > > > > > > > > > > The monitor_register_hmp_info_hrt only does the HMP glue side, and > > > > > > that's only needed if you must dynamically register the HMP command. > > > > > > For statically registered commands set '.cmd_info_hrt' directly in > > > > > > the hml-commands-info.hx for the HMP side. > > > > > > > > > > > > > If a qapi/ schema is needed could you explain what it should look like for > > > > > > > this example and where it should go? Looking at the existing .json files I > > > > > > > can't immediately see one which is the right place for this to live. > > > > > > > > > > > > Take a look in qapi/machine.json for anyof the 'x-query-XXXX' commands > > > > > > there. The QAPI bit is fairly simple. > > > > > > > > > > > > if you want to see an illustration of what's different from a previous > > > > > > pure HMP impl, look at: > > > > > > > > > > > > commit dd98234c059e6bdb05a52998270df6d3d990332e > > > > > > Author: Daniel P. Berrangé <berrange@redhat.com> > > > > > > Date: Wed Sep 8 10:35:43 2021 +0100 > > > > > > > > > > > > qapi: introduce x-query-roms QMP command > > > > > > > > > > I see, thanks for the reference. So qapi/machine.json would be the right > > > > > place to declare the QMP part even for a specific device? > > > > > > > > > > Even this approach still wouldn't work in its current form though, since as > > > > > mentioned in my previous email it seems that only the target CONFIG_* > > > > > defines and not the device CONFIG_* defines are present when processing > > > > > hmp-commands-info.hx. > > > > > > > > Yeah, that's where the pain comes in. While QAPI schema can be made > > > > conditional on a few CONFIG_* parameters - basically those derived > > > > from global configure time options, it is impossible for this to be > > > > with with target specific options like the device CONFIG_* defines. > > > > > > > > This is why I suggested in my othuer reply that it would need to be > > > > done with a generic 'info dev-debug' / 'x-query-dev-debug' command > > > > that can be registered unconditionally, and then individual devices > > > > plug into it. > > > > > > After some more experiments this afternoon I still seem to be falling > > > through the gaps on this one. This is based upon my understanding that all > > > new HMP commands should use a QMP HumanReadableText implementation and the > > > new command should be restricted according to target. > > > > > > Currently I am working with this change to hmp-commands-info.hx and > > > qapi/misc-target.json: > > > > [snip] > > > i.e. qmp_marshal_output_HumanReadableText() isn't protected by the #if > > > TARGET guards and since HumanReadableText is only used by the new > > > qmp_x_query_via() functionality then the compiler complains and aborts the > > > compilation. > > > > > > Possibly this is an error in the QAPI generator for types hidden behind > > > commands using "if"? Otherwise I'm not sure what is the best way to proceed, > > > so I'd be grateful for some further pointers. > > > > Yes, this is pretty much what I expect and exactly what I hit with > > other target specific commands. > > > > That's why I suggested something like a general 'x-device-debug' command > > that is NOT conditionalized in QAPI, against which dev impls can register > > a callback to provide detailed reporting, instead of a device type specific > > command. > > Ah so this is a known issue with this approach then. David mentioned earlier > in the thread that he'd be okay with a HMP command if it was useful and > restricted to the required targets, so would it be okay to add "info via" > for now as just a (non-QMP wrapped) HMP info command if I can get that to > work? I still am from an HMP point of view; it sounds like the right way in the future is to get the info devices or whatever; I suggest you keep it as close to a QMP implementation as possible, still with the HumanReadableText stuff. (Others might still be nervous about an HMP special; but I don't see it's worth holding this trivial stuff up for it). Dave > > ATB, > > Mark. >
On 22/02/2022 15:03, Dr. David Alan Gilbert wrote: > * Mark Cave-Ayland (mark.cave-ayland@ilande.co.uk) wrote: >> On 21/02/2022 17:11, Daniel P. Berrangé wrote: >> >>> On Sun, Feb 20, 2022 at 05:18:33PM +0000, Mark Cave-Ayland wrote: >>>> On 08/02/2022 13:10, Daniel P. Berrangé wrote: >>>> >>>>> On Tue, Feb 08, 2022 at 01:06:59PM +0000, Mark Cave-Ayland wrote: >>>>>> On 08/02/2022 12:49, Daniel P. Berrangé wrote: >>>>>> >>>>>>>> I was under the impression that monitor_register_hmp_info_hrt() does all the >>>>>>>> magic here i.e. it declares the underlying QMP command with an x- prefix and >>>>>>>> effectively encapsulates the text field in a way that says "this is an >>>>>>>> unreliable text opaque for humans"? >>>>>>> >>>>>>> The monitor_register_hmp_info_hrt only does the HMP glue side, and >>>>>>> that's only needed if you must dynamically register the HMP command. >>>>>>> For statically registered commands set '.cmd_info_hrt' directly in >>>>>>> the hml-commands-info.hx for the HMP side. >>>>>>> >>>>>>>> If a qapi/ schema is needed could you explain what it should look like for >>>>>>>> this example and where it should go? Looking at the existing .json files I >>>>>>>> can't immediately see one which is the right place for this to live. >>>>>>> >>>>>>> Take a look in qapi/machine.json for anyof the 'x-query-XXXX' commands >>>>>>> there. The QAPI bit is fairly simple. >>>>>>> >>>>>>> if you want to see an illustration of what's different from a previous >>>>>>> pure HMP impl, look at: >>>>>>> >>>>>>> commit dd98234c059e6bdb05a52998270df6d3d990332e >>>>>>> Author: Daniel P. Berrangé <berrange@redhat.com> >>>>>>> Date: Wed Sep 8 10:35:43 2021 +0100 >>>>>>> >>>>>>> qapi: introduce x-query-roms QMP command >>>>>> >>>>>> I see, thanks for the reference. So qapi/machine.json would be the right >>>>>> place to declare the QMP part even for a specific device? >>>>>> >>>>>> Even this approach still wouldn't work in its current form though, since as >>>>>> mentioned in my previous email it seems that only the target CONFIG_* >>>>>> defines and not the device CONFIG_* defines are present when processing >>>>>> hmp-commands-info.hx. >>>>> >>>>> Yeah, that's where the pain comes in. While QAPI schema can be made >>>>> conditional on a few CONFIG_* parameters - basically those derived >>>>> from global configure time options, it is impossible for this to be >>>>> with with target specific options like the device CONFIG_* defines. >>>>> >>>>> This is why I suggested in my othuer reply that it would need to be >>>>> done with a generic 'info dev-debug' / 'x-query-dev-debug' command >>>>> that can be registered unconditionally, and then individual devices >>>>> plug into it. >>>> >>>> After some more experiments this afternoon I still seem to be falling >>>> through the gaps on this one. This is based upon my understanding that all >>>> new HMP commands should use a QMP HumanReadableText implementation and the >>>> new command should be restricted according to target. >>>> >>>> Currently I am working with this change to hmp-commands-info.hx and >>>> qapi/misc-target.json: >>> >>> [snip] >>>> i.e. qmp_marshal_output_HumanReadableText() isn't protected by the #if >>>> TARGET guards and since HumanReadableText is only used by the new >>>> qmp_x_query_via() functionality then the compiler complains and aborts the >>>> compilation. >>>> >>>> Possibly this is an error in the QAPI generator for types hidden behind >>>> commands using "if"? Otherwise I'm not sure what is the best way to proceed, >>>> so I'd be grateful for some further pointers. >>> >>> Yes, this is pretty much what I expect and exactly what I hit with >>> other target specific commands. >>> >>> That's why I suggested something like a general 'x-device-debug' command >>> that is NOT conditionalized in QAPI, against which dev impls can register >>> a callback to provide detailed reporting, instead of a device type specific >>> command. >> >> Ah so this is a known issue with this approach then. David mentioned earlier >> in the thread that he'd be okay with a HMP command if it was useful and >> restricted to the required targets, so would it be okay to add "info via" >> for now as just a (non-QMP wrapped) HMP info command if I can get that to >> work? > > I still am from an HMP point of view; it sounds like the right way in > the future is to get the info devices or whatever; I suggest you keep > it as close to a QMP implementation as possible, still with the > HumanReadableText stuff. > (Others might still be nervous about an HMP special; but I don't see > it's worth holding this trivial stuff up for it). I've just posted a v2 and what I've done there is to manually add a hmp_info_via() wrapper (taken almost verbatim from https://www.qemu.org/docs/master/devel/writing-monitor-commands.html#id1) and added it to both include/monitor/hmp-target.h and include/hw/misc/mos6522.h which passes a Gitlab run. I think it's worth having as an in-tree reference for when a more formal HMP/QMP per-device as opposed to per-target infrastructure arrives. ATB, Mark.
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 diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c index aaae195d63..cfa6a9c44b 100644 --- a/hw/misc/mos6522.c +++ b/hw/misc/mos6522.c @@ -30,6 +30,8 @@ #include "hw/misc/mos6522.h" #include "hw/qdev-properties.h" #include "migration/vmstate.h" +#include "monitor/monitor.h" +#include "qapi/type-helpers.h" #include "qemu/timer.h" #include "qemu/cutils.h" #include "qemu/log.h" @@ -415,6 +417,95 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) } } +static int qmp_x_query_via_foreach(Object *obj, void *opaque) +{ + GString *buf = opaque; + + if (object_dynamic_cast(obj, TYPE_MOS6522)) { + MOS6522State *s = MOS6522(obj); + int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); + uint16_t t1counter = get_counter(s, &s->timers[0]); + uint16_t t2counter = get_counter(s, &s->timers[1]); + + g_string_append_printf(buf, "%s:\n", object_get_typename(obj)); + + g_string_append_printf(buf, " Registers:\n"); + g_string_append_printf(buf, " %-*s: 0x%x\n", 4, + mos6522_reg_names[0], s->b); + g_string_append_printf(buf, " %-*s: 0x%x\n", 4, + mos6522_reg_names[1], s->a); + g_string_append_printf(buf, " %-*s: 0x%x\n", 4, + mos6522_reg_names[2], s->dirb); + g_string_append_printf(buf, " %-*s: 0x%x\n", 4, + mos6522_reg_names[3], s->dira); + g_string_append_printf(buf, " %-*s: 0x%x\n", 4, + mos6522_reg_names[4], t1counter & 0xff); + g_string_append_printf(buf, " %-*s: 0x%x\n", 4, + mos6522_reg_names[5], t1counter >> 8); + g_string_append_printf(buf, " %-*s: 0x%x\n", 4, + mos6522_reg_names[6], + s->timers[0].latch & 0xff); + g_string_append_printf(buf, " %-*s: 0x%x\n", 4, + mos6522_reg_names[7], + s->timers[0].latch >> 8); + g_string_append_printf(buf, " %-*s: 0x%x\n", 4, + mos6522_reg_names[8], t2counter & 0xff); + g_string_append_printf(buf, " %-*s: 0x%x\n", 4, + mos6522_reg_names[9], t2counter >> 8); + g_string_append_printf(buf, " %-*s: 0x%x\n", 4, + mos6522_reg_names[10], s->sr); + g_string_append_printf(buf, " %-*s: 0x%x\n", 4, + mos6522_reg_names[11], s->acr); + g_string_append_printf(buf, " %-*s: 0x%x\n", 4, + mos6522_reg_names[12], s->pcr); + g_string_append_printf(buf, " %-*s: 0x%x\n", 4, + mos6522_reg_names[13], s->ifr); + g_string_append_printf(buf, " %-*s: 0x%x\n", 4, + mos6522_reg_names[14], s->ier); + + g_string_append_printf(buf, " Timers:\n"); + g_string_append_printf(buf, " Using current time now(ns)=%"PRId64 + "\n", now); + g_string_append_printf(buf, " T1 freq(hz)=%"PRId64 + " mode=%s" + " counter=0x%x" + " latch=0x%x\n" + " load_time(ns)=%"PRId64 + " next_irq_time(ns)=%"PRId64 "\n", + s->timers[0].frequency, + ((s->acr & T1MODE) == T1MODE_CONT) ? "continuous" + : "one-shot", + t1counter, + s->timers[0].latch, + s->timers[0].load_time, + get_next_irq_time(s, &s->timers[0], now)); + g_string_append_printf(buf, " T2 freq(hz)=%"PRId64 + " mode=%s" + " counter=0x%x" + " latch=0x%x\n" + " load_time(ns)=%"PRId64 + " next_irq_time(ns)=%"PRId64 "\n", + s->timers[1].frequency, + "one-shot", + t2counter, + s->timers[1].latch, + s->timers[1].load_time, + get_next_irq_time(s, &s->timers[1], now)); + } + + return 0; +} + +static HumanReadableText *qmp_x_query_via(Error **errp) +{ + g_autoptr(GString) buf = g_string_new(""); + + object_child_foreach_recursive(object_get_root(), + qmp_x_query_via_foreach, buf); + + return human_readable_text_from_str(buf); +} + static const MemoryRegionOps mos6522_ops = { .read = mos6522_read, .write = mos6522_write, @@ -547,6 +638,7 @@ static const TypeInfo mos6522_type_info = { static void mos6522_register_types(void) { type_register_static(&mos6522_type_info); + monitor_register_hmp_info_hrt("via", qmp_x_query_via); } type_init(mos6522_register_types)
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(+)