[Qemu-devel] [PATCH 00/39] Delay destruction of memory regions to instance_finalize

[Qemu-devel] [PATCH 00/39] Delay destruction of memory regions to instance_finalize

[Paolo Bonzini]

QOM splits the destruction of a device in two phases:

- unrealize, also known as "exit" from qdev times, should isolate
  the device from the guest.  After unrealize returns, the guest
  should not be able to issue new requests.

- instance_finalize will reclaim the memory.  This is only called
  after all requests terminate and drop the references on the
  device.

This is important even now, but overlooked.  It will become much
more important for devices that will be able to access memory
out of the big QEMU lock.

This series changes all PCI devices (the sole to support hotplug
_and_ use MemoryRegions) to do memory_region_del_subregion at
unrealize time, and memory_region_destroy at instance_finalize
time.

This is mostly a PCI patch, and independent from the others,
so I believe it should go through mst's tree.

Paolo


Paolo Bonzini (39):
  scsi: keep device alive while it has requests
  dma: keep a device alive while it has SGLists
  pci: split exit and finalize
  ac97: use instance_finalize instead of exit
  es1370: use instance_finalize instead of exit
  hda: use instance_finalize instead of exit
  serial: use instance_finalize instead of exit
  tpci200: use instance_finalize instead of exit
  pci-assign: use instance_finalize instead of exit
  ahci: use instance_finalize instead of exit
  msix: split msix_free from msix_uninit
  cmd646: use instance_finalize instead of exit
  ide/piix: use instance_finalize instead of exit
  ide/via: use instance_finalize instead of exit
  ivshmem: use instance_finalize instead of exit
  pci-testdev: use instance_finalize instead of exit
  vfio: use instance_finalize instead of exit
  e1000: use instance_finalize instead of exit
  eepro100: use instance_finalize instead of exit
  ne2000: use instance_finalize instead of exit
  pcnet: use instance_finalize instead of exit
  rtl8139: use instance_finalize instead of exit
  vmxnet3: use instance_finalize instead of exit
  shpc: use instance_finalize instead of exit
  pci_bridge: split pci_bridge_exitfn from pci_bridge_free
  pcie_aer: pcie_aer_exit really frees stuff
  pci_bridge: use instance_finalize instead of exit
  ioh4320: use instance_finalize instead of exit
  xio3130-downstream: use instance_finalize instead of exit
  xio3130-upstream: use instance_finalize instead of exit
  pcie: do not recreate mmcfg I/O region, use an alias instead
  esp: use instance_finalize instead of exit
  lsi: use instance_finalize instead of exit
  pvscsi: use instance_finalize instead of exit
  usb-uhci: use instance_finalize instead of exit
  virtio-pci: use instance_finalize instead of exit
  wdt_i6300esb: use instance_finalize instead of exit
  xen_pt: use instance_finalize instead of exit
  tpm: move add/del_subregion to realize/unrealize

 dma-helpers.c                      |  6 ++++-
 hw/audio/ac97.c                    |  5 ++--
 hw/audio/es1370.c                  |  5 ++--
 hw/audio/intel-hda.c               |  8 ++++++
 hw/char/serial-pci.c               | 24 ++++++++++++++++++
 hw/char/tpci200.c                  |  5 ++--
 hw/i386/kvm/pci-assign.c           |  8 ++++++
 hw/ide/ahci.c                      |  5 ++--
 hw/ide/ahci.h                      |  2 +-
 hw/ide/cmd646.c                    |  5 ++--
 hw/ide/ich.c                       | 13 +++++++---
 hw/ide/macio.c                     |  4 +--
 hw/ide/piix.c                      |  8 +++---
 hw/ide/via.c                       |  5 ++--
 hw/misc/ivshmem.c                  | 10 +++++++-
 hw/misc/pci-testdev.c              |  5 ++--
 hw/misc/vfio.c                     | 52 +++++++++++++++++++++++++++++++++++---
 hw/net/e1000.c                     |  5 ++--
 hw/net/eepro100.c                  |  5 ++--
 hw/net/ne2000.c                    |  5 ++--
 hw/net/pcnet-pci.c                 |  5 ++--
 hw/net/rtl8139.c                   |  5 ++--
 hw/net/vmxnet3.c                   | 14 ++++++++--
 hw/pci-bridge/i82801b11.c          |  1 +
 hw/pci-bridge/ioh3420.c            | 11 +++++++-
 hw/pci-bridge/pci_bridge_dev.c     | 14 +++++++++-
 hw/pci-bridge/xio3130_downstream.c | 11 +++++++-
 hw/pci-bridge/xio3130_upstream.c   | 11 +++++++-
 hw/pci/msix.c                      | 26 ++++++++++++++-----
 hw/pci/pci.c                       | 15 ++++++++---
 hw/pci/pci_bridge.c                |  5 ++++
 hw/pci/pcie_aer.c                  |  3 ++-
 hw/pci/pcie_host.c                 | 22 ++++++++++++----
 hw/pci/shpc.c                      |  8 +++++-
 hw/scsi/esp-pci.c                  |  5 ++--
 hw/scsi/lsi53c895a.c               |  5 ++--
 hw/scsi/megasas.c                  |  4 +--
 hw/scsi/scsi-bus.c                 |  4 +++
 hw/scsi/virtio-scsi.c              | 10 +++++---
 hw/scsi/vmw_pvscsi.c               | 12 ++++++++-
 hw/tpm/tpm_tis.c                   | 14 +++++++---
 hw/usb/hcd-ehci.c                  |  4 +--
 hw/usb/hcd-uhci.c                  |  5 ++--
 hw/virtio/virtio-pci.c             | 10 +++++++-
 hw/watchdog/wdt_i6300esb.c         |  5 ++--
 hw/xen/xen_pt.c                    | 10 ++++++++
 hw/xen/xen_pt_config_init.c        |  3 ---
 hw/xen/xen_pt_msi.c                |  8 +++++-
 include/hw/pci/msix.h              |  1 +
 include/hw/pci/pci.h               |  2 +-
 include/hw/pci/pci_bridge.h        |  1 +
 include/hw/pci/pcie_aer.h          |  2 +-
 include/hw/pci/pcie_host.h         |  1 +
 include/hw/pci/shpc.h              |  1 +
 include/sysemu/dma.h               |  4 ++-
 55 files changed, 355 insertions(+), 92 deletions(-)

-- 
1.8.1.4

[Qemu-devel] [PATCH 01/39] scsi: keep device alive while it has requests

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi/scsi-bus.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
index 53ea906..e443193 100644
--- a/hw/scsi/scsi-bus.c
+++ b/hw/scsi/scsi-bus.c
@@ -516,6 +516,8 @@ SCSIRequest *scsi_req_alloc(const SCSIReqOps *reqops, SCSIDevice *d,
     req->status = -1;
     req->sense_len = 0;
     req->ops = reqops;
+    object_ref(OBJECT(d));
+    object_ref(OBJECT(req->bus->qbus.parent));
     trace_scsi_req_alloc(req->dev->id, req->lun, req->tag);
     return req;
 }
@@ -1505,6 +1507,8 @@ void scsi_req_unref(SCSIRequest *req)
         if (req->ops->free_req) {
             req->ops->free_req(req);
         }
+        object_unref(OBJECT(req->dev));
+        object_unref(OBJECT(bus->qbus.parent));
         g_free(req);
     }
 }
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 01/39] scsi: keep device alive while it has requests

[Andreas Färber]

Am 04.06.2013 20:51, schrieb Paolo Bonzini:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/scsi/scsi-bus.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
> index 53ea906..e443193 100644
> --- a/hw/scsi/scsi-bus.c
> +++ b/hw/scsi/scsi-bus.c
> @@ -516,6 +516,8 @@ SCSIRequest *scsi_req_alloc(const SCSIReqOps *reqops, SCSIDevice *d,
>      req->status = -1;
>      req->sense_len = 0;
>      req->ops = reqops;
> +    object_ref(OBJECT(d));
> +    object_ref(OBJECT(req->bus->qbus.parent));

BusState *bus = BUS(req->bus);
...
object_ref(OBJECT(bus->parent));

Same below.

Andreas

>      trace_scsi_req_alloc(req->dev->id, req->lun, req->tag);
>      return req;
>  }
> @@ -1505,6 +1507,8 @@ void scsi_req_unref(SCSIRequest *req)
>          if (req->ops->free_req) {
>              req->ops->free_req(req);
>          }
> +        object_unref(OBJECT(req->dev));
> +        object_unref(OBJECT(bus->qbus.parent));
>          g_free(req);
>      }
>  }
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 01/39] scsi: keep device alive while it has requests

[Anthony Liguori]

Andreas Färber <afaerber@suse.de> writes:

> Am 04.06.2013 20:51, schrieb Paolo Bonzini:
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  hw/scsi/scsi-bus.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>> 
>> diff --git a/hw/scsi/scsi-bus.c b/hw/scsi/scsi-bus.c
>> index 53ea906..e443193 100644
>> --- a/hw/scsi/scsi-bus.c
>> +++ b/hw/scsi/scsi-bus.c
>> @@ -516,6 +516,8 @@ SCSIRequest *scsi_req_alloc(const SCSIReqOps *reqops, SCSIDevice *d,
>>      req->status = -1;
>>      req->sense_len = 0;
>>      req->ops = reqops;
>> +    object_ref(OBJECT(d));
>> +    object_ref(OBJECT(req->bus->qbus.parent));
>
> BusState *bus = BUS(req->bus);
> ...
> object_ref(OBJECT(bus->parent));
>
> Same below.

If Paolo has to respin, ack.  But for both ways:

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori

>
> Andreas
>
>>      trace_scsi_req_alloc(req->dev->id, req->lun, req->tag);
>>      return req;
>>  }
>> @@ -1505,6 +1507,8 @@ void scsi_req_unref(SCSIRequest *req)
>>          if (req->ops->free_req) {
>>              req->ops->free_req(req);
>>          }
>> +        object_unref(OBJECT(req->dev));
>> +        object_unref(OBJECT(bus->qbus.parent));
>>          g_free(req);
>>      }
>>  }
>> 
>
>
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

[Qemu-devel] [PATCH 02/39] dma: keep a device alive while it has SGLists

[Paolo Bonzini]

A QEMUSGList has a reference to a device's address space.  Keep
the device alive while the QEMUSGList exists.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 dma-helpers.c         |  6 +++++-
 hw/ide/ahci.c         |  3 ++-
 hw/ide/macio.c        |  4 ++--
 hw/scsi/megasas.c     |  4 ++--
 hw/scsi/virtio-scsi.c | 10 ++++++----
 hw/usb/hcd-ehci.c     |  4 ++--
 include/hw/pci/pci.h  |  2 +-
 include/sysemu/dma.h  |  4 +++-
 8 files changed, 23 insertions(+), 14 deletions(-)

diff --git a/dma-helpers.c b/dma-helpers.c
index 5afba47..499550f 100644
--- a/dma-helpers.c
+++ b/dma-helpers.c
@@ -34,13 +34,16 @@ int dma_memory_set(AddressSpace *as, dma_addr_t addr, uint8_t c, dma_addr_t len)
     return error;
 }
 
-void qemu_sglist_init(QEMUSGList *qsg, int alloc_hint, AddressSpace *as)
+void qemu_sglist_init(QEMUSGList *qsg, DeviceState *dev, int alloc_hint,
+                      AddressSpace *as)
 {
     qsg->sg = g_malloc(alloc_hint * sizeof(ScatterGatherEntry));
     qsg->nsg = 0;
     qsg->nalloc = alloc_hint;
     qsg->size = 0;
     qsg->as = as;
+    qsg->dev = dev;
+    object_ref(OBJECT(dev));
 }
 
 void qemu_sglist_add(QEMUSGList *qsg, dma_addr_t base, dma_addr_t len)
@@ -57,6 +60,7 @@ void qemu_sglist_add(QEMUSGList *qsg, dma_addr_t base, dma_addr_t len)
 
 void qemu_sglist_destroy(QEMUSGList *qsg)
 {
+    object_unref(OBJECT(qsg->dev));
     g_free(qsg->sg);
     memset(qsg, 0, sizeof(*qsg));
 }
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 1adfa0b..6b87549 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -691,7 +691,8 @@ static int ahci_populate_sglist(AHCIDevice *ad, QEMUSGList *sglist, int offset)
             goto out;
         }
 
-        qemu_sglist_init(sglist, (sglist_alloc_hint - off_idx), ad->hba->as);
+        qemu_sglist_init(sglist, DEVICE(ad->hba), (sglist_alloc_hint - off_idx),
+                         ad->hba->as);
         qemu_sglist_add(sglist, le64_to_cpu(tbl[off_idx].addr + off_pos),
                         le32_to_cpu(tbl[off_idx].flags_size) + 1 - off_pos);
 
diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index a1952b0..84288ae 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -70,7 +70,7 @@ static void pmac_ide_atapi_transfer_cb(void *opaque, int ret)
 
     s->io_buffer_size = io->len;
 
-    qemu_sglist_init(&s->sg, io->len / MACIO_PAGE_SIZE + 1,
+    qemu_sglist_init(&s->sg, DEVICE(m), io->len / MACIO_PAGE_SIZE + 1,
                      &address_space_memory);
     qemu_sglist_add(&s->sg, io->addr, io->len);
     io->addr += io->len;
@@ -127,7 +127,7 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
     s->io_buffer_index = 0;
     s->io_buffer_size = io->len;
 
-    qemu_sglist_init(&s->sg, io->len / MACIO_PAGE_SIZE + 1,
+    qemu_sglist_init(&s->sg, DEVICE(m), io->len / MACIO_PAGE_SIZE + 1,
                      &address_space_memory);
     qemu_sglist_add(&s->sg, io->addr, io->len);
     io->addr += io->len;
diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index 7ee7d97..65ccb09 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -232,7 +232,7 @@ static int megasas_map_sgl(MegasasState *s, MegasasCmd *cmd, union mfi_sgl *sgl)
                                          MEGASAS_MAX_SGE);
         return iov_count;
     }
-    qemu_sglist_init(&cmd->qsg, iov_count, pci_get_address_space(&s->dev));
+    pci_dma_sglist_init(&cmd->qsg, &s->dev, iov_count);
     for (i = 0; i < iov_count; i++) {
         dma_addr_t iov_pa, iov_size_p;
 
@@ -628,7 +628,7 @@ static int megasas_map_dcmd(MegasasState *s, MegasasCmd *cmd)
     }
     iov_pa = megasas_sgl_get_addr(cmd, &cmd->frame->dcmd.sgl);
     iov_size = megasas_sgl_get_len(cmd, &cmd->frame->dcmd.sgl);
-    qemu_sglist_init(&cmd->qsg, 1, pci_get_address_space(&s->dev));
+    pci_dma_sglist_init(&cmd->qsg, &s->dev, 1);
     qemu_sglist_add(&cmd->qsg, iov_pa, iov_size);
     cmd->iov_size = iov_size;
     return cmd->iov_size;
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index b8a0abf..712f0ad 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -77,10 +77,12 @@ static void virtio_scsi_bad_req(void)
     exit(1);
 }
 
