Introduce the 'query-accels' QMP command which returns a list
of built-in accelerator names.

- Accelerator is a QAPI enum of all existing accelerators,

- AcceleratorInfo is a QAPI structure providing accelerator
  specific information. Currently the common structure base
  provides the name of the accelerator, while the specific
  part is empty, but each accelerator can expand it.

- 'query-accels' QMP command returns a list of @AcceleratorInfo

For example on a KVM-only build we get:

    { "execute": "query-accels" }
    {
        "return": [
            {
                "name": "qtest"
            },
            {
                "name": "kvm"
            }
        ]
    }

Note that we can't make the enum values or union branches conditional
because of target-specific poisoning of accelerator definitions.

Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Tested-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 qapi/machine.json | 47 +++++++++++++++++++++++++++++++++++++++++++++
 accel/accel-qmp.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++
 accel/meson.build |  2 +-
 3 files changed, 97 insertions(+), 1 deletion(-)
 create mode 100644 accel/accel-qmp.c

diff --git a/qapi/machine.json b/qapi/machine.json
index 6e90d463fc9..6dd3b765248 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1274,3 +1274,50 @@
 ##
 { 'event': 'MEM_UNPLUG_ERROR',
   'data': { 'device': 'str', 'msg': 'str' } }
+
+##
+# @Accelerator:
+#
+# An enumeration of accelerator names.
+#
+# Since: 6.1
+##
+{ 'enum': 'Accelerator',
+  'data': [ 'hax', 'hvf', 'kvm', 'qtest', 'tcg', 'whpx', 'xen' ] }
+
+##
+# @AcceleratorInfo:
+#
+# Accelerator information.
+#
+# @name: The accelerator name.
+#
+# Since: 6.1
+##
+{ 'struct': 'AcceleratorInfo',
+  'data': { 'name': 'Accelerator' } }
+
+##
+# @query-accels:
+#
+# Get a list of AcceleratorInfo for all built-in accelerators.
+#
+# Returns: a list of @AcceleratorInfo describing each accelerator.
+#
+# Since: 6.1
+#
+# Example:
+#
+# -> { "execute": "query-accels" }
+# <- { "return": [
+#        {
+#            "name": "qtest"
+#        },
+#        {
+#            "name": "kvm"
+#        }
+#    ] }
+#
+##
+{ 'command': 'query-accels',
+  'returns': ['AcceleratorInfo'] }
diff --git a/accel/accel-qmp.c b/accel/accel-qmp.c
new file mode 100644
index 00000000000..426737b3f9a
--- /dev/null
+++ b/accel/accel-qmp.c
@@ -0,0 +1,49 @@
+/*
+ * QEMU accelerators, QMP commands
+ *
+ * Copyright (c) 2021 Red Hat Inc.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/qapi-commands-machine.h"
+
+static const bool accel_builtin_list[ACCELERATOR__MAX] = {
+    [ACCELERATOR_QTEST] = true,
+#ifdef CONFIG_TCG
+    [ACCELERATOR_TCG] = true,
+#endif
+#ifdef CONFIG_KVM
+    [ACCELERATOR_KVM] = true,
+#endif
+#ifdef CONFIG_HAX
+    [ACCELERATOR_HAX] = true,
+#endif
+#ifdef CONFIG_HVF
+    [ACCELERATOR_HVF] = true,
+#endif
+#ifdef CONFIG_WHPX
+    [ACCELERATOR_WHPX] = true,
+#endif
+#ifdef CONFIG_XEN_BACKEND
+    [ACCELERATOR_XEN] = true,
+#endif
+};
+
+AcceleratorInfoList *qmp_query_accels(Error **errp)
+{
+    AcceleratorInfoList *list = NULL, **tail = &list;
+
+    for (Accelerator accel = 0; accel < ACCELERATOR__MAX; accel++) {
+        if (accel_builtin_list[accel]) {
+            AcceleratorInfo *info = g_new0(AcceleratorInfo, 1);
+
+            info->name = accel;
+
+            QAPI_LIST_APPEND(tail, info);
+        }
+    }
+
+    return list;
+}
diff --git a/accel/meson.build b/accel/meson.build
index b44ba30c864..7a48f6d568d 100644
--- a/accel/meson.build
+++ b/accel/meson.build
@@ -1,4 +1,4 @@
-specific_ss.add(files('accel-common.c'))
+specific_ss.add(files('accel-common.c', 'accel-qmp.c'))
 softmmu_ss.add(files('accel-softmmu.c'))
 user_ss.add(files('accel-user.c'))
 
-- 
2.26.3

On Wed, May 05, 2021 at 02:57:56PM +0200, Philippe Mathieu-Daudé wrote:
> Introduce the 'query-accels' QMP command which returns a list
> of built-in accelerator names.
> 
> - Accelerator is a QAPI enum of all existing accelerators,
> 
> - AcceleratorInfo is a QAPI structure providing accelerator
>   specific information. Currently the common structure base
>   provides the name of the accelerator, while the specific
>   part is empty, but each accelerator can expand it.
> 
> - 'query-accels' QMP command returns a list of @AcceleratorInfo
> 
> For example on a KVM-only build we get:
> 
>     { "execute": "query-accels" }
>     {
>         "return": [
>             {
>                 "name": "qtest"
>             },
>             {
>                 "name": "kvm"
>             }
>         ]
>     }
> 
> Note that we can't make the enum values or union branches conditional
> because of target-specific poisoning of accelerator definitions.

I assume this will be clarified in v8, based on your reply to v5.
I don't understand what "target-specific poisoning of accelerator
definitions" means.



> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> Tested-by: Alex Bennée <alex.bennee@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Sorry for not even reviewing this before, but still:

Acked-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

On 5/5/21 9:41 PM, Eduardo Habkost wrote:
> On Wed, May 05, 2021 at 02:57:56PM +0200, Philippe Mathieu-Daudé wrote:
>> Introduce the 'query-accels' QMP command which returns a list
>> of built-in accelerator names.
>>
>> - Accelerator is a QAPI enum of all existing accelerators,
>>
>> - AcceleratorInfo is a QAPI structure providing accelerator
>>   specific information. Currently the common structure base
>>   provides the name of the accelerator, while the specific
>>   part is empty, but each accelerator can expand it.
>>
>> - 'query-accels' QMP command returns a list of @AcceleratorInfo
>>
>> For example on a KVM-only build we get:
>>
>>     { "execute": "query-accels" }
>>     {
>>         "return": [
>>             {
>>                 "name": "qtest"
>>             },
>>             {
>>                 "name": "kvm"
>>             }
>>         ]
>>     }
>>
>> Note that we can't make the enum values or union branches conditional
>> because of target-specific poisoning of accelerator definitions.
> 
> I assume this will be clarified in v8, based on your reply to v5.
> I don't understand what "target-specific poisoning of accelerator
> definitions" means.

$ git grep poison
...
include/exec/poison.h:88:#pragma GCC poison CONFIG_HAX
include/exec/poison.h:89:#pragma GCC poison CONFIG_HVF
include/exec/poison.h:90:#pragma GCC poison CONFIG_LINUX_USER
include/exec/poison.h:91:#pragma GCC poison CONFIG_KVM
include/exec/poison.h:92:#pragma GCC poison CONFIG_SOFTMMU
include/exec/poison.h:93:#pragma GCC poison CONFIG_WHPX
include/exec/poison.h:94:#pragma GCC poison CONFIG_XEN

I thought QAPI was target agnostic, but I just found:

  if module.endswith('-target')
    qapi_specific_outputs += qapi_module_outputs

  specific_ss.add(when: 'CONFIG_SOFTMMU', if_true: qapi_files[i])

So instead of adding this to qapi/machine.json I'll see
if I can add it to qapi/machine-target.json and use
conditionals.

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> Introduce the 'query-accels' QMP command which returns a list
> of built-in accelerator names.
>
> - Accelerator is a QAPI enum of all existing accelerators,
>
> - AcceleratorInfo is a QAPI structure providing accelerator
>   specific information. Currently the common structure base
>   provides the name of the accelerator, while the specific
>   part is empty, but each accelerator can expand it.
>
> - 'query-accels' QMP command returns a list of @AcceleratorInfo
>
> For example on a KVM-only build we get:
>
>     { "execute": "query-accels" }
>     {
>         "return": [
>             {
>                 "name": "qtest"
>             },
>             {
>                 "name": "kvm"
>             }
>         ]
>     }
>
> Note that we can't make the enum values or union branches conditional
> because of target-specific poisoning of accelerator definitions.

I second Eduardo's plea to explain this more clearly.  It's important,
because if a properly conditionalized enum is feasible, then query-accel
isn't needed.

> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> Tested-by: Alex Bennée <alex.bennee@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  qapi/machine.json | 47 +++++++++++++++++++++++++++++++++++++++++++++
>  accel/accel-qmp.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++
>  accel/meson.build |  2 +-
>  3 files changed, 97 insertions(+), 1 deletion(-)
>  create mode 100644 accel/accel-qmp.c
>
> diff --git a/qapi/machine.json b/qapi/machine.json
> index 6e90d463fc9..6dd3b765248 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -1274,3 +1274,50 @@
>  ##
>  { 'event': 'MEM_UNPLUG_ERROR',
>    'data': { 'device': 'str', 'msg': 'str' } }
> +
> +##
> +# @Accelerator:
> +#
> +# An enumeration of accelerator names.
> +#
> +# Since: 6.1
> +##
> +{ 'enum': 'Accelerator',
> +  'data': [ 'hax', 'hvf', 'kvm', 'qtest', 'tcg', 'whpx', 'xen' ] }
> +
> +##
> +# @AcceleratorInfo:
> +#
> +# Accelerator information.
> +#
> +# @name: The accelerator name.
> +#
> +# Since: 6.1
> +##
> +{ 'struct': 'AcceleratorInfo',
> +  'data': { 'name': 'Accelerator' } }
> +
> +##
> +# @query-accels:
> +#
> +# Get a list of AcceleratorInfo for all built-in accelerators.

"built-in" means compiled in.  See accel_builtin_list[] below.

> +#
> +# Returns: a list of @AcceleratorInfo describing each accelerator.
> +#
> +# Since: 6.1
> +#
> +# Example:
> +#
> +# -> { "execute": "query-accels" }
> +# <- { "return": [
> +#        {
> +#            "name": "qtest"
> +#        },
> +#        {
> +#            "name": "kvm"
> +#        }
> +#    ] }
> +#
> +##
> +{ 'command': 'query-accels',
> +  'returns': ['AcceleratorInfo'] }
> diff --git a/accel/accel-qmp.c b/accel/accel-qmp.c
> new file mode 100644
> index 00000000000..426737b3f9a
> --- /dev/null
> +++ b/accel/accel-qmp.c
> @@ -0,0 +1,49 @@
> +/*
> + * QEMU accelerators, QMP commands
> + *
> + * Copyright (c) 2021 Red Hat Inc.
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/qapi-commands-machine.h"
> +
> +static const bool accel_builtin_list[ACCELERATOR__MAX] = {
> +    [ACCELERATOR_QTEST] = true,
> +#ifdef CONFIG_TCG
> +    [ACCELERATOR_TCG] = true,
> +#endif
> +#ifdef CONFIG_KVM
> +    [ACCELERATOR_KVM] = true,
> +#endif
> +#ifdef CONFIG_HAX
> +    [ACCELERATOR_HAX] = true,
> +#endif
> +#ifdef CONFIG_HVF
> +    [ACCELERATOR_HVF] = true,
> +#endif
> +#ifdef CONFIG_WHPX
> +    [ACCELERATOR_WHPX] = true,
> +#endif
> +#ifdef CONFIG_XEN_BACKEND
> +    [ACCELERATOR_XEN] = true,
> +#endif
> +};
> +
> +AcceleratorInfoList *qmp_query_accels(Error **errp)
> +{
> +    AcceleratorInfoList *list = NULL, **tail = &list;
> +
> +    for (Accelerator accel = 0; accel < ACCELERATOR__MAX; accel++) {
> +        if (accel_builtin_list[accel]) {
> +            AcceleratorInfo *info = g_new0(AcceleratorInfo, 1);
> +
> +            info->name = accel;
> +
> +            QAPI_LIST_APPEND(tail, info);
> +        }
> +    }
> +
> +    return list;
> +}

CLI -accel help also lists the available accelerators, but finds them
differently.  Whereas query-accel relies on hard-coded
accel_builtin_list[], that one instead finds the concrete subtypes of
TYPE_ACCEL.

Any particular reason for the difference?

> diff --git a/accel/meson.build b/accel/meson.build
> index b44ba30c864..7a48f6d568d 100644
> --- a/accel/meson.build
> +++ b/accel/meson.build
> @@ -1,4 +1,4 @@
> -specific_ss.add(files('accel-common.c'))
> +specific_ss.add(files('accel-common.c', 'accel-qmp.c'))
>  softmmu_ss.add(files('accel-softmmu.c'))
>  user_ss.add(files('accel-user.c'))

Markus Armbruster <armbru@redhat.com> writes:

> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>
>> Introduce the 'query-accels' QMP command which returns a list
>> of built-in accelerator names.
>>
>> - Accelerator is a QAPI enum of all existing accelerators,
>>
>> - AcceleratorInfo is a QAPI structure providing accelerator
>>   specific information. Currently the common structure base
>>   provides the name of the accelerator, while the specific
>>   part is empty, but each accelerator can expand it.
>>
>> - 'query-accels' QMP command returns a list of @AcceleratorInfo
>>
>> For example on a KVM-only build we get:
>>
>>     { "execute": "query-accels" }
>>     {
>>         "return": [
>>             {
>>                 "name": "qtest"
>>             },
>>             {
>>                 "name": "kvm"
>>             }
>>         ]
>>     }
>>
>> Note that we can't make the enum values or union branches conditional

"union branches" existed in a previous iteration, but no more.

>> because of target-specific poisoning of accelerator definitions.
>
> I second Eduardo's plea to explain this more clearly.  It's important,
> because if a properly conditionalized enum is feasible, then query-accel
> isn't needed.

The appended incremental patch conditionalizes the enum.  It applies on
top of the series, and passes "make check" for me.  Seems to contradict
"we can't make the enum values conditional".


diff --git a/qapi/machine-target.json b/qapi/machine-target.json
index e7811654b7..586a61b5d9 100644
--- a/qapi/machine-target.json
+++ b/qapi/machine-target.json
@@ -329,3 +329,57 @@
 ##
 { 'command': 'query-cpu-definitions', 'returns': ['CpuDefinitionInfo'],
   'if': 'defined(TARGET_PPC) || defined(TARGET_ARM) || defined(TARGET_I386) || defined(TARGET_S390X) || defined(TARGET_MIPS)' }
+
+##
+# @Accelerator:
+#
+# An enumeration of accelerator names.
+#
+# Since: 6.1
+##
+{ 'enum': 'Accelerator',
+  'data': [
+      { 'name': 'hax', 'if': 'defined(CONFIG_HAX)' },
+      { 'name': 'hvf', 'if': 'defined(CONFIG_HVF)' },
+      { 'name': 'kvm', 'if': 'defined(CONFIG_KVM)' },
+      { 'name': 'qtest' },
+      { 'name': 'tcg', 'if': 'defined(CONFIG_TCG)' },
+      { 'name': 'whpx', 'if': 'defined(CONFIG_WHPX)' },
+      { 'name': 'xen', 'if': 'defined(CONFIG_XEN_BACKEND)' } ] }
+
+##
+# @AcceleratorInfo:
+#
+# Accelerator information.
+#
+# @name: The accelerator name.
+#
+# Since: 6.1
+##
+{ 'struct': 'AcceleratorInfo',
+  'data': { 'name': 'Accelerator' } }
+
+##
+# @query-accels:
+#
+# Get a list of AcceleratorInfo for all built-in accelerators.
+#
+# Returns: a list of @AcceleratorInfo describing each accelerator.
+#
+# Since: 6.1
+#
+# Example:
+#
+# -> { "execute": "query-accels" }
+# <- { "return": [
+#        {
+#            "name": "qtest"
+#        },
+#        {
+#            "name": "kvm"
+#        }
+#    ] }
+#
+##
+{ 'command': 'query-accels',
+  'returns': ['AcceleratorInfo'] }
diff --git a/qapi/machine.json b/qapi/machine.json
index 79a0891793..58a9c86b36 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1274,50 +1274,3 @@
 ##
 { 'event': 'MEM_UNPLUG_ERROR',
   'data': { 'device': 'str', 'msg': 'str' } }
-
-##
-# @Accelerator:
-#
-# An enumeration of accelerator names.
-#
-# Since: 6.1
-##
-{ 'enum': 'Accelerator',
-  'data': [ 'hax', 'hvf', 'kvm', 'qtest', 'tcg', 'whpx', 'xen' ] }
-
-##
-# @AcceleratorInfo:
-#
-# Accelerator information.
-#
-# @name: The accelerator name.
-#
-# Since: 6.1
-##
-{ 'struct': 'AcceleratorInfo',
-  'data': { 'name': 'Accelerator' } }
-
-##
-# @query-accels:
-#
-# Get a list of AcceleratorInfo for all built-in accelerators.
-#
-# Returns: a list of @AcceleratorInfo describing each accelerator.
-#
-# Since: 6.1
-#
-# Example:
-#
-# -> { "execute": "query-accels" }
-# <- { "return": [
-#        {
-#            "name": "qtest"
-#        },
-#        {
-#            "name": "kvm"
-#        }
-#    ] }
-#
-##
-{ 'command': 'query-accels',
-  'returns': ['AcceleratorInfo'] }
diff --git a/accel/accel-qmp.c b/accel/accel-qmp.c
index 426737b3f9..aca90f8682 100644
--- a/accel/accel-qmp.c
+++ b/accel/accel-qmp.c
@@ -7,42 +7,18 @@
  */
 
 #include "qemu/osdep.h"
