On 9/19/24 9:22 AM, Daniel P. Berrangé wrote:
On Thu, Sep 19, 2024 at 08:20:56AM -0300, Daniel Henrique Barboza wrote:
Add a QMP command that shows all specific properties of the current
accelerator in use.
Why do we need to expose /everything/ ?
I wouldn't mind pick and choose advertised properties for the accelerators
like we do with other APIs.
This would mean that each arch should choose what to advertise or not, given
that
some accelerator properties might be relevant just for some archs. The API would
be implemented by each arch individually.
This can be used as a complement of other APIs like query-machines and
query-cpu-model-expansion, allowing management to get a more complete
picture of the running QEMU process.
query-machines doesn't return every single QOM property, just
a hand selected set of information pieces.
The query-cpu-model-expansion does return everything, but I
consider that command to be bad design, as it doesn't distinguish
between hardware CPU features, and QEMU QOM properties
This is the output with a x86_64 TCG guest:
$ ./build/qemu-system-x86_64 -S -display none -accel tcg -qmp
tcp:localhost:1234,server,wait=off
$ ./scripts/qmp/qmp-shell localhost:1234
Welcome to the QMP low-level shell!
Connected to QEMU 9.1.50
(QEMU) query-accelerator
{"return": {"name": "tcg", "props": {"one-insn-per-tb": false, "thread": "multi", "tb-size": 0,
"split-wx": false, "type": "tcg-accel"}}}
And for a x86_64 KVM guest:
$ ./build/qemu-system-x86_64 -S -display none -accel kvm -qmp
tcp:localhost:1234,server,wait=off
$ ./scripts/qmp/qmp-shell localhost:1234
Welcome to the QMP low-level shell!
Connected to QEMU 9.1.50
(QEMU) query-accelerator
{"return": {"name": "KVM", "props": {"mem-container-smram[0]": "", "xen-gnttab-max-frames": 64, "device": "", "xen-version": 0, "mem-smram[0]": "",
"notify-window": 0, "dirty-ring-size": 0, "kvm-shadow-mem": -1, "type": "kvm-accel", "notify-vmexit": "run", "xen-evtchn-max-pirq": 256}}}
Signed-off-by: Daniel Henrique Barboza <dbarb...@ventanamicro.com>
---
hw/core/machine-qmp-cmds.c | 34 ++++++++++++++++++++++++++++++++++
qapi/machine.json | 27 +++++++++++++++++++++++++++
2 files changed, 61 insertions(+)
diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
index 130217da8f..eac803bf36 100644
--- a/hw/core/machine-qmp-cmds.c
+++ b/hw/core/machine-qmp-cmds.c
+AccelInfo *qmp_query_accelerator(Error **errp)
+{
+ AccelState *accel = current_accel();
+ AccelClass *acc = ACCEL_GET_CLASS(accel);
+ AccelInfo *info = g_new0(AccelInfo, 1);
+ QDict *qdict_out = qdict_new();
+ ObjectPropertyIterator iter;
+ ObjectProperty *prop;
+
+ info->name = g_strdup(acc->name);
+
+ object_property_iter_init(&iter, OBJECT(accel));
+ while ((prop = object_property_iter_next(&iter))) {
+ QObject *value;
+
+ if (!prop->get) {
+ continue;
+ }
+
+ value = object_property_get_qobject(OBJECT(accel), prop->name,
+ &error_abort);
+ qdict_put_obj(qdict_out, prop->name, value);
+ }
I'm not at all convinced trhat we should be exposing every single
QOM property on the accelerator class as public QMP data
+
+ if (!qdict_size(qdict_out)) {
+ qobject_unref(qdict_out);
+ } else {
+ info->props = QOBJECT(qdict_out);
+ }
+
+ return info;
+}
diff --git a/qapi/machine.json b/qapi/machine.json
index a6b8795b09..d0d527d1eb 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1898,3 +1898,30 @@
{ 'command': 'x-query-interrupt-controllers',
'returns': 'HumanReadableText',
'features': [ 'unstable' ]}
+
+##
+# @AccelInfo:
+#
+# Information about the current accelerator.
+#
+# @name: the name of the current accelerator being used
+#
+# @props: a dictionary of the accelerator properties
+#
+# Since: 9.2
+##
+{ 'struct': 'AccelInfo',
+ 'data': { 'name': 'str',
+ '*props': 'any' } }
This is way too open ended. IMHO ideally we would never add more
instances of the 'any' type, as it has many downsides
- zero documentation about what is available
- no version info about when each prop was introduced
- no ability to tag fields as deprecated
For this new API, IMHO 'name' should be an enumeration of the
accelerator types, and thenm 'props' should be a discrinated
union of accelerator specific structs
We have accelerator properties that differs from arch to arch, e.g. x86 has
properties like
notify-vmexit, declared in kvm_arch_accel_class_init() from
target/i386/kvm/kvm.c, that no
other arch has access to. RISC-V has its own share of these properties too.
Is it possible to declare specific structs based on arch for the API? In a
quick glance
it seems like we're doing something like that with query-cpus-fast, where s390x
has
additional properties that are exposed.
Thanks,
Daniel
+
+##
+# @query-accelerator:
+#
+# Shows information about the accelerator in use.
+#
+# Returns: a CpuModelExpansionInfo describing the expanded CPU model
+#
+# Since: 9.2
+##
+{ 'command': 'query-accelerator',
+ 'returns': 'AccelInfo' }
--
2.45.2
With regards,
Daniel
