[PATCH 0/6] qtests: Check accelerator available at runtime via QMP 'query-accels'

[PATCH 0/6] qtests: Check accelerator available at runtime via QMP 'query-accels'

[Philippe Mathieu-Daudé]

Hi,

This series aims at having accelerator-independent qtests
by querying a QEMU instance at runtime to check the list
of built-in accelerators.

First we add the 'query-accels' QMP command,
then we add the qtest_probe_accel() method to libqtest,
finally we use this new method to allow running
bios-tables-test and arm-cpu-features on KVM-only builds.

Please review,

Phil.

Philippe Mathieu-Daudé (6):
  accel: Introduce 'query-accels' QMP command
  tests/qtest: Add qtest_probe_accel() method
  qtest/bios-tables-test: Make test build-independent from accelerator
  qtest/arm-cpu-features: Check KVM availability at runtime
  qtest/arm-cpu-features: Check TCG availability at runtime
  tests/qtest: Do not restrict bios-tables-test to Aarch64 hosts anymore

 qapi/machine.json              | 55 ++++++++++++++++++++++++++++++++++
 tests/qtest/libqos/libqtest.h  |  9 ++++++
 accel/accel-qmp.c              | 47 +++++++++++++++++++++++++++++
 tests/qtest/arm-cpu-features.c | 35 ++++++++++++++++++----
 tests/qtest/bios-tables-test.c | 13 ++++----
 tests/qtest/libqtest.c         | 24 +++++++++++++++
 accel/meson.build              |  2 +-
 tests/qtest/meson.build        |  3 +-
 8 files changed, 173 insertions(+), 15 deletions(-)
 create mode 100644 accel/accel-qmp.c

-- 
2.26.2

[PATCH 1/6] accel: Introduce 'query-accels' QMP command

[Philippe Mathieu-Daudé]

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

- Accelerator is an 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": [
            {
                "type": "qtest"
            },
            {
                "type": "kvm"
            }
        ]
    }

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 qapi/machine.json | 55 +++++++++++++++++++++++++++++++++++++++++++++++
 accel/accel-qmp.c | 47 ++++++++++++++++++++++++++++++++++++++++
 accel/meson.build |  2 +-
 3 files changed, 103 insertions(+), 1 deletion(-)
 create mode 100644 accel/accel-qmp.c

diff --git a/qapi/machine.json b/qapi/machine.json
index 330189efe3d..ffbf28e5d50 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1471,3 +1471,58 @@
 ##
 { 'event': 'MEM_UNPLUG_ERROR',
   'data': { 'device': 'str', 'msg': 'str' } }
+
+##
+# @Accelerator:
+#
+# An enumeration of accelerator names.
+#
+# Since: 6.0
+##
+{ 'enum': 'Accelerator',
+  'data': [ { 'name': 'qtest' },
+            { 'name': 'tcg' },
+            { 'name': 'kvm' },
+            { 'name': 'hax' },
+            { 'name': 'hvf' },
+            { 'name': 'whpx' },
+            { 'name': 'xen' } ] }
+
+##
+# @AcceleratorInfo:
+#
+# Accelerator information.
+#
+# @name: The accelerator name.
+#
+# Since: 6.0
+##
+{ 'union': 'AcceleratorInfo',
+  'base': {'name': 'Accelerator'},
+  'discriminator': 'name',
+  'data': { } }
+
+##
+# @query-accels:
+#
+# Get a list of AcceleratorInfo for all built-in accelerators.
+#
+# Returns: a list of @AcceleratorInfo describing each accelerator.
+#
+# Since: 6.0
+#
+# Example:
+#
+# -> { "execute": "query-accels" }
+# <- { "return": [
+#        {
+#            "type": "qtest"
+#        },
+#        {
+#            "type": "kvm"
+#        }
+#    ] }
+#
+##
+{ 'command': 'query-accels',
+  'returns': ['AcceleratorInfo'] }
diff --git a/accel/accel-qmp.c b/accel/accel-qmp.c
new file mode 100644
index 00000000000..f16e49b8956
--- /dev/null
+++ b/accel/accel-qmp.c
@@ -0,0 +1,47 @@
+/*
+ * 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 Accelerator accel_list[] = {
+    ACCELERATOR_QTEST,
+#ifdef CONFIG_TCG
+    ACCELERATOR_TCG,
+#endif
+#ifdef CONFIG_KVM
+    ACCELERATOR_KVM,
+#endif
+#ifdef CONFIG_HAX
+    ACCELERATOR_HAX,
+#endif
+#ifdef CONFIG_HVF
+    ACCELERATOR_HVF,
+#endif
+#ifdef CONFIG_WHPX
+    ACCELERATOR_WHPX,
+#endif
+#ifdef CONFIG_XEN_BACKEND
+    ACCELERATOR_XEN,
+#endif
+};
+
+AcceleratorInfoList *qmp_query_accels(Error **errp)
+{
+    AcceleratorInfoList *list = NULL, **tail = &list;
+
+    for (unsigned i = 0; i < ARRAY_SIZE(accel_list); i++) {
+        AcceleratorInfo *info = g_new0(AcceleratorInfo, 1);
+
+        info->name = accel_list[i];
+
+        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.2

[PATCH 2/6] tests/qtest: Add qtest_probe_accel() method

[Philippe Mathieu-Daudé]

Introduce the qtest_probe_accel() method which allows
to query at runtime if a QEMU instance has an accelerator
built-in.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 tests/qtest/libqos/libqtest.h |  9 +++++++++
 tests/qtest/libqtest.c        | 24 ++++++++++++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/tests/qtest/libqos/libqtest.h b/tests/qtest/libqos/libqtest.h
index a68dcd79d44..ebedb82ec98 100644
--- a/tests/qtest/libqos/libqtest.h
+++ b/tests/qtest/libqos/libqtest.h
@@ -763,6 +763,15 @@ void qmp_expect_error_and_unref(QDict *rsp, const char *class);
  */
 bool qtest_probe_child(QTestState *s);
 
+/**
+ * qtest_probe_accel:
+ * @s: QTestState instance to operate on.
+ * @name: Accelerator name to check for.
+ *
+ * Returns: true if the accelerator is built in.
+ */
+bool qtest_probe_accel(QTestState *s, const char *name);
+
 /**
  * qtest_set_expected_status:
  * @s: QTestState instance to operate on.
diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index 71e359efcd3..57e7e55b9cc 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -872,6 +872,30 @@ void qtest_qmp_eventwait(QTestState *s, const char *event)
     qobject_unref(response);
 }
 
+bool qtest_probe_accel(QTestState *s, const char *name)
+{
+    bool has_accel = false;
+    QDict *response;
+    QList *accels;
+    QListEntry *accel;
+
+    response = qtest_qmp(s, "{'execute': 'query-accels'}");
+    accels = qdict_get_qlist(response, "return");
+
+    QLIST_FOREACH_ENTRY(accels, accel) {
+        QDict *accel_dict = qobject_to(QDict, qlist_entry_obj(accel));
+        const char *accel_name = qdict_get_str(accel_dict, "name");
+
+        if (!strcmp(name, accel_name)) {
+            has_accel = true;
+            break;
+        }
+    }
+    qobject_unref(response);
+
+    return has_accel;
+}
+
 char *qtest_vhmp(QTestState *s, const char *fmt, va_list ap)
 {
     char *cmd;
-- 
2.26.2

[PATCH 3/6] qtest/bios-tables-test: Make test build-independent from accelerator

[Philippe Mathieu-Daudé]

Now than we can probe if the TCG accelerator is available
at runtime with a QMP command, we can remove the #ifdef'ry.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 tests/qtest/bios-tables-test.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
index e020c83d2a5..73378f9da94 100644
--- a/tests/qtest/bios-tables-test.c
+++ b/tests/qtest/bios-tables-test.c
@@ -718,15 +718,13 @@ static void test_acpi_one(const char *params, test_data *data)
     char *args;
     bool use_uefi = data->uefi_fl1 && data->uefi_fl2;
 
-#ifndef CONFIG_TCG
-    if (data->tcg_only) {
-        g_test_skip("TCG disabled, skipping ACPI tcg_only test");
-        return;
-    }
-#endif /* CONFIG_TCG */
-
     args = test_acpi_create_args(data, params, use_uefi);
     data->qts = qtest_init(args);
+    if (data->tcg_only && !qtest_probe_accel(data->qts, "tcg")) {
+        g_test_skip("TCG not available, skipping test");
+        goto done;
+    }
+
     test_acpi_load_tables(data, use_uefi);
 
     if (getenv(ACPI_REBUILD_EXPECTED_AML)) {
@@ -745,6 +743,7 @@ static void test_acpi_one(const char *params, test_data *data)
         test_smbios_structs(data);
     }
 
+done:
     qtest_quit(data->qts);
     g_free(args);
 }
-- 
2.26.2

[PATCH 4/6] qtest/arm-cpu-features: Check KVM availability at runtime

[Philippe Mathieu-Daudé]

sve_tests_sve_off_kvm() and test_query_cpu_model_expansion_kvm()
are KVM specific tests. Skip them if KVM is not built-in.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 tests/qtest/arm-cpu-features.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/tests/qtest/arm-cpu-features.c b/tests/qtest/arm-cpu-features.c
index 8252b85bb85..2b70104515d 100644
--- a/tests/qtest/arm-cpu-features.c
+++ b/tests/qtest/arm-cpu-features.c
@@ -21,7 +21,7 @@
 #define SVE_MAX_VQ 16
 
 #define MACHINE     "-machine virt,gic-version=max -accel tcg "
-#define MACHINE_KVM "-machine virt,gic-version=max -accel kvm -accel tcg "
+#define MACHINE_KVM "-machine virt,gic-version=max -accel kvm "
 #define QUERY_HEAD  "{ 'execute': 'query-cpu-model-expansion', " \
                     "  'arguments': { 'type': 'full', "
 #define QUERY_TAIL  "}}"
