Re: [PATCH 1/2] s390x/pci: fix cleanup of failed hotplug

[Thomas Huth <thuth@redhat.com>]

On 26/02/2025 22.02, Matthew Rosato wrote:
> In the unlikely event that we must fail hotplug of a PCI passthrough
> device, ensure appropriate clean up of the associated zPCI device is
> performed.
> 
> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
> ---
>   hw/s390x/s390-pci-bus.c | 21 +++++++++++++++++++--
>   1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
> index 913d72cc74..6cc0e7538a 100644
> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -1119,7 +1119,7 @@ static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>                       if (rc) {
>                           error_setg(errp, "Plug failed for zPCI device in "
>                                      "interpretation mode: %d", rc);
> -                        return;
> +                        goto pbdev_cleanup;
>                       }
>                   } else {
>                       trace_s390_pcihost("zPCI interpretation missing");
> @@ -1150,7 +1150,7 @@ static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>           if (s390_pci_msix_init(pbdev) && !pbdev->interp) {
>               error_setg(errp, "MSI-X support is mandatory "
>                          "in the S390 architecture");
> -            return;
> +            goto pbdev_cleanup;
>           }
>   
>           if (dev->hotplugged) {
> @@ -1168,6 +1168,23 @@ static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>       } else {
>           g_assert_not_reached();
>       }
> +
> +    return;
> +
> + pbdev_cleanup:
> +    DeviceState *bdev = DEVICE(pbdev);
> +
> +    if (pbdev->shutdown_notifier.notify) {
> +        notifier_remove(&pbdev->shutdown_notifier);
> +    }
> +    if (pbdev->iommu->dma_limit) {
> +        s390_pci_end_dma_count(s, pbdev->iommu->dma_limit);
> +    }
> +    s390_pci_iommu_free(s, pci_get_bus(pbdev->pdev), pbdev->pdev->devfn);
> +    QTAILQ_REMOVE(&s->zpci_devs, pbdev, link);
> +    g_hash_table_remove(s->zpci_table, &pbdev->idx);
> +    object_unparent(OBJECT(bdev));
> +    qdev_unrealize(bdev);
>   }

Not sure, but should bdev realy be unrealized here already? Or should that 
rather be done by the caller (when it sees the errp set)?

  Thomas