Re: [Qemu-devel] [PATCH 04/15] spapr_pci: add proper rollback on PHB realize error path

[Greg Kurz <groug@kaod.org>]

[-- Attachment #1: Type: text/plain, Size: 4722 bytes --]

On Thu, 3 Jan 2019 12:59:06 +1100
David Gibson <david@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 <groug@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)
> >   
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]