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

[David Gibson]

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 <address@hidden>

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.

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

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature