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

[Alex Williamson]

On Tue, 7 Apr 2026 11:13:53 -0700
Farhan Ali <alifm@xxxxxxxxxxxxx> wrote:

> On 4/7/2026 8:53 AM, Alex Williamson wrote:
> > On Mon, 30 Mar 2026 10:40:09 -0700
> > Farhan Ali <alifm@xxxxxxxxxxxxx> wrote:
> >> diff --git a/drivers/vfio/pci/vfio_pci_zdev.c b/drivers/vfio/pci/vfio_pci_zdev.c
> >> index 0658095ac5b1..0a8663879eee 100644
> >> --- a/drivers/vfio/pci/vfio_pci_zdev.c
> >> +++ b/drivers/vfio/pci/vfio_pci_zdev.c
> >> @@ -141,6 +141,42 @@ 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_dev *zdev;
> >> +	int head = 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;
> >> +
> >> +	mutex_lock(&zdev->pending_errs_lock);
> >> +	if (zdev->pending_errs.count) {
> >> +		head = zdev->pending_errs.head % ZPCI_ERR_PENDING_MAX;
> >> +		err.pec = zdev->pending_errs.err[head].pec;
> >> +		zdev->pending_errs.head++;
> >> +		zdev->pending_errs.count--;
> >> +		err.pending_errors = zdev->pending_errs.count;
> >> +	}
> >> +	mutex_unlock(&zdev->pending_errs_lock);  
> > Inconsistent that this isn't a helper exported from the previous patch.  
> 
> Do you prefer it to be in a helper function? I can move it to a helper 
> function.

Yes, a helper that dequeues and returns the error pec with an arg of the
remaining count would make sense here.  Call it unconditionally and
specify 0/0 = no error and none pending.  All the locking and buffer
manipulation code is localized in s390 code.

> >
> > What's the meaning of err.pec = 0?  Could this be interpreted as an
> > error itself?  
> 
> An err.pec = 0 would indicate there are no pending errors. I don't think 
> anything would prevent userspace from doing a VFIO_DEVICE_FEATURE_GET 
> even if not nudged by an eventfd?

Maybe worth specifying in the feature description.  Thanks,

Alex