Re: [PATCH v8 08/11] tests/qtest: Fix tests when no KVM or TCG are present

[Fabiano Rosas <farosas@suse.de>]

"Michael S. Tsirkin" <mst@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 <farosas@suse.de>
>> Reviewed-by: Juan Quintela <quintela@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-philmd@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