-#include "qapi/qapi-commands-machine.h"
-
-static const bool accel_builtin_list[ACCELERATOR__MAX] = {
-    [ACCELERATOR_QTEST] = true,
-#ifdef CONFIG_TCG
-    [ACCELERATOR_TCG] = true,
-#endif
-#ifdef CONFIG_KVM
-    [ACCELERATOR_KVM] = true,
-#endif
-#ifdef CONFIG_HAX
-    [ACCELERATOR_HAX] = true,
-#endif
-#ifdef CONFIG_HVF
-    [ACCELERATOR_HVF] = true,
-#endif
-#ifdef CONFIG_WHPX
-    [ACCELERATOR_WHPX] = true,
-#endif
-#ifdef CONFIG_XEN_BACKEND
-    [ACCELERATOR_XEN] = true,
-#endif
-};
+#include "qapi/qapi-commands-machine-target.h"
 
 AcceleratorInfoList *qmp_query_accels(Error **errp)
 {
     AcceleratorInfoList *list = NULL, **tail = &list;
 
     for (Accelerator accel = 0; accel < ACCELERATOR__MAX; accel++) {
-        if (accel_builtin_list[accel]) {
-            AcceleratorInfo *info = g_new0(AcceleratorInfo, 1);
+        AcceleratorInfo *info = g_new0(AcceleratorInfo, 1);
 
-            info->name = accel;
+        info->name = accel;
 
-            QAPI_LIST_APPEND(tail, info);
-        }
+        QAPI_LIST_APPEND(tail, info);
     }
 
     return list;

Markus Armbruster <armbru@redhat.com> writes:

> The appended incremental patch conditionalizes the enum.  It applies on
> top of the series, and passes "make check" for me.  Seems to contradict
> "we can't make the enum values conditional".

Neglected to mention that query-accel becomes almost useless in this
version: we need it just to pull enum Accelerator into the QMP
interface, so query-qmp-schema shows it.

It may become a useful command if we add more members to AcceleratorInfo
later.