[PATCH 0/2] s390x/pci: changes to hotplug error handling

[PATCH 0/2] s390x/pci: changes to hotplug error handling

[Matthew Rosato]

Thomas previously requested a more meaningful message when plug of a
passthrough ISM device fails (interpretation facility required).  In
the process of creating that patch, I noticed that devices that fail 
s390_pcihost_plug during a hotplug scneario can potentially crash QEMU
because remnants of the associated S390PCIBusDevice were not properly
cleaned up.  So let's fix that too.

Matthew Rosato (2):
  s390x/pci: fix cleanup of failed hotplug
  s390x/pci: add message for ISM without zPCI interpretation

 hw/s390x/s390-pci-bus.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

-- 
2.48.1

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

[Matthew Rosato]

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);
 }
 
 static void s390_pcihost_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
-- 
2.48.1

[PATCH 2/2] s390x/pci: add message for ISM without zPCI interpretation

[Matthew Rosato]

Currently s390x does not support ISM passthrough without zPCI
interpretation.  This is already fenced, but the current message will
not provide adequate information to explain to the end-user why ISM
passthrough is not allowed.  Add a proper message.

Suggested-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
---
 hw/s390x/s390-pci-bus.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index 6cc0e7538a..c41c88f88e 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -1147,6 +1147,12 @@ static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
             pbdev->forwarding_assist = false;
         }
 
+        if (pbdev->pft == ZPCI_PFT_ISM && !pbdev->interp) {
+            error_setg(errp, "zPCI interpretation is required for ISM device "
+                       "passthrough");
+            goto pbdev_cleanup;
+        }
+
         if (s390_pci_msix_init(pbdev) && !pbdev->interp) {
             error_setg(errp, "MSI-X support is mandatory "
                        "in the S390 architecture");
-- 
2.48.1

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

[Thomas Huth]

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