[Qemu-devel] [PATCH] tests: re-enable vhost-user-test

[Qemu-devel] [PATCH] tests: re-enable vhost-user-test

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Commit 7fe34ca9c2e actually disabled vhost-user-test altogether,
since CONFIG_VHOST_NET is a per-target config variable.

CONFIG_VHOST_NET_USED is the host config, actually unused since
1322629b4f2. But it was set whenever vhost_net was enabled.

tests/vhost-user-test is already x86/64 softmmu specific test, in order
to enable it correctly, kvm & vhost-net are also conditions. To check
that, change CONFIG_VHOST_NET_USED to be set when kvm is also enabled.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 configure      | 4 +---
 tests/Makefile | 4 +---
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/configure b/configure
index 7b5fcd2..bd2ffaf 100755
--- a/configure
+++ b/configure
@@ -5033,9 +5033,6 @@ fi
 if test "$vhost_scsi" = "yes" ; then
   echo "CONFIG_VHOST_SCSI=y" >> $config_host_mak
 fi
-if test "$vhost_net" = "yes" ; then
-  echo "CONFIG_VHOST_NET_USED=y" >> $config_host_mak
-fi
 if test "$blobs" = "yes" ; then
   echo "INSTALL_BLOBS=yes" >> $config_host_mak
 fi
@@ -5581,6 +5578,7 @@ case "$target_name" in
       echo "CONFIG_KVM=y" >> $config_target_mak
       if test "$vhost_net" = "yes" ; then
         echo "CONFIG_VHOST_NET=y" >> $config_target_mak
+        echo "CONFIG_VHOST_NET_USED=y" >> $config_host_mak
       fi
     fi
 esac
diff --git a/tests/Makefile b/tests/Makefile
index 209eca9..7eb70b3 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -191,9 +191,7 @@ gcov-files-i386-y += hw/usb/hcd-xhci.c
 check-qtest-i386-y += tests/pc-cpu-test$(EXESUF)
 check-qtest-i386-y += tests/q35-test$(EXESUF)
 gcov-files-i386-y += hw/pci-host/q35.c
-ifeq ($(CONFIG_VHOST_NET),y)
-check-qtest-i386-$(CONFIG_LINUX) += tests/vhost-user-test$(EXESUF)
-endif
+check-qtest-i386-$(CONFIG_VHOST_NET_USED) += tests/vhost-user-test$(EXESUF)
 check-qtest-i386-y += tests/test-netfilter$(EXESUF)
 check-qtest-x86_64-y = $(check-qtest-i386-y)
 gcov-files-i386-y += i386-softmmu/hw/timer/mc146818rtc.c
-- 
2.4.3

Re: [Qemu-devel] [PATCH] tests: re-enable vhost-user-test

[Marc-André Lureau]

nack, CONFIG_VHOST_NET_USED is actually used, my bad. I'll send another patch

