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

[Laurent Vivier]

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 <address@hidden>
>> ---
>>  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.
> 

...

>> @@ -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.

Yes, you're right. Im' going to fix that.

>>      }
>>      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..

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"?

Thanks,
Laurent