-static void qemu_sgl_init_external(QEMUSGList *qsgl, struct iovec *sg,
+static void qemu_sgl_init_external(VirtIOSCSIReq *req, struct iovec *sg,
                                    hwaddr *addr, int num)
 {
-    qemu_sglist_init(qsgl, num, &address_space_memory);
+    QEMUSGList *qsgl = &req->qsgl;
+
+    qemu_sglist_init(qsgl, DEVICE(req->dev), num, &address_space_memory);
     while (num--) {
         qemu_sglist_add(qsgl, *(addr++), (sg++)->iov_len);
     }
@@ -99,11 +101,11 @@ static void virtio_scsi_parse_req(VirtIOSCSI *s, VirtQueue *vq,
     req->resp.buf = req->elem.in_sg[0].iov_base;
 
     if (req->elem.out_num > 1) {
-        qemu_sgl_init_external(&req->qsgl, &req->elem.out_sg[1],
+        qemu_sgl_init_external(req, &req->elem.out_sg[1],
                                &req->elem.out_addr[1],
                                req->elem.out_num - 1);
     } else {
-        qemu_sgl_init_external(&req->qsgl, &req->elem.in_sg[1],
+        qemu_sgl_init_external(req, &req->elem.in_sg[1],
                                &req->elem.in_addr[1],
                                req->elem.in_num - 1);
     }
diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 1ad2159..79caa63 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -1245,7 +1245,7 @@ static int ehci_init_transfer(EHCIPacket *p)
     cpage  = get_field(p->qtd.token, QTD_TOKEN_CPAGE);
     bytes  = get_field(p->qtd.token, QTD_TOKEN_TBYTES);
     offset = p->qtd.bufptr[0] & ~QTD_BUFPTR_MASK;
-    qemu_sglist_init(&p->sgl, 5, p->queue->ehci->as);
+    qemu_sglist_init(&p->sgl, DEVICE(p->queue->ehci), 5, p->queue->ehci->as);
 
     while (bytes > 0) {
         if (cpage > 4) {
@@ -1484,7 +1484,7 @@ static int ehci_process_itd(EHCIState *ehci,
                 return -1;
             }
 
-            qemu_sglist_init(&ehci->isgl, 2, ehci->as);
+            qemu_sglist_init(&ehci->isgl, DEVICE(ehci), 2, ehci->as);
             if (off + len > 4096) {
                 /* transfer crosses page border */
                 uint32_t len2 = off + len - 4096;
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 6ef1f97..de406d6 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -702,7 +702,7 @@ static inline void pci_dma_unmap(PCIDevice *dev, void *buffer, dma_addr_t len,
 static inline void pci_dma_sglist_init(QEMUSGList *qsg, PCIDevice *dev,
                                        int alloc_hint)
 {
-    qemu_sglist_init(qsg, alloc_hint, pci_get_address_space(dev));
+    qemu_sglist_init(qsg, DEVICE(dev), alloc_hint, pci_get_address_space(dev));
 }
 
 extern const VMStateDescription vmstate_pci_device;
diff --git a/include/sysemu/dma.h b/include/sysemu/dma.h
index 031d1f5..00f21f3 100644
--- a/include/sysemu/dma.h
+++ b/include/sysemu/dma.h
@@ -29,6 +29,7 @@ struct QEMUSGList {
     int nsg;
     int nalloc;
     size_t size;
+    DeviceState *dev;
     AddressSpace *as;
 };
 
@@ -189,7 +190,8 @@ struct ScatterGatherEntry {
     dma_addr_t len;
 };
 
-void qemu_sglist_init(QEMUSGList *qsg, int alloc_hint, AddressSpace *as);
+void qemu_sglist_init(QEMUSGList *qsg, DeviceState *dev, int alloc_hint,
+                      AddressSpace *as);
 void qemu_sglist_add(QEMUSGList *qsg, dma_addr_t base, dma_addr_t len);
 void qemu_sglist_destroy(QEMUSGList *qsg);
 #endif
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 02/39] dma: keep a device alive while it has SGLists

[Andreas Färber]

Am 04.06.2013 20:51, schrieb Paolo Bonzini:
> A QEMUSGList has a reference to a device's address space.  Keep
> the device alive while the QEMUSGList exists.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  dma-helpers.c         |  6 +++++-
>  hw/ide/ahci.c         |  3 ++-
>  hw/ide/macio.c        |  4 ++--
>  hw/scsi/megasas.c     |  4 ++--
>  hw/scsi/virtio-scsi.c | 10 ++++++----
>  hw/usb/hcd-ehci.c     |  4 ++--
>  include/hw/pci/pci.h  |  2 +-
>  include/sysemu/dma.h  |  4 +++-
>  8 files changed, 23 insertions(+), 14 deletions(-)
[...]
> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
> index 7ee7d97..65ccb09 100644
> --- a/hw/scsi/megasas.c
> +++ b/hw/scsi/megasas.c
> @@ -232,7 +232,7 @@ static int megasas_map_sgl(MegasasState *s, MegasasCmd *cmd, union mfi_sgl *sgl)
>                                           MEGASAS_MAX_SGE);
>          return iov_count;
>      }
> -    qemu_sglist_init(&cmd->qsg, iov_count, pci_get_address_space(&s->dev));
> +    pci_dma_sglist_init(&cmd->qsg, &s->dev, iov_count);

PCI_DEVICE(s)?

>      for (i = 0; i < iov_count; i++) {
>          dma_addr_t iov_pa, iov_size_p;
>  
> @@ -628,7 +628,7 @@ static int megasas_map_dcmd(MegasasState *s, MegasasCmd *cmd)
>      }
>      iov_pa = megasas_sgl_get_addr(cmd, &cmd->frame->dcmd.sgl);
>      iov_size = megasas_sgl_get_len(cmd, &cmd->frame->dcmd.sgl);
> -    qemu_sglist_init(&cmd->qsg, 1, pci_get_address_space(&s->dev));
> +    pci_dma_sglist_init(&cmd->qsg, &s->dev, 1);

Ditto?

>      qemu_sglist_add(&cmd->qsg, iov_pa, iov_size);
>      cmd->iov_size = iov_size;
>      return cmd->iov_size;
[snip]

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 02/39] dma: keep a device alive while it has SGLists

[Anthony Liguori]

Andreas Färber <afaerber@suse.de> writes:

> Am 04.06.2013 20:51, schrieb Paolo Bonzini:
>> A QEMUSGList has a reference to a device's address space.  Keep
>> the device alive while the QEMUSGList exists.
>> 
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  dma-helpers.c         |  6 +++++-
>>  hw/ide/ahci.c         |  3 ++-
>>  hw/ide/macio.c        |  4 ++--
>>  hw/scsi/megasas.c     |  4 ++--
>>  hw/scsi/virtio-scsi.c | 10 ++++++----
>>  hw/usb/hcd-ehci.c     |  4 ++--
>>  include/hw/pci/pci.h  |  2 +-
>>  include/sysemu/dma.h  |  4 +++-
>>  8 files changed, 23 insertions(+), 14 deletions(-)
> [...]
>> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
>> index 7ee7d97..65ccb09 100644
>> --- a/hw/scsi/megasas.c
>> +++ b/hw/scsi/megasas.c
>> @@ -232,7 +232,7 @@ static int megasas_map_sgl(MegasasState *s, MegasasCmd *cmd, union mfi_sgl *sgl)
>>                                           MEGASAS_MAX_SGE);
>>          return iov_count;
>>      }
>> -    qemu_sglist_init(&cmd->qsg, iov_count, pci_get_address_space(&s->dev));
>> +    pci_dma_sglist_init(&cmd->qsg, &s->dev, iov_count);
>
> PCI_DEVICE(s)?

With same caveat at patch 1.

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori

>
>>      for (i = 0; i < iov_count; i++) {
>>          dma_addr_t iov_pa, iov_size_p;
>>  
>> @@ -628,7 +628,7 @@ static int megasas_map_dcmd(MegasasState *s, MegasasCmd *cmd)
>>      }
>>      iov_pa = megasas_sgl_get_addr(cmd, &cmd->frame->dcmd.sgl);
>>      iov_size = megasas_sgl_get_len(cmd, &cmd->frame->dcmd.sgl);
>> -    qemu_sglist_init(&cmd->qsg, 1, pci_get_address_space(&s->dev));
>> +    pci_dma_sglist_init(&cmd->qsg, &s->dev, 1);
>
> Ditto?
>
>>      qemu_sglist_add(&cmd->qsg, iov_pa, iov_size);
>>      cmd->iov_size = iov_size;
>>      return cmd->iov_size;
> [snip]
>
> Andreas
>
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

[Qemu-devel] [PATCH 03/39] pci: split exit and finalize

[Paolo Bonzini]

To properly support devices that do DMA out of the BQL, destruction
needs to be done in two phases.  First, the device is unrealized;
at this point, pending memory accesses can still be completed, but
no new accesses will be started.  The second part is freeing the
device, which happens only after the reference count drops to zero;
this means that all memory accesses are complete.

This patch changes the PCI core to delay destruction of the
bus-master address space and root region.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/pci/pci.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 776ad96..b60d9d0d 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -779,6 +779,16 @@ static void pci_config_free(PCIDevice *pci_dev)
     g_free(pci_dev->used);
 }
 
+static void pci_device_instance_finalize(Object *obj)
+{
+    PCIDevice *pci_dev = PCI_DEVICE(obj);
+
+    qemu_free_irqs(pci_dev->irq);
+
+    address_space_destroy(&pci_dev->bus_master_as);
+    memory_region_destroy(&pci_dev->bus_master_enable_region);
+}
+
 /* -1 for devfn means auto assign */
 static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
                                          const char *name, int devfn)
@@ -866,12 +876,8 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
 
 static void do_pci_unregister_device(PCIDevice *pci_dev)
 {
-    qemu_free_irqs(pci_dev->irq);
     pci_dev->bus->devices[pci_dev->devfn] = NULL;
     pci_config_free(pci_dev);
-
-    address_space_destroy(&pci_dev->bus_master_as);
-    memory_region_destroy(&pci_dev->bus_master_enable_region);
 }
 
 static void pci_unregister_io_regions(PCIDevice *pci_dev)
@@ -2243,6 +2249,7 @@ static const TypeInfo pci_device_type_info = {
     .abstract = true,
     .class_size = sizeof(PCIDeviceClass),
     .class_init = pci_device_class_init,
+    .instance_finalize = pci_device_instance_finalize,
 };
 
 static void pci_register_types(void)
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 03/39] pci: split exit and finalize

[Anthony Liguori]

Paolo Bonzini <pbonzini@redhat.com> writes:

> To properly support devices that do DMA out of the BQL, destruction
> needs to be done in two phases.  First, the device is unrealized;
> at this point, pending memory accesses can still be completed, but
> no new accesses will be started.  The second part is freeing the
> device, which happens only after the reference count drops to zero;
> this means that all memory accesses are complete.
>
> This patch changes the PCI core to delay destruction of the
> bus-master address space and root region.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori

> ---
>  hw/pci/pci.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 776ad96..b60d9d0d 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -779,6 +779,16 @@ static void pci_config_free(PCIDevice *pci_dev)
>      g_free(pci_dev->used);
>  }
>  
> +static void pci_device_instance_finalize(Object *obj)
> +{
> +    PCIDevice *pci_dev = PCI_DEVICE(obj);
> +
> +    qemu_free_irqs(pci_dev->irq);
> +
> +    address_space_destroy(&pci_dev->bus_master_as);
> +    memory_region_destroy(&pci_dev->bus_master_enable_region);
> +}
> +
>  /* -1 for devfn means auto assign */
>  static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
>                                           const char *name, int devfn)
> @@ -866,12 +876,8 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
>  
>  static void do_pci_unregister_device(PCIDevice *pci_dev)
>  {
> -    qemu_free_irqs(pci_dev->irq);
>      pci_dev->bus->devices[pci_dev->devfn] = NULL;
>      pci_config_free(pci_dev);
> -
> -    address_space_destroy(&pci_dev->bus_master_as);
> -    memory_region_destroy(&pci_dev->bus_master_enable_region);
>  }
>  
>  static void pci_unregister_io_regions(PCIDevice *pci_dev)
> @@ -2243,6 +2249,7 @@ static const TypeInfo pci_device_type_info = {
>      .abstract = true,
>      .class_size = sizeof(PCIDeviceClass),
>      .class_init = pci_device_class_init,
> +    .instance_finalize = pci_device_instance_finalize,
>  };
>  
>  static void pci_register_types(void)
> -- 
> 1.8.1.4

[Qemu-devel] [PATCH 04/39] ac97: use instance_finalize instead of exit

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/audio/ac97.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/audio/ac97.c b/hw/audio/ac97.c
index baf138b..f53ca74 100644
--- a/hw/audio/ac97.c
+++ b/hw/audio/ac97.c
@@ -1388,8 +1388,9 @@ static int ac97_initfn (PCIDevice *dev)
     return 0;
 }
 
-static void ac97_exitfn (PCIDevice *dev)
+static void ac97_instance_finalize (Object *obj)
 {
+    PCIDevice *dev = PCI_DEVICE(obj);
     AC97LinkState *s = DO_UPCAST (AC97LinkState, dev, dev);
 
     memory_region_destroy (&s->io_nam);
@@ -1413,7 +1414,6 @@ static void ac97_class_init (ObjectClass *klass, void *data)
     PCIDeviceClass *k = PCI_DEVICE_CLASS (klass);
 
     k->init = ac97_initfn;
-    k->exit = ac97_exitfn;
     k->vendor_id = PCI_VENDOR_ID_INTEL;
     k->device_id = PCI_DEVICE_ID_INTEL_82801AA_5;
     k->revision = 0x01;
@@ -1428,6 +1428,7 @@ static const TypeInfo ac97_info = {
     .parent        = TYPE_PCI_DEVICE,
     .instance_size = sizeof (AC97LinkState),
     .class_init    = ac97_class_init,
+    .instance_finalize = ac97_instance_finalize,
 };
 
 static void ac97_register_types (void)
-- 
1.8.1.4

[Qemu-devel] [PATCH 05/39] es1370: use instance_finalize instead of exit

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/audio/es1370.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/audio/es1370.c b/hw/audio/es1370.c
index c1cd169..a89ee10 100644
--- a/hw/audio/es1370.c
+++ b/hw/audio/es1370.c
@@ -1044,8 +1044,9 @@ static int es1370_initfn (PCIDevice *dev)
     return 0;
 }
 
-static void es1370_exitfn (PCIDevice *dev)
+static void es1370_instance_finalize (Object *obj)
 {
+    PCIDevice *dev = PCI_DEVICE(obj);
     ES1370State *s = DO_UPCAST (ES1370State, dev, dev);
 
     memory_region_destroy (&s->io);
@@ -1063,7 +1064,6 @@ static void es1370_class_init (ObjectClass *klass, void *data)
     PCIDeviceClass *k = PCI_DEVICE_CLASS (klass);
 
     k->init = es1370_initfn;
-    k->exit = es1370_exitfn;
     k->vendor_id = PCI_VENDOR_ID_ENSONIQ;
     k->device_id = PCI_DEVICE_ID_ENSONIQ_ES1370;
     k->class_id = PCI_CLASS_MULTIMEDIA_AUDIO;
@@ -1078,6 +1078,7 @@ static const TypeInfo es1370_info = {
     .parent        = TYPE_PCI_DEVICE,
     .instance_size = sizeof (ES1370State),
     .class_init    = es1370_class_init,
+    .instance_finalize = es1370_instance_finalize,
 };
 
 static void es1370_register_types (void)
-- 
1.8.1.4

[Qemu-devel] [PATCH 06/39] hda: split exit and instance_finalize

[Paolo Bonzini]

MSI is still terminated at unrealize time.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/audio/intel-hda.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
index 1016af0..4f2dcf3 100644
--- a/hw/audio/intel-hda.c
+++ b/hw/audio/intel-hda.c
@@ -1148,6 +1148,13 @@ static void intel_hda_exit(PCIDevice *pci)
     IntelHDAState *d = DO_UPCAST(IntelHDAState, pci, pci);
 
     msi_uninit(&d->pci);
+}
+
+static void intel_hda_instance_finalize(Object *obj)
+{
+    PCIDevice *pci = PCI_DEVICE(obj);
+    IntelHDAState *d = DO_UPCAST(IntelHDAState, pci, pci);
+
     memory_region_destroy(&d->mmio);
 }
 
@@ -1273,6 +1280,7 @@ static const TypeInfo intel_hda_info_ich6 = {
     .parent        = TYPE_PCI_DEVICE,
     .instance_size = sizeof(IntelHDAState),
     .class_init    = intel_hda_class_init_ich6,
+    .instance_finalize = intel_hda_instance_finalize,
 };
 
 static const TypeInfo intel_hda_info_ich9 = {
-- 
1.8.1.4

[Qemu-devel] [PATCH 07/39] serial: split exit and instance_finalize

[Paolo Bonzini]

Character devices are still detached at "unrealize" time.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/char/serial-pci.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/hw/char/serial-pci.c b/hw/char/serial-pci.c
index 6b6106b..8cdec94 100644
--- a/hw/char/serial-pci.c
+++ b/hw/char/serial-pci.c
@@ -118,6 +118,14 @@ static void serial_pci_exit(PCIDevice *dev)
     SerialState *s = &pci->state;
 
     serial_exit_core(s);
+}
+
+static void serial_pci_instance_finalize(Object *obj)
+{
+    PCIDevice *dev = PCI_DEVICE(obj);
+    PCISerialState *pci = DO_UPCAST(PCISerialState, dev, dev);
+    SerialState *s = &pci->state;
+
     memory_region_destroy(&s->io);
 }
 
@@ -130,9 +138,22 @@ static void multi_serial_pci_exit(PCIDevice *dev)
     for (i = 0; i < pci->ports; i++) {
         s = pci->state + i;
         serial_exit_core(s);
+    }
+}
+
+static void multi_serial_pci_instance_finalize(Object *obj)
+{
+    PCIDevice *dev = PCI_DEVICE(obj);
+    PCIMultiSerialState *pci = DO_UPCAST(PCIMultiSerialState, dev, dev);
+    SerialState *s;
+    int i;
+
+    for (i = 0; i < pci->ports; i++) {
+        s = pci->state + i;
         memory_region_destroy(&s->io);
         g_free(pci->name[i]);
     }
+
     memory_region_destroy(&pci->iobar);
     qemu_free_irqs(pci->irqs);
 }
@@ -227,6 +248,7 @@ static const TypeInfo serial_pci_info = {
     .parent        = TYPE_PCI_DEVICE,
     .instance_size = sizeof(PCISerialState),
     .class_init    = serial_pci_class_initfn,
+    .instance_finalize = serial_pci_instance_finalize,
 };
 
 static const TypeInfo multi_2x_serial_pci_info = {
@@ -234,6 +256,7 @@ static const TypeInfo multi_2x_serial_pci_info = {
     .parent        = TYPE_PCI_DEVICE,
     .instance_size = sizeof(PCIMultiSerialState),
     .class_init    = multi_2x_serial_pci_class_initfn,
+    .instance_finalize = multi_serial_pci_instance_finalize,
 };
 
 static const TypeInfo multi_4x_serial_pci_info = {
@@ -241,6 +264,7 @@ static const TypeInfo multi_4x_serial_pci_info = {
     .parent        = TYPE_PCI_DEVICE,
     .instance_size = sizeof(PCIMultiSerialState),
     .class_init    = multi_4x_serial_pci_class_initfn,
+    .instance_finalize = multi_serial_pci_instance_finalize,
 };
 
 static void serial_pci_register_types(void)
-- 
1.8.1.4

[Qemu-devel] [PATCH 08/39] tpci200: use instance_finalize instead of exit

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/char/tpci200.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/char/tpci200.c b/hw/char/tpci200.c
index 0170602..18e199b 100644
--- a/hw/char/tpci200.c
+++ b/hw/char/tpci200.c
@@ -613,8 +613,9 @@ static int tpci200_initfn(PCIDevice *pci_dev)
     return 0;
 }
 
-static void tpci200_exitfn(PCIDevice *pci_dev)
+static void tpci200_instance_finalize(Object *obj)
 {
+    PCIDevice *pci_dev = PCI_DEVICE(obj);
     TPCI200State *s = TPCI200(pci_dev);
 
     memory_region_destroy(&s->mmio);
@@ -646,7 +647,6 @@ static void tpci200_class_init(ObjectClass *klass, void *data)
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
     k->init = tpci200_initfn;
-    k->exit = tpci200_exitfn;
     k->vendor_id = PCI_VENDOR_ID_TEWS;
     k->device_id = PCI_DEVICE_ID_TEWS_TPCI200;
     k->class_id = PCI_CLASS_BRIDGE_OTHER;
@@ -661,6 +661,7 @@ static const TypeInfo tpci200_info = {
     .parent        = TYPE_PCI_DEVICE,
     .instance_size = sizeof(TPCI200State),
     .class_init    = tpci200_class_init,
+    .instance_finalize = tpci200_instance_finalize,
 };
 
 static void tpci200_register_types(void)
-- 
1.8.1.4

[Qemu-devel] [PATCH 09/39] pci-assign: split exit and instance_finalize

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/i386/kvm/pci-assign.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
index 4b1c2d9..6eb4c96 100644
--- a/hw/i386/kvm/pci-assign.c
+++ b/hw/i386/kvm/pci-assign.c
@@ -1837,6 +1837,13 @@ static void assigned_exitfn(struct PCIDevice *pci_dev)
     AssignedDevice *dev = DO_UPCAST(AssignedDevice, dev, pci_dev);
 
     deassign_device(dev);
+}
+
+static void assigned_instance_finalize(Object *obj)
+{
+    PCIDevice *pci_dev = PCI_DEVICE(obj);
+    AssignedDevice *dev = DO_UPCAST(AssignedDevice, dev, pci_dev);
+
     free_assigned_device(dev);
 }
 
@@ -1871,6 +1878,7 @@ static const TypeInfo assign_info = {
     .parent             = TYPE_PCI_DEVICE,
     .instance_size      = sizeof(AssignedDevice),
     .class_init         = assign_class_init,
+    .instance_finalize  = assigned_instance_finalize,
 };
 
 static void assign_register_types(void)
-- 
1.8.1.4

[Qemu-devel] [PATCH 10/39] ahci: split exit and instance_finalize

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/ide/ahci.c |  2 +-
 hw/ide/ahci.h |  2 +-
 hw/ide/ich.c  | 13 +++++++++----
 3 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 6b87549..083e0f4 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1174,7 +1174,7 @@ void ahci_init(AHCIState *s, DeviceState *qdev, AddressSpace *as, int ports)
     }
 }
 
-void ahci_uninit(AHCIState *s)
+void ahci_instance_finalize(AHCIState *s)
 {
     memory_region_destroy(&s->mem);
     memory_region_destroy(&s->idp);
diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
index 341a571..b2197c2 100644
--- a/hw/ide/ahci.h
+++ b/hw/ide/ahci.h
@@ -339,7 +339,7 @@ typedef struct NCQFrame {
 } QEMU_PACKED NCQFrame;
 
 void ahci_init(AHCIState *s, DeviceState *qdev, AddressSpace *as, int ports);
-void ahci_uninit(AHCIState *s);
+void ahci_instance_finalize(AHCIState *s);
 
 void ahci_reset(AHCIState *s);
 
diff --git a/hw/ide/ich.c b/hw/ide/ich.c
index 6c0c0c2..7c8c672 100644
--- a/hw/ide/ich.c
+++ b/hw/ide/ich.c
@@ -140,11 +140,15 @@ static int pci_ich9_ahci_init(PCIDevice *dev)
 
 static void pci_ich9_uninit(PCIDevice *dev)
 {
-    struct AHCIPCIState *d;
-    d = DO_UPCAST(struct AHCIPCIState, card, dev);
-
     msi_uninit(dev);
-    ahci_uninit(&d->ahci);
+}
+
+static void pci_ich9_instance_finalize(Object *obj)
+{
+    PCIDevice *dev = PCI_DEVICE(obj);
+    struct AHCIPCIState *d = DO_UPCAST(struct AHCIPCIState, card, dev);
+
+    ahci_instance_finalize(&d->ahci);
 }
 
 static void ich_ahci_class_init(ObjectClass *klass, void *data)
@@ -167,6 +171,7 @@ static const TypeInfo ich_ahci_info = {
     .parent        = TYPE_PCI_DEVICE,
     .instance_size = sizeof(AHCIPCIState),
     .class_init    = ich_ahci_class_init,
+    .instance_finalize = pci_ich9_instance_finalize,
 };
 
 static void ich_ahci_register_types(void)
-- 
1.8.1.4

[Qemu-devel] [PATCH 11/39] msix: split msix_free from msix_uninit

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/misc/vfio.c         |  1 +
 hw/net/vmxnet3.c       |  3 +++
 hw/pci/msix.c          | 26 ++++++++++++++++++++------
 hw/virtio/virtio-pci.c |  1 +
 include/hw/pci/msix.h  |  1 +
 5 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
index 3c0dc9f..2bcef7a 100644
--- a/hw/misc/vfio.c
+++ b/hw/misc/vfio.c
@@ -2167,6 +2167,7 @@ static void vfio_teardown_msi(VFIODevice *vdev)
     if (vdev->msix) {
         msix_uninit(&vdev->pdev, &vdev->bars[vdev->msix->table_bar].mem,
                     &vdev->bars[vdev->msix->pba_bar].mem);
+        msix_free(&vdev->pdev);
     }
 }
 
diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 5f483e7..9de2586 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -1979,6 +1979,7 @@ vmxnet3_init_msix(VMXNET3State *s)
         if (!vmxnet3_use_msix_vectors(s, VMXNET3_MAX_INTRS)) {
             VMW_WRPRN("Failed to use MSI-X vectors, error %d", res);
             msix_uninit(d, &s->msix_bar, &s->msix_bar);
+            msix_free(d);
             s->msix_used = false;
         } else {
             s->msix_used = true;
@@ -1996,6 +1997,8 @@ vmxnet3_cleanup_msix(VMXNET3State *s)
         msix_vector_unuse(d, VMXNET3_MAX_INTRS);
         msix_uninit(d, &s->msix_bar, &s->msix_bar);
     }
+
+    msix_free(d);
 }
 
 #define VMXNET3_MSI_NUM_VECTORS   (1)
diff --git a/hw/pci/msix.c b/hw/pci/msix.c
index 6da75ec..be96b06 100644
--- a/hw/pci/msix.c
+++ b/hw/pci/msix.c
@@ -274,6 +274,10 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
     dev->wmask[cap + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK |
                                              MSIX_MASKALL_MASK;
 
+    if (dev->msix_table || dev->msix_pba || dev->msix_entry_used) {
+        msix_free(dev);
+    }
+
     dev->msix_table = g_malloc0(table_size);
     dev->msix_pba = g_malloc0(pba_size);
     dev->msix_entry_used = g_malloc0(nentries * sizeof *dev->msix_entry_used);
@@ -359,16 +363,26 @@ void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar, MemoryRegion *pba_bar)
     msix_free_irq_entries(dev);
     dev->msix_entries_nr = 0;
     memory_region_del_subregion(pba_bar, &dev->msix_pba_mmio);
-    memory_region_destroy(&dev->msix_pba_mmio);
-    g_free(dev->msix_pba);
-    dev->msix_pba = NULL;
     memory_region_del_subregion(table_bar, &dev->msix_table_mmio);
-    memory_region_destroy(&dev->msix_table_mmio);
-    g_free(dev->msix_table);
+    dev->cap_present &= ~QEMU_PCI_CAP_MSIX;
+}
+
+void msix_free(PCIDevice *dev)
+{
+    if (dev->msix_pba) {
+        memory_region_destroy(&dev->msix_pba_mmio);
+        g_free(dev->msix_pba);
+    }
+    dev->msix_pba = NULL;
+
+    if (dev->msix_table) {
+        memory_region_destroy(&dev->msix_table_mmio);
+        g_free(dev->msix_table);
+    }
     dev->msix_table = NULL;
+
     g_free(dev->msix_entry_used);
     dev->msix_entry_used = NULL;
-    dev->cap_present &= ~QEMU_PCI_CAP_MSIX;
 }
 
 void msix_uninit_exclusive_bar(PCIDevice *dev)
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 444b71a..cbfec6b 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -998,6 +998,7 @@ static void virtio_pci_exit(PCIDevice *pci_dev)
     virtio_pci_stop_ioeventfd(proxy);
     memory_region_destroy(&proxy->bar);
     msix_uninit_exclusive_bar(pci_dev);
+    msix_free(pci_dev);
 }
 
 static void virtio_pci_reset(DeviceState *qdev)
diff --git a/include/hw/pci/msix.h b/include/hw/pci/msix.h
index 954d82b..34e3ba2 100644
--- a/include/hw/pci/msix.h
+++ b/include/hw/pci/msix.h
@@ -18,6 +18,7 @@ void msix_write_config(PCIDevice *dev, uint32_t address, uint32_t val, int len);
 void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar,
                  MemoryRegion *pba_bar);
 void msix_uninit_exclusive_bar(PCIDevice *dev);
+void msix_free(PCIDevice *dev);
 
 unsigned int msix_nr_vectors_allocated(const PCIDevice *dev);
 
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 11/39] msix: split msix_free from msix_uninit

[Michael S. Tsirkin]

On Tue, Jun 04, 2013 at 08:52:06PM +0200, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Please add a commit log with a bit more detail and
documenting the motivation for change.

> ---
>  hw/misc/vfio.c         |  1 +
>  hw/net/vmxnet3.c       |  3 +++
>  hw/pci/msix.c          | 26 ++++++++++++++++++++------
>  hw/virtio/virtio-pci.c |  1 +
>  include/hw/pci/msix.h  |  1 +
>  5 files changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
> index 3c0dc9f..2bcef7a 100644
> --- a/hw/misc/vfio.c
> +++ b/hw/misc/vfio.c
> @@ -2167,6 +2167,7 @@ static void vfio_teardown_msi(VFIODevice *vdev)
>      if (vdev->msix) {
>          msix_uninit(&vdev->pdev, &vdev->bars[vdev->msix->table_bar].mem,
>                      &vdev->bars[vdev->msix->pba_bar].mem);
> +        msix_free(&vdev->pdev);
>      }
>  }
>  
> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> index 5f483e7..9de2586 100644
> --- a/hw/net/vmxnet3.c
> +++ b/hw/net/vmxnet3.c
> @@ -1979,6 +1979,7 @@ vmxnet3_init_msix(VMXNET3State *s)
>          if (!vmxnet3_use_msix_vectors(s, VMXNET3_MAX_INTRS)) {
>              VMW_WRPRN("Failed to use MSI-X vectors, error %d", res);
>              msix_uninit(d, &s->msix_bar, &s->msix_bar);
> +            msix_free(d);
>              s->msix_used = false;
>          } else {
>              s->msix_used = true;
> @@ -1996,6 +1997,8 @@ vmxnet3_cleanup_msix(VMXNET3State *s)
>          msix_vector_unuse(d, VMXNET3_MAX_INTRS);
>          msix_uninit(d, &s->msix_bar, &s->msix_bar);
>      }
> +
> +    msix_free(d);
>  }
>  
>  #define VMXNET3_MSI_NUM_VECTORS   (1)
> diff --git a/hw/pci/msix.c b/hw/pci/msix.c
> index 6da75ec..be96b06 100644
> --- a/hw/pci/msix.c
> +++ b/hw/pci/msix.c
> @@ -274,6 +274,10 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries,
>      dev->wmask[cap + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK |
>                                               MSIX_MASKALL_MASK;
>  
> +    if (dev->msix_table || dev->msix_pba || dev->msix_entry_used) {
> +        msix_free(dev);
> +    }
> +
>      dev->msix_table = g_malloc0(table_size);
>      dev->msix_pba = g_malloc0(pba_size);
>      dev->msix_entry_used = g_malloc0(nentries * sizeof *dev->msix_entry_used);

Wow msix_init calls msix_free, and not on error path?
What's going on here?

> @@ -359,16 +363,26 @@ void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar, MemoryRegion *pba_bar)
>      msix_free_irq_entries(dev);
>      dev->msix_entries_nr = 0;
>      memory_region_del_subregion(pba_bar, &dev->msix_pba_mmio);
> -    memory_region_destroy(&dev->msix_pba_mmio);
> -    g_free(dev->msix_pba);
> -    dev->msix_pba = NULL;
>      memory_region_del_subregion(table_bar, &dev->msix_table_mmio);
> -    memory_region_destroy(&dev->msix_table_mmio);
> -    g_free(dev->msix_table);
> +    dev->cap_present &= ~QEMU_PCI_CAP_MSIX;
> +}
> +
> +void msix_free(PCIDevice *dev)
> +{
> +    if (dev->msix_pba) {
> +        memory_region_destroy(&dev->msix_pba_mmio);
> +        g_free(dev->msix_pba);
> +    }
> +    dev->msix_pba = NULL;
> +
> +    if (dev->msix_table) {
> +        memory_region_destroy(&dev->msix_table_mmio);
> +        g_free(dev->msix_table);
> +    }
>      dev->msix_table = NULL;
> +
>      g_free(dev->msix_entry_used);
>      dev->msix_entry_used = NULL;
> -    dev->cap_present &= ~QEMU_PCI_CAP_MSIX;
>  }
>  
>  void msix_uninit_exclusive_bar(PCIDevice *dev)

As long as we had init and uninit, it was mostly
self-documenting.
Now, there are two cleanup functions, so please add documentation.


> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 444b71a..cbfec6b 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -998,6 +998,7 @@ static void virtio_pci_exit(PCIDevice *pci_dev)
>      virtio_pci_stop_ioeventfd(proxy);
>      memory_region_destroy(&proxy->bar);
>      msix_uninit_exclusive_bar(pci_dev);
> +    msix_free(pci_dev);
>  }
>  
>  static void virtio_pci_reset(DeviceState *qdev)
> diff --git a/include/hw/pci/msix.h b/include/hw/pci/msix.h
> index 954d82b..34e3ba2 100644
> --- a/include/hw/pci/msix.h
> +++ b/include/hw/pci/msix.h
> @@ -18,6 +18,7 @@ void msix_write_config(PCIDevice *dev, uint32_t address, uint32_t val, int len);
>  void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar,
>                   MemoryRegion *pba_bar);
>  void msix_uninit_exclusive_bar(PCIDevice *dev);
> +void msix_free(PCIDevice *dev);
>  
>  unsigned int msix_nr_vectors_allocated(const PCIDevice *dev);
>  
> -- 
> 1.8.1.4
> 

Re: [Qemu-devel] [PATCH 11/39] msix: split msix_free from msix_uninit

[Paolo Bonzini]

Il 05/06/2013 00:03, Michael S. Tsirkin ha scritto:
>> > +    if (dev->msix_table || dev->msix_pba || dev->msix_entry_used) {
>> > +        msix_free(dev);
>> > +    }
>> > +
>> >      dev->msix_table = g_malloc0(table_size);
>> >      dev->msix_pba = g_malloc0(pba_size);
>> >      dev->msix_entry_used = g_malloc0(nentries * sizeof *dev->msix_entry_used);
> Wow msix_init calls msix_free, and not on error path?
> What's going on here?

I wasn't too sure that you could get here only with NULL
msix_table/pba/entry_used and wanted to protect against leaks.  I'll
change it to an assertion.

>> > @@ -359,16 +363,26 @@ void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar, MemoryRegion *pba_bar)
>> >      msix_free_irq_entries(dev);
>> >      dev->msix_entries_nr = 0;
>> >      memory_region_del_subregion(pba_bar, &dev->msix_pba_mmio);
>> > -    memory_region_destroy(&dev->msix_pba_mmio);
>> > -    g_free(dev->msix_pba);
>> > -    dev->msix_pba = NULL;
>> >      memory_region_del_subregion(table_bar, &dev->msix_table_mmio);
>> > -    memory_region_destroy(&dev->msix_table_mmio);
>> > -    g_free(dev->msix_table);
>> > +    dev->cap_present &= ~QEMU_PCI_CAP_MSIX;
>> > +}
>> > +
>> > +void msix_free(PCIDevice *dev)
>> > +{
>> > +    if (dev->msix_pba) {
>> > +        memory_region_destroy(&dev->msix_pba_mmio);
>> > +        g_free(dev->msix_pba);
>> > +    }
>> > +    dev->msix_pba = NULL;
>> > +
>> > +    if (dev->msix_table) {
>> > +        memory_region_destroy(&dev->msix_table_mmio);
>> > +        g_free(dev->msix_table);
>> > +    }
>> >      dev->msix_table = NULL;
>> > +
>> >      g_free(dev->msix_entry_used);
>> >      dev->msix_entry_used = NULL;
>> > -    dev->cap_present &= ~QEMU_PCI_CAP_MSIX;
>> >  }
>> >  
>> >  void msix_uninit_exclusive_bar(PCIDevice *dev)
> As long as we had init and uninit, it was mostly
> self-documenting.
> Now, there are two cleanup functions, so please add documentation.

Yes, will do.

Paolo

Re: [Qemu-devel] [PATCH 11/39] msix: split msix_free from msix_uninit

[Michael S. Tsirkin]

On Wed, Jun 05, 2013 at 12:40:00AM +0200, Paolo Bonzini wrote:
> Il 05/06/2013 00:03, Michael S. Tsirkin ha scritto:
> >> > +    if (dev->msix_table || dev->msix_pba || dev->msix_entry_used) {
> >> > +        msix_free(dev);
> >> > +    }
> >> > +
> >> >      dev->msix_table = g_malloc0(table_size);
> >> >      dev->msix_pba = g_malloc0(pba_size);
> >> >      dev->msix_entry_used = g_malloc0(nentries * sizeof *dev->msix_entry_used);
> > Wow msix_init calls msix_free, and not on error path?
> > What's going on here?
> 
> I wasn't too sure that you could get here only with NULL
> msix_table/pba/entry_used and wanted to protect against leaks.  I'll
> change it to an assertion.

I don't think we should require users allocate all memory with g_malloc0.
So no assertion either.
If there's a leak there was always a leak, let's focus on the
API change in this series, OK?

> >> > @@ -359,16 +363,26 @@ void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar, MemoryRegion *pba_bar)
> >> >      msix_free_irq_entries(dev);
> >> >      dev->msix_entries_nr = 0;
> >> >      memory_region_del_subregion(pba_bar, &dev->msix_pba_mmio);
> >> > -    memory_region_destroy(&dev->msix_pba_mmio);
> >> > -    g_free(dev->msix_pba);
> >> > -    dev->msix_pba = NULL;
> >> >      memory_region_del_subregion(table_bar, &dev->msix_table_mmio);
> >> > -    memory_region_destroy(&dev->msix_table_mmio);
> >> > -    g_free(dev->msix_table);
> >> > +    dev->cap_present &= ~QEMU_PCI_CAP_MSIX;
> >> > +}
> >> > +
> >> > +void msix_free(PCIDevice *dev)
> >> > +{
> >> > +    if (dev->msix_pba) {
> >> > +        memory_region_destroy(&dev->msix_pba_mmio);
> >> > +        g_free(dev->msix_pba);
> >> > +    }
> >> > +    dev->msix_pba = NULL;
> >> > +
> >> > +    if (dev->msix_table) {
> >> > +        memory_region_destroy(&dev->msix_table_mmio);
> >> > +        g_free(dev->msix_table);
> >> > +    }
> >> >      dev->msix_table = NULL;
> >> > +
> >> >      g_free(dev->msix_entry_used);
> >> >      dev->msix_entry_used = NULL;
> >> > -    dev->cap_present &= ~QEMU_PCI_CAP_MSIX;
> >> >  }
> >> >  
> >> >  void msix_uninit_exclusive_bar(PCIDevice *dev)
> > As long as we had init and uninit, it was mostly
> > self-documenting.
> > Now, there are two cleanup functions, so please add documentation.
> 
> Yes, will do.
> 
> Paolo

Re: [Qemu-devel] [PATCH 11/39] msix: split msix_free from msix_uninit

[Paolo Bonzini]

Il 05/06/2013 06:53, Michael S. Tsirkin ha scritto:
> On Wed, Jun 05, 2013 at 12:40:00AM +0200, Paolo Bonzini wrote:
>> Il 05/06/2013 00:03, Michael S. Tsirkin ha scritto:
>>>>> +    if (dev->msix_table || dev->msix_pba || dev->msix_entry_used) {
>>>>> +        msix_free(dev);
>>>>> +    }
>>>>> +
>>>>>      dev->msix_table = g_malloc0(table_size);
>>>>>      dev->msix_pba = g_malloc0(pba_size);
>>>>>      dev->msix_entry_used = g_malloc0(nentries * sizeof *dev->msix_entry_used);
>>> Wow msix_init calls msix_free, and not on error path?
>>> What's going on here?
>>
>> I wasn't too sure that you could get here only with NULL
>> msix_table/pba/entry_used and wanted to protect against leaks.  I'll
>> change it to an assertion.
> 
> I don't think we should require users allocate all memory with g_malloc0.
> So no assertion either.

Assertion that is is NULL, followed by g_malloc0?

> If there's a leak there was always a leak

No, there wasn't because msix_uninit would have freed the memory.  That is,

    msix_init
    msix_uninit
    msix_init
    msix_uninit

had no leak.  Instead, now msix_free is going to be called just once,
right before freeing the object itself:

    msix_init
    msix_uninit
    msix_init     ***
    msix_uninit
    msix_free

and will have a leak at ***.  I don't think this can happen, unrealize
should never be followed by another realize right now, but perhaps in
the future it will be if we implement something like "device_poweroff"
and "device_poweron".

Paolo

, let's focus on the
> API change in this series, OK?
> 
>>>>> @@ -359,16 +363,26 @@ void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar, MemoryRegion *pba_bar)
>>>>>      msix_free_irq_entries(dev);
>>>>>      dev->msix_entries_nr = 0;
>>>>>      memory_region_del_subregion(pba_bar, &dev->msix_pba_mmio);
>>>>> -    memory_region_destroy(&dev->msix_pba_mmio);
>>>>> -    g_free(dev->msix_pba);
>>>>> -    dev->msix_pba = NULL;
>>>>>      memory_region_del_subregion(table_bar, &dev->msix_table_mmio);
>>>>> -    memory_region_destroy(&dev->msix_table_mmio);
>>>>> -    g_free(dev->msix_table);
>>>>> +    dev->cap_present &= ~QEMU_PCI_CAP_MSIX;
>>>>> +}
>>>>> +
>>>>> +void msix_free(PCIDevice *dev)
>>>>> +{
>>>>> +    if (dev->msix_pba) {
>>>>> +        memory_region_destroy(&dev->msix_pba_mmio);
>>>>> +        g_free(dev->msix_pba);
>>>>> +    }
>>>>> +    dev->msix_pba = NULL;
>>>>> +
>>>>> +    if (dev->msix_table) {
>>>>> +        memory_region_destroy(&dev->msix_table_mmio);
>>>>> +        g_free(dev->msix_table);
>>>>> +    }
>>>>>      dev->msix_table = NULL;
>>>>> +
>>>>>      g_free(dev->msix_entry_used);
>>>>>      dev->msix_entry_used = NULL;
>>>>> -    dev->cap_present &= ~QEMU_PCI_CAP_MSIX;
>>>>>  }
>>>>>  
>>>>>  void msix_uninit_exclusive_bar(PCIDevice *dev)
>>> As long as we had init and uninit, it was mostly
>>> self-documenting.
>>> Now, there are two cleanup functions, so please add documentation.
>>
>> Yes, will do.
>>
>> Paolo
> 
> 

Re: [Qemu-devel] [PATCH 11/39] msix: split msix_free from msix_uninit

[Michael S. Tsirkin]

