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>
---
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)
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.
> ---
> 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
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)
> >
>
© 2016 - 2025 Red Hat, Inc.