Re: [qemu-s390x] [PATCH 03/15] pci: allow cleanup/unregistration of PCI root buses

[Michael S. Tsirkin]

On Fri, Dec 21, 2018 at 01:35:30AM +0100, Greg Kurz wrote:
> From: Michael Roth <address@hidden>
> 
> This adds cleanup counterparts to pci_register_root_bus(),
> pci_root_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 <address@hidden>
> Cc: Paolo Bonzini <address@hidden>
> Signed-off-by: Michael Roth <address@hidden>
> Reviewed-by: David Gibson <address@hidden>
> Signed-off-by: Greg Kurz <address@hidden>

Reviewed-by: Michael S. Tsirkin <address@hidden>

> ---
>  hw/pci/pci.c         |   33 +++++++++++++++++++++++++++++++++
>  include/hw/pci/pci.h |    3 +++
>  2 files changed, 36 insertions(+)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index efb5ce196ffb..16354f91206c 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -333,6 +333,13 @@ static void pci_host_bus_register(DeviceState *host)
>      QLIST_INSERT_HEAD(&pci_host_bridges, host_bridge, next);
>  }
>  
> +static void pci_host_bus_unregister(DeviceState *host)
> +{
> +    PCIHostState *host_bridge = PCI_HOST_BRIDGE(host);
> +
> +    QLIST_REMOVE(host_bridge, next);
> +}
> +
>  PCIBus *pci_device_root_bus(const PCIDevice *d)
>  {
>      PCIBus *bus = pci_get_bus(d);
> @@ -379,6 +386,11 @@ static void pci_root_bus_init(PCIBus *bus, DeviceState 
> *parent,
>      pci_host_bus_register(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);
> @@ -413,6 +425,12 @@ PCIBus *pci_root_bus_new(DeviceState *parent, const char 
> *name,
>      return bus;
>  }
>  
> +void pci_root_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)
>  {
> @@ -423,6 +441,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_root_bus(DeviceState *parent, const char *name,
>                                pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
>                                void *irq_opaque,
> @@ -439,6 +466,12 @@ PCIBus *pci_register_root_bus(DeviceState *parent, const 
> char *name,
>      return bus;
>  }
>  
> +void pci_unregister_root_bus(PCIBus *bus)
> +{
> +    pci_bus_irqs_cleanup(bus);
> +    pci_root_bus_cleanup(bus);
> +}
> +
>  int pci_bus_num(PCIBus *s)
>  {
>      return PCI_BUS_GET_CLASS(s)->bus_num(s);
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index e6514bba23aa..8998e3be3390 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -405,8 +405,10 @@ PCIBus *pci_root_bus_new(DeviceState *parent, const char 
> *name,
>                           MemoryRegion *address_space_mem,
>                           MemoryRegion *address_space_io,
>                           uint8_t devfn_min, const char *typename);
> +void pci_root_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);
> @@ -417,6 +419,7 @@ PCIBus *pci_register_root_bus(DeviceState *parent, const 
> char *name,
>                                MemoryRegion *address_space_io,
>                                uint8_t devfn_min, int nirq,
>                                const char *typename);
> +void pci_unregister_root_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);