On Wed, Jun 05, 2013 at 09:48:19AM +0200, Paolo Bonzini wrote:
> Il 05/06/2013 06:53, Michael S. Tsirkin ha scritto:
> > On Wed, Jun 05, 2013 at 12:40:00AM +0200, Paolo Bonzini wrote:
> >> Il 05/06/2013 00:03, Michael S. Tsirkin ha scritto:
> >>>>> +    if (dev->msix_table || dev->msix_pba || dev->msix_entry_used) {
> >>>>> +        msix_free(dev);
> >>>>> +    }
> >>>>> +
> >>>>>      dev->msix_table = g_malloc0(table_size);
> >>>>>      dev->msix_pba = g_malloc0(pba_size);
> >>>>>      dev->msix_entry_used = g_malloc0(nentries * sizeof *dev->msix_entry_used);
> >>> Wow msix_init calls msix_free, and not on error path?
> >>> What's going on here?
> >>
> >> I wasn't too sure that you could get here only with NULL
> >> msix_table/pba/entry_used and wanted to protect against leaks.  I'll
> >> change it to an assertion.
> > 
> > I don't think we should require users allocate all memory with g_malloc0.
> > So no assertion either.
> 
> Assertion that is is NULL, followed by g_malloc0?

No because who sets it to NULL the first time?
msix_init just started.

> > If there's a leak there was always a leak
> 
> No, there wasn't because msix_uninit would have freed the memory.  That is,
> 
>     msix_init
>     msix_uninit
>     msix_init
>     msix_uninit
> 
> had no leak.  Instead, now msix_free is going to be called just once,
> right before freeing the object itself:
> 
>     msix_init
>     msix_uninit
>     msix_init     ***
>     msix_uninit
>     msix_free
> 
> and will have a leak at ***.

Yes. And this looks completely sane from outside,
so this is a bad API.
The way to fix it is not with asserts in code, we need a good API:
alloc/free init/uninit ...

The problem apparently starts in generic code, let's fix it there?

>  I don't think this can happen, unrealize
> should never be followed by another realize right now,

This is not an msix specific problem, I don't think msix should
debug generic core - this will just lead to proliferation of asserts.

This really should be documented prominently in generic code.

Also how about some asserts in generic code making sure ordering
is sane?


> but perhaps in
> the future it will be if we implement something like "device_poweroff"
> and "device_poweron".
> 
> Paolo

> , let's focus on the
> > API change in this series, OK?
> > 
> >>>>> @@ -359,16 +363,26 @@ void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar, MemoryRegion *pba_bar)
> >>>>>      msix_free_irq_entries(dev);
> >>>>>      dev->msix_entries_nr = 0;
> >>>>>      memory_region_del_subregion(pba_bar, &dev->msix_pba_mmio);
> >>>>> -    memory_region_destroy(&dev->msix_pba_mmio);
> >>>>> -    g_free(dev->msix_pba);
> >>>>> -    dev->msix_pba = NULL;
> >>>>>      memory_region_del_subregion(table_bar, &dev->msix_table_mmio);
> >>>>> -    memory_region_destroy(&dev->msix_table_mmio);
> >>>>> -    g_free(dev->msix_table);
> >>>>> +    dev->cap_present &= ~QEMU_PCI_CAP_MSIX;
> >>>>> +}
> >>>>> +
> >>>>> +void msix_free(PCIDevice *dev)
> >>>>> +{
> >>>>> +    if (dev->msix_pba) {
> >>>>> +        memory_region_destroy(&dev->msix_pba_mmio);
> >>>>> +        g_free(dev->msix_pba);
> >>>>> +    }
> >>>>> +    dev->msix_pba = NULL;
> >>>>> +
> >>>>> +    if (dev->msix_table) {
> >>>>> +        memory_region_destroy(&dev->msix_table_mmio);
> >>>>> +        g_free(dev->msix_table);
> >>>>> +    }
> >>>>>      dev->msix_table = NULL;
> >>>>> +
> >>>>>      g_free(dev->msix_entry_used);
> >>>>>      dev->msix_entry_used = NULL;
> >>>>> -    dev->cap_present &= ~QEMU_PCI_CAP_MSIX;
> >>>>>  }
> >>>>>  
> >>>>>  void msix_uninit_exclusive_bar(PCIDevice *dev)
> >>> As long as we had init and uninit, it was mostly
> >>> self-documenting.
> >>> Now, there are two cleanup functions, so please add documentation.
> >>
> >> Yes, will do.
> >>
> >> Paolo
> > 
> > 

Re: [Qemu-devel] [PATCH 11/39] msix: split msix_free from msix_uninit

[Paolo Bonzini]

Il 05/06/2013 06:32, Michael S. Tsirkin ha scritto:
> On Wed, Jun 05, 2013 at 09:48:19AM +0200, Paolo Bonzini wrote:
>> Il 05/06/2013 06:53, Michael S. Tsirkin ha scritto:
>>> On Wed, Jun 05, 2013 at 12:40:00AM +0200, Paolo Bonzini wrote:
>>>> Il 05/06/2013 00:03, Michael S. Tsirkin ha scritto:
>>>>>>> +    if (dev->msix_table || dev->msix_pba || dev->msix_entry_used) {
>>>>>>> +        msix_free(dev);
>>>>>>> +    }
>>>>>>> +
>>>>>>>      dev->msix_table = g_malloc0(table_size);
>>>>>>>      dev->msix_pba = g_malloc0(pba_size);
>>>>>>>      dev->msix_entry_used = g_malloc0(nentries * sizeof *dev->msix_entry_used);
>>>>> Wow msix_init calls msix_free, and not on error path?
>>>>> What's going on here?
>>>>
>>>> I wasn't too sure that you could get here only with NULL
>>>> msix_table/pba/entry_used and wanted to protect against leaks.  I'll
>>>> change it to an assertion.
>>>
>>> I don't think we should require users allocate all memory with g_malloc0.
>>> So no assertion either.
>>
>> Assertion that is is NULL, followed by g_malloc0?
> 
> No because who sets it to NULL the first time?
> msix_init just started.

When an object is created, it is all-zeroed.

>>> If there's a leak there was always a leak
>>
>> No, there wasn't because msix_uninit would have freed the memory.  That is,
>>
>>     msix_init
>>     msix_uninit
>>     msix_init
>>     msix_uninit
>>
>> had no leak.  Instead, now msix_free is going to be called just once,
>> right before freeing the object itself:
>>
>>     msix_init
>>     msix_uninit
>>     msix_init     ***
>>     msix_uninit
>>     msix_free
>>
>> and will have a leak at ***.
> 
> Yes. And this looks completely sane from outside,
> so this is a bad API.
> The way to fix it is not with asserts in code, we need a good API:
> alloc/free init/uninit ...

Can't, because table_size/pba_size is not available at init time (e.g.
for VFIO not until the host BARs are processed).  What about using
g_realloc + memset?

Paolo

[Qemu-devel] [PATCH 12/39] cmd646: use instance_finalize instead of exit

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/ide/cmd646.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index a73eb9a..296dee6 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -295,8 +295,9 @@ static int pci_cmd646_ide_initfn(PCIDevice *dev)
     return 0;
 }
 
-static void pci_cmd646_ide_exitfn(PCIDevice *dev)
+static void pci_cmd646_ide_instance_finalize(Object *obj)
 {
+    PCIDevice *dev = PCI_DEVICE(obj);
     PCIIDEState *d = DO_UPCAST(PCIIDEState, dev, dev);
     unsigned i;
 
@@ -334,7 +335,6 @@ static void cmd646_ide_class_init(ObjectClass *klass, void *data)
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
     k->init = pci_cmd646_ide_initfn;
-    k->exit = pci_cmd646_ide_exitfn;
     k->vendor_id = PCI_VENDOR_ID_CMD;
     k->device_id = PCI_DEVICE_ID_CMD_646;
     k->revision = 0x07;
@@ -347,6 +347,7 @@ static const TypeInfo cmd646_ide_info = {
     .parent        = TYPE_PCI_DEVICE,
     .instance_size = sizeof(PCIIDEState),
     .class_init    = cmd646_ide_class_init,
+    .instance_finalize = pci_cmd646_ide_instance_finalize,
 };
 
 static void cmd646_ide_register_types(void)
-- 
1.8.1.4

[Qemu-devel] [PATCH 13/39] ide/piix: use instance_finalize instead of exit

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/ide/piix.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index bf2856f..a0550c6 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -201,8 +201,9 @@ PCIDevice *pci_piix3_xen_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn)
     return dev;
 }
 
-static void pci_piix_ide_exitfn(PCIDevice *dev)
+static void pci_piix_ide_instance_finalize(Object *obj)
 {
+    PCIDevice *dev = PCI_DEVICE(obj);
     PCIIDEState *d = DO_UPCAST(PCIIDEState, dev, dev);
     unsigned i;
 
@@ -244,7 +245,6 @@ static void piix3_ide_class_init(ObjectClass *klass, void *data)
 
     k->no_hotplug = 1;
     k->init = pci_piix_ide_initfn;
-    k->exit = pci_piix_ide_exitfn;
     k->vendor_id = PCI_VENDOR_ID_INTEL;
     k->device_id = PCI_DEVICE_ID_INTEL_82371SB_1;
     k->class_id = PCI_CLASS_STORAGE_IDE;
@@ -256,6 +256,7 @@ static const TypeInfo piix3_ide_info = {
     .parent        = TYPE_PCI_DEVICE,
     .instance_size = sizeof(PCIIDEState),
     .class_init    = piix3_ide_class_init,
+    .instance_finalize = pci_piix_ide_instance_finalize,
 };
 
 static void piix3_ide_xen_class_init(ObjectClass *klass, void *data)
@@ -276,6 +277,7 @@ static const TypeInfo piix3_ide_xen_info = {
     .parent        = TYPE_PCI_DEVICE,
     .instance_size = sizeof(PCIIDEState),
     .class_init    = piix3_ide_xen_class_init,
+    .instance_finalize = pci_piix_ide_instance_finalize,
 };
 
 static void piix4_ide_class_init(ObjectClass *klass, void *data)
@@ -285,7 +287,6 @@ static void piix4_ide_class_init(ObjectClass *klass, void *data)
 
     k->no_hotplug = 1;
     k->init = pci_piix_ide_initfn;
-    k->exit = pci_piix_ide_exitfn;
     k->vendor_id = PCI_VENDOR_ID_INTEL;
     k->device_id = PCI_DEVICE_ID_INTEL_82371AB;
     k->class_id = PCI_CLASS_STORAGE_IDE;
@@ -297,6 +298,7 @@ static const TypeInfo piix4_ide_info = {
     .parent        = TYPE_PCI_DEVICE,
     .instance_size = sizeof(PCIIDEState),
     .class_init    = piix4_ide_class_init,
+    .instance_finalize = pci_piix_ide_instance_finalize,
 };
 
 static void piix_ide_register_types(void)
-- 
1.8.1.4

[Qemu-devel] [PATCH 14/39] ide/via: use instance_finalize instead of exit

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/ide/via.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/ide/via.c b/hw/ide/via.c
index 5fe053c..58070df 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -190,8 +190,9 @@ static int vt82c686b_ide_initfn(PCIDevice *dev)
     return 0;
 }
 
