[Qemu-devel] [PATCH 0/2] pci: exit path cleanup and fix

[Qemu-devel] [PATCH 0/2] pci: exit path cleanup and fix

[Alex Williamson]

We've currently got a bug in pci_unregister_device in the ordering of
calling the driver exit function and unregistering io regions.  In
every driver memory regions are created in the init function and
destroyed in the exit function.  By calling pci_unregister_io_regions
after the exit function, we're calling memory_region_del_subregion
with a pointer to a MemoryRegion that has already been destroyed.

It's easy enough to change the ordering, but the exit function is
currently allowed to fail.  Even if we wanted to restore the device
at that point, we've interrupted the mappings from the guest
perspective and it seems precarious at best whether an exit function
can fail and leave a usable device.  Fortunately nobody has any
possibility of actually failing the exit path.  Normally I'm a
proponent of error paths, but allowing an exit to fail is like
allowing free(3) to fail.

So, firt redefine that exit can't fail, then fix the ordering of
pci_unregister_device().  If anyone has plans for a failure case in
the exit path, please speak now.  Thanks,

Alex

---

Alex Williamson (2):
      pci: Unregister BARs before device exit
      pci: convert PCIUnregisterFunc to void


 hw/ac97.c               |    3 +--
 hw/e1000.c              |    3 +--
 hw/eepro100.c           |    3 +--
 hw/es1370.c             |    3 +--
 hw/ide/cmd646.c         |    4 +---
 hw/ide/ich.c            |    4 +---
 hw/ide/piix.c           |    4 +---
 hw/ide/via.c            |    4 +---
 hw/intel-hda.c          |    3 +--
 hw/ioh3420.c            |    8 +++-----
 hw/ivshmem.c            |    4 +---
 hw/lsi53c895a.c         |    4 +---
 hw/ne2000.c             |    3 +--
 hw/pci.c                |   11 +++++------
 hw/pci.h                |    2 +-
 hw/pci_bridge.c         |    3 +--
 hw/pci_bridge.h         |    2 +-
 hw/pci_bridge_dev.c     |   12 ++++--------
 hw/pcnet-pci.c          |    3 +--
 hw/rtl8139.c            |    3 +--
 hw/usb/hcd-uhci.c       |    3 +--
 hw/virtio-pci.c         |   23 +++++++++++------------
 hw/wdt_i6300esb.c       |    4 +---
 hw/xio3130_downstream.c |    8 +++-----
 hw/xio3130_upstream.c   |    8 +++-----
 25 files changed, 48 insertions(+), 84 deletions(-)

[Qemu-devel] [PATCH 1/2] pci: convert PCIUnregisterFunc to void

[Alex Williamson]

Not a single driver has any possibility of failure on their
exit function, let's keep it that way.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

 hw/ac97.c               |    3 +--
 hw/e1000.c              |    3 +--
 hw/eepro100.c           |    3 +--
 hw/es1370.c             |    3 +--
 hw/ide/cmd646.c         |    4 +---
 hw/ide/ich.c            |    4 +---
 hw/ide/piix.c           |    4 +---
 hw/ide/via.c            |    4 +---
 hw/intel-hda.c          |    3 +--
 hw/ioh3420.c            |    8 +++-----
 hw/ivshmem.c            |    4 +---
 hw/lsi53c895a.c         |    4 +---
 hw/ne2000.c             |    3 +--
 hw/pci.c                |    8 +++-----
 hw/pci.h                |    2 +-
 hw/pci_bridge.c         |    3 +--
 hw/pci_bridge.h         |    2 +-
 hw/pci_bridge_dev.c     |   12 ++++--------
 hw/pcnet-pci.c          |    3 +--
 hw/rtl8139.c            |    3 +--
 hw/usb/hcd-uhci.c       |    3 +--
 hw/virtio-pci.c         |   23 +++++++++++------------
 hw/wdt_i6300esb.c       |    4 +---
 hw/xio3130_downstream.c |    8 +++-----
 hw/xio3130_upstream.c   |    8 +++-----
 25 files changed, 46 insertions(+), 83 deletions(-)

diff --git a/hw/ac97.c b/hw/ac97.c
index e791b9d..0f561fa 100644
--- a/hw/ac97.c
+++ b/hw/ac97.c
@@ -1319,13 +1319,12 @@ static int ac97_initfn (PCIDevice *dev)
     return 0;
 }
 
