[v2,1/2] vfio-pci: Introduce vfio_set_event_handler helper
diff mbox series

Message ID 20190117210632.18567-2-eric.auger@redhat.com
State New
Headers show
Series
  • vfio-pci: Introduce vfio_set_event_handler()
Related show

Commit Message

Auger Eric Jan. 17, 2019, 9:06 p.m. UTC
The code used to attach the eventfd handler for the ERR and
REQ irq indices can be factorized into a helper. In subsequent
patches we will extend this helper to support other irq indices.

We test whether the notification is allowed outside of the helper:
respectively check vdev->pci_aer and VFIO_FEATURE_ENABLE_REQ.
Depending on the returned value we set vdev->pci_aer and
vdev->req_enabled. An error handle is introduced for future usage
although not strictly useful here.

Signed-off-by: Eric Auger <eric.auger@redhat.com>

---

v1 -> v2:
- s/vfio_register_event_notifier/vfio_set_event_handler
- turned into a void function
- do the qemu_set_fd_handler(*pfd, NULL, NULL, vdev) and
  event_notifier_cleanup on VFIO_DEVICE_SET_IRQS failure
  as reported by Alexey
- reset err in vfio_realize as reported by Cornelia
- Text/comment fixes suggested by Cornelia
---
 hw/vfio/pci.c | 296 ++++++++++++++++++++++----------------------------
 1 file changed, 132 insertions(+), 164 deletions(-)

Comments

Alex Williamson Jan. 22, 2019, 7:51 p.m. UTC | #1
On Thu, 17 Jan 2019 22:06:31 +0100
Eric Auger <eric.auger@redhat.com> wrote:

> The code used to attach the eventfd handler for the ERR and
> REQ irq indices can be factorized into a helper. In subsequent
> patches we will extend this helper to support other irq indices.
> 
> We test whether the notification is allowed outside of the helper:
> respectively check vdev->pci_aer and VFIO_FEATURE_ENABLE_REQ.
> Depending on the returned value we set vdev->pci_aer and
> vdev->req_enabled. An error handle is introduced for future usage
> although not strictly useful here.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> 
> ---
> 
> v1 -> v2:
> - s/vfio_register_event_notifier/vfio_set_event_handler
> - turned into a void function
> - do the qemu_set_fd_handler(*pfd, NULL, NULL, vdev) and
>   event_notifier_cleanup on VFIO_DEVICE_SET_IRQS failure
>   as reported by Alexey
> - reset err in vfio_realize as reported by Cornelia
> - Text/comment fixes suggested by Cornelia
> ---
>  hw/vfio/pci.c | 296 ++++++++++++++++++++++----------------------------
>  1 file changed, 132 insertions(+), 164 deletions(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index c0cb1ec289..3cae4c99ef 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -105,6 +105,96 @@ static void vfio_intx_eoi(VFIODevice *vbasedev)
>      vfio_unmask_single_irqindex(vbasedev, VFIO_PCI_INTX_IRQ_INDEX);
>  }
>  
> +/*
> + * vfio_set_event_handler - setup/tear down eventfd
> + * notification and handling for IRQ indices that span over
> + * a single IRQ
> + *
> + * @vdev: VFIO device handle
> + * @index: IRQ index the eventfd/handler is associated with
> + * @enable: true means notifier chain needs to be set up
> + * @handler: handler to attach if @enable is true

Therefore @enable is redundant.

> + * @errp: error handle
> + */
> +static void vfio_set_event_handler(VFIOPCIDevice *vdev,
> +                                   int index,
> +                                   bool enable,
> +                                   void (*handler)(void *opaque),
> +                                   Error **errp)
> +{
> +    struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info),
> +                                      .index = index };
> +    struct vfio_irq_set *irq_set;
> +    EventNotifier *notifier;
> +    int argsz, ret = 0;
> +    int32_t *pfd, fd;
> +
> +    switch (index) {
> +    case VFIO_PCI_REQ_IRQ_INDEX:
> +        notifier = &vdev->req_notifier;
> +        break;
> +    case VFIO_PCI_ERR_IRQ_INDEX:
> +        notifier = &vdev->err_notifier;
> +        break;

This blows the abstraction of a helper that this seems to be trying to
create, seems cleaner to pass @notifier.

> +    default:
> +        g_assert_not_reached();
> +    }
> +
> +    if (ioctl(vdev->vbasedev.fd,
> +              VFIO_DEVICE_GET_IRQ_INFO, &irq_info) < 0 || irq_info.count < 1) {
> +        error_setg_errno(errp, errno, "No irq index %d available", index);
> +        return;

The implicit count, aka sub-index, is also problematic for the
abstraction.  Can we tackle applying this to MSI/X to validate if this
needs to go another step to allow the caller to specify index and
sub-index?

> +    }
> +
> +    if (enable) {
> +        ret = event_notifier_init(notifier, 0);
> +        if (ret) {
> +            error_setg_errno(errp, -ret,
> +                             "Unable to init event notifier for irq index %d",
> +                             index);
> +            return;
> +        }
> +    }
> +
> +    argsz = sizeof(*irq_set) + sizeof(*pfd);
> +
> +    irq_set = g_malloc0(argsz);
> +    irq_set->argsz = argsz;
> +    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
> +                     VFIO_IRQ_SET_ACTION_TRIGGER;
> +    irq_set->index = index;
> +    irq_set->start = 0;
> +    irq_set->count = 1;
> +    pfd = (int32_t *)&irq_set->data;
> +
> +    if (!notifier) {
> +        error_setg(errp, "Notifier not initialized for irq index %d", index);
> +        return;
> +    }

What is this supposed to check?  @notifier is not NULL initialized, the
case statement will assert if it doesn't get set, and this doesn't
actually test if it's properly initialized.

> +
> +    fd = event_notifier_get_fd(notifier);
> +
> +    if (enable) {
> +        qemu_set_fd_handler(fd, handler, NULL, vdev);
> +        *pfd = fd;
> +    } else {
> +        *pfd = -1;
> +    }
> +
> +    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
> +    g_free(irq_set);
> +
> +    if (ret) {
> +        error_setg_errno(errp, -ret,
> +                         "Failed to %s eventfd signalling for index %d",
> +                         enable ? "set up" : "tear down", index);
> +    }
> +    if (ret || !enable) {
> +        qemu_set_fd_handler(fd, NULL, NULL, vdev);
> +        event_notifier_cleanup(notifier);
> +    }
> +}

Suggest passing @notifier as a parameter and using @handler in place of
@enable, more generic and more obvious calling convention.

