Re: [Qemu-devel] [PATCH v5 09/17] spapr_pci: add PHB unrealize

[David Gibson]

On Tue, Feb 19, 2019 at 06:18:18PM +0100, Greg Kurz 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_unparent() on the PHB object, which in turn would normally
> 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,
> with the notable exception of DRCs. This is needed to avoid a QEMU
> crash when unplugging a PHB and resetting the machine before the
> guest could handle the event. The DRCs are removed from the QOM tree
> by  pci_unregister_root_bus() and we must make sure we're not leaving
> stale aliases under the global /dr-connector path.
> 
> The formula that gives the number of DMA windows is moved to an
> inline function in the hw/pci-host/spapr.h header because it
> will have other users.
> 
> The unrealize function is able to cope with partially realized PHBs.
> It is hence used to implement proper rollback on the realize error
> path.
> 
> Signed-off-by: Michael Roth <address@hidden>
> Signed-off-by: Greg Kurz <address@hidden>
> Reviewed-by: David Gibson <address@hidden>

Applied, thanks.

> ---
> v5: - unparent child DRCs at unrealize
> v4: - reverted to v2
> v3: - don't free LSIs at unrealize
> v2: - implement rollback with unrealize function
> ---
> ---
>  hw/ppc/spapr_pci.c          |   86 
> +++++++++++++++++++++++++++++++++++++++++--
>  include/hw/pci-host/spapr.h |    5 +++
>  2 files changed, 87 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index e2bc9fec824b..ede928b0bff3 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1570,6 +1570,75 @@ static void spapr_pci_unplug_request(HotplugHandler 
> *plug_handler,
>      }
>  }
>  
> +static void spapr_phb_finalizefn(Object *obj)
> +{
> +    sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(obj);
> +
> +    g_free(sphb->dtbusname);
> +    sphb->dtbusname = NULL;
> +}
> +
> +static void spapr_phb_unrealize(DeviceState *dev, Error **errp)
> +{
> +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> +    SysBusDevice *s = SYS_BUS_DEVICE(dev);
> +    PCIHostState *phb = PCI_HOST_BRIDGE(s);
> +    sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(phb);
> +    sPAPRTCETable *tcet;
> +    int i;
> +    const unsigned windows_supported = spapr_phb_windows_supported(sphb);
> +
> +    if (sphb->msi) {
> +        g_hash_table_unref(sphb->msi);
> +        sphb->msi = NULL;
> +    }
> +
> +    /*
> +     * Remove IO/MMIO subregions and aliases, rest should get cleaned
> +     * via PHB's unrealize->object_finalize
> +     */
> +    for (i = windows_supported - 1; i >= 0; i--) {
> +        tcet = spapr_tce_find_by_liobn(sphb->dma_liobn[i]);
> +        if (tcet) {
> +            memory_region_del_subregion(&sphb->iommu_root,
> +                                        spapr_tce_get_iommu(tcet));
> +        }
> +    }
> +
> +    if (sphb->dr_enabled) {
> +        for (i = PCI_SLOT_MAX * 8 - 1; i >= 0; i--) {
> +            sPAPRDRConnector *drc = spapr_drc_by_id(TYPE_SPAPR_DRC_PCI,
> +                                                    (sphb->index << 16) | i);
> +
> +            if (drc) {
> +                object_unparent(OBJECT(drc));
> +            }
> +        }
> +    }
> +
> +    for (i = PCI_NUM_PINS - 1; i >= 0; i--) {
> +        if (sphb->lsi_table[i].irq) {
> +            spapr_irq_free(spapr, sphb->lsi_table[i].irq, 1);
> +            sphb->lsi_table[i].irq = 0;
> +        }
> +    }
> +
> +    QLIST_REMOVE(sphb, list);
> +
> +    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 void spapr_phb_realize(DeviceState *dev, Error **errp)
>  {
>      /* We don't use SPAPR_MACHINE() in order to exit gracefully if the user
> @@ -1587,8 +1656,7 @@ static void spapr_phb_realize(DeviceState *dev, Error 
> **errp)
>      PCIBus *bus;
>      uint64_t msi_window_size = 4096;
>      sPAPRTCETable *tcet;
> -    const unsigned windows_supported =
> -        sphb->ddw_enabled ? SPAPR_PCI_DMA_MAX_WINDOWS : 1;
> +    const unsigned windows_supported = spapr_phb_windows_supported(sphb);
>  
>      if (!spapr) {
>          error_setg(errp, TYPE_SPAPR_PCI_HOST_BRIDGE " needs a pseries 
> machine");
> @@ -1745,6 +1813,10 @@ static void spapr_phb_realize(DeviceState *dev, Error 
> **errp)
>              if (local_err) {
>                  error_propagate_prepend(errp, local_err,
>                                          "can't allocate LSIs: ");
> +                /*
> +                 * Older machines will never support PHB hotplug, ie, this 
> is an
> +                 * init only path and QEMU will terminate. No need to 
> rollback.
> +                 */
>                  return;
>              }
>          }
> @@ -1752,7 +1824,7 @@ 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;
> +            goto unrealize;
>          }
>  
>          sphb->lsi_table[i].irq = irq;
> @@ -1772,13 +1844,17 @@ static void spapr_phb_realize(DeviceState *dev, Error 
> **errp)
>          if (!tcet) {
>              error_setg(errp, "Creating window#%d failed for %s",
>                         i, sphb->dtbusname);
> -            return;
> +            goto unrealize;
>          }
>          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;
> +
> +unrealize:
> +    spapr_phb_unrealize(dev, NULL);
>  }
>  
>  static int spapr_phb_children_reset(Object *child, void *opaque)
> @@ -1977,6 +2053,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;
> @@ -1992,6 +2069,7 @@ static const TypeInfo spapr_phb_info = {
>      .name          = TYPE_SPAPR_PCI_HOST_BRIDGE,
>      .parent        = TYPE_PCI_HOST_BRIDGE,
>      .instance_size = sizeof(sPAPRPHBState),
> +    .instance_finalize = spapr_phb_finalizefn,
>      .class_init    = spapr_phb_class_init,
>      .interfaces    = (InterfaceInfo[]) {
>          { TYPE_HOTPLUG_HANDLER },
> diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
> index f6e43f48fefa..4b0443f4cfe4 100644
> --- a/include/hw/pci-host/spapr.h
> +++ b/include/hw/pci-host/spapr.h
> @@ -165,4 +165,9 @@ static inline void spapr_phb_vfio_reset(DeviceState *qdev)
>  
>  void spapr_phb_dma_reset(sPAPRPHBState *sphb);
>  
> +static inline unsigned spapr_phb_windows_supported(sPAPRPHBState *sphb)
> +{
> +    return sphb->ddw_enabled ? SPAPR_PCI_DMA_MAX_WINDOWS : 1;
> +}
> +
>  #endif /* PCI_HOST_SPAPR_H */
> 

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