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

[Matthew Rosato <mjrosato@linux.ibm.com>]

> +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).

> +        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);


> +
> +    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?

> +
> +    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

Thanks,
Matt