Re: [PATCH v2 3/4] s390x/pci: Add PCI error handling for vfio pci devices

[Matthew Rosato <mjrosato@linux.ibm.com>]

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 <alifm@linux.ibm.com>
>>> ---
>>>   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...

>>> +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.