From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54250) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YnzbG-0004pk-JX for qemu-devel@nongnu.org; Thu, 30 Apr 2015 21:18:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YnzbD-0003lX-C1 for qemu-devel@nongnu.org; Thu, 30 Apr 2015 21:18:42 -0400 Received: from e8.ny.us.ibm.com ([32.97.182.138]:50865) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YnzbD-0003lN-81 for qemu-devel@nongnu.org; Thu, 30 Apr 2015 21:18:39 -0400 Received: from /spool/local by e8.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 30 Apr 2015 21:18:37 -0400 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable From: Michael Roth In-Reply-To: <5542369D.90801@redhat.com> References: <1430335224-6716-1-git-send-email-mdroth@linux.vnet.ibm.com> <1430335224-6716-6-git-send-email-mdroth@linux.vnet.ibm.com> <5542369D.90801@redhat.com> Message-ID: <20150501011829.25451.23595@loki> Date: Thu, 30 Apr 2015 20:18:29 -0500 Subject: Re: [Qemu-devel] [RFC PATCH 05/15] spapr_pci: add PHB unrealize List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini , qemu-devel@nongnu.org Cc: aik@ozlabs.ru, nfont@linux.vnet.ibm.com, david@gibson.dropbear.id.au, qemu-ppc@nongnu.org, bharata@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 > > --- > > 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(HotplugHa= ndler *plug_handler, > > } > > } > > = > > +static void spapr_phb_unrealize(DeviceState *dev, Error **errp) > > +{ > > + SysBusDevice *s =3D SYS_BUS_DEVICE(dev); > > + PCIHostState *phb =3D PCI_HOST_BRIDGE(s); > > + sPAPRPHBState *sphb =3D SPAPR_PCI_HOST_BRIDGE(phb); > > + sPAPRTCETable *tcet; > > + > > + pci_unregister_bus(phb->bus); > > + > > + g_free(sphb->dtbusname); > > + sphb->dtbusname =3D NULL; > = > This g_free can probably also be moved for simplicity to instance_finaliz= e. > = > > + /* 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 =3D 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 =3D SYS_BUS_DEVICE(dev); > > @@ -1442,6 +1473,7 @@ static void spapr_phb_class_init(ObjectClass *kla= ss, void *data) > > = > > hc->root_bus_path =3D spapr_phb_root_bus_path; > > dc->realize =3D spapr_phb_realize; > > + dc->unrealize =3D spapr_phb_unrealize; > > dc->props =3D spapr_phb_properties; > > dc->reset =3D spapr_phb_reset; > > dc->vmsd =3D &vmstate_spapr_pci; > > = >=20