[PATCH v6 0/2] Error recovery for zPCI passthrough devices

[PATCH v6 0/2] Error recovery for zPCI passthrough devices

[Farhan Ali]

Hi,

This patch series introduces support for error recovery for passthrough PCI
devices on System Z (s390x). This is the user space component for the Linux
kernel patches [1]. The kernel patches were merged for 7.3 release.

The current design for QEMU updates the callback for vfio error notifier to
an s390x specific error handler if the kernel supports the new device
feature VFIO_DEVICE_FEATURE_ZPCI_ERROR, for s390 vfio-pci devices. So now
on an eventfd notification for error notifier it will invoke the s390x
specific error handler.  The s390x error handler will retrieve the
architecture specific PCI error information and inject the information into
the guest. Once the guest receives the error information, the guest drivers
will drive the error recovery.  Typically recovery involves a device reset
which translate to CLP disable/enable cycle for the device.

I would appreciate some feedback on this patch series.

Thanks Farhan

[1] https://lore.kernel.org/all/20260818164326.387bb27b@shazbot.org/

ChangeLog
---------
v5 https://lore.kernel.org/all/20260914174420.12309-1-alifm@linux.ibm.com/
v5 -> v6
    - Address Cedric's feedback (patch 1).
    - Update error handling to follow QEMU style (patch 1).
    - Rebase on master.
    - Remove linux-headers update as latest QEMU master has the VFIO headers
    merged.

v4 https://lore.kernel.org/all/20260831183151.12626-1-alifm@linux.ibm.com/
v4 -> v5
    - Remove err_handler() callback in vfio pci core.
    - Update the vfio error notifier callback to an s390x specific callback
    for s390x devices
    - Include linux headers for 7.3-rc3.
v3 https://lore.kernel.org/qemu-devel/20250925174852.1302-1-alifm@linux.ibm.com/
v3 -> v4
    - Include linux headers for 7.3-rc1.
    - Rework VFIO API changes based on the kernel API (patch 3).
    - Address Markus's comments from v3 (patch 2).

v2 https://lore.kernel.org/qemu-devel/20250825212434.2255-1-alifm@linux.ibm.com/
v2 -> v3
    - Update arch_err_handler to err_handler and include Error ** in
    function definition. (patch 2)

    - Introduce helper function to hide the internal indirection of device_feature()
    (patch 3)

    - Update function definitions to include Error ** (patch 4)
    


