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

["Cédric Le Goater" <clg@redhat.com>]

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

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.