@@ -413,6 +413,10 @@ static void sve_tests_sve_off_kvm(const void *data)
     QTestState *qts;
 
     qts = qtest_init(MACHINE_KVM "-cpu max,sve=off");
+    if (!qtest_probe_accel(qts, "kvm")) {
+        g_test_skip("KVM not available, skipping test");
+        goto done;
+    }
 
     /*
      * We don't know if this host supports SVE so we don't
@@ -424,6 +428,7 @@ static void sve_tests_sve_off_kvm(const void *data)
     assert_sve_vls(qts, "max", 0, NULL);
     assert_sve_vls(qts, "max", 0, "{ 'sve128': false }");
 
+done:
     qtest_quit(qts);
 }
 
@@ -492,6 +497,10 @@ static void test_query_cpu_model_expansion_kvm(const void *data)
     QTestState *qts;
 
     qts = qtest_init(MACHINE_KVM "-cpu max");
+    if (!qtest_probe_accel(qts, "kvm")) {
+        g_test_skip("KVM not available, skipping test");
+        goto done;
+    }
 
     /*
      * These tests target the 'host' CPU type, so KVM must be enabled.
@@ -609,6 +618,7 @@ static void test_query_cpu_model_expansion_kvm(const void *data)
         assert_has_not_feature(qts, "host", "kvm-steal-time");
     }
 
+done:
     qtest_quit(qts);
 }
 
-- 
2.26.2

[PATCH 5/6] qtest/arm-cpu-features: Check TCG availability at runtime

[Philippe Mathieu-Daudé]

Now than we can probe if the TCG accelerator is available
at runtime with a QMP command, skip these TCG specific tests
when TCG is not built into the QEMU binary.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 tests/qtest/arm-cpu-features.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/tests/qtest/arm-cpu-features.c b/tests/qtest/arm-cpu-features.c
index 2b70104515d..7acdccd10ef 100644
--- a/tests/qtest/arm-cpu-features.c
+++ b/tests/qtest/arm-cpu-features.c
@@ -20,7 +20,7 @@
  */
 #define SVE_MAX_VQ 16
 
-#define MACHINE     "-machine virt,gic-version=max -accel tcg "
+#define MACHINE_TCG "-machine virt,gic-version=max -accel tcg "
 #define MACHINE_KVM "-machine virt,gic-version=max -accel kvm "
 #define QUERY_HEAD  "{ 'execute': 'query-cpu-model-expansion', " \
                     "  'arguments': { 'type': 'full', "
@@ -352,7 +352,11 @@ static void sve_tests_sve_max_vq_8(const void *data)
 {
     QTestState *qts;
 
-    qts = qtest_init(MACHINE "-cpu max,sve-max-vq=8");
+    qts = qtest_init(MACHINE_TCG "-cpu max,sve-max-vq=8");
+    if (!qtest_probe_accel(qts, "tcg")) {
+        g_test_skip("TCG not available, skipping test");
+        goto done;
+    }
 
     assert_sve_vls(qts, "max", BIT_ULL(8) - 1, NULL);
 
@@ -380,6 +384,7 @@ static void sve_tests_sve_max_vq_8(const void *data)
     assert_sve_vls(qts, "max", 0xff, "{ 'sve256': true }");
     assert_error(qts, "max", "cannot disable sve256", "{ 'sve256': false }");
 
+done:
     qtest_quit(qts);
 }
 
@@ -387,7 +392,11 @@ static void sve_tests_sve_off(const void *data)
 {
     QTestState *qts;
 
-    qts = qtest_init(MACHINE "-cpu max,sve=off");
+    qts = qtest_init(MACHINE_TCG "-cpu max,sve=off");
+    if (!qtest_probe_accel(qts, "tcg")) {
+        g_test_skip("TCG not available, skipping test");
+        goto done;
+    }
 
     /* SVE is off, so the map should be empty. */
     assert_sve_vls(qts, "max", 0, NULL);
@@ -405,6 +414,7 @@ static void sve_tests_sve_off(const void *data)
     assert_sve_vls(qts, "max", 0x3,
                    "{ 'sve': true, 'sve128': true, 'sve256': true }");
 
+done:
     qtest_quit(qts);
 }
 
@@ -448,7 +458,11 @@ static void test_query_cpu_model_expansion(const void *data)
 {
     QTestState *qts;
 
-    qts = qtest_init(MACHINE "-cpu max");
+    qts = qtest_init(MACHINE_TCG "-cpu max");
+    if (!qtest_probe_accel(qts, "tcg")) {
+        g_test_skip("TCG not available, skipping test");
+        goto done;
+    }
 
     /* Test common query-cpu-model-expansion input validation */
     assert_type_full(qts);
@@ -489,6 +503,7 @@ static void test_query_cpu_model_expansion(const void *data)
                      "{ 'aarch64': false }");
     }
 
+done:
     qtest_quit(qts);
 }
 
-- 
2.26.2

[PATCH 6/6] tests/qtest: Do not restrict bios-tables-test to Aarch64 hosts anymore

[Philippe Mathieu-Daudé]

Since commit 82bf7ae84ce ("target/arm: Remove KVM support for
32-bit Arm hosts") we can remove the comment / check added in
commit ab6b6a77774 and directly run the bios-tables-test.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 tests/qtest/meson.build | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index 2688e1bfad7..405ae7a5602 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -170,14 +170,13 @@
    'boot-serial-test',
    'hexloader-test']
 
-# TODO: once aarch64 TCG is fixed on ARM 32 bit host, make bios-tables-test unconditional
 qtests_aarch64 = \
-  (cpu != 'arm' ? ['bios-tables-test'] : []) +                                                  \
   (config_all_devices.has_key('CONFIG_TPM_TIS_SYSBUS') ? ['tpm-tis-device-test'] : []) +        \
   (config_all_devices.has_key('CONFIG_TPM_TIS_SYSBUS') ? ['tpm-tis-device-swtpm-test'] : []) +  \
   ['arm-cpu-features',
    'numa-test',
    'boot-serial-test',
+   'bios-tables-test',
    'xlnx-can-test',
    'migration-test']
 
-- 
2.26.2

Re: [PATCH 1/6] accel: Introduce 'query-accels' QMP command

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 4481 bytes --]

On Fri, Mar 12, 2021 at 3:14 AM Philippe Mathieu-Daudé <philmd@redhat.com>
wrote:

> Introduce the 'query-accels' QMP command which returns a list
> of built-in accelerators names.
>
> - Accelerator is an 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": [
>             {
>                 "type": "qtest"
>             },
>             {
>                 "type": "kvm"
>             }
>

s/type/name (in this version)

        ]
>     }
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  qapi/machine.json | 55 +++++++++++++++++++++++++++++++++++++++++++++++
>  accel/accel-qmp.c | 47 ++++++++++++++++++++++++++++++++++++++++
>  accel/meson.build |  2 +-
>  3 files changed, 103 insertions(+), 1 deletion(-)
>  create mode 100644 accel/accel-qmp.c
>
> diff --git a/qapi/machine.json b/qapi/machine.json
> index 330189efe3d..ffbf28e5d50 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -1471,3 +1471,58 @@
>  ##
>  { 'event': 'MEM_UNPLUG_ERROR',
>    'data': { 'device': 'str', 'msg': 'str' } }
> +
> +##
> +# @Accelerator:
> +#
> +# An enumeration of accelerator names.
> +#
> +# Since: 6.0
> +##
> +{ 'enum': 'Accelerator',
> +  'data': [ { 'name': 'qtest' },
> +            { 'name': 'tcg' },
> +            { 'name': 'kvm' },
> +            { 'name': 'hax' },
> +            { 'name': 'hvf' },
> +            { 'name': 'whpx' },
> +            { 'name': 'xen' } ] }
> +
>

Why not use a simple enum?

+##
> +# @AcceleratorInfo:
> +#
> +# Accelerator information.
> +#
> +# @name: The accelerator name.
> +#
> +# Since: 6.0
> +##
> +{ 'union': 'AcceleratorInfo',
> +  'base': {'name': 'Accelerator'},
> +  'discriminator': 'name',
> +  'data': { } }
>
+
>

Making room for future details, why not.

+##
> +# @query-accels:
> +#
> +# Get a list of AcceleratorInfo for all built-in accelerators.
> +#
> +# Returns: a list of @AcceleratorInfo describing each accelerator.
> +#
> +# Since: 6.0
> +#
> +# Example:
> +#
> +# -> { "execute": "query-accels" }
> +# <- { "return": [
> +#        {
> +#            "type": "qtest"
> +#        },
> +#        {
> +#            "type": "kvm"
> +#        }
> +#    ] }
> +#
> +##
> +{ 'command': 'query-accels',
> +  'returns': ['AcceleratorInfo'] }
>

That's nice, but how do you know which accels are actually enabled?

diff --git a/accel/accel-qmp.c b/accel/accel-qmp.c
> new file mode 100644
> index 00000000000..f16e49b8956
> --- /dev/null
> +++ b/accel/accel-qmp.c
> @@ -0,0 +1,47 @@
> +/*
> + * 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 Accelerator accel_list[] = {
> +    ACCELERATOR_QTEST,
> +#ifdef CONFIG_TCG
> +    ACCELERATOR_TCG,
> +#endif
> +#ifdef CONFIG_KVM
> +    ACCELERATOR_KVM,
> +#endif
> +#ifdef CONFIG_HAX
> +    ACCELERATOR_HAX,
> +#endif
> +#ifdef CONFIG_HVF
> +    ACCELERATOR_HVF,
> +#endif
> +#ifdef CONFIG_WHPX
> +    ACCELERATOR_WHPX,
> +#endif
> +#ifdef CONFIG_XEN_BACKEND
> +    ACCELERATOR_XEN,
> +#endif
> +};
> +
> +AcceleratorInfoList *qmp_query_accels(Error **errp)
> +{
> +    AcceleratorInfoList *list = NULL, **tail = &list;
> +
> +    for (unsigned i = 0; i < ARRAY_SIZE(accel_list); i++) {
> +        AcceleratorInfo *info = g_new0(AcceleratorInfo, 1);
> +
> +        info->name = accel_list[i];
> +
> +        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.2
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 6759 bytes --]

Re: [PATCH 1/6] accel: Introduce 'query-accels' QMP command

[Thomas Huth]

On 12/03/2021 08.42, Marc-André Lureau wrote:
> 
> 
> On Fri, Mar 12, 2021 at 3:14 AM Philippe Mathieu-Daudé <philmd@redhat.com 
> <mailto:philmd@redhat.com>> wrote:
> 
[...]
>     +##
>     +# @AcceleratorInfo:
>     +#
>     +# Accelerator information.
>     +#
>     +# @name: The accelerator name.
>     +#
>     +# Since: 6.0
>     +##
>     +{ 'union': 'AcceleratorInfo',
>     +  'base': {'name': 'Accelerator'},
>     +  'discriminator': 'name',
>     +  'data': { } }
> 
>     +
> 
> 
> Making room for future details, why not.

I think we definitely need the additional data section here: For KVM on 
POWER, it would be good to know whether it's KVM-HV or KVM-PR, for KVM on 
MIPS it would be good to know whether it's KVM_VM_MIPS_VZ or KVM_VM_MIPS_TE, 
for KVM on x86 whether it's the AMD flavor or Intel, ...

>     +##
>     +# @query-accels:
>     +#
>     +# Get a list of AcceleratorInfo for all built-in accelerators.
>     +#
>     +# Returns: a list of @AcceleratorInfo describing each accelerator.
>     +#
>     +# Since: 6.0
>     +#
>     +# Example:
>     +#
>     +# -> { "execute": "query-accels" }
>     +# <- { "return": [
>     +#        {
>     +#            "type": "qtest"
>     +#        },
>     +#        {
>     +#            "type": "kvm"
>     +#        }
>     +#    ] }
>     +#
>     +##
>     +{ 'command': 'query-accels',
>     +  'returns': ['AcceleratorInfo'] }
> 
> 
> That's nice, but how do you know which accels are actually enabled?

I guess we need two commands in the end, one for querying which accelerators 
are available, and one for querying the data from the currently active 
accelerator...?

  Thomas

Re: [PATCH 2/6] tests/qtest: Add qtest_probe_accel() method

[Thomas Huth]

On 12/03/2021 00.11, Philippe Mathieu-Daudé wrote:
> Introduce the qtest_probe_accel() method which allows
> to query at runtime if a QEMU instance has an accelerator
> built-in.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   tests/qtest/libqos/libqtest.h |  9 +++++++++
>   tests/qtest/libqtest.c        | 24 ++++++++++++++++++++++++
>   2 files changed, 33 insertions(+)
> 
> diff --git a/tests/qtest/libqos/libqtest.h b/tests/qtest/libqos/libqtest.h
> index a68dcd79d44..ebedb82ec98 100644
> --- a/tests/qtest/libqos/libqtest.h
> +++ b/tests/qtest/libqos/libqtest.h
> @@ -763,6 +763,15 @@ void qmp_expect_error_and_unref(QDict *rsp, const char *class);
>    */
>   bool qtest_probe_child(QTestState *s);
>   
> +/**
> + * qtest_probe_accel:
> + * @s: QTestState instance to operate on.
> + * @name: Accelerator name to check for.
> + *
> + * Returns: true if the accelerator is built in.
> + */
> +bool qtest_probe_accel(QTestState *s, const char *name);

Maybe better qtest_has_accel() ? ... that makes it clear right from the 
start what the return type is about.

>   /**
>    * qtest_set_expected_status:
>    * @s: QTestState instance to operate on.
> diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
> index 71e359efcd3..57e7e55b9cc 100644
> --- a/tests/qtest/libqtest.c
> +++ b/tests/qtest/libqtest.c
> @@ -872,6 +872,30 @@ void qtest_qmp_eventwait(QTestState *s, const char *event)
>       qobject_unref(response);
>   }
>   
> +bool qtest_probe_accel(QTestState *s, const char *name)
> +{
> +    bool has_accel = false;
> +    QDict *response;
> +    QList *accels;
> +    QListEntry *accel;
> +
> +    response = qtest_qmp(s, "{'execute': 'query-accels'}");
> +    accels = qdict_get_qlist(response, "return");
> +
> +    QLIST_FOREACH_ENTRY(accels, accel) {
> +        QDict *accel_dict = qobject_to(QDict, qlist_entry_obj(accel));
> +        const char *accel_name = qdict_get_str(accel_dict, "name");
> +
> +        if (!strcmp(name, accel_name)) {

I'd prefer g_str_equal() ... that's easier to read.

> +            has_accel = true;
> +            break;
> +        }
> +    }
> +    qobject_unref(response);
> +
> +    return has_accel;
> +}
> +
>   char *qtest_vhmp(QTestState *s, const char *fmt, va_list ap)
>   {
>       char *cmd;
> 

  Thomas

Re: [PATCH 1/6] accel: Introduce 'query-accels' QMP command

[Claudio Fontana]

On 3/12/21 8:42 AM, Marc-André Lureau wrote:
> On Fri, Mar 12, 2021 at 3:14 AM Philippe Mathieu-Daudé <philmd@redhat.com>
> wrote:
> 
>> Introduce the 'query-accels' QMP command which returns a list
>> of built-in accelerators names.
>>
>> - Accelerator is an 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": [
>>             {
>>                 "type": "qtest"
>>             },
>>             {
>>                 "type": "kvm"
>>             }
>>
> 
> s/type/name (in this version)
> 
>         ]
>>     }
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>  qapi/machine.json | 55 +++++++++++++++++++++++++++++++++++++++++++++++
>>  accel/accel-qmp.c | 47 ++++++++++++++++++++++++++++++++++++++++
>>  accel/meson.build |  2 +-
>>  3 files changed, 103 insertions(+), 1 deletion(-)
>>  create mode 100644 accel/accel-qmp.c
>>
>> diff --git a/qapi/machine.json b/qapi/machine.json
>> index 330189efe3d..ffbf28e5d50 100644
>> --- a/qapi/machine.json
>> +++ b/qapi/machine.json
>> @@ -1471,3 +1471,58 @@
>>  ##
>>  { 'event': 'MEM_UNPLUG_ERROR',
>>    'data': { 'device': 'str', 'msg': 'str' } }
>> +
>> +##
>> +# @Accelerator:
>> +#
>> +# An enumeration of accelerator names.
>> +#
>> +# Since: 6.0
>> +##
>> +{ 'enum': 'Accelerator',
>> +  'data': [ { 'name': 'qtest' },
>> +            { 'name': 'tcg' },
>> +            { 'name': 'kvm' },
>> +            { 'name': 'hax' },
>> +            { 'name': 'hvf' },
>> +            { 'name': 'whpx' },
>> +            { 'name': 'xen' } ] }
>> +
>>
> 
> Why not use a simple enum?
> 
> +##
>> +# @AcceleratorInfo:
>> +#
>> +# Accelerator information.
>> +#
>> +# @name: The accelerator name.
>> +#
>> +# Since: 6.0
>> +##
>> +{ 'union': 'AcceleratorInfo',
>> +  'base': {'name': 'Accelerator'},
>> +  'discriminator': 'name',
>> +  'data': { } }
>>
> +
>>
> 
> Making room for future details, why not.
> 
> +##
>> +# @query-accels:
>> +#
>> +# Get a list of AcceleratorInfo for all built-in accelerators.
>> +#
>> +# Returns: a list of @AcceleratorInfo describing each accelerator.
>> +#
>> +# Since: 6.0
>> +#
>> +# Example:
>> +#
>> +# -> { "execute": "query-accels" }
>> +# <- { "return": [
>> +#        {
>> +#            "type": "qtest"
>> +#        },
>> +#        {
>> +#            "type": "kvm"
>> +#        }
>> +#    ] }
>> +#
>> +##
>> +{ 'command': 'query-accels',
>> +  'returns': ['AcceleratorInfo'] }
>>
> 
> That's nice, but how do you know which accels are actually enabled?


Maybe for clarity this could be 'query-accels-available' (which is probably the goal of this series).
Possibly a separate one would be 'query-accel-enabled'?

Can we see these commands being used for libvirt too, to improve feature detection? Are these useful beyond the confines of just testing?
I would think so right?

Ciao,

Claudio


> 
> diff --git a/accel/accel-qmp.c b/accel/accel-qmp.c
>> new file mode 100644
>> index 00000000000..f16e49b8956
>> --- /dev/null
>> +++ b/accel/accel-qmp.c
>> @@ -0,0 +1,47 @@
>> +/*
>> + * 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 Accelerator accel_list[] = {
>> +    ACCELERATOR_QTEST,
>> +#ifdef CONFIG_TCG
>> +    ACCELERATOR_TCG,
>> +#endif
>> +#ifdef CONFIG_KVM
>> +    ACCELERATOR_KVM,
>> +#endif
>> +#ifdef CONFIG_HAX
>> +    ACCELERATOR_HAX,
>> +#endif
>> +#ifdef CONFIG_HVF
>> +    ACCELERATOR_HVF,
>> +#endif
>> +#ifdef CONFIG_WHPX
>> +    ACCELERATOR_WHPX,
>> +#endif
>> +#ifdef CONFIG_XEN_BACKEND
>> +    ACCELERATOR_XEN,
>> +#endif
>> +};
>> +
>> +AcceleratorInfoList *qmp_query_accels(Error **errp)
>> +{
>> +    AcceleratorInfoList *list = NULL, **tail = &list;
>> +
>> +    for (unsigned i = 0; i < ARRAY_SIZE(accel_list); i++) {
>> +        AcceleratorInfo *info = g_new0(AcceleratorInfo, 1);
>> +
>> +        info->name = accel_list[i];
>> +
>> +        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.2
>>
>>
>>
> 

Re: [PATCH 1/6] accel: Introduce 'query-accels' QMP command

[Andrew Jones]

On Fri, Mar 12, 2021 at 09:11:45AM +0100, Thomas Huth wrote:
> On 12/03/2021 08.42, Marc-André Lureau wrote:
> > 
> > 
> > On Fri, Mar 12, 2021 at 3:14 AM Philippe Mathieu-Daudé
> > <philmd@redhat.com <mailto:philmd@redhat.com>> wrote:
> > 
> [...]
> >     +##
> >     +# @AcceleratorInfo:
> >     +#
> >     +# Accelerator information.
> >     +#
> >     +# @name: The accelerator name.
> >     +#
> >     +# Since: 6.0
> >     +##
> >     +{ 'union': 'AcceleratorInfo',
> >     +  'base': {'name': 'Accelerator'},
> >     +  'discriminator': 'name',
> >     +  'data': { } }
> > 
> >     +
> > 
> > 
> > Making room for future details, why not.
> 
> I think we definitely need the additional data section here: For KVM on
> POWER, it would be good to know whether it's KVM-HV or KVM-PR, for KVM on
> MIPS it would be good to know whether it's KVM_VM_MIPS_VZ or KVM_VM_MIPS_TE,
> for KVM on x86 whether it's the AMD flavor or Intel, ...

Could also pre-expand all of these and list them individually.

> 
> >     +##
> >     +# @query-accels:
> >     +#
> >     +# Get a list of AcceleratorInfo for all built-in accelerators.
> >     +#
> >     +# Returns: a list of @AcceleratorInfo describing each accelerator.
> >     +#
> >     +# Since: 6.0
> >     +#
> >     +# Example:
> >     +#
> >     +# -> { "execute": "query-accels" }
> >     +# <- { "return": [
> >     +#        {
> >     +#            "type": "qtest"
> >     +#        },
> >     +#        {
> >     +#            "type": "kvm"
> >     +#        }
> >     +#    ] }
> >     +#
> >     +##
> >     +{ 'command': 'query-accels',
> >     +  'returns': ['AcceleratorInfo'] }
> > 
> > 
> > That's nice, but how do you know which accels are actually enabled?
> 
> I guess we need two commands in the end, one for querying which accelerators
> are available, and one for querying the data from the currently active
> accelerator...?
>

If we listed each accelerator individually, then we could use booleans
for them, where only the active one would be true. If we can't come up
with another need for the accelerator-specific info now, then I'd leave
it to be added at a later time when it is needed.

Thanks,
drew

Re: [PATCH 1/6] accel: Introduce 'query-accels' QMP command

[Claudio Fontana]

On 3/12/21 9:48 AM, Andrew Jones wrote:
> On Fri, Mar 12, 2021 at 09:11:45AM +0100, Thomas Huth wrote:
>> On 12/03/2021 08.42, Marc-André Lureau wrote:
>>>
>>>
>>> On Fri, Mar 12, 2021 at 3:14 AM Philippe Mathieu-Daudé
>>> <philmd@redhat.com <mailto:philmd@redhat.com>> wrote:
>>>
>> [...]
>>>     +##
>>>     +# @AcceleratorInfo:
>>>     +#
>>>     +# Accelerator information.
>>>     +#
>>>     +# @name: The accelerator name.
>>>     +#
>>>     +# Since: 6.0
>>>     +##
>>>     +{ 'union': 'AcceleratorInfo',
>>>     +  'base': {'name': 'Accelerator'},
>>>     +  'discriminator': 'name',
>>>     +  'data': { } }
>>>
>>>     +
>>>
>>>
>>> Making room for future details, why not.
>>
>> I think we definitely need the additional data section here: For KVM on
>> POWER, it would be good to know whether it's KVM-HV or KVM-PR, for KVM on
>> MIPS it would be good to know whether it's KVM_VM_MIPS_VZ or KVM_VM_MIPS_TE,
>> for KVM on x86 whether it's the AMD flavor or Intel, ...
> 
> Could also pre-expand all of these and list them individually.



Let us consider simplicity for the reader, and which use cases we expect for this.


> 
>>
>>>     +##
>>>     +# @query-accels:
>>>     +#
>>>     +# Get a list of AcceleratorInfo for all built-in accelerators.
>>>     +#
>>>     +# Returns: a list of @AcceleratorInfo describing each accelerator.
>>>     +#
>>>     +# Since: 6.0
>>>     +#
>>>     +# Example:
>>>     +#
>>>     +# -> { "execute": "query-accels" }
>>>     +# <- { "return": [
>>>     +#        {
>>>     +#            "type": "qtest"
>>>     +#        },
>>>     +#        {
>>>     +#            "type": "kvm"
>>>     +#        }
>>>     +#    ] }
>>>     +#
>>>     +##
>>>     +{ 'command': 'query-accels',
>>>     +  'returns': ['AcceleratorInfo'] }
>>>
>>>
>>> That's nice, but how do you know which accels are actually enabled?
>>
>> I guess we need two commands in the end, one for querying which accelerators
>> are available, and one for querying the data from the currently active
>> accelerator...?
>>
> 
> If we listed each accelerator individually, then we could use booleans
> for them, where only the active one would be true. If we can't come up
> with another need for the accelerator-specific info now, then I'd leave
> it to be added at a later time when it is needed.
> 
> Thanks,
> drew
> 

Re: [PATCH 2/6] tests/qtest: Add qtest_probe_accel() method

[Andrew Jones]

On Fri, Mar 12, 2021 at 09:16:01AM +0100, Thomas Huth wrote:
> On 12/03/2021 00.11, Philippe Mathieu-Daudé wrote:
> > Introduce the qtest_probe_accel() method which allows
> > to query at runtime if a QEMU instance has an accelerator
> > built-in.
> > 
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > ---
> >   tests/qtest/libqos/libqtest.h |  9 +++++++++
> >   tests/qtest/libqtest.c        | 24 ++++++++++++++++++++++++
> >   2 files changed, 33 insertions(+)
> > 
> > diff --git a/tests/qtest/libqos/libqtest.h b/tests/qtest/libqos/libqtest.h
> > index a68dcd79d44..ebedb82ec98 100644
> > --- a/tests/qtest/libqos/libqtest.h
> > +++ b/tests/qtest/libqos/libqtest.h
> > @@ -763,6 +763,15 @@ void qmp_expect_error_and_unref(QDict *rsp, const char *class);
> >    */
> >   bool qtest_probe_child(QTestState *s);
> > +/**
> > + * qtest_probe_accel:
> > + * @s: QTestState instance to operate on.
> > + * @name: Accelerator name to check for.
> > + *
> > + * Returns: true if the accelerator is built in.
> > + */
> > +bool qtest_probe_accel(QTestState *s, const char *name);
> 
> Maybe better qtest_has_accel() ? ... that makes it clear right from the
> start what the return type is about.