-static void vt82c686b_ide_exitfn(PCIDevice *dev)
+static void vt82c686b_ide_instance_finalize(Object *obj)
 {
+    PCIDevice *dev = PCI_DEVICE(obj);
     PCIIDEState *d = DO_UPCAST(PCIIDEState, dev, dev);
     unsigned i;
 
@@ -218,7 +219,6 @@ static void via_ide_class_init(ObjectClass *klass, void *data)
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
     k->init = vt82c686b_ide_initfn;
-    k->exit = vt82c686b_ide_exitfn;
     k->vendor_id = PCI_VENDOR_ID_VIA;
     k->device_id = PCI_DEVICE_ID_VIA_IDE;
     k->revision = 0x06;
@@ -231,6 +231,7 @@ static const TypeInfo via_ide_info = {
     .parent        = TYPE_PCI_DEVICE,
     .instance_size = sizeof(PCIIDEState),
     .class_init    = via_ide_class_init,
+    .instance_finalize = vt82c686b_ide_instance_finalize,
 };
 
 static void via_ide_register_types(void)
-- 
1.8.1.4

[Qemu-devel] [PATCH 15/39] ivshmem: split exit and instance_finalize

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/misc/ivshmem.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index a19a6d6..d4af440 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -772,13 +772,20 @@ static void pci_ivshmem_uninit(PCIDevice *dev)
 {
     IVShmemState *s = DO_UPCAST(IVShmemState, dev, dev);
 
+    memory_region_del_subregion(&s->bar, &s->ivshmem);
+}
+
+static void pci_ivshmem_instance_finalize(Object *obj)
+{
+    PCIDevice *dev = PCI_DEVICE(obj);
+    IVShmemState *s = DO_UPCAST(IVShmemState, dev, dev);
+
     if (s->migration_blocker) {
         migrate_del_blocker(s->migration_blocker);
         error_free(s->migration_blocker);
     }
 
     memory_region_destroy(&s->ivshmem_mmio);
-    memory_region_del_subregion(&s->bar, &s->ivshmem);
     vmstate_unregister_ram(&s->ivshmem, &s->dev.qdev);
     memory_region_destroy(&s->ivshmem);
     memory_region_destroy(&s->bar);
@@ -816,6 +823,7 @@ static const TypeInfo ivshmem_info = {
     .parent        = TYPE_PCI_DEVICE,
     .instance_size = sizeof(IVShmemState),
     .class_init    = ivshmem_class_init,
+    .instance_finalize = pci_ivshmem_instance_finalize,
 };
 
 static void ivshmem_register_types(void)
-- 
1.8.1.4

[Qemu-devel] [PATCH 16/39] pci-testdev: use instance_finalize instead of exit

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/misc/pci-testdev.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/misc/pci-testdev.c b/hw/misc/pci-testdev.c
index 71ce5a3..731748a 100644
--- a/hw/misc/pci-testdev.c
+++ b/hw/misc/pci-testdev.c
@@ -272,8 +272,9 @@ static int pci_testdev_init(PCIDevice *pci_dev)
 }
 
 static void
-pci_testdev_uninit(PCIDevice *dev)
+pci_testdev_instance_finalize(Object *obj)
 {
+    PCIDevice *dev = PCI_DEVICE(obj);
     PCITestDevState *d = DO_UPCAST(PCITestDevState, dev, dev);
     int i;
 
@@ -301,7 +302,6 @@ static void pci_testdev_class_init(ObjectClass *klass, void *data)
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
     k->init = pci_testdev_init;
-    k->exit = pci_testdev_uninit;
     k->vendor_id = PCI_VENDOR_ID_REDHAT;
     k->device_id = PCI_DEVICE_ID_REDHAT_TEST;
     k->revision = 0x00;
@@ -315,6 +315,7 @@ static const TypeInfo pci_testdev_info = {
     .parent        = TYPE_PCI_DEVICE,
     .instance_size = sizeof(PCITestDevState),
     .class_init    = pci_testdev_class_init,
+    .instance_finalize = pci_testdev_instance_finalize,
 };
 
 static void pci_testdev_register_types(void)
-- 
1.8.1.4

[Qemu-devel] [PATCH 17/39] vfio: split exit and instance_finalize

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/misc/vfio.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 47 insertions(+), 4 deletions(-)

diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
index 2bcef7a..c9c68fa 100644
--- a/hw/misc/vfio.c
+++ b/hw/misc/vfio.c
@@ -2167,6 +2167,12 @@ static void vfio_teardown_msi(VFIODevice *vdev)
     if (vdev->msix) {
         msix_uninit(&vdev->pdev, &vdev->bars[vdev->msix->table_bar].mem,
                     &vdev->bars[vdev->msix->pba_bar].mem);
+    }
+}
+
+static void vfio_destroy_msi(VFIODevice *vdev)
+{
+    if (vdev->msix) {
         msix_free(&vdev->pdev);
     }
 }
@@ -2203,10 +2209,23 @@ static void vfio_unmap_bar(VFIODevice *vdev, int nr)
     vfio_bar_quirk_teardown(vdev, nr);
 
     memory_region_del_subregion(&bar->mem, &bar->mmap_mem);
-    munmap(bar->mmap, memory_region_size(&bar->mmap_mem));
 
     if (vdev->msix && vdev->msix->table_bar == nr) {
         memory_region_del_subregion(&bar->mem, &vdev->msix->mmap_mem);
+    }
+}
+
+static void vfio_destroy_bar(VFIODevice *vdev, int nr)
+{
+    VFIOBAR *bar = &vdev->bars[nr];
+
+    if (!bar->size) {
+        return;
+    }
+
+    munmap(bar->mmap, memory_region_size(&bar->mmap_mem));
+
+    if (vdev->msix && vdev->msix->table_bar == nr) {
         munmap(vdev->msix->mmap, memory_region_size(&vdev->msix->mmap_mem));
     }
 
@@ -2360,6 +2379,18 @@ static void vfio_unmap_bars(VFIODevice *vdev)
     if (vdev->has_vga) {
         vfio_vga_quirk_teardown(vdev);
         pci_unregister_vga(&vdev->pdev);
+    }
+}
+
+static void vfio_destroy_bars(VFIODevice *vdev)
+{
+    int i;
+
+    for (i = 0; i < PCI_ROM_SLOT; i++) {
+        vfio_destroy_bar(vdev, i);
+    }
+
+    if (vdev->has_vga) {
         memory_region_destroy(&vdev->vga.region[QEMU_PCI_VGA_MEM].mem);
         memory_region_destroy(&vdev->vga.region[QEMU_PCI_VGA_IO_LO].mem);
         memory_region_destroy(&vdev->vga.region[QEMU_PCI_VGA_IO_HI].mem);
@@ -3092,7 +3123,9 @@ static int vfio_initfn(PCIDevice *pdev)
 out_teardown:
     pci_device_set_intx_routing_notifier(&vdev->pdev, NULL);
     vfio_teardown_msi(vdev);
+    vfio_destroy_msi(vdev);
     vfio_unmap_bars(vdev);
+    vfio_destroy_bars(vdev);
 out_put:
     g_free(vdev->emulated_config_bits);
     vfio_put_device(vdev);
@@ -3103,15 +3136,24 @@ out_put:
 static void vfio_exitfn(PCIDevice *pdev)
 {
     VFIODevice *vdev = DO_UPCAST(VFIODevice, pdev, pdev);
-    VFIOGroup *group = vdev->group;
 
     pci_device_set_intx_routing_notifier(&vdev->pdev, NULL);
     vfio_disable_interrupts(vdev);
+    vfio_teardown_msi(vdev);
+    vfio_unmap_bars(vdev);
+}
+
+static void vfio_instance_finalize(Object *obj)
+{
+    PCIDevice *pdev = PCI_DEVICE(obj);
+    VFIODevice *vdev = DO_UPCAST(VFIODevice, pdev, pdev);
+    VFIOGroup *group = vdev->group;
+
     if (vdev->intx.mmap_timer) {
         qemu_free_timer(vdev->intx.mmap_timer);
     }
-    vfio_teardown_msi(vdev);
-    vfio_unmap_bars(vdev);
+    vfio_destroy_msi(vdev);
+    vfio_destroy_bars(vdev);
     g_free(vdev->emulated_config_bits);
     vfio_put_device(vdev);
     vfio_put_group(group);
@@ -3209,6 +3251,7 @@ static const TypeInfo vfio_pci_dev_info = {
     .parent = TYPE_PCI_DEVICE,
     .instance_size = sizeof(VFIODevice),
     .class_init = vfio_pci_dev_class_init,
+    .instance_finalize = vfio_instance_finalize,
 };
 
 static void register_vfio_pci_dev_type(void)
-- 
1.8.1.4

[Qemu-devel] [PATCH 18/39] e1000: use instance_finalize instead of exit

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/net/e1000.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index e6f46f0..cad4fd6 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -1294,8 +1294,9 @@ e1000_cleanup(NetClientState *nc)
 }
 
 static void
-pci_e1000_uninit(PCIDevice *dev)
+pci_e1000_instance_finalize(Object *obj)
 {
+    PCIDevice *dev = PCI_DEVICE(obj);
     E1000State *d = DO_UPCAST(E1000State, dev, dev);
 
     qemu_del_timer(d->autoneg_timer);
@@ -1377,7 +1378,6 @@ static void e1000_class_init(ObjectClass *klass, void *data)
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
     k->init = pci_e1000_init;
-    k->exit = pci_e1000_uninit;
     k->romfile = "efi-e1000.rom";
     k->vendor_id = PCI_VENDOR_ID_INTEL;
     k->device_id = E1000_DEVID;
@@ -1394,6 +1394,7 @@ static const TypeInfo e1000_info = {
     .parent        = TYPE_PCI_DEVICE,
     .instance_size = sizeof(E1000State),
     .class_init    = e1000_class_init,
+    .instance_finalize = pci_e1000_instance_finalize,
 };
 
 static void e1000_register_types(void)
-- 
1.8.1.4

[Qemu-devel] [PATCH 19/39] eepro100: use instance_finalize instead of exit

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/net/eepro100.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
index dc99ea6..5f95240 100644
--- a/hw/net/eepro100.c
+++ b/hw/net/eepro100.c
@@ -1840,8 +1840,9 @@ static void nic_cleanup(NetClientState *nc)
     s->nic = NULL;
 }
 
-static void pci_nic_uninit(PCIDevice *pci_dev)
+static void pci_nic_instance_finalize(Object *obj)
 {
+    PCIDevice *pci_dev = PCI_DEVICE(obj);
     EEPRO100State *s = DO_UPCAST(EEPRO100State, dev, pci_dev);
 
     memory_region_destroy(&s->mmio_bar);
@@ -2089,7 +2090,6 @@ static void eepro100_class_init(ObjectClass *klass, void *data)
     k->class_id = PCI_CLASS_NETWORK_ETHERNET;
     k->romfile = "pxe-eepro100.rom";
     k->init = e100_nic_init;
-    k->exit = pci_nic_uninit;
     k->device_id = info->device_id;
     k->revision = info->revision;
     k->subsystem_vendor_id = info->subsystem_vendor_id;
@@ -2107,6 +2107,7 @@ static void eepro100_register_types(void)
         type_info.parent = TYPE_PCI_DEVICE;
         type_info.class_init = eepro100_class_init;
         type_info.instance_size = sizeof(EEPRO100State);
+        type_info.instance_finalize = pci_nic_instance_finalize;
         
         type_register(&type_info);
     }
-- 
1.8.1.4

[Qemu-devel] [PATCH 20/39] ne2000: use instance_finalize instead of exit

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/net/ne2000.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/net/ne2000.c b/hw/net/ne2000.c
index 33ee03e..aded2da 100644
--- a/hw/net/ne2000.c
+++ b/hw/net/ne2000.c
@@ -745,8 +745,9 @@ static int pci_ne2000_init(PCIDevice *pci_dev)
     return 0;
 }
 
-static void pci_ne2000_exit(PCIDevice *pci_dev)
+static void pci_ne2000_instance_finalize(Object *obj)
 {
+    PCIDevice *pci_dev = PCI_DEVICE(obj);
     PCINE2000State *d = DO_UPCAST(PCINE2000State, dev, pci_dev);
     NE2000State *s = &d->ne2000;
 
@@ -765,7 +766,6 @@ static void ne2000_class_init(ObjectClass *klass, void *data)
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
     k->init = pci_ne2000_init;
-    k->exit = pci_ne2000_exit;
     k->romfile = "efi-ne2k_pci.rom",
     k->vendor_id = PCI_VENDOR_ID_REALTEK;
     k->device_id = PCI_DEVICE_ID_REALTEK_8029;
@@ -779,6 +779,7 @@ static const TypeInfo ne2000_info = {
     .parent        = TYPE_PCI_DEVICE,
     .instance_size = sizeof(PCINE2000State),
     .class_init    = ne2000_class_init,
+    .instance_finalize = pci_ne2000_instance_finalize,
 };
 
 static void ne2000_register_types(void)
-- 
1.8.1.4

[Qemu-devel] [PATCH 21/39] pcnet: use instance_finalize instead of exit

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/net/pcnet-pci.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/net/pcnet-pci.c b/hw/net/pcnet-pci.c
index 9df2b87..2cd2927 100644
--- a/hw/net/pcnet-pci.c
+++ b/hw/net/pcnet-pci.c
@@ -271,8 +271,9 @@ static void pci_pcnet_cleanup(NetClientState *nc)
     pcnet_common_cleanup(d);
 }
 
-static void pci_pcnet_uninit(PCIDevice *dev)
+static void pci_pcnet_instance_finalize(Object *obj)
 {
+    PCIDevice *dev = PCI_DEVICE(obj);
     PCIPCNetState *d = DO_UPCAST(PCIPCNetState, pci_dev, dev);
 
     memory_region_destroy(&d->state.mmio);
@@ -350,7 +351,6 @@ static void pcnet_class_init(ObjectClass *klass, void *data)
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
     k->init = pci_pcnet_init;
-    k->exit = pci_pcnet_uninit;
     k->romfile = "efi-pcnet.rom",
     k->vendor_id = PCI_VENDOR_ID_AMD;
     k->device_id = PCI_DEVICE_ID_AMD_LANCE;
@@ -366,6 +366,7 @@ static const TypeInfo pcnet_info = {
     .parent        = TYPE_PCI_DEVICE,
     .instance_size = sizeof(PCIPCNetState),
     .class_init    = pcnet_class_init,
+    .instance_finalize = pci_pcnet_instance_finalize,
 };
 
 static void pci_pcnet_register_types(void)
-- 
1.8.1.4

[Qemu-devel] [PATCH 22/39] rtl8139: use instance_finalize instead of exit

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/net/rtl8139.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
index 7993f9f..c1c4921 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -3437,8 +3437,9 @@ static void rtl8139_cleanup(NetClientState *nc)
     s->nic = NULL;
 }
 
-static void pci_rtl8139_uninit(PCIDevice *dev)
+static void pci_rtl8139_instance_finalize(Object *obj)
 {
+    PCIDevice *dev = PCI_DEVICE(obj);
     RTL8139State *s = DO_UPCAST(RTL8139State, dev, dev);
 
     memory_region_destroy(&s->bar_io);
@@ -3532,7 +3533,6 @@ static void rtl8139_class_init(ObjectClass *klass, void *data)
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
     k->init = pci_rtl8139_init;
-    k->exit = pci_rtl8139_uninit;
     k->romfile = "efi-rtl8139.rom";
     k->vendor_id = PCI_VENDOR_ID_REALTEK;
     k->device_id = PCI_DEVICE_ID_REALTEK_8139;
@@ -3548,6 +3548,7 @@ static const TypeInfo rtl8139_info = {
     .parent        = TYPE_PCI_DEVICE,
     .instance_size = sizeof(RTL8139State),
     .class_init    = rtl8139_class_init,
+    .instance_finalize = pci_rtl8139_instance_finalize,
 };
 
 static void rtl8139_register_types(void)
-- 
1.8.1.4

[Qemu-devel] [PATCH 23/39] vmxnet3: split exit and instance_finalize

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/net/vmxnet3.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 9de2586..5b1b9e0 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -1979,7 +1979,6 @@ vmxnet3_init_msix(VMXNET3State *s)
         if (!vmxnet3_use_msix_vectors(s, VMXNET3_MAX_INTRS)) {
             VMW_WRPRN("Failed to use MSI-X vectors, error %d", res);
             msix_uninit(d, &s->msix_bar, &s->msix_bar);
-            msix_free(d);
             s->msix_used = false;
         } else {
             s->msix_used = true;
@@ -2124,11 +2123,18 @@ static void vmxnet3_pci_uninit(PCIDevice *pci_dev)
 
     unregister_savevm(dev, "vmxnet3-msix", s);
 
-    vmxnet3_net_uninit(s);
-
     vmxnet3_cleanup_msix(s);
 
     vmxnet3_cleanup_msi(s);
+}
+
+static void vmxnet3_pci_instance_finalize(Object *obj)
+{
+    PCIDevice *pci_dev = PCI_DEVICE(obj);
+    VMXNET3State *s = VMXNET3(pci_dev);
+
+    vmxnet3_net_uninit(s);
+    msix_free(pci_dev);
 
     memory_region_destroy(&s->bar0);
     memory_region_destroy(&s->bar1);
@@ -2463,6 +2469,7 @@ static const TypeInfo vmxnet3_info = {
     .parent        = TYPE_PCI_DEVICE,
     .instance_size = sizeof(VMXNET3State),
     .class_init    = vmxnet3_class_init,
+    .instance_finalize = vmxnet3_pci_instance_finalize,
 };
 
 static void vmxnet3_register_types(void)
-- 
1.8.1.4

[Qemu-devel] [PATCH 24/39] shpc: split shpc_free out of shpc_cleanup

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/pci-bridge/pci_bridge_dev.c | 2 ++
 hw/pci/shpc.c                  | 8 +++++++-
 include/hw/pci/shpc.h          | 1 +
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
index 971b432..39deb1f 100644
--- a/hw/pci-bridge/pci_bridge_dev.c
+++ b/hw/pci-bridge/pci_bridge_dev.c
@@ -72,6 +72,7 @@ msi_error:
     slotid_cap_cleanup(dev);
 slotid_error:
     shpc_cleanup(dev, &bridge_dev->bar);
+    shpc_free(dev);
 shpc_error:
     memory_region_destroy(&bridge_dev->bar);
     pci_bridge_exitfn(dev);
@@ -88,6 +89,7 @@ static void pci_bridge_dev_exitfn(PCIDevice *dev)
     }
     slotid_cap_cleanup(dev);
     shpc_cleanup(dev, &bridge_dev->bar);
+    shpc_free(dev);
     memory_region_destroy(&bridge_dev->bar);
     pci_bridge_exitfn(dev);
 }
diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
index d35c2ee..db88da9 100644
--- a/hw/pci/shpc.c
+++ b/hw/pci/shpc.c
@@ -630,15 +630,21 @@ int shpc_bar_size(PCIDevice *d)
 void shpc_cleanup(PCIDevice *d, MemoryRegion *bar)
 {
     SHPCDevice *shpc = d->shpc;
+    /* TODO: cleanup config space changes? */
     d->cap_present &= ~QEMU_PCI_CAP_SHPC;
     memory_region_del_subregion(bar, &shpc->mmio);
-    /* TODO: cleanup config space changes? */
+}
+
+void shpc_free(PCIDevice *d)
+{
+    SHPCDevice *shpc = d->shpc;
     g_free(shpc->config);
     g_free(shpc->cmask);
     g_free(shpc->wmask);
     g_free(shpc->w1cmask);
     memory_region_destroy(&shpc->mmio);
     g_free(shpc);
+    d->shpc = NULL;
 }
 
 void shpc_cap_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
diff --git a/include/hw/pci/shpc.h b/include/hw/pci/shpc.h
index 467911a..5f27431 100644
--- a/include/hw/pci/shpc.h
+++ b/include/hw/pci/shpc.h
@@ -39,6 +39,7 @@ void shpc_reset(PCIDevice *d);
 int shpc_bar_size(PCIDevice *dev);
 int shpc_init(PCIDevice *dev, PCIBus *sec_bus, MemoryRegion *bar, unsigned off);
 void shpc_cleanup(PCIDevice *dev, MemoryRegion *bar);
+void shpc_free(PCIDevice *d);
 void shpc_cap_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int len);
 
 extern VMStateInfo shpc_vmstate_info;
-- 
1.8.1.4

[Qemu-devel] [PATCH 25/39] pci_bridge: split pci_bridge_free from pci_bridge_exitfn

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/pci-bridge/i82801b11.c          | 1 +
 hw/pci-bridge/ioh3420.c            | 2 ++
 hw/pci-bridge/pci_bridge_dev.c     | 2 ++
 hw/pci-bridge/xio3130_downstream.c | 2 ++
 hw/pci-bridge/xio3130_upstream.c   | 2 ++
 hw/pci/pci_bridge.c                | 5 +++++
 include/hw/pci/pci_bridge.h        | 1 +
 7 files changed, 15 insertions(+)

diff --git a/hw/pci-bridge/i82801b11.c b/hw/pci-bridge/i82801b11.c
index 5807a92..f965958 100644
--- a/hw/pci-bridge/i82801b11.c
+++ b/hw/pci-bridge/i82801b11.c
@@ -74,6 +74,7 @@ static int i82801b11_bridge_initfn(PCIDevice *d)
 
 err_bridge:
     pci_bridge_exitfn(d);
+    pci_bridge_free(d);
 
     return rc;
 }
diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c
index bb541eb..74ab7c8 100644
--- a/hw/pci-bridge/ioh3420.c
+++ b/hw/pci-bridge/ioh3420.c
@@ -143,6 +143,7 @@ err_msi:
     msi_uninit(d);
 err_bridge:
     pci_bridge_exitfn(d);
+    pci_bridge_free(d);
     return rc;
 }
 
@@ -157,6 +158,7 @@ static void ioh3420_exitfn(PCIDevice *d)
     pcie_cap_exit(d);
     msi_uninit(d);
     pci_bridge_exitfn(d);
+    pci_bridge_free(d);
 }
 
 PCIESlot *ioh3420_init(PCIBus *bus, int devfn, bool multifunction,
diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
index 39deb1f..c1aab15 100644
--- a/hw/pci-bridge/pci_bridge_dev.c
+++ b/hw/pci-bridge/pci_bridge_dev.c
@@ -76,6 +76,7 @@ slotid_error:
 shpc_error:
     memory_region_destroy(&bridge_dev->bar);
     pci_bridge_exitfn(dev);
+    pci_bridge_free(dev);
 bridge_error:
     return err;
 }
@@ -92,6 +93,7 @@ static void pci_bridge_dev_exitfn(PCIDevice *dev)
     shpc_free(dev);
     memory_region_destroy(&bridge_dev->bar);
     pci_bridge_exitfn(dev);
+    pci_bridge_free(dev);
 }
 
 static void pci_bridge_dev_write_config(PCIDevice *d,
diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
index 1810dd2..57af725 100644
--- a/hw/pci-bridge/xio3130_downstream.c
+++ b/hw/pci-bridge/xio3130_downstream.c
@@ -108,6 +108,7 @@ err_msi:
     msi_uninit(d);
 err_bridge:
     pci_bridge_exitfn(d);
+    pci_bridge_free(d);
     return rc;
 }
 
@@ -122,6 +123,7 @@ static void xio3130_downstream_exitfn(PCIDevice *d)
     pcie_cap_exit(d);
     msi_uninit(d);
     pci_bridge_exitfn(d);
+    pci_bridge_free(d);
 }
 
 PCIESlot *xio3130_downstream_init(PCIBus *bus, int devfn, bool multifunction,
diff --git a/hw/pci-bridge/xio3130_upstream.c b/hw/pci-bridge/xio3130_upstream.c
index 8e0d97a..58c3d29 100644
--- a/hw/pci-bridge/xio3130_upstream.c
+++ b/hw/pci-bridge/xio3130_upstream.c
@@ -95,6 +95,7 @@ err_msi:
     msi_uninit(d);
 err_bridge:
     pci_bridge_exitfn(d);
+    pci_bridge_free(d);
     return rc;
 }
 
@@ -104,6 +105,7 @@ static void xio3130_upstream_exitfn(PCIDevice *d)
     pcie_cap_exit(d);
     msi_uninit(d);
     pci_bridge_exitfn(d);
