Re: [Qemu-devel] [PATCH 03/15] pci: allow cleanup/unregistration of PCI root buses
On Wed, Jan 02, 2019 at 10:27:14PM -0500, Michael S. Tsirkin wrote: > On Thu, Jan 03, 2019 at 11:36:33AM +1100, David Gibson wrote: > > On Fri, Dec 21, 2018 at 11:19:18AM -0500, Michael S. Tsirkin wrote: > > > On Fri, Dec 21, 2018 at 01:35:30AM +0100, Greg Kurz wrote: > > > > From: Michael Roth > > > > > > > > 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 > > > > Cc: Paolo Bonzini > > > > Signed-off-by: Michael Roth > > > > Reviewed-by: David Gibson > > > > Signed-off-by: Greg Kurz > > > > > > Reviewed-by: Michael S. Tsirkin > > > > I've applied this tentatively to ppc-for-4.0. Let me know, Michael, > > if you'd prefer to take it through your tree. > > > I think your tree makes sense for this. Great, will do. > > > > > > > > --- > > > > 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(_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, > > > >
Re: [Qemu-devel] [PATCH 03/15] pci: allow cleanup/unregistration of PCI root buses
On Thu, Jan 03, 2019 at 11:36:33AM +1100, David Gibson wrote: > On Fri, Dec 21, 2018 at 11:19:18AM -0500, Michael S. Tsirkin wrote: > > On Fri, Dec 21, 2018 at 01:35:30AM +0100, Greg Kurz wrote: > > > From: Michael Roth > > > > > > 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 > > > Cc: Paolo Bonzini > > > Signed-off-by: Michael Roth > > > Reviewed-by: David Gibson > > > Signed-off-by: Greg Kurz > > > > Reviewed-by: Michael S. Tsirkin > > I've applied this tentatively to ppc-for-4.0. Let me know, Michael, > if you'd prefer to take it through your tree. I think your tree makes sense for this. > > > > > --- > > > 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(_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
Re: [Qemu-devel] [PATCH 03/15] pci: allow cleanup/unregistration of PCI root buses
On Fri, Dec 21, 2018 at 11:19:18AM -0500, Michael S. Tsirkin wrote: > On Fri, Dec 21, 2018 at 01:35:30AM +0100, Greg Kurz wrote: > > From: Michael Roth > > > > 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 > > Cc: Paolo Bonzini > > Signed-off-by: Michael Roth > > Reviewed-by: David Gibson > > Signed-off-by: Greg Kurz > > Reviewed-by: Michael S. Tsirkin I've applied this tentatively to ppc-for-4.0. Let me know, Michael, if you'd prefer to take it through your tree. > > > --- > > 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(_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, > >
Re: [Qemu-devel] [PATCH 03/15] pci: allow cleanup/unregistration of PCI root buses
On Fri, Dec 21, 2018 at 01:35:30AM +0100, Greg Kurz wrote: > From: Michael Roth > > 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 > Cc: Paolo Bonzini > Signed-off-by: Michael Roth > Reviewed-by: David Gibson > Signed-off-by: Greg Kurz Reviewed-by: Michael S. Tsirkin > --- > 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(_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);