Re: [Qemu-devel] [RFC PATCH 05/15] spapr_pci: add PHB unrealize

[Michael Roth <mdroth@linux.vnet.ibm.com>]

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;
> > 
>