It looks like qtest_probe_accel() is getting used in contexts in the
following patches that would be better suited with an "enabled" API

 qtest_accel_enabled(s, accel_name)

So, I think we should create that interface. Do we also need a
qtest_has_accel()? Does it matter which ones have been compiled
in when they're not active?

(Hmm, 'active' might be a better verb yet. qtest_accel_active() ?)

Thanks,
drew

Re: [PATCH 1/6] accel: Introduce 'query-accels' QMP command

[Paolo Bonzini]

On 12/03/21 09:48, Andrew Jones wrote:
>> I think we definitely need the additional data section here: For KVM on
>> POWER, it would be good to know whether it's KVM-HV or KVM-PR, for KVM on
>> MIPS it would be good to know whether it's KVM_VM_MIPS_VZ or KVM_VM_MIPS_TE,
>> for KVM on x86 whether it's the AMD flavor or Intel, ...
>
> Could also pre-expand all of these and list them individually.

That seems worse (in general) because in a lot of cases you don't care; 
the basic query-accels output should match the argument to "-accel".

Paolo

Re: [PATCH 1/6] accel: Introduce 'query-accels' QMP command

[Paolo Bonzini]

On 12/03/21 09:46, Claudio Fontana wrote:
> 
> Maybe for clarity this could be 'query-accels-available' (which is probably the goal of this series).
> Possibly a separate one would be 'query-accel-enabled'?

The accelerator object is not included in the QOM tree.  I think it 
should be added to /machine/accel so that you can get the enabled 
accelerator with "qom-get /machine/accel type".

Paolo

Re: [PATCH 4/6] qtest/arm-cpu-features: Check KVM availability at runtime

[Paolo Bonzini]

On 12/03/21 00:12, Philippe Mathieu-Daudé wrote:
> -#define MACHINE_KVM "-machine virt,gic-version=max -accel kvm -accel tcg "
> +#define MACHINE_KVM "-machine virt,gic-version=max -accel kvm "

Wouldn't qtest_init simply fail here if KVM is not available?

Paolo

Re: [PATCH 5/6] qtest/arm-cpu-features: Check TCG availability at runtime

[Paolo Bonzini]

