Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/4] libqos: add PCI management in qtest_vboot()/qtest_shutdown()

[David Gibson <david@gibson.dropbear.id.au>]

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

On Mon, Sep 26, 2016 at 04:10:47PM +0200, Laurent Vivier wrote:
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  tests/e1000e-test.c         |  2 +-
>  tests/i440fx-test.c         |  2 +-
>  tests/ide-test.c            |  2 +-
>  tests/ivshmem-test.c        |  2 +-
>  tests/libqos/ahci.c         |  2 +-
>  tests/libqos/libqos-pc.c    |  5 ++++-
>  tests/libqos/libqos-spapr.c |  5 ++++-
>  tests/libqos/libqos.c       | 21 ++++++++++++++++-----
>  tests/libqos/libqos.h       |  3 +++
>  tests/libqos/pci-pc.c       |  2 +-
>  tests/libqos/pci-pc.h       |  3 ++-
>  tests/q35-test.c            |  2 +-
>  tests/rtl8139-test.c        |  2 +-
>  tests/tco-test.c            |  2 +-
>  tests/usb-hcd-ehci-test.c   |  2 +-
>  tests/usb-hcd-uhci-test.c   |  2 +-
>  tests/vhost-user-test.c     |  2 +-
>  tests/virtio-9p-test.c      |  2 +-
>  tests/virtio-blk-test.c     |  2 +-
>  tests/virtio-net-test.c     |  2 +-
>  tests/virtio-scsi-test.c    |  2 +-
>  21 files changed, 45 insertions(+), 24 deletions(-)

Couple of queries below.

