Re: [Qemu-devel] [PATCH 3/3] tests: enable virtio tests on SPAPR

[Greg Kurz]

On Fri, 30 Sep 2016 12:30:08 +0200
Laurent Vivier <address@hidden> wrote:

> On 30/09/2016 12:18, Greg Kurz wrote:
> > On Thu, 29 Sep 2016 19:15:07 +0200
> > Laurent Vivier <address@hidden> wrote:
> >   
> >> but disable MSI-X tests on SPAPR as we can't check the result
> >> (the memory region used on PC is not readable on SPAPR).
> >>
> >> Signed-off-by: Laurent Vivier <address@hidden>
> >> ---
> >>  tests/Makefile.include    |  3 ++-
> >>  tests/libqos/virtio-pci.c | 22 ++++++++++++++++++++--
> >>  tests/virtio-9p-test.c    | 11 ++++++++++-
> >>  tests/virtio-blk-test.c   | 22 +++++++++++++++++-----
> >>  tests/virtio-net-test.c   | 17 +++++++++++++++--
> >>  tests/virtio-rng-test.c   |  7 ++++++-
> >>  tests/virtio-scsi-test.c  | 10 +++++++++-
> >>  7 files changed, 79 insertions(+), 13 deletions(-)  
> ...
> >> diff --git a/tests/virtio-9p-test.c b/tests/virtio-9p-test.c
> >> index 28d7f5b..a73bccb 100644
> >> --- a/tests/virtio-9p-test.c
> >> +++ b/tests/virtio-9p-test.c
> >> @@ -11,6 +11,7 @@
> >>  #include "libqtest.h"
> >>  #include "qemu-common.h"
> >>  #include "libqos/libqos-pc.h"
> >> +#include "libqos/libqos-spapr.h"
> >>  #include "libqos/virtio.h"
> >>  #include "libqos/virtio-pci.h"
> >>  #include "standard-headers/linux/virtio_ids.h"
> >> @@ -22,12 +23,20 @@ static char *test_share;
> >>  
> >>  static QOSState *qvirtio_9p_start(void)
> >>  {
> >> +    const char *arch = qtest_get_arch();
> >> +    QOSState *qs = NULL;
> >>      test_share = g_strdup("/tmp/qtest.XXXXXX");
> >>      g_assert_nonnull(mkdtemp(test_share));
> >>      const char *cmd = "-fsdev local,id=fsdev0,security_model=none,path=%s 
> >> "
> >>                        "-device virtio-9p-pci,fsdev=fsdev0,mount_tag=%s";
> >>  
> >> -    return qtest_pc_boot(cmd, test_share, mount_tag);
> >> +    if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
> >> +        qs = qtest_pc_boot(cmd, test_share, mount_tag);
> >> +    } else if (strcmp(arch, "ppc64") == 0) {
> >> +        qs = qtest_spapr_boot(cmd, test_share, mount_tag);
> >> +    }
> >> +  
> > 
> > What about introducing a qtest_arch_boot() helper that does ^^ and
> > 
> > } else {
> >     g_printerr("qtest_arch_boot() not supported for arch %s\n",
> >                qtest_get_arch());
> >     exit(EXIT_FAILURE);
> > }
> >   
> 
> The problem with adding a function like that is it will pull
> $(libqos-pc-obj-y) and  $(libqos-spapr-obj-y) for every tests using it,
> and for the moment we are pulling pc or spapr objects only if we need
> them for the given test.
> 
> I think it explains why qtest_pc_boot() calls qtest_vboot() and we don't
> have a generic qtest_boot() calling the architecture specific function.
> 

Ok I see the problem... this would call for a libqos-all-arch-obj-y variable
to be used by specific tests maybe.

And by the way, even if not adding such generic function, each test that can
only run on specific archs should have an g_printerr()+exit() path, instead
of calling g_assert(), for the same reason it was done for rtas-test.c.

> I cc: John Snow as he has written the initial code for this.
> ("90e5add libqos: add pc specific interface")
> 
> Laurent
> 
> 

Cheers.

--
Greg