Re: [PATCH v6 1/2] s390x/pci: Add PCI error handling for vfio pci devices

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

On 9/30/2026 9:30 AM, Matthew Rosato wrote:
>> +static int s390_pci_get_feature_err(VFIOPCIDevice *vfio_pci,
>> +                                    PciCcdfErr *ccdf,
>> +                                    uint32_t ccdf_err_length,
>> +                                    Error **errp)
>> +{
>> +    ERRP_GUARD();
>> +    int ret;
>> +    size_t total_size;
>> +    struct vfio_device_feature_zpci_err *err;
>> +    g_autofree void *buf = NULL;
>> +    g_autofree struct vfio_device_feature *feature = NULL;
>> +
>> +    total_size = sizeof(*feature) + sizeof(*err);
>> +    feature = g_malloc(total_size);
>> +    feature->argsz = total_size;
>> +    feature->flags = VFIO_DEVICE_FEATURE_GET | VFIO_DEVICE_FEATURE_ZPCI_ERROR;
>> +
>> +    buf = g_malloc(ccdf_err_length);
>> +    err = (void *)feature->data;
>> +    err->data = (uint64_t)buf;
>> +    ret = vfio_device_get_feature(&vfio_pci->vbasedev, feature);
>> +
>> +    if (ret) {
>> +        error_setg(errp, "Failed feature get VFIO_DEVICE_FEATURE_ZPCI_ERROR"
>> +                   " (rc=%d)", ret);
> I see this was changed from last version, but doesn't this fall under
> 'avoid useless error object creation and destruction' described in
> qapi/error.h for the -ENOMSG return value?
>
> We create it here only to explicitly destroy it from the caller.
>
> Can we instead switch to the negative/non-negative return structure with
> something like...
>
> <0: error as defined (so we set errp)
> 0-N: number of errors to process (and we do not set errp)
>
> For the non-negative case we can realistically only have 0 (-ENOMSG maps
> to this) or 1 (the feature found something - it is reporting 1 error to
> handle).

I was trying to follow what I assumed to be existing pattern for 
handling expected errno[1]. I am okay to change it, but I would like to 
understand what is the preferred approach here.

[1] https://elixir.bootlin.com/qemu/v11.1.2/source/hw/vfio/iommufd.c#L408


>> +        return ret;
>> +    }
>> +
>> +    memcpy(ccdf, (PciCcdfErr *) err->data, ccdf_err_length);
> You already had err->data in void *buf, can we just
>
> memcpy(ccdf, buf, ccdf_err_length);
>
Sure, I can make the change.


>> +
>> +    return 0;
>> +}
>> +
>> +static void s390_pci_err_handler(void *opaque)
>> +{
>> +    VFIOPCIDevice *vfio_pci;
>> +    S390PCIBusDevice *pbdev;
>> +    Error *local_err = NULL;
>> +    PciCcdfErr ccdf;
>> +    int ret = 0;
>> +
>> +    vfio_pci = opaque;
>> +    if (!event_notifier_test_and_clear(&vfio_pci->err_notifier)) {
>> +        return;
>> +    }
> Another one I should have commented on during v5...  You're right about
> the recent patch and Z support, but what if the patch is missing on the
> host kernel?
>
> I'm fine with the feature not working with AER is off rather than doing
> our own setup, but don't we need to avoid referencing this in that case
> and ideally give some log message?

Discussed this offline and will add a check to see if the err_notifier 
is registered. Though this is not strictly necessary, but better to add 
a defensive check.


>> +
>> +    pbdev = s390_pci_find_dev_by_target(s390_get_phb(),
>> +                                        DEVICE(&vfio_pci->parent_obj)->id);
>> +
> We can get the pci device from the vfio-pci device, so would it work to
> just do this?
>
> s390_pci_find_dev_by_pci(s390_get_phb(), PCI_DEVICE(vfio_pci));
>
> Then we cut out the strcmps

Yup, can change this.

Thanks

Farhan