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

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