On Thu, Oct 15, 2015 at 2:07 PM,  <marcandre.lureau@redhat.com> wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Commit 7fe34ca9c2e actually disabled vhost-user-test altogether,
> since CONFIG_VHOST_NET is a per-target config variable.
>
> CONFIG_VHOST_NET_USED is the host config, actually unused since
> 1322629b4f2. But it was set whenever vhost_net was enabled.
>
> tests/vhost-user-test is already x86/64 softmmu specific test, in order
> to enable it correctly, kvm & vhost-net are also conditions. To check
> that, change CONFIG_VHOST_NET_USED to be set when kvm is also enabled.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  configure      | 4 +---
>  tests/Makefile | 4 +---
>  2 files changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/configure b/configure
> index 7b5fcd2..bd2ffaf 100755
> --- a/configure
> +++ b/configure
> @@ -5033,9 +5033,6 @@ fi
>  if test "$vhost_scsi" = "yes" ; then
>    echo "CONFIG_VHOST_SCSI=y" >> $config_host_mak
>  fi
> -if test "$vhost_net" = "yes" ; then
> -  echo "CONFIG_VHOST_NET_USED=y" >> $config_host_mak
> -fi
>  if test "$blobs" = "yes" ; then
>    echo "INSTALL_BLOBS=yes" >> $config_host_mak
>  fi
> @@ -5581,6 +5578,7 @@ case "$target_name" in
>        echo "CONFIG_KVM=y" >> $config_target_mak
>        if test "$vhost_net" = "yes" ; then
>          echo "CONFIG_VHOST_NET=y" >> $config_target_mak
> +        echo "CONFIG_VHOST_NET_USED=y" >> $config_host_mak
>        fi
>      fi
>  esac
> diff --git a/tests/Makefile b/tests/Makefile
> index 209eca9..7eb70b3 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -191,9 +191,7 @@ gcov-files-i386-y += hw/usb/hcd-xhci.c
>  check-qtest-i386-y += tests/pc-cpu-test$(EXESUF)
>  check-qtest-i386-y += tests/q35-test$(EXESUF)
>  gcov-files-i386-y += hw/pci-host/q35.c
> -ifeq ($(CONFIG_VHOST_NET),y)
> -check-qtest-i386-$(CONFIG_LINUX) += tests/vhost-user-test$(EXESUF)
> -endif
> +check-qtest-i386-$(CONFIG_VHOST_NET_USED) += tests/vhost-user-test$(EXESUF)
>  check-qtest-i386-y += tests/test-netfilter$(EXESUF)
>  check-qtest-x86_64-y = $(check-qtest-i386-y)
>  gcov-files-i386-y += i386-softmmu/hw/timer/mc146818rtc.c
> --
> 2.4.3
>



-- 
Marc-André Lureau

[Qemu-devel] [PATCH] tests: re-enable vhost-user-test

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Commit 7fe34ca9c2e actually disabled vhost-user-test altogether,
since CONFIG_VHOST_NET is a per-target config variable.

tests/vhost-user-test is already x86/64 softmmu specific test, in order
to enable it correctly, kvm & vhost-net are also conditions. To check
that, set CONFIG_VHOST_NET_TEST when kvm is also enabled.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 configure      | 1 +
 tests/Makefile | 4 +---
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/configure b/configure
index 7b5fcd2..8d9b83c 100755
--- a/configure
+++ b/configure
@@ -5581,6 +5581,7 @@ case "$target_name" in
       echo "CONFIG_KVM=y" >> $config_target_mak
       if test "$vhost_net" = "yes" ; then
         echo "CONFIG_VHOST_NET=y" >> $config_target_mak
+        echo "CONFIG_VHOST_NET_TEST=y" >> $config_host_mak
       fi
     fi
 esac
diff --git a/tests/Makefile b/tests/Makefile
index 209eca9..518248a 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -191,9 +191,7 @@ gcov-files-i386-y += hw/usb/hcd-xhci.c
 check-qtest-i386-y += tests/pc-cpu-test$(EXESUF)
 check-qtest-i386-y += tests/q35-test$(EXESUF)
 gcov-files-i386-y += hw/pci-host/q35.c
-ifeq ($(CONFIG_VHOST_NET),y)
-check-qtest-i386-$(CONFIG_LINUX) += tests/vhost-user-test$(EXESUF)
-endif
+check-qtest-i386-$(CONFIG_VHOST_NET_TEST) += tests/vhost-user-test$(EXESUF)
 check-qtest-i386-y += tests/test-netfilter$(EXESUF)
 check-qtest-x86_64-y = $(check-qtest-i386-y)
 gcov-files-i386-y += i386-softmmu/hw/timer/mc146818rtc.c
-- 
2.4.3

Re: [Qemu-devel] [PATCH] tests: re-enable vhost-user-test

[Michael S. Tsirkin]

