On 9/3/25 19:49, Matthew Rosato wrote:
On 9/3/25 1:12 PM, Farhan Ali wrote:
On 9/1/2025 4:25 AM, Cédric Le Goater wrote:
On 8/25/25 23:24, 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 memory region 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 <[email protected]>
---
hw/s390x/s390-pci-bus.c | 5 +++
hw/s390x/s390-pci-vfio.c | 76 ++++++++++++++++++++++++++++++++
include/hw/s390x/s390-pci-bus.h | 1 +
include/hw/s390x/s390-pci-vfio.h | 2 +
4 files changed, 84 insertions(+)
diff --git a/hw/s390x/s390-pci-bus.c b/hw/s390x/s390-pci-bus.c
index f87d2748b6..af42eb9938 100644
--- a/hw/s390x/s390-pci-bus.c
+++ b/hw/s390x/s390-pci-bus.c
@@ -158,6 +158,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);
}
@@ -1140,6 +1142,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")) {
/*
@@ -1164,6 +1167,8 @@ 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 */
+ s390_pci_setup_err_handler(pbdev);
This can fail. Please add an 'Error **' parameter and change the returned
value to bool.
I wanted to avoid hard failing here as we can have mismatch in kernel and QEMU
support for the feature. For example we can have a newer QEMU version with the
feature running on an older kernel. So wanted to treat any error in setting up
the error handler would be more of an info/warn message.
+1, please do not cause a hard failure if the underlying host kernel is simply
missing support...
It doesn't have to be a hard failure. The code could issue a
warn_report_err(). Similarly to what is done for the vfio_ap
device when IRQ notifying support is not available on the host.
This is minor.
C.
+void s390_pci_setup_err_handler(S390PCIBusDevice *pbdev)
+{
+ int ret;
+ VFIOPCIDevice *vfio_pci = container_of(pbdev->pdev, VFIOPCIDevice, 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_pci->vbasedev.io_ops->device_feature(&vfio_pci->vbasedev,
+ feature);
Please introduce vfio helpers to hide the internal indirection :
->vbasedev.io_ops->device_feature(...)
+
+ if (ret) {
Shouldn't we test the return value to decide if the error is
an unimplemented feature or an unexpected error ?
Yeah, I think it makes sense separate out error for unimplemented feature
(ENOTTY) vs any other unexpected error. Will change this.
... But if you add differentiation here between the 2 types of errors then I
would be fine with hard-fail for unexpected cases and info/warn for missing
host kernel support.
