Re: [PATCH v8 08/11] tests/qtest: Fix tests when no KVM or TCG are present
On Fri, Mar 10, 2023 at 10:23:33AM -0300, Fabiano Rosas wrote: > > How about a special make check target that will just test > > xen things? > > > > Probably overkill for this particular issue. I don't see any > Xen-specific tests yet. It would run almost the same set of tests. Well, a subset for now. -- MST
Re: [PATCH v8 08/11] tests/qtest: Fix tests when no KVM or TCG are present
Thomas Huth writes:
> On 10/03/2023 16.37, Fabiano Rosas wrote:
>> Thomas Huth writes:
>>
>>> On 10/03/2023 14.06, Fabiano Rosas wrote:
"Michael S. Tsirkin" writes:
> On Thu, Mar 09, 2023 at 05:14:31PM -0300, Fabiano Rosas wrote:
>> It is possible to have a build with both TCG and KVM disabled due to
>> Xen requiring the i386 and x86_64 binaries to be present in an aarch64
>> host.
>>
>> If we build with --disable-tcg on the aarch64 host, we will end-up
>> with a QEMU binary (x86) that does not support TCG nor KVM.
>>
>> Fix tests that crash or hang in the above scenario. Do not include any
>> test cases if TCG and KVM are missing.
>>
>> Make sure that calls to qtest_has_accel are placed after g_test_init
>> in similar fashion to commit ae4b01b349 ("tests: Ensure TAP version is
>> printed before other messages") to avoid TAP parsing errors.
>>
>> Signed-off-by: Fabiano Rosas
>> Reviewed-by: Juan Quintela
>
> I don't like it that we are hard-coding the list of accelerators
> like this. Make a wrapper?
>
Are you thinking of some sort of "has_any_accel" wrapper?
>>>
>>> I think in the long run, we want something like what I described here:
>>>
>>> https://lore.kernel.org/qemu-devel/ee0cad00-a6f3-f0c1-adf0-ba3232935...@redhat.com/
>>>
>>
>> Wont't that function be too generic? The choice of accelerator is quite
>> specific to each individual test, some might not work with TCG, some
>> might not work with HVF and so on. There is no link between build-time
>> configuration and runtime test execution after all. We could always have
>> a build without an accelerator and then try to run a test that uses that
>> accelerator. And also have an accelerator present that the test does not
>> support at all.
>>
>>
>> For this particularly bizarre case of not having TCG nor KVM in the
>> build I'm inclined to go with Michael's suggestion of checking it at
>> build time and skipping all the hassle. This is what I'm preparing:
>>
>> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
>> index 29a4efb4c2..e698cdcb60 100644
>> --- a/tests/qtest/meson.build
>> +++ b/tests/qtest/meson.build
>> @@ -27,6 +27,12 @@ if config_host.has_key('CONFIG_MODULES')
>> qtests_generic += [ 'modules-test' ]
>> endif
>>
>> +# For x86_64, i386 and aarch64 it is possible to have only Xen as an
>> +# accelerator. Some tests require either TCG or KVM, so make sure they
>> +# are present before building those tests.
>> +tcg_or_kvm_available = (config_all.has_key('CONFIG_TCG') or
>> +config_all.has_key('CONFIG_KVM'))
Ouch, this doesn't work because one of the binaries could still have KVM
support and we build the tests only once.
On an aarch64 host:
---disable-tcg --enable-kvm --enable-xen =>
qemu-system-aarch64 w/ Xen, KVM, no TCG
qemu-system-x86_64 w/ Xen, no KVM, no TCG
Re: [PATCH v8 08/11] tests/qtest: Fix tests when no KVM or TCG are present
On 10/03/2023 16.37, Fabiano Rosas wrote:
Thomas Huth writes:
On 10/03/2023 14.06, Fabiano Rosas wrote:
"Michael S. Tsirkin" writes:
On Thu, Mar 09, 2023 at 05:14:31PM -0300, Fabiano Rosas wrote:
It is possible to have a build with both TCG and KVM disabled due to
Xen requiring the i386 and x86_64 binaries to be present in an aarch64
host.
If we build with --disable-tcg on the aarch64 host, we will end-up
with a QEMU binary (x86) that does not support TCG nor KVM.
Fix tests that crash or hang in the above scenario. Do not include any
test cases if TCG and KVM are missing.
Make sure that calls to qtest_has_accel are placed after g_test_init
in similar fashion to commit ae4b01b349 ("tests: Ensure TAP version is
printed before other messages") to avoid TAP parsing errors.
Signed-off-by: Fabiano Rosas
Reviewed-by: Juan Quintela
I don't like it that we are hard-coding the list of accelerators
like this. Make a wrapper?
Are you thinking of some sort of "has_any_accel" wrapper?
I think in the long run, we want something like what I described here:
https://lore.kernel.org/qemu-devel/ee0cad00-a6f3-f0c1-adf0-ba3232935...@redhat.com/
Wont't that function be too generic? The choice of accelerator is quite
specific to each individual test, some might not work with TCG, some
might not work with HVF and so on. There is no link between build-time
configuration and runtime test execution after all. We could always have
a build without an accelerator and then try to run a test that uses that
accelerator. And also have an accelerator present that the test does not
support at all.
For this particularly bizarre case of not having TCG nor KVM in the
build I'm inclined to go with Michael's suggestion of checking it at
build time and skipping all the hassle. This is what I'm preparing:
diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index 29a4efb4c2..e698cdcb60 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -27,6 +27,12 @@ if config_host.has_key('CONFIG_MODULES')
qtests_generic += [ 'modules-test' ]
endif
+# For x86_64, i386 and aarch64 it is possible to have only Xen as an
+# accelerator. Some tests require either TCG or KVM, so make sure they
+# are present before building those tests.
+tcg_or_kvm_available = (config_all.has_key('CONFIG_TCG') or
+config_all.has_key('CONFIG_KVM'))
+
qtests_pci = \
(config_all_devices.has_key('CONFIG_VGA') ? ['display-vga-test'] : []) +
\
(config_all_devices.has_key('CONFIG_IVSHMEM_DEVICE') ? ['ivshmem-test'] :
[])
@@ -40,11 +46,12 @@ qtests_filter = \
(config_host.has_key('CONFIG_POSIX') ? ['test-filter-redirector'] : [])
qtests_i386 = \
- (slirp.found() ? ['pxe-test'] : []) + \
+ (slirp.found() and tcg_or_kvm_available ? ['pxe-test'] : []) +
\
---
What do you think?
It's likely ok as additional check for Xen-only builds, but we certainly
cannot remove the runtime checks for KVM (it's perfectly fine to have KVM
included during compilation - but unavailable during runtime (e.g. if the
kernel module is not loaded).
Thomas
Re: [PATCH v8 08/11] tests/qtest: Fix tests when no KVM or TCG are present
Thomas Huth writes:
> On 10/03/2023 14.06, Fabiano Rosas wrote:
>> "Michael S. Tsirkin" writes:
>>
>>> On Thu, Mar 09, 2023 at 05:14:31PM -0300, Fabiano Rosas wrote:
It is possible to have a build with both TCG and KVM disabled due to
Xen requiring the i386 and x86_64 binaries to be present in an aarch64
host.
If we build with --disable-tcg on the aarch64 host, we will end-up
with a QEMU binary (x86) that does not support TCG nor KVM.
Fix tests that crash or hang in the above scenario. Do not include any
test cases if TCG and KVM are missing.
Make sure that calls to qtest_has_accel are placed after g_test_init
in similar fashion to commit ae4b01b349 ("tests: Ensure TAP version is
printed before other messages") to avoid TAP parsing errors.
Signed-off-by: Fabiano Rosas
Reviewed-by: Juan Quintela
>>>
>>> I don't like it that we are hard-coding the list of accelerators
>>> like this. Make a wrapper?
>>>
>>
>> Are you thinking of some sort of "has_any_accel" wrapper?
>
> I think in the long run, we want something like what I described here:
>
> https://lore.kernel.org/qemu-devel/ee0cad00-a6f3-f0c1-adf0-ba3232935...@redhat.com/
>
Wont't that function be too generic? The choice of accelerator is quite
specific to each individual test, some might not work with TCG, some
might not work with HVF and so on. There is no link between build-time
configuration and runtime test execution after all. We could always have
a build without an accelerator and then try to run a test that uses that
accelerator. And also have an accelerator present that the test does not
support at all.
For this particularly bizarre case of not having TCG nor KVM in the
build I'm inclined to go with Michael's suggestion of checking it at
build time and skipping all the hassle. This is what I'm preparing:
diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index 29a4efb4c2..e698cdcb60 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -27,6 +27,12 @@ if config_host.has_key('CONFIG_MODULES')
qtests_generic += [ 'modules-test' ]
endif
+# For x86_64, i386 and aarch64 it is possible to have only Xen as an
+# accelerator. Some tests require either TCG or KVM, so make sure they
+# are present before building those tests.
+tcg_or_kvm_available = (config_all.has_key('CONFIG_TCG') or
+config_all.has_key('CONFIG_KVM'))
+
qtests_pci = \
(config_all_devices.has_key('CONFIG_VGA') ? ['display-vga-test'] : []) +
\
(config_all_devices.has_key('CONFIG_IVSHMEM_DEVICE') ? ['ivshmem-test'] : [])
@@ -40,11 +46,12 @@ qtests_filter = \
(config_host.has_key('CONFIG_POSIX') ? ['test-filter-redirector'] : [])
qtests_i386 = \
- (slirp.found() ? ['pxe-test'] : []) + \
+ (slirp.found() and tcg_or_kvm_available ? ['pxe-test'] : []) +
\
---
What do you think?
Re: [PATCH v8 08/11] tests/qtest: Fix tests when no KVM or TCG are present
On 10/03/2023 14.06, Fabiano Rosas wrote:
"Michael S. Tsirkin" writes:
On Thu, Mar 09, 2023 at 05:14:31PM -0300, Fabiano Rosas wrote:
It is possible to have a build with both TCG and KVM disabled due to
Xen requiring the i386 and x86_64 binaries to be present in an aarch64
host.
If we build with --disable-tcg on the aarch64 host, we will end-up
with a QEMU binary (x86) that does not support TCG nor KVM.
Fix tests that crash or hang in the above scenario. Do not include any
test cases if TCG and KVM are missing.
Make sure that calls to qtest_has_accel are placed after g_test_init
in similar fashion to commit ae4b01b349 ("tests: Ensure TAP version is
printed before other messages") to avoid TAP parsing errors.
Signed-off-by: Fabiano Rosas
Reviewed-by: Juan Quintela
I don't like it that we are hard-coding the list of accelerators
like this. Make a wrapper?
Are you thinking of some sort of "has_any_accel" wrapper?
I think in the long run, we want something like what I described here:
https://lore.kernel.org/qemu-devel/ee0cad00-a6f3-f0c1-adf0-ba3232935...@redhat.com/
@@ -2120,6 +2119,13 @@ int main(int argc, char *argv[])
g_test_init(, , NULL);
+has_kvm = qtest_has_accel("kvm");
+has_tcg = qtest_has_accel("tcg");
+
why are you moving these? init at declaration time is
generally cleaner.
Thomas had asked me to put calls to qtest_has_accel after g_test_init. I
just brought the existing one along for consistency. From the commit
message:
"Make sure that calls to qtest_has_accel are placed after g_test_init
in similar fashion to commit ae4b01b349 ("tests: Ensure TAP version is
printed before other messages") to avoid TAP parsing errors."
Right, otherwise this might cause problems with the latest version of glib
(in Fedora, I think).
Thomas
Re: [PATCH v8 08/11] tests/qtest: Fix tests when no KVM or TCG are present
"Michael S. Tsirkin" writes:
> On Thu, Mar 09, 2023 at 05:14:31PM -0300, Fabiano Rosas wrote:
>> It is possible to have a build with both TCG and KVM disabled due to
>> Xen requiring the i386 and x86_64 binaries to be present in an aarch64
>> host.
>>
>> If we build with --disable-tcg on the aarch64 host, we will end-up
>> with a QEMU binary (x86) that does not support TCG nor KVM.
>>
>> Fix tests that crash or hang in the above scenario. Do not include any
>> test cases if TCG and KVM are missing.
>>
>> Make sure that calls to qtest_has_accel are placed after g_test_init
>> in similar fashion to commit ae4b01b349 ("tests: Ensure TAP version is
>> printed before other messages") to avoid TAP parsing errors.
>>
>> Signed-off-by: Fabiano Rosas
>> Reviewed-by: Juan Quintela
>
> I don't like it that it's a pass not a skip.
>
Noted. I'm always questioning myself whether to skip or pass.
> Also, if we are not testing acpi should we not
> skip building acpi?
>
Good point. I'll try to do that for the next version.
> Also, a misconfigured qemu would previously be caught,
> now it will seem to pass tests.
Well, we can only call it misconfigured if we have a specific setup in
mind. In the general sense there is never a misconfigured qemu unless
there's a bug in the configuration path (configure, Kconfig, meson,
etc). Then these tests would have nothing to do with it.
So in this particular case, the "bug" perhaps is that we're still trying
to build and run the tests even when the accelerator(s) they require are
not present. I think your suggestion above of not building the test
covers that.
> How about a special make check target that will just test
> xen things?
>
Probably overkill for this particular issue. I don't see any
Xen-specific tests yet. It would run almost the same set of tests.
Re: [PATCH v8 08/11] tests/qtest: Fix tests when no KVM or TCG are present
"Michael S. Tsirkin" writes:
> On Thu, Mar 09, 2023 at 05:14:31PM -0300, Fabiano Rosas wrote:
>> It is possible to have a build with both TCG and KVM disabled due to
>> Xen requiring the i386 and x86_64 binaries to be present in an aarch64
>> host.
>>
>> If we build with --disable-tcg on the aarch64 host, we will end-up
>> with a QEMU binary (x86) that does not support TCG nor KVM.
>>
>> Fix tests that crash or hang in the above scenario. Do not include any
>> test cases if TCG and KVM are missing.
>>
>> Make sure that calls to qtest_has_accel are placed after g_test_init
>> in similar fashion to commit ae4b01b349 ("tests: Ensure TAP version is
>> printed before other messages") to avoid TAP parsing errors.
>>
>> Signed-off-by: Fabiano Rosas
>> Reviewed-by: Juan Quintela
>
> I don't like it that we are hard-coding the list of accelerators
> like this. Make a wrapper?
>
Are you thinking of some sort of "has_any_accel" wrapper?
Some of the code uses has_tcg/kvm for other purposes, so there would be
slight duplication. And the issue really is !tcg && !kvm, i.e. other
accelerators are not taken into account.
>> ---
>> This currently affects Arm, but will also affect x86 after the xenpvh
>> series gets merged. This patch fixes both scenarios.
>> ---
>> tests/qtest/bios-tables-test.c | 10 --
>> tests/qtest/boot-serial-test.c | 10 ++
>> tests/qtest/migration-test.c | 9 -
>> tests/qtest/pxe-test.c | 7 ++-
>> tests/qtest/vmgenid-test.c | 8 ++--
>> 5 files changed, 38 insertions(+), 6 deletions(-)
>>
>> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
>> index d29a4e47af..5cbad2f29f 100644
>> --- a/tests/qtest/bios-tables-test.c
>> +++ b/tests/qtest/bios-tables-test.c
>> @@ -2109,8 +2109,7 @@ static void test_acpi_virt_oem_fields(void)
>> int main(int argc, char *argv[])
>> {
>> const char *arch = qtest_get_arch();
>> -const bool has_kvm = qtest_has_accel("kvm");
>> -const bool has_tcg = qtest_has_accel("tcg");
>> +bool has_kvm, has_tcg;
>> char *v_env = getenv("V");
>> int ret;
>>
>> @@ -2120,6 +2119,13 @@ int main(int argc, char *argv[])
>>
>> g_test_init(, , NULL);
>>
>> +has_kvm = qtest_has_accel("kvm");
>> +has_tcg = qtest_has_accel("tcg");
>> +
>
> why are you moving these? init at declaration time is
> generally cleaner.
>
Thomas had asked me to put calls to qtest_has_accel after g_test_init. I
just brought the existing one along for consistency. From the commit
message:
"Make sure that calls to qtest_has_accel are placed after g_test_init
in similar fashion to commit ae4b01b349 ("tests: Ensure TAP version is
printed before other messages") to avoid TAP parsing errors."
>> +if (!has_tcg && !has_kvm) {
>> +return 0;
>> +}
>> +
>> if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
>> ret = boot_sector_init(disk);
>> if (ret) {
>> diff --git a/tests/qtest/boot-serial-test.c b/tests/qtest/boot-serial-test.c
>> index 3aef3a97a9..406b4421cc 100644
>> --- a/tests/qtest/boot-serial-test.c
>> +++ b/tests/qtest/boot-serial-test.c
>> @@ -17,6 +17,9 @@
>> #include "libqtest.h"
>> #include "libqos/libqos-spapr.h"
>>
>> +static bool has_tcg;
>> +static bool has_kvm;
>> +
>> static const uint8_t bios_avr[] = {
>> 0x88, 0xe0, /* ldi r24, 0x08 */
>> 0x80, 0x93, 0xc1, 0x00, /* sts 0x00C1, r24 ; Enable tx */
>> @@ -287,6 +290,13 @@ int main(int argc, char *argv[])
>>
>> g_test_init(, , NULL);
>>
>> +has_tcg = qtest_has_accel("tcg");
>> +has_kvm = qtest_has_accel("kvm");
>> +
>
> and here why do we need them global?
>
I was trying to make things easier when merging this other series that
puts the variables there as well:
https://lore.kernel.org/r/20230119145838.41835-5-phi...@linaro.org
Second time I get asked this so I'll change it for the next version =)
>> +if (!has_tcg && !has_kvm) {
>> +return 0;
>> +}
>> +
>> for (i = 0; tests[i].arch != NULL; i++) {
>> if (g_str_equal(arch, tests[i].arch) &&
>> qtest_has_machine(tests[i].machine)) {
>> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
>> index d4ab3934ed..7eedee7b2d 100644
>> --- a/tests/qtest/migration-test.c
>> +++ b/tests/qtest/migration-test.c
>> @@ -2459,7 +2459,7 @@ static bool kvm_dirty_ring_supported(void)
>>
>> int main(int argc, char **argv)
>> {
>> -const bool has_kvm = qtest_has_accel("kvm");
>> +bool has_kvm, has_tcg;
>> const bool has_uffd = ufd_version_check();
>> const char *arch = qtest_get_arch();
>> g_autoptr(GError) err = NULL;
>> @@ -2467,6 +2467,13 @@ int main(int argc, char **argv)
>>
>> g_test_init(, , NULL);
>>
>> +has_kvm = qtest_has_accel("kvm");
>> +has_tcg = qtest_has_accel("tcg");
>> +
>
> same. why the move?
>
g_test_init
>> +if (!has_tcg && !has_kvm) {
>> +return 0;
>> +
Re: [PATCH v8 08/11] tests/qtest: Fix tests when no KVM or TCG are present
On Thu, Mar 09, 2023 at 05:14:31PM -0300, Fabiano Rosas wrote:
> It is possible to have a build with both TCG and KVM disabled due to
> Xen requiring the i386 and x86_64 binaries to be present in an aarch64
> host.
>
> If we build with --disable-tcg on the aarch64 host, we will end-up
> with a QEMU binary (x86) that does not support TCG nor KVM.
>
> Fix tests that crash or hang in the above scenario. Do not include any
> test cases if TCG and KVM are missing.
>
> Make sure that calls to qtest_has_accel are placed after g_test_init
> in similar fashion to commit ae4b01b349 ("tests: Ensure TAP version is
> printed before other messages") to avoid TAP parsing errors.
>
> Signed-off-by: Fabiano Rosas
> Reviewed-by: Juan Quintela
I don't like it that it's a pass not a skip.
Also, if we are not testing acpi should we not
skip building acpi?
Also, a misconfigured qemu would previously be caught,
now it will seem to pass tests.
How about a special make check target that will just test
xen things?
> ---
> This currently affects Arm, but will also affect x86 after the xenpvh
> series gets merged. This patch fixes both scenarios.
> ---
> tests/qtest/bios-tables-test.c | 10 --
> tests/qtest/boot-serial-test.c | 10 ++
> tests/qtest/migration-test.c | 9 -
> tests/qtest/pxe-test.c | 7 ++-
> tests/qtest/vmgenid-test.c | 8 ++--
> 5 files changed, 38 insertions(+), 6 deletions(-)
>
> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> index d29a4e47af..5cbad2f29f 100644
> --- a/tests/qtest/bios-tables-test.c
> +++ b/tests/qtest/bios-tables-test.c
> @@ -2109,8 +2109,7 @@ static void test_acpi_virt_oem_fields(void)
> int main(int argc, char *argv[])
> {
> const char *arch = qtest_get_arch();
> -const bool has_kvm = qtest_has_accel("kvm");
> -const bool has_tcg = qtest_has_accel("tcg");
> +bool has_kvm, has_tcg;
> char *v_env = getenv("V");
> int ret;
>
> @@ -2120,6 +2119,13 @@ int main(int argc, char *argv[])
>
> g_test_init(, , NULL);
>
> +has_kvm = qtest_has_accel("kvm");
> +has_tcg = qtest_has_accel("tcg");
> +
> +if (!has_tcg && !has_kvm) {
> +return 0;
> +}
> +
> if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
> ret = boot_sector_init(disk);
> if (ret) {
> diff --git a/tests/qtest/boot-serial-test.c b/tests/qtest/boot-serial-test.c
> index 3aef3a97a9..406b4421cc 100644
> --- a/tests/qtest/boot-serial-test.c
> +++ b/tests/qtest/boot-serial-test.c
> @@ -17,6 +17,9 @@
> #include "libqtest.h"
> #include "libqos/libqos-spapr.h"
>
> +static bool has_tcg;
> +static bool has_kvm;
> +
> static const uint8_t bios_avr[] = {
> 0x88, 0xe0, /* ldi r24, 0x08 */
> 0x80, 0x93, 0xc1, 0x00, /* sts 0x00C1, r24 ; Enable tx */
> @@ -287,6 +290,13 @@ int main(int argc, char *argv[])
>
> g_test_init(, , NULL);
>
> +has_tcg = qtest_has_accel("tcg");
> +has_kvm = qtest_has_accel("kvm");
> +
> +if (!has_tcg && !has_kvm) {
> +return 0;
> +}
> +
> for (i = 0; tests[i].arch != NULL; i++) {
> if (g_str_equal(arch, tests[i].arch) &&
> qtest_has_machine(tests[i].machine)) {
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index d4ab3934ed..7eedee7b2d 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -2459,7 +2459,7 @@ static bool kvm_dirty_ring_supported(void)
>
> int main(int argc, char **argv)
> {
> -const bool has_kvm = qtest_has_accel("kvm");
> +bool has_kvm, has_tcg;
> const bool has_uffd = ufd_version_check();
> const char *arch = qtest_get_arch();
> g_autoptr(GError) err = NULL;
> @@ -2467,6 +2467,13 @@ int main(int argc, char **argv)
>
> g_test_init(, , NULL);
>
> +has_kvm = qtest_has_accel("kvm");
> +has_tcg = qtest_has_accel("tcg");
> +
> +if (!has_tcg && !has_kvm) {
> +return 0;
> +}
> +
> /*
> * On ppc64, the test only works with kvm-hv, but not with kvm-pr and TCG
> * is touchy due to race conditions on dirty bits (especially on PPC for
> diff --git a/tests/qtest/pxe-test.c b/tests/qtest/pxe-test.c
> index 62b6eef464..935b661dac 100644
> --- a/tests/qtest/pxe-test.c
> +++ b/tests/qtest/pxe-test.c
> @@ -131,11 +131,16 @@ int main(int argc, char *argv[])
> int ret;
> const char *arch = qtest_get_arch();
>
> +g_test_init(, , NULL);
> +
> +if (!qtest_has_accel("tcg") && !qtest_has_accel("kvm")) {
> +return 0;
> +}
> +
> ret = boot_sector_init(disk);
> if(ret)
> return ret;
>
> -g_test_init(, , NULL);
>
> if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
> test_batch(x86_tests, false);
> diff --git a/tests/qtest/vmgenid-test.c b/tests/qtest/vmgenid-test.c
> index efba76e716..9eb6371ae8 100644
> --- a/tests/qtest/vmgenid-test.c
> +++
Re: [PATCH v8 08/11] tests/qtest: Fix tests when no KVM or TCG are present
On Thu, Mar 09, 2023 at 05:14:31PM -0300, Fabiano Rosas wrote:
> It is possible to have a build with both TCG and KVM disabled due to
> Xen requiring the i386 and x86_64 binaries to be present in an aarch64
> host.
>
> If we build with --disable-tcg on the aarch64 host, we will end-up
> with a QEMU binary (x86) that does not support TCG nor KVM.
>
> Fix tests that crash or hang in the above scenario. Do not include any
> test cases if TCG and KVM are missing.
>
> Make sure that calls to qtest_has_accel are placed after g_test_init
> in similar fashion to commit ae4b01b349 ("tests: Ensure TAP version is
> printed before other messages") to avoid TAP parsing errors.
>
> Signed-off-by: Fabiano Rosas
> Reviewed-by: Juan Quintela
I don't like it that we are hard-coding the list of accelerators
like this. Make a wrapper?
> ---
> This currently affects Arm, but will also affect x86 after the xenpvh
> series gets merged. This patch fixes both scenarios.
> ---
> tests/qtest/bios-tables-test.c | 10 --
> tests/qtest/boot-serial-test.c | 10 ++
> tests/qtest/migration-test.c | 9 -
> tests/qtest/pxe-test.c | 7 ++-
> tests/qtest/vmgenid-test.c | 8 ++--
> 5 files changed, 38 insertions(+), 6 deletions(-)
>
> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> index d29a4e47af..5cbad2f29f 100644
> --- a/tests/qtest/bios-tables-test.c
> +++ b/tests/qtest/bios-tables-test.c
> @@ -2109,8 +2109,7 @@ static void test_acpi_virt_oem_fields(void)
> int main(int argc, char *argv[])
> {
> const char *arch = qtest_get_arch();
> -const bool has_kvm = qtest_has_accel("kvm");
> -const bool has_tcg = qtest_has_accel("tcg");
> +bool has_kvm, has_tcg;
> char *v_env = getenv("V");
> int ret;
>
> @@ -2120,6 +2119,13 @@ int main(int argc, char *argv[])
>
> g_test_init(, , NULL);
>
> +has_kvm = qtest_has_accel("kvm");
> +has_tcg = qtest_has_accel("tcg");
> +
why are you moving these? init at declaration time is
generally cleaner.
> +if (!has_tcg && !has_kvm) {
> +return 0;
> +}
> +
> if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
> ret = boot_sector_init(disk);
> if (ret) {
> diff --git a/tests/qtest/boot-serial-test.c b/tests/qtest/boot-serial-test.c
> index 3aef3a97a9..406b4421cc 100644
> --- a/tests/qtest/boot-serial-test.c
> +++ b/tests/qtest/boot-serial-test.c
> @@ -17,6 +17,9 @@
> #include "libqtest.h"
> #include "libqos/libqos-spapr.h"
>
> +static bool has_tcg;
> +static bool has_kvm;
> +
> static const uint8_t bios_avr[] = {
> 0x88, 0xe0, /* ldi r24, 0x08 */
> 0x80, 0x93, 0xc1, 0x00, /* sts 0x00C1, r24 ; Enable tx */
> @@ -287,6 +290,13 @@ int main(int argc, char *argv[])
>
> g_test_init(, , NULL);
>
> +has_tcg = qtest_has_accel("tcg");
> +has_kvm = qtest_has_accel("kvm");
> +
and here why do we need them global?
> +if (!has_tcg && !has_kvm) {
> +return 0;
> +}
> +
> for (i = 0; tests[i].arch != NULL; i++) {
> if (g_str_equal(arch, tests[i].arch) &&
> qtest_has_machine(tests[i].machine)) {
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index d4ab3934ed..7eedee7b2d 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -2459,7 +2459,7 @@ static bool kvm_dirty_ring_supported(void)
>
> int main(int argc, char **argv)
> {
> -const bool has_kvm = qtest_has_accel("kvm");
> +bool has_kvm, has_tcg;
> const bool has_uffd = ufd_version_check();
> const char *arch = qtest_get_arch();
> g_autoptr(GError) err = NULL;
> @@ -2467,6 +2467,13 @@ int main(int argc, char **argv)
>
> g_test_init(, , NULL);
>
> +has_kvm = qtest_has_accel("kvm");
> +has_tcg = qtest_has_accel("tcg");
> +
same. why the move?
> +if (!has_tcg && !has_kvm) {
> +return 0;
> +}
> +
> /*
> * On ppc64, the test only works with kvm-hv, but not with kvm-pr and TCG
> * is touchy due to race conditions on dirty bits (especially on PPC for
> diff --git a/tests/qtest/pxe-test.c b/tests/qtest/pxe-test.c
> index 62b6eef464..935b661dac 100644
> --- a/tests/qtest/pxe-test.c
> +++ b/tests/qtest/pxe-test.c
> @@ -131,11 +131,16 @@ int main(int argc, char *argv[])
> int ret;
> const char *arch = qtest_get_arch();
>
> +g_test_init(, , NULL);
> +
> +if (!qtest_has_accel("tcg") && !qtest_has_accel("kvm")) {
> +return 0;
> +}
> +
> ret = boot_sector_init(disk);
> if(ret)
> return ret;
>
> -g_test_init(, , NULL);
>
> if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
> test_batch(x86_tests, false);
> diff --git a/tests/qtest/vmgenid-test.c b/tests/qtest/vmgenid-test.c
> index efba76e716..9eb6371ae8 100644
> --- a/tests/qtest/vmgenid-test.c
> +++ b/tests/qtest/vmgenid-test.c
> @@ -165,13 +165,17
[PATCH v8 08/11] tests/qtest: Fix tests when no KVM or TCG are present
It is possible to have a build with both TCG and KVM disabled due to
Xen requiring the i386 and x86_64 binaries to be present in an aarch64
host.
If we build with --disable-tcg on the aarch64 host, we will end-up
with a QEMU binary (x86) that does not support TCG nor KVM.
Fix tests that crash or hang in the above scenario. Do not include any
test cases if TCG and KVM are missing.
Make sure that calls to qtest_has_accel are placed after g_test_init
in similar fashion to commit ae4b01b349 ("tests: Ensure TAP version is
printed before other messages") to avoid TAP parsing errors.
Signed-off-by: Fabiano Rosas
Reviewed-by: Juan Quintela
---
This currently affects Arm, but will also affect x86 after the xenpvh
series gets merged. This patch fixes both scenarios.
---
tests/qtest/bios-tables-test.c | 10 --
tests/qtest/boot-serial-test.c | 10 ++
tests/qtest/migration-test.c | 9 -
tests/qtest/pxe-test.c | 7 ++-
tests/qtest/vmgenid-test.c | 8 ++--
5 files changed, 38 insertions(+), 6 deletions(-)
diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
index d29a4e47af..5cbad2f29f 100644
--- a/tests/qtest/bios-tables-test.c
+++ b/tests/qtest/bios-tables-test.c
@@ -2109,8 +2109,7 @@ static void test_acpi_virt_oem_fields(void)
int main(int argc, char *argv[])
{
const char *arch = qtest_get_arch();
-const bool has_kvm = qtest_has_accel("kvm");
-const bool has_tcg = qtest_has_accel("tcg");
+bool has_kvm, has_tcg;
char *v_env = getenv("V");
int ret;
@@ -2120,6 +2119,13 @@ int main(int argc, char *argv[])
g_test_init(, , NULL);
+has_kvm = qtest_has_accel("kvm");
+has_tcg = qtest_has_accel("tcg");
+
+if (!has_tcg && !has_kvm) {
+return 0;
+}
+
if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
ret = boot_sector_init(disk);
if (ret) {
diff --git a/tests/qtest/boot-serial-test.c b/tests/qtest/boot-serial-test.c
index 3aef3a97a9..406b4421cc 100644
--- a/tests/qtest/boot-serial-test.c
+++ b/tests/qtest/boot-serial-test.c
@@ -17,6 +17,9 @@
#include "libqtest.h"
#include "libqos/libqos-spapr.h"
+static bool has_tcg;
+static bool has_kvm;
+
static const uint8_t bios_avr[] = {
0x88, 0xe0, /* ldi r24, 0x08 */
0x80, 0x93, 0xc1, 0x00, /* sts 0x00C1, r24 ; Enable tx */
@@ -287,6 +290,13 @@ int main(int argc, char *argv[])
g_test_init(, , NULL);
+has_tcg = qtest_has_accel("tcg");
+has_kvm = qtest_has_accel("kvm");
+
+if (!has_tcg && !has_kvm) {
+return 0;
+}
+
for (i = 0; tests[i].arch != NULL; i++) {
if (g_str_equal(arch, tests[i].arch) &&
qtest_has_machine(tests[i].machine)) {
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index d4ab3934ed..7eedee7b2d 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -2459,7 +2459,7 @@ static bool kvm_dirty_ring_supported(void)
int main(int argc, char **argv)
{
-const bool has_kvm = qtest_has_accel("kvm");
+bool has_kvm, has_tcg;
const bool has_uffd = ufd_version_check();
const char *arch = qtest_get_arch();
g_autoptr(GError) err = NULL;
@@ -2467,6 +2467,13 @@ int main(int argc, char **argv)
g_test_init(, , NULL);
+has_kvm = qtest_has_accel("kvm");
+has_tcg = qtest_has_accel("tcg");
+
+if (!has_tcg && !has_kvm) {
+return 0;
+}
+
/*
* On ppc64, the test only works with kvm-hv, but not with kvm-pr and TCG
* is touchy due to race conditions on dirty bits (especially on PPC for
diff --git a/tests/qtest/pxe-test.c b/tests/qtest/pxe-test.c
index 62b6eef464..935b661dac 100644
--- a/tests/qtest/pxe-test.c
+++ b/tests/qtest/pxe-test.c
@@ -131,11 +131,16 @@ int main(int argc, char *argv[])
int ret;
const char *arch = qtest_get_arch();
+g_test_init(, , NULL);
+
+if (!qtest_has_accel("tcg") && !qtest_has_accel("kvm")) {
+return 0;
+}
+
ret = boot_sector_init(disk);
if(ret)
return ret;
-g_test_init(, , NULL);
if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
test_batch(x86_tests, false);
diff --git a/tests/qtest/vmgenid-test.c b/tests/qtest/vmgenid-test.c
index efba76e716..9eb6371ae8 100644
--- a/tests/qtest/vmgenid-test.c
+++ b/tests/qtest/vmgenid-test.c
@@ -165,13 +165,17 @@ int main(int argc, char **argv)
{
int ret;
+g_test_init(, , NULL);
+
+if (!qtest_has_accel("tcg") && !qtest_has_accel("kvm")) {
+return 0;
+}
+
ret = boot_sector_init(disk);
if (ret) {
return ret;
}
-g_test_init(, , NULL);
-
qtest_add_func("/vmgenid/vmgenid/set-guid",
vmgenid_set_guid_test);
qtest_add_func("/vmgenid/vmgenid/set-guid-auto",
--
2.35.3
