Add a QMP command that shows all specific properties of the current
accelerator in use.

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.

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 <dbarboza@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
@@ -15,6 +15,7 @@
 #include "qapi/error.h"
 #include "qapi/qapi-builtin-visit.h"
 #include "qapi/qapi-commands-machine.h"
+#include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qobject.h"
 #include "qapi/qobject-input-visitor.h"
 #include "qapi/type-helpers.h"
@@ -406,3 +407,36 @@ GuidInfo *qmp_query_vm_generation_id(Error **errp)
     info->guid = qemu_uuid_unparse_strdup(&vms->guid);
     return info;
 }
+
+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);
+    }
+
+    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' } }
+
+##
+# @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

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/ ?

> 
> 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 <dbarboza@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

> +
> +##
> +# @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
-- 
|: 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 :|

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 <dbarboza@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

Markus: QAPI design Qs for you at the bottom

On Wed, Sep 25, 2024 at 10:19:33AM -0300, Daniel Henrique Barboza wrote:
> 
> 
> 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.

Well with qemu-system-any we might get multiple arches reporting
info in the same binary, so we'll need to fan out to fill in the
per-arch info, after doing a common base.

Hmmm, i wonder if qemu-system-any will support mixing KVM and TCG ?
ie KVM for the host native accelerator, combined with TCG for the
foreign archs ??? Hopefully not !

> > > 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 <dbarboza@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.

