[Qemu-devel] [RFC PATCH 0/2] tests: avoid kvm accel when it is not accessible

[Qemu-devel] [RFC PATCH 0/2] tests: avoid kvm accel when it is not accessible

[Philippe Mathieu-Daudé]

This series reduce warnings when kvm is not accessible:

- kernel without kvm:

    # make check-qtest-x86_64
      GTESTER check-qtest-x86_64
    Could not access KVM kernel module: No such device
    qemu-system-x86_64: failed to initialize KVM: No such device
    qemu-system-x86_64: Back to tcg accelerator
    [... many of these ...]

- tests ran as user:

    $ make check-qtest-x86_64
      GTESTER check-qtest-x86_64
    Could not access KVM kernel module: Permission denied
    qemu-system-x86_64: failed to initialize KVM: Permission denied
    qemu-system-x86_64: Back to tcg accelerator
    [... many of these ...]

Once applied:

    $ make check-qtest-x86_64
      GTESTER check-qtest-x86_64
    kvm not accessible, using tcg
    kvm not accessible, using tcg
    kvm not accessible, using tcg
    kvm not accessible, using tcg
    kvm not accessible, using tcg

Philippe Mathieu-Daudé (2):
  libqtest: add qtest_accel() to avoid warnings when kvm is not
    available
  tests: use qtest_accel()

 tests/libqtest.h         |  8 ++++++++
 tests/bios-tables-test.c |  2 +-
 tests/boot-serial-test.c |  5 +++--
 tests/libqtest.c         | 18 ++++++++++++++++++
 tests/postcopy-test.c    | 10 ++++++----
 tests/pxe-test.c         |  5 +++--
 tests/vmgenid-test.c     |  4 ++--
 7 files changed, 41 insertions(+), 11 deletions(-)

-- 
2.14.1

[Qemu-devel] [RFC PATCH 1/2] libqtest: add qtest_accel() to avoid warnings when kvm is not available

[Philippe Mathieu-Daudé]

only warn once about it.

- kernel without kvm:

    # make check-qtest-x86_64
      GTESTER check-qtest-x86_64
    Could not access KVM kernel module: No such device
    qemu-system-x86_64: failed to initialize KVM: No such device
    qemu-system-x86_64: Back to tcg accelerator

- tests ran as user:

    $ make check-qtest-x86_64
      GTESTER check-qtest-x86_64
    Could not access KVM kernel module: Permission denied
    qemu-system-x86_64: failed to initialize KVM: Permission denied
    qemu-system-x86_64: Back to tcg accelerator

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 tests/libqtest.h |  8 ++++++++
 tests/libqtest.c | 18 ++++++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/tests/libqtest.h b/tests/libqtest.h
index 38bc1e9953..24e03148eb 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -927,4 +927,12 @@ QDict *qmp_fd(int fd, const char *fmt, ...);
  */
 void qtest_cb_for_every_machine(void (*cb)(const char *machine));
 
+/**
+ * qtest_accel:
+ * @accel: List of accelerators
+ *
+ *  Filter accelerators accessible on the host.
+ */
+const char *qtest_accel(const char *accel);
+
 #endif
diff --git a/tests/libqtest.c b/tests/libqtest.c
index b9a1f180e1..d2dfca35a3 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -987,3 +987,21 @@ void qtest_cb_for_every_machine(void (*cb)(const char *machine))
     qtest_end();
     QDECREF(response);
 }
+
+const char *qtest_accel(const char *accel)
+{
+    static bool kvm_accessible = true;
+
+    if (strlen(accel) <= 4 || strncmp(accel, "kvm:", 4)) {
+        return accel; /* no match */
+    }
+
+    if (!kvm_accessible || !access("/dev/kvm", W_OK)) {
+        accel += 4; /* skip "kvm:" */
+        if (kvm_accessible) {
+            kvm_accessible = false; /* warn once */
+            g_printerr("kvm not accessible, using %s\n", accel);
+        }
+    }
+    return accel;
+}
-- 
2.14.1

[Qemu-devel] [PATCH 2/2] tests: use qtest_accel()

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 tests/bios-tables-test.c |  2 +-
 tests/boot-serial-test.c |  5 +++--
 tests/postcopy-test.c    | 10 ++++++----
 tests/pxe-test.c         |  5 +++--
 tests/vmgenid-test.c     |  4 ++--
 5 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index 564da45f65..621bd8d95d 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -627,7 +627,7 @@ static void test_acpi_one(const char *params, test_data *data)
                            "-net none -display none %s "
                            "-drive id=hd0,if=none,file=%s,format=raw "
                            "-device ide-hd,drive=hd0 ",
