"Michael S. Tsirkin" <m...@redhat.com> 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 <faro...@suse.de>
>> Reviewed-by: Juan Quintela <quint...@redhat.com>
>
> 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(&argc, &argv, 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(&argc, &argv, 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(&argc, &argv, 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;
>> + }
>> +
>> /*
>> * 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(&argc, &argv, NULL);
>> +
>> + if (!qtest_has_accel("tcg") && !qtest_has_accel("kvm")) {
>> + return 0;
>> + }
>> +
>> ret = boot_sector_init(disk);
>> if(ret)
>> return ret;
>>
>> - g_test_init(&argc, &argv, 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(&argc, &argv, NULL);
>> +
>> + if (!qtest_has_accel("tcg") && !qtest_has_accel("kvm")) {
>> + return 0;
>> + }
>> +
>> ret = boot_sector_init(disk);
>> if (ret) {
>> return ret;
>> }
>>
>> - g_test_init(&argc, &argv, NULL);
>> -
>> qtest_add_func("/vmgenid/vmgenid/set-guid",
>> vmgenid_set_guid_test);
>> qtest_add_func("/vmgenid/vmgenid/set-guid-auto",
>> --
>> 2.35.3
