From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46887) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1boov4-0001vn-Pk for qemu-devel@nongnu.org; Tue, 27 Sep 2016 05:43:24 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1boov0-0000uA-Ht for qemu-devel@nongnu.org; Tue, 27 Sep 2016 05:43:21 -0400 Date: Tue, 27 Sep 2016 18:29:28 +1000 From: David Gibson Message-ID: <20160927082928.GA14447@umbus.fritz.box> References: <1474899049-12506-1-git-send-email-lvivier@redhat.com> <1474899049-12506-3-git-send-email-lvivier@redhat.com> <20160927034812.GF15376@umbus.fritz.box> <3349b845-dc5e-f21a-1377-cbabb70b6eb9@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="9jxsPFA5p3P2qPhR" Content-Disposition: inline In-Reply-To: <3349b845-dc5e-f21a-1377-cbabb70b6eb9@redhat.com> Subject: Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/4] libqos: add PCI management in qtest_vboot()/qtest_shutdown() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Laurent Vivier Cc: qemu-devel@nongnu.org, thuth@redhat.com, Greg Kurz , qemu-ppc@nongnu.org, Gerd Hoffmann , dgibson@redhat.com --9jxsPFA5p3P2qPhR Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Sep 27, 2016 at 09:33:58AM +0200, Laurent Vivier wrote: >=20 >=20 > On 27/09/2016 05:48, David Gibson wrote: > > On Mon, Sep 26, 2016 at 04:10:47PM +0200, Laurent Vivier wrote: > >> Signed-off-by: Laurent Vivier > >> --- > >> 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(-) > >=20 > > Couple of queries below. > >=20 >=20 > ... >=20 > >> @@ -49,9 +54,15 @@ QOSState *qtest_boot(QOSOps *ops, const char *cmdli= ne_fmt, ...) > >> */ > >> void qtest_shutdown(QOSState *qs) > >> { > >> - if (qs->alloc && qs->ops && qs->ops->uninit_allocator) { > >> - qs->ops->uninit_allocator(qs->alloc); > >> - qs->alloc =3D NULL; > >> + if (qs->ops) { > >> + if (qs->alloc && qs->ops->uninit_allocator) { > >> + qs->ops->uninit_allocator(qs->alloc); > >> + qs->alloc =3D NULL; > >> + } > >> + if (qs->pcibus && qs->ops->qpci_free) { > >> + qs->ops->qpci_free(qs->pcibus); > >> + qs->pcibus =3D NULL; > >> + } > >=20 > > Is it safe to cleanup the allocator before the PCI stuff? Usually > > cleanups want to go in the opposite order to initialization. >=20 > Yes, you're right. Im' going to fix that. Ok. > >> } > >> 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; > >> =20 > >> typedef struct QOSState { > >> QTestState *qts; > >> QGuestAllocator *alloc; > >> + QPCIBus *pcibus; > >> QOSOps *ops; > >> } QOSState; > >> =20 > >> 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 *da= ta) > >> /* FIXME */ > >> } > >> =20 > >> -QPCIBus *qpci_init_pc(void) > >> +QPCIBus *qpci_init_pc(QGuestAllocator *alloc) > >> { > >> QPCIBusPC *ret; > >> > >=20 > > You've added the alloc parameter, but you don't actually appear to use = it.. >=20 > It's normal: qpci_init_spapr() needs it and to have the same function > signature we have to add it to qpci_init_pc() even if it is not used. > (it's why I have added a lot of of qpci_init_pc(NULL)), so we can add > init in a generic way in "struct QOSOps". Perhaps we can use "void > *opaque" instead of "QGuestAllocator *alloc"? Oh, of course. Sorry I missed that. --=20 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 --9jxsPFA5p3P2qPhR Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJX6i3lAAoJEGw4ysog2bOS7SQP+gNPNzBSKUHnHLaW0pb6QMYz oEsndY4a9EupRg+PmOQgoq+B37+oeGzoLND4pakz3EZmlV7g/1LsOmFpZjffVQhl +mlcmKgbnXdiLMIrnUoBdzLXeOiiBv/Fqg5if8S3WpR5Y+0L+J8DU/1DnWqk2oan TpmJyYw9OY1gVO2Hemhz0bHNKyoqfKm9peoHvnPbmZs2/329YYlKuGxJ5oX3iRTJ EG3rdaMGD4vEcwc/0svjbozFghP6Y/UtJDGsS/ZCUJ41W45vTXG5qosXP8O80gQj ip1nCrBucLWT4APcPnAi1uxZx8+2/M57/5lzMBbtIdjxDGbzm43JZx3k4/LCYPpr eewi9ZtAg0NXATzEJPzhHg1p2HoE20U8yKragDU2c1lprWlImK4Tvfk7y+mqzlos VpQnA9012VAgA8+KNERyD34MvL6vBdaN/fQ4xy54NSZ58bxbO09uAvuju/rhvKQJ oDejQF6W7+it0pc1a603xRNeT4adsFK7lkddsJjKpIc0bTd0mfu3XP25Itaik569 JvlH5z18Aox4Sg7KgYg8bPBFYnMxMZiOggdAbQt3SxaP/lMGcsIJlDlzIJLAvXse Loc6Kfb+9duCXfkX7f5VfMjStdYVzCOVucGRmvZUiML2R88XsljyycBzpvPHAjdg +WuL/aQfDobXnEr0fLmT =zz3c -----END PGP SIGNATURE----- --9jxsPFA5p3P2qPhR--