-                           data->machine, "kvm:tcg",
+                           data->machine, qtest_accel("kvm:tcg"),
                            params ? params : "", disk);
 
     qtest_start(args);
diff --git a/tests/boot-serial-test.c b/tests/boot-serial-test.c
index a8ca877168..f986ea51b5 100644
--- a/tests/boot-serial-test.c
+++ b/tests/boot-serial-test.c
@@ -78,10 +78,11 @@ static void test_machine(const void *data)
     fd = mkstemp(tmpname);
     g_assert(fd != -1);
 
-    args = g_strdup_printf("-M %s,accel=kvm:tcg "
+    args = g_strdup_printf("-M %s,accel=%s "
                            "-chardev file,id=serial0,path=%s "
                            "-no-shutdown -serial chardev:serial0 %s",
-                           test->machine, tmpname, test->extra);
+                           test->machine, qtest_accel("kvm:tcg"),
+                           tmpname, test->extra);
 
     qtest_start(args);
     unlink(tmpname);
diff --git a/tests/postcopy-test.c b/tests/postcopy-test.c
index 8142f2ab90..781b955a59 100644
--- a/tests/postcopy-test.c
+++ b/tests/postcopy-test.c
@@ -367,18 +367,20 @@ static void test_migrate(void)
     got_stop = false;
 
     if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
+        const char *accel = qtest_accel("kvm:tcg");
+
         init_bootfile_x86(bootpath);