On 12/03/21 00:12, Philippe Mathieu-Daudé wrote:
> Now than we can probe if the TCG accelerator is available
> at runtime with a QMP command, skip these TCG specific tests
> when TCG is not built into the QEMU binary.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   tests/qtest/arm-cpu-features.c | 23 +++++++++++++++++++----
>   1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/qtest/arm-cpu-features.c b/tests/qtest/arm-cpu-features.c
> index 2b70104515d..7acdccd10ef 100644
> --- a/tests/qtest/arm-cpu-features.c
> +++ b/tests/qtest/arm-cpu-features.c
> @@ -20,7 +20,7 @@
>    */
>   #define SVE_MAX_VQ 16
>   
> -#define MACHINE     "-machine virt,gic-version=max -accel tcg "
> +#define MACHINE_TCG "-machine virt,gic-version=max -accel tcg "
>   #define MACHINE_KVM "-machine virt,gic-version=max -accel kvm "
>   #define QUERY_HEAD  "{ 'execute': 'query-cpu-model-expansion', " \
>                       "  'arguments': { 'type': 'full', "
> @@ -352,7 +352,11 @@ static void sve_tests_sve_max_vq_8(const void *data)
>   {
>       QTestState *qts;
>   
> -    qts = qtest_init(MACHINE "-cpu max,sve-max-vq=8");
> +    qts = qtest_init(MACHINE_TCG "-cpu max,sve-max-vq=8");
> +    if (!qtest_probe_accel(qts, "tcg")) {
> +        g_test_skip("TCG not available, skipping test");
> +        goto done;

... and likewise here probing seems unnecessary.

Paolo

> +    }
>   
>       assert_sve_vls(qts, "max", BIT_ULL(8) - 1, NULL);
>   
> @@ -380,6 +384,7 @@ static void sve_tests_sve_max_vq_8(const void *data)
>       assert_sve_vls(qts, "max", 0xff, "{ 'sve256': true }");
>       assert_error(qts, "max", "cannot disable sve256", "{ 'sve256': false }");
>   
> +done:
>       qtest_quit(qts);
>   }
>   
> @@ -387,7 +392,11 @@ static void sve_tests_sve_off(const void *data)
>   {
>       QTestState *qts;
>   
> -    qts = qtest_init(MACHINE "-cpu max,sve=off");
> +    qts = qtest_init(MACHINE_TCG "-cpu max,sve=off");
> +    if (!qtest_probe_accel(qts, "tcg")) {
> +        g_test_skip("TCG not available, skipping test");
> +        goto done;
> +    }
>   
>       /* SVE is off, so the map should be empty. */
>       assert_sve_vls(qts, "max", 0, NULL);
> @@ -405,6 +414,7 @@ static void sve_tests_sve_off(const void *data)
>       assert_sve_vls(qts, "max", 0x3,
>                      "{ 'sve': true, 'sve128': true, 'sve256': true }");
>   
> +done:
>       qtest_quit(qts);
>   }
>   
> @@ -448,7 +458,11 @@ static void test_query_cpu_model_expansion(const void *data)
>   {
>       QTestState *qts;
>   
> -    qts = qtest_init(MACHINE "-cpu max");
> +    qts = qtest_init(MACHINE_TCG "-cpu max");
> +    if (!qtest_probe_accel(qts, "tcg")) {
> +        g_test_skip("TCG not available, skipping test");
> +        goto done;
> +    }
>   
>       /* Test common query-cpu-model-expansion input validation */
>       assert_type_full(qts);
> @@ -489,6 +503,7 @@ static void test_query_cpu_model_expansion(const void *data)
>                        "{ 'aarch64': false }");
>       }
>   
> +done:
>       qtest_quit(qts);
>   }
>   
> 

Re: [PATCH 6/6] tests/qtest: Do not restrict bios-tables-test to Aarch64 hosts anymore

[Paolo Bonzini]

On 12/03/21 00:12, Philippe Mathieu-Daudé wrote:
> Since commit 82bf7ae84ce ("target/arm: Remove KVM support for
> 32-bit Arm hosts") we can remove the comment / check added in
> commit ab6b6a77774 and directly run the bios-tables-test.
> 
> Signed-off-by: Philippe Mathieu-Daudé<philmd@redhat.com>
> ---
>   tests/qtest/meson.build | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> index 2688e1bfad7..405ae7a5602 100644
> --- a/tests/qtest/meson.build
> +++ b/tests/qtest/meson.build
> @@ -170,14 +170,13 @@
>      'boot-serial-test',
>      'hexloader-test']
>   
> -# TODO: once aarch64 TCG is fixed on ARM 32 bit host, make bios-tables-test unconditional
>   qtests_aarch64 = \
> -  (cpu != 'arm' ? ['bios-tables-test'] : []) +                                                  \

I don't understand, has aarch64-on-ARM TCG been fixed?

Paolo

Re: [PATCH 1/6] accel: Introduce 'query-accels' QMP command

[Andrew Jones]

On Fri, Mar 12, 2021 at 09:52:33AM +0100, Claudio Fontana wrote:
> On 3/12/21 9:48 AM, Andrew Jones wrote:
> > On Fri, Mar 12, 2021 at 09:11:45AM +0100, Thomas Huth wrote:
> >> On 12/03/2021 08.42, Marc-André Lureau wrote:
> >>>
> >>>
> >>> On Fri, Mar 12, 2021 at 3:14 AM Philippe Mathieu-Daudé
> >>> <philmd@redhat.com <mailto:philmd@redhat.com>> wrote:
> >>>
> >> [...]
> >>>     +##
> >>>     +# @AcceleratorInfo:
> >>>     +#
> >>>     +# Accelerator information.
> >>>     +#
> >>>     +# @name: The accelerator name.
> >>>     +#
> >>>     +# Since: 6.0
> >>>     +##
> >>>     +{ 'union': 'AcceleratorInfo',
> >>>     +  'base': {'name': 'Accelerator'},
> >>>     +  'discriminator': 'name',
> >>>     +  'data': { } }
> >>>
> >>>     +
> >>>
> >>>
> >>> Making room for future details, why not.
> >>
> >> I think we definitely need the additional data section here: For KVM on
> >> POWER, it would be good to know whether it's KVM-HV or KVM-PR, for KVM on
> >> MIPS it would be good to know whether it's KVM_VM_MIPS_VZ or KVM_VM_MIPS_TE,
> >> for KVM on x86 whether it's the AMD flavor or Intel, ...
> > 
> > Could also pre-expand all of these and list them individually.
> 
> 
> 
> Let us consider simplicity for the reader, and which use cases we expect for this.

What do you mean by "reader"? If you mean the QMP client that needs this
information, then I can't think of anything more simple than a single list
of booleans with descriptive names to process. If you mean that the
expected use cases don't care about all those variants, then you don't
need to worry about that. Only the ones compiled in will be in the list.
The expected use cases will never see the other ones.

Thanks,
drew

Re: [PATCH 1/6] accel: Introduce 'query-accels' QMP command

[Andrew Jones]

On Fri, Mar 12, 2021 at 10:01:43AM +0100, Paolo Bonzini wrote:
> On 12/03/21 09:48, Andrew Jones wrote:
> > > I think we definitely need the additional data section here: For KVM on
> > > POWER, it would be good to know whether it's KVM-HV or KVM-PR, for KVM on
> > > MIPS it would be good to know whether it's KVM_VM_MIPS_VZ or KVM_VM_MIPS_TE,
> > > for KVM on x86 whether it's the AMD flavor or Intel, ...
> > 
> > Could also pre-expand all of these and list them individually.
> 
> That seems worse (in general) because in a lot of cases you don't care; the
> basic query-accels output should match the argument to "-accel".
>

For these special subtypes, what's the property/state that indicates it
when just using '-accel kvm' on the command line? Because if this qmp
list matches the '-accel' parameter list, then qtest and other qmp clients
may need to query that other information too, in order for this accel
query to be useful. And, do we need an accel-specific qmp query for it?
Or, is that information already available?

Thanks,
drew

Re: [PATCH 1/6] accel: Introduce 'query-accels' QMP command

[Paolo Bonzini]

On 12/03/21 10:17, Andrew Jones wrote:
> On Fri, Mar 12, 2021 at 10:01:43AM +0100, Paolo Bonzini wrote:
>> On 12/03/21 09:48, Andrew Jones wrote:
>>>> I think we definitely need the additional data section here: For KVM on
>>>> POWER, it would be good to know whether it's KVM-HV or KVM-PR, for KVM on
>>>> MIPS it would be good to know whether it's KVM_VM_MIPS_VZ or KVM_VM_MIPS_TE,
>>>> for KVM on x86 whether it's the AMD flavor or Intel, ...
>>>
>>> Could also pre-expand all of these and list them individually.
>>
>> That seems worse (in general) because in a lot of cases you don't care; the
>> basic query-accels output should match the argument to "-accel".
>>
> 
> For these special subtypes, what's the property/state that indicates it
> when just using '-accel kvm' on the command line? Because if this qmp
> list matches the '-accel' parameter list, then qtest and other qmp clients
> may need to query that other information too, in order for this accel
> query to be useful. And, do we need an accel-specific qmp query for it?
> Or, is that information already available?

It depends.

On PPC (if I remember/understand correctly) only pseries supports both 
HV and PR, while all other machines only support KVM-PR.  So in that 
case it's a kvm-type machine property that is defined only for the 
pseries machine.

On MIPS instead there's no option and VZ always wins over TE.  I think 
it could be made an option on -accel, but I'm not familiar with MIPS 
machine types.

Something like "name: 'kvm', types: ['book3s-hv', 'pr']" would work 
nicely for KVM-PPC, and likewise for MIPS.

Paolo

Re: [PATCH 4/6] qtest/arm-cpu-features: Check KVM availability at runtime

[Philippe Mathieu-Daudé]

On 3/12/21 10:05 AM, Paolo Bonzini wrote:
> On 12/03/21 00:12, Philippe Mathieu-Daudé wrote:
>> -#define MACHINE_KVM "-machine virt,gic-version=max -accel kvm -accel
>> tcg "
>> +#define MACHINE_KVM "-machine virt,gic-version=max -accel kvm "
> 
> Wouldn't qtest_init simply fail here if KVM is not available?

I guess my previous approach was what you suggested.
The previous patch (bios-tables-test) is a better example:

  g_autofree char *args = NULL;

  args = test_acpi_create_args(data, params, use_uefi);
  data->qts = qtest_init_for_accel(args, "tcg");
  if (data->tcg_only && !data->qts) {
     g_test_skip("TCG not available, skipping test");
     return;
  } else {
     // check data->qts or the following will abort
  }

Having qtest_init_for_accel() calling qtest_quit(),
but this makes the tests logic more complex IMO...

Is that what you have in mind?

Re: [PATCH 4/6] qtest/arm-cpu-features: Check KVM availability at runtime

[Paolo Bonzini]

On 12/03/21 10:32, Philippe Mathieu-Daudé wrote:
> On 3/12/21 10:05 AM, Paolo Bonzini wrote:
>> On 12/03/21 00:12, Philippe Mathieu-Daudé wrote:
>>> -#define MACHINE_KVM "-machine virt,gic-version=max -accel kvm -accel
>>> tcg "
>>> +#define MACHINE_KVM "-machine virt,gic-version=max -accel kvm "
>>
>> Wouldn't qtest_init simply fail here if KVM is not available?
> 
> I guess my previous approach was what you suggested.
> The previous patch (bios-tables-test) is a better example:

