Re: [PATCH v13 5/7] vfio-pci/zdev: Add a device feature for error information

Farhan Ali
2026-04-14T17:13:31Z

On 4/14/2026 7:12 AM, Alex Williamson wrote:
> On Mon, 13 Apr 2026 16:40:49 -0700
> Farhan Ali <alifm@linux.ibm.com> wrote:
>
>> On 4/13/2026 3:57 PM, Alex Williamson wrote:
>>> On Mon, 13 Apr 2026 14:06:06 -0700
>>> Farhan Ali <alifm@linux.ibm.com> wrote:
>>>
>>>> For zPCI devices, we have platform specific error information. The platform
>>>> firmware provides this error information to the operating system in an
>>>> architecture specific mechanism. To enable recovery from userspace for
>>>> these devices, we want to expose this error information to userspace. Add a
>>>> new device feature to expose this information.
>>>>
>>>> Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
>>>> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
>>>> ---
>>>> arch/s390/include/asm/pci.h | 3 +++
>>>> arch/s390/pci/pci_event.c | 19 +++++++++++++++++++
>>>> drivers/vfio/pci/vfio_pci_core.c | 2 ++
>>>> drivers/vfio/pci/vfio_pci_priv.h | 9 +++++++++
>>>> drivers/vfio/pci/vfio_pci_zdev.c | 31 +++++++++++++++++++++++++++++++
>>>> include/uapi/linux/vfio.h | 20 ++++++++++++++++++++
>>>> 6 files changed, 84 insertions(+)
>>>>
>>>> diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
>>>> index 9a6a4eb9d7c1..9c8ee97d7e8a 100644
>>>> --- a/arch/s390/include/asm/pci.h
>>>> +++ b/arch/s390/include/asm/pci.h
>>>> @@ -360,6 +360,9 @@ int zpci_clear_error_state(struct zpci_dev *zdev);
>>>> int zpci_reset_load_store_blocked(struct zpci_dev *zdev);
>>>> void zpci_start_mediated_recovery(struct zpci_dev *zdev);
>>>> void zpci_stop_mediated_recovery(struct zpci_dev *zdev);
>>>> +void zpci_get_pending_error_and_count(struct zpci_dev *zdev,
>>>> + struct zpci_ccdf_err *ccdf,
>>>> + int *count);
>>>>
>>>> #ifdef CONFIG_NUMA
>>>>
>>>> diff --git a/arch/s390/pci/pci_event.c b/arch/s390/pci/pci_event.c
>>>> index c279a9f50a64..c8714d4a32fa 100644
>>>> --- a/arch/s390/pci/pci_event.c
>>>> +++ b/arch/s390/pci/pci_event.c
>>>> @@ -74,6 +74,25 @@ static void zpci_store_pci_error(struct pci_dev *pdev,
>>>> zdev->pending_errs.count++;
>>>> }
>>>>
>>>> +void zpci_get_pending_error_and_count(struct zpci_dev *zdev,
>>>> + struct zpci_ccdf_err *ccdf,
>>>> + int *count)
>>>> +{
>>>> + int head = 0;
>>> Unnecessary. Should also be a blank line between variable declaration
>>> and code.
>>>
>>>> + *count = 0;
>>> But why do we zero this and not ccdf?
>> I had thought of memsetting ccdf to 0, but the only caller right already
>> initializes to 0. So didn't think it was needed.
> The caller initializes count as well.
>
>>>> +
>>>> + guard(mutex)(&zdev->pending_errs_lock);
>>>> + if (zdev->pending_errs.count) {
>>>> + head = zdev->pending_errs.head % ZPCI_ERR_PENDING_MAX;
>>>> + memcpy(ccdf, &zdev->pending_errs.err[head],
>>>> + sizeof(struct zpci_ccdf_err));
>>>> + zdev->pending_errs.head++;
>>>> + zdev->pending_errs.count--;
>>>> + *count = zdev->pending_errs.count;
>>>> + }
>>>> +}
>>> You've describe in the uAPI now how pec = 0 means no error, but why not
>>> remove that ambiguity altogether and return -ENOMSG in that case. We
>>> could start here and pass it through:
>>>
>>> {
>>> int head;
>>>
>>> guard(mutex)(&zdev->pending_errs_lock);
>>>
>>> if (!zdev->pending_errs.count)
>>> return -ENOMSG;
>>>
>>> head = zdev->pending_errs.head % ZPCI_ERR_PENDING_MAX;
>>> memcpy(ccdf, &zdev->pending_errs.err[head],
>>> sizeof(struct zpci_ccdf_err));
>>> zdev->pending_errs.head++;
>>> zdev->pending_errs.count--;
>>> *count = zdev->pending_errs.count;
>>>
>>> return 0;
>>> }
>> I had thought of returning an error (ENOENT was what I was going with),
>> but perhaps I misunderstood your comment from v12 about specifying pec =
>> 0 and pending error = 0 as not being an error. My assumption was as this
>> is not an error we shouldn't return an error to userspace.
> I think I was trying to get clarity on how 0/0 is interpreted because
> it seems like a weak point of the uAPI. We could make a more intuitive
> uAPI if we make the "I have no message of desired type" response an
> explicit, unique errno, rather than a reserved pec value.
>
>>>> +EXPORT_SYMBOL_GPL(zpci_get_pending_error_and_count);
>>>> +
>>>> void zpci_start_mediated_recovery(struct zpci_dev *zdev)
>>>> {
>>>> guard(mutex)(&zdev->pending_errs_lock);
>>>> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
>>>> index ad52abc46c04..5403730786a1 100644
>>>> --- a/drivers/vfio/pci/vfio_pci_core.c
>>>> +++ b/drivers/vfio/pci/vfio_pci_core.c
>>>> @@ -1534,6 +1534,8 @@ int vfio_pci_core_ioctl_feature(struct vfio_device *device, u32 flags,
>>>> return vfio_pci_core_feature_token(vdev, flags, arg, argsz);
>>>> case VFIO_DEVICE_FEATURE_DMA_BUF:
>>>> return vfio_pci_core_feature_dma_buf(vdev, flags, arg, argsz);
>>>> + case VFIO_DEVICE_FEATURE_ZPCI_ERROR:
>>>> + return vfio_pci_zdev_feature_err(device, flags, arg, argsz);
>>>> default:
>>>> return -ENOTTY;
>>>> }
>>>> diff --git a/drivers/vfio/pci/vfio_pci_priv.h b/drivers/vfio/pci/vfio_pci_priv.h
>>>> index fca9d0dfac90..4e7162234a2e 100644
>>>> --- a/drivers/vfio/pci/vfio_pci_priv.h
>>>> +++ b/drivers/vfio/pci/vfio_pci_priv.h
>>>> @@ -93,6 +93,8 @@ int vfio_pci_info_zdev_add_caps(struct vfio_pci_core_device *vdev,
>>>> struct vfio_info_cap *caps);
>>>> int vfio_pci_zdev_open_device(struct vfio_pci_core_device *vdev);
>>>> void vfio_pci_zdev_close_device(struct vfio_pci_core_device *vdev);
>>>> +int vfio_pci_zdev_feature_err(struct vfio_device *device, u32 flags,
>>>> + void __user *arg, size_t argsz);
>>>> #else
>>>> static inline int vfio_pci_info_zdev_add_caps(struct vfio_pci_core_device *vdev,
>>>> struct vfio_info_cap *caps)
>>>> @@ -107,6 +109,13 @@ static inline int vfio_pci_zdev_open_device(struct vfio_pci_core_device *vdev)
>>>>
>>>> static inline void vfio_pci_zdev_close_device(struct vfio_pci_core_device *vdev)
>>>> {}
>>>> +
>>>> +static inline int vfio_pci_zdev_feature_err(struct vfio_device *device,
>>>> + u32 flags, void __user *arg,
>>>> + size_t argsz)
>>>> +{
>>>> + return -ENOTTY;
>>>> +}
>>>> #endif
>>>>
>>>> static inline bool vfio_pci_is_vga(struct pci_dev *pdev)
>>>> diff --git a/drivers/vfio/pci/vfio_pci_zdev.c b/drivers/vfio/pci/vfio_pci_zdev.c
>>>> index 0658095ac5b1..ee1647f0ffe6 100644
>>>> --- a/drivers/vfio/pci/vfio_pci_zdev.c
>>>> +++ b/drivers/vfio/pci/vfio_pci_zdev.c
>>>> @@ -141,6 +141,37 @@ int vfio_pci_info_zdev_add_caps(struct vfio_pci_core_device *vdev,
>>>> return ret;
>>>> }
>>>>
>>>> +int vfio_pci_zdev_feature_err(struct vfio_device *device, u32 flags,
>>>> + void __user *arg, size_t argsz)
>>>> +{
>>>> + struct vfio_device_feature_zpci_err err = {};
>>>> + struct vfio_pci_core_device *vdev;
>>>> + struct zpci_ccdf_err ccdf = {};
>>>> + struct zpci_dev *zdev;
>>>> + int pending_errors = 0;
>>>> + int ret;
>>>> +
>>>> + vdev = container_of(device, struct vfio_pci_core_device, vdev);
>>>> + zdev = to_zpci(vdev->pdev);
>>>> + if (!zdev)
>>>> + return -ENODEV;
>>>> +
>>>> + ret = vfio_check_feature(flags, argsz, VFIO_DEVICE_FEATURE_GET,
>>>> + sizeof(err));
>>>> + if (ret != 1)
>>>> + return ret;
>>>> +
>>>> + zpci_get_pending_error_and_count(zdev, &ccdf, &pending_errors);
>>>> +
>>>> + err.version = 1;
>>>> + err.pec = ccdf.pec;
>>>> + err.pending_errors = pending_errors;
>>>> + if (copy_to_user(arg, &err, sizeof(err)))
>>>> + return -EFAULT;
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> int vfio_pci_zdev_open_device(struct vfio_pci_core_device *vdev)
>>>> {
>>>> struct zpci_dev *zdev = to_zpci(vdev->pdev);
>>>> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
>>>> index 5de618a3a5ee..2980ca39dd38 100644
>>>> --- a/include/uapi/linux/vfio.h
>>>> +++ b/include/uapi/linux/vfio.h
>>>> @@ -1534,6 +1534,26 @@ struct vfio_device_feature_dma_buf {
>>>> */
>>>> #define VFIO_DEVICE_FEATURE_MIG_PRECOPY_INFOv2 12
>>>>
>>>> +/**
>>>> + * VFIO_DEVICE_FEATURE_ZPCI_ERROR feature provides PCI error information to
>>>> + * userspace for vfio-pci devices on s390x. On s390x, PCI error recovery
>>>> + * involves platform firmware and notification to operating system is done
>>>> + * by architecture specific mechanism. Exposing this information to
>>>> + * userspace allows it to take appropriate actions to handle an
>>>> + * error on the device. The pending_errors provide any additional errors
>>>> + * pending for the device, and userspace should read until zero. A value of
>>>> + * 0 for pending_errors and pec would indicate no pending errors that need
>>>> + * to be handled.
>>>> + */
>>>> +
>>>> +struct vfio_device_feature_zpci_err {
>>>> + __u8 version;
>>>> + __u8 pending_errors;
>>>> + __u16 pec;
>>>> +};
>>> I assume .version is for compatibility, but we don't define a strategy
>>> for using it or specify what the version should be for this table. It
>>> doesn't seem like there's actually an value-add to having it.
>> Its possible we may need to extend this structure in the future if we
>> want to report more information to userspace. I at least want the
>> flexibility to do so. We had some discussion around this [1] in an
>> earlier version. I was trying to follow similar versioning pattern we
>> had around vfio-pci/zdev structures.
> IMHO, the version field is a dead end towards achieving this,
> especially if we don't specify from the onset the expected version
> value or the compatibility semantics. All that's going to happen is
> that some userspace will hard code that it understands version 1
> because that's what's currently reported and matches the struct defined
> here, and you can never ever report anything other than version 1
> without breaking that user. At that point you need to come up with
> some other means for the user to opt-in to a new version, whether it's
> triggered by another feature (as we did with the PRECOPY_INFOv2 above
> this), or we reimplement the whole v2 feature.

My understanding was based on how we version some of the capability
structures for zdev (in include/uapi/linux/vfio_zdev.h). If we wanted to
provide more information to userspace in the future, what would be
preferred approach? Do we need to explicitly define a v2 feature? I am
open to suggestions on this.

If we need to define v2 explicitly in the future, then yes I agree we
can simplify it to return only the PEC code (or an error code otherwise).

Thanks

Farhan