-        cmd_src = g_strdup_printf("-machine accel=kvm:tcg -m 150M"
+        cmd_src = g_strdup_printf("-machine accel=%s -m 150M"
                                   " -name pcsource,debug-threads=on"
                                   " -serial file:%s/src_serial"
                                   " -drive file=%s,format=raw",
-                                  tmpfs, bootpath);
-        cmd_dst = g_strdup_printf("-machine accel=kvm:tcg -m 150M"
+                                  accel, tmpfs, bootpath);
+        cmd_dst = g_strdup_printf("-machine accel=%s -m 150M"
                                   " -name pcdest,debug-threads=on"
                                   " -serial file:%s/dest_serial"
                                   " -drive file=%s,format=raw"
                                   " -incoming %s",
-                                  tmpfs, bootpath, uri);
+                                  accel, tmpfs, bootpath, uri);
     } else if (strcmp(arch, "ppc64") == 0) {
         const char *accel;
 
diff --git a/tests/pxe-test.c b/tests/pxe-test.c
index cf6e225330..531e0d5506 100644
--- a/tests/pxe-test.c
+++ b/tests/pxe-test.c
@@ -25,9 +25,10 @@ static void test_pxe_one(const char *params, bool ipv6)
 {
     char *args;
 
-    args = g_strdup_printf("-machine accel=kvm:tcg -nodefaults -boot order=n "
+    args = g_strdup_printf("-machine accel=%s -nodefaults -boot order=n "
                            "-netdev user,id=" NETNAME ",tftp=./,bootfile=%s,"
-                           "ipv4=%s,ipv6=%s %s", disk, ipv6 ? "off" : "on",
+                           "ipv4=%s,ipv6=%s %s", qtest_accel("kvm:tcg"),
+                           disk, ipv6 ? "off" : "on",
                            ipv6 ? "on" : "off", params);
 
     qtest_start(args);
diff --git a/tests/vmgenid-test.c b/tests/vmgenid-test.c
index 3d5c1c3615..82b52aa44c 100644
--- a/tests/vmgenid-test.c
+++ b/tests/vmgenid-test.c
@@ -132,11 +132,11 @@ static char disk[] = "tests/vmgenid-test-disk-XXXXXX";
 
 static char *guid_cmd_strdup(const char *guid)
 {
-    return g_strdup_printf("-machine accel=kvm:tcg "
+    return g_strdup_printf("-machine accel=%s "
                            "-device vmgenid,id=testvgid,guid=%s "
                            "-drive id=hd0,if=none,file=%s,format=raw "
                            "-device ide-hd,drive=hd0 ",
-                           guid, disk);
+                           qtest_accel("kvm:tcg"), guid, disk);
 }
 
 
-- 
2.14.1

Re: [Qemu-devel] [RFC PATCH 1/2] libqtest: add qtest_accel() to avoid warnings when kvm is not available

[Eric Blake]

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

On 08/15/2017 11:39 AM, Philippe Mathieu-Daudé wrote:
> only warn once about it.
> 
> - kernel without kvm:
> 
>     # make check-qtest-x86_64
>       GTESTER check-qtest-x86_64
>     Could not access KVM kernel module: No such device
>     qemu-system-x86_64: failed to initialize KVM: No such device
>     qemu-system-x86_64: Back to tcg accelerator

How does this differ from what commit 2f6b38d1 was trying to do?

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [RFC PATCH 1/2] libqtest: add qtest_accel() to avoid warnings when kvm is not available

[Philippe Mathieu-Daudé]

On 08/15/2017 03:13 PM, Eric Blake wrote:
> On 08/15/2017 11:39 AM, Philippe Mathieu-Daudé wrote:
>> only warn once about it.
>>
>> - kernel without kvm:
>>
>>      # make check-qtest-x86_64
>>        GTESTER check-qtest-x86_64
>>      Could not access KVM kernel module: No such device
>>      qemu-system-x86_64: failed to initialize KVM: No such device
>>      qemu-system-x86_64: Back to tcg accelerator
> 
> How does this differ from what commit 2f6b38d1 was trying to do?

I'd say 2f6b38d1 is for common end-user usage while this is for 
QA/testers usage. If you run N qtests with the same machine arguments, 
having this warning displayed only once is enough and allow you to focus 
on the tests output.

Re: [Qemu-devel] [RFC PATCH 1/2] libqtest: add qtest_accel() to avoid warnings when kvm is not available

[Peter Maydell]

On 15 August 2017 at 19:27, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> On 08/15/2017 03:13 PM, Eric Blake wrote:
>>
>> On 08/15/2017 11:39 AM, Philippe Mathieu-Daudé wrote:
>>>
>>> only warn once about it.
>>>
>>> - kernel without kvm:
>>>
>>>      # make check-qtest-x86_64
>>>        GTESTER check-qtest-x86_64
>>>      Could not access KVM kernel module: No such device
>>>      qemu-system-x86_64: failed to initialize KVM: No such device
>>>      qemu-system-x86_64: Back to tcg accelerator
>>
>>
>> How does this differ from what commit 2f6b38d1 was trying to do?
>
>
> I'd say 2f6b38d1 is for common end-user usage while this is for QA/testers
> usage. If you run N qtests with the same machine arguments, having this
> warning displayed only once is enough and allow you to focus on the tests
> output.

I think I'd rather we just straightforwardly didn't complain at
all in the 'make check' output. There's no benefit to this
for all these tests, especially given that they pretty much
don't care whether they run under KVM or TCG because they're
not trying to test that. (If they *are* trying to test KVM
specific functionality then they should either skip the
test quietly if KVM isn't present or they should fail it;
printing useless waffle to the logs isn't helpful IMHO.)

thanks
-- PMM

Re: [Qemu-devel] [RFC PATCH 1/2] libqtest: add qtest_accel() to avoid warnings when kvm is not available

[Eric Blake]

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

On 08/15/2017 01:27 PM, Philippe Mathieu-Daudé wrote:
> On 08/15/2017 03:13 PM, Eric Blake wrote:
>> On 08/15/2017 11:39 AM, Philippe Mathieu-Daudé wrote:
>>> only warn once about it.
>>>
>>> - kernel without kvm:
>>>
>>>      # make check-qtest-x86_64
>>>        GTESTER check-qtest-x86_64
>>>      Could not access KVM kernel module: No such device
>>>      qemu-system-x86_64: failed to initialize KVM: No such device
>>>      qemu-system-x86_64: Back to tcg accelerator
>>
>> How does this differ from what commit 2f6b38d1 was trying to do?
> 
> I'd say 2f6b38d1 is for common end-user usage while this is for
> QA/testers usage. If you run N qtests with the same machine arguments,
> having this warning displayed only once is enough and allow you to focus
> on the tests output.

I guess my question is: Are we still getting a warning output even with
2f6b38d1 applied?  If so, why, because the point of that commit was to
avoid the warning.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [RFC PATCH 1/2] libqtest: add qtest_accel() to avoid warnings when kvm is not available

[Eric Blake]

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

On 08/15/2017 01:40 PM, Eric Blake wrote:
> On 08/15/2017 01:27 PM, Philippe Mathieu-Daudé wrote:
>> On 08/15/2017 03:13 PM, Eric Blake wrote:
>>> On 08/15/2017 11:39 AM, Philippe Mathieu-Daudé wrote:
>>>> only warn once about it.
>>>>
>>>> - kernel without kvm:
>>>>
>>>>      # make check-qtest-x86_64
>>>>        GTESTER check-qtest-x86_64
>>>>      Could not access KVM kernel module: No such device
>>>>      qemu-system-x86_64: failed to initialize KVM: No such device
>>>>      qemu-system-x86_64: Back to tcg accelerator
>>>
>>> How does this differ from what commit 2f6b38d1 was trying to do?
>>
>> I'd say 2f6b38d1 is for common end-user usage while this is for
>> QA/testers usage. If you run N qtests with the same machine arguments,
>> having this warning displayed only once is enough and allow you to focus
>> on the tests output.
> 
> I guess my question is: Are we still getting a warning output even with
> 2f6b38d1 applied?  If so, why, because the point of that commit was to
> avoid the warning.

And answering my own question: yes, the builds are still verbose when
done in a VM that lacks nested kvm.  :(

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [RFC PATCH 1/2] libqtest: add qtest_accel() to avoid warnings when kvm is not available

[Philippe Mathieu-Daudé]

On 08/15/2017 03:40 PM, Eric Blake wrote:
> On 08/15/2017 01:27 PM, Philippe Mathieu-Daudé wrote:
>> On 08/15/2017 03:13 PM, Eric Blake wrote:
>>> On 08/15/2017 11:39 AM, Philippe Mathieu-Daudé wrote:
>>>> only warn once about it.
>>>>
>>>> - kernel without kvm:
>>>>
>>>>       # make check-qtest-x86_64
>>>>         GTESTER check-qtest-x86_64
>>>>       Could not access KVM kernel module: No such device
>>>>       qemu-system-x86_64: failed to initialize KVM: No such device
>>>>       qemu-system-x86_64: Back to tcg accelerator
>>>
>>> How does this differ from what commit 2f6b38d1 was trying to do?
>>
>> I'd say 2f6b38d1 is for common end-user usage while this is for
>> QA/testers usage. If you run N qtests with the same machine arguments,
>> having this warning displayed only once is enough and allow you to focus
>> on the tests output.
> 
> I guess my question is: Are we still getting a warning output even with
> 2f6b38d1 applied?  If so, why, because the point of that commit was to
> avoid the warning.

The function this patch adds is called by gtester to prepare the QEMU 
command line options, usually this is done once in main(), then each 
qtest is run with the same QEMU command line.

Once started QEMU calls accel_init_machine() displaying warnings from 
303d4e865b7.

Having 3 lines of warnings for each test is way too verbose.

Re: [Qemu-devel] [RFC PATCH 1/2] libqtest: add qtest_accel() to avoid warnings when kvm is not available

[Philippe Mathieu-Daudé]

On 08/15/2017 03:39 PM, Peter Maydell wrote:
> On 15 August 2017 at 19:27, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> On 08/15/2017 03:13 PM, Eric Blake wrote:
>>>
>>> On 08/15/2017 11:39 AM, Philippe Mathieu-Daudé wrote:
>>>>
>>>> only warn once about it.
>>>>
>>>> - kernel without kvm:
>>>>
>>>>       # make check-qtest-x86_64
>>>>         GTESTER check-qtest-x86_64
>>>>       Could not access KVM kernel module: No such device
>>>>       qemu-system-x86_64: failed to initialize KVM: No such device
>>>>       qemu-system-x86_64: Back to tcg accelerator
>>>
>>>
>>> How does this differ from what commit 2f6b38d1 was trying to do?
>>
>>
>> I'd say 2f6b38d1 is for common end-user usage while this is for QA/testers
>> usage. If you run N qtests with the same machine arguments, having this
>> warning displayed only once is enough and allow you to focus on the tests
>> output.
> 
> I think I'd rather we just straightforwardly didn't complain at
> all in the 'make check' output. There's no benefit to this
> for all these tests, especially given that they pretty much
> don't care whether they run under KVM or TCG because they're

Oh you mean checking qtest_enabled() within configure_accelerator()? Ok, 
clever :)

> not trying to test that. (If they *are* trying to test KVM
> specific functionality then they should either skip the
> test quietly if KVM isn't present or they should fail it;
> printing useless waffle to the logs isn't helpful IMHO.)
> 
> thanks
> -- PMM
>