> 
> diff --git a/tests/e1000e-test.c b/tests/e1000e-test.c
> index d497b08..3979b20 100644
> --- a/tests/e1000e-test.c
> +++ b/tests/e1000e-test.c
> @@ -390,7 +390,7 @@ static void data_test_init(e1000e_device *d)
>      qtest_start(cmdline);
>      g_free(cmdline);
>  
> -    test_bus = qpci_init_pc();
> +    test_bus = qpci_init_pc(NULL);
>      g_assert_nonnull(test_bus);
>  
>      test_alloc = pc_alloc_init();
> diff --git a/tests/i440fx-test.c b/tests/i440fx-test.c
> index 3542ad1..da2d5a5 100644
> --- a/tests/i440fx-test.c
> +++ b/tests/i440fx-test.c
> @@ -38,7 +38,7 @@ static QPCIBus *test_start_get_bus(const TestData *s)
>      cmdline = g_strdup_printf("-smp %d", s->num_cpus);
>      qtest_start(cmdline);
>      g_free(cmdline);
> -    return qpci_init_pc();
> +    return qpci_init_pc(NULL);
>  }
>  
>  static void test_i440fx_defaults(gconstpointer opaque)
> diff --git a/tests/ide-test.c b/tests/ide-test.c
> index 1e51af2..a8a4081 100644
> --- a/tests/ide-test.c
> +++ b/tests/ide-test.c
> @@ -143,7 +143,7 @@ static QPCIDevice *get_pci_device(uint16_t *bmdma_base)
>      uint16_t vendor_id, device_id;
>  
>      if (!pcibus) {
> -        pcibus = qpci_init_pc();
> +        pcibus = qpci_init_pc(NULL);
>      }
>  
>      /* Find PCI device and verify it's the right one */
> diff --git a/tests/ivshmem-test.c b/tests/ivshmem-test.c
> index 0957ee7..f36bfe7 100644
> --- a/tests/ivshmem-test.c
> +++ b/tests/ivshmem-test.c
> @@ -105,7 +105,7 @@ static void setup_vm_cmd(IVState *s, const char *cmd, bool msix)
>      uint64_t barsize;
>  
>      s->qtest = qtest_start(cmd);
> -    s->pcibus = qpci_init_pc();
> +    s->pcibus = qpci_init_pc(NULL);
>      s->dev = get_device(s->pcibus);
>  
>      s->reg_base = qpci_iomap(s->dev, 0, &barsize);
> diff --git a/tests/libqos/ahci.c b/tests/libqos/ahci.c
> index f3be550..716ab79 100644
> --- a/tests/libqos/ahci.c
> +++ b/tests/libqos/ahci.c
> @@ -128,7 +128,7 @@ QPCIDevice *get_ahci_device(uint32_t *fingerprint)
>      uint32_t ahci_fingerprint;
>      QPCIBus *pcibus;
>  
> -    pcibus = qpci_init_pc();
> +    pcibus = qpci_init_pc(NULL);
>  
>      /* Find the AHCI PCI device and verify it's the right one. */
>      ahci = qpci_device_find(pcibus, QPCI_DEVFN(0x1F, 0x02));
> diff --git a/tests/libqos/libqos-pc.c b/tests/libqos/libqos-pc.c
> index df34092..aa17c98 100644
> --- a/tests/libqos/libqos-pc.c
> +++ b/tests/libqos/libqos-pc.c
> @@ -1,10 +1,13 @@
>  #include "qemu/osdep.h"
>  #include "libqos/libqos-pc.h"
>  #include "libqos/malloc-pc.h"
> +#include "libqos/pci-pc.h"
>  
>  static QOSOps qos_ops = {
>      .init_allocator = pc_alloc_init_flags,
> -    .uninit_allocator = pc_alloc_uninit
> +    .uninit_allocator = pc_alloc_uninit,
> +    .qpci_init = qpci_init_pc,
> +    .qpci_free = qpci_free_pc,
>  };
>  
>  QOSState *qtest_pc_vboot(const char *cmdline_fmt, va_list ap)
> diff --git a/tests/libqos/libqos-spapr.c b/tests/libqos/libqos-spapr.c
> index f19408b..125c6b3 100644
> --- a/tests/libqos/libqos-spapr.c
> +++ b/tests/libqos/libqos-spapr.c
> @@ -1,10 +1,13 @@
>  #include "qemu/osdep.h"
>  #include "libqos/libqos-spapr.h"
>  #include "libqos/malloc-spapr.h"
> +#include "libqos/pci-spapr.h"
>  
>  static QOSOps qos_ops = {
>      .init_allocator = spapr_alloc_init_flags,
> -    .uninit_allocator = spapr_alloc_uninit
> +    .uninit_allocator = spapr_alloc_uninit,
> +    .qpci_init = qpci_init_spapr,
> +    .qpci_free = qpci_free_spapr
>  };
>  
>  QOSState *qtest_spapr_vboot(const char *cmdline_fmt, va_list ap)
> diff --git a/tests/libqos/libqos.c b/tests/libqos/libqos.c
> index a852dc5..332d60e 100644
> --- a/tests/libqos/libqos.c
> +++ b/tests/libqos/libqos.c
> @@ -20,8 +20,13 @@ QOSState *qtest_vboot(QOSOps *ops, const char *cmdline_fmt, va_list ap)
>      cmdline = g_strdup_vprintf(cmdline_fmt, ap);
>      qs->qts = qtest_start(cmdline);
>      qs->ops = ops;
> -    if (ops && ops->init_allocator) {
> -        qs->alloc = ops->init_allocator(ALLOC_NO_FLAGS);
> +    if (ops) {
> +        if (ops->init_allocator) {
> +            qs->alloc = ops->init_allocator(ALLOC_NO_FLAGS);
> +        }
> +        if (ops->qpci_init && qs->alloc) {
> +            qs->pcibus = ops->qpci_init(qs->alloc);
> +        }
>      }
>  
>      g_free(cmdline);
> @@ -49,9 +54,15 @@ QOSState *qtest_boot(QOSOps *ops, const char *cmdline_fmt, ...)
>   */
>  void qtest_shutdown(QOSState *qs)
>  {
> -    if (qs->alloc && qs->ops && qs->ops->uninit_allocator) {
> -        qs->ops->uninit_allocator(qs->alloc);
> -        qs->alloc = NULL;
> +    if (qs->ops) {
> +        if (qs->alloc && qs->ops->uninit_allocator) {
> +            qs->ops->uninit_allocator(qs->alloc);
> +            qs->alloc = NULL;
> +        }
> +        if (qs->pcibus && qs->ops->qpci_free) {
> +            qs->ops->qpci_free(qs->pcibus);
> +            qs->pcibus = NULL;
> +        }

Is it safe to cleanup the allocator before the PCI stuff?  Usually
cleanups want to go in the opposite order to initialization.

>      }
>      qtest_quit(qs->qts);
>      g_free(qs);
> diff --git a/tests/libqos/libqos.h b/tests/libqos/libqos.h
> index 604980d..a9f6990 100644
> --- a/tests/libqos/libqos.h
> +++ b/tests/libqos/libqos.h
> @@ -8,11 +8,14 @@
>  typedef struct QOSOps {
>      QGuestAllocator *(*init_allocator)(QAllocOpts);
>      void (*uninit_allocator)(QGuestAllocator *);
> +    QPCIBus *(*qpci_init)(QGuestAllocator *alloc);
> +    void (*qpci_free)(QPCIBus *bus);
>  } QOSOps;
>  
>  typedef struct QOSState {
>      QTestState *qts;
>      QGuestAllocator *alloc;
> +    QPCIBus *pcibus;
>      QOSOps *ops;
>  } QOSState;
>  
> diff --git a/tests/libqos/pci-pc.c b/tests/libqos/pci-pc.c
> index 82066b8..9600ed6 100644
> --- a/tests/libqos/pci-pc.c
> +++ b/tests/libqos/pci-pc.c
> @@ -212,7 +212,7 @@ static void qpci_pc_iounmap(QPCIBus *bus, void *data)
>      /* FIXME */
>  }
>  
> -QPCIBus *qpci_init_pc(void)
> +QPCIBus *qpci_init_pc(QGuestAllocator *alloc)
>  {
>      QPCIBusPC *ret;
>

You've added the alloc parameter, but you don't actually appear to use it..

> diff --git a/tests/libqos/pci-pc.h b/tests/libqos/pci-pc.h
> index 2621179..9479b51 100644
> --- a/tests/libqos/pci-pc.h
> +++ b/tests/libqos/pci-pc.h
> @@ -14,8 +14,9 @@
>  #define LIBQOS_PCI_PC_H
>  
>  #include "libqos/pci.h"
> +#include "libqos/malloc.h"
>  
> -QPCIBus *qpci_init_pc(void);
> +QPCIBus *qpci_init_pc(QGuestAllocator *alloc);
>  void     qpci_free_pc(QPCIBus *bus);
>  
>  #endif
> diff --git a/tests/q35-test.c b/tests/q35-test.c
> index 71538fc..763fe3d 100644
> --- a/tests/q35-test.c
> +++ b/tests/q35-test.c
> @@ -42,7 +42,7 @@ static void test_smram_lock(void)
>      QPCIDevice *pcidev;
>      QDict *response;
>  
> -    pcibus = qpci_init_pc();
> +    pcibus = qpci_init_pc(NULL);
>      g_assert(pcibus != NULL);
>  
>      pcidev = qpci_device_find(pcibus, 0);
> diff --git a/tests/rtl8139-test.c b/tests/rtl8139-test.c
> index 13de7ee..c2f601a 100644
> --- a/tests/rtl8139-test.c
> +++ b/tests/rtl8139-test.c
> @@ -35,7 +35,7 @@ static QPCIDevice *get_device(void)
>  {
>      QPCIDevice *dev;
>  
> -    pcibus = qpci_init_pc();
> +    pcibus = qpci_init_pc(NULL);
>      qpci_device_foreach(pcibus, 0x10ec, 0x8139, save_fn, &dev);
>      g_assert(dev != NULL);
>  
> diff --git a/tests/tco-test.c b/tests/tco-test.c
> index 0d13aa8..0d201b1 100644
> --- a/tests/tco-test.c
> +++ b/tests/tco-test.c
> @@ -57,7 +57,7 @@ static void test_init(TestData *d)
>      qtest_irq_intercept_in(qs, "ioapic");
>      g_free(s);
>  
> -    bus = qpci_init_pc();
> +    bus = qpci_init_pc(NULL);
>      d->dev = qpci_device_find(bus, QPCI_DEVFN(0x1f, 0x00));
>      g_assert(d->dev != NULL);
>  
> diff --git a/tests/usb-hcd-ehci-test.c b/tests/usb-hcd-ehci-test.c
> index eb247ad..a4ceeaa 100644
> --- a/tests/usb-hcd-ehci-test.c
> +++ b/tests/usb-hcd-ehci-test.c
> @@ -56,7 +56,7 @@ static void pci_init(void)
>      if (pcibus) {
>          return;
>      }
> -    pcibus = qpci_init_pc();
> +    pcibus = qpci_init_pc(NULL);
>      g_assert(pcibus != NULL);
>  
>      qusb_pci_init_one(pcibus, &uhci1, QPCI_DEVFN(0x1d, 0), 4);
> diff --git a/tests/usb-hcd-uhci-test.c b/tests/usb-hcd-uhci-test.c
> index 5cd59ad..c24063e 100644
> --- a/tests/usb-hcd-uhci-test.c
> +++ b/tests/usb-hcd-uhci-test.c
> @@ -23,7 +23,7 @@ static void test_port(int port)
>      struct qhc uhci;
>  
>      g_assert(port > 0);
> -    pcibus = qpci_init_pc();
> +    pcibus = qpci_init_pc(NULL);
>      g_assert(pcibus != NULL);
>      qusb_pci_init_one(pcibus, &uhci, QPCI_DEVFN(0x1d, 0), 4);
>      uhci_port_test(&uhci, port - 1, UHCI_PORT_CCS);
> diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
> index b89a551..300308c 100644
> --- a/tests/vhost-user-test.c
> +++ b/tests/vhost-user-test.c
> @@ -146,7 +146,7 @@ static void init_virtio_dev(TestServer *s)
>      QVirtioPCIDevice *dev;
>      uint32_t features;
>  
> -    bus = qpci_init_pc();
> +    bus = qpci_init_pc(NULL);
>      g_assert_nonnull(bus);
>  
>      dev = qvirtio_pci_device_find(bus, VIRTIO_ID_NET);
> diff --git a/tests/virtio-9p-test.c b/tests/virtio-9p-test.c
> index b8fb6cd..e8b2196 100644
> --- a/tests/virtio-9p-test.c
> +++ b/tests/virtio-9p-test.c
> @@ -63,7 +63,7 @@ static QVirtIO9P *qvirtio_9p_pci_init(void)
>  
>      v9p = g_new0(QVirtIO9P, 1);
>      v9p->alloc = pc_alloc_init();
> -    v9p->bus = qpci_init_pc();
> +    v9p->bus = qpci_init_pc(NULL);
>  
>      dev = qvirtio_pci_device_find(v9p->bus, VIRTIO_ID_9P);
>      g_assert_nonnull(dev);
> diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
> index 811cf75..3c4fecc 100644
> --- a/tests/virtio-blk-test.c
> +++ b/tests/virtio-blk-test.c
> @@ -75,7 +75,7 @@ static QPCIBus *pci_test_start(void)
>      g_free(tmp_path);
>      g_free(cmdline);
>  
> -    return qpci_init_pc();
> +    return qpci_init_pc(NULL);
>  }
>  
>  static void arm_test_start(void)
> diff --git a/tests/virtio-net-test.c b/tests/virtio-net-test.c
> index 361506f..a343a6b 100644
> --- a/tests/virtio-net-test.c
> +++ b/tests/virtio-net-test.c
> @@ -62,7 +62,7 @@ static QPCIBus *pci_test_start(int socket)
>      qtest_start(cmdline);
>      g_free(cmdline);
>  
> -    return qpci_init_pc();
> +    return qpci_init_pc(NULL);
>  }
>  
>  static void driver_init(const QVirtioBus *bus, QVirtioDevice *dev)
> diff --git a/tests/virtio-scsi-test.c b/tests/virtio-scsi-test.c
> index f1489e6..79088bb 100644
> --- a/tests/virtio-scsi-test.c
> +++ b/tests/virtio-scsi-test.c
> @@ -146,7 +146,7 @@ static QVirtIOSCSI *qvirtio_scsi_pci_init(int slot)
>  
>      vs = g_new0(QVirtIOSCSI, 1);
>      vs->alloc = pc_alloc_init();
> -    vs->bus = qpci_init_pc();
> +    vs->bus = qpci_init_pc(NULL);
>  
>      dev = qvirtio_pci_device_find(vs->bus, VIRTIO_ID_SCSI);
>      vs->dev = (QVirtioDevice *)dev;

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]