Ah no, qtest_init adds "-accel qtest" automatically.  So this patch is fine.

An alternative is to first probe accelerators using "-machine none" and 
then skip the tests *before* the call to qtest_init.

Paolo

>    g_autofree char *args = NULL;
> 
>    args = test_acpi_create_args(data, params, use_uefi);
>    data->qts = qtest_init_for_accel(args, "tcg");
>    if (data->tcg_only && !data->qts) {
>       g_test_skip("TCG not available, skipping test");
>       return;
>    } else {
>       // check data->qts or the following will abort
>    }
> 
> Having qtest_init_for_accel() calling qtest_quit(),
> but this makes the tests logic more complex IMO...
> 
> Is that what you have in mind?
> 

Re: [PATCH 1/6] accel: Introduce 'query-accels' QMP command

[Eric Blake]

On 3/11/21 5:11 PM, Philippe Mathieu-Daudé wrote:
> Introduce the 'query-accels' QMP command which returns a list
> of built-in accelerators names.

accelerator names

> 
> - Accelerator is an QAPI enum of all existing accelerators,

is a QAPI enum

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

Do we expand it later in this series?  If not,...

> 
> - 'query-accels' QMP command returns a list of @AcceleratorInfo
> 
> For example on a KVM-only build we get:
> 
>     { "execute": "query-accels" }
>     {
>         "return": [
>             {
>                 "type": "qtest"
>             },
>             {
>                 "type": "kvm"
>             }

Inconsistent with the code, already pointed out in other reviews.

>         ]
>     }
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---

> +##
> +# @AcceleratorInfo:
> +#
> +# Accelerator information.
> +#
> +# @name: The accelerator name.
> +#
> +# Since: 6.0
> +##
> +{ 'union': 'AcceleratorInfo',
> +  'base': {'name': 'Accelerator'},
> +  'discriminator': 'name',
> +  'data': { } }

...it feels a bit over-engineered (we can turn it into a union later
while still preserving back-compat).  On the other hand, since you are
using conditional compilation...

> +
> +##
> +# @query-accels:
> +#
> +# Get a list of AcceleratorInfo for all built-in accelerators.
> +#
> +# Returns: a list of @AcceleratorInfo describing each accelerator.
> +#
> +# Since: 6.0
> +#
> +# Example:
> +#
> +# -> { "execute": "query-accels" }
> +# <- { "return": [
> +#        {
> +#            "type": "qtest"
> +#        },
> +#        {
> +#            "type": "kvm"

Another inconsistent example.

> +#        }
> +#    ] }
> +#
> +##
> +{ 'command': 'query-accels',
> +  'returns': ['AcceleratorInfo'] }
> diff --git a/accel/accel-qmp.c b/accel/accel-qmp.c
> new file mode 100644
> index 00000000000..f16e49b8956
> --- /dev/null
> +++ b/accel/accel-qmp.c
> @@ -0,0 +1,47 @@
> +/*
> + * 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 Accelerator accel_list[] = {
> +    ACCELERATOR_QTEST,
> +#ifdef CONFIG_TCG
> +    ACCELERATOR_TCG,
> +#endif
> +#ifdef CONFIG_KVM
> +    ACCELERATOR_KVM,
> +#endif

...would it be worth compiling the enum to only list enum values that
were actually compiled in?  That would change it to:

{ 'enum': 'Accelerator',
  'data': [ 'qtest',
            { 'name': 'tcg', 'if': 'defined(CONFIG_TCG)' },
...


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 1/6] accel: Introduce 'query-accels' QMP command

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 3/11/21 5:11 PM, Philippe Mathieu-Daudé wrote:
[...]
>> diff --git a/accel/accel-qmp.c b/accel/accel-qmp.c
>> new file mode 100644
>> index 00000000000..f16e49b8956
>> --- /dev/null
>> +++ b/accel/accel-qmp.c
>> @@ -0,0 +1,47 @@
>> +/*
>> + * 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 Accelerator accel_list[] = {
>> +    ACCELERATOR_QTEST,
>> +#ifdef CONFIG_TCG
>> +    ACCELERATOR_TCG,
>> +#endif
>> +#ifdef CONFIG_KVM
>> +    ACCELERATOR_KVM,
>> +#endif
>
> ...would it be worth compiling the enum to only list enum values that
> were actually compiled in?  That would change it to:
>
> { 'enum': 'Accelerator',
>   'data': [ 'qtest',
>             { 'name': 'tcg', 'if': 'defined(CONFIG_TCG)' },
> ...

Makes introspection more useful.  Management applications can get the
information the list of compiled-in accelerators from query-qmp-schema.
They don't have to be taught to use query-accels.

In fact, query-accels becomes useless except as a tool to force
visibility of Accelerator in query-qmp-schema.  We wouldn't have to
force if we had CLI introspection that shows the type of -accel's
parameter @accel.  Adding a query command is a common work-around for
our anemic CLI introspection capabilities.

The query command could be made more useful than introspection if it
reflected run time state, i.e. it showed an accelerator only when the
host system actually supports it.  Can't say how practical that would
be.

>>
>> +AcceleratorInfoList *qmp_query_accels(Error **errp)
>> +{
>> +    AcceleratorInfoList *list = NULL, **tail = &list;
>> +
>> +    for (unsigned i = 0; i < ARRAY_SIZE(accel_list); i++) {
>> +        AcceleratorInfo *info = g_new0(AcceleratorInfo, 1);
>> +
>> +        info->name = accel_list[i];
>> +
>> +        QAPI_LIST_APPEND(tail, info);
>> +    }
>> +
>> +    return list;
>> +}

You could then use something like

        for (accel = 0; accel < ACCELERATOR__MAX; accel++) {
            AcceleratorInfo *info = g_new0(AcceleratorInfo, 1);

            info->name = Accelerator_str(accel);

            QAPI_LIST_APPEND(tail, info);
        }

Re: [PATCH 1/6] accel: Introduce 'query-accels' QMP command

[Paolo Bonzini]

On 16/03/21 07:51, Markus Armbruster wrote:
> The query command could be made more useful than introspection if it
> reflected run time state, i.e. it showed an accelerator only when the
> host system actually supports it.  Can't say how practical that would
> be.

At least for KVM there is runtime state that can be included in 
query-accels.

Paolo

Re: [PATCH 1/6] accel: Introduce 'query-accels' QMP command

[Philippe Mathieu-Daudé]

On 3/16/21 7:51 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> On 3/11/21 5:11 PM, Philippe Mathieu-Daudé wrote:
> [...]
>>> diff --git a/accel/accel-qmp.c b/accel/accel-qmp.c
>>> new file mode 100644
>>> index 00000000000..f16e49b8956
>>> --- /dev/null
>>> +++ b/accel/accel-qmp.c
>>> @@ -0,0 +1,47 @@
>>> +/*
>>> + * 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 Accelerator accel_list[] = {
>>> +    ACCELERATOR_QTEST,
>>> +#ifdef CONFIG_TCG
>>> +    ACCELERATOR_TCG,
>>> +#endif
>>> +#ifdef CONFIG_KVM
>>> +    ACCELERATOR_KVM,
>>> +#endif
>>
>> ...would it be worth compiling the enum to only list enum values that
>> were actually compiled in?  That would change it to:
>>
>> { 'enum': 'Accelerator',
>>   'data': [ 'qtest',
>>             { 'name': 'tcg', 'if': 'defined(CONFIG_TCG)' },
>> ...

These accelerator definitions are supposed to be poisoned in generic
code... But I like the simplicity of your suggestion, so I'll give it
a try and see what happens with removing the poisoned definitions.

> Makes introspection more useful.  Management applications can get the
> information the list of compiled-in accelerators from query-qmp-schema.
> They don't have to be taught to use query-accels.
> 
> In fact, query-accels becomes useless except as a tool to force
> visibility of Accelerator in query-qmp-schema.  We wouldn't have to
> force if we had CLI introspection that shows the type of -accel's
> parameter @accel.  Adding a query command is a common work-around for
> our anemic CLI introspection capabilities.
> 
> The query command could be made more useful than introspection if it
> reflected run time state, i.e. it showed an accelerator only when the
> host system actually supports it.  Can't say how practical that would
> be.
> 
>>>
>>> +AcceleratorInfoList *qmp_query_accels(Error **errp)
>>> +{
>>> +    AcceleratorInfoList *list = NULL, **tail = &list;
>>> +
>>> +    for (unsigned i = 0; i < ARRAY_SIZE(accel_list); i++) {
>>> +        AcceleratorInfo *info = g_new0(AcceleratorInfo, 1);
>>> +
>>> +        info->name = accel_list[i];
>>> +
>>> +        QAPI_LIST_APPEND(tail, info);
>>> +    }
>>> +
>>> +    return list;
>>> +}
> 
> You could then use something like
> 
>         for (accel = 0; accel < ACCELERATOR__MAX; accel++) {
>             AcceleratorInfo *info = g_new0(AcceleratorInfo, 1);
> 
>             info->name = Accelerator_str(accel);
> 
>             QAPI_LIST_APPEND(tail, info);
>         }
> 

Re: [PATCH 1/6] accel: Introduce 'query-accels' QMP command

[Philippe Mathieu-Daudé]

On 3/16/21 10:02 AM, Philippe Mathieu-Daudé wrote:
> On 3/16/21 7:51 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>>
>>> On 3/11/21 5:11 PM, Philippe Mathieu-Daudé wrote:
>> [...]
>>>> diff --git a/accel/accel-qmp.c b/accel/accel-qmp.c
>>>> new file mode 100644
>>>> index 00000000000..f16e49b8956
>>>> --- /dev/null
>>>> +++ b/accel/accel-qmp.c
>>>> @@ -0,0 +1,47 @@
>>>> +/*
>>>> + * 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 Accelerator accel_list[] = {
>>>> +    ACCELERATOR_QTEST,
>>>> +#ifdef CONFIG_TCG
>>>> +    ACCELERATOR_TCG,
>>>> +#endif
>>>> +#ifdef CONFIG_KVM
>>>> +    ACCELERATOR_KVM,
>>>> +#endif
>>>
>>> ...would it be worth compiling the enum to only list enum values that
>>> were actually compiled in?  That would change it to:
>>>
>>> { 'enum': 'Accelerator',
>>>   'data': [ 'qtest',
>>>             { 'name': 'tcg', 'if': 'defined(CONFIG_TCG)' },
>>> ...
> 
> These accelerator definitions are supposed to be poisoned in generic
> code... But I like the simplicity of your suggestion, so I'll give it
> a try and see what happens with removing the poisoned definitions.

This is actually quite interesting :) Accelerator definitions are
declared in config-target.h, but acceleration is host specific...

We certainly don't want to make qapi target-specific.

Re: [PATCH 1/6] accel: Introduce 'query-accels' QMP command

[Markus Armbruster]

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

> On 3/16/21 10:02 AM, Philippe Mathieu-Daudé wrote:
>> On 3/16/21 7:51 AM, Markus Armbruster wrote:
>>> Eric Blake <eblake@redhat.com> writes:
>>>
>>>> On 3/11/21 5:11 PM, Philippe Mathieu-Daudé wrote:
>>> [...]
>>>>> diff --git a/accel/accel-qmp.c b/accel/accel-qmp.c
>>>>> new file mode 100644
>>>>> index 00000000000..f16e49b8956
>>>>> --- /dev/null
>>>>> +++ b/accel/accel-qmp.c
>>>>> @@ -0,0 +1,47 @@
>>>>> +/*
>>>>> + * 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 Accelerator accel_list[] = {
>>>>> +    ACCELERATOR_QTEST,
>>>>> +#ifdef CONFIG_TCG
>>>>> +    ACCELERATOR_TCG,
>>>>> +#endif
>>>>> +#ifdef CONFIG_KVM
>>>>> +    ACCELERATOR_KVM,
>>>>> +#endif
>>>>
>>>> ...would it be worth compiling the enum to only list enum values that
>>>> were actually compiled in?  That would change it to:
>>>>
>>>> { 'enum': 'Accelerator',
>>>>   'data': [ 'qtest',
>>>>             { 'name': 'tcg', 'if': 'defined(CONFIG_TCG)' },
>>>> ...
>> 
>> These accelerator definitions are supposed to be poisoned in generic
>> code... But I like the simplicity of your suggestion, so I'll give it
>> a try and see what happens with removing the poisoned definitions.
>
> This is actually quite interesting :) Accelerator definitions are
> declared in config-target.h, but acceleration is host specific...
>
> We certainly don't want to make qapi target-specific.

We certainly don't want to make all the generated QAPI headers
target-specific.

We have made *selected* QAPI sub-modules target-specific, namely the
ones ending with "-target", currently qapi/machine-target.json and
qapi/misc-target.json.  Only these may use poisoned symbols in 'if'
conditions.  Example:

    { 'command': 'query-sev', 'returns': 'SevInfo',
      'if': 'defined(TARGET_I386)' }

Re: [PATCH 1/6] accel: Introduce 'query-accels' QMP command

[Philippe Mathieu-Daudé]

On 3/16/21 11:26 AM, Philippe Mathieu-Daudé wrote:
> On 3/16/21 10:02 AM, Philippe Mathieu-Daudé wrote:
>> On 3/16/21 7:51 AM, Markus Armbruster wrote:
>>> Eric Blake <eblake@redhat.com> writes:
>>>
>>>> On 3/11/21 5:11 PM, Philippe Mathieu-Daudé wrote:
>>> [...]
>>>>> diff --git a/accel/accel-qmp.c b/accel/accel-qmp.c
>>>>> new file mode 100644
>>>>> index 00000000000..f16e49b8956
>>>>> --- /dev/null
>>>>> +++ b/accel/accel-qmp.c
>>>>> @@ -0,0 +1,47 @@
>>>>> +/*
>>>>> + * 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 Accelerator accel_list[] = {
>>>>> +    ACCELERATOR_QTEST,
>>>>> +#ifdef CONFIG_TCG
>>>>> +    ACCELERATOR_TCG,
>>>>> +#endif
>>>>> +#ifdef CONFIG_KVM
>>>>> +    ACCELERATOR_KVM,
>>>>> +#endif
>>>>
>>>> ...would it be worth compiling the enum to only list enum values that
>>>> were actually compiled in?  That would change it to:
>>>>
>>>> { 'enum': 'Accelerator',
>>>>   'data': [ 'qtest',
>>>>             { 'name': 'tcg', 'if': 'defined(CONFIG_TCG)' },
>>>> ...
>>
>> These accelerator definitions are supposed to be poisoned in generic
>> code... But I like the simplicity of your suggestion, so I'll give it
>> a try and see what happens with removing the poisoned definitions.
> 
> This is actually quite interesting :) Accelerator definitions are
> declared in config-target.h, but acceleration is host specific...

Thomas, I guess I hit Claudio's reported bug again...

1/ generic libqemuutil.a is built without any CONFIG_accel definition.

So this qapi-generated enum ... :

typedef enum Accelerator {
    ACCELERATOR_QTEST,
#if defined(CONFIG_TCG)
    ACCELERATOR_TCG,
#endif /* defined(CONFIG_TCG) */
#if defined(CONFIG_KVM)
    ACCELERATOR_KVM,