To allow for qemu-system-any, which will eventually have multiple arches in
one, I guess we'll need multiple levels of nesting. Perhaps  something like
this:

  { 'enum': 'AccelType',
    'data': ['tcg', 'kvm', ....] }

  { 'union': 'AccelInfo',
    'type': 'AccelType',
    'data': {
        'tcg': 'AccelInfoTCG',
	'kvm': 'AccelInfoKVM',
    } }

  { 'struct': 'AccelInfoTCGX86',
    'data': {
        'notify-vmexit': ...
    } }

  { 'struct': 'AccelInfoTCGArch',
    'data': {
       'x86': 'AccelInfoTCGX86',
       'riscv': 'AccelInfoTCGRiscV',
       ...etc...
    }

  { 'struct': 'AccelInfoTCG',
    'data': {
         ...non-arch specific fields...,
	 'arch': 'AccelInfoTCGArch',
    } }

 ...equiv AccelInfoKVM* structs....

Markus:  any other/better ideas ?

> > > +
> > > +##
> > > +# @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' }
> > > -- 

With 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:

> Markus: QAPI design Qs for you at the bottom
>
> On Wed, Sep 25, 2024 at 10:19:33AM -0300, Daniel Henrique Barboza wrote:
>> 
>> 
>> 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.
>
> Well with qemu-system-any we might get multiple arches reporting
> info in the same binary, so we'll need to fan out to fill in the
> per-arch info, after doing a common base.
>
> Hmmm, i wonder if qemu-system-any will support mixing KVM and TCG ?
> ie KVM for the host native accelerator, combined with TCG for the
> foreign archs ??? Hopefully not !
>
>> > > 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 <dbarboza@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

Yes, 'any' is best avoided in stable interfaces.

>> > 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.

Yes.  Schema pattern:

    { 'union: ...,
      'base': {
          'target': 'SysEmuTarget',
          ... target-independent data ... }
      'data': {
          T: { ... data for target T ... }
          ... } }

On the wire:

    {"target": T,
     ... target-independent data ...
     ... data for target T}

This is for returning info on a single target.

Typically, a qemu-system-T will only ever return branch T.  The other
branches are dead code.  Tolerable.

With qemu-system-any, which provides multiple (eventually all, we hope)
targets, the union still has data for just one target.  To get data for
multiple targets, we'd have to return several objects.

Fine for query-cpus-fast: it returns a CpuInfoFast per CPU, and each of
them uses the branch T appropriate for the CPU.

> To allow for qemu-system-any, which will eventually have multiple arches in
> one,

Actually, qemu-system-any can work almost exactly like the existing
qemu-system-T, until we mix in heterogeneous machines.

Without heterogeneous machines, qemu-system-any's target T gets fixed at
some point.  From that point on, no difference to qemu-system-T.

The "almost" is for running commands before that point.

This applies to all commands that take the target as implicit input.  I
believe several exist.

How accelerators will work for heterogeneous machines is not clear to
me.  Until it is, I can't really advise on how to design accelerator
information for them.

>      I guess we'll need multiple levels of nesting. Perhaps  something like
> this:
>
>   { 'enum': 'AccelType',
>     'data': ['tcg', 'kvm', ....] }
>
>   { 'union': 'AccelInfo',
>     'type': 'AccelType',
>     'data': {
>         'tcg': 'AccelInfoTCG',
> 	'kvm': 'AccelInfoKVM',
>     } }
>
>   { 'struct': 'AccelInfoTCGX86',
>     'data': {
>         'notify-vmexit': ...
>     } }
>
>   { 'struct': 'AccelInfoTCGArch',
>     'data': {
>        'x86': 'AccelInfoTCGX86',
>        'riscv': 'AccelInfoTCGRiscV',
>        ...etc...
>     }
>
>   { 'struct': 'AccelInfoTCG',
>     'data': {
>          ...non-arch specific fields...,
> 	 'arch': 'AccelInfoTCGArch',
>     } }

Here, you want to return data for all targets, not just one.  Instead of
a sum type (union branches), you use a product type (struct members).
Feels fair.

>  ...equiv AccelInfoKVM* structs....

The only target that can actually work is the one matching the host
hardware.  Perhaps we want to return just that.

> Markus:  any other/better ideas ?

I guess there are other ways to skin this cat.  But I think there are
questions to ask before we delve deeper:

1. query-accelerator use cases?  What accelerator information exactly do
we need to provide for them?

2. How fancy do we want to get for homogeneous qemu-system-any before
the target gets fixed?

3. How will accelerators work for heterogeneous qemu-system-any?

[...]

On 10/7/24 6:53 AM, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
>> Markus: QAPI design Qs for you at the bottom
>>
>> On Wed, Sep 25, 2024 at 10:19:33AM -0300, Daniel Henrique Barboza wrote:
>>>
>>>
>>> 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.
>>
>> Well with qemu-system-any we might get multiple arches reporting
>> info in the same binary, so we'll need to fan out to fill in the
>> per-arch info, after doing a common base.
>>
>> Hmmm, i wonder if qemu-system-any will support mixing KVM and TCG ?
>> ie KVM for the host native accelerator, combined with TCG for the
>> foreign archs ??? Hopefully not !
>>
>>>>> 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 <dbarboza@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
> 
> Yes, 'any' is best avoided in stable interfaces.
> 
>>>> 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.
> 
> Yes.  Schema pattern:
> 
>      { 'union: ...,
>        'base': {
>            'target': 'SysEmuTarget',
>            ... target-independent data ... }
>        'data': {
>            T: { ... data for target T ... }
>            ... } }
> 
> On the wire:
> 
>      {"target": T,
>       ... target-independent data ...
>       ... data for target T}
> 
> This is for returning info on a single target.
> 
> Typically, a qemu-system-T will only ever return branch T.  The other
> branches are dead code.  Tolerable.
> 
> With qemu-system-any, which provides multiple (eventually all, we hope)
> targets, the union still has data for just one target.  To get data for
> multiple targets, we'd have to return several objects.
> 
> Fine for query-cpus-fast: it returns a CpuInfoFast per CPU, and each of
> them uses the branch T appropriate for the CPU.
> 
>> To allow for qemu-system-any, which will eventually have multiple arches in
>> one,
> 
> Actually, qemu-system-any can work almost exactly like the existing
> qemu-system-T, until we mix in heterogeneous machines.
> 
> Without heterogeneous machines, qemu-system-any's target T gets fixed at
> some point.  From that point on, no difference to qemu-system-T.
> 
> The "almost" is for running commands before that point.
> 
> This applies to all commands that take the target as implicit input.  I
> believe several exist.
> 
> How accelerators will work for heterogeneous machines is not clear to
> me.  Until it is, I can't really advise on how to design accelerator
> information for them.
> 
>>       I guess we'll need multiple levels of nesting. Perhaps  something like
>> this:
>>
>>    { 'enum': 'AccelType',
>>      'data': ['tcg', 'kvm', ....] }
>>
>>    { 'union': 'AccelInfo',
>>      'type': 'AccelType',
>>      'data': {
>>          'tcg': 'AccelInfoTCG',
>> 	'kvm': 'AccelInfoKVM',
>>      } }
>>
>>    { 'struct': 'AccelInfoTCGX86',
>>      'data': {
>>          'notify-vmexit': ...
>>      } }
>>
>>    { 'struct': 'AccelInfoTCGArch',
>>      'data': {
>>         'x86': 'AccelInfoTCGX86',
>>         'riscv': 'AccelInfoTCGRiscV',
>>         ...etc...
>>      }
>>
>>    { 'struct': 'AccelInfoTCG',
>>      'data': {
>>           ...non-arch specific fields...,
>> 	 'arch': 'AccelInfoTCGArch',
>>      } }
> 
> Here, you want to return data for all targets, not just one.  Instead of
> a sum type (union branches), you use a product type (struct members).
> Feels fair.
> 
>>   ...equiv AccelInfoKVM* structs....
> 
> The only target that can actually work is the one matching the host
> hardware.  Perhaps we want to return just that.
> 
>> Markus:  any other/better ideas ?
> 
> I guess there are other ways to skin this cat.  But I think there are
> questions to ask before we delve deeper:
> 
> 1. query-accelerator use cases?  What accelerator information exactly do
> we need to provide for them?

My original motivation with this API was to expose arch specific attributes
to the guest, most notably RISC-V 'riscv-aia' properties, that are defined by
RISC-V kvm_arch_accel_class_init().

In fact the very first impl I did was just a list of properties that each arch
would want to advertise, like query-cpu-model-expansion does, and then each
arch would be responsible to fill in whatever they want to advertise. I decided
to go with this current approach instead because it would bring more discussions
to the table, instead of making a RISC-V specific API that only RISC-V folks
would care about.

Reading all the thread I wonder if we should go back to my original intent and
make the API arch specific and let every arch define what they want to expose.
Seems simpler than to make a catch all API for all archs. Each arch would be
responsible for the stability of their schema and so on.

We're still adding RISC-V specific properties in the KVM accel, so for now I
wouldn't mind implementing this API for x86 first just to validate the concept.
Granted, I have no idea what x86 would like to expose from the KVM accel (or any
other accel for that matter), so if someone could give me a list on what x86
would like to expose that would be terrific.


Thanks,

Daniel




> 
> 2. How fancy do we want to get for homogeneous qemu-system-any before
> the target gets fixed?
> 
> 3. How will accelerators work for heterogeneous qemu-system-any?
> 
> [...]
> 

On 9/27/24 7:50 AM, Daniel P. Berrangé wrote:
> Markus: QAPI design Qs for you at the bottom
> 
> On Wed, Sep 25, 2024 at 10:19:33AM -0300, Daniel Henrique Barboza wrote:
>>
>>
>> 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.
> 
> Well with qemu-system-any we might get multiple arches reporting
> info in the same binary, so we'll need to fan out to fill in the
> per-arch info, after doing a common base.
> 
> Hmmm, i wonder if qemu-system-any will support mixing KVM and TCG ?
> ie KVM for the host native accelerator, combined with TCG for the
> foreign archs ??? Hopefully not !

If you're talking about Phil's patches it seems that it'll be TCG only:

https://lore.kernel.org/qemu-devel/20240305220938.85410-1-philmd@linaro.org/

Patch 2 commit msg states:

------
Add the 'any'-architecture target.

- Only consider 64-bit targets
- Do not use any hardware accelerator (except qtest)
- For architecture constants, use:
   . max of supported targets phys/virt address space
   . max of supported targets MMU modes
   . min of supported targets variable page bits
------


Thanks,

Daniel



> 
>>>> 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 <dbarboza@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.
> 
> To allow for qemu-system-any, which will eventually have multiple arches in
> one, I guess we'll need multiple levels of nesting. Perhaps  something like
> this:
> 
>    { 'enum': 'AccelType',
>      'data': ['tcg', 'kvm', ....] }
> 
>    { 'union': 'AccelInfo',
>      'type': 'AccelType',
>      'data': {
>          'tcg': 'AccelInfoTCG',
> 	'kvm': 'AccelInfoKVM',
>      } }
> 
>    { 'struct': 'AccelInfoTCGX86',
>      'data': {
>          'notify-vmexit': ...
>      } }
> 
>    { 'struct': 'AccelInfoTCGArch',
>      'data': {
>         'x86': 'AccelInfoTCGX86',
>         'riscv': 'AccelInfoTCGRiscV',
>         ...etc...
>      }
> 
>    { 'struct': 'AccelInfoTCG',
>      'data': {
>           ...non-arch specific fields...,
> 	 'arch': 'AccelInfoTCGArch',
>      } }
> 
>   ...equiv AccelInfoKVM* structs....
> 
> Markus:  any other/better ideas ?
> 
>>>> +
>>>> +##
>>>> +# @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' }
>>>> -- 
> 
> With regards,
> Daniel

On 2/10/24 20:06, Daniel Henrique Barboza wrote:
> 
> 
> On 9/27/24 7:50 AM, Daniel P. Berrangé wrote:
>> Markus: QAPI design Qs for you at the bottom
>>
>> On Wed, Sep 25, 2024 at 10:19:33AM -0300, Daniel Henrique Barboza wrote:
>>>
>>>
>>> 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.
>>
>> Well with qemu-system-any we might get multiple arches reporting
>> info in the same binary, so we'll need to fan out to fill in the
>> per-arch info, after doing a common base.
>>
>> Hmmm, i wonder if qemu-system-any will support mixing KVM and TCG ?
>> ie KVM for the host native accelerator, combined with TCG for the
>> foreign archs ??? Hopefully not !
> 
> If you're talking about Phil's patches it seems that it'll be TCG only:

Orthogonaly to the single binary series (mentioned below with the
'any'-architecture target), we plan to add a "Dual HW/SW accelerator".
It will be implemented using the same AccelOps structure we currently
have, and internally it will dispatch between HW/SW. First prototype
plan to provide HVF+TCG.

IMHO query-accelerator should expose accelerator generic properties,
not the target-specific ones. At least by default, we could add an
optional flag to include target-specific stuff. At least that'd keep
the core accel core simpler.

> https://lore.kernel.org/qemu-devel/20240305220938.85410-1-philmd@linaro.org/
> 
> Patch 2 commit msg states:
> 
> ------
> Add the 'any'-architecture target.
> 
> - Only consider 64-bit targets
> - Do not use any hardware accelerator (except qtest)
> - For architecture constants, use:
>    . max of supported targets phys/virt address space
>    . max of supported targets MMU modes
>    . min of supported targets variable page bits
> ------
> 
> 
> Thanks,
> 
> Daniel