-static int ac97_exitfn (PCIDevice *dev)
+static void ac97_exitfn (PCIDevice *dev)
 {
     AC97LinkState *s = DO_UPCAST (AC97LinkState, dev, dev);
 
     memory_region_destroy (&s->io_nam);
     memory_region_destroy (&s->io_nabm);
-    return 0;
 }
 
 int ac97_init (PCIBus *bus)
diff --git a/hw/e1000.c b/hw/e1000.c
index 4573f13..6c5bc44 100644
--- a/hw/e1000.c
+++ b/hw/e1000.c
@@ -1192,7 +1192,7 @@ e1000_cleanup(VLANClientState *nc)
     s->nic = NULL;
 }
 
-static int
+static void
 pci_e1000_uninit(PCIDevice *dev)
 {
     E1000State *d = DO_UPCAST(E1000State, dev, dev);
@@ -1202,7 +1202,6 @@ pci_e1000_uninit(PCIDevice *dev)
     memory_region_destroy(&d->mmio);
     memory_region_destroy(&d->io);
     qemu_del_vlan_client(&d->nic->nc);
-    return 0;
 }
 
 static NetClientInfo net_e1000_info = {
diff --git a/hw/eepro100.c b/hw/eepro100.c
index 6279ae3..9745ad5 100644
--- a/hw/eepro100.c
+++ b/hw/eepro100.c
@@ -1831,7 +1831,7 @@ static void nic_cleanup(VLANClientState *nc)
     s->nic = NULL;
 }
 
-static int pci_nic_uninit(PCIDevice *pci_dev)
+static void pci_nic_uninit(PCIDevice *pci_dev)
 {
     EEPRO100State *s = DO_UPCAST(EEPRO100State, dev, pci_dev);
 
@@ -1841,7 +1841,6 @@ static int pci_nic_uninit(PCIDevice *pci_dev)
     vmstate_unregister(&pci_dev->qdev, s->vmstate, s);
     eeprom93xx_free(&pci_dev->qdev, s->eeprom);
     qemu_del_vlan_client(&s->nic->nc);
-    return 0;
 }
 
 static NetClientInfo net_eepro100_info = {
diff --git a/hw/es1370.c b/hw/es1370.c
index 573f747..e34234c 100644
--- a/hw/es1370.c
+++ b/hw/es1370.c
@@ -1018,12 +1018,11 @@ static int es1370_initfn (PCIDevice *dev)
     return 0;
 }
 
-static int es1370_exitfn (PCIDevice *dev)
+static void es1370_exitfn (PCIDevice *dev)
 {
     ES1370State *s = DO_UPCAST (ES1370State, dev, dev);
 
     memory_region_destroy (&s->io);
-    return 0;
 }
 
 int es1370_init (PCIBus *bus)
diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index bf8ece4..4ff3624 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -295,7 +295,7 @@ static int pci_cmd646_ide_initfn(PCIDevice *dev)
     return 0;
 }
 
-static int pci_cmd646_ide_exitfn(PCIDevice *dev)
+static void pci_cmd646_ide_exitfn(PCIDevice *dev)
 {
     PCIIDEState *d = DO_UPCAST(PCIIDEState, dev, dev);
     unsigned i;
@@ -309,8 +309,6 @@ static int pci_cmd646_ide_exitfn(PCIDevice *dev)
         memory_region_destroy(&d->cmd646_bar[i].data);
     }
     memory_region_destroy(&d->bmdma_bar);
-
-    return 0;
 }
 
 void pci_cmd646_ide_init(PCIBus *bus, DriveInfo **hd_table,
diff --git a/hw/ide/ich.c b/hw/ide/ich.c
index e3eaaea..c3a425b 100644
--- a/hw/ide/ich.c
+++ b/hw/ide/ich.c
@@ -132,15 +132,13 @@ static int pci_ich9_ahci_init(PCIDevice *dev)
     return 0;
 }
 
-static int pci_ich9_uninit(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);
-
-    return 0;
 }
 
 static void ich_ahci_class_init(ObjectClass *klass, void *data)
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index bcaa400..455c1b2 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -199,7 +199,7 @@ PCIDevice *pci_piix3_xen_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn)
     return dev;
 }
 