On Thu, Oct 15, 2015 at 04:39:25PM +0200, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Commit 7fe34ca9c2e actually disabled vhost-user-test altogether,
> since CONFIG_VHOST_NET is a per-target config variable.
> 
> tests/vhost-user-test is already x86/64 softmmu specific test, in order
> to enable it correctly, kvm & vhost-net are also conditions. To check
> that, set CONFIG_VHOST_NET_TEST when kvm is also enabled.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

I had to drop this, this still seems to break on some configs.
Pls work to fix this up.

> ---
>  configure      | 1 +
>  tests/Makefile | 4 +---
>  2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/configure b/configure
> index 7b5fcd2..8d9b83c 100755
> --- a/configure
> +++ b/configure
> @@ -5581,6 +5581,7 @@ case "$target_name" in
>        echo "CONFIG_KVM=y" >> $config_target_mak
>        if test "$vhost_net" = "yes" ; then
>          echo "CONFIG_VHOST_NET=y" >> $config_target_mak
> +        echo "CONFIG_VHOST_NET_TEST=y" >> $config_host_mak
>        fi
>      fi
>  esac
> diff --git a/tests/Makefile b/tests/Makefile
> index 209eca9..518248a 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -191,9 +191,7 @@ gcov-files-i386-y += hw/usb/hcd-xhci.c
>  check-qtest-i386-y += tests/pc-cpu-test$(EXESUF)
>  check-qtest-i386-y += tests/q35-test$(EXESUF)
>  gcov-files-i386-y += hw/pci-host/q35.c
> -ifeq ($(CONFIG_VHOST_NET),y)
> -check-qtest-i386-$(CONFIG_LINUX) += tests/vhost-user-test$(EXESUF)
> -endif
> +check-qtest-i386-$(CONFIG_VHOST_NET_TEST) += tests/vhost-user-test$(EXESUF)
>  check-qtest-i386-y += tests/test-netfilter$(EXESUF)
>  check-qtest-x86_64-y = $(check-qtest-i386-y)
>  gcov-files-i386-y += i386-softmmu/hw/timer/mc146818rtc.c
> -- 
> 2.4.3

Re: [Qemu-devel] [PATCH] tests: re-enable vhost-user-test

[Marc-André Lureau]

Hi

On Sat, Oct 24, 2015 at 9:44 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Thu, Oct 15, 2015 at 04:39:25PM +0200, marcandre.lureau@redhat.com wrote:
>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> Commit 7fe34ca9c2e actually disabled vhost-user-test altogether,
>> since CONFIG_VHOST_NET is a per-target config variable.
>>
>> tests/vhost-user-test is already x86/64 softmmu specific test, in order
>> to enable it correctly, kvm & vhost-net are also conditions. To check
>> that, set CONFIG_VHOST_NET_TEST when kvm is also enabled.
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> I had to drop this, this still seems to break on some configs.
> Pls work to fix this up.

I am not sure I understand completely the issue that Peter is having
on arm. I suppose my arm VM doesn't have kvm, and fails to reproduce
it. What probably happens is that CONFIG_VHOST_NET_TEST is enabled
because  "$target_name" = "$cpu" for $target_name = aarch64, then the
test is trying to run with the qemu-system-i386 binary, but that one
doesn't have vhost-net. We would probably need something like that
(pseudo-code, I failed to express this with Makefile):

@@ -5652,6 +5654,7 @@ case "$target_name" in
       echo "CONFIG_KVM=y" >> $config_target_mak
       if test "$vhost_net" = "yes" ; then
         echo "CONFIG_VHOST_NET=y" >> $config_target_mak
+        echo "CONFIG_VHOST_NET_TEST+=$cpu" >> $config_host_mak
       fi
     fi
 esac
