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

[Niklas Schnelle <schnelle@linux.ibm.com>]

On Tue, 2026-04-14 at 11:45 -0700, Farhan Ali wrote:
> On 4/14/2026 10:41 AM, Alex Williamson wrote:
> > On Tue, 14 Apr 2026 10:13:22 -0700
> > Farhan Ali <alifm@linux.ibm.com> wrote:
> > > 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:
> > > > > > > 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).
> > Maybe I'm reading too much into it, but it sounds like you already have
> > plans to expand this.
> > 
> > The existing zdev structures seem to rely on the version field in the
> > capability header and maybe you've gotten away with bumping the version
> > without breaking userspace, but it's fragile.  AFAICT, the ioctls do not
> > define a versioning strategy where vN+1 only adds fields to vN, so all
> > it takes is one outspoken userspace tool that hard codes its
> > compatibility to a specific version to become a problem for further
> > updates.
> > 
> > Likewise this feature doesn't define a versioning policy for userspace
> > to follow.  I'm sure there are other examples within vfio that are
> > problematic, but let's try not to create more.
> > 
> > If you want to use a version, then also define what the version is and
> > what the compatibility policy is for future versions.  A flags field is
> > another option that we've used extensively in vfio.  The version field
> > might be better for incremental expansion of the structure, while flags
> > can address specific fields more directly, ex. a reserved field being
> > redefined.  Thanks,
> > 
> > Alex
> 
> Just wanted to understand and clarify if we can associate multiple 
> fields with a feature flag? I think in that case flags would be better 
> here and given its also something that is widely used in vfio. I am just 
> trying to figure out the best way we can extend this without a lot of 
> code churn in the future.
> 
> I appreciate the feedback and discussion on this.
> 
> Thanks
> 
> Farhan

Here is an idea. I think we even discussed this internally at some
point. How about we just use the architected CCDF Error Event (i.e.
basically struct zpci_ccdf_err with possible uAPI type adjustments) as
the data user-space reads. The bit definitions of it are already part
of the architecture and it has provisions for extensibility based on
s390 architecture principles which have worked quite well for forward
compatibility. If we additionally make it possible to query its length
as kind of a version it could even grow. I think this would also work
well with the idea of returning an error on read if there are no more
error events and user-space reading again and again until it sees the
error. I agree with Alex that this seems much easier to get right than
a pending_errs count. As a bonus we can immediately expose all the
error event information Linux currently uses. That said, I would opt to
setting the struct fields by copying field by field explicitly and
leaving reserved fields 0 such that we don't pass-through any bits a
future platform may add without having a defined field in Linux.
Thoughts?

Thanks,
Niklas