On Thu, 3 Jan 2019 12:59:06 +1100 David Gibson <da...@gibson.dropbear.id.au> wrote:
> On Fri, Dec 21, 2018 at 01:35:52AM +0100, Greg Kurz wrote: > > The current realize code assumes the PHB is coldplugged, ie, QEMU will > > terminate if an error is detected, and does not bother to free anything > > it has already allocated. > > > > In order to support PHB hotplug, let's first ensure spapr_phb_realize() > > doesn't leak anything in case of error. > > > > Signed-off-by: Greg Kurz <gr...@kaod.org> > > This looks ok as far as it goes. However, it looks like there will be > a lot of fragments duplicated between the rollback paths and > unrealize() when it's added in the next patch. > > A common pattern to avoid that is to make unrealize() safe to call on > a partially realized object, then call that from the realize() failure > paths. Is it possible to do that here? I imagine that would involve > folding this patch with the next as well. > Makes sense. I'll give a try. > > --- > > hw/ppc/spapr_pci.c | 40 +++++++++++++++++++++++++++++++++++----- > > 1 file changed, 35 insertions(+), 5 deletions(-) > > > > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > > index e59adbe706bb..46d7062dd143 100644 > > --- a/hw/ppc/spapr_pci.c > > +++ b/hw/ppc/spapr_pci.c > > @@ -1570,6 +1570,7 @@ static void spapr_phb_realize(DeviceState *dev, Error > > **errp) > > sPAPRTCETable *tcet; > > const unsigned windows_supported = > > sphb->ddw_enabled ? SPAPR_PCI_DMA_MAX_WINDOWS : 1; > > + Object *drcs[PCI_SLOT_MAX * 8]; > > > > if (!spapr) { > > error_setg(errp, TYPE_SPAPR_PCI_HOST_BRIDGE " needs a pseries > > machine"); > > @@ -1733,7 +1734,10 @@ static void spapr_phb_realize(DeviceState *dev, > > Error **errp) > > spapr_irq_claim(spapr, irq, true, &local_err); > > if (local_err) { > > error_propagate_prepend(errp, local_err, "can't allocate LSIs: > > "); > > - return; > > + while (--i >= 0) { > > + spapr_irq_free(spapr, sphb->lsi_table[i].irq, 1); > > + } > > + goto fail_del_msiwindow; > > } > > > > sphb->lsi_table[i].irq = irq; > > @@ -1741,9 +1745,10 @@ static void spapr_phb_realize(DeviceState *dev, > > Error **errp) > > > > /* allocate connectors for child PCI devices */ > > if (sphb->dr_enabled) { > > - for (i = 0; i < PCI_SLOT_MAX * 8; i++) { > > - spapr_dr_connector_new(OBJECT(phb), TYPE_SPAPR_DRC_PCI, > > - (sphb->index << 16) | i); > > + for (i = 0; i < ARRAY_SIZE(drcs); i++) { > > + drcs[i] = > > + OBJECT(spapr_dr_connector_new(OBJECT(phb), > > TYPE_SPAPR_DRC_PCI, > > + (sphb->index << 16) | i)); > > } > > } > > > > @@ -1753,13 +1758,38 @@ static void spapr_phb_realize(DeviceState *dev, > > Error **errp) > > if (!tcet) { > > error_setg(errp, "Creating window#%d failed for %s", > > i, sphb->dtbusname); > > - return; > > + while (--i >= 0) { > > + tcet = spapr_tce_find_by_liobn(sphb->dma_liobn[i]); > > + assert(tcet); > > + memory_region_del_subregion(&sphb->iommu_root, > > + spapr_tce_get_iommu(tcet)); > > + object_unparent(OBJECT(tcet)); > > + } > > + goto fail_free_drcs; > > } > > memory_region_add_subregion(&sphb->iommu_root, 0, > > spapr_tce_get_iommu(tcet)); > > } > > > > sphb->msi = g_hash_table_new_full(g_int_hash, g_int_equal, g_free, > > g_free); > > + return; > > + > > +fail_free_drcs: > > + if (sphb->dr_enabled) { > > + for (i = 0; i < ARRAY_SIZE(drcs); i++) { > > + object_unparent(drcs[i]); > > + } > > + } > > +fail_del_msiwindow: > > + memory_region_del_subregion(&sphb->iommu_root, &sphb->msiwindow); > > + address_space_destroy(&sphb->iommu_as); > > + qbus_set_hotplug_handler(BUS(phb->bus), NULL, &error_abort); > > + pci_unregister_root_bus(phb->bus); > > + memory_region_del_subregion(get_system_memory(), &sphb->iowindow); > > + if (sphb->mem64_win_pciaddr != (hwaddr)-1) { > > + memory_region_del_subregion(get_system_memory(), > > &sphb->mem64window); > > + } > > + memory_region_del_subregion(get_system_memory(), &sphb->mem32window); > > } > > > > static int spapr_phb_children_reset(Object *child, void *opaque) > > >
pgp0xnb4Md8BG.pgp
Description: OpenPGP digital signature