+    pci_bridge_free(d);
 }
 
 PCIEPort *xio3130_upstream_init(PCIBus *bus, int devfn, bool multifunction,
diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
index 24be6c5..4191729 100644
--- a/hw/pci/pci_bridge.c
+++ b/hw/pci/pci_bridge.c
@@ -383,6 +383,11 @@ void pci_bridge_exitfn(PCIDevice *pci_dev)
     assert(QLIST_EMPTY(&s->sec_bus.child));
     QLIST_REMOVE(&s->sec_bus, sibling);
     pci_bridge_region_del(s, s->windows);
+}
+
+void pci_bridge_free(PCIDevice *pci_dev)
+{
+    PCIBridge *s = DO_UPCAST(PCIBridge, dev, pci_dev);
     pci_bridge_region_cleanup(s, s->windows);
     memory_region_destroy(&s->address_space_mem);
     memory_region_destroy(&s->address_space_io);
diff --git a/include/hw/pci/pci_bridge.h b/include/hw/pci/pci_bridge.h
index 1868f7a..99ba0eb 100644
--- a/include/hw/pci/pci_bridge.h
+++ b/include/hw/pci/pci_bridge.h
@@ -45,6 +45,7 @@ void pci_bridge_reset(DeviceState *qdev);
 
 int pci_bridge_initfn(PCIDevice *pci_dev, const char *typename);
 void pci_bridge_exitfn(PCIDevice *pci_dev);
+void pci_bridge_free(PCIDevice *pci_dev);
 
 
 /*
-- 
1.8.1.4

[Qemu-devel] [PATCH 26/39] pcie_aer: pcie_aer_exit really frees stuff

[Paolo Bonzini]

Rename it, and move its call towards the end to prepare for
unrealize/instance_finalize split.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/pci-bridge/ioh3420.c            | 2 +-
 hw/pci-bridge/xio3130_downstream.c | 2 +-
 hw/pci-bridge/xio3130_upstream.c   | 2 +-
 hw/pci/pcie_aer.c                  | 3 ++-
 include/hw/pci/pcie_aer.h          | 2 +-
 5 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c
index 74ab7c8..0a3cbb7 100644
--- a/hw/pci-bridge/ioh3420.c
+++ b/hw/pci-bridge/ioh3420.c
@@ -153,11 +153,11 @@ static void ioh3420_exitfn(PCIDevice *d)
     PCIEPort *p = DO_UPCAST(PCIEPort, br, br);
     PCIESlot *s = DO_UPCAST(PCIESlot, port, p);
 
-    pcie_aer_exit(d);
     pcie_chassis_del_slot(s);
     pcie_cap_exit(d);
     msi_uninit(d);
     pci_bridge_exitfn(d);
+    pcie_aer_free(d);
     pci_bridge_free(d);
 }
 
diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
index 57af725..6ff336f 100644
--- a/hw/pci-bridge/xio3130_downstream.c
+++ b/hw/pci-bridge/xio3130_downstream.c
@@ -118,11 +118,11 @@ static void xio3130_downstream_exitfn(PCIDevice *d)
     PCIEPort *p = DO_UPCAST(PCIEPort, br, br);
     PCIESlot *s = DO_UPCAST(PCIESlot, port, p);
 
-    pcie_aer_exit(d);
     pcie_chassis_del_slot(s);
     pcie_cap_exit(d);
     msi_uninit(d);
     pci_bridge_exitfn(d);
+    pcie_aer_free(d);
     pci_bridge_free(d);
 }
 
diff --git a/hw/pci-bridge/xio3130_upstream.c b/hw/pci-bridge/xio3130_upstream.c
index 58c3d29..9485f5e 100644
--- a/hw/pci-bridge/xio3130_upstream.c
+++ b/hw/pci-bridge/xio3130_upstream.c
@@ -101,10 +101,10 @@ err_bridge:
 
 static void xio3130_upstream_exitfn(PCIDevice *d)
 {
-    pcie_aer_exit(d);
     pcie_cap_exit(d);
     msi_uninit(d);
     pci_bridge_exitfn(d);
+    pcie_aer_free(d);
     pci_bridge_free(d);
 }
 
diff --git a/hw/pci/pcie_aer.c b/hw/pci/pcie_aer.c
index 1ce72ce..7db4e8e 100644
--- a/hw/pci/pcie_aer.c
+++ b/hw/pci/pcie_aer.c
@@ -163,9 +163,10 @@ int pcie_aer_init(PCIDevice *dev, uint16_t offset)
     return 0;
 }
 
-void pcie_aer_exit(PCIDevice *dev)
+void pcie_aer_free(PCIDevice *dev)
 {
     g_free(dev->exp.aer_log.log);
+    dev->exp.aer_log.log = NULL;
 }
 
 static void pcie_aer_update_uncor_status(PCIDevice *dev)
diff --git a/include/hw/pci/pcie_aer.h b/include/hw/pci/pcie_aer.h
index bcac80a..af1dec3 100644
--- a/include/hw/pci/pcie_aer.h
+++ b/include/hw/pci/pcie_aer.h
@@ -88,7 +88,7 @@ struct PCIEAERErr {
 extern const VMStateDescription vmstate_pcie_aer_log;
 
 int pcie_aer_init(PCIDevice *dev, uint16_t offset);
-void pcie_aer_exit(PCIDevice *dev);
+void pcie_aer_free(PCIDevice *dev);
 void pcie_aer_write_config(PCIDevice *dev,
                            uint32_t addr, uint32_t val, int len);
 
-- 
1.8.1.4

[Qemu-devel] [PATCH 27/39] pci_bridge: split exit and instance_finalize

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/pci-bridge/pci_bridge_dev.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
index c1aab15..3f06e56 100644
--- a/hw/pci-bridge/pci_bridge_dev.c
+++ b/hw/pci-bridge/pci_bridge_dev.c
@@ -90,9 +90,16 @@ static void pci_bridge_dev_exitfn(PCIDevice *dev)
     }
     slotid_cap_cleanup(dev);
     shpc_cleanup(dev, &bridge_dev->bar);
+    pci_bridge_exitfn(dev);
+}
+
+static void pci_bridge_dev_instance_finalize(Object *obj)
+{
+    PCIDevice *dev = PCI_DEVICE(obj);
+    PCIBridge *br = DO_UPCAST(PCIBridge, dev, dev);
+    PCIBridgeDev *bridge_dev = DO_UPCAST(PCIBridgeDev, bridge, br);
     shpc_free(dev);
     memory_region_destroy(&bridge_dev->bar);
-    pci_bridge_exitfn(dev);
     pci_bridge_free(dev);
 }
 
@@ -152,6 +159,7 @@ static const TypeInfo pci_bridge_dev_info = {
     .parent        = TYPE_PCI_DEVICE,
     .instance_size = sizeof(PCIBridgeDev),
     .class_init = pci_bridge_dev_class_init,
+    .instance_finalize = pci_bridge_dev_instance_finalize,
 };
 
 static void pci_bridge_dev_register(void)
-- 
1.8.1.4

[Qemu-devel] [PATCH 28/39] ioh4320: split exit and instance_finalize

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/pci-bridge/ioh3420.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c
index 0a3cbb7..50df259 100644
--- a/hw/pci-bridge/ioh3420.c
+++ b/hw/pci-bridge/ioh3420.c
@@ -157,6 +157,12 @@ static void ioh3420_exitfn(PCIDevice *d)
     pcie_cap_exit(d);
     msi_uninit(d);
     pci_bridge_exitfn(d);
+}
+
+static void ioh3420_instance_finalize(Object *obj)
+{
+    PCIDevice *d = PCI_DEVICE(obj);
+
     pcie_aer_free(d);
     pci_bridge_free(d);
 }
@@ -233,6 +239,7 @@ static const TypeInfo ioh3420_info = {
     .parent        = TYPE_PCI_DEVICE,
     .instance_size = sizeof(PCIESlot),
     .class_init    = ioh3420_class_init,
+    .instance_finalize = ioh3420_instance_finalize,
 };
 
 static void ioh3420_register_types(void)
-- 
1.8.1.4

[Qemu-devel] [PATCH 29/39] xio3130-downstream: split exit and instance_finalize

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/pci-bridge/xio3130_downstream.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
index 6ff336f..477cb12 100644
--- a/hw/pci-bridge/xio3130_downstream.c
+++ b/hw/pci-bridge/xio3130_downstream.c
@@ -122,6 +122,12 @@ static void xio3130_downstream_exitfn(PCIDevice *d)
     pcie_cap_exit(d);
     msi_uninit(d);
     pci_bridge_exitfn(d);
+}
+
+static void xio3130_downstream_instance_finalize(Object *obj)
+{
+    PCIDevice *d = PCI_DEVICE(obj);
+
     pcie_aer_free(d);
     pci_bridge_free(d);
 }
@@ -200,6 +206,7 @@ static const TypeInfo xio3130_downstream_info = {
     .parent        = TYPE_PCI_DEVICE,
     .instance_size = sizeof(PCIESlot),
     .class_init    = xio3130_downstream_class_init,
+    .instance_finalize = xio3130_downstream_instance_finalize,
 };
 
 static void xio3130_downstream_register_types(void)
-- 
1.8.1.4

[Qemu-devel] [PATCH 30/39] xio3130-upstream: split exit and instance_finalize

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/pci-bridge/xio3130_upstream.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/pci-bridge/xio3130_upstream.c b/hw/pci-bridge/xio3130_upstream.c
index 9485f5e..f719496 100644
--- a/hw/pci-bridge/xio3130_upstream.c
+++ b/hw/pci-bridge/xio3130_upstream.c
@@ -104,6 +104,12 @@ static void xio3130_upstream_exitfn(PCIDevice *d)
     pcie_cap_exit(d);
     msi_uninit(d);
     pci_bridge_exitfn(d);
+}
+
+static void xio3130_upstream_instance_finalize(Object *obj)
+{
+    PCIDevice *d = PCI_DEVICE(obj);
+
     pcie_aer_free(d);
     pci_bridge_free(d);
 }
@@ -174,6 +180,7 @@ static const TypeInfo xio3130_upstream_info = {
     .parent        = TYPE_PCI_DEVICE,
     .instance_size = sizeof(PCIEPort),
     .class_init    = xio3130_upstream_class_init,
+    .instance_finalize = xio3130_upstream_instance_finalize,
 };
 
 static void xio3130_upstream_register_types(void)
-- 
1.8.1.4

[Qemu-devel] [PATCH 31/39] pcie: do not recreate mmcfg I/O region, use an alias instead

[Paolo Bonzini]

The MMIO regions should not be destroyed and recreated on the fly,
because they might have I/O in progress.  Instead, create an area that
is as large as possible, and then map a window of it into system memory.
This is possible because containers and aliases are never the destination
of I/O and can be safely deleted at any time.  It is also similar to
how PAM maps windows of system RAM.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/pci/pcie_host.c         | 22 +++++++++++++++++-----
 include/hw/pci/pcie_host.h |  1 +
 2 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/hw/pci/pcie_host.c b/hw/pci/pcie_host.c
index b2d942b..43aeeba 100644
--- a/hw/pci/pcie_host.c
+++ b/hw/pci/pcie_host.c
@@ -110,19 +110,26 @@ static const MemoryRegionOps pcie_mmcfg_ops = {
 int pcie_host_init(PCIExpressHost *e)
 {
     e->base_addr = PCIE_BASE_ADDR_UNMAPPED;
-
+    memory_region_init_io(&e->mmio, &pcie_mmcfg_ops, e, "pcie-mmcfg",
+                          PCIE_MMCFG_SIZE_MAX);
     return 0;
 }
 
 void pcie_host_mmcfg_unmap(PCIExpressHost *e)
 {
     if (e->base_addr != PCIE_BASE_ADDR_UNMAPPED) {
-        memory_region_del_subregion(get_system_memory(), &e->mmio);
-        memory_region_destroy(&e->mmio);
+        memory_region_del_subregion(get_system_memory(), &e->mapped_mmio);
+        memory_region_destroy(&e->mapped_mmio);
         e->base_addr = PCIE_BASE_ADDR_UNMAPPED;
     }
 }
 
+static void pcie_host_instance_finalize(Object *obj)
+{
+    PCIExpressHost *e = PCIE_HOST_BRIDGE(obj);
+    memory_region_destroy(&e->mmio);
+}
+
 void pcie_host_mmcfg_map(PCIExpressHost *e, hwaddr addr,
                          uint32_t size)
 {
@@ -130,9 +137,13 @@ void pcie_host_mmcfg_map(PCIExpressHost *e, hwaddr addr,
     assert(size >= PCIE_MMCFG_SIZE_MIN);
     assert(size <= PCIE_MMCFG_SIZE_MAX);
     e->size = size;
-    memory_region_init_io(&e->mmio, &pcie_mmcfg_ops, e, "pcie-mmcfg", e->size);
     e->base_addr = addr;
-    memory_region_add_subregion(get_system_memory(), e->base_addr, &e->mmio);
+
+    memory_region_init_alias(&e->mapped_mmio,
+                             "pci_bridge_vga_io_lo", &e->mmio,
+                             0, e->size);
+
+    memory_region_add_subregion(get_system_memory(), e->base_addr, &e->mapped_mmio);
 }
 
 void pcie_host_mmcfg_update(PCIExpressHost *e,
@@ -151,6 +162,7 @@ static const TypeInfo pcie_host_type_info = {
     .parent = TYPE_PCI_HOST_BRIDGE,
     .abstract = true,
     .instance_size = sizeof(PCIExpressHost),
+    .instance_finalize = pcie_host_instance_finalize,
 };
 
 static void pcie_host_register_types(void)
diff --git a/include/hw/pci/pcie_host.h b/include/hw/pci/pcie_host.h
index 1228e36..1656bde 100644
--- a/include/hw/pci/pcie_host.h
+++ b/include/hw/pci/pcie_host.h
@@ -41,6 +41,7 @@ struct PCIExpressHost {
 
     /* MMCONFIG mmio area */
     MemoryRegion mmio;
+    MemoryRegion mapped_mmio;
 };
 
 int pcie_host_init(PCIExpressHost *e);
-- 
1.8.1.4

[Qemu-devel] [PATCH 32/39] esp: use instance_finalize instead of exit

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi/esp-pci.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/scsi/esp-pci.c b/hw/scsi/esp-pci.c
index 029789a..4fe3445 100644
--- a/hw/scsi/esp-pci.c
+++ b/hw/scsi/esp-pci.c
@@ -361,8 +361,9 @@ static int esp_pci_scsi_init(PCIDevice *dev)
     return 0;
 }
 
-static void esp_pci_scsi_uninit(PCIDevice *d)
+static void esp_pci_scsi_instance_finalize(Object *obj)
 {
+    PCIDevice *d = PCI_DEVICE(obj);
     PCIESPState *pci = DO_UPCAST(PCIESPState, dev, d);
 
     memory_region_destroy(&pci->io);
@@ -374,7 +375,6 @@ static void esp_pci_class_init(ObjectClass *klass, void *data)
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
     k->init = esp_pci_scsi_init;
-    k->exit = esp_pci_scsi_uninit;
     k->vendor_id = PCI_VENDOR_ID_AMD;
     k->device_id = PCI_DEVICE_ID_AMD_SCSI;
     k->revision = 0x10;
@@ -389,6 +389,7 @@ static const TypeInfo esp_pci_info = {
     .parent = TYPE_PCI_DEVICE,
     .instance_size = sizeof(PCIESPState),
     .class_init = esp_pci_class_init,
+    .instance_finalize = esp_pci_scsi_instance_finalize,
 };
 
 typedef struct {
-- 
1.8.1.4

[Qemu-devel] [PATCH 33/39] lsi: use instance_finalize instead of exit

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi/lsi53c895a.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
index 22b8e98..53bbc01 100644
--- a/hw/scsi/lsi53c895a.c
+++ b/hw/scsi/lsi53c895a.c
@@ -2059,8 +2059,9 @@ static const VMStateDescription vmstate_lsi_scsi = {
     }
 };
 
-static void lsi_scsi_uninit(PCIDevice *d)
+static void lsi_scsi_instance_finalize(Object *obj)
 {
+    PCIDevice *d = PCI_DEVICE(obj);
     LSIState *s = DO_UPCAST(LSIState, dev, d);
 
     memory_region_destroy(&s->mmio_io);
@@ -2112,7 +2113,6 @@ static void lsi_class_init(ObjectClass *klass, void *data)
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
     k->init = lsi_scsi_init;
-    k->exit = lsi_scsi_uninit;
     k->vendor_id = PCI_VENDOR_ID_LSI_LOGIC;
     k->device_id = PCI_DEVICE_ID_LSI_53C895A;
     k->class_id = PCI_CLASS_STORAGE_SCSI;
@@ -2126,6 +2126,7 @@ static const TypeInfo lsi_info = {
     .parent        = TYPE_PCI_DEVICE,
     .instance_size = sizeof(LSIState),
     .class_init    = lsi_class_init,
+    .instance_finalize = lsi_scsi_instance_finalize,
 };
 
 static void lsi53c895a_register_types(void)
-- 
1.8.1.4

[Qemu-devel] [PATCH 34/39] pvscsi: split exit and instance_finalize

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi/vmw_pvscsi.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
index eb2270f..5e97555 100644
--- a/hw/scsi/vmw_pvscsi.c
+++ b/hw/scsi/vmw_pvscsi.c
@@ -1100,9 +1100,18 @@ pvscsi_uninit(PCIDevice *pci_dev)
     PVSCSIState *s = PVSCSI(pci_dev);
 
     trace_pvscsi_state("uninit");
-    qemu_bh_delete(s->completion_worker);
 
     pvscsi_cleanup_msi(s);
+}
+
+static void 
+pvscsi_instance_finalize(Object *obj)
+{
+    PVSCSIState *s = PVSCSI(obj);
+
+    trace_pvscsi_state("finalize");
+
+    qemu_bh_delete(s->completion_worker);
 
     memory_region_destroy(&s->io_space);
 }
@@ -1205,6 +1214,7 @@ static const TypeInfo pvscsi_info = {
     .parent        = TYPE_PCI_DEVICE,
     .instance_size = sizeof(PVSCSIState),
     .class_init    = pvscsi_class_init,
+    .instance_finalize = pvscsi_instance_finalize,
 };
 
 static void
-- 
1.8.1.4

[Qemu-devel] [PATCH 35/39] usb-uhci: use instance_finalize instead of exit

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/usb/hcd-uhci.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index c85b203..766e0fb 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -1282,8 +1282,9 @@ static int usb_uhci_vt82c686b_initfn(PCIDevice *dev)
     return usb_uhci_common_initfn(dev);
 }
 
-static void usb_uhci_exit(PCIDevice *dev)
+static void usb_uhci_instance_finalize(Object *obj)
 {
+    PCIDevice *dev = PCI_DEVICE(obj);
     UHCIState *s = DO_UPCAST(UHCIState, dev, dev);
 
     memory_region_destroy(&s->io_bar);
@@ -1305,7 +1306,6 @@ static void uhci_class_init(ObjectClass *klass, void *data)
     UHCIInfo *info = data;
 
     k->init = info->initfn ? info->initfn : usb_uhci_common_initfn;
-    k->exit = info->unplug ? usb_uhci_exit : NULL;
     k->vendor_id = info->vendor_id;
     k->device_id = info->device_id;
     k->revision  = info->revision;
@@ -1391,6 +1391,7 @@ static void uhci_register_types(void)
         .instance_size = sizeof(UHCIState),
         .class_size    = sizeof(UHCIPCIDeviceClass),
         .class_init    = uhci_class_init,
+        .instance_finalize = usb_uhci_instance_finalize,
     };
     int i;
 
-- 
1.8.1.4

[Qemu-devel] [PATCH 36/39] virtio-pci: split exit and instance_finalize

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/virtio/virtio-pci.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index cbfec6b..61f4dcb 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -996,8 +996,14 @@ static void virtio_pci_exit(PCIDevice *pci_dev)
 {
     VirtIOPCIProxy *proxy = VIRTIO_PCI(pci_dev);
     virtio_pci_stop_ioeventfd(proxy);
-    memory_region_destroy(&proxy->bar);
     msix_uninit_exclusive_bar(pci_dev);
+}
+
+static void virtio_pci_instance_finalize(Object *obj)
+{
+    PCIDevice *pci_dev = PCI_DEVICE(obj);
+    VirtIOPCIProxy *proxy = VIRTIO_PCI(obj);
+    memory_region_destroy(&proxy->bar);
     msix_free(pci_dev);
 }
 
@@ -1030,6 +1036,7 @@ static const TypeInfo virtio_pci_info = {
     .instance_size = sizeof(VirtIOPCIProxy),
     .class_init    = virtio_pci_class_init,
     .class_size    = sizeof(VirtioPCIClass),
+    .instance_finalize = virtio_pci_instance_finalize,
     .abstract      = true,
 };
 
-- 
1.8.1.4

[Qemu-devel] [PATCH 37/39] wdt_i6300esb: use instance_finalize instead of exit

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/watchdog/wdt_i6300esb.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/watchdog/wdt_i6300esb.c b/hw/watchdog/wdt_i6300esb.c
index 1407fba..b8cfc64 100644
--- a/hw/watchdog/wdt_i6300esb.c
+++ b/hw/watchdog/wdt_i6300esb.c
@@ -411,8 +411,9 @@ static int i6300esb_init(PCIDevice *dev)
     return 0;
 }
 
-static void i6300esb_exit(PCIDevice *dev)
+static void i6300esb_instance_finalize(Object *obj)
 {
+    PCIDevice *dev = PCI_DEVICE(obj);
     I6300State *d = DO_UPCAST(I6300State, dev, dev);
 
     memory_region_destroy(&d->io_mem);
@@ -431,7 +432,6 @@ static void i6300esb_class_init(ObjectClass *klass, void *data)
     k->config_read = i6300esb_config_read;
     k->config_write = i6300esb_config_write;
     k->init = i6300esb_init;
-    k->exit = i6300esb_exit;
     k->vendor_id = PCI_VENDOR_ID_INTEL;
     k->device_id = PCI_DEVICE_ID_INTEL_ESB_9;
     k->class_id = PCI_CLASS_SYSTEM_OTHER;
@@ -444,6 +444,7 @@ static const TypeInfo i6300esb_info = {
     .parent        = TYPE_PCI_DEVICE,
     .instance_size = sizeof(I6300State),
     .class_init    = i6300esb_class_init,
+    .instance_finalize = i6300esb_instance_finalize,
 };
 
 static void i6300esb_register_types(void)
-- 
1.8.1.4

[Qemu-devel] [PATCH 38/39] xen_pt: use instance_finalize instead of exit

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/xen/xen_pt.c             | 10 ++++++++++
 hw/xen/xen_pt_config_init.c |  3 ---
 hw/xen/xen_pt_msi.c         |  8 +++++++-
 3 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
index be1fd52..f40e761 100644
--- a/hw/xen/xen_pt.c
+++ b/hw/xen/xen_pt.c
@@ -805,7 +805,16 @@ static void xen_pt_unregister_device(PCIDevice *d)
         }
     }
 
+    xen_pt_msix_delete(s);
+}
+
+static void xen_pt_instance_finalize(Object *obj)
+{
+    PCIDevice *d = PCI_DEVICE(obj);
+    XenPCIPassthroughState *s = DO_UPCAST(XenPCIPassthroughState, dev, d);
+
     /* delete all emulated config registers */
+    xen_pt_msix_free(s);
     xen_pt_config_delete(s);
 
     xen_pt_unregister_regions(s);
@@ -838,6 +847,7 @@ static const TypeInfo xen_pci_passthrough_info = {
     .parent = TYPE_PCI_DEVICE,
     .instance_size = sizeof(XenPCIPassthroughState),
     .class_init = xen_pci_passthrough_class_init,
+    .instance_finalize = xen_pt_instance_finalize,
 };
 
 static void xen_pci_passthrough_register_types(void)
diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
index 01872db..94870c7 100644
--- a/hw/xen/xen_pt_config_init.c
+++ b/hw/xen/xen_pt_config_init.c
@@ -1861,9 +1861,6 @@ void xen_pt_config_delete(XenPCIPassthroughState *s)
     struct XenPTReg *reg, *next_reg;
 
     /* free MSI/MSI-X info table */
-    if (s->msix) {
-        xen_pt_msix_delete(s);
-    }
     if (s->msi) {
         g_free(s->msi);
     }
diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c
index db2c842..026a8e1 100644
--- a/hw/xen/xen_pt_msi.c
+++ b/hw/xen/xen_pt_msi.c
@@ -604,6 +604,17 @@ void xen_pt_msix_delete(XenPCIPassthroughState *s)
         return;
     }
 
