On 9/2/2026 1:22 AM, Cédric Le Goater wrote:
On 8/31/26 20:31, Farhan Ali wrote:Add an s390x specific callback for vfio error handling. 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_FEATUREioctl which exposes that information.Once this error information is retrieved we can then inject an error intothe guest, and let the guest drive the recovery. Signed-off-by: Farhan Ali <[email protected]> --- hw/s390x/s390-pci-bus.c | 9 +++ hw/s390x/s390-pci-vfio-stubs.c | 6 ++ hw/s390x/s390-pci-vfio.c | 100 +++++++++++++++++++++++++++++++ include/hw/s390x/s390-pci-bus.h | 2 + include/hw/s390x/s390-pci-vfio.h | 1 + 5 files changed, 118 insertions(+) diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c index 9ecf3c30ad..b2db7f3df4 100644 --- a/hw/s390x/s390-pci-bus.c +++ b/hw/s390x/s390-pci-bus.c@@ -160,6 +160,8 @@ static void s390_pci_perform_unplug(S390PCIBusDevice *pbdev){ HotplugHandler *hotplug_ctrl; + qemu_mutex_destroy(&pbdev->err_handler_lock); + if (pbdev->pft == ZPCI_PFT_ISM) { notifier_remove(&pbdev->shutdown_notifier); }@@ -1123,6 +1125,7 @@ static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev,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)) {@@ -1189,6 +1192,7 @@ static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev,pbdev->iommu->pbdev = pbdev; pbdev->state = ZPCI_FS_DISABLED; set_pbdev_info(pbdev); + qemu_mutex_init(&pbdev->err_handler_lock); if (object_dynamic_cast(OBJECT(dev), "vfio-pci")) { /*@@ -1213,6 +1217,11 @@ static void s390_pcihost_plug(HotplugHandler *hotplug_dev, DeviceState *dev, 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.cindex 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..9a63040e22 100644 --- a/hw/s390x/s390-pci-vfio.c +++ b/hw/s390x/s390-pci-vfio.c@@ -105,6 +105,73 @@ void s390_pci_end_dma_count(S390pciState *s, S390PCIDMACount *cnt)} } +static int s390_pci_get_feature_err(VFIOPCIDevice *vfio_pci,returning a bool would be preferred unless the errno is important
I can change it to bool. Don't have a strong preference here.
+ PciCcdfErr *ccdf, + uint32_t ccdf_err_length, + Error **errp) +{ + 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) { + if (ret != -ENOMSG) {+ 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 bool s390_pci_err_handler(VFIOPCIDevice *vfio_pci, Error **errp) +{ + S390PCIBusDevice *pbdev; + PciCcdfErr ccdf; + bool success = false;This means errp should be set. It is not obvious from the code below.+ int ret = 0; + + pbdev = s390_pci_find_dev_by_target(s390_get_phb(), + DEVICE(&vfio_pci->parent_obj)->id);can pbdev be NULL ? if (!pbdev) { error_setg(errp, "No matching zpci device found"); return false; }+ + QEMU_LOCK_GUARD(&pbdev->err_handler_lock); + pbdev->state = ZPCI_FS_ERROR; + + if (sizeof(ccdf) != pbdev->ccdf_err_length) { + error_setg(errp,+ "CCDF size mismatch expected size=%zu, provided size=%d",+ sizeof(ccdf), pbdev->ccdf_err_length); + return false; + } + + while (ret == 0) { + ret = s390_pci_get_feature_err(vfio_pci, &ccdf, + pbdev->ccdf_err_length, errp); + if (ret) { + success = ret == -ENOMSG ? true : false; + break; + }+ s390_pci_generate_error_event(ccdf.pec, pbdev->fh, pbdev->fid, 0, 0);why not use ccdf.e and ccdf.faddr for more diagnostic details ?
That's a fair point, will add that. I think this was based on the older version which only had the pec as the valid information.
Thanks Farhan
+ } + + return success; +} + static void s390_pci_read_base(S390PCIBusDevice *pbdev, struct vfio_device_info *info) {@@ -134,6 +201,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 +442,32 @@ 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; + 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; + } + + vfio_pci->err_handler = s390_pci_err_handler; + + return true; +}diff --git a/include/hw/s390x/s390-pci-bus.h b/include/hw/s390x/s390-pci-bus.hindex 9228523ce8..bc67f7e065 100644 --- a/include/hw/s390x/s390-pci-bus.h +++ b/include/hw/s390x/s390-pci-bus.h @@ -364,6 +364,8 @@ struct S390PCIBusDevice { bool forwarding_assist; bool aif; bool rtr_avail; + QemuMutex err_handler_lock; + 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.hindex 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
