diff mbox

[RFC,01/15] pci: allow cleanup/unregistration of PCI buses

Message ID 1430335224-6716-2-git-send-email-mdroth@linux.vnet.ibm.com
State New
Headers show

Commit Message

Michael Roth April 29, 2015, 7:20 p.m. UTC 
This adds cleanup counterparts to pci_register_bus(),
pci_bus_new(), and pci_bus_irqs().

These cleanup routines are needed in the case of hotpluggable
PCIHostBridge implementations. Currently we can rely on the
object_unparent()'ing of the PCIHostState recursively unparenting
and cleaning up it's child buses, but we need explicit calls
to also:

  1) remove the PCIHostState from pci_host_bridges global list.
     otherwise, we risk accessing freed memory when we access
     the list later
  2) clean up memory allocated in pci_bus_irqs()

Both are handled outside the context of any particular bus or
host bridge's init/realize functions, making it difficult to
avoid the need for explicit cleanup functions without remodeling
how PCIHostBridges are created. So keep it simple and just add
them for now.

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 hw/pci/pci.c         | 33 +++++++++++++++++++++++++++++++++
 include/hw/pci/pci.h |  3 +++
 2 files changed, 36 insertions(+)

Comments

David Gibson May 5, 2015, 7:56 a.m. UTC | #1 
On Wed, Apr 29, 2015 at 02:20:10PM -0500, Michael Roth wrote:
> This adds cleanup counterparts to pci_register_bus(),
> pci_bus_new(), and pci_bus_irqs().
> 
> These cleanup routines are needed in the case of hotpluggable
> PCIHostBridge implementations. Currently we can rely on the
> object_unparent()'ing of the PCIHostState recursively unparenting
> and cleaning up it's child buses, but we need explicit calls
> to also:
> 
>   1) remove the PCIHostState from pci_host_bridges global list.
>      otherwise, we risk accessing freed memory when we access
>      the list later
>   2) clean up memory allocated in pci_bus_irqs()
> 
> Both are handled outside the context of any particular bus or
> host bridge's init/realize functions, making it difficult to
> avoid the need for explicit cleanup functions without remodeling
> how PCIHostBridges are created. So keep it simple and just add
> them for now.
> 
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

As with Bharata's cpu and memory hotplug series, you may want to split
out those patches which are reasonable cleanups regardless of exactly
what happens with the hotplug code itself.
diff mbox

Patch

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index a88c8f3..201ad49 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -258,6 +258,13 @@  static void pci_host_bus_register(PCIBus *bus, DeviceState *parent)
     QLIST_INSERT_HEAD(&pci_host_bridges, host_bridge, next);
 }
 
+static void pci_host_bus_unregister(DeviceState *parent)
+{
+    PCIHostState *host_bridge = PCI_HOST_BRIDGE(parent);
+
+    QLIST_REMOVE(host_bridge, next);
+}
+
 PCIBus *pci_find_primary_bus(void)
 {
     PCIBus *primary_bus = NULL;
@@ -318,6 +325,11 @@  static void pci_bus_init(PCIBus *bus, DeviceState *parent,
     pci_host_bus_register(bus, parent);
 }
 
+static void pci_bus_uninit(PCIBus *bus)
+{
+    pci_host_bus_unregister(BUS(bus)->parent);
+}
+
 bool pci_bus_is_express(PCIBus *bus)
 {
     return object_dynamic_cast(OBJECT(bus), TYPE_PCIE_BUS);
@@ -352,6 +364,12 @@  PCIBus *pci_bus_new(DeviceState *parent, const char *name,
     return bus;
 }
 
+void pci_bus_cleanup(PCIBus *bus)
+{
+    pci_bus_uninit(bus);
+    object_unparent(OBJECT(bus));
+}
+
 void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
                   void *irq_opaque, int nirq)
 {
@@ -362,6 +380,15 @@  void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
     bus->irq_count = g_malloc0(nirq * sizeof(bus->irq_count[0]));
 }
 
+void pci_bus_irqs_cleanup(PCIBus *bus)
+{
+    bus->set_irq = NULL;
+    bus->map_irq = NULL;
+    bus->irq_opaque = NULL;
+    bus->nirq = 0;
+    g_free(bus->irq_count);
+}
+
 PCIBus *pci_register_bus(DeviceState *parent, const char *name,
                          pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
                          void *irq_opaque,
@@ -377,6 +404,12 @@  PCIBus *pci_register_bus(DeviceState *parent, const char *name,
     return bus;
 }
 
+void pci_unregister_bus(PCIBus *bus)
+{
+    pci_bus_irqs_cleanup(bus);
+    pci_bus_cleanup(bus);
+}
+
 int pci_bus_num(PCIBus *s)
 {
     if (pci_bus_is_root(s))
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 1a4e0be..eb4a292 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -353,8 +353,10 @@  PCIBus *pci_bus_new(DeviceState *parent, const char *name,
                     MemoryRegion *address_space_mem,
                     MemoryRegion *address_space_io,
                     uint8_t devfn_min, const char *typename);
+void pci_bus_cleanup(PCIBus *bus);
 void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
                   void *irq_opaque, int nirq);
+void pci_bus_irqs_cleanup(PCIBus *bus);
 int pci_bus_get_irq_level(PCIBus *bus, int irq_num);
 /* 0 <= pin <= 3 0 = INTA, 1 = INTB, 2 = INTC, 3 = INTD */
 int pci_swizzle_map_irq_fn(PCIDevice *pci_dev, int pin);
@@ -364,6 +366,7 @@  PCIBus *pci_register_bus(DeviceState *parent, const char *name,
                          MemoryRegion *address_space_mem,
                          MemoryRegion *address_space_io,
                          uint8_t devfn_min, int nirq, const char *typename);
+void pci_unregister_bus(PCIBus *bus);
 void pci_bus_set_route_irq_fn(PCIBus *, pci_route_irq_fn);
 PCIINTxRoute pci_device_route_intx_to_irq(PCIDevice *dev, int pin);
 bool pci_intx_route_changed(PCIINTxRoute *old, PCIINTxRoute *new);