docs/devel/writing-qmp-commands.rst | 25 +++ hw/core/cpu-common.c | 15 ++ hw/core/machine-qmp-cmds.c | 28 +++ include/hw/core/cpu.h | 13 +- monitor/misc.c | 25 ++- qapi/machine.json | 37 ++++ target/i386/cpu-dump.c | 325 +++++++++++++++------------- target/i386/cpu.c | 2 +- target/i386/cpu.h | 2 +- 9 files changed, 307 insertions(+), 165 deletions(-)
We are still adding HMP commands without any QMP counterparts. This is done because there are a reasonable number of scenarios where the cost of designing a QAPI data type for the command is not justified. This has the downside, however, that we will never be able to fully isolate the monitor code from the remainder of QEMU internals. It is desirable to be able to get to a point where subsystems in QEMU are exclusively implemented using QAPI types and never need to have any knowledge of the monitor APIs. The way to get there is to stop adding commands to HMP only. All commands must be implemented using QMP, and any HMP implementation be a shim around the QMP implementation. We don't want to compromise our supportability of QMP long term though. This series proposes that we relax our requirements around fine grained QAPI data design, but with the caveat that any command taking this design approach is mandated to use the 'x-' name prefix. This tradeoff should be suitable for any commands we have been adding exclusively to HMP in recent times, and thus mean we have mandate QMP support for all new commands going forward. This series illustrates the concept by converting the "info registers" HMP to invoke a new 'x-query-registers' QMP command. Note that only the i386 CPU target is converted to work with this new approach, so this series needs to be considered incomplete. If we go forward with this idea, then a subsequent version of this series would need to obviously convert all other CPU targets. After doing that conversion the only use of qemu_fprintf() would be the disas.c file. Remaining uses of qemu_fprintf and qemu_printf could be tackled in a similar way and eventually eliminate the need for any of these printf wrappers in QEMU. NB: I added docs to devel/writing-qmp-commands.rst about the two design approaches to QMP. I didn't see another good place to put an explicit note that we will not add any more HMP-only commands. Obviously HMP/QMP maintainers control this in their reviews of patches, and maybe that's sufficient ? NB: if we take this approach we'll want to figure out how many HMP-only commands we actually have left and then perhaps have a task to track their conversion to QMP. This could possibly be a useful task for newbies if we make it clear that they wouldn't be required to undertake complex QAPI modelling in doing this conversion. Daniel P. Berrangé (5): docs/devel: document expectations for QAPI data modelling for QMP hw/core: introduce 'format_state' callback to replace 'dump_state' target/i386: convert to use format_state instead of dump_state qapi: introduce x-query-registers QMP command monitor: rewrite 'info registers' in terms of 'x-query-registers' docs/devel/writing-qmp-commands.rst | 25 +++ hw/core/cpu-common.c | 15 ++ hw/core/machine-qmp-cmds.c | 28 +++ include/hw/core/cpu.h | 13 +- monitor/misc.c | 25 ++- qapi/machine.json | 37 ++++ target/i386/cpu-dump.c | 325 +++++++++++++++------------- target/i386/cpu.c | 2 +- target/i386/cpu.h | 2 +- 9 files changed, 307 insertions(+), 165 deletions(-) -- 2.31.1
Daniel P. Berrangé <berrange@redhat.com> writes:
> We are still adding HMP commands without any QMP counterparts. This is
> done because there are a reasonable number of scenarios where the cost
> of designing a QAPI data type for the command is not justified.
>
> This has the downside, however, that we will never be able to fully
> isolate the monitor code from the remainder of QEMU internals. It is
> desirable to be able to get to a point where subsystems in QEMU are
> exclusively implemented using QAPI types and never need to have any
> knowledge of the monitor APIs.
>
> The way to get there is to stop adding commands to HMP only. All
> commands must be implemented using QMP, and any HMP implementation
> be a shim around the QMP implementation.
>
> We don't want to compromise our supportability of QMP long term though.
>
> This series proposes that we relax our requirements around fine grained
> QAPI data design,
Specifics? QMP command returns a string, HMP wrapper prints that
string?
> but with the caveat that any command taking this
> design approach is mandated to use the 'x-' name prefix.
>
> This tradeoff should be suitable for any commands we have been adding
> exclusively to HMP in recent times, and thus mean we have mandate QMP
> support for all new commands going forward.
>
> This series illustrates the concept by converting the "info registers"
> HMP to invoke a new 'x-query-registers' QMP command. Note that only
> the i386 CPU target is converted to work with this new approach, so
> this series needs to be considered incomplete. If we go forward with
> this idea, then a subsequent version of this series would need to
> obviously convert all other CPU targets.
>
> After doing that conversion the only use of qemu_fprintf() would be
> the disas.c file. Remaining uses of qemu_fprintf and qemu_printf
> could be tackled in a similar way and eventually eliminate the need
> for any of these printf wrappers in QEMU.
>
> NB: I added docs to devel/writing-qmp-commands.rst about the two
> design approaches to QMP. I didn't see another good place to put
> an explicit note that we will not add any more HMP-only commands.
> Obviously HMP/QMP maintainers control this in their reviews of
> patches, and maybe that's sufficient ?
We could add devel/writing-hmp-commands.rst, or go with a single
document devel/writing-monitor-commands.rst.
> NB: if we take this approach we'll want to figure out how many
> HMP-only commands we actually have left and then perhaps have
> HMP-only commands we actually have left
Yes.
For many HMP commands, a QMP commands with the same name exists, and the
former is probably a wrapper around the latter. Same for HMP "info FOO"
and QMP query-FOO.
HMP commands without such a match:
boot_set
change
commit
cpu
delvm
drive_add
drive_del
exit_preconfig
gdbserver
gpa2hpa
gpa2hva
gva2gpa
help
hostfwd_add
hostfwd_remove
i
info
info capture
info cmma
info cpus
info history
info ioapic
info irq
info jit
info lapic
info mem
info mtree
info network
info numa
info opcount
info pic
info profile
info qdm
info qom-tree
info qtree
info ramblock
info rdma
info registers
info roms
info skeys
info snapshots
info sync-profile
info tlb
info trace-events
info usb
info usbhost
info usernet
loadvm
log
logfile
mce
migrate_set_capability
migrate_set_parameter
migration_mode
mouse_button
mouse_move
mouse_set
nmi
o
pcie_aer_inject_error
print
qemu-io
savevm
sendkey
singlestep
snapshot_blkdev
snapshot_blkdev_internal
snapshot_delete_blkdev_internal
stopcapture
sum
sync-profile
trace-event
trace-file
watchdog_action
wavcapture
x
xp
This is 77 out of 170 HMP commands. I was hoping for fewer.
> and then perhaps have
> a task to track their conversion to QMP. This could possibly
> be a useful task for newbies if we make it clear that they
> wouldn't be required to undertake complex QAPI modelling in
> doing this conversion.
Yes.
Thanks for tackling this!
On Wed, Sep 08, 2021 at 05:09:13PM +0200, Markus Armbruster wrote: > Daniel P. Berrangé <berrange@redhat.com> writes: > > > We are still adding HMP commands without any QMP counterparts. This is > > done because there are a reasonable number of scenarios where the cost > > of designing a QAPI data type for the command is not justified. > > > > This has the downside, however, that we will never be able to fully > > isolate the monitor code from the remainder of QEMU internals. It is > > desirable to be able to get to a point where subsystems in QEMU are > > exclusively implemented using QAPI types and never need to have any > > knowledge of the monitor APIs. > > > > The way to get there is to stop adding commands to HMP only. All > > commands must be implemented using QMP, and any HMP implementation > > be a shim around the QMP implementation. > > > > We don't want to compromise our supportability of QMP long term though. > > > > This series proposes that we relax our requirements around fine grained > > QAPI data design, > > Specifics? QMP command returns a string, HMP wrapper prints that > string? yes, a command returning a single opaque printf formatted string would be the common case. At a more general POV though, the JSON doc received by the client should be usable "as received", immediately after JSON de-serialization without needing any further custom parsing on top. ie if a value needs to be parsed by the client, then it must be split into multiple distinct values in the QAPI data type design to remove the need for parsing by the client. If a command's design violates that, then it must remain under the "x-" prefix. "info registers" is a example because we're printf formatting each register value into a opaque string. Any client needing a specific register value would have to scanf parse this string. The justification for not representing each register as a distinct QAPI field is that we don't think machines genuinely need to parse this, as its just adhoc human operator debug info. So we take the easy option and just printf to a string and put it under "x-" prefix > For many HMP commands, a QMP commands with the same name exists, and the > former is probably a wrapper around the latter. Same for HMP "info FOO" > and QMP query-FOO. > > HMP commands without such a match: > > boot_set > change > commit > cpu > delvm > drive_add > drive_del > exit_preconfig > gdbserver > gpa2hpa > gpa2hva > gva2gpa > help > hostfwd_add > hostfwd_remove > i > info > info capture > info cmma > info cpus > info history > info ioapic > info irq > info jit > info lapic > info mem > info mtree > info network > info numa > info opcount > info pic > info profile > info qdm > info qom-tree > info qtree > info ramblock > info rdma > info registers > info roms > info skeys > info snapshots > info sync-profile > info tlb > info trace-events > info usb > info usbhost > info usernet > loadvm > log > logfile > mce > migrate_set_capability > migrate_set_parameter > migration_mode > mouse_button > mouse_move > mouse_set > nmi > o > pcie_aer_inject_error > print > qemu-io > savevm > sendkey > singlestep > snapshot_blkdev > snapshot_blkdev_internal > snapshot_delete_blkdev_internal > stopcapture > sum > sync-profile > trace-event > trace-file > watchdog_action > wavcapture > x > xp > > This is 77 out of 170 HMP commands. I was hoping for fewer. Wow, yes, I'm surprised ! A few of those do indeed have QMP equivalents, but with slight differences eg savevm -> snapshot-save, but clearly we have a big list regardless Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Daniel P. Berrangé <berrange@redhat.com> writes: > On Wed, Sep 08, 2021 at 05:09:13PM +0200, Markus Armbruster wrote: >> Daniel P. Berrangé <berrange@redhat.com> writes: >> >> > We are still adding HMP commands without any QMP counterparts. This is >> > done because there are a reasonable number of scenarios where the cost >> > of designing a QAPI data type for the command is not justified. >> > >> > This has the downside, however, that we will never be able to fully >> > isolate the monitor code from the remainder of QEMU internals. It is >> > desirable to be able to get to a point where subsystems in QEMU are >> > exclusively implemented using QAPI types and never need to have any >> > knowledge of the monitor APIs. >> > >> > The way to get there is to stop adding commands to HMP only. All >> > commands must be implemented using QMP, and any HMP implementation >> > be a shim around the QMP implementation. >> > >> > We don't want to compromise our supportability of QMP long term though. >> > >> > This series proposes that we relax our requirements around fine grained >> > QAPI data design, >> >> Specifics? QMP command returns a string, HMP wrapper prints that >> string? > > yes, a command returning a single opaque printf formatted string would > be the common case. At a more general POV though, the JSON doc received > by the client should be usable "as received", immediately after JSON > de-serialization without needing any further custom parsing on top. > > ie if a value needs to be parsed by the client, then it must be split > into multiple distinct values in the QAPI data type design to remove > the need for parsing by the client. Yes, that's QMP doctrine. > If a command's design violates that, then it must remain under the > "x-" prefix. "info registers" is a example because we're printf > formatting each register value into a opaque string. Any client > needing a specific register value would have to scanf parse this > string. The justification for not representing each register as > a distinct QAPI field is that we don't think machines genuinely > need to parse this, as its just adhoc human operator debug info. > So we take the easy option and just printf to a string and put > it under "x-" prefix Got it. Limitations: 1. If we convert a long-running HMP command to this technique, we print its output only after it completed its work. We also end up with a long-running QMP command, which is bad, because it stops the main loop and makes the QMP monitor unresponsive (except for OOB commands, if the client is careful). The former can be mitigated with 'coroutine': true. The latter can't. 2. We can't prompt for input. The only current use I can see is HMP "change vnc passwd" prompting for a password. Except you currently have to say "change vnc passwd wtf" to get it to prompt (suspect logic error in commit cfb5387a1de). [...]
Markus Armbruster <armbru@redhat.com> writes: [...] > Limitations: > > 1. If we convert a long-running HMP command to this technique, we print > its output only after it completed its work. We also end up with a > long-running QMP command, which is bad, because it stops the main > loop and makes the QMP monitor unresponsive (except for OOB commands, > if the client is careful). The former can be mitigated with > 'coroutine': true. The latter can't. > > 2. We can't prompt for input. > > The only current use I can see is HMP "change vnc passwd" prompting > for a password. Except you currently have to say "change vnc passwd > wtf" to get it to prompt (suspect logic error in commit cfb5387a1de). Subject: [PATCH 1/2] hmp: Unbreak "change vnc" Message-Id: <20210909081219.308065-2-armbru@redhat.com> > > > [...]
On 9/8/21 5:09 PM, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
>
>> We are still adding HMP commands without any QMP counterparts. This is
>> done because there are a reasonable number of scenarios where the cost
>> of designing a QAPI data type for the command is not justified.
>>
>> This has the downside, however, that we will never be able to fully
>> isolate the monitor code from the remainder of QEMU internals. It is
>> desirable to be able to get to a point where subsystems in QEMU are
>> exclusively implemented using QAPI types and never need to have any
>> knowledge of the monitor APIs.
>>
>> The way to get there is to stop adding commands to HMP only. All
>> commands must be implemented using QMP, and any HMP implementation
>> be a shim around the QMP implementation.
>>
>> We don't want to compromise our supportability of QMP long term though.
>>
>> This series proposes that we relax our requirements around fine grained
>> QAPI data design,
>
> Specifics? QMP command returns a string, HMP wrapper prints that
> string?
>
>> but with the caveat that any command taking this
>> design approach is mandated to use the 'x-' name prefix.
>>
>> This tradeoff should be suitable for any commands we have been adding
>> exclusively to HMP in recent times, and thus mean we have mandate QMP
>> support for all new commands going forward.
>>
>> This series illustrates the concept by converting the "info registers"
>> HMP to invoke a new 'x-query-registers' QMP command. Note that only
>> the i386 CPU target is converted to work with this new approach, so
>> this series needs to be considered incomplete. If we go forward with
>> this idea, then a subsequent version of this series would need to
>> obviously convert all other CPU targets.
>>
>> After doing that conversion the only use of qemu_fprintf() would be
>> the disas.c file. Remaining uses of qemu_fprintf and qemu_printf
>> could be tackled in a similar way and eventually eliminate the need
>> for any of these printf wrappers in QEMU.
>>
>> NB: I added docs to devel/writing-qmp-commands.rst about the two
>> design approaches to QMP. I didn't see another good place to put
>> an explicit note that we will not add any more HMP-only commands.
>> Obviously HMP/QMP maintainers control this in their reviews of
>> patches, and maybe that's sufficient ?
>
> We could add devel/writing-hmp-commands.rst, or go with a single
> document devel/writing-monitor-commands.rst.
>
>> NB: if we take this approach we'll want to figure out how many
>> HMP-only commands we actually have left and then perhaps have
>> HMP-only commands we actually have left
>
> Yes.
>
> For many HMP commands, a QMP commands with the same name exists, and the
> former is probably a wrapper around the latter. Same for HMP "info FOO"
> and QMP query-FOO.
>
> HMP commands without such a match:
(1) Handy HMP commands while debugging:
- help
- info *
- i/o
- loadvm/savevm
- trace-event/trace-file
- wavcapture/stopcapture
- xp
Eventually also:
- hostfwd_add/hostfwd_remove
- log
- logfile
- print
- sendkey
(2) I suppose these are pre-QMP and wonder about their
usefulness in the monitor (I'd certainly use a QMP
equivalent to test).
- migrate_set_capability
- migrate_set_parameter
- migration_mode
- mouse_button
- mouse_move
- mouse_set
- nmi
- pcie_aer_inject_error
- exit_preconfig
- singlestep
- snapshot_blkdev_internal
- snapshot_delete_blkdev_internal
(3) I don't use them, I expect them to belong
in either (1) or (2).
> boot_set
> change
> commit
> cpu
> delvm
> drive_add
> drive_del
> gdbserver
> gpa2hpa
> gpa2hva
> gva2gpa
> mce
> qemu-io
> snapshot_blkdev
> sum
> sync-profile
> watchdog_action
>
> This is 77 out of 170 HMP commands. I was hoping for fewer.
On 08/09/21 17:09, Markus Armbruster wrote:
> This is 77 out of 170 HMP commands. I was hoping for fewer.
There are several that are not quite 1:1, but still not HMP-specific.
> exit_preconfig
This is x-exit-preconfig.
> migrate_set_capability
> migrate_set_parameter
These are migrate-set-{capabilities,parameters}
> mouse_button
> mouse_move
> mouse_set
> sendkey
These are input-send-event, but they are not implemented in terms of it.
> nmi
This is inject-nmi.
> loadvm
> savevm
These are snapshot-{load,save}.
> watchdog_action
This is set-action, but it is not implemented in terms of it.
Paolo
>> and then perhaps have
>> a task to track their conversion to QMP. This could possibly
>> be a useful task for newbies if we make it clear that they
>> wouldn't be required to undertake complex QAPI modelling in
>> doing this conversion.
>
> Yes.
>
> Thanks for tackling this!
>
>
© 2016 - 2024 Red Hat, Inc.