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

[Farhan Ali <alifm@linux.ibm.com>]

On 4/14/2026 12:43 PM, Niklas Schnelle wrote:
> 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

My concern with exposing the entire struct zpci_ccdf_err was it could be 
error prone trying to keep the struct and vfio uAPI struct in sync. Just 
thinking out loud, to query the length we would need a different vfio 
device feature bit? But if it makes for a better uAPI, I can make the 
change. Thanks

Farhan