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

[Paolo Bonzini <pbonzini@redhat.com>]

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