> +
>  static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev, Error **errp)
>  {
>  #ifdef CONFIG_KVM
> @@ -2621,86 +2711,6 @@ static void vfio_err_notifier_handler(void *opaque)
>      vm_stop(RUN_STATE_INTERNAL_ERROR);
>  }
>  
> -/*
> - * Registers error notifier for devices supporting error recovery.
> - * If we encounter a failure in this function, we report an error
> - * and continue after disabling error recovery support for the
> - * device.
> - */
> -static void vfio_register_err_notifier(VFIOPCIDevice *vdev)
> -{
> -    int ret;
> -    int argsz;
> -    struct vfio_irq_set *irq_set;
> -    int32_t *pfd;
> -
> -    if (!vdev->pci_aer) {
> -        return;
> -    }
> -
> -    if (event_notifier_init(&vdev->err_notifier, 0)) {
> -        error_report("vfio: Unable to init event notifier for error detection");
> -        vdev->pci_aer = false;
> -        return;
> -    }
> -
> -    argsz = sizeof(*irq_set) + sizeof(*pfd);
> -
> -    irq_set = g_malloc0(argsz);
> -    irq_set->argsz = argsz;
> -    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
> -                     VFIO_IRQ_SET_ACTION_TRIGGER;
> -    irq_set->index = VFIO_PCI_ERR_IRQ_INDEX;
> -    irq_set->start = 0;
> -    irq_set->count = 1;
> -    pfd = (int32_t *)&irq_set->data;
> -
> -    *pfd = event_notifier_get_fd(&vdev->err_notifier);
> -    qemu_set_fd_handler(*pfd, vfio_err_notifier_handler, NULL, vdev);
> -
> -    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
> -    if (ret) {
> -        error_report("vfio: Failed to set up error notification");
> -        qemu_set_fd_handler(*pfd, NULL, NULL, vdev);
> -        event_notifier_cleanup(&vdev->err_notifier);
> -        vdev->pci_aer = false;
> -    }
> -    g_free(irq_set);
> -}
> -
> -static void vfio_unregister_err_notifier(VFIOPCIDevice *vdev)
> -{
> -    int argsz;
> -    struct vfio_irq_set *irq_set;
> -    int32_t *pfd;
> -    int ret;
> -
> -    if (!vdev->pci_aer) {
> -        return;
> -    }
> -
> -    argsz = sizeof(*irq_set) + sizeof(*pfd);
> -
> -    irq_set = g_malloc0(argsz);
> -    irq_set->argsz = argsz;
> -    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
> -                     VFIO_IRQ_SET_ACTION_TRIGGER;
> -    irq_set->index = VFIO_PCI_ERR_IRQ_INDEX;
> -    irq_set->start = 0;
> -    irq_set->count = 1;
> -    pfd = (int32_t *)&irq_set->data;
> -    *pfd = -1;
> -
> -    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
> -    if (ret) {
> -        error_report("vfio: Failed to de-assign error fd: %m");
> -    }
> -    g_free(irq_set);
> -    qemu_set_fd_handler(event_notifier_get_fd(&vdev->err_notifier),
> -                        NULL, NULL, vdev);
> -    event_notifier_cleanup(&vdev->err_notifier);
> -}
> -
>  static void vfio_req_notifier_handler(void *opaque)
>  {
>      VFIOPCIDevice *vdev = opaque;
> @@ -2716,86 +2726,6 @@ static void vfio_req_notifier_handler(void *opaque)
>      }
>  }
>  
> -static void vfio_register_req_notifier(VFIOPCIDevice *vdev)
> -{
> -    struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info),
> -                                      .index = VFIO_PCI_REQ_IRQ_INDEX };
> -    int argsz;
> -    struct vfio_irq_set *irq_set;
> -    int32_t *pfd;
> -
> -    if (!(vdev->features & VFIO_FEATURE_ENABLE_REQ)) {
> -        return;
> -    }
> -
> -    if (ioctl(vdev->vbasedev.fd,
> -              VFIO_DEVICE_GET_IRQ_INFO, &irq_info) < 0 || irq_info.count < 1) {
> -        return;
> -    }
> -
> -    if (event_notifier_init(&vdev->req_notifier, 0)) {
> -        error_report("vfio: Unable to init event notifier for device request");
> -        return;
> -    }
> -
> -    argsz = sizeof(*irq_set) + sizeof(*pfd);
> -
> -    irq_set = g_malloc0(argsz);
> -    irq_set->argsz = argsz;
> -    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
> -                     VFIO_IRQ_SET_ACTION_TRIGGER;
> -    irq_set->index = VFIO_PCI_REQ_IRQ_INDEX;
> -    irq_set->start = 0;
> -    irq_set->count = 1;
> -    pfd = (int32_t *)&irq_set->data;
> -
> -    *pfd = event_notifier_get_fd(&vdev->req_notifier);
> -    qemu_set_fd_handler(*pfd, vfio_req_notifier_handler, NULL, vdev);
> -
> -    if (ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set)) {
> -        error_report("vfio: Failed to set up device request notification");
> -        qemu_set_fd_handler(*pfd, NULL, NULL, vdev);
> -        event_notifier_cleanup(&vdev->req_notifier);
> -    } else {
> -        vdev->req_enabled = true;
> -    }
> -
> -    g_free(irq_set);
> -}
> -
> -static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev)
> -{
> -    int argsz;
> -    struct vfio_irq_set *irq_set;
> -    int32_t *pfd;
> -
> -    if (!vdev->req_enabled) {
> -        return;
> -    }
> -
> -    argsz = sizeof(*irq_set) + sizeof(*pfd);
> -
> -    irq_set = g_malloc0(argsz);
> -    irq_set->argsz = argsz;
> -    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
> -                     VFIO_IRQ_SET_ACTION_TRIGGER;
> -    irq_set->index = VFIO_PCI_REQ_IRQ_INDEX;
> -    irq_set->start = 0;
> -    irq_set->count = 1;
> -    pfd = (int32_t *)&irq_set->data;
> -    *pfd = -1;
> -
> -    if (ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set)) {
> -        error_report("vfio: Failed to de-assign device request fd: %m");
> -    }
> -    g_free(irq_set);
> -    qemu_set_fd_handler(event_notifier_get_fd(&vdev->req_notifier),
> -                        NULL, NULL, vdev);
> -    event_notifier_cleanup(&vdev->req_notifier);
> -
> -    vdev->req_enabled = false;
> -}
> -
>  static void vfio_realize(PCIDevice *pdev, Error **errp)
>  {
>      VFIOPCIDevice *vdev = PCI_VFIO(pdev);
> @@ -3069,8 +2999,33 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>          goto out_teardown;
>      }
>  
> -    vfio_register_err_notifier(vdev);
> -    vfio_register_req_notifier(vdev);
> +    if (vdev->pci_aer) {
> +        Error *err = NULL;
> +
> +        /*
> +         * Registers error notifier for devices supporting error recovery.
> +         * If we encounter a failure during registration, we report an error
> +         * and continue after disabling error recovery support for the
> +         * device.
> +         */
> +        vfio_set_event_handler(vdev, VFIO_PCI_ERR_IRQ_INDEX, true,
> +                               vfio_err_notifier_handler, &err);
> +        if (err) {
> +            warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
> +        }

Why not just return -1 on error and zero on success so we can call as:

    if (vfio_set_event_handler(...)) {
        warn_reportf_err()...
    }

> +        vdev->pci_aer = !err;

We could also avoid this weirdness of this negation to get a bool.

> +    }

It's not obvious how doing away with the register/unregister helpers
and doing everything inline is an improvement.  Simple helpers calling
common helpers seems better than inline sprawl calling common helpers.
Thanks,

Alex

