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

[Greg Kurz <groug@kaod.org>]

On Fri, 30 Sep 2016 11:19:39 -0400
John Snow <jsnow@redhat.com> wrote:

> On 09/30/2016 06:30 AM, Laurent Vivier wrote:
> >
> >
> > On 30/09/2016 12:18, Greg Kurz wrote:  
> >> On Thu, 29 Sep 2016 19:15:07 +0200
> >> Laurent Vivier <lvivier@redhat.com> 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 <lvivier@redhat.com>
> >>> ---
> >>>  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.
> >
> > I cc: John Snow as he has written the initial code for this.
> > ("90e5add libqos: add pc specific interface")
> >
> > Laurent
> >
> >  
> 
> This was a while ago for me (and I was brand new to QEMU!), but that 
> sounds about right. I wasn't able to reason about requirements for other 
> architectures, so we made the PC-specific frontend to do the 
> configuration for us. libqos.o does not pull in any of the PC-specific 
> requirements as a result. Neither does the allocator.
> 
> I didn't necessarily design it to be like this, just a path of least 
> resistance type of thing.
> 
> You probably could make an ArchOps callback structure if you wanted and 
> pass that along to a generic bootup function to avoid the linking issues 
> if you wanted a one-size-fits-all initialization function.
> 
> --js

It looks like libqos may be divided in 3 families:
- base libqos for tests that don't need platform specific support (basically
  what we currently have in libqos-y)
- platfrom specific libqos for platform specific tests (libqos-pc, libqos-spapr)
- one-size-fits-all libqos for tests that should run on several platforms

Makes sense ?

--
Greg