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

[Farhan Ali]

On 4/14/2026 7:12 AM, Alex Williamson wrote:
> On Mon, 13 Apr 2026 16:40:49 -0700
> Farhan Ali <alifm@xxxxxxxxxxxxx> wrote:
>
> > On 4/13/2026 3:57 PM, Alex Williamson wrote:
> > > On Mon, 13 Apr 2026 14:06:06 -0700
> > > Farhan Ali <alifm@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> > > > Signed-off-by: Farhan Ali <alifm@xxxxxxxxxxxxx>
> > > > ---
> > > >    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