> +
> +    if (vdev->features & VFIO_FEATURE_ENABLE_REQ) {
> +        Error *err = NULL;
> +
> +        vfio_set_event_handler(vdev, VFIO_PCI_REQ_IRQ_INDEX, true,
> +                               vfio_req_notifier_handler, &err);
> +        if (err) {
> +            warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
> +        }
> +        vdev->req_enabled = !err;
> +    }
>      vfio_setup_resetfn_quirk(vdev);
>  
>      return;
> @@ -3106,9 +3061,22 @@ static void vfio_instance_finalize(Object *obj)
>  static void vfio_exitfn(PCIDevice *pdev)
>  {
>      VFIOPCIDevice *vdev = PCI_VFIO(pdev);
> +    Error *err = NULL;
>  
> -    vfio_unregister_req_notifier(vdev);
> -    vfio_unregister_err_notifier(vdev);
> +    if (vdev->req_enabled) {
> +        vfio_set_event_handler(vdev, VFIO_PCI_REQ_IRQ_INDEX,
> +                               false, NULL, &err);
> +        if (err) {
> +            warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
> +        }
> +    }
> +    if (vdev->pci_aer) {
> +        vfio_set_event_handler(vdev, VFIO_PCI_ERR_IRQ_INDEX,
> +                                     false, NULL, &err);
> +        if (err) {
> +            warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
> +        }
> +    }
>      pci_device_set_intx_routing_notifier(&vdev->pdev, NULL);
>      vfio_disable_interrupts(vdev);
>      if (vdev->intx.mmap_timer) {
Auger Eric Jan. 23, 2019, 3:28 p.m. UTC | #2
Hi Alex,

On 1/22/19 8:51 PM, Alex Williamson wrote:
> On Thu, 17 Jan 2019 22:06:31 +0100
> Eric Auger <eric.auger@redhat.com> wrote:
> 
>> The code used to attach the eventfd handler for the ERR and
>> REQ irq indices can be factorized into a helper. In subsequent
>> patches we will extend this helper to support other irq indices.
>>
>> We test whether the notification is allowed outside of the helper:
>> respectively check vdev->pci_aer and VFIO_FEATURE_ENABLE_REQ.
>> Depending on the returned value we set vdev->pci_aer and
>> vdev->req_enabled. An error handle is introduced for future usage
>> although not strictly useful here.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>
>> ---
>>
>> v1 -> v2:
>> - s/vfio_register_event_notifier/vfio_set_event_handler
>> - turned into a void function
>> - do the qemu_set_fd_handler(*pfd, NULL, NULL, vdev) and
>>   event_notifier_cleanup on VFIO_DEVICE_SET_IRQS failure
>>   as reported by Alexey
>> - reset err in vfio_realize as reported by Cornelia
>> - Text/comment fixes suggested by Cornelia
>> ---
>>  hw/vfio/pci.c | 296 ++++++++++++++++++++++----------------------------
>>  1 file changed, 132 insertions(+), 164 deletions(-)
>>
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index c0cb1ec289..3cae4c99ef 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -105,6 +105,96 @@ static void vfio_intx_eoi(VFIODevice *vbasedev)
>>      vfio_unmask_single_irqindex(vbasedev, VFIO_PCI_INTX_IRQ_INDEX);
>>  }
>>  
>> +/*
>> + * vfio_set_event_handler - setup/tear down eventfd
>> + * notification and handling for IRQ indices that span over
>> + * a single IRQ
>> + *
>> + * @vdev: VFIO device handle
>> + * @index: IRQ index the eventfd/handler is associated with
>> + * @enable: true means notifier chain needs to be set up
>> + * @handler: handler to attach if @enable is true
> 
> Therefore @enable is redundant.
agreed
> 
>> + * @errp: error handle
>> + */
>> +static void vfio_set_event_handler(VFIOPCIDevice *vdev,
>> +                                   int index,
>> +                                   bool enable,
>> +                                   void (*handler)(void *opaque),
>> +                                   Error **errp)
>> +{
>> +    struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info),
>> +                                      .index = index };
>> +    struct vfio_irq_set *irq_set;
>> +    EventNotifier *notifier;
>> +    int argsz, ret = 0;
>> +    int32_t *pfd, fd;
>> +
>> +    switch (index) {
>> +    case VFIO_PCI_REQ_IRQ_INDEX:
>> +        notifier = &vdev->req_notifier;
>> +        break;
>> +    case VFIO_PCI_ERR_IRQ_INDEX:
>> +        notifier = &vdev->err_notifier;
>> +        break;
> 
> This blows the abstraction of a helper that this seems to be trying to
> create, seems cleaner to pass @notifier.


Not sure I really get the point eventually. Don't we have the following
indirection: irq index -> notifier.fd -> handler. When we want to use
this helper don't we simply want to associate an handler to a given IRQ
index without taking care of the notifier mechanics.

As discussed with Alexey, moving the notifier initialization outside of
this helper removes some code factorization.
> 
>> +    default:
>> +        g_assert_not_reached();
>> +    }
>> +
>> +    if (ioctl(vdev->vbasedev.fd,
>> +              VFIO_DEVICE_GET_IRQ_INFO, &irq_info) < 0 || irq_info.count < 1) {
>> +        error_setg_errno(errp, errno, "No irq index %d available", index);
>> +        return;
> 
> The implicit count, aka sub-index, is also problematic for the
> abstraction.  Can we tackle applying this to MSI/X to validate if this
> needs to go another step to allow the caller to specify index and
> sub-index?
I mentioned the helper stands for IRQ indices with no sub-index. I am
afraid applying this to MSI/X would oblige use to revisit a bulk of code
without knowing whether it is interesting.
> 
>> +    }
>> +
>> +    if (enable) {
>> +        ret = event_notifier_init(notifier, 0);
>> +        if (ret) {
>> +            error_setg_errno(errp, -ret,
>> +                             "Unable to init event notifier for irq index %d",
>> +                             index);
>> +            return;
>> +        }
>> +    }
>> +
>> +    argsz = sizeof(*irq_set) + sizeof(*pfd);
>> +
>> +    irq_set = g_malloc0(argsz);
>> +    irq_set->argsz = argsz;
>> +    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
>> +                     VFIO_IRQ_SET_ACTION_TRIGGER;
>> +    irq_set->index = index;
>> +    irq_set->start = 0;
>> +    irq_set->count = 1;
>> +    pfd = (int32_t *)&irq_set->data;
>> +
>> +    if (!notifier) {
>> +        error_setg(errp, "Notifier not initialized for irq index %d", index);
>> +        return;
>> +    }
> 
> What is this supposed to check?  @notifier is not NULL initialized, the
> case statement will assert if it doesn't get set, and this doesn't
> actually test if it's properly initialized.
The goal was to check the helper was not called on a valid IRQ index
with !enabled while the notifier was not properly initialized. But if we
trust the calling sites I can remove it.
> 
>> +
>> +    fd = event_notifier_get_fd(notifier);
>> +
>> +    if (enable) {
>> +        qemu_set_fd_handler(fd, handler, NULL, vdev);
>> +        *pfd = fd;
>> +    } else {
>> +        *pfd = -1;
>> +    }
>> +
>> +    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
>> +    g_free(irq_set);
>> +
>> +    if (ret) {
>> +        error_setg_errno(errp, -ret,
>> +                         "Failed to %s eventfd signalling for index %d",
>> +                         enable ? "set up" : "tear down", index);
>> +    }
>> +    if (ret || !enable) {
>> +        qemu_set_fd_handler(fd, NULL, NULL, vdev);
>> +        event_notifier_cleanup(notifier);
>> +    }
>> +}
> 
> Suggest passing @notifier as a parameter and using @handler in place of
> @enable, more generic and more obvious calling convention.
ok
> 
>> +
>>  static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev, Error **errp)
>>  {
>>  #ifdef CONFIG_KVM
>> @@ -2621,86 +2711,6 @@ static void vfio_err_notifier_handler(void *opaque)
>>      vm_stop(RUN_STATE_INTERNAL_ERROR);
>>  }
>>  
>> -/*
>> - * Registers error notifier for devices supporting error recovery.
>> - * If we encounter a failure in this function, we report an error
>> - * and continue after disabling error recovery support for the
>> - * device.
>> - */
>> -static void vfio_register_err_notifier(VFIOPCIDevice *vdev)
>> -{
>> -    int ret;
>> -    int argsz;
>> -    struct vfio_irq_set *irq_set;
>> -    int32_t *pfd;
>> -
>> -    if (!vdev->pci_aer) {
>> -        return;
>> -    }
>> -
>> -    if (event_notifier_init(&vdev->err_notifier, 0)) {
>> -        error_report("vfio: Unable to init event notifier for error detection");
>> -        vdev->pci_aer = false;
>> -        return;
>> -    }
>> -
>> -    argsz = sizeof(*irq_set) + sizeof(*pfd);
>> -
>> -    irq_set = g_malloc0(argsz);
>> -    irq_set->argsz = argsz;
>> -    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
>> -                     VFIO_IRQ_SET_ACTION_TRIGGER;
>> -    irq_set->index = VFIO_PCI_ERR_IRQ_INDEX;
>> -    irq_set->start = 0;
>> -    irq_set->count = 1;
>> -    pfd = (int32_t *)&irq_set->data;
>> -
>> -    *pfd = event_notifier_get_fd(&vdev->err_notifier);
>> -    qemu_set_fd_handler(*pfd, vfio_err_notifier_handler, NULL, vdev);
>> -
>> -    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
>> -    if (ret) {
>> -        error_report("vfio: Failed to set up error notification");
>> -        qemu_set_fd_handler(*pfd, NULL, NULL, vdev);
>> -        event_notifier_cleanup(&vdev->err_notifier);
>> -        vdev->pci_aer = false;
>> -    }
>> -    g_free(irq_set);
>> -}
>> -
>> -static void vfio_unregister_err_notifier(VFIOPCIDevice *vdev)
>> -{
>> -    int argsz;
>> -    struct vfio_irq_set *irq_set;
>> -    int32_t *pfd;
>> -    int ret;
>> -
>> -    if (!vdev->pci_aer) {
>> -        return;
>> -    }
>> -
>> -    argsz = sizeof(*irq_set) + sizeof(*pfd);
>> -
>> -    irq_set = g_malloc0(argsz);
>> -    irq_set->argsz = argsz;
>> -    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
>> -                     VFIO_IRQ_SET_ACTION_TRIGGER;
>> -    irq_set->index = VFIO_PCI_ERR_IRQ_INDEX;
>> -    irq_set->start = 0;
>> -    irq_set->count = 1;
>> -    pfd = (int32_t *)&irq_set->data;
>> -    *pfd = -1;
>> -
>> -    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
>> -    if (ret) {
>> -        error_report("vfio: Failed to de-assign error fd: %m");
>> -    }
>> -    g_free(irq_set);
>> -    qemu_set_fd_handler(event_notifier_get_fd(&vdev->err_notifier),
>> -                        NULL, NULL, vdev);
>> -    event_notifier_cleanup(&vdev->err_notifier);
>> -}
>> -
>>  static void vfio_req_notifier_handler(void *opaque)
>>  {
>>      VFIOPCIDevice *vdev = opaque;
>> @@ -2716,86 +2726,6 @@ static void vfio_req_notifier_handler(void *opaque)
>>      }
>>  }
>>  
>> -static void vfio_register_req_notifier(VFIOPCIDevice *vdev)
>> -{
>> -    struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info),
>> -                                      .index = VFIO_PCI_REQ_IRQ_INDEX };
>> -    int argsz;
>> -    struct vfio_irq_set *irq_set;
>> -    int32_t *pfd;
>> -
>> -    if (!(vdev->features & VFIO_FEATURE_ENABLE_REQ)) {
>> -        return;
>> -    }
>> -
>> -    if (ioctl(vdev->vbasedev.fd,
>> -              VFIO_DEVICE_GET_IRQ_INFO, &irq_info) < 0 || irq_info.count < 1) {
>> -        return;
>> -    }
>> -
>> -    if (event_notifier_init(&vdev->req_notifier, 0)) {
>> -        error_report("vfio: Unable to init event notifier for device request");
>> -        return;
>> -    }
>> -
>> -    argsz = sizeof(*irq_set) + sizeof(*pfd);
>> -
>> -    irq_set = g_malloc0(argsz);
>> -    irq_set->argsz = argsz;
>> -    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
>> -                     VFIO_IRQ_SET_ACTION_TRIGGER;
>> -    irq_set->index = VFIO_PCI_REQ_IRQ_INDEX;
>> -    irq_set->start = 0;
>> -    irq_set->count = 1;
>> -    pfd = (int32_t *)&irq_set->data;
>> -
>> -    *pfd = event_notifier_get_fd(&vdev->req_notifier);
>> -    qemu_set_fd_handler(*pfd, vfio_req_notifier_handler, NULL, vdev);
>> -
>> -    if (ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set)) {
>> -        error_report("vfio: Failed to set up device request notification");
>> -        qemu_set_fd_handler(*pfd, NULL, NULL, vdev);
>> -        event_notifier_cleanup(&vdev->req_notifier);
>> -    } else {
>> -        vdev->req_enabled = true;
>> -    }
>> -
>> -    g_free(irq_set);
>> -}
>> -
>> -static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev)
>> -{
>> -    int argsz;
>> -    struct vfio_irq_set *irq_set;
>> -    int32_t *pfd;
>> -
>> -    if (!vdev->req_enabled) {
>> -        return;
>> -    }
>> -
>> -    argsz = sizeof(*irq_set) + sizeof(*pfd);
>> -
>> -    irq_set = g_malloc0(argsz);
>> -    irq_set->argsz = argsz;
>> -    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
>> -                     VFIO_IRQ_SET_ACTION_TRIGGER;
>> -    irq_set->index = VFIO_PCI_REQ_IRQ_INDEX;
>> -    irq_set->start = 0;
>> -    irq_set->count = 1;
>> -    pfd = (int32_t *)&irq_set->data;
>> -    *pfd = -1;
>> -
>> -    if (ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set)) {
>> -        error_report("vfio: Failed to de-assign device request fd: %m");
>> -    }
>> -    g_free(irq_set);
>> -    qemu_set_fd_handler(event_notifier_get_fd(&vdev->req_notifier),
>> -                        NULL, NULL, vdev);
>> -    event_notifier_cleanup(&vdev->req_notifier);
>> -
>> -    vdev->req_enabled = false;
>> -}
>> -
>>  static void vfio_realize(PCIDevice *pdev, Error **errp)
>>  {
>>      VFIOPCIDevice *vdev = PCI_VFIO(pdev);
>> @@ -3069,8 +2999,33 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>>          goto out_teardown;
>>      }
>>  
>> -    vfio_register_err_notifier(vdev);
>> -    vfio_register_req_notifier(vdev);
>> +    if (vdev->pci_aer) {
>> +        Error *err = NULL;
>> +
>> +        /*
>> +         * Registers error notifier for devices supporting error recovery.
>> +         * If we encounter a failure during registration, we report an error
>> +         * and continue after disabling error recovery support for the
>> +         * device.
>> +         */
>> +        vfio_set_event_handler(vdev, VFIO_PCI_ERR_IRQ_INDEX, true,
>> +                               vfio_err_notifier_handler, &err);
>> +        if (err) {
>> +            warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
>> +        }
> 
> Why not just return -1 on error and zero on success so we can call as:
> 
>     if (vfio_set_event_handler(...)) {
>         warn_reportf_err()...
>     }
The point is that if you have both the err and the returned value , it
is error prone as you expect both to be consistent (reported by Alexey).
You expect err to be set whenever you have a an error returned. The
calling site can call warn_reportf_err() whenever the function returns
an error and if somebody, later on, ignores to set the err on error
case, it will crash.

Reading again Markus' answer
(https://www.mail-archive.com/qemu-devel@nongnu.org/msg402893.html), I
may put the returned value back and make sure my code is consistent ;-)
> 
>> +        vdev->pci_aer = !err;
> 
> We could also avoid this weirdness of this negation to get a bool.
ok
> 
>> +    }
> 
> It's not obvious how doing away with the register/unregister helpers
> and doing everything inline is an improvement.  Simple helpers calling
> common helpers seems better than inline sprawl calling common helpers.
> Thanks,

On my side I noticed vfio_(un)register_err|req_notifier are mostly
identical at the exception of the irq index/notifier and enable logic.
As I am about to propose another single index IRQ, I am going to add 2
similar functions and I felt it was a pitty. Now it is not a big deal
and if you prefer to keep the code as it is I will simply add those.

Thanks

Eric

> 
> Alex
> 
>> +
>> +    if (vdev->features & VFIO_FEATURE_ENABLE_REQ) {
>> +        Error *err = NULL;
>> +
>> +        vfio_set_event_handler(vdev, VFIO_PCI_REQ_IRQ_INDEX, true,
>> +                               vfio_req_notifier_handler, &err);
>> +        if (err) {
>> +            warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
>> +        }
>> +        vdev->req_enabled = !err;
>> +    }
>>      vfio_setup_resetfn_quirk(vdev);
>>  
>>      return;
>> @@ -3106,9 +3061,22 @@ static void vfio_instance_finalize(Object *obj)
>>  static void vfio_exitfn(PCIDevice *pdev)
>>  {
>>      VFIOPCIDevice *vdev = PCI_VFIO(pdev);
>> +    Error *err = NULL;
>>  
>> -    vfio_unregister_req_notifier(vdev);
>> -    vfio_unregister_err_notifier(vdev);
>> +    if (vdev->req_enabled) {
>> +        vfio_set_event_handler(vdev, VFIO_PCI_REQ_IRQ_INDEX,
>> +                               false, NULL, &err);
>> +        if (err) {
>> +            warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
>> +        }
>> +    }
>> +    if (vdev->pci_aer) {
>> +        vfio_set_event_handler(vdev, VFIO_PCI_ERR_IRQ_INDEX,
>> +                                     false, NULL, &err);
>> +        if (err) {
>> +            warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
>> +        }
>> +    }
>>      pci_device_set_intx_routing_notifier(&vdev->pdev, NULL);
>>      vfio_disable_interrupts(vdev);
>>      if (vdev->intx.mmap_timer) {
>
Alex Williamson Jan. 23, 2019, 4 p.m. UTC | #3
On Wed, 23 Jan 2019 16:28:50 +0100
Auger Eric <eric.auger@redhat.com> wrote:

> Hi Alex,
> 
> On 1/22/19 8:51 PM, Alex Williamson wrote:
> > On Thu, 17 Jan 2019 22:06:31 +0100
> > Eric Auger <eric.auger@redhat.com> wrote:
> >   
> >> The code used to attach the eventfd handler for the ERR and
> >> REQ irq indices can be factorized into a helper. In subsequent
> >> patches we will extend this helper to support other irq indices.
> >>
> >> We test whether the notification is allowed outside of the helper:
> >> respectively check vdev->pci_aer and VFIO_FEATURE_ENABLE_REQ.
> >> Depending on the returned value we set vdev->pci_aer and
> >> vdev->req_enabled. An error handle is introduced for future usage
> >> although not strictly useful here.
> >>
> >> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> >>
> >> ---
> >>
> >> v1 -> v2:
> >> - s/vfio_register_event_notifier/vfio_set_event_handler
> >> - turned into a void function
> >> - do the qemu_set_fd_handler(*pfd, NULL, NULL, vdev) and
> >>   event_notifier_cleanup on VFIO_DEVICE_SET_IRQS failure
> >>   as reported by Alexey
> >> - reset err in vfio_realize as reported by Cornelia
> >> - Text/comment fixes suggested by Cornelia
> >> ---
> >>  hw/vfio/pci.c | 296 ++++++++++++++++++++++----------------------------
> >>  1 file changed, 132 insertions(+), 164 deletions(-)
> >>
> >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> >> index c0cb1ec289..3cae4c99ef 100644
> >> --- a/hw/vfio/pci.c
> >> +++ b/hw/vfio/pci.c
> >> @@ -105,6 +105,96 @@ static void vfio_intx_eoi(VFIODevice *vbasedev)
> >>      vfio_unmask_single_irqindex(vbasedev, VFIO_PCI_INTX_IRQ_INDEX);
> >>  }
> >>  
> >> +/*
> >> + * vfio_set_event_handler - setup/tear down eventfd
> >> + * notification and handling for IRQ indices that span over
> >> + * a single IRQ
> >> + *
> >> + * @vdev: VFIO device handle
> >> + * @index: IRQ index the eventfd/handler is associated with
> >> + * @enable: true means notifier chain needs to be set up
> >> + * @handler: handler to attach if @enable is true  
> > 
> > Therefore @enable is redundant.  
> agreed
> >   
> >> + * @errp: error handle
> >> + */
> >> +static void vfio_set_event_handler(VFIOPCIDevice *vdev,
> >> +                                   int index,
> >> +                                   bool enable,
> >> +                                   void (*handler)(void *opaque),
> >> +                                   Error **errp)
> >> +{
> >> +    struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info),
> >> +                                      .index = index };
> >> +    struct vfio_irq_set *irq_set;
> >> +    EventNotifier *notifier;
> >> +    int argsz, ret = 0;
> >> +    int32_t *pfd, fd;
> >> +
> >> +    switch (index) {
> >> +    case VFIO_PCI_REQ_IRQ_INDEX:
> >> +        notifier = &vdev->req_notifier;
> >> +        break;
> >> +    case VFIO_PCI_ERR_IRQ_INDEX:
> >> +        notifier = &vdev->err_notifier;
> >> +        break;  
> > 
> > This blows the abstraction of a helper that this seems to be trying to
> > create, seems cleaner to pass @notifier.  
> 
> 
> Not sure I really get the point eventually. Don't we have the following
> indirection: irq index -> notifier.fd -> handler. When we want to use
> this helper don't we simply want to associate an handler to a given IRQ
> index without taking care of the notifier mechanics.

With that logic we could eliminate all the parameters and have the
function infer everything.  I don't think that's how we build a good
helper function though.  To me the function is wanting to set or clear
a handler for an irq index, but it also needs the notifier to trigger
to call that handler, so we simply need to pass that as another arg
rather than inferring it from the index.
 
> As discussed with Alexey, moving the notifier initialization outside of
> this helper removes some code factorization.

I don't see why passing the notifier implies this function cannot
perform the init and cleanup of that notifier.

> >> +    default:
> >> +        g_assert_not_reached();
> >> +    }
> >> +
> >> +    if (ioctl(vdev->vbasedev.fd,
> >> +              VFIO_DEVICE_GET_IRQ_INFO, &irq_info) < 0 || irq_info.count < 1) {
> >> +        error_setg_errno(errp, errno, "No irq index %d available", index);
> >> +        return;  
> > 
> > The implicit count, aka sub-index, is also problematic for the
> > abstraction.  Can we tackle applying this to MSI/X to validate if this
> > needs to go another step to allow the caller to specify index and
> > sub-index?  
> I mentioned the helper stands for IRQ indices with no sub-index. I am
> afraid applying this to MSI/X would oblige use to revisit a bulk of code
> without knowing whether it is interesting.

I'm afraid that the helper shows some holes with INTx integration and
I'm wondering if MSI/X integration would show us how to improve the
helper.

> >> +    }
> >> +
> >> +    if (enable) {
> >> +        ret = event_notifier_init(notifier, 0);
> >> +        if (ret) {
> >> +            error_setg_errno(errp, -ret,
> >> +                             "Unable to init event notifier for irq index %d",
> >> +                             index);
> >> +            return;
> >> +        }
> >> +    }
> >> +
> >> +    argsz = sizeof(*irq_set) + sizeof(*pfd);
> >> +
> >> +    irq_set = g_malloc0(argsz);
> >> +    irq_set->argsz = argsz;
> >> +    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
> >> +                     VFIO_IRQ_SET_ACTION_TRIGGER;
> >> +    irq_set->index = index;
> >> +    irq_set->start = 0;
> >> +    irq_set->count = 1;
> >> +    pfd = (int32_t *)&irq_set->data;
> >> +
> >> +    if (!notifier) {
> >> +        error_setg(errp, "Notifier not initialized for irq index %d", index);
> >> +        return;
> >> +    }  
> > 
> > What is this supposed to check?  @notifier is not NULL initialized, the
> > case statement will assert if it doesn't get set, and this doesn't
> > actually test if it's properly initialized.  
> The goal was to check the helper was not called on a valid IRQ index
> with !enabled while the notifier was not properly initialized. But if we
> trust the calling sites I can remove it.

But this doesn't test if the notifier is initialized.  Seems you'd need
to check if fd of the notifier is -1.

