[RFC,05/15] spapr_pci: add PHB unrealize

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(+)

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