+    memory_region_del_subregion(&s->bar[msix->bar_index], &msix->mmio);
+}
+
+void xen_pt_msix_free(XenPCIPassthroughState *s)
+{
+    XenPTMSIX *msix = s->msix;
+
+    if (!msix) {
+        return;
+    }
+
     /* unmap the MSI-X memory mapped register area */
     if (msix->phys_iomem_base) {
         XEN_PT_LOG(&s->dev, "unmapping physical MSI-X table from %p\n",
@@ -612,7 +619,6 @@ void xen_pt_msix_delete(XenPCIPassthroughState *s)
                + msix->table_offset_adjust);
     }
 
-    memory_region_del_subregion(&s->bar[msix->bar_index], &msix->mmio);
     memory_region_destroy(&msix->mmio);
 
     g_free(s->msix);
-- 
1.8.1.4

[Qemu-devel] [PATCH 39/39] tpm: move add/del_subregion to realize/unrealize

[Paolo Bonzini]

Only do init/destroy in instance_init/finalize.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/tpm/tpm_tis.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
index d4d8152..303d778 100644
--- a/hw/tpm/tpm_tis.c
+++ b/hw/tpm/tpm_tis.c
@@ -881,24 +881,29 @@ static void tpm_tis_realizefn(DeviceState *dev, Error **errp)
     tis->bh = qemu_bh_new(tpm_tis_receive_bh, s);
 
     isa_init_irq(&s->busdev, &tis->irq, tis->irq_num);
+    memory_region_add_subregion(isa_address_space(&s->busdev), TPM_TIS_ADDR_BASE,
+                                &s->mmio);
+}
+
+static void tpm_tis_unrealizefn(DeviceState *dev, Error **errp)
+{
+    TPMState *s = TPM(dev);
+
+    memory_region_del_subregion(get_system_memory(), &s->mmio);
 }
 
 static void tpm_tis_initfn(Object *obj)
 {
-    ISADevice *dev = ISA_DEVICE(obj);
     TPMState *s = TPM(obj);
 
     memory_region_init_io(&s->mmio, &tpm_tis_memory_ops, s, "tpm-tis-mmio",
                           TPM_TIS_NUM_LOCALITIES << TPM_TIS_LOCALITY_SHIFT);
-    memory_region_add_subregion(isa_address_space(dev), TPM_TIS_ADDR_BASE,
-                                &s->mmio);
 }
 
 static void tpm_tis_uninitfn(Object *obj)
 {
     TPMState *s = TPM(obj);
 
-    memory_region_del_subregion(get_system_memory(), &s->mmio);
     memory_region_destroy(&s->mmio);
 }
 
@@ -907,6 +912,7 @@ static void tpm_tis_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->realize = tpm_tis_realizefn;
+    dc->unrealize = tpm_tis_unrealizefn;
     dc->props = tpm_tis_properties;
     dc->reset = tpm_tis_reset;
     dc->vmsd  = &vmstate_tpm_tis;
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 39/39] tpm: move add/del_subregion to realize/unrealize

[Andreas Färber]

Am 04.06.2013 20:52, schrieb Paolo Bonzini:
> Only do init/destroy in instance_init/finalize.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/tpm/tpm_tis.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
> index d4d8152..303d778 100644
> --- a/hw/tpm/tpm_tis.c
> +++ b/hw/tpm/tpm_tis.c
> @@ -881,24 +881,29 @@ static void tpm_tis_realizefn(DeviceState *dev, Error **errp)

ISADevice *d = ISA_DEVICE(dev);

>      tis->bh = qemu_bh_new(tpm_tis_receive_bh, s);
>  
>      isa_init_irq(&s->busdev, &tis->irq, tis->irq_num);

isa_init_irq(d, ...

> +    memory_region_add_subregion(isa_address_space(&s->busdev), TPM_TIS_ADDR_BASE,
> +                                &s->mmio);

isa_address_space(d)

> +}
> +
> +static void tpm_tis_unrealizefn(DeviceState *dev, Error **errp)
> +{
> +    TPMState *s = TPM(dev);
> +
> +    memory_region_del_subregion(get_system_memory(), &s->mmio);
>  }
>  
>  static void tpm_tis_initfn(Object *obj)
>  {
> -    ISADevice *dev = ISA_DEVICE(obj);
>      TPMState *s = TPM(obj);
>  
>      memory_region_init_io(&s->mmio, &tpm_tis_memory_ops, s, "tpm-tis-mmio",
>                            TPM_TIS_NUM_LOCALITIES << TPM_TIS_LOCALITY_SHIFT);
> -    memory_region_add_subregion(isa_address_space(dev), TPM_TIS_ADDR_BASE,
> -                                &s->mmio);

Regression from here.

Andreas

>  }
>  
>  static void tpm_tis_uninitfn(Object *obj)
>  {
>      TPMState *s = TPM(obj);
>  
> -    memory_region_del_subregion(get_system_memory(), &s->mmio);
>      memory_region_destroy(&s->mmio);
>  }
>  
> @@ -907,6 +912,7 @@ static void tpm_tis_class_init(ObjectClass *klass, void *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      dc->realize = tpm_tis_realizefn;
> +    dc->unrealize = tpm_tis_unrealizefn;
>      dc->props = tpm_tis_properties;
>      dc->reset = tpm_tis_reset;
>      dc->vmsd  = &vmstate_tpm_tis;
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 00/39] Delay destruction of memory regions to instance_finalize

[Andreas Färber]

Am 04.06.2013 20:51, schrieb Paolo Bonzini:
> This series changes all PCI devices (the sole to support hotplug
> _and_ use MemoryRegions) to do memory_region_del_subregion at
> unrealize time, and memory_region_destroy at instance_finalize
> time.

The general idea looks good.

Could you please follow-up with a patch that switches from exit to
unrealize?

Also I notice some patches are accessing parent fields directly - please
use BUS(), PCI_DEVICE() etc. to hide this.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 00/39] Delay destruction of memory regions to instance_finalize

[Michael S. Tsirkin]

On Wed, Jun 05, 2013 at 11:50:52AM +0200, Andreas Färber wrote:
> Am 04.06.2013 20:51, schrieb Paolo Bonzini:
> > This series changes all PCI devices (the sole to support hotplug
> > _and_ use MemoryRegions) to do memory_region_del_subregion at
> > unrealize time, and memory_region_destroy at instance_finalize
> > time.
> 
> The general idea looks good.
> 
> Could you please follow-up with a patch that switches from exit to
> unrealize?

What do you guys think about changing the name to something
else e.g. "free" or "destroy"?

unrealize is not a word in english:
http://dictionary.cambridge.org/spellcheck/american-english/?q=unrealize

I can do it easily if people agree.

> use BUS(), PCI_DEVICE() etc. to hide this.
> 
> Andreas
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 00/39] Delay destruction of memory regions to instance_finalize

[Andreas Färber]

Am 05.06.2013 13:10, schrieb Michael S. Tsirkin:
> On Wed, Jun 05, 2013 at 11:50:52AM +0200, Andreas Färber wrote:
>> Am 04.06.2013 20:51, schrieb Paolo Bonzini:
>>> This series changes all PCI devices (the sole to support hotplug
>>> _and_ use MemoryRegions) to do memory_region_del_subregion at
>>> unrealize time, and memory_region_destroy at instance_finalize
>>> time.
>>
>> The general idea looks good.
>>
>> Could you please follow-up with a patch that switches from exit to
>> unrealize?
> 
> What do you guys think about changing the name to something
> else e.g. "free" or "destroy"?

I'm not generally opposed to renaming things, but current unrealize is a
pair with realize, and destroy or free doesn't really fit it's purpose -
that's instance_finalize. Let's CC Anthony.

Andreas

> 
> unrealize is not a word in english:
> http://dictionary.cambridge.org/spellcheck/american-english/?q=unrealize
> 
> I can do it easily if people agree.
> 
>> use BUS(), PCI_DEVICE() etc. to hide this.
>>
>> Andreas
>>
>> -- 
>> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
>> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 00/39] Delay destruction of memory regions to instance_finalize

[Michael S. Tsirkin]

On Wed, Jun 05, 2013 at 01:32:17PM +0200, Andreas Färber wrote:
> Am 05.06.2013 13:10, schrieb Michael S. Tsirkin:
> > On Wed, Jun 05, 2013 at 11:50:52AM +0200, Andreas Färber wrote:
> >> Am 04.06.2013 20:51, schrieb Paolo Bonzini:
> >>> This series changes all PCI devices (the sole to support hotplug
> >>> _and_ use MemoryRegions) to do memory_region_del_subregion at
> >>> unrealize time, and memory_region_destroy at instance_finalize
> >>> time.
> >>
> >> The general idea looks good.
> >>
> >> Could you please follow-up with a patch that switches from exit to
> >> unrealize?
> > 
> > What do you guys think about changing the name to something
> > else e.g. "free" or "destroy"?
> 
> I'm not generally opposed to renaming things, but current unrealize is a
> pair with realize, and destroy or free doesn't really fit it's purpose -
> that's instance_finalize. Let's CC Anthony.
> 
> Andreas

So @instance_init -> instance_alloc
instance_finalize -> @instance_free?



> > 
> > unrealize is not a word in english:
> > http://dictionary.cambridge.org/spellcheck/american-english/?q=unrealize
> > 
> > I can do it easily if people agree.
> > 
> >> use BUS(), PCI_DEVICE() etc. to hide this.
> >>
> >> Andreas
> >>
> >> -- 
> >> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> >> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
> 
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 00/39] Delay destruction of memory regions to instance_finalize

[Andreas Färber]

Am 05.06.2013 14:06, schrieb Michael S. Tsirkin:
> On Wed, Jun 05, 2013 at 01:32:17PM +0200, Andreas Färber wrote:
>> Am 05.06.2013 13:10, schrieb Michael S. Tsirkin:
>>> On Wed, Jun 05, 2013 at 11:50:52AM +0200, Andreas Färber wrote:
>>>> Am 04.06.2013 20:51, schrieb Paolo Bonzini:
>>>>> This series changes all PCI devices (the sole to support hotplug
>>>>> _and_ use MemoryRegions) to do memory_region_del_subregion at
>>>>> unrealize time, and memory_region_destroy at instance_finalize
>>>>> time.
>>>>
>>>> The general idea looks good.
>>>>
>>>> Could you please follow-up with a patch that switches from exit to
>>>> unrealize?
>>>
>>> What do you guys think about changing the name to something
>>> else e.g. "free" or "destroy"?
>>
>> I'm not generally opposed to renaming things, but current unrealize is a
>> pair with realize, and destroy or free doesn't really fit it's purpose -
>> that's instance_finalize. Let's CC Anthony.
> 
> So @instance_init -> instance_alloc

No, allocation happens before instance_init, it only initializes fields
of the instance, so that name seems good to me.

My ISA realize patches (need to respin after Paolo enabled gus) worked
towards resolving the DeviceClass::init vs. instance_init ambiguity, so
once completed only instance_init and class_init would remain as
"init"s. PCI is a bit more involved, and would collide with this series;
Jesse's virtio-net config size issue is calling for converting
VirtioDevice, which might be quicker.

> instance_finalize -> @instance_free?

/me misunderstandable, sorry. It doesn't free the instance either, and
Java uses "finalize" too and so does .NET iirc.

Anyway, my point was, when moving stuff out of exit, we should also
change the signature to the new one - DeviceState* and (unused) Error**.
Then we're getting closer to removing the old exit field, and at that
point renaming individual hooks - if desired - becomes a trivial patch.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 00/39] Delay destruction of memory regions to instance_finalize

[Michael S. Tsirkin]

On Wed, Jun 05, 2013 at 02:23:03PM +0200, Andreas Färber wrote:
> Am 05.06.2013 14:06, schrieb Michael S. Tsirkin:
> > On Wed, Jun 05, 2013 at 01:32:17PM +0200, Andreas Färber wrote:
> >> Am 05.06.2013 13:10, schrieb Michael S. Tsirkin:
> >>> On Wed, Jun 05, 2013 at 11:50:52AM +0200, Andreas Färber wrote:
> >>>> Am 04.06.2013 20:51, schrieb Paolo Bonzini:
> >>>>> This series changes all PCI devices (the sole to support hotplug
> >>>>> _and_ use MemoryRegions) to do memory_region_del_subregion at
> >>>>> unrealize time, and memory_region_destroy at instance_finalize
> >>>>> time.
> >>>>
> >>>> The general idea looks good.
> >>>>
> >>>> Could you please follow-up with a patch that switches from exit to
> >>>> unrealize?
> >>>
> >>> What do you guys think about changing the name to something
> >>> else e.g. "free" or "destroy"?
> >>
> >> I'm not generally opposed to renaming things, but current unrealize is a
> >> pair with realize, and destroy or free doesn't really fit it's purpose -
> >> that's instance_finalize. Let's CC Anthony.
> > 
> > So @instance_init -> instance_alloc
> 
> No, allocation happens before instance_init, it only initializes fields
> of the instance, so that name seems good to me.
> 
> My ISA realize patches (need to respin after Paolo enabled gus) worked
> towards resolving the DeviceClass::init vs. instance_init ambiguity, so
> once completed only instance_init and class_init would remain as
> "init"s. PCI is a bit more involved, and would collide with this series;
> Jesse's virtio-net config size issue is calling for converting
> VirtioDevice, which might be quicker.
> 
> > instance_finalize -> @instance_free?
> 
> /me misunderstandable, sorry. It doesn't free the instance either, and
> Java uses "finalize" too and so does .NET iirc.

Well the do not have initialize though, so if someone comes from .NET
background that person will *still* be confused.

I think we should use names that pair well and are not ambiguous:
alloc/free  create/destroy   init/cleanup  (some people do init/uninit)
get/put ...

These are all standard C things with no ambiguity.




> Anyway, my point was, when moving stuff out of exit, we should also
> change the signature to the new one - DeviceState* and (unused) Error**.
> Then we're getting closer to removing the old exit field, and at that
> point renaming individual hooks - if desired - becomes a trivial patch.
> 
> Andreas

Why is renaming new hooks related to getting rid of old ones?

> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 00/39] Delay destruction of memory regions to instance_finalize

[Andreas Färber]

Am 05.06.2013 14:36, schrieb Michael S. Tsirkin:
>> Anyway, my point was, when moving stuff out of exit, we should also
>> change the signature to the new one - DeviceState* and (unused) Error**.
>> Then we're getting closer to removing the old exit field, and at that
>> point renaming individual hooks - if desired - becomes a trivial patch.
> 
> Why is renaming new hooks related to getting rid of old ones?

* less ambiguity and more names to choose from
* introducing new callbacks as done here for instance_finalize requires
care for variable names (PCIDevice *dev vs. DeviceState *dev is the
classic) whereas renaming a hook once used is a trivial one-line change
* renaming hooks now adds to the already existing confusion of a
half-done conversion

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 00/39] Delay destruction of memory regions to instance_finalize

[Peter Maydell]

On 5 June 2013 12:10, Michael S. Tsirkin <mst@redhat.com> wrote:
> unrealize is not a word in english:

The OED says:
# unˈrealize, v.
# trans. To make unreal; to deprive of reality.

with the earliest citation from 1804.

so if it seems like the best term (and it does make
clear the pairing with realize, which I think is
a strong argument) we should go ahead and use it.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 00/39] Delay destruction of memory regions to instance_finalize

[Michael S. Tsirkin]

On Wed, Jun 05, 2013 at 12:38:35PM +0100, Peter Maydell wrote:
> On 5 June 2013 12:10, Michael S. Tsirkin <mst@redhat.com> wrote:
> > unrealize is not a word in english:
> 
> The OED says:
> # unˈrealize, v.
> # trans. To make unreal; to deprive of reality.
> 
> with the earliest citation from 1804.
> 
> so if it seems like the best term (and it does make
> clear the pairing with realize, which I think is
> a strong argument) we should go ahead and use it.
> 
> thanks
> -- PMM

realize is a bad name too.

what does it mean? make real?
It's still all virtual ...

If we want it to mean hide from guest/expose to guest,
then why not call it like this?

	expose_to_guest

	unexpose_to_guest


finalize is even more ambigous, and not pairing
with anything as far as I could see.



-- 
MST

Re: [Qemu-devel] [PATCH 00/39] Delay destruction of memory regions to instance_finalize

[Michael S. Tsirkin]

On Wed, Jun 05, 2013 at 12:38:35PM +0100, Peter Maydell wrote:
> On 5 June 2013 12:10, Michael S. Tsirkin <mst@redhat.com> wrote:
> > unrealize is not a word in english:
> 
> The OED says:
> # unˈrealize, v.
> # trans. To make unreal; to deprive of reality.
> 
> with the earliest citation from 1804.

So someone somewhere uses it like this once.
It's still a bad idea to use uncommon words,
it won't be in a dictionary of non-native english speakers
and attempts to look it up in online dictionaries fail
to return useful info.

Documentation also talks about Realization as a process
of making real.

You are going to say someone used it like that in the 19th century?

It does not change the fact that realize means "understand" in
the most common meaning of this word.
include/hw/qdev-core.h also uses the term Realization.

Again for most people Realization means becoming aware of
http://oxforddictionaries.com/definition/english/realization?q=Realization

So at least, this is ambigous.

Can we use terms which are less ambigous?

> so if it seems like the best term (and it does make
> clear the pairing with realize, which I think is
> a strong argument) we should go ahead and use it.
> 
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH 00/39] Delay destruction of memory regions to instance_finalize

[Anthony Liguori]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Wed, Jun 05, 2013 at 11:50:52AM +0200, Andreas Färber wrote:
>> Am 04.06.2013 20:51, schrieb Paolo Bonzini:
>> > This series changes all PCI devices (the sole to support hotplug
>> > _and_ use MemoryRegions) to do memory_region_del_subregion at
>> > unrealize time, and memory_region_destroy at instance_finalize
>> > time.
>> 
>> The general idea looks good.
>> 
>> Could you please follow-up with a patch that switches from exit to
>> unrealize?
>
> What do you guys think about changing the name to something
> else e.g. "free" or "destroy"?

exit/unrealize != free/destroy.

You don't actually free anything.  See 00/39 in this series for a
precise description.

> unrealize is not a word in english:
> http://dictionary.cambridge.org/spellcheck/american-english/?q=unrealize

English is a fluid language.  I wouldn't worry too much about that.

Regards,

Anthony Liguori

> I can do it easily if people agree.
>
>> use BUS(), PCI_DEVICE() etc. to hide this.
>> 
>> Andreas
>> 
>> -- 
>> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
>> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 00/39] Delay destruction of memory regions to instance_finalize

[Michael S. Tsirkin]

On Wed, Jun 05, 2013 at 07:53:05AM -0500, Anthony Liguori wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Wed, Jun 05, 2013 at 11:50:52AM +0200, Andreas Färber wrote:
> >> Am 04.06.2013 20:51, schrieb Paolo Bonzini:
> >> > This series changes all PCI devices (the sole to support hotplug
> >> > _and_ use MemoryRegions) to do memory_region_del_subregion at
> >> > unrealize time, and memory_region_destroy at instance_finalize
> >> > time.
> >> 
> >> The general idea looks good.
> >> 
> >> Could you please follow-up with a patch that switches from exit to
> >> unrealize?
> >
> > What do you guys think about changing the name to something
> > else e.g. "free" or "destroy"?
> 
> exit/unrealize != free/destroy.
> 
> You don't actually free anything.  See 00/39 in this series for a
> precise description.