> >   
> >> +
> >> +    fd = event_notifier_get_fd(notifier);
> >> +
> >> +    if (enable) {
> >> +        qemu_set_fd_handler(fd, handler, NULL, vdev);
> >> +        *pfd = fd;
> >> +    } else {
> >> +        *pfd = -1;
> >> +    }
> >> +
> >> +    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
> >> +    g_free(irq_set);
> >> +
> >> +    if (ret) {
> >> +        error_setg_errno(errp, -ret,
> >> +                         "Failed to %s eventfd signalling for index %d",
> >> +                         enable ? "set up" : "tear down", index);
> >> +    }
> >> +    if (ret || !enable) {
> >> +        qemu_set_fd_handler(fd, NULL, NULL, vdev);
> >> +        event_notifier_cleanup(notifier);
> >> +    }
> >> +}  
> > 
> > Suggest passing @notifier as a parameter and using @handler in place of
> > @enable, more generic and more obvious calling convention.  
> ok
> >   
> >> +
> >>  static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev, Error **errp)
> >>  {
> >>  #ifdef CONFIG_KVM
> >> @@ -2621,86 +2711,6 @@ static void vfio_err_notifier_handler(void *opaque)
> >>      vm_stop(RUN_STATE_INTERNAL_ERROR);
> >>  }
> >>  
> >> -/*
> >> - * Registers error notifier for devices supporting error recovery.
> >> - * If we encounter a failure in this function, we report an error
> >> - * and continue after disabling error recovery support for the
> >> - * device.
> >> - */
> >> -static void vfio_register_err_notifier(VFIOPCIDevice *vdev)
> >> -{
> >> -    int ret;
> >> -    int argsz;
> >> -    struct vfio_irq_set *irq_set;
> >> -    int32_t *pfd;
> >> -
> >> -    if (!vdev->pci_aer) {
> >> -        return;
> >> -    }
> >> -
> >> -    if (event_notifier_init(&vdev->err_notifier, 0)) {
> >> -        error_report("vfio: Unable to init event notifier for error detection");
> >> -        vdev->pci_aer = false;
> >> -        return;
> >> -    }
> >> -
> >> -    argsz = sizeof(*irq_set) + sizeof(*pfd);
> >> -
> >> -    irq_set = g_malloc0(argsz);
> >> -    irq_set->argsz = argsz;
> >> -    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
> >> -                     VFIO_IRQ_SET_ACTION_TRIGGER;
> >> -    irq_set->index = VFIO_PCI_ERR_IRQ_INDEX;
> >> -    irq_set->start = 0;
> >> -    irq_set->count = 1;
> >> -    pfd = (int32_t *)&irq_set->data;
> >> -
> >> -    *pfd = event_notifier_get_fd(&vdev->err_notifier);
> >> -    qemu_set_fd_handler(*pfd, vfio_err_notifier_handler, NULL, vdev);
> >> -
> >> -    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
> >> -    if (ret) {
> >> -        error_report("vfio: Failed to set up error notification");
> >> -        qemu_set_fd_handler(*pfd, NULL, NULL, vdev);
> >> -        event_notifier_cleanup(&vdev->err_notifier);
> >> -        vdev->pci_aer = false;
> >> -    }
> >> -    g_free(irq_set);
> >> -}
> >> -
> >> -static void vfio_unregister_err_notifier(VFIOPCIDevice *vdev)
> >> -{
> >> -    int argsz;
> >> -    struct vfio_irq_set *irq_set;
> >> -    int32_t *pfd;
> >> -    int ret;
> >> -
> >> -    if (!vdev->pci_aer) {
> >> -        return;
> >> -    }
> >> -
> >> -    argsz = sizeof(*irq_set) + sizeof(*pfd);
> >> -
> >> -    irq_set = g_malloc0(argsz);
> >> -    irq_set->argsz = argsz;
> >> -    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
> >> -                     VFIO_IRQ_SET_ACTION_TRIGGER;
> >> -    irq_set->index = VFIO_PCI_ERR_IRQ_INDEX;
> >> -    irq_set->start = 0;
> >> -    irq_set->count = 1;
> >> -    pfd = (int32_t *)&irq_set->data;
> >> -    *pfd = -1;
> >> -
> >> -    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
> >> -    if (ret) {
> >> -        error_report("vfio: Failed to de-assign error fd: %m");
> >> -    }
> >> -    g_free(irq_set);
> >> -    qemu_set_fd_handler(event_notifier_get_fd(&vdev->err_notifier),
> >> -                        NULL, NULL, vdev);
> >> -    event_notifier_cleanup(&vdev->err_notifier);
> >> -}
> >> -
> >>  static void vfio_req_notifier_handler(void *opaque)
> >>  {
> >>      VFIOPCIDevice *vdev = opaque;
> >> @@ -2716,86 +2726,6 @@ static void vfio_req_notifier_handler(void *opaque)
> >>      }
> >>  }
> >>  
> >> -static void vfio_register_req_notifier(VFIOPCIDevice *vdev)
> >> -{
> >> -    struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info),
> >> -                                      .index = VFIO_PCI_REQ_IRQ_INDEX };
> >> -    int argsz;
> >> -    struct vfio_irq_set *irq_set;
> >> -    int32_t *pfd;
> >> -
> >> -    if (!(vdev->features & VFIO_FEATURE_ENABLE_REQ)) {
> >> -        return;
> >> -    }
> >> -
> >> -    if (ioctl(vdev->vbasedev.fd,
> >> -              VFIO_DEVICE_GET_IRQ_INFO, &irq_info) < 0 || irq_info.count < 1) {
> >> -        return;
> >> -    }
> >> -
> >> -    if (event_notifier_init(&vdev->req_notifier, 0)) {
> >> -        error_report("vfio: Unable to init event notifier for device request");
> >> -        return;
> >> -    }
> >> -
> >> -    argsz = sizeof(*irq_set) + sizeof(*pfd);
> >> -
> >> -    irq_set = g_malloc0(argsz);
> >> -    irq_set->argsz = argsz;
> >> -    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
> >> -                     VFIO_IRQ_SET_ACTION_TRIGGER;
> >> -    irq_set->index = VFIO_PCI_REQ_IRQ_INDEX;
> >> -    irq_set->start = 0;
> >> -    irq_set->count = 1;
> >> -    pfd = (int32_t *)&irq_set->data;
> >> -
> >> -    *pfd = event_notifier_get_fd(&vdev->req_notifier);
> >> -    qemu_set_fd_handler(*pfd, vfio_req_notifier_handler, NULL, vdev);
> >> -
> >> -    if (ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set)) {
> >> -        error_report("vfio: Failed to set up device request notification");
> >> -        qemu_set_fd_handler(*pfd, NULL, NULL, vdev);
> >> -        event_notifier_cleanup(&vdev->req_notifier);
> >> -    } else {
> >> -        vdev->req_enabled = true;
> >> -    }
> >> -
> >> -    g_free(irq_set);
> >> -}
> >> -
> >> -static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev)
> >> -{
> >> -    int argsz;
> >> -    struct vfio_irq_set *irq_set;
> >> -    int32_t *pfd;
> >> -
> >> -    if (!vdev->req_enabled) {
> >> -        return;
> >> -    }
> >> -
> >> -    argsz = sizeof(*irq_set) + sizeof(*pfd);
> >> -
> >> -    irq_set = g_malloc0(argsz);
> >> -    irq_set->argsz = argsz;
> >> -    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
> >> -                     VFIO_IRQ_SET_ACTION_TRIGGER;
> >> -    irq_set->index = VFIO_PCI_REQ_IRQ_INDEX;
> >> -    irq_set->start = 0;
> >> -    irq_set->count = 1;
> >> -    pfd = (int32_t *)&irq_set->data;
> >> -    *pfd = -1;
> >> -
> >> -    if (ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set)) {
> >> -        error_report("vfio: Failed to de-assign device request fd: %m");
> >> -    }
> >> -    g_free(irq_set);
> >> -    qemu_set_fd_handler(event_notifier_get_fd(&vdev->req_notifier),
> >> -                        NULL, NULL, vdev);
> >> -    event_notifier_cleanup(&vdev->req_notifier);
> >> -
> >> -    vdev->req_enabled = false;
> >> -}
> >> -
> >>  static void vfio_realize(PCIDevice *pdev, Error **errp)
> >>  {
> >>      VFIOPCIDevice *vdev = PCI_VFIO(pdev);
> >> @@ -3069,8 +2999,33 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
> >>          goto out_teardown;
> >>      }
> >>  
> >> -    vfio_register_err_notifier(vdev);
> >> -    vfio_register_req_notifier(vdev);
> >> +    if (vdev->pci_aer) {
> >> +        Error *err = NULL;
> >> +
> >> +        /*
> >> +         * Registers error notifier for devices supporting error recovery.
> >> +         * If we encounter a failure during registration, we report an error
> >> +         * and continue after disabling error recovery support for the
> >> +         * device.
> >> +         */
> >> +        vfio_set_event_handler(vdev, VFIO_PCI_ERR_IRQ_INDEX, true,
> >> +                               vfio_err_notifier_handler, &err);
> >> +        if (err) {
> >> +            warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
> >> +        }  
> > 
> > Why not just return -1 on error and zero on success so we can call as:
> > 
> >     if (vfio_set_event_handler(...)) {
> >         warn_reportf_err()...
> >     }  
> The point is that if you have both the err and the returned value , it
> is error prone as you expect both to be consistent (reported by Alexey).
> You expect err to be set whenever you have a an error returned. The
> calling site can call warn_reportf_err() whenever the function returns
> an error and if somebody, later on, ignores to set the err on error
> case, it will crash.
> 
> Reading again Markus' answer
> (https://www.mail-archive.com/qemu-devel@nongnu.org/msg402893.html), I
> may put the returned value back and make sure my code is consistent ;-)

It's not like C programmers aren't used to checking return values.  Not
exactly analogous, but we check the return value of an ioctl() then
look at errno.  So it feels like standard practice to return an error
code on failure and try to only use void returns for functions that
cannot fail.

> >   
> >> +        vdev->pci_aer = !err;  
> > 
> > We could also avoid this weirdness of this negation to get a bool.  
> ok
> >   
> >> +    }  
> > 
> > It's not obvious how doing away with the register/unregister helpers
> > and doing everything inline is an improvement.  Simple helpers calling
> > common helpers seems better than inline sprawl calling common helpers.
> > Thanks,  
> 
> On my side I noticed vfio_(un)register_err|req_notifier are mostly
> identical at the exception of the irq index/notifier and enable logic.
> As I am about to propose another single index IRQ, I am going to add 2
> similar functions and I felt it was a pitty. Now it is not a big deal
> and if you prefer to keep the code as it is I will simply add those.

Simple helper functions are a good thing IMO, especially with the
prospects of open coding two more setup and teardown sections further
bloating the base function.  Thanks,

Alex
Auger Eric Jan. 23, 2019, 5:31 p.m. UTC | #4
Hi Alex,

