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"