[PATCH 0/9] hw/arm/virt: Improve CPU help and fix testing under KVM

[PATCH 0/9] hw/arm/virt: Improve CPU help and fix testing under KVM

[Philippe Mathieu-Daudé]

Yet again bugfixes and cleanup patches noticed while
rebasing my "Support disabling TCG on ARM (part 2)" series.

Sending them independently as they aren't directly dependent
of it so don't have to be delayed by other unanswered questions.

Please review,

Phil.

Philippe Mathieu-Daudé (9):
  tests/qtest/arm-cpu-features: Remove Cortex-A15 check
  tests/qtest: Restrict xlnx-can-test to TCG builds
  tests/qtest/boot-serial-test: Test Virt machine with 'max'
  tests/qtest/cdrom-test: Only allow the Virt machine under KVM
  hw/arm/virt: Improve CPU name in help message
  hw/arm/virt: Display list of valid CPUs for the Virt machine
  hw/arm/virt: Do not include 64-bit CPUs in 32-bit build
  hw/arm/virt: Restrict 32-bit CPUs to TCG
  tests/qtest/arm-cpu-features: Restrict TCG-only tests

 hw/arm/virt.c                  | 20 +++++++++++++++++-
 tests/qtest/arm-cpu-features.c | 37 ++++++++++++++++++++++++++--------
 tests/qtest/boot-serial-test.c |  2 +-
 tests/qtest/cdrom-test.c       |  5 ++++-
 tests/qtest/meson.build        |  2 +-
 5 files changed, 54 insertions(+), 12 deletions(-)

-- 
2.26.2

[PATCH 1/9] tests/qtest/arm-cpu-features: Remove Cortex-A15 check

[Philippe Mathieu-Daudé]

Support for ARMv7 has been dropped in commit 82bf7ae84ce
("target/arm: Remove KVM support for 32-bit Arm hosts"),
no need to check for Cortex A15 host cpu anymore.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 tests/qtest/arm-cpu-features.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/tests/qtest/arm-cpu-features.c b/tests/qtest/arm-cpu-features.c
index 8252b85bb85..c59c3cb002b 100644
--- a/tests/qtest/arm-cpu-features.c
+++ b/tests/qtest/arm-cpu-features.c
@@ -515,10 +515,6 @@ static void test_query_cpu_model_expansion_kvm(const void *data)
         QDict *resp;
         char *error;
 
-        assert_error(qts, "cortex-a15",
-            "We cannot guarantee the CPU type 'cortex-a15' works "
-            "with KVM on this host", NULL);
-
         assert_has_feature_enabled(qts, "host", "aarch64");
 
         /* Enabling and disabling pmu should always work. */
-- 
2.26.2

[PATCH 2/9] tests/qtest: Restrict xlnx-can-test to TCG builds

[Philippe Mathieu-Daudé]

The Xilinx CAN controller test is uses the ZCU102 board which is
based on a ZynqMP SoC. In the default configuration - used by this
test - this SoC creates 2 Cortex R5F cores. Such cores are not
v8A archicture, thus can not be run under KVM. Therefore restrict
this test to TCG.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
Cc: Alistair Francis <alistair@alistair23.me>
Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
Cc: Vikram Garhwal <fnu.vikram@xilinx.com>
---
 tests/qtest/meson.build | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index c83bc211b6a..d8ebd5bf98e 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -159,10 +159,10 @@
   (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'] : []) +  \
+  (config_all.has_key('CONFIG_TCG') ? ['xlnx-can-test'] : []) +  \
   ['arm-cpu-features',
    'numa-test',
    'boot-serial-test',
-   'xlnx-can-test',
    'migration-test']
 
 qtests_s390x = \
-- 
2.26.2

[PATCH 3/9] tests/qtest/boot-serial-test: Test Virt machine with 'max'

[Philippe Mathieu-Daudé]

When using KVM, using a specific cpu type will only work if the
host CPU really is that exact CPU type.

During testing we can simply use the 'max' CPU which will select
all the features available from the host.