On 1/23/19 5:00 PM, Alex Williamson wrote:
> On Wed, 23 Jan 2019 16:28:50 +0100
> Auger Eric <eric.auger@redhat.com> wrote:
> 
>> Hi Alex,
>>
>> On 1/22/19 8:51 PM, Alex Williamson wrote:
>>> On Thu, 17 Jan 2019 22:06:31 +0100
>>> Eric Auger <eric.auger@redhat.com> wrote:
>>>   
>>>> The code used to attach the eventfd handler for the ERR and
>>>> REQ irq indices can be factorized into a helper. In subsequent
>>>> patches we will extend this helper to support other irq indices.
>>>>
>>>> We test whether the notification is allowed outside of the helper:
>>>> respectively check vdev->pci_aer and VFIO_FEATURE_ENABLE_REQ.
>>>> Depending on the returned value we set vdev->pci_aer and
>>>> vdev->req_enabled. An error handle is introduced for future usage
>>>> although not strictly useful here.
>>>>
>>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>>>
>>>> ---
>>>>
>>>> v1 -> v2:
>>>> - s/vfio_register_event_notifier/vfio_set_event_handler
>>>> - turned into a void function
>>>> - do the qemu_set_fd_handler(*pfd, NULL, NULL, vdev) and
>>>>   event_notifier_cleanup on VFIO_DEVICE_SET_IRQS failure
>>>>   as reported by Alexey
>>>> - reset err in vfio_realize as reported by Cornelia
>>>> - Text/comment fixes suggested by Cornelia
>>>> ---
>>>>  hw/vfio/pci.c | 296 ++++++++++++++++++++++----------------------------
>>>>  1 file changed, 132 insertions(+), 164 deletions(-)
>>>>
>>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>>>> index c0cb1ec289..3cae4c99ef 100644
>>>> --- a/hw/vfio/pci.c
>>>> +++ b/hw/vfio/pci.c
>>>> @@ -105,6 +105,96 @@ static void vfio_intx_eoi(VFIODevice *vbasedev)
>>>>      vfio_unmask_single_irqindex(vbasedev, VFIO_PCI_INTX_IRQ_INDEX);
>>>>  }
>>>>  
>>>> +/*
>>>> + * vfio_set_event_handler - setup/tear down eventfd
>>>> + * notification and handling for IRQ indices that span over
>>>> + * a single IRQ
>>>> + *
>>>> + * @vdev: VFIO device handle
>>>> + * @index: IRQ index the eventfd/handler is associated with
>>>> + * @enable: true means notifier chain needs to be set up
>>>> + * @handler: handler to attach if @enable is true  
>>>
>>> Therefore @enable is redundant.  
>> agreed
>>>   
>>>> + * @errp: error handle
>>>> + */
>>>> +static void vfio_set_event_handler(VFIOPCIDevice *vdev,
>>>> +                                   int index,
>>>> +                                   bool enable,
>>>> +                                   void (*handler)(void *opaque),
>>>> +                                   Error **errp)
>>>> +{
>>>> +    struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info),
>>>> +                                      .index = index };
>>>> +    struct vfio_irq_set *irq_set;
>>>> +    EventNotifier *notifier;
>>>> +    int argsz, ret = 0;
>>>> +    int32_t *pfd, fd;
>>>> +
>>>> +    switch (index) {
>>>> +    case VFIO_PCI_REQ_IRQ_INDEX:
>>>> +        notifier = &vdev->req_notifier;
>>>> +        break;
>>>> +    case VFIO_PCI_ERR_IRQ_INDEX:
>>>> +        notifier = &vdev->err_notifier;
>>>> +        break;  
>>>
>>> This blows the abstraction of a helper that this seems to be trying to
>>> create, seems cleaner to pass @notifier.  
>>
>>
>> Not sure I really get the point eventually. Don't we have the following
>> indirection: irq index -> notifier.fd -> handler. When we want to use
>> this helper don't we simply want to associate an handler to a given IRQ
>> index without taking care of the notifier mechanics.
> 
> With that logic we could eliminate all the parameters and have the
> function infer everything.  I don't think that's how we build a good
> helper function though.  To me the function is wanting to set or clear
> a handler for an irq index, but it also needs the notifier to trigger
> to call that handler, so we simply need to pass that as another arg
> rather than inferring it from the index.
OK
>  
>> As discussed with Alexey, moving the notifier initialization outside of
>> this helper removes some code factorization.
> 
> I don't see why passing the notifier implies this function cannot
> perform the init and cleanup of that notifier.
Got it now.
> 
>>>> +    default:
>>>> +        g_assert_not_reached();
>>>> +    }
>>>> +
>>>> +    if (ioctl(vdev->vbasedev.fd,
>>>> +              VFIO_DEVICE_GET_IRQ_INFO, &irq_info) < 0 || irq_info.count < 1) {
>>>> +        error_setg_errno(errp, errno, "No irq index %d available", index);
>>>> +        return;  
>>>
>>> The implicit count, aka sub-index, is also problematic for the
>>> abstraction.  Can we tackle applying this to MSI/X to validate if this
>>> needs to go another step to allow the caller to specify index and
>>> sub-index?  
>> I mentioned the helper stands for IRQ indices with no sub-index. I am
>> afraid applying this to MSI/X would oblige use to revisit a bulk of code
>> without knowing whether it is interesting.
> 
> I'm afraid that the helper shows some holes with INTx integration and
> I'm wondering if MSI/X integration would show us how to improve the
> helper.
OK I will do the exercise
> 
>>>> +    }
>>>> +
>>>> +    if (enable) {
>>>> +        ret = event_notifier_init(notifier, 0);
>>>> +        if (ret) {
>>>> +            error_setg_errno(errp, -ret,
>>>> +                             "Unable to init event notifier for irq index %d",
>>>> +                             index);
>>>> +            return;
>>>> +        }
>>>> +    }
>>>> +
>>>> +    argsz = sizeof(*irq_set) + sizeof(*pfd);
>>>> +
>>>> +    irq_set = g_malloc0(argsz);
>>>> +    irq_set->argsz = argsz;
>>>> +    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
>>>> +                     VFIO_IRQ_SET_ACTION_TRIGGER;
>>>> +    irq_set->index = index;
>>>> +    irq_set->start = 0;
>>>> +    irq_set->count = 1;
>>>> +    pfd = (int32_t *)&irq_set->data;
>>>> +
>>>> +    if (!notifier) {
>>>> +        error_setg(errp, "Notifier not initialized for irq index %d", index);
>>>> +        return;
>>>> +    }  
>>>
>>> What is this supposed to check?  @notifier is not NULL initialized, the
>>> case statement will assert if it doesn't get set, and this doesn't
>>> actually test if it's properly initialized.  
>> The goal was to check the helper was not called on a valid IRQ index
>> with !enabled while the notifier was not properly initialized. But if we
>> trust the calling sites I can remove it.
> 
> But this doesn't test if the notifier is initialized.  Seems you'd need
> to check if fd of the notifier is -1.
I understand the point now
> 
>>>   
>>>> +
>>>> +    fd = event_notifier_get_fd(notifier);
>>>> +
>>>> +    if (enable) {
>>>> +        qemu_set_fd_handler(fd, handler, NULL, vdev);
>>>> +        *pfd = fd;
>>>> +    } else {
>>>> +        *pfd = -1;
>>>> +    }
>>>> +
>>>> +    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
>>>> +    g_free(irq_set);
>>>> +
>>>> +    if (ret) {
>>>> +        error_setg_errno(errp, -ret,
>>>> +                         "Failed to %s eventfd signalling for index %d",
>>>> +                         enable ? "set up" : "tear down", index);
>>>> +    }
>>>> +    if (ret || !enable) {
>>>> +        qemu_set_fd_handler(fd, NULL, NULL, vdev);
>>>> +        event_notifier_cleanup(notifier);
>>>> +    }
>>>> +}  
>>>
>>> Suggest passing @notifier as a parameter and using @handler in place of
>>> @enable, more generic and more obvious calling convention.  
>> ok
>>>   
>>>> +
>>>>  static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev, Error **errp)
>>>>  {
>>>>  #ifdef CONFIG_KVM
>>>> @@ -2621,86 +2711,6 @@ static void vfio_err_notifier_handler(void *opaque)
>>>>      vm_stop(RUN_STATE_INTERNAL_ERROR);
>>>>  }
>>>>  
>>>> -/*
>>>> - * Registers error notifier for devices supporting error recovery.
>>>> - * If we encounter a failure in this function, we report an error
>>>> - * and continue after disabling error recovery support for the
>>>> - * device.
>>>> - */
>>>> -static void vfio_register_err_notifier(VFIOPCIDevice *vdev)
>>>> -{
>>>> -    int ret;
>>>> -    int argsz;
>>>> -    struct vfio_irq_set *irq_set;
>>>> -    int32_t *pfd;
>>>> -
>>>> -    if (!vdev->pci_aer) {
>>>> -        return;
>>>> -    }
>>>> -
>>>> -    if (event_notifier_init(&vdev->err_notifier, 0)) {
>>>> -        error_report("vfio: Unable to init event notifier for error detection");
>>>> -        vdev->pci_aer = false;
>>>> -        return;
>>>> -    }
>>>> -
>>>> -    argsz = sizeof(*irq_set) + sizeof(*pfd);
>>>> -
>>>> -    irq_set = g_malloc0(argsz);
>>>> -    irq_set->argsz = argsz;
>>>> -    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
>>>> -                     VFIO_IRQ_SET_ACTION_TRIGGER;
>>>> -    irq_set->index = VFIO_PCI_ERR_IRQ_INDEX;
>>>> -    irq_set->start = 0;
>>>> -    irq_set->count = 1;
>>>> -    pfd = (int32_t *)&irq_set->data;
>>>> -
>>>> -    *pfd = event_notifier_get_fd(&vdev->err_notifier);
>>>> -    qemu_set_fd_handler(*pfd, vfio_err_notifier_handler, NULL, vdev);
>>>> -
>>>> -    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
>>>> -    if (ret) {
>>>> -        error_report("vfio: Failed to set up error notification");
>>>> -        qemu_set_fd_handler(*pfd, NULL, NULL, vdev);
>>>> -        event_notifier_cleanup(&vdev->err_notifier);
>>>> -        vdev->pci_aer = false;
>>>> -    }
>>>> -    g_free(irq_set);
>>>> -}
>>>> -
>>>> -static void vfio_unregister_err_notifier(VFIOPCIDevice *vdev)
>>>> -{
>>>> -    int argsz;
>>>> -    struct vfio_irq_set *irq_set;
>>>> -    int32_t *pfd;
>>>> -    int ret;
>>>> -
>>>> -    if (!vdev->pci_aer) {
>>>> -        return;
>>>> -    }
>>>> -
>>>> -    argsz = sizeof(*irq_set) + sizeof(*pfd);
>>>> -
>>>> -    irq_set = g_malloc0(argsz);
>>>> -    irq_set->argsz = argsz;
>>>> -    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
>>>> -                     VFIO_IRQ_SET_ACTION_TRIGGER;
>>>> -    irq_set->index = VFIO_PCI_ERR_IRQ_INDEX;
>>>> -    irq_set->start = 0;
>>>> -    irq_set->count = 1;
>>>> -    pfd = (int32_t *)&irq_set->data;
>>>> -    *pfd = -1;
>>>> -
>>>> -    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
>>>> -    if (ret) {
>>>> -        error_report("vfio: Failed to de-assign error fd: %m");
>>>> -    }
>>>> -    g_free(irq_set);
>>>> -    qemu_set_fd_handler(event_notifier_get_fd(&vdev->err_notifier),
>>>> -                        NULL, NULL, vdev);
>>>> -    event_notifier_cleanup(&vdev->err_notifier);
>>>> -}
>>>> -
>>>>  static void vfio_req_notifier_handler(void *opaque)
>>>>  {
>>>>      VFIOPCIDevice *vdev = opaque;
>>>> @@ -2716,86 +2726,6 @@ static void vfio_req_notifier_handler(void *opaque)
>>>>      }
>>>>  }
>>>>  
>>>> -static void vfio_register_req_notifier(VFIOPCIDevice *vdev)
>>>> -{
>>>> -    struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info),
>>>> -                                      .index = VFIO_PCI_REQ_IRQ_INDEX };
>>>> -    int argsz;
>>>> -    struct vfio_irq_set *irq_set;
>>>> -    int32_t *pfd;
>>>> -
>>>> -    if (!(vdev->features & VFIO_FEATURE_ENABLE_REQ)) {
>>>> -        return;
>>>> -    }
>>>> -
>>>> -    if (ioctl(vdev->vbasedev.fd,
>>>> -              VFIO_DEVICE_GET_IRQ_INFO, &irq_info) < 0 || irq_info.count < 1) {
>>>> -        return;
>>>> -    }
>>>> -
>>>> -    if (event_notifier_init(&vdev->req_notifier, 0)) {
>>>> -        error_report("vfio: Unable to init event notifier for device request");
>>>> -        return;
>>>> -    }
>>>> -
>>>> -    argsz = sizeof(*irq_set) + sizeof(*pfd);
>>>> -
>>>> -    irq_set = g_malloc0(argsz);
>>>> -    irq_set->argsz = argsz;
>>>> -    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
>>>> -                     VFIO_IRQ_SET_ACTION_TRIGGER;
>>>> -    irq_set->index = VFIO_PCI_REQ_IRQ_INDEX;
>>>> -    irq_set->start = 0;
>>>> -    irq_set->count = 1;
>>>> -    pfd = (int32_t *)&irq_set->data;
>>>> -
>>>> -    *pfd = event_notifier_get_fd(&vdev->req_notifier);
>>>> -    qemu_set_fd_handler(*pfd, vfio_req_notifier_handler, NULL, vdev);
>>>> -
>>>> -    if (ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set)) {
>>>> -        error_report("vfio: Failed to set up device request notification");
>>>> -        qemu_set_fd_handler(*pfd, NULL, NULL, vdev);
>>>> -        event_notifier_cleanup(&vdev->req_notifier);
>>>> -    } else {
>>>> -        vdev->req_enabled = true;
>>>> -    }
>>>> -
>>>> -    g_free(irq_set);
>>>> -}
>>>> -
>>>> -static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev)
>>>> -{
>>>> -    int argsz;
>>>> -    struct vfio_irq_set *irq_set;
>>>> -    int32_t *pfd;
>>>> -
>>>> -    if (!vdev->req_enabled) {
>>>> -        return;
>>>> -    }
>>>> -
>>>> -    argsz = sizeof(*irq_set) + sizeof(*pfd);
>>>> -
>>>> -    irq_set = g_malloc0(argsz);
>>>> -    irq_set->argsz = argsz;
>>>> -    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
>>>> -                     VFIO_IRQ_SET_ACTION_TRIGGER;
>>>> -    irq_set->index = VFIO_PCI_REQ_IRQ_INDEX;
>>>> -    irq_set->start = 0;
>>>> -    irq_set->count = 1;
>>>> -    pfd = (int32_t *)&irq_set->data;
>>>> -    *pfd = -1;
>>>> -
>>>> -    if (ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set)) {
>>>> -        error_report("vfio: Failed to de-assign device request fd: %m");
>>>> -    }
>>>> -    g_free(irq_set);
>>>> -    qemu_set_fd_handler(event_notifier_get_fd(&vdev->req_notifier),
>>>> -                        NULL, NULL, vdev);
>>>> -    event_notifier_cleanup(&vdev->req_notifier);
>>>> -
>>>> -    vdev->req_enabled = false;
>>>> -}
>>>> -
>>>>  static void vfio_realize(PCIDevice *pdev, Error **errp)
>>>>  {
>>>>      VFIOPCIDevice *vdev = PCI_VFIO(pdev);
>>>> @@ -3069,8 +2999,33 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>>>>          goto out_teardown;
>>>>      }
>>>>  
>>>> -    vfio_register_err_notifier(vdev);
>>>> -    vfio_register_req_notifier(vdev);
>>>> +    if (vdev->pci_aer) {
>>>> +        Error *err = NULL;
>>>> +
>>>> +        /*
>>>> +         * Registers error notifier for devices supporting error recovery.
>>>> +         * If we encounter a failure during registration, we report an error
>>>> +         * and continue after disabling error recovery support for the
>>>> +         * device.
>>>> +         */
>>>> +        vfio_set_event_handler(vdev, VFIO_PCI_ERR_IRQ_INDEX, true,
>>>> +                               vfio_err_notifier_handler, &err);
>>>> +        if (err) {
>>>> +            warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
>>>> +        }  
>>>
>>> Why not just return -1 on error and zero on success so we can call as:
>>>
>>>     if (vfio_set_event_handler(...)) {
>>>         warn_reportf_err()...
>>>     }  
>> The point is that if you have both the err and the returned value , it
>> is error prone as you expect both to be consistent (reported by Alexey).
>> You expect err to be set whenever you have a an error returned. The
>> calling site can call warn_reportf_err() whenever the function returns
>> an error and if somebody, later on, ignores to set the err on error
>> case, it will crash.
>>
>> Reading again Markus' answer
>> (https://www.mail-archive.com/qemu-devel@nongnu.org/msg402893.html), I
>> may put the returned value back and make sure my code is consistent ;-)
> 
> It's not like C programmers aren't used to checking return values.  Not
> exactly analogous, but we check the return value of an ioctl() then
> look at errno.  So it feels like standard practice to return an error
> code on failure and try to only use void returns for functions that
> cannot fail.
ok
> 
>>>   
>>>> +        vdev->pci_aer = !err;  
>>>
>>> We could also avoid this weirdness of this negation to get a bool.  
>> ok
>>>   
>>>> +    }  
>>>
>>> It's not obvious how doing away with the register/unregister helpers
>>> and doing everything inline is an improvement.  Simple helpers calling
>>> common helpers seems better than inline sprawl calling common helpers.
>>> Thanks,  
>>
>> On my side I noticed vfio_(un)register_err|req_notifier are mostly
>> identical at the exception of the irq index/notifier and enable logic.
>> As I am about to propose another single index IRQ, I am going to add 2
>> similar functions and I felt it was a pitty. Now it is not a big deal
>> and if you prefer to keep the code as it is I will simply add those.
> 
> Simple helper functions are a good thing IMO, especially with the
> prospects of open coding two more setup and teardown sections further
> bloating the base function.  Thanks,
Thanks

