Message ID | CAJ+F1CKiqTjUcwXYn-w-jNy4gpADbpXS34pt5pMuiWCk7b=xXA@mail.gmail.com |
---|---|
State | New |
Headers | show |
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.
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
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
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