diff --git a/tests/Makefile b/tests/Makefile
index 9341498..40fd02a 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -192,9 +192,8 @@ gcov-files-i386-y += hw/usb/hcd-xhci.c
 check-qtest-i386-y += tests/pc-cpu-test$(EXESUF)
 check-qtest-i386-y += tests/q35-test$(EXESUF)
 gcov-files-i386-y += hw/pci-host/q35.c
-ifeq ($(CONFIG_VHOST_NET),y)
-check-qtest-i386-$(CONFIG_LINUX) += tests/vhost-user-test$(EXESUF)
-endif
+# foreach CPU in CONFIG_VHOST_NET_TEST
+# check-qtest-$(CPU)-y += tests/vhost-user-test$(EXESUF)

I don't feel very confortable with that sort of per-host/per-target
complex configure-time conditions. I would rather simply use a simple
runtime test check such as:

+static bool check_qemu_support(const char *qemu_cmd)
+{
+    GError *err = NULL;
+    gchar *cmd = g_strdup_printf("%s %s -display none -monitor stdio",
+                                 getenv("QTEST_QEMU_BINARY"), qemu_cmd);
+    gchar **argv;
+    GPid pid;
+    int output, outerr, hmp, status = -1;
+
+    g_shell_parse_argv(cmd, NULL, &argv, &err);
+    g_assert_no_error(err);
+
+    g_spawn_async_with_pipes(NULL, argv, NULL, G_SPAWN_DO_NOT_REAP_CHILD,
+                             NULL, NULL, &pid, &hmp, &output, &outerr, &err);
+    g_assert_no_error(err);
+
+    write(hmp, "q\n", 2);
+    waitpid(pid, &status, 0);
+
+    g_strfreev(argv);
+    close(hmp);
+    close(output);
+    close(outerr);
+    g_spawn_close_pid(pid);
+
+    return status == 0;
+}
+
@@ -602,6 +630,9 @@ int main(int argc, char **argv)
     g_thread_new(NULL, thread_function, NULL);

     qemu_cmd = GET_QEMU_CMD(server);
+    if (!check_qemu_support(qemu_cmd)) {
+        goto cleanup;
+    }

any help appreciated

-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH] tests: re-enable vhost-user-test

[Michael S. Tsirkin]

On Mon, Oct 26, 2015 at 12:25:38PM +0100, Marc-André Lureau wrote:
> Hi
> 
> On Sat, Oct 24, 2015 at 9:44 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Thu, Oct 15, 2015 at 04:39:25PM +0200, marcandre.lureau@redhat.com wrote:
> >> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >>
> >> Commit 7fe34ca9c2e actually disabled vhost-user-test altogether,
> >> since CONFIG_VHOST_NET is a per-target config variable.
> >>
> >> tests/vhost-user-test is already x86/64 softmmu specific test, in order
> >> to enable it correctly, kvm & vhost-net are also conditions. To check
> >> that, set CONFIG_VHOST_NET_TEST when kvm is also enabled.
> >>
> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > I had to drop this, this still seems to break on some configs.
> > Pls work to fix this up.
> 
> I am not sure I understand completely the issue that Peter is having
> on arm. I suppose my arm VM doesn't have kvm, and fails to reproduce
> it. What probably happens is that CONFIG_VHOST_NET_TEST is enabled
> because  "$target_name" = "$cpu" for $target_name = aarch64, then the
> test is trying to run with the qemu-system-i386 binary, but that one
> doesn't have vhost-net. We would probably need something like that
> (pseudo-code, I failed to express this with Makefile):
> 
> @@ -5652,6 +5654,7 @@ case "$target_name" in
>        echo "CONFIG_KVM=y" >> $config_target_mak
>        if test "$vhost_net" = "yes" ; then
>          echo "CONFIG_VHOST_NET=y" >> $config_target_mak
> +        echo "CONFIG_VHOST_NET_TEST+=$cpu" >> $config_host_mak
>        fi
>      fi
>  esac
> diff --git a/tests/Makefile b/tests/Makefile
> index 9341498..40fd02a 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -192,9 +192,8 @@ gcov-files-i386-y += hw/usb/hcd-xhci.c
>  check-qtest-i386-y += tests/pc-cpu-test$(EXESUF)
>  check-qtest-i386-y += tests/q35-test$(EXESUF)
>  gcov-files-i386-y += hw/pci-host/q35.c
> -ifeq ($(CONFIG_VHOST_NET),y)
> -check-qtest-i386-$(CONFIG_LINUX) += tests/vhost-user-test$(EXESUF)
> -endif
> +# foreach CPU in CONFIG_VHOST_NET_TEST
> +# check-qtest-$(CPU)-y += tests/vhost-user-test$(EXESUF)