Eric
> 
> Alex
>

Patch
diff mbox series

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index c0cb1ec289..3cae4c99ef 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -105,6 +105,96 @@  static void vfio_intx_eoi(VFIODevice *vbasedev)
     vfio_unmask_single_irqindex(vbasedev, VFIO_PCI_INTX_IRQ_INDEX);
 }
 
+/*
+ * vfio_set_event_handler - setup/tear down eventfd
+ * notification and handling for IRQ indices that span over
+ * a single IRQ
+ *
+ * @vdev: VFIO device handle
+ * @index: IRQ index the eventfd/handler is associated with
+ * @enable: true means notifier chain needs to be set up
+ * @handler: handler to attach if @enable is true
+ * @errp: error handle
+ */
+static void vfio_set_event_handler(VFIOPCIDevice *vdev,
+                                   int index,
+                                   bool enable,
+                                   void (*handler)(void *opaque),
+                                   Error **errp)
+{
+    struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info),
+                                      .index = index };
+    struct vfio_irq_set *irq_set;
+    EventNotifier *notifier;
+    int argsz, ret = 0;
+    int32_t *pfd, fd;
+
+    switch (index) {
+    case VFIO_PCI_REQ_IRQ_INDEX:
+        notifier = &vdev->req_notifier;
+        break;
+    case VFIO_PCI_ERR_IRQ_INDEX:
+        notifier = &vdev->err_notifier;
+        break;
+    default:
+        g_assert_not_reached();
+    }
+
+    if (ioctl(vdev->vbasedev.fd,
+              VFIO_DEVICE_GET_IRQ_INFO, &irq_info) < 0 || irq_info.count < 1) {
+        error_setg_errno(errp, errno, "No irq index %d available", index);
+        return;
+    }
+
+    if (enable) {
+        ret = event_notifier_init(notifier, 0);
+        if (ret) {
+            error_setg_errno(errp, -ret,
+                             "Unable to init event notifier for irq index %d",
+                             index);
+            return;
+        }
+    }
+
+    argsz = sizeof(*irq_set) + sizeof(*pfd);
+
+    irq_set = g_malloc0(argsz);
+    irq_set->argsz = argsz;
+    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
+                     VFIO_IRQ_SET_ACTION_TRIGGER;
+    irq_set->index = index;
+    irq_set->start = 0;
+    irq_set->count = 1;
+    pfd = (int32_t *)&irq_set->data;
+
+    if (!notifier) {
+        error_setg(errp, "Notifier not initialized for irq index %d", index);
+        return;
+    }
+
+    fd = event_notifier_get_fd(notifier);
+
+    if (enable) {
+        qemu_set_fd_handler(fd, handler, NULL, vdev);
+        *pfd = fd;
+    } else {
+        *pfd = -1;
+    }
+
+    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
+    g_free(irq_set);
+
+    if (ret) {
+        error_setg_errno(errp, -ret,
+                         "Failed to %s eventfd signalling for index %d",
+                         enable ? "set up" : "tear down", index);
+    }
+    if (ret || !enable) {
+        qemu_set_fd_handler(fd, NULL, NULL, vdev);
+        event_notifier_cleanup(notifier);
+    }
+}
+
 static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev, Error **errp)
 {
 #ifdef CONFIG_KVM
@@ -2621,86 +2711,6 @@  static void vfio_err_notifier_handler(void *opaque)
     vm_stop(RUN_STATE_INTERNAL_ERROR);
 }
 