#endif /* defined(CONFIG_KVM) */
#if defined(CONFIG_HAX)
    ACCELERATOR_HAX,
#endif /* defined(CONFIG_HAX) */
#if defined(CONFIG_HVF)
    ACCELERATOR_HVF,
#endif /* defined(CONFIG_HVF) */
#if defined(CONFIG_WHPX)
    ACCELERATOR_WHPX,
#endif /* defined(CONFIG_WHPX) */
#if defined(CONFIG_XEN_BACKEND)
    ACCELERATOR_XEN,
#endif /* defined(CONFIG_XEN_BACKEND) */
    ACCELERATOR__MAX,
} Accelerator;

... is expanded to:

typedef enum Accelerator {
    ACCELERATOR_QTEST,
    ACCELERATOR__MAX,
} Accelerator;

2/ softmmu code and qtest do get the definition, the enum
   is different:

typedef enum Accelerator {
    ACCELERATOR_QTEST,
    ACCELERATOR_KVM,
    ACCELERATOR__MAX,
} Accelerator;

qmp_query_accels() fills AcceleratorInfoList with 2 entries

3/ trying to understand what's happening, query-qmp-schema
   returns:

        {
            "name": "206",
            "tag": "name",
            "variants": [
                {
                    "case": "qtest",
                    "type": "0"
                },
                {
                    "case": "kvm",
                    "type": "0"
                }
            ],
            "members": [
                {
                    "name": "name",
                    "type": "403"
                }
            ],
            "meta-type": "object"
        },
        {
            "name": "403",
            "meta-type": "enum",
            "values": [
                "qtest",
                "kvm"
            ]
        },

So accelerators are listed, but with the same enum index?

4/ Running 'query-accels' aborts in qapi_enum_lookup():

The first entry 'qtest' is visited correctly.
When the second entry 'kvm' is visited, this assertion fires:

    assert(val >= 0 && val < lookup->size);

because lookup->size = 1 (remember from 1/ Accelerator_lookup
has been compiled without definitions, in libqemuutil.a).

I'll keep the current patch, as it works, and address the rest
of this series comments.

Thanks,

Phil.

Re: [PATCH 1/6] accel: Introduce 'query-accels' QMP command

[Markus Armbruster]

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

> On 3/16/21 11:26 AM, Philippe Mathieu-Daudé wrote:
>> On 3/16/21 10:02 AM, Philippe Mathieu-Daudé wrote:
>>> On 3/16/21 7:51 AM, Markus Armbruster wrote:
>>>> Eric Blake <eblake@redhat.com> writes:
>>>>
>>>>> On 3/11/21 5:11 PM, Philippe Mathieu-Daudé wrote:
>>>> [...]
>>>>>> diff --git a/accel/accel-qmp.c b/accel/accel-qmp.c
>>>>>> new file mode 100644
>>>>>> index 00000000000..f16e49b8956
>>>>>> --- /dev/null
>>>>>> +++ b/accel/accel-qmp.c
>>>>>> @@ -0,0 +1,47 @@
>>>>>> +/*
>>>>>> + * 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 Accelerator accel_list[] = {
>>>>>> +    ACCELERATOR_QTEST,
>>>>>> +#ifdef CONFIG_TCG
>>>>>> +    ACCELERATOR_TCG,
>>>>>> +#endif
>>>>>> +#ifdef CONFIG_KVM
>>>>>> +    ACCELERATOR_KVM,
>>>>>> +#endif
>>>>>
>>>>> ...would it be worth compiling the enum to only list enum values that
>>>>> were actually compiled in?  That would change it to:
>>>>>
>>>>> { 'enum': 'Accelerator',
>>>>>   'data': [ 'qtest',
>>>>>             { 'name': 'tcg', 'if': 'defined(CONFIG_TCG)' },
>>>>> ...
>>>
>>> These accelerator definitions are supposed to be poisoned in generic
>>> code... But I like the simplicity of your suggestion, so I'll give it
>>> a try and see what happens with removing the poisoned definitions.
>> 
>> This is actually quite interesting :) Accelerator definitions are
>> declared in config-target.h, but acceleration is host specific...
>
> Thomas, I guess I hit Claudio's reported bug again...
>
> 1/ generic libqemuutil.a is built without any CONFIG_accel definition.
>
> So this qapi-generated enum ... :
>
> typedef enum Accelerator {
>     ACCELERATOR_QTEST,
> #if defined(CONFIG_TCG)
>     ACCELERATOR_TCG,
> #endif /* defined(CONFIG_TCG) */
> #if defined(CONFIG_KVM)
>     ACCELERATOR_KVM,
> #endif /* defined(CONFIG_KVM) */
> #if defined(CONFIG_HAX)
>     ACCELERATOR_HAX,
> #endif /* defined(CONFIG_HAX) */
> #if defined(CONFIG_HVF)
>     ACCELERATOR_HVF,
> #endif /* defined(CONFIG_HVF) */
> #if defined(CONFIG_WHPX)
>     ACCELERATOR_WHPX,
> #endif /* defined(CONFIG_WHPX) */
> #if defined(CONFIG_XEN_BACKEND)
>     ACCELERATOR_XEN,
> #endif /* defined(CONFIG_XEN_BACKEND) */
>     ACCELERATOR__MAX,
> } Accelerator;
>
> ... is expanded to:
>
> typedef enum Accelerator {
>     ACCELERATOR_QTEST,
>     ACCELERATOR__MAX,
> } Accelerator;