-static int pci_piix_ide_exitfn(PCIDevice *dev)
+static void pci_piix_ide_exitfn(PCIDevice *dev)
 {
     PCIIDEState *d = DO_UPCAST(PCIIDEState, dev, dev);
     unsigned i;
@@ -211,8 +211,6 @@ static int pci_piix_ide_exitfn(PCIDevice *dev)
         memory_region_destroy(&d->bmdma[i].addr_ioport);
     }
     memory_region_destroy(&d->bmdma_bar);
-
-    return 0;
 }
 
 /* hd_table must contain 4 block drivers */
diff --git a/hw/ide/via.c b/hw/ide/via.c
index eec5136..3e25085 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -189,7 +189,7 @@ static int vt82c686b_ide_initfn(PCIDevice *dev)
     return 0;
 }
 
-static int vt82c686b_ide_exitfn(PCIDevice *dev)
+static void vt82c686b_ide_exitfn(PCIDevice *dev)
 {
     PCIIDEState *d = DO_UPCAST(PCIIDEState, dev, dev);
     unsigned i;
@@ -201,8 +201,6 @@ static int vt82c686b_ide_exitfn(PCIDevice *dev)
         memory_region_destroy(&d->bmdma[i].addr_ioport);
     }
     memory_region_destroy(&d->bmdma_bar);
-
-    return 0;
 }
 
 void vt82c686b_ide_init(PCIBus *bus, DriveInfo **hd_table, int devfn)
diff --git a/hw/intel-hda.c b/hw/intel-hda.c
index 8f3b70b..04bed5e 100644
--- a/hw/intel-hda.c
+++ b/hw/intel-hda.c
@@ -1144,13 +1144,12 @@ static int intel_hda_init(PCIDevice *pci)
     return 0;
 }
 
-static int intel_hda_exit(PCIDevice *pci)
+static void intel_hda_exit(PCIDevice *pci)
 {
     IntelHDAState *d = DO_UPCAST(IntelHDAState, pci, pci);
 
     msi_uninit(&d->pci);
     memory_region_destroy(&d->mmio);
-    return 0;
 }
 
 static int intel_hda_post_load(void *opaque, int version)
diff --git a/hw/ioh3420.c b/hw/ioh3420.c
index 2f3d56e..01aaab8 100644
--- a/hw/ioh3420.c
+++ b/hw/ioh3420.c
@@ -96,7 +96,6 @@ static int ioh3420_initfn(PCIDevice *d)
     PCIEPort *p = DO_UPCAST(PCIEPort, br, br);
     PCIESlot *s = DO_UPCAST(PCIESlot, port, p);
     int rc;
-    int tmp;
 
     rc = pci_bridge_initfn(d);
     if (rc < 0) {
@@ -144,12 +143,11 @@ err_pcie_cap:
 err_msi:
     msi_uninit(d);
 err_bridge:
-    tmp = pci_bridge_exitfn(d);
-    assert(!tmp);
+    pci_bridge_exitfn(d);
     return rc;
 }
 
-static int ioh3420_exitfn(PCIDevice *d)
+static void ioh3420_exitfn(PCIDevice *d)
 {
     PCIBridge* br = DO_UPCAST(PCIBridge, dev, d);
     PCIEPort *p = DO_UPCAST(PCIEPort, br, br);
@@ -159,7 +157,7 @@ static int ioh3420_exitfn(PCIDevice *d)
     pcie_chassis_del_slot(s);
     pcie_cap_exit(d);
     msi_uninit(d);
-    return pci_bridge_exitfn(d);
+    pci_bridge_exitfn(d);
 }
 
 PCIESlot *ioh3420_init(PCIBus *bus, int devfn, bool multifunction,
diff --git a/hw/ivshmem.c b/hw/ivshmem.c
index 8b49eee..7d4123c 100644
--- a/hw/ivshmem.c
+++ b/hw/ivshmem.c
@@ -760,7 +760,7 @@ static int pci_ivshmem_init(PCIDevice *dev)
     return 0;
 }
 
-static int pci_ivshmem_uninit(PCIDevice *dev)
+static void pci_ivshmem_uninit(PCIDevice *dev)
 {
     IVShmemState *s = DO_UPCAST(IVShmemState, dev, dev);
 
@@ -775,8 +775,6 @@ static int pci_ivshmem_uninit(PCIDevice *dev)
     memory_region_destroy(&s->ivshmem);
     memory_region_destroy(&s->bar);
     unregister_savevm(&dev->qdev, "ivshmem", s);
-
-    return 0;
 }
 
 static Property ivshmem_properties[] = {
diff --git a/hw/lsi53c895a.c b/hw/lsi53c895a.c
index f022a02..9205f65 100644
--- a/hw/lsi53c895a.c
+++ b/hw/lsi53c895a.c
@@ -2070,15 +2070,13 @@ static const VMStateDescription vmstate_lsi_scsi = {
     }
 };
 