This allow running this test on a Cavium CN8890 (ThunderX cores).

Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 tests/qtest/boot-serial-test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qtest/boot-serial-test.c b/tests/qtest/boot-serial-test.c
index b6b1c23cd01..d74509b1c57 100644
--- a/tests/qtest/boot-serial-test.c
+++ b/tests/qtest/boot-serial-test.c
@@ -149,7 +149,7 @@ static testdef_t tests[] = {
     { "arm", "raspi2", "", "TT", sizeof(bios_raspi2), 0, bios_raspi2 },
     /* For hppa, force bios to output to serial by disabling graphics. */
     { "hppa", "hppa", "-vga none", "SeaBIOS wants SYSTEM HALT" },
-    { "aarch64", "virt", "-cpu cortex-a57", "TT", sizeof(kernel_aarch64),
+    { "aarch64", "virt", "-cpu max", "TT", sizeof(kernel_aarch64),
       kernel_aarch64 },
     { "arm", "microbit", "", "T", sizeof(kernel_nrf51), kernel_nrf51 },
 
-- 
2.26.2

[PATCH 4/9] tests/qtest/cdrom-test: Only allow the Virt machine under KVM

[Philippe Mathieu-Daudé]

Only the Virt and Versal machines are supported under KVM.
Restrict the other ones to TCG.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 tests/qtest/cdrom-test.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tests/qtest/cdrom-test.c b/tests/qtest/cdrom-test.c
index 5af944a5fb7..ac02f2bb4f1 100644
--- a/tests/qtest/cdrom-test.c
+++ b/tests/qtest/cdrom-test.c
@@ -222,9 +222,12 @@ int main(int argc, char **argv)
         add_cdrom_param_tests(mips64machines);
     } else if (g_str_equal(arch, "arm") || g_str_equal(arch, "aarch64")) {
         const char *armmachines[] = {
+#ifdef CONFIG_TCG
             "realview-eb", "realview-eb-mpcore", "realview-pb-a8",
             "realview-pbx-a9", "versatileab", "versatilepb", "vexpress-a15",
-            "vexpress-a9", "virt", NULL
+            "vexpress-a9",
+#endif /* CONFIG_TCG */
+            "virt", NULL
         };
         add_cdrom_param_tests(armmachines);
     } else {
-- 
2.26.2

[PATCH 5/9] hw/arm/virt: Improve CPU name in help message

[Philippe Mathieu-Daudé]

When selecting an incorrect CPU, there is a mismatch between the
CPU name provided and the one displayed (which is some QEMU internal
name):

  $ qemu-system-aarch64 -M virt -cpu cortex-a8
  qemu-system-aarch64: mach-virt: CPU type cortex-a8-arm-cpu not supported

Strip the suffix to display the correct CPU name:

  $ qemu-system-aarch64 -M virt -cpu cortex-a8
  qemu-system-aarch64: mach-virt: CPU type cortex-a8-arm

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/virt.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 399da734548..7802d3a66e8 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1829,7 +1829,10 @@ static void machvirt_init(MachineState *machine)
     finalize_gic_version(vms);
 
     if (!cpu_type_valid(machine->cpu_type)) {
-        error_report("mach-virt: CPU type %s not supported", machine->cpu_type);
+        int len = strlen(machine->cpu_type) - strlen(ARM_CPU_TYPE_SUFFIX);
+
+        error_report("mach-virt: CPU type %.*s not supported",
+                     len, machine->cpu_type);
         exit(1);
     }
 
-- 
2.26.2

[PATCH 6/9] hw/arm/virt: Display list of valid CPUs for the Virt machine

[Philippe Mathieu-Daudé]

The Virt machine is restricted to a subset of the CPU provided
by QEMU. Instead of having the user run '--cpu help' and try
each CPUs until finding a match, display the list from start:

  $ qemu-system-aarch64 -M virt -cpu cortex-a8
  qemu-system-aarch64: mach-virt: CPU type cortex-a8 not supported
  qemu-system-aarch64: mach-virt: Please select one of the following CPU types:  cortex-a7, cortex-a15, cortex-a53, cortex-a57, cortex-a72, host, max

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/virt.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 7802d3a66e8..6ffe091804f 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1830,9 +1830,20 @@ static void machvirt_init(MachineState *machine)
 
     if (!cpu_type_valid(machine->cpu_type)) {
         int len = strlen(machine->cpu_type) - strlen(ARM_CPU_TYPE_SUFFIX);
+        g_autoptr(GString) s = g_string_new(NULL);
 
         error_report("mach-virt: CPU type %.*s not supported",
                      len, machine->cpu_type);
+
+        for (n = 0; n < ARRAY_SIZE(valid_cpus); n++) {
+            len = strlen(valid_cpus[n]) - strlen(ARM_CPU_TYPE_SUFFIX);
+            g_string_append_printf(s, " %.*s", len, valid_cpus[n]);
+            if (n + 1 < ARRAY_SIZE(valid_cpus)) {
+                g_string_append_c(s, ',');
+            }
+        }
+        error_report("mach-virt: Please select one of the following CPU types: %s",
+                     g_string_free(s, FALSE));
         exit(1);
     }
 
-- 
2.26.2

[PATCH 7/9] hw/arm/virt: Do not include 64-bit CPUs in 32-bit build

[Philippe Mathieu-Daudé]

Similarly to commit 210f47840dd, remove 64-bit CPUs (which have
never been available on 32-bit build, see commit d14d42f19bf),
to fix:

  $ make check-qtest-arm
  ...
  Running test qtest-arm/device-introspect-test
  missing object type 'cortex-a53-arm-cpu'
  Broken pipe
  ../tests/qtest/libqtest.c:181: kill_qemu() detected QEMU death from signal 6 (Aborted) (core dumped)
  ERROR qtest-arm/device-introspect-test - too few tests run (expected 6, got 5)

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/virt.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 6ffe091804f..f5e4a6ec914 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -199,9 +199,11 @@ static const int a15irqmap[] = {
 static const char *valid_cpus[] = {
     ARM_CPU_TYPE_NAME("cortex-a7"),
     ARM_CPU_TYPE_NAME("cortex-a15"),
+#ifdef TARGET_AARCH64
     ARM_CPU_TYPE_NAME("cortex-a53"),
     ARM_CPU_TYPE_NAME("cortex-a57"),
     ARM_CPU_TYPE_NAME("cortex-a72"),
+#endif /* TARGET_AARCH64 */
     ARM_CPU_TYPE_NAME("host"),
     ARM_CPU_TYPE_NAME("max"),
 };
-- 
2.26.2

[PATCH 8/9] hw/arm/virt: Restrict 32-bit CPUs to TCG

[Philippe Mathieu-Daudé]

Support for ARMv7 has been dropped in commit 82bf7ae84ce
("target/arm: Remove KVM support for 32-bit Arm hosts").
Restrict the 32-bit CPUs to --enable-tcg builds.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/virt.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index f5e4a6ec914..ab6300650f9 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -197,8 +197,10 @@ static const int a15irqmap[] = {
 };
 
 static const char *valid_cpus[] = {
+#ifdef CONFIG_TCG
     ARM_CPU_TYPE_NAME("cortex-a7"),
     ARM_CPU_TYPE_NAME("cortex-a15"),
+#endif /* CONFIG_TCG */
 #ifdef TARGET_AARCH64
     ARM_CPU_TYPE_NAME("cortex-a53"),
     ARM_CPU_TYPE_NAME("cortex-a57"),
-- 
2.26.2

[RFC PATCH 9/9] tests/qtest/arm-cpu-features: Restrict TCG-only tests

[Philippe Mathieu-Daudé]

Some tests explicitly request the TCG accelerator. As these
tests will obviously fails if TCG is not present, disable
them in such case.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
Cc: Roman Bolshakov <r.bolshakov@yadro.com>
Cc: Claudio Fontana <cfontana@suse.de>

RFC because of the TODO.

Roman posted a series to have a QMP command to query enabled
accelerators.
---
 tests/qtest/arm-cpu-features.c | 33 +++++++++++++++++++++++++++++----
 1 file changed, 29 insertions(+), 4 deletions(-)

diff --git a/tests/qtest/arm-cpu-features.c b/tests/qtest/arm-cpu-features.c
index c59c3cb002b..c6e86282b66 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 -accel tcg "
 #define QUERY_HEAD  "{ 'execute': 'query-cpu-model-expansion', " \
                     "  'arguments': { 'type': 'full', "
@@ -41,6 +41,16 @@ static bool kvm_enabled(QTestState *qts)
     return enabled;
 }
 
+static bool tcg_enabled(QTestState *qts)
+{
+    /* TODO: Implement QMP query-accel? */
+#ifdef CONFIG_TCG
+    return true;
+#else
+    return false;
+#endif /* CONFIG_TCG */
+}
+
 static QDict *do_query_no_props(QTestState *qts, const char *cpu_type)
 {
     return qtest_qmp(qts, QUERY_HEAD "'model': { 'name': %s }"
@@ -352,7 +362,12 @@ 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 (!tcg_enabled(qts)) {
+        qtest_quit(qts);
+        return;
+    }
 
     assert_sve_vls(qts, "max", BIT_ULL(8) - 1, NULL);
 
@@ -387,7 +402,12 @@ 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 (!tcg_enabled(qts)) {
+        qtest_quit(qts);
+        return;
+    }
 
     /* SVE is off, so the map should be empty. */
     assert_sve_vls(qts, "max", 0, NULL);
@@ -443,7 +463,12 @@ 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 (!tcg_enabled(qts)) {
+        qtest_quit(qts);
+        return;
+    }
 
     /* Test common query-cpu-model-expansion input validation */
     assert_type_full(qts);
-- 
2.26.2

Re: [PATCH 5/9] hw/arm/virt: Improve CPU name in help message

[Philippe Mathieu-Daudé]

On 2/5/21 3:43 PM, Philippe Mathieu-Daudé wrote:
> When selecting an incorrect CPU, there is a mismatch between the
> CPU name provided and the one displayed (which is some QEMU internal
> name):
> 
>   $ qemu-system-aarch64 -M virt -cpu cortex-a8
>   qemu-system-aarch64: mach-virt: CPU type cortex-a8-arm-cpu not supported
> 
> Strip the suffix to display the correct CPU name:
> 
>   $ qemu-system-aarch64 -M virt -cpu cortex-a8
>   qemu-system-aarch64: mach-virt: CPU type cortex-a8-arm

Grrr wrong paste. Should be:

  qemu-system-aarch64: mach-virt: CPU type cortex-a8 not supported

> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/arm/virt.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 399da734548..7802d3a66e8 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1829,7 +1829,10 @@ static void machvirt_init(MachineState *machine)
>      finalize_gic_version(vms);
>  
>      if (!cpu_type_valid(machine->cpu_type)) {
> -        error_report("mach-virt: CPU type %s not supported", machine->cpu_type);
> +        int len = strlen(machine->cpu_type) - strlen(ARM_CPU_TYPE_SUFFIX);
> +
> +        error_report("mach-virt: CPU type %.*s not supported",
> +                     len, machine->cpu_type);
>          exit(1);
>      }
>  
> 

Re: [PATCH 1/9] tests/qtest/arm-cpu-features: Remove Cortex-A15 check

[Andrew Jones]

On Fri, Feb 05, 2021 at 03:43:37PM +0100, Philippe Mathieu-Daudé wrote:
> Support for ARMv7 has been dropped in commit 82bf7ae84ce
> ("target/arm: Remove KVM support for 32-bit Arm hosts"),
> no need to check for Cortex A15 host cpu anymore.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  tests/qtest/arm-cpu-features.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/tests/qtest/arm-cpu-features.c b/tests/qtest/arm-cpu-features.c
> index 8252b85bb85..c59c3cb002b 100644
> --- a/tests/qtest/arm-cpu-features.c
> +++ b/tests/qtest/arm-cpu-features.c
> @@ -515,10 +515,6 @@ static void test_query_cpu_model_expansion_kvm(const void *data)
>          QDict *resp;
>          char *error;
>  
> -        assert_error(qts, "cortex-a15",
> -            "We cannot guarantee the CPU type 'cortex-a15' works "
> -            "with KVM on this host", NULL);
> -

This isn't testing anything regarding 32-bit KVM host support. It's
testing that an error is returned when a given cpu type that can't
be known to work with KVM is used. We know that the cortex-a15 can't
be known to work. If we were to use a 64-bit cpu type here then there's
a chance that it would work, failing the test that an error be returned.

>          assert_has_feature_enabled(qts, "host", "aarch64");
>  
>          /* Enabling and disabling pmu should always work. */
> -- 
> 2.26.2
> 
>

This file could use a cleanup patch regarding the dropping of 32-bit KVM
support though. At least the comment in main(), "For now we only run KVM
specific tests..." could be reworded. It was written that way when we
planned to try testing on 32-bit KVM too eventually, but we never did,
and now we'll never need to.

Thanks,
drew

Re: [PATCH 3/9] tests/qtest/boot-serial-test: Test Virt machine with 'max'

[Andrew Jones]

On Fri, Feb 05, 2021 at 03:43:39PM +0100, Philippe Mathieu-Daudé wrote:
> When using KVM, using a specific cpu type will only work if the
> host CPU really is that exact CPU type.
> 
> During testing we can simply use the 'max' CPU which will select
> all the features available from the host.
> 
> This allow running this test on a Cavium CN8890 (ThunderX cores).
> 
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  tests/qtest/boot-serial-test.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/qtest/boot-serial-test.c b/tests/qtest/boot-serial-test.c
> index b6b1c23cd01..d74509b1c57 100644
> --- a/tests/qtest/boot-serial-test.c
> +++ b/tests/qtest/boot-serial-test.c
> @@ -149,7 +149,7 @@ static testdef_t tests[] = {
>      { "arm", "raspi2", "", "TT", sizeof(bios_raspi2), 0, bios_raspi2 },
>      /* For hppa, force bios to output to serial by disabling graphics. */
>      { "hppa", "hppa", "-vga none", "SeaBIOS wants SYSTEM HALT" },
> -    { "aarch64", "virt", "-cpu cortex-a57", "TT", sizeof(kernel_aarch64),
> +    { "aarch64", "virt", "-cpu max", "TT", sizeof(kernel_aarch64),
>        kernel_aarch64 },
>      { "arm", "microbit", "", "T", sizeof(kernel_nrf51), kernel_nrf51 },
>  
> -- 
> 2.26.2
> 
>

Reviewed-by: Andrew Jones <drjones@redhat.com>

Re: [PATCH 4/9] tests/qtest/cdrom-test: Only allow the Virt machine under KVM

[Andrew Jones]

On Fri, Feb 05, 2021 at 03:43:40PM +0100, Philippe Mathieu-Daudé wrote:
> Only the Virt and Versal machines are supported under KVM.
> Restrict the other ones to TCG.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  tests/qtest/cdrom-test.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qtest/cdrom-test.c b/tests/qtest/cdrom-test.c
> index 5af944a5fb7..ac02f2bb4f1 100644
> --- a/tests/qtest/cdrom-test.c
> +++ b/tests/qtest/cdrom-test.c
> @@ -222,9 +222,12 @@ int main(int argc, char **argv)
>          add_cdrom_param_tests(mips64machines);
>      } else if (g_str_equal(arch, "arm") || g_str_equal(arch, "aarch64")) {
>          const char *armmachines[] = {
> +#ifdef CONFIG_TCG
>              "realview-eb", "realview-eb-mpcore", "realview-pb-a8",
>              "realview-pbx-a9", "versatileab", "versatilepb", "vexpress-a15",
> -            "vexpress-a9", "virt", NULL
> +            "vexpress-a9",
> +#endif /* CONFIG_TCG */
> +            "virt", NULL
>          };
>          add_cdrom_param_tests(armmachines);
>      } else {
> -- 
> 2.26.2
>

Don't we need to use a runtime check for this? I'd guess we can
build a QEMU that supports both KVM and TCG and then attempt to
run this test with KVM, which would still try all these other
machine types.

Thanks,
drew 

Re: [PATCH 5/9] hw/arm/virt: Improve CPU name in help message

[Andrew Jones]

On Fri, Feb 05, 2021 at 03:43:41PM +0100, Philippe Mathieu-Daudé wrote:
> When selecting an incorrect CPU, there is a mismatch between the
> CPU name provided and the one displayed (which is some QEMU internal
> name):
> 
>   $ qemu-system-aarch64 -M virt -cpu cortex-a8
>   qemu-system-aarch64: mach-virt: CPU type cortex-a8-arm-cpu not supported
> 
> Strip the suffix to display the correct CPU name:
> 
>   $ qemu-system-aarch64 -M virt -cpu cortex-a8
>   qemu-system-aarch64: mach-virt: CPU type cortex-a8-arm
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/arm/virt.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 399da734548..7802d3a66e8 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1829,7 +1829,10 @@ static void machvirt_init(MachineState *machine)
>      finalize_gic_version(vms);
>  
>      if (!cpu_type_valid(machine->cpu_type)) {
> -        error_report("mach-virt: CPU type %s not supported", machine->cpu_type);
> +        int len = strlen(machine->cpu_type) - strlen(ARM_CPU_TYPE_SUFFIX);
> +
> +        error_report("mach-virt: CPU type %.*s not supported",
> +                     len, machine->cpu_type);
>          exit(1);
>      }
>  
> -- 
> 2.26.2
>

Reviewed-by: Andrew Jones <drjones@redhat.com>

Re: [PATCH 6/9] hw/arm/virt: Display list of valid CPUs for the Virt machine

[Andrew Jones]

On Fri, Feb 05, 2021 at 03:43:42PM +0100, Philippe Mathieu-Daudé wrote:
> The Virt machine is restricted to a subset of the CPU provided
> by QEMU. Instead of having the user run '--cpu help' and try
> each CPUs until finding a match, display the list from start:
> 
>   $ qemu-system-aarch64 -M virt -cpu cortex-a8
>   qemu-system-aarch64: mach-virt: CPU type cortex-a8 not supported
>   qemu-system-aarch64: mach-virt: Please select one of the following CPU types:  cortex-a7, cortex-a15, cortex-a53, cortex-a57, cortex-a72, host, max
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/arm/virt.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 7802d3a66e8..6ffe091804f 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1830,9 +1830,20 @@ static void machvirt_init(MachineState *machine)
>  
>      if (!cpu_type_valid(machine->cpu_type)) {
>          int len = strlen(machine->cpu_type) - strlen(ARM_CPU_TYPE_SUFFIX);
> +        g_autoptr(GString) s = g_string_new(NULL);
>  
>          error_report("mach-virt: CPU type %.*s not supported",
>                       len, machine->cpu_type);
> +
> +        for (n = 0; n < ARRAY_SIZE(valid_cpus); n++) {
> +            len = strlen(valid_cpus[n]) - strlen(ARM_CPU_TYPE_SUFFIX);
> +            g_string_append_printf(s, " %.*s", len, valid_cpus[n]);
> +            if (n + 1 < ARRAY_SIZE(valid_cpus)) {
> +                g_string_append_c(s, ',');
> +            }
> +        }
> +        error_report("mach-virt: Please select one of the following CPU types: %s",
> +                     g_string_free(s, FALSE));
>          exit(1);
>      }
>  
> -- 
> 2.26.2
>

It'd be nice if './qemu-system-aarch64 -M virt -cpu \?' would only output
the CPUs that the virt machine type supports. Then this error message
could suggest running that in order to get the list.

Thanks,
drew

Re: [PATCH 7/9] hw/arm/virt: Do not include 64-bit CPUs in 32-bit build

[Andrew Jones]

On Fri, Feb 05, 2021 at 03:43:43PM +0100, Philippe Mathieu-Daudé wrote:
> Similarly to commit 210f47840dd, remove 64-bit CPUs (which have
> never been available on 32-bit build, see commit d14d42f19bf),
> to fix:
> 
>   $ make check-qtest-arm
>   ...
>   Running test qtest-arm/device-introspect-test
>   missing object type 'cortex-a53-arm-cpu'
>   Broken pipe
>   ../tests/qtest/libqtest.c:181: kill_qemu() detected QEMU death from signal 6 (Aborted) (core dumped)
>   ERROR qtest-arm/device-introspect-test - too few tests run (expected 6, got 5)
> 
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/arm/virt.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 6ffe091804f..f5e4a6ec914 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -199,9 +199,11 @@ static const int a15irqmap[] = {
>  static const char *valid_cpus[] = {
>      ARM_CPU_TYPE_NAME("cortex-a7"),
>      ARM_CPU_TYPE_NAME("cortex-a15"),
> +#ifdef TARGET_AARCH64
>      ARM_CPU_TYPE_NAME("cortex-a53"),
>      ARM_CPU_TYPE_NAME("cortex-a57"),
>      ARM_CPU_TYPE_NAME("cortex-a72"),
> +#endif /* TARGET_AARCH64 */
>      ARM_CPU_TYPE_NAME("host"),
>      ARM_CPU_TYPE_NAME("max"),
>  };
> -- 
> 2.26.2
>

Reviewed-by: Andrew Jones <drjones@redhat.com>

Re: [PATCH 4/9] tests/qtest/cdrom-test: Only allow the Virt machine under KVM

[Peter Maydell]

On Fri, 5 Feb 2021 at 15:08, Andrew Jones <drjones@redhat.com> wrote:
>
> On Fri, Feb 05, 2021 at 03:43:40PM +0100, Philippe Mathieu-Daudé wrote:
> > Only the Virt and Versal machines are supported under KVM.
> > Restrict the other ones to TCG.
> >
> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > ---
> >  tests/qtest/cdrom-test.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/tests/qtest/cdrom-test.c b/tests/qtest/cdrom-test.c
> > index 5af944a5fb7..ac02f2bb4f1 100644
> > --- a/tests/qtest/cdrom-test.c
> > +++ b/tests/qtest/cdrom-test.c
> > @@ -222,9 +222,12 @@ int main(int argc, char **argv)
> >          add_cdrom_param_tests(mips64machines);
> >      } else if (g_str_equal(arch, "arm") || g_str_equal(arch, "aarch64")) {
> >          const char *armmachines[] = {
> > +#ifdef CONFIG_TCG
> >              "realview-eb", "realview-eb-mpcore", "realview-pb-a8",
> >              "realview-pbx-a9", "versatileab", "versatilepb", "vexpress-a15",
> > -            "vexpress-a9", "virt", NULL
> > +            "vexpress-a9",
> > +#endif /* CONFIG_TCG */
> > +            "virt", NULL
> >          };
> >          add_cdrom_param_tests(armmachines);
> >      } else {
> > --
> > 2.26.2
> >
>
> Don't we need to use a runtime check for this? I'd guess we can
> build a QEMU that supports both KVM and TCG and then attempt to
> run this test with KVM, which would still try all these other
> machine types.

More generally, it would be nice to avoid hardcoding into the
tests what accelerators particular machines work with, because
then if we move a machine into or out of the "TCG-only" list
we now have multiple places to update. Ideally we should
be able to just change the main meson.build files and have
everything else cope.

-- PMM

Re: [PATCH 1/9] tests/qtest/arm-cpu-features: Remove Cortex-A15 check

[Philippe Mathieu-Daudé]

Hi Drew,

On 2/5/21 3:59 PM, Andrew Jones wrote:
> On Fri, Feb 05, 2021 at 03:43:37PM +0100, Philippe Mathieu-Daudé wrote:
>> Support for ARMv7 has been dropped in commit 82bf7ae84ce
>> ("target/arm: Remove KVM support for 32-bit Arm hosts"),
>> no need to check for Cortex A15 host cpu anymore.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  tests/qtest/arm-cpu-features.c | 4 ----
>>  1 file changed, 4 deletions(-)
>>
>> diff --git a/tests/qtest/arm-cpu-features.c b/tests/qtest/arm-cpu-features.c
>> index 8252b85bb85..c59c3cb002b 100644
>> --- a/tests/qtest/arm-cpu-features.c
>> +++ b/tests/qtest/arm-cpu-features.c
>> @@ -515,10 +515,6 @@ static void test_query_cpu_model_expansion_kvm(const void *data)
>>          QDict *resp;
>>          char *error;
>>  
>> -        assert_error(qts, "cortex-a15",
>> -            "We cannot guarantee the CPU type 'cortex-a15' works "
>> -            "with KVM on this host", NULL);
>> -
> 
> This isn't testing anything regarding 32-bit KVM host support. It's
> testing that an error is returned when a given cpu type that can't
> be known to work with KVM is used. We know that the cortex-a15 can't
> be known to work. If we were to use a 64-bit cpu type here then there's
> a chance that it would work, failing the test that an error be returned.

This was my first understanding, but then why does it fail?

PASS 1 qtest-aarch64/arm-cpu-features /aarch64/arm/query-cpu-model-expansion
**
ERROR:../../tests/qtest/arm-cpu-features.c:543:test_query_cpu_model_expansion_kvm:
assertion failed: (g_str_equal(_error, "We cannot guarantee the CPU type
'cortex-a15' works " "with KVM on this host"))
ERROR qtest-aarch64/arm-cpu-features - Bail out!
ERROR:../../tests/qtest/arm-cpu-features.c:543:test_query_cpu_model_expansion_kvm:
assertion failed: (g_str_equal(_error, "We cannot guarantee the CPU type
'cortex-a15' works " "with KVM on this host"))
make: *** [Makefile.mtest:905: run-test-111] Error 1

FWIW when tracing (cavium thunderX1 host, dmesg reports 0x431f0a11):
kvm_vcpu_ioctl cpu_index 0, type 0x4020aeae, arg 0xffff9b7f9b18

> 
>>          assert_has_feature_enabled(qts, "host", "aarch64");
>>  
>>          /* Enabling and disabling pmu should always work. */
>> -- 
>> 2.26.2
>>
>>
> 
> This file could use a cleanup patch regarding the dropping of 32-bit KVM
> support though. At least the comment in main(), "For now we only run KVM
> specific tests..." could be reworded. It was written that way when we
> planned to try testing on 32-bit KVM too eventually, but we never did,
> and now we'll never need to.
> 
> Thanks,
> drew
> 
> 

Re: [PATCH 8/9] hw/arm/virt: Restrict 32-bit CPUs to TCG

[Andrew Jones]

On Fri, Feb 05, 2021 at 03:43:44PM +0100, Philippe Mathieu-Daudé wrote:
> Support for ARMv7 has been dropped in commit 82bf7ae84ce
> ("target/arm: Remove KVM support for 32-bit Arm hosts").
> Restrict the 32-bit CPUs to --enable-tcg builds.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/arm/virt.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index f5e4a6ec914..ab6300650f9 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -197,8 +197,10 @@ static const int a15irqmap[] = {
>  };
>  
>  static const char *valid_cpus[] = {
> +#ifdef CONFIG_TCG
>      ARM_CPU_TYPE_NAME("cortex-a7"),
>      ARM_CPU_TYPE_NAME("cortex-a15"),
> +#endif /* CONFIG_TCG */
>  #ifdef TARGET_AARCH64
>      ARM_CPU_TYPE_NAME("cortex-a53"),
>      ARM_CPU_TYPE_NAME("cortex-a57"),
> -- 
> 2.26.2
>

So this filters the cpus out of KVM only builds, which seems
reasonable to do. Of course, if the build is for both KVM and
TCG, then the cpus won't be filtered out and we'll have to rely
on the runtime checks to error out if one where to try a 32-bit
cpu with KVM. But that's fine too, so

Reviewed-by: Andrew Jones <drjones@redhat.com>

Thanks,
drew

Re: [PATCH 4/9] tests/qtest/cdrom-test: Only allow the Virt machine under KVM

[Philippe Mathieu-Daudé]

On 2/5/21 4:08 PM, Andrew Jones wrote:
> On Fri, Feb 05, 2021 at 03:43:40PM +0100, Philippe Mathieu-Daudé wrote:
>> Only the Virt and Versal machines are supported under KVM.
>> Restrict the other ones to TCG.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  tests/qtest/cdrom-test.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/qtest/cdrom-test.c b/tests/qtest/cdrom-test.c
>> index 5af944a5fb7..ac02f2bb4f1 100644
>> --- a/tests/qtest/cdrom-test.c
>> +++ b/tests/qtest/cdrom-test.c
>> @@ -222,9 +222,12 @@ int main(int argc, char **argv)
>>          add_cdrom_param_tests(mips64machines);
>>      } else if (g_str_equal(arch, "arm") || g_str_equal(arch, "aarch64")) {
>>          const char *armmachines[] = {
>> +#ifdef CONFIG_TCG
>>              "realview-eb", "realview-eb-mpcore", "realview-pb-a8",
>>              "realview-pbx-a9", "versatileab", "versatilepb", "vexpress-a15",
>> -            "vexpress-a9", "virt", NULL
>> +            "vexpress-a9",
>> +#endif /* CONFIG_TCG */
>> +            "virt", NULL
>>          };
>>          add_cdrom_param_tests(armmachines);
>>      } else {
>> -- 
>> 2.26.2
>>
> 
> Don't we need to use a runtime check for this? I'd guess we can
> build a QEMU that supports both KVM and TCG and then attempt to
> run this test with KVM, which would still try all these other
> machine types.

Yes, I followed commit c51a5a23d87 fix ("qtest: unbreak non-TCG
builds in bios-tables-test").
We need that QMP 'query-accelerators' command then.

Re: [RFC PATCH 9/9] tests/qtest/arm-cpu-features: Restrict TCG-only tests

[Claudio Fontana]

On 2/5/21 3:43 PM, Philippe Mathieu-Daudé wrote:
> Some tests explicitly request the TCG accelerator. As these
> tests will obviously fails if TCG is not present, disable
> them in such case.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> Cc: Roman Bolshakov <r.bolshakov@yadro.com>
> Cc: Claudio Fontana <cfontana@suse.de>
> 
> RFC because of the TODO.
> 
> Roman posted a series to have a QMP command to query enabled
> accelerators.
> ---
>  tests/qtest/arm-cpu-features.c | 33 +++++++++++++++++++++++++++++----
>  1 file changed, 29 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/qtest/arm-cpu-features.c b/tests/qtest/arm-cpu-features.c
> index c59c3cb002b..c6e86282b66 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 -accel tcg "
>  #define QUERY_HEAD  "{ 'execute': 'query-cpu-model-expansion', " \
>                      "  'arguments': { 'type': 'full', "
> @@ -41,6 +41,16 @@ static bool kvm_enabled(QTestState *qts)
>      return enabled;
>  }
>  
> +static bool tcg_enabled(QTestState *qts)
> +{
> +    /* TODO: Implement QMP query-accel? */
> +#ifdef CONFIG_TCG
> +    return true;
> +#else
> +    return false;
> +#endif /* CONFIG_TCG */


I would not use the same name as the existing tcg_enabled(), which has different semantics, even in test code;

what you mean here is tcg_available() right?



> +}
> +
>  static QDict *do_query_no_props(QTestState *qts, const char *cpu_type)
>  {
>      return qtest_qmp(qts, QUERY_HEAD "'model': { 'name': %s }"
> @@ -352,7 +362,12 @@ 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 (!tcg_enabled(qts)) {
> +        qtest_quit(qts);
> +        return;
> +    }
>  
>      assert_sve_vls(qts, "max", BIT_ULL(8) - 1, NULL);
>  
> @@ -387,7 +402,12 @@ 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 (!tcg_enabled(qts)) {
> +        qtest_quit(qts);
> +        return;
> +    }
>  
>      /* SVE is off, so the map should be empty. */
>      assert_sve_vls(qts, "max", 0, NULL);
> @@ -443,7 +463,12 @@ 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 (!tcg_enabled(qts)) {
> +        qtest_quit(qts);
> +        return;
> +    }
>  
>      /* Test common query-cpu-model-expansion input validation */
>      assert_type_full(qts);
> 

Re: [PATCH 6/9] hw/arm/virt: Display list of valid CPUs for the Virt machine

[Philippe Mathieu-Daudé]

On 2/5/21 4:12 PM, Andrew Jones wrote:
> On Fri, Feb 05, 2021 at 03:43:42PM +0100, Philippe Mathieu-Daudé wrote:
>> The Virt machine is restricted to a subset of the CPU provided
>> by QEMU. Instead of having the user run '--cpu help' and try
>> each CPUs until finding a match, display the list from start:
>>
>>   $ qemu-system-aarch64 -M virt -cpu cortex-a8
>>   qemu-system-aarch64: mach-virt: CPU type cortex-a8 not supported
>>   qemu-system-aarch64: mach-virt: Please select one of the following CPU types:  cortex-a7, cortex-a15, cortex-a53, cortex-a57, cortex-a72, host, max
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  hw/arm/virt.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 7802d3a66e8..6ffe091804f 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -1830,9 +1830,20 @@ static void machvirt_init(MachineState *machine)
>>  
>>      if (!cpu_type_valid(machine->cpu_type)) {
>>          int len = strlen(machine->cpu_type) - strlen(ARM_CPU_TYPE_SUFFIX);
>> +        g_autoptr(GString) s = g_string_new(NULL);
>>  
>>          error_report("mach-virt: CPU type %.*s not supported",
>>                       len, machine->cpu_type);
>> +
>> +        for (n = 0; n < ARRAY_SIZE(valid_cpus); n++) {
>> +            len = strlen(valid_cpus[n]) - strlen(ARM_CPU_TYPE_SUFFIX);
>> +            g_string_append_printf(s, " %.*s", len, valid_cpus[n]);
>> +            if (n + 1 < ARRAY_SIZE(valid_cpus)) {
>> +                g_string_append_c(s, ',');
>> +            }
>> +        }
>> +        error_report("mach-virt: Please select one of the following CPU types: %s",
>> +                     g_string_free(s, FALSE));
>>          exit(1);
>>      }
>>  
>> -- 
>> 2.26.2
>>
> 
> It'd be nice if './qemu-system-aarch64 -M virt -cpu \?' would only output
> the CPUs that the virt machine type supports. Then this error message
> could suggest running that in order to get the list.

+1 very nice =) But not how the command line options processing
works. Maybe later after John Snow command line rework is merged?

Re: [RFC PATCH 9/9] tests/qtest/arm-cpu-features: Restrict TCG-only tests

[Andrew Jones]

On Fri, Feb 05, 2021 at 03:43:45PM +0100, Philippe Mathieu-Daudé wrote:
> Some tests explicitly request the TCG accelerator. As these
> tests will obviously fails if TCG is not present, disable
> them in such case.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> Cc: Roman Bolshakov <r.bolshakov@yadro.com>
> Cc: Claudio Fontana <cfontana@suse.de>
> 
> RFC because of the TODO.
> 
> Roman posted a series to have a QMP command to query enabled
> accelerators.
> ---
>  tests/qtest/arm-cpu-features.c | 33 +++++++++++++++++++++++++++++----
>  1 file changed, 29 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/qtest/arm-cpu-features.c b/tests/qtest/arm-cpu-features.c
> index c59c3cb002b..c6e86282b66 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 -accel tcg "

Should probably also drop the TCG fallback from MACHINE_KVM when
TCG is not present and then find another way to confirm KVM is
present in the kvm tests prior to calling qtest_init().

>  #define QUERY_HEAD  "{ 'execute': 'query-cpu-model-expansion', " \
>                      "  'arguments': { 'type': 'full', "
> @@ -41,6 +41,16 @@ static bool kvm_enabled(QTestState *qts)
>      return enabled;
>  }
>  
> +static bool tcg_enabled(QTestState *qts)
> +{
> +    /* TODO: Implement QMP query-accel? */
> +#ifdef CONFIG_TCG
> +    return true;
> +#else
> +    return false;
> +#endif /* CONFIG_TCG */
> +}
> +
>  static QDict *do_query_no_props(QTestState *qts, const char *cpu_type)
>  {
>      return qtest_qmp(qts, QUERY_HEAD "'model': { 'name': %s }"
> @@ -352,7 +362,12 @@ 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");

Won't this fail when TCG isn't present? If so, then the test will
either have already aborted or at least qts can't be passed to
tcg_enabled().

> +
> +    if (!tcg_enabled(qts)) {
> +        qtest_quit(qts);
> +        return;
> +    }
>  
>      assert_sve_vls(qts, "max", BIT_ULL(8) - 1, NULL);
>  
> @@ -387,7 +402,12 @@ 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 (!tcg_enabled(qts)) {
> +        qtest_quit(qts);
> +        return;
> +    }
>  
>      /* SVE is off, so the map should be empty. */
>      assert_sve_vls(qts, "max", 0, NULL);
> @@ -443,7 +463,12 @@ 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 (!tcg_enabled(qts)) {
> +        qtest_quit(qts);
> +        return;
> +    }
>  
>      /* Test common query-cpu-model-expansion input validation */
>      assert_type_full(qts);
> -- 
> 2.26.2
>

Thanks,
drew 

Re: [RFC PATCH 9/9] tests/qtest/arm-cpu-features: Restrict TCG-only tests

[Philippe Mathieu-Daudé]

On 2/5/21 4:20 PM, Claudio Fontana wrote:
> On 2/5/21 3:43 PM, Philippe Mathieu-Daudé wrote:
>> Some tests explicitly request the TCG accelerator. As these
>> tests will obviously fails if TCG is not present, disable
>> them in such case.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>> Cc: Roman Bolshakov <r.bolshakov@yadro.com>
>> Cc: Claudio Fontana <cfontana@suse.de>
>>
>> RFC because of the TODO.
>>
>> Roman posted a series to have a QMP command to query enabled
>> accelerators.
>> ---
>>  tests/qtest/arm-cpu-features.c | 33 +++++++++++++++++++++++++++++----
>>  1 file changed, 29 insertions(+), 4 deletions(-)
>>
>> diff --git a/tests/qtest/arm-cpu-features.c b/tests/qtest/arm-cpu-features.c
>> index c59c3cb002b..c6e86282b66 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 -accel tcg "
>>  #define QUERY_HEAD  "{ 'execute': 'query-cpu-model-expansion', " \
>>                      "  'arguments': { 'type': 'full', "
>> @@ -41,6 +41,16 @@ static bool kvm_enabled(QTestState *qts)
>>      return enabled;
>>  }
>>  
>> +static bool tcg_enabled(QTestState *qts)
>> +{
>> +    /* TODO: Implement QMP query-accel? */
>> +#ifdef CONFIG_TCG
>> +    return true;
>> +#else
>> +    return false;
>> +#endif /* CONFIG_TCG */
> 
> 
> I would not use the same name as the existing tcg_enabled(), which has different semantics, even in test code;
> 
> what you mean here is tcg_available() right?

No, I meant static tcg_enabled as the kvm_enabled() earlier method:

static bool kvm_enabled(QTestState *qts)
{
    QDict *resp, *qdict;
    bool enabled;

    resp = qtest_qmp(qts, "{ 'execute': 'query-kvm' }");
    g_assert(qdict_haskey(resp, "return"));
    qdict = qdict_get_qdict(resp, "return");
    g_assert(qdict_haskey(qdict, "enabled"));
    enabled = qdict_get_bool(qdict, "enabled");
    qobject_unref(resp);

    return enabled;
}

This should be moved to something generic to QTest IMO,
and we need some runtime qtest_is_accelerator_enabled().

Re: [PATCH 0/9] hw/arm/virt: Improve CPU help and fix testing under KVM

[Philippe Mathieu-Daudé]

On 2/5/21 3:43 PM, Philippe Mathieu-Daudé wrote:
> Yet again bugfixes and cleanup patches noticed while
> rebasing my "Support disabling TCG on ARM (part 2)" series.
> 
> Sending them independently as they aren't directly dependent
> of it so don't have to be delayed by other unanswered questions.

Proven wrong 45min later, not trivial and not ready yet =)

> Please review,
> 
> Phil.

Re: [PATCH 1/9] tests/qtest/arm-cpu-features: Remove Cortex-A15 check

[Andrew Jones]

On Fri, Feb 05, 2021 at 04:15:45PM +0100, Philippe Mathieu-Daudé wrote:
> Hi Drew,
> 
> On 2/5/21 3:59 PM, Andrew Jones wrote:
> > On Fri, Feb 05, 2021 at 03:43:37PM +0100, Philippe Mathieu-Daudé wrote:
> >> Support for ARMv7 has been dropped in commit 82bf7ae84ce
> >> ("target/arm: Remove KVM support for 32-bit Arm hosts"),
> >> no need to check for Cortex A15 host cpu anymore.
> >>
> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> >> ---
> >>  tests/qtest/arm-cpu-features.c | 4 ----
> >>  1 file changed, 4 deletions(-)
> >>
> >> diff --git a/tests/qtest/arm-cpu-features.c b/tests/qtest/arm-cpu-features.c
> >> index 8252b85bb85..c59c3cb002b 100644
> >> --- a/tests/qtest/arm-cpu-features.c
> >> +++ b/tests/qtest/arm-cpu-features.c
> >> @@ -515,10 +515,6 @@ static void test_query_cpu_model_expansion_kvm(const void *data)
> >>          QDict *resp;
> >>          char *error;
> >>  
> >> -        assert_error(qts, "cortex-a15",
> >> -            "We cannot guarantee the CPU type 'cortex-a15' works "
> >> -            "with KVM on this host", NULL);
> >> -
> > 
> > This isn't testing anything regarding 32-bit KVM host support. It's
> > testing that an error is returned when a given cpu type that can't
> > be known to work with KVM is used. We know that the cortex-a15 can't
> > be known to work. If we were to use a 64-bit cpu type here then there's
> > a chance that it would work, failing the test that an error be returned.
> 
> This was my first understanding, but then why does it fail?
> 
> PASS 1 qtest-aarch64/arm-cpu-features /aarch64/arm/query-cpu-model-expansion
> **
> ERROR:../../tests/qtest/arm-cpu-features.c:543:test_query_cpu_model_expansion_kvm:
> assertion failed: (g_str_equal(_error, "We cannot guarantee the CPU type
> 'cortex-a15' works " "with KVM on this host"))
> ERROR qtest-aarch64/arm-cpu-features - Bail out!
> ERROR:../../tests/qtest/arm-cpu-features.c:543:test_query_cpu_model_expansion_kvm:
> assertion failed: (g_str_equal(_error, "We cannot guarantee the CPU type
> 'cortex-a15' works " "with KVM on this host"))
> make: *** [Makefile.mtest:905: run-test-111] Error 1
> 
> FWIW when tracing (cavium thunderX1 host, dmesg reports 0x431f0a11):
> kvm_vcpu_ioctl cpu_index 0, type 0x4020aeae, arg 0xffff9b7f9b18

Hmm... I don't know. It works for me

$ QTEST_QEMU_BINARY=./qemu-system-aarch64 ./tests/qtest/arm-cpu-features
/aarch64/arm/query-cpu-model-expansion: OK
/aarch64/arm/kvm/query-cpu-model-expansion: OK
/aarch64/arm/kvm/query-cpu-model-expansion/sve-off: OK
/aarch64/arm/max/query-cpu-model-expansion/sve-max-vq-8: OK
/aarch64/arm/max/query-cpu-model-expansion/sve-off: OK

$ lscpu
Architecture:        aarch64
Byte Order:          Little Endian
CPU(s):              48
On-line CPU(s) list: 0-47
Thread(s) per core:  1
Core(s) per cluster: 16
Socket(s):           -
Cluster(s):          3
NUMA node(s):        1
Vendor ID:           Cavium
Model:               1
Model name:          ThunderX 88XX
Stepping:            0x1
BogoMIPS:            200.00
NUMA node0 CPU(s):   0-47
Flags:               fp asimd evtstrm aes pmull sha1 sha2 crc32 cpuid

> 
> > 
> >>          assert_has_feature_enabled(qts, "host", "aarch64");
> >>  
> >>          /* Enabling and disabling pmu should always work. */
> >> -- 
> >> 2.26.2
> >>
> >>
> > 
> > This file could use a cleanup patch regarding the dropping of 32-bit KVM
> > support though. At least the comment in main(), "For now we only run KVM
> > specific tests..." could be reworded. It was written that way when we
> > planned to try testing on 32-bit KVM too eventually, but we never did,
> > and now we'll never need to.
> > 
> > Thanks,
> > drew
> > 
> > 
> 

Re: [RFC PATCH 9/9] tests/qtest/arm-cpu-features: Restrict TCG-only tests

[Claudio Fontana]

On 2/5/21 4:30 PM, Philippe Mathieu-Daudé wrote:
> On 2/5/21 4:20 PM, Claudio Fontana wrote:
>> On 2/5/21 3:43 PM, Philippe Mathieu-Daudé wrote:
>>> Some tests explicitly request the TCG accelerator. As these
>>> tests will obviously fails if TCG is not present, disable
>>> them in such case.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> ---
>>> Cc: Roman Bolshakov <r.bolshakov@yadro.com>
>>> Cc: Claudio Fontana <cfontana@suse.de>
>>>
>>> RFC because of the TODO.
>>>
>>> Roman posted a series to have a QMP command to query enabled
>>> accelerators.
>>> ---
>>>  tests/qtest/arm-cpu-features.c | 33 +++++++++++++++++++++++++++++----
>>>  1 file changed, 29 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/tests/qtest/arm-cpu-features.c b/tests/qtest/arm-cpu-features.c
>>> index c59c3cb002b..c6e86282b66 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 -accel tcg "
>>>  #define QUERY_HEAD  "{ 'execute': 'query-cpu-model-expansion', " \
>>>                      "  'arguments': { 'type': 'full', "
>>> @@ -41,6 +41,16 @@ static bool kvm_enabled(QTestState *qts)
>>>      return enabled;
>>>  }
>>>  
>>> +static bool tcg_enabled(QTestState *qts)
>>> +{
>>> +    /* TODO: Implement QMP query-accel? */
>>> +#ifdef CONFIG_TCG
>>> +    return true;
>>> +#else
>>> +    return false;
>>> +#endif /* CONFIG_TCG */
>>
>>
>> I would not use the same name as the existing tcg_enabled(), which has different semantics, even in test code;
>>
>> what you mean here is tcg_available() right?
> 
> No, I meant static tcg_enabled as the kvm_enabled() earlier method:

Aha, so it's the other way around, we are actually testing if the TCG accelerator is currently selected in QEMU,
and the patch implements it using CONFIG_TCG as a placeholder for it, since we do not have query-accel yet, got it.

> 
> static bool kvm_enabled(QTestState *qts)
> {
>     QDict *resp, *qdict;
>     bool enabled;
> 
>     resp = qtest_qmp(qts, "{ 'execute': 'query-kvm' }");
>     g_assert(qdict_haskey(resp, "return"));
>     qdict = qdict_get_qdict(resp, "return");
>     g_assert(qdict_haskey(qdict, "enabled"));
>     enabled = qdict_get_bool(qdict, "enabled");
>     qobject_unref(resp);
> 
>     return enabled;
> }
> 
> This should be moved to something generic to QTest IMO,
> and we need some runtime qtest_is_accelerator_enabled().
> 

Agreed,

thanks,

Claudio

Re: [PATCH 1/9] tests/qtest/arm-cpu-features: Remove Cortex-A15 check

[Philippe Mathieu-Daudé]

On 2/5/21 4:33 PM, Andrew Jones wrote:
> On Fri, Feb 05, 2021 at 04:15:45PM +0100, Philippe Mathieu-Daudé wrote:
>> Hi Drew,
>>
>> On 2/5/21 3:59 PM, Andrew Jones wrote:
>>> On Fri, Feb 05, 2021 at 03:43:37PM +0100, Philippe Mathieu-Daudé wrote:
>>>> Support for ARMv7 has been dropped in commit 82bf7ae84ce
>>>> ("target/arm: Remove KVM support for 32-bit Arm hosts"),
>>>> no need to check for Cortex A15 host cpu anymore.
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>> ---
>>>>  tests/qtest/arm-cpu-features.c | 4 ----
>>>>  1 file changed, 4 deletions(-)
>>>>
>>>> diff --git a/tests/qtest/arm-cpu-features.c b/tests/qtest/arm-cpu-features.c
>>>> index 8252b85bb85..c59c3cb002b 100644
>>>> --- a/tests/qtest/arm-cpu-features.c
>>>> +++ b/tests/qtest/arm-cpu-features.c
>>>> @@ -515,10 +515,6 @@ static void test_query_cpu_model_expansion_kvm(const void *data)
>>>>          QDict *resp;
>>>>          char *error;
>>>>  
>>>> -        assert_error(qts, "cortex-a15",
>>>> -            "We cannot guarantee the CPU type 'cortex-a15' works "
>>>> -            "with KVM on this host", NULL);
>>>> -
>>>
>>> This isn't testing anything regarding 32-bit KVM host support. It's
>>> testing that an error is returned when a given cpu type that can't
>>> be known to work with KVM is used. We know that the cortex-a15 can't
>>> be known to work. If we were to use a 64-bit cpu type here then there's
>>> a chance that it would work, failing the test that an error be returned.
>>
>> This was my first understanding, but then why does it fail?
>>
>> PASS 1 qtest-aarch64/arm-cpu-features /aarch64/arm/query-cpu-model-expansion
>> **
>> ERROR:../../tests/qtest/arm-cpu-features.c:543:test_query_cpu_model_expansion_kvm:
>> assertion failed: (g_str_equal(_error, "We cannot guarantee the CPU type
>> 'cortex-a15' works " "with KVM on this host"))
>> ERROR qtest-aarch64/arm-cpu-features - Bail out!
>> ERROR:../../tests/qtest/arm-cpu-features.c:543:test_query_cpu_model_expansion_kvm:
>> assertion failed: (g_str_equal(_error, "We cannot guarantee the CPU type
>> 'cortex-a15' works " "with KVM on this host"))
>> make: *** [Makefile.mtest:905: run-test-111] Error 1
>>
>> FWIW when tracing (cavium thunderX1 host, dmesg reports 0x431f0a11):
>> kvm_vcpu_ioctl cpu_index 0, type 0x4020aeae, arg 0xffff9b7f9b18
> 
> Hmm... I don't know. It works for me
> 
> $ QTEST_QEMU_BINARY=./qemu-system-aarch64 ./tests/qtest/arm-cpu-features
> /aarch64/arm/query-cpu-model-expansion: OK
> /aarch64/arm/kvm/query-cpu-model-expansion: OK
> /aarch64/arm/kvm/query-cpu-model-expansion/sve-off: OK
> /aarch64/arm/max/query-cpu-model-expansion/sve-max-vq-8: OK
> /aarch64/arm/max/query-cpu-model-expansion/sve-off: OK

Thanks, that helped.

I ran my tests including the "Restrict v7A TCG cpus to TCG accel"
patch which removes the A15 in KVM-only build:
https://lists.gnu.org/archive/html/qemu-devel/2021-01/msg08051.html
 So when TCG is disabled,

So I get:

{ "execute": "query-cpu-model-expansion", 'arguments': { 'type': 'full',
'model': { 'name': 'cortex-a15' }}}
{
    "error": {
        "class": "GenericError",
        "desc": "The CPU type 'cortex-a15' is not a recognized ARM CPU type"
    }
}

which fails the g_str_equal().

BTW is there some easy way to dump QMP traffic on stdio?

> 
> $ lscpu
> Architecture:        aarch64
> Byte Order:          Little Endian
> CPU(s):              48
> On-line CPU(s) list: 0-47
> Thread(s) per core:  1
> Core(s) per cluster: 16
> Socket(s):           -
> Cluster(s):          3
> NUMA node(s):        1
> Vendor ID:           Cavium
> Model:               1
> Model name:          ThunderX 88XX
> Stepping:            0x1
> BogoMIPS:            200.00
> NUMA node0 CPU(s):   0-47
> Flags:               fp asimd evtstrm aes pmull sha1 sha2 crc32 cpuid
> 
>>
>>>
>>>>          assert_has_feature_enabled(qts, "host", "aarch64");
>>>>  
>>>>          /* Enabling and disabling pmu should always work. */
>>>> -- 
>>>> 2.26.2
>>>>
>>>>
>>>
>>> This file could use a cleanup patch regarding the dropping of 32-bit KVM
>>> support though. At least the comment in main(), "For now we only run KVM
>>> specific tests..." could be reworded. It was written that way when we
>>> planned to try testing on 32-bit KVM too eventually, but we never did,
>>> and now we'll never need to.
>>>
>>> Thanks,
>>> drew
>>>
>>>
>>
> 
> 

Re: [PATCH 2/9] tests/qtest: Restrict xlnx-can-test to TCG builds

[Alistair Francis]

On Fri, Feb 5, 2021 at 6:45 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> The Xilinx CAN controller test is uses the ZCU102 board which is
> based on a ZynqMP SoC. In the default configuration - used by this
> test - this SoC creates 2 Cortex R5F cores. Such cores are not
> v8A archicture, thus can not be run under KVM. Therefore restrict
> this test to TCG.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
> Cc: Alistair Francis <alistair@alistair23.me>
> Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
> Cc: Vikram Garhwal <fnu.vikram@xilinx.com>
> ---
>  tests/qtest/meson.build | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> index c83bc211b6a..d8ebd5bf98e 100644
> --- a/tests/qtest/meson.build
> +++ b/tests/qtest/meson.build
> @@ -159,10 +159,10 @@
>    (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'] : []) +  \
> +  (config_all.has_key('CONFIG_TCG') ? ['xlnx-can-test'] : []) +  \
>    ['arm-cpu-features',
>     'numa-test',
>     'boot-serial-test',
> -   'xlnx-can-test',
>     'migration-test']
>
>  qtests_s390x = \
> --
> 2.26.2
>
>

Re: [PATCH 2/9] tests/qtest: Restrict xlnx-can-test to TCG builds

[Peter Maydell]

On Fri, 5 Feb 2021 at 14:43, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> The Xilinx CAN controller test is uses the ZCU102 board which is
> based on a ZynqMP SoC. In the default configuration - used by this
> test - this SoC creates 2 Cortex R5F cores. Such cores are not
> v8A archicture, thus can not be run under KVM. Therefore restrict
> this test to TCG.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> Cc: Alistair Francis <alistair@alistair23.me>
> Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
> Cc: Vikram Garhwal <fnu.vikram@xilinx.com>
> ---
>  tests/qtest/meson.build | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> index c83bc211b6a..d8ebd5bf98e 100644
> --- a/tests/qtest/meson.build
> +++ b/tests/qtest/meson.build
> @@ -159,10 +159,10 @@
>    (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'] : []) +  \
> +  (config_all.has_key('CONFIG_TCG') ? ['xlnx-can-test'] : []) +  \
>    ['arm-cpu-features',
>     'numa-test',
>     'boot-serial-test',
> -   'xlnx-can-test',
>     'migration-test']

The implementation in hw/net/can/meson.build is conditioned on
CONFIG_XLNX_ZYNQMP -- does it work to use that here too?

thanks
-- PMM

Re: [PATCH 2/9] tests/qtest: Restrict xlnx-can-test to TCG builds

[Philippe Mathieu-Daudé]

On 2/5/21 5:57 PM, Peter Maydell wrote:
> On Fri, 5 Feb 2021 at 14:43, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> The Xilinx CAN controller test is uses the ZCU102 board which is
>> based on a ZynqMP SoC. In the default configuration - used by this
>> test - this SoC creates 2 Cortex R5F cores. Such cores are not
>> v8A archicture, thus can not be run under KVM. Therefore restrict
>> this test to TCG.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>> Cc: Alistair Francis <alistair@alistair23.me>
>> Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
>> Cc: Vikram Garhwal <fnu.vikram@xilinx.com>
>> ---
>>  tests/qtest/meson.build | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
>> index c83bc211b6a..d8ebd5bf98e 100644
>> --- a/tests/qtest/meson.build
>> +++ b/tests/qtest/meson.build
>> @@ -159,10 +159,10 @@
>>    (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'] : []) +  \
>> +  (config_all.has_key('CONFIG_TCG') ? ['xlnx-can-test'] : []) +  \
>>    ['arm-cpu-features',
>>     'numa-test',
>>     'boot-serial-test',
>> -   'xlnx-can-test',
>>     'migration-test']
> 
> The implementation in hw/net/can/meson.build is conditioned on
> CONFIG_XLNX_ZYNQMP -- does it work to use that here too?

Yes. Thanks, clever idea :)

Re: [PATCH 1/9] tests/qtest/arm-cpu-features: Remove Cortex-A15 check

[Andrew Jones]

On Fri, Feb 05, 2021 at 05:03:08PM +0100, Philippe Mathieu-Daudé wrote:
> On 2/5/21 4:33 PM, Andrew Jones wrote:
> > On Fri, Feb 05, 2021 at 04:15:45PM +0100, Philippe Mathieu-Daudé wrote:
> >> Hi Drew,
> >>
> >> On 2/5/21 3:59 PM, Andrew Jones wrote:
> >>> On Fri, Feb 05, 2021 at 03:43:37PM +0100, Philippe Mathieu-Daudé wrote:
> >>>> Support for ARMv7 has been dropped in commit 82bf7ae84ce
> >>>> ("target/arm: Remove KVM support for 32-bit Arm hosts"),
> >>>> no need to check for Cortex A15 host cpu anymore.
> >>>>
> >>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> >>>> ---
> >>>>  tests/qtest/arm-cpu-features.c | 4 ----
> >>>>  1 file changed, 4 deletions(-)
> >>>>
> >>>> diff --git a/tests/qtest/arm-cpu-features.c b/tests/qtest/arm-cpu-features.c
> >>>> index 8252b85bb85..c59c3cb002b 100644
> >>>> --- a/tests/qtest/arm-cpu-features.c
> >>>> +++ b/tests/qtest/arm-cpu-features.c
> >>>> @@ -515,10 +515,6 @@ static void test_query_cpu_model_expansion_kvm(const void *data)
> >>>>          QDict *resp;
> >>>>          char *error;
> >>>>  
> >>>> -        assert_error(qts, "cortex-a15",
> >>>> -            "We cannot guarantee the CPU type 'cortex-a15' works "
> >>>> -            "with KVM on this host", NULL);
> >>>> -
> >>>
> >>> This isn't testing anything regarding 32-bit KVM host support. It's
> >>> testing that an error is returned when a given cpu type that can't
> >>> be known to work with KVM is used. We know that the cortex-a15 can't
> >>> be known to work. If we were to use a 64-bit cpu type here then there's
> >>> a chance that it would work, failing the test that an error be returned.
> >>
> >> This was my first understanding, but then why does it fail?
> >>
> >> PASS 1 qtest-aarch64/arm-cpu-features /aarch64/arm/query-cpu-model-expansion
> >> **
> >> ERROR:../../tests/qtest/arm-cpu-features.c:543:test_query_cpu_model_expansion_kvm:
> >> assertion failed: (g_str_equal(_error, "We cannot guarantee the CPU type
> >> 'cortex-a15' works " "with KVM on this host"))
> >> ERROR qtest-aarch64/arm-cpu-features - Bail out!
> >> ERROR:../../tests/qtest/arm-cpu-features.c:543:test_query_cpu_model_expansion_kvm:
> >> assertion failed: (g_str_equal(_error, "We cannot guarantee the CPU type
> >> 'cortex-a15' works " "with KVM on this host"))
> >> make: *** [Makefile.mtest:905: run-test-111] Error 1
> >>
> >> FWIW when tracing (cavium thunderX1 host, dmesg reports 0x431f0a11):
> >> kvm_vcpu_ioctl cpu_index 0, type 0x4020aeae, arg 0xffff9b7f9b18
> > 
> > Hmm... I don't know. It works for me
> > 
> > $ QTEST_QEMU_BINARY=./qemu-system-aarch64 ./tests/qtest/arm-cpu-features
> > /aarch64/arm/query-cpu-model-expansion: OK
> > /aarch64/arm/kvm/query-cpu-model-expansion: OK
> > /aarch64/arm/kvm/query-cpu-model-expansion/sve-off: OK
> > /aarch64/arm/max/query-cpu-model-expansion/sve-max-vq-8: OK
> > /aarch64/arm/max/query-cpu-model-expansion/sve-off: OK
> 
> Thanks, that helped.
> 
> I ran my tests including the "Restrict v7A TCG cpus to TCG accel"
> patch which removes the A15 in KVM-only build:
> https://lists.gnu.org/archive/html/qemu-devel/2021-01/msg08051.html

Oh, I see. So this patch is OK if we merge "Restrict v7A TCG cpus to
TCG accel", but we should change the commit message of this patch
to point to the real reason for it, which is "Restrict v7A TCG cpus to
TCG accel". Also, I'd prefer we don't remove this test, but rather find
another way to perform it without the cortex-a15 cpu type.

>  So when TCG is disabled,
> 
> So I get:
> 
> { "execute": "query-cpu-model-expansion", 'arguments': { 'type': 'full',
> 'model': { 'name': 'cortex-a15' }}}
> {
>     "error": {
>         "class": "GenericError",
>         "desc": "The CPU type 'cortex-a15' is not a recognized ARM CPU type"
>     }
> }
> 
> which fails the g_str_equal().
> 
> BTW is there some easy way to dump QMP traffic on stdio?

You can use scripts/qmp/qmp-shell to manually test stuff.

Thanks,
drew

> 
> > 
> > $ lscpu
> > Architecture:        aarch64
> > Byte Order:          Little Endian
> > CPU(s):              48
> > On-line CPU(s) list: 0-47
> > Thread(s) per core:  1
> > Core(s) per cluster: 16
> > Socket(s):           -
> > Cluster(s):          3
> > NUMA node(s):        1
> > Vendor ID:           Cavium
> > Model:               1
> > Model name:          ThunderX 88XX
> > Stepping:            0x1
> > BogoMIPS:            200.00
> > NUMA node0 CPU(s):   0-47
> > Flags:               fp asimd evtstrm aes pmull sha1 sha2 crc32 cpuid
> > 
> >>
> >>>
> >>>>          assert_has_feature_enabled(qts, "host", "aarch64");
> >>>>  
> >>>>          /* Enabling and disabling pmu should always work. */
> >>>> -- 
> >>>> 2.26.2
> >>>>
> >>>>
> >>>
> >>> This file could use a cleanup patch regarding the dropping of 32-bit KVM
> >>> support though. At least the comment in main(), "For now we only run KVM
> >>> specific tests..." could be reworded. It was written that way when we
> >>> planned to try testing on 32-bit KVM too eventually, but we never did,
> >>> and now we'll never need to.
> >>>
> >>> Thanks,
> >>> drew
> >>>
> >>>
> >>
> > 
> > 
> 

Re: [PATCH 1/9] tests/qtest/arm-cpu-features: Remove Cortex-A15 check

[John Snow]

On 2/6/21 5:40 AM, Andrew Jones wrote:
>> BTW is there some easy way to dump QMP traffic on stdio?
> You can use scripts/qmp/qmp-shell to manually test stuff.
> 
> Thanks,
> drew
> 

If you enable debug logging in python too, it'll print to STDERR. This 
may or may not be useful depending on how the library is getting used 
and by whom.

(For iotests, you used to be able to engage this mode by passing -d. I 
don't know if the new test runner has changed this behavior. I'm sure 
avocado has something similar, somewhere, too.)

Otherwise, the runes are something like:

```
import logging

logging.basicConfig(level=logging.DEBUG)
```

but this only works once per process; the iotests entry point already 
has a call like this. To override it:

```
import logging

logging.getLogger().setLevel(logging.DEBUG)
```

There are other, more complex incantations; you can read 
https://docs.python.org/3/howto/logging.html if you'd like; the logger 
we care about is "QMP" (for QMP instances created without a 'nickname') 
and "QMP.{nickname}" for ones that were. So you can do this:

logging.getLogger("QMP").setLevel(logging.DEBUG)

but you might need to adjust other settings to get it to appear on 
STDERR (I don't remember), like setting handler propagation settings and 
so on.

--js

Re: [PATCH 0/9] hw/arm/virt: Improve CPU help and fix testing under KVM

[Claudio Fontana]

On 2/5/21 3:43 PM, Philippe Mathieu-Daudé wrote:
> Yet again bugfixes and cleanup patches noticed while
> rebasing my "Support disabling TCG on ARM (part 2)" series.
> 
> Sending them independently as they aren't directly dependent
> of it so don't have to be delayed by other unanswered questions.
> 
> Please review,
> 
> Phil.
> 
> Philippe Mathieu-Daudé (9):
>   tests/qtest/arm-cpu-features: Remove Cortex-A15 check
>   tests/qtest: Restrict xlnx-can-test to TCG builds
>   tests/qtest/boot-serial-test: Test Virt machine with 'max'
>   tests/qtest/cdrom-test: Only allow the Virt machine under KVM
>   hw/arm/virt: Improve CPU name in help message
>   hw/arm/virt: Display list of valid CPUs for the Virt machine
>   hw/arm/virt: Do not include 64-bit CPUs in 32-bit build
>   hw/arm/virt: Restrict 32-bit CPUs to TCG
>   tests/qtest/arm-cpu-features: Restrict TCG-only tests
> 
>  hw/arm/virt.c                  | 20 +++++++++++++++++-
>  tests/qtest/arm-cpu-features.c | 37 ++++++++++++++++++++++++++--------
>  tests/qtest/boot-serial-test.c |  2 +-
>  tests/qtest/cdrom-test.c       |  5 ++++-
>  tests/qtest/meson.build        |  2 +-
>  5 files changed, 54 insertions(+), 12 deletions(-)
> 

Hi Philippe, Markus,

I encountered an issue where device-introspect-test.c gets me a qemu-system-aarch64 launched as:

./qemu-system-aarch64 -qtest ... -display none -nodefaults -machine none -accel qtest

and results in an attempt to create a device "ast2400-a1",
which tries to create a arm926-arm-cpu, which fails because it is a "TCG" cpu, that is not built in my kvm-only build.

Any ideas why this happens?

Thanks,

Claudio

Re: [PATCH 8/9] hw/arm/virt: Restrict 32-bit CPUs to TCG

[Claudio Fontana]

Hi Peter,

what do you think of the following patch? We messaged yesterday about cortex-a15 being the default cpu for virt,

this patch would need also changing the default CPU for virt under KVM I would think.

Or, we could change the virt default cpu to "max"?

Thanks,

Claudio


On 2/5/21 4:19 PM, Andrew Jones wrote:
> On Fri, Feb 05, 2021 at 03:43:44PM +0100, Philippe Mathieu-Daudé wrote:
>> Support for ARMv7 has been dropped in commit 82bf7ae84ce
>> ("target/arm: Remove KVM support for 32-bit Arm hosts").
>> Restrict the 32-bit CPUs to --enable-tcg builds.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  hw/arm/virt.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index f5e4a6ec914..ab6300650f9 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -197,8 +197,10 @@ static const int a15irqmap[] = {
>>  };
>>  
>>  static const char *valid_cpus[] = {
>> +#ifdef CONFIG_TCG
>>      ARM_CPU_TYPE_NAME("cortex-a7"),
>>      ARM_CPU_TYPE_NAME("cortex-a15"),
>> +#endif /* CONFIG_TCG */
>>  #ifdef TARGET_AARCH64
>>      ARM_CPU_TYPE_NAME("cortex-a53"),
>>      ARM_CPU_TYPE_NAME("cortex-a57"),
>> -- 
>> 2.26.2
>>
> 
> So this filters the cpus out of KVM only builds, which seems
> reasonable to do. Of course, if the build is for both KVM and
> TCG, then the cpus won't be filtered out and we'll have to rely
> on the runtime checks to error out if one where to try a 32-bit
> cpu with KVM. But that's fine too, so
> 
> Reviewed-by: Andrew Jones <drjones@redhat.com>
> 
> Thanks,
> drew
> 
> 

Re: [PATCH 8/9] hw/arm/virt: Restrict 32-bit CPUs to TCG

[Peter Maydell]

On Fri, 5 Feb 2021 at 14:44, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Support for ARMv7 has been dropped in commit 82bf7ae84ce
> ("target/arm: Remove KVM support for 32-bit Arm hosts").
> Restrict the 32-bit CPUs to --enable-tcg builds.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/arm/virt.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index f5e4a6ec914..ab6300650f9 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -197,8 +197,10 @@ static const int a15irqmap[] = {
>  };
>
>  static const char *valid_cpus[] = {
> +#ifdef CONFIG_TCG
>      ARM_CPU_TYPE_NAME("cortex-a7"),
>      ARM_CPU_TYPE_NAME("cortex-a15"),
> +#endif /* CONFIG_TCG */
>  #ifdef TARGET_AARCH64
>      ARM_CPU_TYPE_NAME("cortex-a53"),
>      ARM_CPU_TYPE_NAME("cortex-a57"),

How painful would it be to just have it check whether the
CPU type is present in the executable, rather than hard-coding an ifdef ?

I think that if you try to run the virt board with command line
arguments that (implicitly or explicitly) mean you've asked for
a CPU which isn't present in the QEMU executable, it should give
an error rather than silently selecting something else.

thanks
-- PMM