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