v1 https://lore.kernel.org/qemu-devel/20250813174152.1238-1-alifm@linux.ibm.com/
v1 -> v2
   - Use VFIO_DEVICE_FEATURE ioctl to get device error information.
   (Based on Alex's feedback on kernel series)


Farhan Ali (2):
  s390x/pci: Add PCI error handling for vfio pci devices
  s390x/pci: Reset a device in error state

 hw/s390x/s390-pci-bus.c          |  13 ++++
 hw/s390x/s390-pci-vfio-stubs.c   |  10 +++
 hw/s390x/s390-pci-vfio.c         | 130 +++++++++++++++++++++++++++++++
 include/hw/s390x/s390-pci-bus.h  |   1 +
 include/hw/s390x/s390-pci-vfio.h |   2 +
 5 files changed, 156 insertions(+)

-- 
2.43.0

[PATCH v6 1/2] s390x/pci: Add PCI error handling for vfio pci devices

[Farhan Ali]

Add an s390x specific handler for vfio error notifier. For s390x pci devices,
we have platform specific error information. We need to retrieve this error
information for passthrough devices. This is done via a VFIO_DEVICE_FEATURE
ioctl which exposes that information.

Once this error information is retrieved we can then inject an error into
the guest, and let the guest drive the recovery.

Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
---
 hw/s390x/s390-pci-bus.c          |   6 ++
 hw/s390x/s390-pci-vfio-stubs.c   |   6 ++
 hw/s390x/s390-pci-vfio.c         | 121 +++++++++++++++++++++++++++++++
 include/hw/s390x/s390-pci-bus.h  |   1 +
 include/hw/s390x/s390-pci-vfio.h |   1 +
 5 files changed, 135 insertions(+)

diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index 2eb4e8cec4..b2967dacba 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -1085,6 +1085,7 @@ static void s390_pcihost_plug(const HotplugHandler *hotplug_dev, DeviceState *de
     S390pciState *s = S390_PCI_HOST_BRIDGE(hotplug_dev);
     PCIDevice *pdev = NULL;
     S390PCIBusDevice *pbdev = NULL;
+    Error *local_err = NULL;
     int rc;
 
     if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_BRIDGE)) {
@@ -1175,6 +1176,11 @@ static void s390_pcihost_plug(const HotplugHandler *hotplug_dev, DeviceState *de
             pbdev->iommu->dma_limit = s390_pci_start_dma_count(s, pbdev);
             /* Fill in CLP information passed via the vfio region */
             s390_pci_get_clp_info(pbdev);
+            /* Setup error handler for error recovery */
+            if (!s390_pci_setup_err_handler(pbdev, &local_err)) {
+                warn_report_err(local_err);
+            }
+
             if (!pbdev->interp) {
                 /* Do vfio passthrough but intercept for I/O */
                 pbdev->fh |= FH_SHM_VFIO;
diff --git a/hw/s390x/s390-pci-vfio-stubs.c b/hw/s390x/s390-pci-vfio-stubs.c
index d9882b7aad..9fc84ca135 100644
--- a/hw/s390x/s390-pci-vfio-stubs.c
+++ b/hw/s390x/s390-pci-vfio-stubs.c
@@ -30,3 +30,9 @@ bool s390_pci_get_host_fh(S390PCIBusDevice *pbdev, uint32_t *fh)
 void s390_pci_get_clp_info(S390PCIBusDevice *pbdev)
 {
 }
+
+bool s390_pci_setup_err_handler(S390PCIBusDevice *pbdev, Error **errp)
+{
+    error_setg(errp, "VFIO not available, cannot setup error handler");
+    return false;
+}
diff --git a/hw/s390x/s390-pci-vfio.c b/hw/s390x/s390-pci-vfio.c
index db6de00bd2..6b7c554fe5 100644
--- a/hw/s390x/s390-pci-vfio.c
+++ b/hw/s390x/s390-pci-vfio.c
@@ -10,6 +10,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/error-report.h"
 
 #include <sys/ioctl.h>
 #include <linux/vfio.h>
@@ -105,6 +106,84 @@ void s390_pci_end_dma_count(S390pciState *s, S390PCIDMACount *cnt)
     }
 }
 
+static int s390_pci_get_feature_err(VFIOPCIDevice *vfio_pci,
+                                    PciCcdfErr *ccdf,
+                                    uint32_t ccdf_err_length,
+                                    Error **errp)
+{
+    ERRP_GUARD();
+    int ret;
+    size_t total_size;
+    struct vfio_device_feature_zpci_err *err;
+    g_autofree void *buf = NULL;
+    g_autofree struct vfio_device_feature *feature = NULL;
+
+    total_size = sizeof(*feature) + sizeof(*err);
+    feature = g_malloc(total_size);
+    feature->argsz = total_size;
+    feature->flags = VFIO_DEVICE_FEATURE_GET | VFIO_DEVICE_FEATURE_ZPCI_ERROR;
+
+    buf = g_malloc(ccdf_err_length);
+    err = (void *)feature->data;
+    err->data = (uint64_t)buf;
+    ret = vfio_device_get_feature(&vfio_pci->vbasedev, feature);
+
+    if (ret) {
+        error_setg(errp, "Failed feature get VFIO_DEVICE_FEATURE_ZPCI_ERROR"
+                   " (rc=%d)", ret);
+        return ret;
+    }
+
+    memcpy(ccdf, (PciCcdfErr *) err->data, ccdf_err_length);
+
+    return 0;
+}
+
+static void s390_pci_err_handler(void *opaque)
+{
+    VFIOPCIDevice *vfio_pci;
+    S390PCIBusDevice *pbdev;
+    Error *local_err = NULL;
+    PciCcdfErr ccdf;
+    int ret = 0;
+
+    vfio_pci = opaque;
+    if (!event_notifier_test_and_clear(&vfio_pci->err_notifier)) {
+        return;
+    }
+
+    pbdev = s390_pci_find_dev_by_target(s390_get_phb(),
+                                        DEVICE(&vfio_pci->parent_obj)->id);
+
+    if (!pbdev) {
+        error_report("No matching zpci device found");
+        return;
+    }
+    pbdev->state = ZPCI_FS_ERROR;
+
+    while (ret == 0) {
+        ret = s390_pci_get_feature_err(vfio_pci, &ccdf,
+                                       pbdev->ccdf_err_length, &local_err);
+        if (ret) {
+            /*
+             * This is an expected errno indicating there are no pending
+             * PCI errors to handle for the device.
+             */
+            if (ret == -ENOMSG) {
+                error_free(local_err);
+                local_err = NULL;
+            } else {
+                error_report_err(local_err);
+            }
+            break;
+        }
+        s390_pci_generate_error_event(ccdf.pec, pbdev->fh, pbdev->fid,
+                                      ccdf.faddr, ccdf.e);
+    }
+
+    return;
+}
+
 static void s390_pci_read_base(S390PCIBusDevice *pbdev,
                                struct vfio_device_info *info)
 {
@@ -134,6 +213,10 @@ static void s390_pci_read_base(S390PCIBusDevice *pbdev,
     /* Store function type separately for type-specific behavior */
     pbdev->pft = cap->pft;
 
+    if (hdr->version >= 3) {
+        pbdev->ccdf_err_length = cap->ccdf_err_length;
+    }
+
     /*
      * If the device is a passthrough ISM device, disallow relaxed
      * translation.
@@ -371,3 +454,41 @@ void s390_pci_get_clp_info(S390PCIBusDevice *pbdev)
     s390_pci_read_util(pbdev, info);
     s390_pci_read_pfip(pbdev, info);
 }
+
+bool s390_pci_setup_err_handler(S390PCIBusDevice *pbdev, Error **errp)
+{
+    int ret;
+    int32_t fd;
+    VFIOPCIDevice *vfio_pci = VFIO_PCI_DEVICE(pbdev->pdev);
+    uint64_t buf[DIV_ROUND_UP(sizeof(struct vfio_device_feature),
+                              sizeof(uint64_t))] = {};
+    struct vfio_device_feature *feature = (struct vfio_device_feature *)buf;
+
+    feature->argsz = sizeof(buf);
+    feature->flags = VFIO_DEVICE_FEATURE_PROBE | VFIO_DEVICE_FEATURE_ZPCI_ERROR;
+
+    ret = vfio_device_get_feature(&vfio_pci->vbasedev, feature);
+
+    if (ret != 0) {
+        if (ret == -ENOTTY) {
+            error_setg(errp, "Automated error recovery unavailable for device");
+        } else {
+            error_setg(errp,
+                       "Failed to probe for VFIO_DEVICE_FEATURE_ZPCI_ERROR (ret=%d)",
+                       ret);
+        }
+        return false;
+    }
+
+    if (sizeof(PciCcdfErr) != pbdev->ccdf_err_length) {
+        error_setg(errp,
+                   "CCDF size mismatch expected size=%zu, provided size=%d",
+                   sizeof(PciCcdfErr), pbdev->ccdf_err_length);
+        return false;
+    }
+
+    fd = event_notifier_get_fd(&vfio_pci->err_notifier);
+    qemu_set_fd_handler(fd, s390_pci_err_handler, NULL, vfio_pci);
+
+    return true;
+}
diff --git a/include/hw/s390x/s390-pci-bus.h b/include/hw/s390x/s390-pci-bus.h
index 9228523ce8..c2348ede86 100644
--- a/include/hw/s390x/s390-pci-bus.h
+++ b/include/hw/s390x/s390-pci-bus.h
@@ -364,6 +364,7 @@ struct S390PCIBusDevice {
     bool forwarding_assist;
     bool aif;
     bool rtr_avail;
+    uint32_t ccdf_err_length;
     QTAILQ_ENTRY(S390PCIBusDevice) link;
 };
 
diff --git a/include/hw/s390x/s390-pci-vfio.h b/include/hw/s390x/s390-pci-vfio.h
index f7d6149daf..c7886b63ea 100644
--- a/include/hw/s390x/s390-pci-vfio.h
+++ b/include/hw/s390x/s390-pci-vfio.h
@@ -20,5 +20,6 @@ S390PCIDMACount *s390_pci_start_dma_count(S390pciState *s,
 void s390_pci_end_dma_count(S390pciState *s, S390PCIDMACount *cnt);
 bool s390_pci_get_host_fh(S390PCIBusDevice *pbdev, uint32_t *fh);
 void s390_pci_get_clp_info(S390PCIBusDevice *pbdev);
+bool s390_pci_setup_err_handler(S390PCIBusDevice *pbdev, Error **errp);
 
 #endif
-- 
2.43.0

[PATCH v6 2/2] s390x/pci: Reset a device in error state

[Farhan Ali]

For passthrough devices in error state, for a guest driven reset of the
device we can attempt a reset to recover the device. A reset of the device
will trigger a CLP disable/enable cycle on the host to bring the device
into a recovered state.

Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
---
 hw/s390x/s390-pci-bus.c          | 7 +++++++
 hw/s390x/s390-pci-vfio-stubs.c   | 4 ++++
 hw/s390x/s390-pci-vfio.c         | 9 +++++++++
 include/hw/s390x/s390-pci-vfio.h | 1 +
 4 files changed, 21 insertions(+)

diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index b2967dacba..8418da9372 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -1505,6 +1505,8 @@ static void s390_pci_device_reset(DeviceState *dev)
         return;
     case ZPCI_FS_STANDBY:
         break;
+    case ZPCI_FS_ERROR:
+        break;
     default:
         pbdev->fh &= ~FH_MASK_ENABLE;
         pbdev->state = ZPCI_FS_DISABLED;
@@ -1517,6 +1519,11 @@ static void s390_pci_device_reset(DeviceState *dev)
     } else if (pbdev->summary_ind) {
         pci_dereg_irqs(pbdev);
     }
+
+    if (pbdev->state == ZPCI_FS_ERROR) {
+        s390_pci_reset(pbdev);
+    }
+
     if (pbdev->iommu->enabled) {
         pci_dereg_ioat(pbdev->iommu);
     }
diff --git a/hw/s390x/s390-pci-vfio-stubs.c b/hw/s390x/s390-pci-vfio-stubs.c
index 9fc84ca135..c68c612038 100644
--- a/hw/s390x/s390-pci-vfio-stubs.c
+++ b/hw/s390x/s390-pci-vfio-stubs.c
@@ -36,3 +36,7 @@ bool s390_pci_setup_err_handler(S390PCIBusDevice *pbdev, Error **errp)
     error_setg(errp, "VFIO not available, cannot setup error handler");
     return false;
 }
+
+void s390_pci_reset(S390PCIBusDevice *pbdev)
+{
+}
diff --git a/hw/s390x/s390-pci-vfio.c b/hw/s390x/s390-pci-vfio.c
index 6b7c554fe5..da708ceb47 100644
--- a/hw/s390x/s390-pci-vfio.c
+++ b/hw/s390x/s390-pci-vfio.c
@@ -184,6 +184,15 @@ static void s390_pci_err_handler(void *opaque)
     return;
 }
 
+void s390_pci_reset(S390PCIBusDevice *pbdev)
+{
+    VFIOPCIDevice *vfio_pci = VFIO_PCI_DEVICE(pbdev->pdev);
+    if (ioctl(vfio_pci->vbasedev.fd, VFIO_DEVICE_RESET)) {
+        error_report("Failed to reset PCI device %s : %s ",
+                      vfio_pci->vbasedev.name, strerror(errno));
+    }
+}
+
 static void s390_pci_read_base(S390PCIBusDevice *pbdev,
                                struct vfio_device_info *info)
 {
diff --git a/include/hw/s390x/s390-pci-vfio.h b/include/hw/s390x/s390-pci-vfio.h
index c7886b63ea..38ccf445ea 100644
--- a/include/hw/s390x/s390-pci-vfio.h
+++ b/include/hw/s390x/s390-pci-vfio.h
@@ -21,5 +21,6 @@ void s390_pci_end_dma_count(S390pciState *s, S390PCIDMACount *cnt);
 bool s390_pci_get_host_fh(S390PCIBusDevice *pbdev, uint32_t *fh);
 void s390_pci_get_clp_info(S390PCIBusDevice *pbdev);
 bool s390_pci_setup_err_handler(S390PCIBusDevice *pbdev, Error **errp);
+void s390_pci_reset(S390PCIBusDevice *pbdev);
 
 #endif
-- 
2.43.0

Re: [PATCH v6 0/2] Error recovery for zPCI passthrough devices

[Farhan Ali]

Hi,

Polite ping for this series.

Thanks

Farhan

On 9/22/2026 10:17 AM, Farhan Ali wrote:
> Hi,
>
> This patch series introduces support for error recovery for passthrough PCI
> devices on System Z (s390x). This is the user space component for the Linux
> kernel patches [1]. The kernel patches were merged for 7.3 release.
>
> The current design for QEMU updates the callback for vfio error notifier to
> an s390x specific error handler if the kernel supports the new device
> feature VFIO_DEVICE_FEATURE_ZPCI_ERROR, for s390 vfio-pci devices. So now
> on an eventfd notification for error notifier it will invoke the s390x
> specific error handler.  The s390x error handler will retrieve the
> architecture specific PCI error information and inject the information into
> the guest. Once the guest receives the error information, the guest drivers
> will drive the error recovery.  Typically recovery involves a device reset
> which translate to CLP disable/enable cycle for the device.
>
> I would appreciate some feedback on this patch series.
>
> Thanks Farhan
>
> [1] https://lore.kernel.org/all/20260818164326.387bb27b@shazbot.org/
>
> ChangeLog
> ---------
> v5 https://lore.kernel.org/all/20260914174420.12309-1-alifm@linux.ibm.com/
> v5 -> v6
>      - Address Cedric's feedback (patch 1).
>      - Update error handling to follow QEMU style (patch 1).
>      - Rebase on master.
>      - Remove linux-headers update as latest QEMU master has the VFIO headers
>      merged.
>
> v4 https://lore.kernel.org/all/20260831183151.12626-1-alifm@linux.ibm.com/
> v4 -> v5
>      - Remove err_handler() callback in vfio pci core.
>      - Update the vfio error notifier callback to an s390x specific callback
>      for s390x devices
>      - Include linux headers for 7.3-rc3.
> v3 https://lore.kernel.org/qemu-devel/20250925174852.1302-1-alifm@linux.ibm.com/
> v3 -> v4
>      - Include linux headers for 7.3-rc1.
>      - Rework VFIO API changes based on the kernel API (patch 3).
>      - Address Markus's comments from v3 (patch 2).
>
> v2 https://lore.kernel.org/qemu-devel/20250825212434.2255-1-alifm@linux.ibm.com/
> v2 -> v3
>      - Update arch_err_handler to err_handler and include Error ** in
>      function definition. (patch 2)
>
>      - Introduce helper function to hide the internal indirection of device_feature()
>      (patch 3)
>
>      - Update function definitions to include Error ** (patch 4)
>      
>
>
> v1 https://lore.kernel.org/qemu-devel/20250813174152.1238-1-alifm@linux.ibm.com/
> v1 -> v2
>     - Use VFIO_DEVICE_FEATURE ioctl to get device error information.
>     (Based on Alex's feedback on kernel series)
>
>
> Farhan Ali (2):
>    s390x/pci: Add PCI error handling for vfio pci devices
>    s390x/pci: Reset a device in error state
>
>   hw/s390x/s390-pci-bus.c          |  13 ++++
>   hw/s390x/s390-pci-vfio-stubs.c   |  10 +++
>   hw/s390x/s390-pci-vfio.c         | 130 +++++++++++++++++++++++++++++++
>   include/hw/s390x/s390-pci-bus.h  |   1 +
>   include/hw/s390x/s390-pci-vfio.h |   2 +
>   5 files changed, 156 insertions(+)
>

Re: [PATCH v6 2/2] s390x/pci: Reset a device in error state

[Matthew Rosato]

> --- a/hw/s390x/s390-pci-bus.c
> +++ b/hw/s390x/s390-pci-bus.c
> @@ -1505,6 +1505,8 @@ static void s390_pci_device_reset(DeviceState *dev)
>          return;
>      case ZPCI_FS_STANDBY:
>          break;
> +    case ZPCI_FS_ERROR:
> +        break;
>      default:
>          pbdev->fh &= ~FH_MASK_ENABLE;
>          pbdev->state = ZPCI_FS_DISABLED;
> @@ -1517,6 +1519,11 @@ static void s390_pci_device_reset(DeviceState *dev)
>      } else if (pbdev->summary_ind) {
>          pci_dereg_irqs(pbdev);
>      }
> +
> +    if (pbdev->state == ZPCI_FS_ERROR) {
> +        s390_pci_reset(pbdev);
> +    }

I know it was mentioned in the prior version, so apologies for not
chiming in til now.  But you really need a comment block explaining why
it's OK to reset the device yet leave it in the ZPCI_FS_ERROR state
after the fact.  That's confusing.

You mentioned in the last version it's because this happens on the
disable path, and there yes we will set the device to ZPCI_FS_DISABLED
right after this -- but what about any other case where we drive this
reset path (e.g. subsystem reset, reboot, and the ISM-specific extra paths)

Note that the non-error cases are ensuring we either leave this function
with the device listed as disabled or standby/reserved.

Would there be harm in, after performing the reset, doing
pbdev->fh &= ~FH_MASK_ENABLE;
pbdev->state = ZPCI_FS_DISABLED;
for the ZPCI_FS_ERROR case now?

Consider that:
1) For a guest-initiated disable, we already checked those were valid
before reaching this point.
2) For all other reset cases, if the device were not in
error/reserved/standby we were leaving here with the device disabled
before this patch.  If the device happened to be in error, now you fix
it up but isn't the proper fix state now 'disabled' to match what an
enabled device would have been?

Thanks,
Matt

Re: [PATCH v6 1/2] s390x/pci: Add PCI error handling for vfio pci devices

[Matthew Rosato]

> +static int s390_pci_get_feature_err(VFIOPCIDevice *vfio_pci,
> +                                    PciCcdfErr *ccdf,
> +                                    uint32_t ccdf_err_length,
> +                                    Error **errp)
> +{
> +    ERRP_GUARD();
> +    int ret;
> +    size_t total_size;
> +    struct vfio_device_feature_zpci_err *err;
> +    g_autofree void *buf = NULL;
> +    g_autofree struct vfio_device_feature *feature = NULL;
> +
> +    total_size = sizeof(*feature) + sizeof(*err);
> +    feature = g_malloc(total_size);
> +    feature->argsz = total_size;
> +    feature->flags = VFIO_DEVICE_FEATURE_GET | VFIO_DEVICE_FEATURE_ZPCI_ERROR;
> +
> +    buf = g_malloc(ccdf_err_length);
> +    err = (void *)feature->data;
> +    err->data = (uint64_t)buf;
> +    ret = vfio_device_get_feature(&vfio_pci->vbasedev, feature);
> +
> +    if (ret) {
> +        error_setg(errp, "Failed feature get VFIO_DEVICE_FEATURE_ZPCI_ERROR"
> +                   " (rc=%d)", ret);

I see this was changed from last version, but doesn't this fall under
'avoid useless error object creation and destruction' described in
qapi/error.h for the -ENOMSG return value?

We create it here only to explicitly destroy it from the caller.

Can we instead switch to the negative/non-negative return structure with
something like...

<0: error as defined (so we set errp)
0-N: number of errors to process (and we do not set errp)

For the non-negative case we can realistically only have 0 (-ENOMSG maps
to this) or 1 (the feature found something - it is reporting 1 error to
handle).

> +        return ret;
> +    }
> +
> +    memcpy(ccdf, (PciCcdfErr *) err->data, ccdf_err_length);

You already had err->data in void *buf, can we just

memcpy(ccdf, buf, ccdf_err_length);


> +
> +    return 0;
> +}
> +
> +static void s390_pci_err_handler(void *opaque)
> +{
> +    VFIOPCIDevice *vfio_pci;
> +    S390PCIBusDevice *pbdev;
> +    Error *local_err = NULL;
> +    PciCcdfErr ccdf;
> +    int ret = 0;
> +
> +    vfio_pci = opaque;
> +    if (!event_notifier_test_and_clear(&vfio_pci->err_notifier)) {
> +        return;
> +    }

Another one I should have commented on during v5...  You're right about
the recent patch and Z support, but what if the patch is missing on the
host kernel?

I'm fine with the feature not working with AER is off rather than doing
our own setup, but don't we need to avoid referencing this in that case
and ideally give some log message?

> +
> +    pbdev = s390_pci_find_dev_by_target(s390_get_phb(),
> +                                        DEVICE(&vfio_pci->parent_obj)->id);
> +

We can get the pci device from the vfio-pci device, so would it work to
just do this?

s390_pci_find_dev_by_pci(s390_get_phb(), PCI_DEVICE(vfio_pci));

Then we cut out the strcmps

Thanks,
Matt

Re: [PATCH v6 1/2] s390x/pci: Add PCI error handling for vfio pci devices

[Farhan Ali]

On 9/30/2026 9:30 AM, Matthew Rosato wrote:
>> +static int s390_pci_get_feature_err(VFIOPCIDevice *vfio_pci,
>> +                                    PciCcdfErr *ccdf,
>> +                                    uint32_t ccdf_err_length,
>> +                                    Error **errp)
>> +{
>> +    ERRP_GUARD();
>> +    int ret;
>> +    size_t total_size;
>> +    struct vfio_device_feature_zpci_err *err;
>> +    g_autofree void *buf = NULL;
>> +    g_autofree struct vfio_device_feature *feature = NULL;
>> +
>> +    total_size = sizeof(*feature) + sizeof(*err);
>> +    feature = g_malloc(total_size);
>> +    feature->argsz = total_size;
>> +    feature->flags = VFIO_DEVICE_FEATURE_GET | VFIO_DEVICE_FEATURE_ZPCI_ERROR;
>> +
>> +    buf = g_malloc(ccdf_err_length);
>> +    err = (void *)feature->data;
>> +    err->data = (uint64_t)buf;
>> +    ret = vfio_device_get_feature(&vfio_pci->vbasedev, feature);
>> +
>> +    if (ret) {
>> +        error_setg(errp, "Failed feature get VFIO_DEVICE_FEATURE_ZPCI_ERROR"
>> +                   " (rc=%d)", ret);
> I see this was changed from last version, but doesn't this fall under
> 'avoid useless error object creation and destruction' described in
> qapi/error.h for the -ENOMSG return value?
>
> We create it here only to explicitly destroy it from the caller.
>
> Can we instead switch to the negative/non-negative return structure with
> something like...
>
> <0: error as defined (so we set errp)
> 0-N: number of errors to process (and we do not set errp)
>
> For the non-negative case we can realistically only have 0 (-ENOMSG maps
> to this) or 1 (the feature found something - it is reporting 1 error to
> handle).

I was trying to follow what I assumed to be existing pattern for 
handling expected errno[1]. I am okay to change it, but I would like to 
understand what is the preferred approach here.

[1] https://elixir.bootlin.com/qemu/v11.1.2/source/hw/vfio/iommufd.c#L408


>> +        return ret;
>> +    }
>> +
>> +    memcpy(ccdf, (PciCcdfErr *) err->data, ccdf_err_length);
> You already had err->data in void *buf, can we just
>
> memcpy(ccdf, buf, ccdf_err_length);
>
Sure, I can make the change.


>> +
>> +    return 0;
>> +}
>> +
>> +static void s390_pci_err_handler(void *opaque)
>> +{
>> +    VFIOPCIDevice *vfio_pci;
>> +    S390PCIBusDevice *pbdev;
>> +    Error *local_err = NULL;
>> +    PciCcdfErr ccdf;
>> +    int ret = 0;
>> +
>> +    vfio_pci = opaque;
>> +    if (!event_notifier_test_and_clear(&vfio_pci->err_notifier)) {
>> +        return;
>> +    }
> Another one I should have commented on during v5...  You're right about
> the recent patch and Z support, but what if the patch is missing on the
> host kernel?
>
> I'm fine with the feature not working with AER is off rather than doing
> our own setup, but don't we need to avoid referencing this in that case
> and ideally give some log message?

Discussed this offline and will add a check to see if the err_notifier 
is registered. Though this is not strictly necessary, but better to add 
a defensive check.


>> +
>> +    pbdev = s390_pci_find_dev_by_target(s390_get_phb(),
>> +                                        DEVICE(&vfio_pci->parent_obj)->id);
>> +
> We can get the pci device from the vfio-pci device, so would it work to
> just do this?
>
> s390_pci_find_dev_by_pci(s390_get_phb(), PCI_DEVICE(vfio_pci));
>
> Then we cut out the strcmps

Yup, can change this.

Thanks

Farhan

Re: [PATCH v6 2/2] s390x/pci: Reset a device in error state

[Farhan Ali]

On 9/30/2026 8:32 AM, Matthew Rosato wrote:
>> --- a/hw/s390x/s390-pci-bus.c
>> +++ b/hw/s390x/s390-pci-bus.c
>> @@ -1505,6 +1505,8 @@ static void s390_pci_device_reset(DeviceState *dev)
>>           return;
>>       case ZPCI_FS_STANDBY:
>>           break;
>> +    case ZPCI_FS_ERROR:
>> +        break;
>>       default:
>>           pbdev->fh &= ~FH_MASK_ENABLE;
>>           pbdev->state = ZPCI_FS_DISABLED;
>> @@ -1517,6 +1519,11 @@ static void s390_pci_device_reset(DeviceState *dev)
>>       } else if (pbdev->summary_ind) {
>>           pci_dereg_irqs(pbdev);
>>       }
>> +
>> +    if (pbdev->state == ZPCI_FS_ERROR) {
>> +        s390_pci_reset(pbdev);
>> +    }
> I know it was mentioned in the prior version, so apologies for not
> chiming in til now.  But you really need a comment block explaining why
> it's OK to reset the device yet leave it in the ZPCI_FS_ERROR state
> after the fact.  That's confusing.

I can add a comment block explaining why we don't change the state. But 
my understanding is the guest needs to drive the change of the function 
state.


>
> You mentioned in the last version it's because this happens on the
> disable path, and there yes we will set the device to ZPCI_FS_DISABLED
> right after this -- but what about any other case where we drive this
> reset path (e.g. subsystem reset, reboot, and the ISM-specific extra paths)
>
> Note that the non-error cases are ensuring we either leave this function
> with the device listed as disabled or standby/reserved.
>
> Would there be harm in, after performing the reset, doing
> pbdev->fh &= ~FH_MASK_ENABLE;
> pbdev->state = ZPCI_FS_DISABLED;
> for the ZPCI_FS_ERROR case now?

One reason I didn't change the state was also because per my 
understanding of architecture we need the guest to clear the error 
state. This could be done via a mpcifc instruction (oc=7 
ZPCI_MOD_FC_RESET_ERROR) or through a CLP set pci function 
enable/disable cycle. So if we leave the device in disabled state after 
a reset without the guest driving any change, I think it might present a 
wrong view to the guest. I am open to suggestions if you think it would 
be more appropriate to keep the device in disabled state.

Thanks

Farhan

> Consider that:
> 1) For a guest-initiated disable, we already checked those were valid
> before reaching this point.
> 2) For all other reset cases, if the device were not in
> error/reserved/standby we were leaving here with the device disabled
> before this patch.  If the device happened to be in error, now you fix
> it up but isn't the proper fix state now 'disabled' to match what an
> enabled device would have been?
>
> Thanks,
> Matt

Re: [PATCH v6 2/2] s390x/pci: Reset a device in error state

[Matthew Rosato]

>>
>> You mentioned in the last version it's because this happens on the
>> disable path, and there yes we will set the device to ZPCI_FS_DISABLED
>> right after this -- but what about any other case where we drive this
>> reset path (e.g. subsystem reset, reboot, and the ISM-specific extra
>> paths)
>>
>> Note that the non-error cases are ensuring we either leave this function
>> with the device listed as disabled or standby/reserved.
>>
>> Would there be harm in, after performing the reset, doing
>> pbdev->fh &= ~FH_MASK_ENABLE;
>> pbdev->state = ZPCI_FS_DISABLED;
>> for the ZPCI_FS_ERROR case now?
> 
> One reason I didn't change the state was also because per my
> understanding of architecture we need the guest to clear the error
> state. This could be done via a mpcifc instruction (oc=7
> ZPCI_MOD_FC_RESET_ERROR) or through a CLP set pci function enable/

That's an interesting one.  This series doesn't change that path, so it
just resets the error state but we don't actually take any action, so I
guess we just hope the device will now work and just didn't need
recovery on the host or a disable/enable cycle.

> disable cycle. So if we leave the device in disabled state after a reset
> without the guest driving any change, I think it might present a wrong
> view to the guest. I am open to suggestions if you think it would be
> more appropriate to keep the device in disabled state.

I get what you're saying, but the reality is that this reset code as
implemented is going to get driven in a few ways:

1) During the guest-initiated disable, quite likely in response to the
PEC we injected if the device is in ZPCI_FS_ERROR.  Here the guest is
responsible for clearing their error state as you say, but they're
already on the way to doing that when you come through this path.
They're doing the disable now, next step will be the enable.

2) Subsystem reset that is potentially running in parallel with the
guest recovery action (or the guest is not taking recovery action at
all).  Here I don't believe the guest is responsible for clearing the
error state actually -- either the device remains in an error state over
the course of the subsystem reset (the way it used to work, because we
had no way of fixing it) or, if we already did host recovery, we have
reason to believe the device will now work and so it is free to be set
to disabled because the guest will no longer have the prior context to
believe it is responsible for clearing any error state -- it thinks it's
starting 'from the beginning' due to the subsystem reset.  This is
exactly why we disable the device in the reset path for an enabled
device (that isn't in an error state) today, because that is the initial
state the guest expects.

To say it another way, when the subsystem reset occurs it will trigger
the device reset for all devices on the bus (plus a special direct
invocation a bit earlier for ISM devices).  And the expectation after a
subsystem reset is that the devices will be in a state such that the
guest can clp enable them.  This is the case that I am concerned you are
missing with this implementation -- if we hit this path during reboot
(and we cannot assume the guest issued its own CLP disable during the
reboot) then the next thing the guest will try is a CLP enable during
boot, which I think will fail even though you will have already done the
host recovery action.  And I actually think the failure will be not due
to the state == ZPCI_FS_ERROR (because enable ignores that) but actually
because you never disabled the handle -- so the guest will get
CLP_RC_SETPCIFN_FHOP.

AFAICT the only way the guest will be able to clear that up and force
the device to be usable again is to do a disable/enable cycle AFTER the
subsystem reset (or use the mpcifc) to clear the handle.  That doesn't
sound right either.  Yes, this is likely a small window (subsystem reset
at the same time a PEC comes from the host) but I think it highlights
the problem with leaving the device in ZPCI_FS_ERROR / handle enabled.

If you think placing the device into a disabled state straight away
won't cover architecture, we could consider a new state to track when a
device is in error state (ZPCI_FS_ERROR) vs a device that has been reset
and is ready for the guest to clear the error state (ZPCI_FS_RECOVERED
or something)?  The disable path can go straight from
RECOVERED->DISABLED, but then the subsystem_reset path could have logic
that ensures all devices that are ZPCI_FS_RECOVERED get moved to
ZPCI_FS_DISABLED (and their handle masked to disabled) before the guest
starts running again.  Any that are in STANDBY/RESERVED/ERROR stay that
way, and everything else should already be DISABLED.

I'm not sure if that overhead buys us anything though vs short-pathing
right to 'disabled', because I can't think of a case where we reset the
device on a path where the guest isn't either already doing the
error-state-clearing action, is no longer responsible for it, or the
device is being removed from the guest configuration.

Thanks,
Matt

Re: [PATCH v6 2/2] s390x/pci: Reset a device in error state

[Matthew Rosato]

On 9/30/26 5:50 PM, Matthew Rosato wrote:
> 
>>>
>>> You mentioned in the last version it's because this happens on the
>>> disable path, and there yes we will set the device to ZPCI_FS_DISABLED
>>> right after this -- but what about any other case where we drive this
>>> reset path (e.g. subsystem reset, reboot, and the ISM-specific extra
>>> paths)
>>>
>>> Note that the non-error cases are ensuring we either leave this function
>>> with the device listed as disabled or standby/reserved.
>>>
>>> Would there be harm in, after performing the reset, doing
>>> pbdev->fh &= ~FH_MASK_ENABLE;
>>> pbdev->state = ZPCI_FS_DISABLED;
>>> for the ZPCI_FS_ERROR case now?
>>
>> One reason I didn't change the state was also because per my
>> understanding of architecture we need the guest to clear the error
>> state. This could be done via a mpcifc instruction (oc=7
>> ZPCI_MOD_FC_RESET_ERROR) or through a CLP set pci function enable/
> 
> That's an interesting one.  This series doesn't change that path, so it
> just resets the error state but we don't actually take any action, so I
> guess we just hope the device will now work and just didn't need
> recovery on the host or a disable/enable cycle.
> 
>> disable cycle. So if we leave the device in disabled state after a reset
>> without the guest driving any change, I think it might present a wrong
>> view to the guest. I am open to suggestions if you think it would be
>> more appropriate to keep the device in disabled state.
> 
> I get what you're saying, but the reality is that this reset code as
> implemented is going to get driven in a few ways:
> 
> 1) During the guest-initiated disable, quite likely in response to the
> PEC we injected if the device is in ZPCI_FS_ERROR.  Here the guest is
> responsible for clearing their error state as you say, but they're
> already on the way to doing that when you come through this path.
> They're doing the disable now, next step will be the enable.
> 
> 2) Subsystem reset that is potentially running in parallel with the
> guest recovery action (or the guest is not taking recovery action at
> all).  Here I don't believe the guest is responsible for clearing the
> error state actually -- either the device remains in an error state over
> the course of the subsystem reset (the way it used to work, because we
> had no way of fixing it) or, if we already did host recovery, we have

Just realized I got that part wrong -- before this patch we would also
just clear the error state and mark the device disabled during reset.

> reason to believe the device will now work and so it is free to be set
> to disabled because the guest will no longer have the prior context to
> believe it is responsible for clearing any error state -- it thinks it's
> starting 'from the beginning' due to the subsystem reset.  This is
> exactly why we disable the device in the reset path for an enabled
> device (that isn't in an error state) today, because that is the initial
> state the guest expects.
> 
> To say it another way, when the subsystem reset occurs it will trigger
> the device reset for all devices on the bus (plus a special direct
> invocation a bit earlier for ISM devices).  And the expectation after a
> subsystem reset is that the devices will be in a state such that the
> guest can clp enable them.  This is the case that I am concerned you are
> missing with this implementation -- if we hit this path during reboot
> (and we cannot assume the guest issued its own CLP disable during the
> reboot) then the next thing the guest will try is a CLP enable during
> boot, which I think will fail even though you will have already done the
> host recovery action.  And I actually think the failure will be not due
> to the state == ZPCI_FS_ERROR (because enable ignores that) but actually
> because you never disabled the handle -- so the guest will get
> CLP_RC_SETPCIFN_FHOP.
> 
> AFAICT the only way the guest will be able to clear that up and force
> the device to be usable again is to do a disable/enable cycle AFTER the
> subsystem reset (or use the mpcifc) to clear the handle.  That doesn't
> sound right either.  Yes, this is likely a small window (subsystem reset
> at the same time a PEC comes from the host) but I think it highlights
> the problem with leaving the device in ZPCI_FS_ERROR / handle enabled.
> 
> If you think placing the device into a disabled state straight away
> won't cover architecture, we could consider a new state to track when a
> device is in error state (ZPCI_FS_ERROR) vs a device that has been reset
> and is ready for the guest to clear the error state (ZPCI_FS_RECOVERED
> or something)?  The disable path can go straight from
> RECOVERED->DISABLED, but then the subsystem_reset path could have logic
> that ensures all devices that are ZPCI_FS_RECOVERED get moved to
> ZPCI_FS_DISABLED (and their handle masked to disabled) before the guest
> starts running again.  Any that are in STANDBY/RESERVED/ERROR stay that
> way, and everything else should already be DISABLED.
> 
> I'm not sure if that overhead buys us anything though vs short-pathing
> right to 'disabled', because I can't think of a case where we reset the
> device on a path where the guest isn't either already doing the
> error-state-clearing action, is no longer responsible for it, or the
> device is being removed from the guest configuration.
> 
> Thanks,
> Matt
> 

Re: [PATCH v6 1/2] s390x/pci: Add PCI error handling for vfio pci devices

[Matthew Rosato]

On 9/30/26 1:46 PM, Farhan Ali wrote:
> 
> On 9/30/2026 9:30 AM, Matthew Rosato wrote:
>>> +static int s390_pci_get_feature_err(VFIOPCIDevice *vfio_pci,
>>> +                                    PciCcdfErr *ccdf,
>>> +                                    uint32_t ccdf_err_length,
>>> +                                    Error **errp)
>>> +{
>>> +    ERRP_GUARD();
>>> +    int ret;
>>> +    size_t total_size;
>>> +    struct vfio_device_feature_zpci_err *err;
>>> +    g_autofree void *buf = NULL;
>>> +    g_autofree struct vfio_device_feature *feature = NULL;
>>> +
>>> +    total_size = sizeof(*feature) + sizeof(*err);
>>> +    feature = g_malloc(total_size);
>>> +    feature->argsz = total_size;
>>> +    feature->flags = VFIO_DEVICE_FEATURE_GET |
>>> VFIO_DEVICE_FEATURE_ZPCI_ERROR;
>>> +
>>> +    buf = g_malloc(ccdf_err_length);
>>> +    err = (void *)feature->data;
>>> +    err->data = (uint64_t)buf;
>>> +    ret = vfio_device_get_feature(&vfio_pci->vbasedev, feature);
>>> +
>>> +    if (ret) {
>>> +        error_setg(errp, "Failed feature get
>>> VFIO_DEVICE_FEATURE_ZPCI_ERROR"
>>> +                   " (rc=%d)", ret);
>> I see this was changed from last version, but doesn't this fall under
>> 'avoid useless error object creation and destruction' described in
>> qapi/error.h for the -ENOMSG return value?
>>
>> We create it here only to explicitly destroy it from the caller.
>>
>> Can we instead switch to the negative/non-negative return structure with
>> something like...
>>
>> <0: error as defined (so we set errp)
>> 0-N: number of errors to process (and we do not set errp)
>>
>> For the non-negative case we can realistically only have 0 (-ENOMSG maps
>> to this) or 1 (the feature found something - it is reporting 1 error to
>> handle).
> 
> I was trying to follow what I assumed to be existing pattern for
> handling expected errno[1]. I am okay to change it, but I would like to
> understand what is the preferred approach here.
> 
> [1] https://elixir.bootlin.com/qemu/v11.1.2/source/hw/vfio/iommufd.c#L408
> 
AFAICT the example you provided is using a shared routine where in some
cases the errp generated for -EINVAL is actually propagated vs just
thrown out immediately. (see line 483 of the same reference)

In your case, this isn't a shared routine and the errp associated with
-ENOMSG will never be used.

But anyway, I think we're at a point of semantics.

You made this change based on [a] from include/qapi/error.h:
 * - On success, the function should not touch *errp.

And you've done that successfully.

But I'm complaining about [b] from the same file:
 * - Whenever practical, also return a value that indicates success /
 *   failure.  This can make the error checking more concise, and can
 *   avoid useless error object creation and destruction.

AFAIU, the point is to always have an errp when there was a failure, and
to not have an errp when there is a success.

Do you consider the -ENOMSG return from the ioctl a success or a
failure?  It seems like a success to me (the ioctl worked, there was
just nothing to get), but you're treating it as an error case.  That
satisfies [a] but I'm suggesting it violates [b] since you create an
errp that is guaranteed to never be used.

So as for preferred approach for this: I would prefer not to create
Error objects that are guaranteed not to get used, but I will also not
reject the series on that alone.

Thanks,
Matt