CONFIG_KVM, CONFIG_TCG, ...  are defined in ${target}-config-target.h,
and may only be used in target-specific code.

If the enum ends up in libqemuutil.a, there are uses outside
target-specific code.

exec/poison.h lacks CONFIG_KVM, CONFIG_TCG, ...  Should they be added?

[...]

Re: [PATCH 1/6] accel: Introduce 'query-accels' QMP command

[Thomas Huth]

On 16/03/2021 13.41, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
> 
>> On 3/16/21 11:26 AM, Philippe Mathieu-Daudé wrote:
>>> On 3/16/21 10:02 AM, Philippe Mathieu-Daudé wrote:
>>>> On 3/16/21 7:51 AM, Markus Armbruster wrote:
>>>>> Eric Blake <eblake@redhat.com> writes:
>>>>>
>>>>>> On 3/11/21 5:11 PM, Philippe Mathieu-Daudé wrote:
>>>>> [...]
>>>>>>> diff --git a/accel/accel-qmp.c b/accel/accel-qmp.c
>>>>>>> new file mode 100644
>>>>>>> index 00000000000..f16e49b8956
>>>>>>> --- /dev/null
>>>>>>> +++ b/accel/accel-qmp.c
>>>>>>> @@ -0,0 +1,47 @@
>>>>>>> +/*
>>>>>>> + * 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 Accelerator accel_list[] = {
>>>>>>> +    ACCELERATOR_QTEST,
>>>>>>> +#ifdef CONFIG_TCG
>>>>>>> +    ACCELERATOR_TCG,
>>>>>>> +#endif
>>>>>>> +#ifdef CONFIG_KVM
>>>>>>> +    ACCELERATOR_KVM,
>>>>>>> +#endif
>>>>>>
>>>>>> ...would it be worth compiling the enum to only list enum values that
>>>>>> were actually compiled in?  That would change it to:
>>>>>>
>>>>>> { 'enum': 'Accelerator',
>>>>>>    'data': [ 'qtest',
>>>>>>              { 'name': 'tcg', 'if': 'defined(CONFIG_TCG)' },
>>>>>> ...
>>>>
>>>> These accelerator definitions are supposed to be poisoned in generic
>>>> code... But I like the simplicity of your suggestion, so I'll give it
>>>> a try and see what happens with removing the poisoned definitions.
>>>
>>> This is actually quite interesting :) Accelerator definitions are
>>> declared in config-target.h, but acceleration is host specific...
>>
>> Thomas, I guess I hit Claudio's reported bug again...
>>
>> 1/ generic libqemuutil.a is built without any CONFIG_accel definition.
>>
>> So this qapi-generated enum ... :
>>
>> typedef enum Accelerator {
>>      ACCELERATOR_QTEST,
>> #if defined(CONFIG_TCG)
>>      ACCELERATOR_TCG,
>> #endif /* defined(CONFIG_TCG) */
>> #if defined(CONFIG_KVM)
>>      ACCELERATOR_KVM,
>> #endif /* defined(CONFIG_KVM) */
>> #if defined(CONFIG_HAX)
>>      ACCELERATOR_HAX,
>> #endif /* defined(CONFIG_HAX) */
>> #if defined(CONFIG_HVF)
>>      ACCELERATOR_HVF,
>> #endif /* defined(CONFIG_HVF) */
>> #if defined(CONFIG_WHPX)
>>      ACCELERATOR_WHPX,
>> #endif /* defined(CONFIG_WHPX) */
>> #if defined(CONFIG_XEN_BACKEND)
>>      ACCELERATOR_XEN,
>> #endif /* defined(CONFIG_XEN_BACKEND) */
>>      ACCELERATOR__MAX,
>> } Accelerator;
>>
>> ... is expanded to:
>>
>> typedef enum Accelerator {
>>      ACCELERATOR_QTEST,
>>      ACCELERATOR__MAX,
>> } Accelerator;
> 
> CONFIG_KVM, CONFIG_TCG, ...  are defined in ${target}-config-target.h,
> and may only be used in target-specific code.
> 
> If the enum ends up in libqemuutil.a, there are uses outside
> target-specific code.
> 
> exec/poison.h lacks CONFIG_KVM, CONFIG_TCG, ...  Should they be added?

CONFIG_KVM is already in poison.h, and CONFIG_TCG cannot be added there 
since it is also defined in config-host.h. But the other accelerator 
switches should be marked as poisoned, too. I've got a patch for this in the 
works already - I'll send it out in a minute...

  Thomas

Re: [PATCH 1/6] accel: Introduce 'query-accels' QMP command

[Philippe Mathieu-Daudé]

On 3/16/21 1:48 PM, Thomas Huth wrote:
> On 16/03/2021 13.41, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>>
>>> On 3/16/21 11:26 AM, Philippe Mathieu-Daudé wrote:
>>>> On 3/16/21 10:02 AM, Philippe Mathieu-Daudé wrote:
>>>>> On 3/16/21 7:51 AM, Markus Armbruster wrote:
>>>>>> Eric Blake <eblake@redhat.com> writes:
>>>>>>
>>>>>>> On 3/11/21 5:11 PM, Philippe Mathieu-Daudé wrote:
>>>>>> [...]
>>>>>>>> diff --git a/accel/accel-qmp.c b/accel/accel-qmp.c
>>>>>>>> new file mode 100644
>>>>>>>> index 00000000000..f16e49b8956
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/accel/accel-qmp.c
>>>>>>>> @@ -0,0 +1,47 @@
>>>>>>>> +/*
>>>>>>>> + * 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 Accelerator accel_list[] = {
>>>>>>>> +    ACCELERATOR_QTEST,
>>>>>>>> +#ifdef CONFIG_TCG
>>>>>>>> +    ACCELERATOR_TCG,
>>>>>>>> +#endif
>>>>>>>> +#ifdef CONFIG_KVM
>>>>>>>> +    ACCELERATOR_KVM,
>>>>>>>> +#endif
>>>>>>>
>>>>>>> ...would it be worth compiling the enum to only list enum values
>>>>>>> that
>>>>>>> were actually compiled in?  That would change it to:
>>>>>>>
>>>>>>> { 'enum': 'Accelerator',
>>>>>>>    'data': [ 'qtest',
>>>>>>>              { 'name': 'tcg', 'if': 'defined(CONFIG_TCG)' },
>>>>>>> ...
>>>>>
>>>>> These accelerator definitions are supposed to be poisoned in generic
>>>>> code... But I like the simplicity of your suggestion, so I'll give it
>>>>> a try and see what happens with removing the poisoned definitions.
>>>>
>>>> This is actually quite interesting :) Accelerator definitions are
>>>> declared in config-target.h, but acceleration is host specific...
>>>
>>> Thomas, I guess I hit Claudio's reported bug again...
>>>
>>> 1/ generic libqemuutil.a is built without any CONFIG_accel definition.
>>>
>>> So this qapi-generated enum ... :
>>>
>>> typedef enum Accelerator {
>>>      ACCELERATOR_QTEST,
>>> #if defined(CONFIG_TCG)
>>>      ACCELERATOR_TCG,
>>> #endif /* defined(CONFIG_TCG) */
>>> #if defined(CONFIG_KVM)
>>>      ACCELERATOR_KVM,
>>> #endif /* defined(CONFIG_KVM) */
>>> #if defined(CONFIG_HAX)
>>>      ACCELERATOR_HAX,
>>> #endif /* defined(CONFIG_HAX) */
>>> #if defined(CONFIG_HVF)
>>>      ACCELERATOR_HVF,
>>> #endif /* defined(CONFIG_HVF) */
>>> #if defined(CONFIG_WHPX)
>>>      ACCELERATOR_WHPX,
>>> #endif /* defined(CONFIG_WHPX) */
>>> #if defined(CONFIG_XEN_BACKEND)
>>>      ACCELERATOR_XEN,
>>> #endif /* defined(CONFIG_XEN_BACKEND) */
>>>      ACCELERATOR__MAX,
>>> } Accelerator;
>>>
>>> ... is expanded to:
>>>
>>> typedef enum Accelerator {
>>>      ACCELERATOR_QTEST,
>>>      ACCELERATOR__MAX,
>>> } Accelerator;
>>
>> CONFIG_KVM, CONFIG_TCG, ...  are defined in ${target}-config-target.h,
>> and may only be used in target-specific code.
>>
>> If the enum ends up in libqemuutil.a, there are uses outside
>> target-specific code.
>>
>> exec/poison.h lacks CONFIG_KVM, CONFIG_TCG, ...  Should they be added?
> 
> CONFIG_KVM is already in poison.h, and CONFIG_TCG cannot be added there
> since it is also defined in config-host.h. But the other accelerator
> switches should be marked as poisoned, too.

Maybe we hit "exec/poison.h" limit. Maybe it is too wide / generic,
and need split, or multiple ones.

Should we redefine on which code area we want these definitions
poisoned?

AFAIK accel/ is not target specific but host specific.

My list of area / useful to poison:

- user-mode
  . non-tcg accel
  . hardware emulation

- generic hardware emulation
  . target specific
  . accel specific

For this one it is already too late:

- target acceleration
  . hardware emulation

Thoughts?