Message ID | 1430335224-6716-6-git-send-email-mdroth@linux.vnet.ibm.com |
---|---|
State | New |
Headers | show |
On 29/04/2015 21:20, Michael Roth wrote: > To support PHB hotplug we need to clean up lingering references, > memory, child properties, etc. prior to the PHB object being > finalized. Generally this will be called as a result of calling > object_unref() on the PHB object, which in turn would normally s/object_unref/object_unparent/ > be called as the result of an unplug() operation. > > When the PHB is finalized, child objects will be unparented in > turn, and finalized if the PHB was the only reference holder. so > we don't bother to explicitly unparent child objects of the PHB > (spapr_iommu, spapr_drc, etc). > > We do need to handle memory regions explicitly however, since > they also take a reference on the PHB, and won't allow it to > be finalized otherwise. They shouldn't hold a reference anymore as soon as the regions are not visible in an AddressSpace (and the RCU thread has picked up the changes). In fact, docs/memory.txt documents (!) that you must call object_unparent() for memory regions in the instance_finalize function, not in the unrealize function. > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com> > --- > hw/ppc/spapr_pci.c | 32 ++++++++++++++++++++++++++++++++ > 1 file changed, 32 insertions(+) > > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > index 2e7590c..25a738c 100644 > --- a/hw/ppc/spapr_pci.c > +++ b/hw/ppc/spapr_pci.c > @@ -1108,6 +1108,37 @@ static void spapr_phb_hot_unplug_child(HotplugHandler *plug_handler, > } > } > > +static void spapr_phb_unrealize(DeviceState *dev, Error **errp) > +{ > + SysBusDevice *s = SYS_BUS_DEVICE(dev); > + PCIHostState *phb = PCI_HOST_BRIDGE(s); > + sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(phb); > + sPAPRTCETable *tcet; > + > + pci_unregister_bus(phb->bus); > + > + g_free(sphb->dtbusname); > + sphb->dtbusname = NULL; This g_free can probably also be moved for simplicity to instance_finalize. > + /* remove IO/MMIO subregions and aliases, rest should get cleaned > + * via PHB's unrealize->object_finalize > + */ > + memory_region_del_subregion(get_system_memory(), &sphb->iowindow); ^^ You should indeed do this here. > + object_unparent(OBJECT(&sphb->iowindow)); > + object_unparent(OBJECT(&sphb->iospace)); > + > + memory_region_del_subregion(get_system_memory(), &sphb->memwindow); ^^ and this > + object_unparent(OBJECT(&sphb->memwindow)); > + object_unparent(OBJECT(&sphb->memspace)); > + > + tcet = spapr_tce_find_by_liobn(sphb->dma_liobn); > + memory_region_del_subregion(&sphb->iommu_root, &sphb->msiwindow); > + memory_region_del_subregion(&sphb->iommu_root, spapr_tce_get_iommu(tcet)); > + address_space_destroy(&sphb->iommu_as); ^^ and these three. However, the object_unparents should be in instance_finalize. Paolo > + QLIST_REMOVE(sphb, list); > +} > + > static void spapr_phb_realize(DeviceState *dev, Error **errp) > { > SysBusDevice *s = SYS_BUS_DEVICE(dev); > @@ -1442,6 +1473,7 @@ static void spapr_phb_class_init(ObjectClass *klass, void *data) > > hc->root_bus_path = spapr_phb_root_bus_path; > dc->realize = spapr_phb_realize; > + dc->unrealize = spapr_phb_unrealize; > dc->props = spapr_phb_properties; > dc->reset = spapr_phb_reset; > dc->vmsd = &vmstate_spapr_pci; >
Quoting Paolo Bonzini (2015-04-30 09:05:17) > > > On 29/04/2015 21:20, Michael Roth wrote: > > To support PHB hotplug we need to clean up lingering references, > > memory, child properties, etc. prior to the PHB object being > > finalized. Generally this will be called as a result of calling > > object_unref() on the PHB object, which in turn would normally > > s/object_unref/object_unparent/ > > > be called as the result of an unplug() operation. > > > > When the PHB is finalized, child objects will be unparented in > > turn, and finalized if the PHB was the only reference holder. so > > we don't bother to explicitly unparent child objects of the PHB > > (spapr_iommu, spapr_drc, etc). > > > > We do need to handle memory regions explicitly however, since > > they also take a reference on the PHB, and won't allow it to > > be finalized otherwise. > > They shouldn't hold a reference anymore as soon as the regions are not > visible in an AddressSpace (and the RCU thread has picked up the changes). Sorry, I mixed up memory regions with memory region alias. Memory region aliases do a memory_region_ref() on the original MR, similar to memory_region_add_subregion(), so that's what ends up creating the reference to the owner/PHB. So I think I do need to object_unparent() the 2 MR aliases in realize (otherwise the PHB doesn't get finalized), but everything else can get moved to instance_finalize() as you suggested and that seems to do the trick. > > In fact, docs/memory.txt documents (!) that you must call > object_unparent() for memory regions in the instance_finalize function, > not in the unrealize function. They seem to hint that creation should follow the same guidelines, so I assume I should probably moved all the memory_region_init()'s to instance_init()? I see a lot of counter-examples elsewhere, but not sure if those are intended or not. > > > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com> > > --- > > hw/ppc/spapr_pci.c | 32 ++++++++++++++++++++++++++++++++ > > 1 file changed, 32 insertions(+) > > > > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > > index 2e7590c..25a738c 100644 > > --- a/hw/ppc/spapr_pci.c > > +++ b/hw/ppc/spapr_pci.c > > @@ -1108,6 +1108,37 @@ static void spapr_phb_hot_unplug_child(HotplugHandler *plug_handler, > > } > > } > > > > +static void spapr_phb_unrealize(DeviceState *dev, Error **errp) > > +{ > > + SysBusDevice *s = SYS_BUS_DEVICE(dev); > > + PCIHostState *phb = PCI_HOST_BRIDGE(s); > > + sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(phb); > > + sPAPRTCETable *tcet; > > + > > + pci_unregister_bus(phb->bus); > > + > > + g_free(sphb->dtbusname); > > + sphb->dtbusname = NULL; > > This g_free can probably also be moved for simplicity to instance_finalize. > > > + /* remove IO/MMIO subregions and aliases, rest should get cleaned > > + * via PHB's unrealize->object_finalize > > + */ > > + memory_region_del_subregion(get_system_memory(), &sphb->iowindow); > > ^^ You should indeed do this here. > > > + object_unparent(OBJECT(&sphb->iowindow)); > > + object_unparent(OBJECT(&sphb->iospace)); > > + > > + memory_region_del_subregion(get_system_memory(), &sphb->memwindow); > > ^^ and this > > > + object_unparent(OBJECT(&sphb->memwindow)); > > + object_unparent(OBJECT(&sphb->memspace)); > > + > > + tcet = spapr_tce_find_by_liobn(sphb->dma_liobn); > > + memory_region_del_subregion(&sphb->iommu_root, &sphb->msiwindow); > > + memory_region_del_subregion(&sphb->iommu_root, spapr_tce_get_iommu(tcet)); > > + address_space_destroy(&sphb->iommu_as); > > ^^ and these three. However, the object_unparents should be in > instance_finalize. > > Paolo > > > + QLIST_REMOVE(sphb, list); > > +} > > + > > static void spapr_phb_realize(DeviceState *dev, Error **errp) > > { > > SysBusDevice *s = SYS_BUS_DEVICE(dev); > > @@ -1442,6 +1473,7 @@ static void spapr_phb_class_init(ObjectClass *klass, void *data) > > > > hc->root_bus_path = spapr_phb_root_bus_path; > > dc->realize = spapr_phb_realize; > > + dc->unrealize = spapr_phb_unrealize; > > dc->props = spapr_phb_properties; > > dc->reset = spapr_phb_reset; > > dc->vmsd = &vmstate_spapr_pci; > > >
On 01/05/2015 03:18, Michael Roth wrote: > Sorry, I mixed up memory regions with memory region alias. Memory region > aliases do a memory_region_ref() on the original MR, similar to > memory_region_add_subregion(), so that's what ends up creating the > reference to the owner/PHB. > > So I think I do need to object_unparent() the 2 MR aliases in realize > (otherwise the PHB doesn't get finalized), but everything else can > get moved to instance_finalize() as you suggested and that seems to > do the trick. Yes, unparenting the aliases in unrealize is okay and in fact may be required to avoid a leak. Patching docs/memory.txt to point it out would be great. :) Paolo
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c index 2e7590c..25a738c 100644 --- a/hw/ppc/spapr_pci.c +++ b/hw/ppc/spapr_pci.c @@ -1108,6 +1108,37 @@ static void spapr_phb_hot_unplug_child(HotplugHandler *plug_handler, } } +static void spapr_phb_unrealize(DeviceState *dev, Error **errp) +{ + SysBusDevice *s = SYS_BUS_DEVICE(dev); + PCIHostState *phb = PCI_HOST_BRIDGE(s); + sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(phb); + sPAPRTCETable *tcet; + + pci_unregister_bus(phb->bus); + + g_free(sphb->dtbusname); + sphb->dtbusname = NULL; + + /* remove IO/MMIO subregions and aliases, rest should get cleaned + * via PHB's unrealize->object_finalize + */ + memory_region_del_subregion(get_system_memory(), &sphb->iowindow); + object_unparent(OBJECT(&sphb->iowindow)); + object_unparent(OBJECT(&sphb->iospace)); + + memory_region_del_subregion(get_system_memory(), &sphb->memwindow); + object_unparent(OBJECT(&sphb->memwindow)); + object_unparent(OBJECT(&sphb->memspace)); + + tcet = spapr_tce_find_by_liobn(sphb->dma_liobn); + memory_region_del_subregion(&sphb->iommu_root, &sphb->msiwindow); + memory_region_del_subregion(&sphb->iommu_root, spapr_tce_get_iommu(tcet)); + address_space_destroy(&sphb->iommu_as); + + QLIST_REMOVE(sphb, list); +} + static void spapr_phb_realize(DeviceState *dev, Error **errp) { SysBusDevice *s = SYS_BUS_DEVICE(dev); @@ -1442,6 +1473,7 @@ static void spapr_phb_class_init(ObjectClass *klass, void *data) hc->root_bus_path = spapr_phb_root_bus_path; dc->realize = spapr_phb_realize; + dc->unrealize = spapr_phb_unrealize; dc->props = spapr_phb_properties; dc->reset = spapr_phb_reset; dc->vmsd = &vmstate_spapr_pci;
To support PHB hotplug we need to clean up lingering references, memory, child properties, etc. prior to the PHB object being finalized. Generally this will be called as a result of calling object_unref() on the PHB object, which in turn would normally be called as the result of an unplug() operation. When the PHB is finalized, child objects will be unparented in turn, and finalized if the PHB was the only reference holder. so we don't bother to explicitly unparent child objects of the PHB (spapr_iommu, spapr_drc, etc). We do need to handle memory regions explicitly however, since they also take a reference on the PHB, and won't allow it to be finalized otherwise. Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com> --- hw/ppc/spapr_pci.c | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+)