-/*
- * Registers error notifier for devices supporting error recovery.
- * If we encounter a failure in this function, we report an error
- * and continue after disabling error recovery support for the
- * device.
- */
-static void vfio_register_err_notifier(VFIOPCIDevice *vdev)
-{
-    int ret;
-    int argsz;
-    struct vfio_irq_set *irq_set;
-    int32_t *pfd;
-
-    if (!vdev->pci_aer) {
-        return;
-    }
-
-    if (event_notifier_init(&vdev->err_notifier, 0)) {
-        error_report("vfio: Unable to init event notifier for error detection");
-        vdev->pci_aer = false;
-        return;
-    }
-
-    argsz = sizeof(*irq_set) + sizeof(*pfd);
-
-    irq_set = g_malloc0(argsz);
-    irq_set->argsz = argsz;
-    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
-                     VFIO_IRQ_SET_ACTION_TRIGGER;
-    irq_set->index = VFIO_PCI_ERR_IRQ_INDEX;
-    irq_set->start = 0;
-    irq_set->count = 1;
-    pfd = (int32_t *)&irq_set->data;
-
-    *pfd = event_notifier_get_fd(&vdev->err_notifier);
-    qemu_set_fd_handler(*pfd, vfio_err_notifier_handler, NULL, vdev);
-
-    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
-    if (ret) {
-        error_report("vfio: Failed to set up error notification");
-        qemu_set_fd_handler(*pfd, NULL, NULL, vdev);
-        event_notifier_cleanup(&vdev->err_notifier);
-        vdev->pci_aer = false;
-    }
-    g_free(irq_set);
-}
-
-static void vfio_unregister_err_notifier(VFIOPCIDevice *vdev)
-{
-    int argsz;
-    struct vfio_irq_set *irq_set;
-    int32_t *pfd;
-    int ret;
-
-    if (!vdev->pci_aer) {
-        return;
-    }
-
-    argsz = sizeof(*irq_set) + sizeof(*pfd);
-
-    irq_set = g_malloc0(argsz);
-    irq_set->argsz = argsz;
-    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
-                     VFIO_IRQ_SET_ACTION_TRIGGER;
-    irq_set->index = VFIO_PCI_ERR_IRQ_INDEX;
-    irq_set->start = 0;
-    irq_set->count = 1;
-    pfd = (int32_t *)&irq_set->data;
-    *pfd = -1;
-
-    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
-    if (ret) {
-        error_report("vfio: Failed to de-assign error fd: %m");
-    }
-    g_free(irq_set);
-    qemu_set_fd_handler(event_notifier_get_fd(&vdev->err_notifier),
-                        NULL, NULL, vdev);
-    event_notifier_cleanup(&vdev->err_notifier);
-}
-
 static void vfio_req_notifier_handler(void *opaque)
 {
     VFIOPCIDevice *vdev = opaque;
@@ -2716,86 +2726,6 @@  static void vfio_req_notifier_handler(void *opaque)
     }
 }
 
-static void vfio_register_req_notifier(VFIOPCIDevice *vdev)
-{
-    struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info),
-                                      .index = VFIO_PCI_REQ_IRQ_INDEX };
-    int argsz;
-    struct vfio_irq_set *irq_set;
-    int32_t *pfd;
-
-    if (!(vdev->features & VFIO_FEATURE_ENABLE_REQ)) {
-        return;
-    }
-
-    if (ioctl(vdev->vbasedev.fd,
-              VFIO_DEVICE_GET_IRQ_INFO, &irq_info) < 0 || irq_info.count < 1) {
-        return;
-    }
-
-    if (event_notifier_init(&vdev->req_notifier, 0)) {
-        error_report("vfio: Unable to init event notifier for device request");
-        return;
-    }
-
-    argsz = sizeof(*irq_set) + sizeof(*pfd);
-
-    irq_set = g_malloc0(argsz);
-    irq_set->argsz = argsz;
-    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
-                     VFIO_IRQ_SET_ACTION_TRIGGER;
-    irq_set->index = VFIO_PCI_REQ_IRQ_INDEX;
-    irq_set->start = 0;
-    irq_set->count = 1;
-    pfd = (int32_t *)&irq_set->data;
-
-    *pfd = event_notifier_get_fd(&vdev->req_notifier);
-    qemu_set_fd_handler(*pfd, vfio_req_notifier_handler, NULL, vdev);
-
-    if (ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set)) {
-        error_report("vfio: Failed to set up device request notification");
-        qemu_set_fd_handler(*pfd, NULL, NULL, vdev);
-        event_notifier_cleanup(&vdev->req_notifier);
-    } else {
-        vdev->req_enabled = true;
-    }
-
-    g_free(irq_set);
-}
-
-static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev)
-{
-    int argsz;
-    struct vfio_irq_set *irq_set;
-    int32_t *pfd;
-
-    if (!vdev->req_enabled) {
-        return;
-    }
-
-    argsz = sizeof(*irq_set) + sizeof(*pfd);
-
-    irq_set = g_malloc0(argsz);
-    irq_set->argsz = argsz;
-    irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
-                     VFIO_IRQ_SET_ACTION_TRIGGER;
-    irq_set->index = VFIO_PCI_REQ_IRQ_INDEX;
-    irq_set->start = 0;
-    irq_set->count = 1;
-    pfd = (int32_t *)&irq_set->data;
-    *pfd = -1;
-
-    if (ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set)) {
-        error_report("vfio: Failed to de-assign device request fd: %m");
-    }
-    g_free(irq_set);
-    qemu_set_fd_handler(event_notifier_get_fd(&vdev->req_notifier),
-                        NULL, NULL, vdev);
-    event_notifier_cleanup(&vdev->req_notifier);
-
-    vdev->req_enabled = false;
-}
-
 static void vfio_realize(PCIDevice *pdev, Error **errp)
 {
     VFIOPCIDevice *vdev = PCI_VFIO(pdev);
@@ -3069,8 +2999,33 @@  static void vfio_realize(PCIDevice *pdev, Error **errp)
         goto out_teardown;
     }
 
-    vfio_register_err_notifier(vdev);
-    vfio_register_req_notifier(vdev);
+    if (vdev->pci_aer) {
+        Error *err = NULL;
+
+        /*
+         * Registers error notifier for devices supporting error recovery.
+         * If we encounter a failure during registration, we report an error
+         * and continue after disabling error recovery support for the
+         * device.
+         */
+        vfio_set_event_handler(vdev, VFIO_PCI_ERR_IRQ_INDEX, true,
+                               vfio_err_notifier_handler, &err);
+        if (err) {
+            warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
+        }
+        vdev->pci_aer = !err;
+    }
+
+    if (vdev->features & VFIO_FEATURE_ENABLE_REQ) {
+        Error *err = NULL;
+
+        vfio_set_event_handler(vdev, VFIO_PCI_REQ_IRQ_INDEX, true,
+                               vfio_req_notifier_handler, &err);
+        if (err) {
+            warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
+        }
+        vdev->req_enabled = !err;
+    }
     vfio_setup_resetfn_quirk(vdev);
 
     return;
@@ -3106,9 +3061,22 @@  static void vfio_instance_finalize(Object *obj)
 static void vfio_exitfn(PCIDevice *pdev)
 {
     VFIOPCIDevice *vdev = PCI_VFIO(pdev);
+    Error *err = NULL;
 
-    vfio_unregister_req_notifier(vdev);
-    vfio_unregister_err_notifier(vdev);
+    if (vdev->req_enabled) {
+        vfio_set_event_handler(vdev, VFIO_PCI_REQ_IRQ_INDEX,
+                               false, NULL, &err);
+        if (err) {
+            warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
+        }
+    }
+    if (vdev->pci_aer) {
+        vfio_set_event_handler(vdev, VFIO_PCI_ERR_IRQ_INDEX,
+                                     false, NULL, &err);
+        if (err) {
+            warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
+        }
+    }
     pci_device_set_intx_routing_notifier(&vdev->pdev, NULL);
     vfio_disable_interrupts(vdev);
     if (vdev->intx.mmap_timer) {