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 <mdr...@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;
> >
>