-static int lsi_scsi_uninit(PCIDevice *d)
+static void lsi_scsi_uninit(PCIDevice *d)
 {
     LSIState *s = DO_UPCAST(LSIState, dev, d);
 
     memory_region_destroy(&s->mmio_io);
     memory_region_destroy(&s->ram_io);
     memory_region_destroy(&s->io_io);
-
-    return 0;
 }
 
 static const struct SCSIBusInfo lsi_scsi_info = {
diff --git a/hw/ne2000.c b/hw/ne2000.c
index d02e60c..afadbdb 100644
--- a/hw/ne2000.c
+++ b/hw/ne2000.c
@@ -744,14 +744,13 @@ static int pci_ne2000_init(PCIDevice *pci_dev)
     return 0;
 }
 
-static int pci_ne2000_exit(PCIDevice *pci_dev)
+static void pci_ne2000_exit(PCIDevice *pci_dev)
 {
     PCINE2000State *d = DO_UPCAST(PCINE2000State, dev, pci_dev);
     NE2000State *s = &d->ne2000;
 
     memory_region_destroy(&s->io);
     qemu_del_vlan_client(&s->nic->nc);
-    return 0;
 }
 
 static Property ne2000_properties[] = {
diff --git a/hw/pci.c b/hw/pci.c
index 170de29..8934722 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -874,12 +874,10 @@ static int pci_unregister_device(DeviceState *dev)
 {
     PCIDevice *pci_dev = PCI_DEVICE(dev);
     PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(pci_dev);
-    int ret = 0;
 
-    if (pc->exit)
-        ret = pc->exit(pci_dev);
-    if (ret)
-        return ret;
+    if (pc->exit) {
+        pc->exit(pci_dev);
+    }
 
     pci_unregister_io_regions(pci_dev);
     pci_del_option_rom(pci_dev);
diff --git a/hw/pci.h b/hw/pci.h
index ac9d906..2ffb250 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -85,7 +85,7 @@ typedef uint32_t PCIConfigReadFunc(PCIDevice *pci_dev,
                                    uint32_t address, int len);
 typedef void PCIMapIORegionFunc(PCIDevice *pci_dev, int region_num,
                                 pcibus_t addr, pcibus_t size, int type);
-typedef int PCIUnregisterFunc(PCIDevice *pci_dev);
+typedef void PCIUnregisterFunc(PCIDevice *pci_dev);
 
 typedef struct PCIIORegion {
     pcibus_t addr; /* current PCI mapping address. -1 means not mapped */
diff --git a/hw/pci_bridge.c b/hw/pci_bridge.c
index 0d59125..96e271b 100644
--- a/hw/pci_bridge.c
+++ b/hw/pci_bridge.c
@@ -333,7 +333,7 @@ int pci_bridge_initfn(PCIDevice *dev)
 }
 
 /* default qdev clean up function for PCI-to-PCI bridge */
-int pci_bridge_exitfn(PCIDevice *pci_dev)
+void pci_bridge_exitfn(PCIDevice *pci_dev)
 {
     PCIBridge *s = DO_UPCAST(PCIBridge, dev, pci_dev);
     assert(QLIST_EMPTY(&s->sec_bus.child));
@@ -342,7 +342,6 @@ int pci_bridge_exitfn(PCIDevice *pci_dev)
     memory_region_destroy(&s->address_space_mem);
     memory_region_destroy(&s->address_space_io);
     /* qbus_free() is called automatically by qdev_free() */
-    return 0;
 }
 
 /*
diff --git a/hw/pci_bridge.h b/hw/pci_bridge.h
index ac019a9..b37b885 100644
--- a/hw/pci_bridge.h
+++ b/hw/pci_bridge.h
@@ -44,7 +44,7 @@ void pci_bridge_reset_reg(PCIDevice *dev);
 void pci_bridge_reset(DeviceState *qdev);
 
 int pci_bridge_initfn(PCIDevice *pci_dev);
-int pci_bridge_exitfn(PCIDevice *pci_dev);
+void pci_bridge_exitfn(PCIDevice *pci_dev);
 
 
 /*
diff --git a/hw/pci_bridge_dev.c b/hw/pci_bridge_dev.c
index 600c39f..5d222aa 100644
--- a/hw/pci_bridge_dev.c
+++ b/hw/pci_bridge_dev.c
@@ -52,7 +52,7 @@ static int pci_bridge_dev_initfn(PCIDevice *dev)
 {
     PCIBridge *br = DO_UPCAST(PCIBridge, dev, dev);
     PCIBridgeDev *bridge_dev = DO_UPCAST(PCIBridgeDev, bridge, br);
-    int err, ret;
+    int err;
 
     pci_bridge_prepare(br, NULL, pci_bridge_dev_route_intx_pin);
     err = pci_bridge_initfn(dev);
@@ -87,26 +87,22 @@ slotid_error:
     shpc_cleanup(dev, &bridge_dev->bar);
 shpc_error:
     memory_region_destroy(&bridge_dev->bar);
-    ret = pci_bridge_exitfn(dev);
-    assert(!ret);
+    pci_bridge_exitfn(dev);
 bridge_error:
     return err;
 }
 
-static int pci_bridge_dev_exitfn(PCIDevice *dev)
+static void pci_bridge_dev_exitfn(PCIDevice *dev)
 {
     PCIBridge *br = DO_UPCAST(PCIBridge, dev, dev);
     PCIBridgeDev *bridge_dev = DO_UPCAST(PCIBridgeDev, bridge, br);
-    int ret;
     if (msi_present(dev)) {
         msi_uninit(dev);
     }
     slotid_cap_cleanup(dev);
     shpc_cleanup(dev, &bridge_dev->bar);
     memory_region_destroy(&bridge_dev->bar);
-    ret = pci_bridge_exitfn(dev);
-    assert(!ret);
-    return 0;
+    pci_bridge_exitfn(dev);
 }
 
 static void pci_bridge_dev_write_config(PCIDevice *d,
diff --git a/hw/pcnet-pci.c b/hw/pcnet-pci.c
index 34d73aa..5439db3 100644
--- a/hw/pcnet-pci.c
+++ b/hw/pcnet-pci.c
@@ -271,7 +271,7 @@ static void pci_pcnet_cleanup(VLANClientState *nc)
     pcnet_common_cleanup(d);
 }
 
-static int pci_pcnet_uninit(PCIDevice *dev)
+static void pci_pcnet_uninit(PCIDevice *dev)
 {
     PCIPCNetState *d = DO_UPCAST(PCIPCNetState, pci_dev, dev);
 
@@ -280,7 +280,6 @@ static int pci_pcnet_uninit(PCIDevice *dev)
     qemu_del_timer(d->state.poll_timer);
     qemu_free_timer(d->state.poll_timer);
     qemu_del_vlan_client(&d->state.nic->nc);
-    return 0;
 }
 
 static NetClientInfo net_pci_pcnet_info = {
diff --git a/hw/rtl8139.c b/hw/rtl8139.c
index 060404c..a7a3f27 100644
--- a/hw/rtl8139.c
+++ b/hw/rtl8139.c
@@ -3427,7 +3427,7 @@ static void rtl8139_cleanup(VLANClientState *nc)
     s->nic = NULL;
 }
 
-static int pci_rtl8139_uninit(PCIDevice *dev)
+static void pci_rtl8139_uninit(PCIDevice *dev)
 {
     RTL8139State *s = DO_UPCAST(RTL8139State, dev, dev);
 
@@ -3440,7 +3440,6 @@ static int pci_rtl8139_uninit(PCIDevice *dev)
     qemu_del_timer(s->timer);
     qemu_free_timer(s->timer);
     qemu_del_vlan_client(&s->nic->nc);
-    return 0;
 }
 
 static NetClientInfo net_rtl8139_info = {
diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index 9e211a0..04aabd9 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -1233,12 +1233,11 @@ static int usb_uhci_vt82c686b_initfn(PCIDevice *dev)
     return usb_uhci_common_initfn(dev);
 }
 
-static int usb_uhci_exit(PCIDevice *dev)
+static void usb_uhci_exit(PCIDevice *dev)
 {
     UHCIState *s = DO_UPCAST(UHCIState, dev, dev);
 
     memory_region_destroy(&s->io_bar);
-    return 0;
 }
 
 static Property uhci_properties[] = {
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 3dca37f..6ed21b7 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -828,22 +828,21 @@ static int virtio_blk_init_pci(PCIDevice *pci_dev)
     return 0;
 }
 
-static int virtio_exit_pci(PCIDevice *pci_dev)
+static void virtio_exit_pci(PCIDevice *pci_dev)
 {
     VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
 
     memory_region_destroy(&proxy->bar);
     msix_uninit_exclusive_bar(pci_dev);
-    return 0;
 }
 
-static int virtio_blk_exit_pci(PCIDevice *pci_dev)
+static void virtio_blk_exit_pci(PCIDevice *pci_dev)
 {
     VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
 
     virtio_pci_stop_ioeventfd(proxy);
     virtio_blk_exit(proxy->vdev);
-    return virtio_exit_pci(pci_dev);
+    virtio_exit_pci(pci_dev);
 }
 
 static int virtio_serial_init_pci(PCIDevice *pci_dev)
@@ -868,13 +867,13 @@ static int virtio_serial_init_pci(PCIDevice *pci_dev)
     return 0;
 }
 
-static int virtio_serial_exit_pci(PCIDevice *pci_dev)
+static void virtio_serial_exit_pci(PCIDevice *pci_dev)
 {
     VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
 
     virtio_pci_stop_ioeventfd(proxy);
     virtio_serial_exit(proxy->vdev);
-    return virtio_exit_pci(pci_dev);
+    virtio_exit_pci(pci_dev);
 }
 
 static int virtio_net_init_pci(PCIDevice *pci_dev)
@@ -892,13 +891,13 @@ static int virtio_net_init_pci(PCIDevice *pci_dev)
     return 0;
 }
 
-static int virtio_net_exit_pci(PCIDevice *pci_dev)
+static void virtio_net_exit_pci(PCIDevice *pci_dev)
 {
     VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
 
     virtio_pci_stop_ioeventfd(proxy);
     virtio_net_exit(proxy->vdev);
-    return virtio_exit_pci(pci_dev);
+    virtio_exit_pci(pci_dev);
 }
 
 static int virtio_balloon_init_pci(PCIDevice *pci_dev)
@@ -919,13 +918,13 @@ static int virtio_balloon_init_pci(PCIDevice *pci_dev)
     return 0;
 }
 
-static int virtio_balloon_exit_pci(PCIDevice *pci_dev)
+static void virtio_balloon_exit_pci(PCIDevice *pci_dev)
 {
     VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
 
     virtio_pci_stop_ioeventfd(proxy);
     virtio_balloon_exit(proxy->vdev);
-    return virtio_exit_pci(pci_dev);
+    virtio_exit_pci(pci_dev);
 }
 
 static Property virtio_blk_properties[] = {
@@ -1074,12 +1073,12 @@ static int virtio_scsi_init_pci(PCIDevice *pci_dev)
     return 0;
 }
 
-static int virtio_scsi_exit_pci(PCIDevice *pci_dev)
+static void virtio_scsi_exit_pci(PCIDevice *pci_dev)
 {
     VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
 
     virtio_scsi_exit(proxy->vdev);
-    return virtio_exit_pci(pci_dev);
+    virtio_exit_pci(pci_dev);
 }
 
 static Property virtio_scsi_properties[] = {
diff --git a/hw/wdt_i6300esb.c b/hw/wdt_i6300esb.c
index 15c69db..4a83474 100644
--- a/hw/wdt_i6300esb.c
+++ b/hw/wdt_i6300esb.c
@@ -411,13 +411,11 @@ static int i6300esb_init(PCIDevice *dev)
     return 0;
 }
 
-static int i6300esb_exit(PCIDevice *dev)
+static void i6300esb_exit(PCIDevice *dev)
 {
     I6300State *d = DO_UPCAST(I6300State, dev, dev);
 
     memory_region_destroy(&d->io_mem);
-
-    return 0;
 }
 
 static WatchdogTimerModel model = {
diff --git a/hw/xio3130_downstream.c b/hw/xio3130_downstream.c
index d215dc8..91993f7 100644
--- a/hw/xio3130_downstream.c
+++ b/hw/xio3130_downstream.c
@@ -60,7 +60,6 @@ static int xio3130_downstream_initfn(PCIDevice *d)
     PCIEPort *p = DO_UPCAST(PCIEPort, br, br);
     PCIESlot *s = DO_UPCAST(PCIESlot, port, p);
     int rc;
-    int tmp;
 
     rc = pci_bridge_initfn(d);
     if (rc < 0) {
@@ -108,12 +107,11 @@ err_pcie_cap:
 err_msi:
     msi_uninit(d);
 err_bridge:
-    tmp = pci_bridge_exitfn(d);
-    assert(!tmp);
+    pci_bridge_exitfn(d);
     return rc;
 }
 
-static int xio3130_downstream_exitfn(PCIDevice *d)
+static void xio3130_downstream_exitfn(PCIDevice *d)
 {
     PCIBridge* br = DO_UPCAST(PCIBridge, dev, d);
     PCIEPort *p = DO_UPCAST(PCIEPort, br, br);
@@ -123,7 +121,7 @@ static int xio3130_downstream_exitfn(PCIDevice *d)
     pcie_chassis_del_slot(s);
     pcie_cap_exit(d);
     msi_uninit(d);
-    return pci_bridge_exitfn(d);
+    pci_bridge_exitfn(d);
 }
 
 PCIESlot *xio3130_downstream_init(PCIBus *bus, int devfn, bool multifunction,
diff --git a/hw/xio3130_upstream.c b/hw/xio3130_upstream.c
index 80bff7f..503b79e 100644
--- a/hw/xio3130_upstream.c
+++ b/hw/xio3130_upstream.c
@@ -56,7 +56,6 @@ static int xio3130_upstream_initfn(PCIDevice *d)
     PCIBridge* br = DO_UPCAST(PCIBridge, dev, d);
     PCIEPort *p = DO_UPCAST(PCIEPort, br, br);
     int rc;
-    int tmp;
 
     rc = pci_bridge_initfn(d);
     if (rc < 0) {
@@ -95,17 +94,16 @@ err:
 err_msi:
     msi_uninit(d);
 err_bridge:
-    tmp =  pci_bridge_exitfn(d);
-    assert(!tmp);
+    pci_bridge_exitfn(d);
     return rc;
 }
 
-static int xio3130_upstream_exitfn(PCIDevice *d)
+static void xio3130_upstream_exitfn(PCIDevice *d)
 {
     pcie_aer_exit(d);
     pcie_cap_exit(d);
     msi_uninit(d);
-    return pci_bridge_exitfn(d);
+    pci_bridge_exitfn(d);
 }
 
 PCIEPort *xio3130_upstream_init(PCIBus *bus, int devfn, bool multifunction,

[Qemu-devel] [PATCH 2/2] pci: Unregister BARs before device exit

[Alex Williamson]

BARs are registered in init functions from memory regions created
by the drivers.  Exit functions destroy those memory regions.
By unregistering the io regions after exit(), we're calling
memory_region_del_subregion on freed memory.  Don't do that.  The
option rom comes along for the ride because it's more symmetric
to how it's created.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

 hw/pci.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index 8934722..1fc73f8 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -875,12 +875,13 @@ static int pci_unregister_device(DeviceState *dev)
     PCIDevice *pci_dev = PCI_DEVICE(dev);
     PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(pci_dev);
 
+    pci_unregister_io_regions(pci_dev);
+    pci_del_option_rom(pci_dev);
+
     if (pc->exit) {
         pc->exit(pci_dev);
     }
 
-    pci_unregister_io_regions(pci_dev);
-    pci_del_option_rom(pci_dev);
     do_pci_unregister_device(pci_dev);
     return 0;
 }

Re: [Qemu-devel] [PATCH 0/2] pci: exit path cleanup and fix

[Paolo Bonzini]

Il 04/07/2012 06:39, Alex Williamson ha scritto:
> We've currently got a bug in pci_unregister_device in the ordering of
> calling the driver exit function and unregistering io regions.  In
> every driver memory regions are created in the init function and
> destroyed in the exit function.  By calling pci_unregister_io_regions
> after the exit function, we're calling memory_region_del_subregion
> with a pointer to a MemoryRegion that has already been destroyed.
> 
> It's easy enough to change the ordering, but the exit function is
> currently allowed to fail.  Even if we wanted to restore the device
> at that point, we've interrupted the mappings from the guest
> perspective and it seems precarious at best whether an exit function
> can fail and leave a usable device.  Fortunately nobody has any
> possibility of actually failing the exit path.  Normally I'm a
> proponent of error paths, but allowing an exit to fail is like
> allowing free(3) to fail.
> 
> So, firt redefine that exit can't fail, then fix the ordering of
> pci_unregister_device().  If anyone has plans for a failure case in
> the exit path, please speak now.  Thanks,
> 
> Alex
> 
> ---
> 
> Alex Williamson (2):
>       pci: Unregister BARs before device exit
>       pci: convert PCIUnregisterFunc to void
> 
> 
>  hw/ac97.c               |    3 +--
>  hw/e1000.c              |    3 +--
>  hw/eepro100.c           |    3 +--
>  hw/es1370.c             |    3 +--
>  hw/ide/cmd646.c         |    4 +---
>  hw/ide/ich.c            |    4 +---
>  hw/ide/piix.c           |    4 +---
>  hw/ide/via.c            |    4 +---
>  hw/intel-hda.c          |    3 +--
>  hw/ioh3420.c            |    8 +++-----
>  hw/ivshmem.c            |    4 +---
>  hw/lsi53c895a.c         |    4 +---
>  hw/ne2000.c             |    3 +--
>  hw/pci.c                |   11 +++++------
>  hw/pci.h                |    2 +-
>  hw/pci_bridge.c         |    3 +--
>  hw/pci_bridge.h         |    2 +-
>  hw/pci_bridge_dev.c     |   12 ++++--------
>  hw/pcnet-pci.c          |    3 +--
>  hw/rtl8139.c            |    3 +--
>  hw/usb/hcd-uhci.c       |    3 +--
>  hw/virtio-pci.c         |   23 +++++++++++------------
>  hw/wdt_i6300esb.c       |    4 +---
>  hw/xio3130_downstream.c |    8 +++-----
>  hw/xio3130_upstream.c   |    8 +++-----
>  25 files changed, 48 insertions(+), 84 deletions(-)
> 
> 

The diffstat caught my eye. :)

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH 0/2] pci: exit path cleanup and fix

[Michael S. Tsirkin]

On Tue, Jul 03, 2012 at 10:39:20PM -0600, Alex Williamson wrote:
> We've currently got a bug in pci_unregister_device in the ordering of
> calling the driver exit function and unregistering io regions.  In
> every driver memory regions are created in the init function and
> destroyed in the exit function.  By calling pci_unregister_io_regions
> after the exit function, we're calling memory_region_del_subregion
> with a pointer to a MemoryRegion that has already been destroyed.
> 
> It's easy enough to change the ordering, but the exit function is
> currently allowed to fail.  Even if we wanted to restore the device
> at that point, we've interrupted the mappings from the guest
> perspective and it seems precarious at best whether an exit function
> can fail and leave a usable device.  Fortunately nobody has any
> possibility of actually failing the exit path.  Normally I'm a
> proponent of error paths, but allowing an exit to fail is like
> allowing free(3) to fail.

Like, totally.

> So, firt redefine that exit can't fail, then fix the ordering of
> pci_unregister_device().  If anyone has plans for a failure case in
> the exit path, please speak now.  Thanks,
> 
> Alex

Applied, thanks!

> ---
> 
> Alex Williamson (2):
>       pci: Unregister BARs before device exit
>       pci: convert PCIUnregisterFunc to void
> 
> 
>  hw/ac97.c               |    3 +--
>  hw/e1000.c              |    3 +--
>  hw/eepro100.c           |    3 +--
>  hw/es1370.c             |    3 +--
>  hw/ide/cmd646.c         |    4 +---
>  hw/ide/ich.c            |    4 +---
>  hw/ide/piix.c           |    4 +---
>  hw/ide/via.c            |    4 +---
>  hw/intel-hda.c          |    3 +--
>  hw/ioh3420.c            |    8 +++-----
>  hw/ivshmem.c            |    4 +---
>  hw/lsi53c895a.c         |    4 +---
>  hw/ne2000.c             |    3 +--
>  hw/pci.c                |   11 +++++------
>  hw/pci.h                |    2 +-
>  hw/pci_bridge.c         |    3 +--
>  hw/pci_bridge.h         |    2 +-
>  hw/pci_bridge_dev.c     |   12 ++++--------
>  hw/pcnet-pci.c          |    3 +--
>  hw/rtl8139.c            |    3 +--
>  hw/usb/hcd-uhci.c       |    3 +--
>  hw/virtio-pci.c         |   23 +++++++++++------------
>  hw/wdt_i6300esb.c       |    4 +---
>  hw/xio3130_downstream.c |    8 +++-----
>  hw/xio3130_upstream.c   |    8 +++-----
>  25 files changed, 48 insertions(+), 84 deletions(-)