Like this then?

if test target_name == "i386" -o target_name == "x86_64"
then
	echo "CONFIG_VHOST_NET_TEST_$target_name=y" >> $config_host_mak
fi


ifeq ($(CONFIG_VHOST_NET_i386),y)
check-qtest-i386-y += tests/vhost-user-test$(EXESUF)
endif
ifeq ($(CONFIG_VHOST_NET_x86_64),y)
check-qtest-x86_64-y += tests/vhost-user-test$(EXESUF)
endif

> 
> I don't feel very confortable with that sort of per-host/per-target
> complex configure-time conditions. I would rather simply use a simple
> runtime test check such as:

Problem with runtime checks is it makes people not notice
there's a problem.

-- 
MST

Re: [Qemu-devel] [PATCH] tests: re-enable vhost-user-test

[Marc-André Lureau]

HI

On Mon, Oct 26, 2015 at 12:38 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> Like this then?
>
> if test target_name == "i386" -o target_name == "x86_64"
> then
>         echo "CONFIG_VHOST_NET_TEST_$target_name=y" >> $config_host_mak
> fi
>
>
> ifeq ($(CONFIG_VHOST_NET_i386),y)
> check-qtest-i386-y += tests/vhost-user-test$(EXESUF)
> endif
> ifeq ($(CONFIG_VHOST_NET_x86_64),y)
> check-qtest-x86_64-y += tests/vhost-user-test$(EXESUF)
> endif
>

I think I understand your idea, but it looks verbose, you are missing
the arm case, and I don't have a good way to test it.

>>
>> I don't feel very confortable with that sort of per-host/per-target
>> complex configure-time conditions. I would rather simply use a simple
>> runtime test check such as:
>
> Problem with runtime checks is it makes people not notice
> there's a problem.

fair enough


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH] tests: re-enable vhost-user-test

[Michael S. Tsirkin]

On Mon, Oct 26, 2015 at 01:09:56PM +0100, Marc-André Lureau wrote:
> HI
> 
> On Mon, Oct 26, 2015 at 12:38 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > Like this then?
> >
> > if test target_name == "i386" -o target_name == "x86_64"
> > then
> >         echo "CONFIG_VHOST_NET_TEST_$target_name=y" >> $config_host_mak
> > fi
> >
> >
> > ifeq ($(CONFIG_VHOST_NET_i386),y)
> > check-qtest-i386-y += tests/vhost-user-test$(EXESUF)
> > endif
> > ifeq ($(CONFIG_VHOST_NET_x86_64),y)
> > check-qtest-x86_64-y += tests/vhost-user-test$(EXESUF)
> > endif
> >
> 
> I think I understand your idea, but it looks verbose, you are missing
> the arm case,

We didn't previously support vhost-user-test on ARM,
adding that seems like a separate project.
In fact adding x86_64 might best be done in
a separate patch.


> and I don't have a good way to test it.
> >>
> >> I don't feel very confortable with that sort of per-host/per-target
> >> complex configure-time conditions. I would rather simply use a simple
> >> runtime test check such as:
> >
> > Problem with runtime checks is it makes people not notice
> > there's a problem.
> 
> fair enough
> 
> 
> -- 
> Marc-André Lureau