That's where I got this. It says:
"instance_finalize will reclaim the memory"

> > http://dictionary.cambridge.org/spellcheck/american-english/?q=unrealize
> 
> English is a fluid language.  I wouldn't worry too much about that.
> 
> Regards,
> 
> Anthony Liguori

Well I am not worried about English at all.
I'm just confused by the function naming, and
I think it can be improved.

Can we have names actually say what a function is
doing? There's no need to use ambiguous terms and then
document what they mean.


> > I can do it easily if people agree.
> >
> >> use BUS(), PCI_DEVICE() etc. to hide this.
> >> 
> >> Andreas
> >> 
> >> -- 
> >> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> >> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 00/39] Delay destruction of memory regions to instance_finalize

[Anthony Liguori]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Wed, Jun 05, 2013 at 07:53:05AM -0500, Anthony Liguori wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> 
>> > On Wed, Jun 05, 2013 at 11:50:52AM +0200, Andreas Färber wrote:
>> >> Am 04.06.2013 20:51, schrieb Paolo Bonzini:
>> >> > This series changes all PCI devices (the sole to support hotplug
>> >> > _and_ use MemoryRegions) to do memory_region_del_subregion at
>> >> > unrealize time, and memory_region_destroy at instance_finalize
>> >> > time.
>> >> 
>> >> The general idea looks good.
>> >> 
>> >> Could you please follow-up with a patch that switches from exit to
>> >> unrealize?
>> >
>> > What do you guys think about changing the name to something
>> > else e.g. "free" or "destroy"?
>> 
>> exit/unrealize != free/destroy.
>> 
>> You don't actually free anything.  See 00/39 in this series for a
>> precise description.
>
> That's where I got this. It says:
> "instance_finalize will reclaim the memory"

I'm not sure what you're talking about.

There are two callbacks: exit and instance_finalize.

exit() is called to disconnect the device to the guest.  Andreas is
proposing renaming it to unrealize.

Ideally, reset() would be implemented as a combination of calls to
unrealize() + realize() (or exit() + init() as they are named today).

Nothing should be freed in exit.

instance_finalize is only called once, before the object is freed().  It
should be used to release resources associated with the object.

Paolo is using this to destroy memory regions.  Normally this callback
is never used because child objects are automatically handled by QOM.

"finalize" is the standard name of the hook that gets called before
garbage collection as Andreas previously pointed out.  It's this way in
Java, Python, C#, etc.

Regards,

Anthony Liguori

>
>> > http://dictionary.cambridge.org/spellcheck/american-english/?q=unrealize
>> 
>> English is a fluid language.  I wouldn't worry too much about that.
>> 
>> Regards,
>> 
>> Anthony Liguori
>
> Well I am not worried about English at all.
> I'm just confused by the function naming, and
> I think it can be improved.
>
> Can we have names actually say what a function is
> doing? There's no need to use ambiguous terms and then
> document what they mean.
>
>
>> > I can do it easily if people agree.
>> >
>> >> use BUS(), PCI_DEVICE() etc. to hide this.
>> >> 
>> >> Andreas
>> >> 
>> >> -- 
>> >> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
>> >> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 00/39] Delay destruction of memory regions to instance_finalize

[Michael S. Tsirkin]

On Wed, Jun 05, 2013 at 10:33:05AM -0500, Anthony Liguori wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Wed, Jun 05, 2013 at 07:53:05AM -0500, Anthony Liguori wrote:
> >> "Michael S. Tsirkin" <mst@redhat.com> writes:
> >> 
> >> > On Wed, Jun 05, 2013 at 11:50:52AM +0200, Andreas Färber wrote:
> >> >> Am 04.06.2013 20:51, schrieb Paolo Bonzini:
> >> >> > This series changes all PCI devices (the sole to support hotplug
> >> >> > _and_ use MemoryRegions) to do memory_region_del_subregion at
> >> >> > unrealize time, and memory_region_destroy at instance_finalize
> >> >> > time.
> >> >> 
> >> >> The general idea looks good.
> >> >> 
> >> >> Could you please follow-up with a patch that switches from exit to
> >> >> unrealize?
> >> >
> >> > What do you guys think about changing the name to something
> >> > else e.g. "free" or "destroy"?
> >> 
> >> exit/unrealize != free/destroy.
> >> 
> >> You don't actually free anything.  See 00/39 in this series for a
> >> precise description.
> >
> > That's where I got this. It says:
> > "instance_finalize will reclaim the memory"
> 
> I'm not sure what you're talking about.
> 
> There are two callbacks: exit and instance_finalize.
> 
> exit() is called to disconnect the device to the guest.  Andreas is
> proposing renaming it to unrealize.
> 
> Ideally, reset() would be implemented as a combination of calls to
> unrealize() + realize() (or exit() + init() as they are named today).
> 
> Nothing should be freed in exit.
> 
> instance_finalize is only called once, before the object is freed().  It
> should be used to release resources associated with the object.
> 
> Paolo is using this to destroy memory regions.  Normally this callback
> is never used because child objects are automatically handled by QOM.
>
> "finalize" is the standard name of the hook that gets called before
> garbage collection as Andreas previously pointed out.

I am simply asking for function names to tell us
what they do, not when they are called.

>  It's this way in
> Java, Python, C#, etc.

Well bringing in idioms from more languages just serves to confuse
readers even more.

> Regards,
> 
> Anthony Liguori
> 
> >
> >> > http://dictionary.cambridge.org/spellcheck/american-english/?q=unrealize
> >> 
> >> English is a fluid language.  I wouldn't worry too much about that.
> >> 
> >> Regards,
> >> 
> >> Anthony Liguori
> >
> > Well I am not worried about English at all.
> > I'm just confused by the function naming, and
> > I think it can be improved.
> >
> > Can we have names actually say what a function is
> > doing? There's no need to use ambiguous terms and then
> > document what they mean.
> >
> >
> >> > I can do it easily if people agree.
> >> >
> >> >> use BUS(), PCI_DEVICE() etc. to hide this.
> >> >> 
> >> >> Andreas
> >> >> 
> >> >> -- 
> >> >> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> >> >> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 00/39] Delay destruction of memory regions to instance_finalize

[Peter Maydell]

On 5 June 2013 16:44, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Wed, Jun 05, 2013 at 10:33:05AM -0500, Anthony Liguori wrote:
>> "finalize" is the standard name of the hook that gets called before
>> garbage collection as Andreas previously pointed out.
>
> I am simply asking for function names to tell us
> what they do, not when they are called.

These functions are object lifecycle ones -- "when they
are called" is exactly the key information about what
they do.

-- PMM

Re: [Qemu-devel] [PATCH 00/39] Delay destruction of memory regions to instance_finalize

[Paolo Bonzini]

Il 05/06/2013 05:50, Andreas Färber ha scritto:
> Am 04.06.2013 20:51, schrieb Paolo Bonzini:
>> This series changes all PCI devices (the sole to support hotplug
>> _and_ use MemoryRegions) to do memory_region_del_subregion at
>> unrealize time, and memory_region_destroy at instance_finalize
>> time.
> 
> The general idea looks good.
> 
> Could you please follow-up with a patch that switches from exit to
> unrealize?

I can add it to the queue, but I have at least 4 pending series.

> Also I notice some patches are accessing parent fields directly - please
> use BUS(), PCI_DEVICE() etc. to hide this.

I'm always using them.  For example:

+static void intel_hda_instance_finalize(Object *obj)
+{
+    PCIDevice *pci = PCI_DEVICE(obj);
+    IntelHDAState *d = DO_UPCAST(IntelHDAState, pci, pci);

What I'm not doing, is adding new cast macros---one thing at a time.

Paolo

Re: [Qemu-devel] [PATCH 00/39] Delay destruction of memory regions to instance_finalize

[Andreas Färber]

Am 07.06.2013 03:03, schrieb Paolo Bonzini:
> Il 05/06/2013 05:50, Andreas Färber ha scritto:
>> Also I notice some patches are accessing parent fields directly - please
>> use BUS(), PCI_DEVICE() etc. to hide this.
> 
> I'm always using them.  For example:
> 
> +static void intel_hda_instance_finalize(Object *obj)
> +{
> +    PCIDevice *pci = PCI_DEVICE(obj);
> +    IntelHDAState *d = DO_UPCAST(IntelHDAState, pci, pci);

I'll comment inline then. :)

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 00/39] Delay destruction of memory regions to instance_finalize

[Paolo Bonzini]

Il 07/06/2013 03:45, Andreas Färber ha scritto:
> Am 07.06.2013 03:03, schrieb Paolo Bonzini:
>> Il 05/06/2013 05:50, Andreas Färber ha scritto:
>>> Also I notice some patches are accessing parent fields directly - please
>>> use BUS(), PCI_DEVICE() etc. to hide this.
>>
>> I'm always using them.  For example:
>>
>> +static void intel_hda_instance_finalize(Object *obj)
>> +{
>> +    PCIDevice *pci = PCI_DEVICE(obj);
>> +    IntelHDAState *d = DO_UPCAST(IntelHDAState, pci, pci);
> 
> I'll comment inline then. :)

Ah right, in those cases I was mostly doing what the
adjacent/pre-existing code did.  But I can definitely fix them up!

Paolo

Re: [Qemu-devel] [PATCH 00/39] Delay destruction of memory regions to instance_finalize

[Peter Crosthwaite]

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

Hi,

On Jun 7, 2013 11:04 AM, "Paolo Bonzini" <pbonzini@redhat.com> wrote:
>
> Il 05/06/2013 05:50, Andreas Färber ha scritto:
> > Am 04.06.2013 20:51, schrieb Paolo Bonzini:
> >> This series changes all PCI devices (the sole to support hotplug
> >> _and_ use MemoryRegions) to do memory_region_del_subregion at
> >> unrealize time, and memory_region_destroy at instance_finalize
> >> time.
> >
> > The general idea looks good.
> >
> > Could you please follow-up with a patch that switches from exit to
> > unrealize?
>
> I can add it to the queue, but I have at least 4 pending series.
>
> > Also I notice some patches are accessing parent fields directly - please
> > use BUS(), PCI_DEVICE() etc. to hide this.
>
> I'm always using them.  For example:
>
> +static void intel_hda_instance_finalize(Object *obj)
> +{
> +    PCIDevice *pci = PCI_DEVICE(obj);
> +    IntelHDAState *d = DO_UPCAST(IntelHDAState, pci, pci);
>
> What I'm not doing, is adding new cast macros---one thing at a time.
>

I have a series that fixes all qom cast macros for all PCI devices tree
wide. Can post. Qom cast macros added as needed.

How are you regression testing this series? If you have a pc/PCI regression
suite I could use it for my series.

Regards
Peter

> Paolo
>
On Jun 7, 2013 11:04 AM, "Paolo Bonzini" <pbonzini@redhat.com> wrote:

> Il 05/06/2013 05:50, Andreas Färber ha scritto:
> > Am 04.06.2013 20:51, schrieb Paolo Bonzini:
> >> This series changes all PCI devices (the sole to support hotplug
> >> _and_ use MemoryRegions) to do memory_region_del_subregion at
> >> unrealize time, and memory_region_destroy at instance_finalize
> >> time.
> >
> > The general idea looks good.
> >
> > Could you please follow-up with a patch that switches from exit to
> > unrealize?
>
> I can add it to the queue, but I have at least 4 pending series.
>
> > Also I notice some patches are accessing parent fields directly - please
> > use BUS(), PCI_DEVICE() etc. to hide this.
>
> I'm always using them.  For example:
>
> +static void intel_hda_instance_finalize(Object *obj)
> +{
> +    PCIDevice *pci = PCI_DEVICE(obj);
> +    IntelHDAState *d = DO_UPCAST(IntelHDAState, pci, pci);
>
> What I'm not doing, is adding new cast macros---one thing at a time.
>
> Paolo
>
>

[-- Attachment #2: Type: text/html, Size: 3018 bytes --]

Re: [Qemu-devel] [PATCH 00/39] Delay destruction of memory regions to instance_finalize

[Andreas Färber]

Hi Peter,

Am 07.06.2013 10:41, schrieb Peter Crosthwaite:
> I have a series that fixes all qom cast macros for all PCI devices tree
> wide. Can post. Qom cast macros added as needed.

Sounds promising! I just CC'ed you on my ISA series v2, which touches on
PCI_BUS() in the final patch, dropping FROM_QBUS().

> How are you regression testing this series? If you have a pc/PCI
> regression suite I could use it for my series.

I rely on `make check` to find the most obvious QOM errors.
libqos PCI support is still very new, so we don't have full device
coverage there yet.

Usually I run an openSUSE and/or SLES guest under KVM (pick your
favorite), which will exercise the relevant init, realize and reset code
paths at least.

And finally I re-review things in gitk before sending - not the nicest
tool but convenient for fixing up things and verifying quickly.

HTH,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 00/39] Delay destruction of memory regions to instance_finalize

[Anthony Liguori]

Paolo Bonzini <pbonzini@redhat.com> writes:

> QOM splits the destruction of a device in two phases:
>
> - unrealize, also known as "exit" from qdev times, should isolate
>   the device from the guest.  After unrealize returns, the guest
>   should not be able to issue new requests.
>
> - instance_finalize will reclaim the memory.  This is only called
>   after all requests terminate and drop the references on the
>   device.

Does it make sense to QOM-ify the memory regions?  If they were QOM
objects, they could be added as children and then the reaping would be
handled automatically.

Could be a follow up.

Regards,

Anthony Liguori

>
> This is important even now, but overlooked.  It will become much
> more important for devices that will be able to access memory
> out of the big QEMU lock.
>
> This series changes all PCI devices (the sole to support hotplug
> _and_ use MemoryRegions) to do memory_region_del_subregion at
> unrealize time, and memory_region_destroy at instance_finalize
> time.
>
> This is mostly a PCI patch, and independent from the others,
> so I believe it should go through mst's tree.
>
> Paolo
>
>
> Paolo Bonzini (39):
>   scsi: keep device alive while it has requests
>   dma: keep a device alive while it has SGLists
>   pci: split exit and finalize
>   ac97: use instance_finalize instead of exit
>   es1370: use instance_finalize instead of exit
>   hda: use instance_finalize instead of exit
>   serial: use instance_finalize instead of exit
>   tpci200: use instance_finalize instead of exit
>   pci-assign: use instance_finalize instead of exit
>   ahci: use instance_finalize instead of exit
>   msix: split msix_free from msix_uninit
>   cmd646: use instance_finalize instead of exit
>   ide/piix: use instance_finalize instead of exit
>   ide/via: use instance_finalize instead of exit
>   ivshmem: use instance_finalize instead of exit
>   pci-testdev: use instance_finalize instead of exit
>   vfio: use instance_finalize instead of exit
>   e1000: use instance_finalize instead of exit
>   eepro100: use instance_finalize instead of exit
>   ne2000: use instance_finalize instead of exit
>   pcnet: use instance_finalize instead of exit
>   rtl8139: use instance_finalize instead of exit
>   vmxnet3: use instance_finalize instead of exit
>   shpc: use instance_finalize instead of exit
>   pci_bridge: split pci_bridge_exitfn from pci_bridge_free
>   pcie_aer: pcie_aer_exit really frees stuff
>   pci_bridge: use instance_finalize instead of exit
>   ioh4320: use instance_finalize instead of exit
>   xio3130-downstream: use instance_finalize instead of exit
>   xio3130-upstream: use instance_finalize instead of exit
>   pcie: do not recreate mmcfg I/O region, use an alias instead
>   esp: use instance_finalize instead of exit
>   lsi: use instance_finalize instead of exit
>   pvscsi: use instance_finalize instead of exit
>   usb-uhci: use instance_finalize instead of exit
>   virtio-pci: use instance_finalize instead of exit
>   wdt_i6300esb: use instance_finalize instead of exit
>   xen_pt: use instance_finalize instead of exit
>   tpm: move add/del_subregion to realize/unrealize
>
>  dma-helpers.c                      |  6 ++++-
>  hw/audio/ac97.c                    |  5 ++--
>  hw/audio/es1370.c                  |  5 ++--
>  hw/audio/intel-hda.c               |  8 ++++++
>  hw/char/serial-pci.c               | 24 ++++++++++++++++++
>  hw/char/tpci200.c                  |  5 ++--
>  hw/i386/kvm/pci-assign.c           |  8 ++++++
>  hw/ide/ahci.c                      |  5 ++--
>  hw/ide/ahci.h                      |  2 +-
>  hw/ide/cmd646.c                    |  5 ++--
>  hw/ide/ich.c                       | 13 +++++++---
>  hw/ide/macio.c                     |  4 +--
>  hw/ide/piix.c                      |  8 +++---
>  hw/ide/via.c                       |  5 ++--
>  hw/misc/ivshmem.c                  | 10 +++++++-
>  hw/misc/pci-testdev.c              |  5 ++--
>  hw/misc/vfio.c                     | 52 +++++++++++++++++++++++++++++++++++---
>  hw/net/e1000.c                     |  5 ++--
>  hw/net/eepro100.c                  |  5 ++--
>  hw/net/ne2000.c                    |  5 ++--
>  hw/net/pcnet-pci.c                 |  5 ++--
>  hw/net/rtl8139.c                   |  5 ++--
>  hw/net/vmxnet3.c                   | 14 ++++++++--
>  hw/pci-bridge/i82801b11.c          |  1 +
>  hw/pci-bridge/ioh3420.c            | 11 +++++++-
>  hw/pci-bridge/pci_bridge_dev.c     | 14 +++++++++-
>  hw/pci-bridge/xio3130_downstream.c | 11 +++++++-
>  hw/pci-bridge/xio3130_upstream.c   | 11 +++++++-
>  hw/pci/msix.c                      | 26 ++++++++++++++-----
>  hw/pci/pci.c                       | 15 ++++++++---
>  hw/pci/pci_bridge.c                |  5 ++++
>  hw/pci/pcie_aer.c                  |  3 ++-
>  hw/pci/pcie_host.c                 | 22 ++++++++++++----
>  hw/pci/shpc.c                      |  8 +++++-
>  hw/scsi/esp-pci.c                  |  5 ++--
>  hw/scsi/lsi53c895a.c               |  5 ++--
>  hw/scsi/megasas.c                  |  4 +--
>  hw/scsi/scsi-bus.c                 |  4 +++
>  hw/scsi/virtio-scsi.c              | 10 +++++---
>  hw/scsi/vmw_pvscsi.c               | 12 ++++++++-
>  hw/tpm/tpm_tis.c                   | 14 +++++++---
>  hw/usb/hcd-ehci.c                  |  4 +--
>  hw/usb/hcd-uhci.c                  |  5 ++--
>  hw/virtio/virtio-pci.c             | 10 +++++++-
>  hw/watchdog/wdt_i6300esb.c         |  5 ++--
>  hw/xen/xen_pt.c                    | 10 ++++++++
>  hw/xen/xen_pt_config_init.c        |  3 ---
>  hw/xen/xen_pt_msi.c                |  8 +++++-
>  include/hw/pci/msix.h              |  1 +
>  include/hw/pci/pci.h               |  2 +-
>  include/hw/pci/pci_bridge.h        |  1 +
>  include/hw/pci/pcie_aer.h          |  2 +-
>  include/hw/pci/pcie_host.h         |  1 +
>  include/hw/pci/shpc.h              |  1 +
>  include/sysemu/dma.h               |  4 ++-
>  55 files changed, 355 insertions(+), 92 deletions(-)
>
> -- 
> 1.8.1.4