| Message ID | 20190111165801.15181-2-eric.auger@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | vfio-pci: Introduce vfio_register_event_notifier() | expand |
On Fri, 11 Jan 2019 17:58:00 +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. Looks like a nice refactoring to me. > > We test the notification is allowed outside of the helper: s/We test/We test whether/ > 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> > --- > hw/vfio/pci.c | 291 ++++++++++++++++++++++---------------------------- > 1 file changed, 127 insertions(+), 164 deletions(-) > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > index c0cb1ec289..c589a4e666 100644 > --- a/hw/vfio/pci.c > +++ b/hw/vfio/pci.c > @@ -105,6 +105,95 @@ static void vfio_intx_eoi(VFIODevice *vbasedev) > vfio_unmask_single_irqindex(vbasedev, VFIO_PCI_INTX_IRQ_INDEX); > } > > +/* > + * vfio_register_event_notifier - 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 to s/to/with/ ? > + * @target_state: true means notifier needs to be set up > + * @handler to attach if @target_state is true > + * @errp error handle > + */ > +static int vfio_register_event_notifier(VFIOPCIDevice *vdev, > + int index, > + bool target_state, > + void (*handler)(void *opaque), > + Error **errp) (...) > @@ -3069,8 +2998,29 @@ 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) { > + /* > + * Registers error notifier for devices supporting error recovery. > + * If we encounter a failure in this function, we report an error s/in this function/while registering it/ ? > + * and continue after disabling error recovery support for the > + * device. > + */ > + vdev->pci_aer = > + !vfio_register_event_notifier(vdev, VFIO_PCI_ERR_IRQ_INDEX, true, > + vfio_err_notifier_handler, &err); > + if (err) { > + warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name); > + } I think you need to reset err to NULL if you want to reuse the variable. Alternatively, you could keep the wrappers and define a local error variable there. > + } > + > + if (vdev->features & VFIO_FEATURE_ENABLE_REQ) { > + vdev->req_enabled = > + !vfio_register_event_notifier(vdev, VFIO_PCI_REQ_IRQ_INDEX, true, > + vfio_req_notifier_handler, &err); > + if (err) { > + warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name); > + } > + } > vfio_setup_resetfn_quirk(vdev); > > return; > @@ -3106,9 +3056,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_register_event_notifier(vdev, VFIO_PCI_REQ_IRQ_INDEX, > + false, NULL, &err); > + if (err) { > + warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name); > + } Likewise. > + } > + if (vdev->pci_aer) { > + vfio_register_event_notifier(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) {
Hi Cornelia, On 1/15/19 1:03 PM, Cornelia Huck wrote: > On Fri, 11 Jan 2019 17:58:00 +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. > > Looks like a nice refactoring to me. thank you > >> >> We test the notification is allowed outside of the helper: > > s/We test/We test whether/ > >> 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> >> --- >> hw/vfio/pci.c | 291 ++++++++++++++++++++++---------------------------- >> 1 file changed, 127 insertions(+), 164 deletions(-) >> >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >> index c0cb1ec289..c589a4e666 100644 >> --- a/hw/vfio/pci.c >> +++ b/hw/vfio/pci.c >> @@ -105,6 +105,95 @@ static void vfio_intx_eoi(VFIODevice *vbasedev) >> vfio_unmask_single_irqindex(vbasedev, VFIO_PCI_INTX_IRQ_INDEX); >> } >> >> +/* >> + * vfio_register_event_notifier - 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 to > > s/to/with/ ? > >> + * @target_state: true means notifier needs to be set up >> + * @handler to attach if @target_state is true >> + * @errp error handle >> + */ >> +static int vfio_register_event_notifier(VFIOPCIDevice *vdev, >> + int index, >> + bool target_state, >> + void (*handler)(void *opaque), >> + Error **errp) > > (...) > >> @@ -3069,8 +2998,29 @@ 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) { >> + /* >> + * Registers error notifier for devices supporting error recovery. >> + * If we encounter a failure in this function, we report an error > > s/in this function/while registering it/ ? > >> + * and continue after disabling error recovery support for the >> + * device. >> + */ >> + vdev->pci_aer = >> + !vfio_register_event_notifier(vdev, VFIO_PCI_ERR_IRQ_INDEX, true, >> + vfio_err_notifier_handler, &err); >> + if (err) { >> + warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name); >> + } > > I think you need to reset err to NULL if you want to reuse the variable. Yes you're right. Thanks for spotting this. > > Alternatively, you could keep the wrappers and define a local error > variable there. > >> + } >> + >> + if (vdev->features & VFIO_FEATURE_ENABLE_REQ) { >> + vdev->req_enabled = >> + !vfio_register_event_notifier(vdev, VFIO_PCI_REQ_IRQ_INDEX, true, >> + vfio_req_notifier_handler, &err); >> + if (err) { >> + warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name); >> + } >> + } >> vfio_setup_resetfn_quirk(vdev); >> >> return; >> @@ -3106,9 +3056,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_register_event_notifier(vdev, VFIO_PCI_REQ_IRQ_INDEX, >> + false, NULL, &err); >> + if (err) { >> + warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name); >> + } > > Likewise. Thank you for the review! Eric > >> + } >> + if (vdev->pci_aer) { >> + vfio_register_event_notifier(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) { > >
On 12/01/2019 03:58, Eric Auger 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 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> > --- > hw/vfio/pci.c | 291 ++++++++++++++++++++++---------------------------- > 1 file changed, 127 insertions(+), 164 deletions(-) > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > index c0cb1ec289..c589a4e666 100644 > --- a/hw/vfio/pci.c > +++ b/hw/vfio/pci.c > @@ -105,6 +105,95 @@ static void vfio_intx_eoi(VFIODevice *vbasedev) > vfio_unmask_single_irqindex(vbasedev, VFIO_PCI_INTX_IRQ_INDEX); > } > > +/* > + * vfio_register_event_notifier - 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 to > + * @target_state: true means notifier needs to be set up > + * @handler to attach if @target_state is true > + * @errp error handle > + */ > +static int vfio_register_event_notifier(VFIOPCIDevice *vdev, > + int index, > + bool target_state, > + 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) { I'd pass the notifier as a parameter as well so index/handler/notifier would walk together. > + case VFIO_PCI_REQ_IRQ_INDEX: > + notifier = &vdev->req_notifier; > + break; > + case VFIO_PCI_ERR_IRQ_INDEX: > + notifier = &vdev->err_notifier; > + break; > + default: > + return -EINVAL; > + } > + > + 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 -EINVAL; > + } > + > + if (target_state) { > + ret = event_notifier_init(notifier, 0); > + if (ret) { > + error_setg_errno(errp, -ret, > + "Unable to init event notifier for irq index %d", > + index); > + return ret; > + } > + } > + > + 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) { > + return -EINVAL; > + } > + > + fd = event_notifier_get_fd(notifier); > + > + if (target_state) { > + qemu_set_fd_handler(fd, handler, NULL, vdev); > + *pfd = fd; > + } else { > + qemu_set_fd_handler(fd, NULL, NULL, vdev); > + event_notifier_cleanup(notifier); > + *pfd = -1; > + } > + > + ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set); > + g_free(irq_set); > + > + if (ret) { > + error_setg_errno(errp, -ret, > + "vfio: Failed to %s eventfd signalling for index %d", > + *pfd < 0 ? "set up" : "tear down", index); > + } > + return ret; > +} > + > static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev, Error **errp) > { > #ifdef CONFIG_KVM > @@ -2621,86 +2710,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); qemu_set_fd_handler() and event_notifier_cleanup() have not made it to the new vfio_register_event_notifier() for the case when ioctl(VFIO_DEVICE_SET_IRQS) failed. > - 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 +2725,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); Same here. > - 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 +2998,29 @@ 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) { > + /* > + * 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. > + */ > + vdev->pci_aer = > + !vfio_register_event_notifier(vdev, VFIO_PCI_ERR_IRQ_INDEX, true, > + vfio_err_notifier_handler, &err); > + if (err) { > + warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name); > + } As the commit log said, these returned @err are not particularly useful as they are handled exactly the same way and they are not forwarded further. Also, the new helper returns int which is not tested anywhere and it can also return with an error and err==NULL so these errors will be silently skipped. Some consistency here would not hurt :) > + } > + > + if (vdev->features & VFIO_FEATURE_ENABLE_REQ) { > + vdev->req_enabled = > + !vfio_register_event_notifier(vdev, VFIO_PCI_REQ_IRQ_INDEX, true, > + vfio_req_notifier_handler, &err); > + if (err) { > + warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name); > + } > + } > vfio_setup_resetfn_quirk(vdev); > > return; > @@ -3106,9 +3056,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_register_event_notifier(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_register_event_notifier(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) { >
On Thu, 17 Jan 2019 14:46:42 +1100 Alexey Kardashevskiy <aik@ozlabs.ru> wrote: > On 12/01/2019 03:58, Eric Auger 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 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> > > --- > > hw/vfio/pci.c | 291 ++++++++++++++++++++++---------------------------- > > 1 file changed, 127 insertions(+), 164 deletions(-) > > > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > > index c0cb1ec289..c589a4e666 100644 > > --- a/hw/vfio/pci.c > > +++ b/hw/vfio/pci.c > > @@ -105,6 +105,95 @@ static void vfio_intx_eoi(VFIODevice *vbasedev) > > vfio_unmask_single_irqindex(vbasedev, VFIO_PCI_INTX_IRQ_INDEX); > > } > > > > +/* > > + * vfio_register_event_notifier - 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 to > > + * @target_state: true means notifier needs to be set up > > + * @handler to attach if @target_state is true > > + * @errp error handle > > + */ > > +static int vfio_register_event_notifier(VFIOPCIDevice *vdev, > > + int index, > > + bool target_state, > > + 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) { > > I'd pass the notifier as a parameter as well so index/handler/notifier > would walk together. I first had the same thought, but I actually like that everything is together in this switch statement. Alternatively, pack it together in an indexed array? But that gets a bit awkward with the intx stuff. > > > > + case VFIO_PCI_REQ_IRQ_INDEX: > > + notifier = &vdev->req_notifier; > > + break; > > + case VFIO_PCI_ERR_IRQ_INDEX: > > + notifier = &vdev->err_notifier; > > + break; > > + default: > > + return -EINVAL; > > + }
Hi Alexey, Cornelia, On 1/17/19 4:46 AM, Alexey Kardashevskiy wrote: > > > On 12/01/2019 03:58, Eric Auger 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 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> >> --- >> hw/vfio/pci.c | 291 ++++++++++++++++++++++---------------------------- >> 1 file changed, 127 insertions(+), 164 deletions(-) >> >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >> index c0cb1ec289..c589a4e666 100644 >> --- a/hw/vfio/pci.c >> +++ b/hw/vfio/pci.c >> @@ -105,6 +105,95 @@ static void vfio_intx_eoi(VFIODevice *vbasedev) >> vfio_unmask_single_irqindex(vbasedev, VFIO_PCI_INTX_IRQ_INDEX); >> } >> >> +/* >> + * vfio_register_event_notifier - 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 to >> + * @target_state: true means notifier needs to be set up >> + * @handler to attach if @target_state is true >> + * @errp error handle >> + */ >> +static int vfio_register_event_notifier(VFIOPCIDevice *vdev, >> + int index, >> + bool target_state, >> + 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) { > > I'd pass the notifier as a parameter as well so index/handler/notifier > would walk together. I tend to agree with Cornelia. moving the notifier out of this helper would remove some factorization and this way, the caller does not have to care about it. > > >> + case VFIO_PCI_REQ_IRQ_INDEX: >> + notifier = &vdev->req_notifier; >> + break; >> + case VFIO_PCI_ERR_IRQ_INDEX: >> + notifier = &vdev->err_notifier; >> + break; >> + default: >> + return -EINVAL; >> + } >> + >> + 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 -EINVAL; >> + } >> + >> + if (target_state) { >> + ret = event_notifier_init(notifier, 0); >> + if (ret) { >> + error_setg_errno(errp, -ret, >> + "Unable to init event notifier for irq index %d", >> + index); >> + return ret; >> + } >> + } >> + >> + 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) { >> + return -EINVAL; >> + } >> + >> + fd = event_notifier_get_fd(notifier); >> + >> + if (target_state) { >> + qemu_set_fd_handler(fd, handler, NULL, vdev); >> + *pfd = fd; >> + } else { >> + qemu_set_fd_handler(fd, NULL, NULL, vdev); >> + event_notifier_cleanup(notifier); >> + *pfd = -1; >> + } >> + >> + ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set); >> + g_free(irq_set); >> + >> + if (ret) { >> + error_setg_errno(errp, -ret, >> + "vfio: Failed to %s eventfd signalling for index %d", >> + *pfd < 0 ? "set up" : "tear down", index); >> + } >> + return ret; >> +} >> + >> static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev, Error **errp) >> { >> #ifdef CONFIG_KVM >> @@ -2621,86 +2710,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); > > > qemu_set_fd_handler() and event_notifier_cleanup() have not made it to > the new vfio_register_event_notifier() for the case when > ioctl(VFIO_DEVICE_SET_IRQS) failed. definitively missing. Thanks! > > >> - 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 +2725,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); > > Same here. > > >> - 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 +2998,29 @@ 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) { >> + /* >> + * 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. >> + */ >> + vdev->pci_aer = >> + !vfio_register_event_notifier(vdev, VFIO_PCI_ERR_IRQ_INDEX, true, >> + vfio_err_notifier_handler, &err); >> + if (err) { >> + warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name); >> + } > > > As the commit log said, these returned @err are not particularly useful > as they are handled exactly the same way and they are not forwarded further. Yes that's awkward but the issue is vfio_intx_enable() propagates the error. > > Also, the new helper returns int which is not tested anywhere and it can > also return with an error and err==NULL so these errors will be silently > skipped. Some consistency here would not hurt :) Yes I think I will make it a void function and the caller will only test err. Thank you for the review! Eric > > >> + } >> + >> + if (vdev->features & VFIO_FEATURE_ENABLE_REQ) { >> + vdev->req_enabled = >> + !vfio_register_event_notifier(vdev, VFIO_PCI_REQ_IRQ_INDEX, true, >> + vfio_req_notifier_handler, &err); >> + if (err) { >> + warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name); >> + } >> + } >> vfio_setup_resetfn_quirk(vdev); >> >> return; >> @@ -3106,9 +3056,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_register_event_notifier(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_register_event_notifier(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) { >> >
On 17/01/2019 20:16, Auger Eric wrote: > Hi Alexey, Cornelia, > > On 1/17/19 4:46 AM, Alexey Kardashevskiy wrote: >> >> >> On 12/01/2019 03:58, Eric Auger 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 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> >>> --- >>> hw/vfio/pci.c | 291 ++++++++++++++++++++++---------------------------- >>> 1 file changed, 127 insertions(+), 164 deletions(-) >>> >>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >>> index c0cb1ec289..c589a4e666 100644 >>> --- a/hw/vfio/pci.c >>> +++ b/hw/vfio/pci.c >>> @@ -105,6 +105,95 @@ static void vfio_intx_eoi(VFIODevice *vbasedev) >>> vfio_unmask_single_irqindex(vbasedev, VFIO_PCI_INTX_IRQ_INDEX); >>> } >>> >>> +/* >>> + * vfio_register_event_notifier - 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 to >>> + * @target_state: true means notifier needs to be set up >>> + * @handler to attach if @target_state is true >>> + * @errp error handle >>> + */ >>> +static int vfio_register_event_notifier(VFIOPCIDevice *vdev, >>> + int index, >>> + bool target_state, >>> + 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) { >> >> I'd pass the notifier as a parameter as well so index/handler/notifier >> would walk together. > > I tend to agree with Cornelia. moving the notifier out of this helper > would remove some factorization and this way, the caller does not have > to care about it. Then why pass the handler? It also could go into this switch, vfio_register_event_notifier()/vfio_set_event_handler() is never called with more than one handler per index (or NULL but then target_state==false).
Hi Alexey, On 1/18/19 5:14 AM, Alexey Kardashevskiy wrote: > > > On 17/01/2019 20:16, Auger Eric wrote: >> Hi Alexey, Cornelia, >> >> On 1/17/19 4:46 AM, Alexey Kardashevskiy wrote: >>> >>> >>> On 12/01/2019 03:58, Eric Auger 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 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> >>>> --- >>>> hw/vfio/pci.c | 291 ++++++++++++++++++++++---------------------------- >>>> 1 file changed, 127 insertions(+), 164 deletions(-) >>>> >>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c >>>> index c0cb1ec289..c589a4e666 100644 >>>> --- a/hw/vfio/pci.c >>>> +++ b/hw/vfio/pci.c >>>> @@ -105,6 +105,95 @@ static void vfio_intx_eoi(VFIODevice *vbasedev) >>>> vfio_unmask_single_irqindex(vbasedev, VFIO_PCI_INTX_IRQ_INDEX); >>>> } >>>> >>>> +/* >>>> + * vfio_register_event_notifier - 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 to >>>> + * @target_state: true means notifier needs to be set up >>>> + * @handler to attach if @target_state is true >>>> + * @errp error handle >>>> + */ >>>> +static int vfio_register_event_notifier(VFIOPCIDevice *vdev, >>>> + int index, >>>> + bool target_state, >>>> + 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) { >>> >>> I'd pass the notifier as a parameter as well so index/handler/notifier >>> would walk together. >> >> I tend to agree with Cornelia. moving the notifier out of this helper >> would remove some factorization and this way, the caller does not have >> to care about it. > > > Then why pass the handler? It also could go into this switch, > vfio_register_event_notifier()/vfio_set_event_handler() is never called > with more than one handler per index (or NULL but then target_state==false). I don't have any strong opinion here. I will align with the majority's opinion. Thanks Eric > >
On Fri, 18 Jan 2019 10:08:12 +0100 Auger Eric <eric.auger@redhat.com> wrote: > Hi Alexey, > > On 1/18/19 5:14 AM, Alexey Kardashevskiy wrote: > > > > > > On 17/01/2019 20:16, Auger Eric wrote: > >> Hi Alexey, Cornelia, > >> > >> On 1/17/19 4:46 AM, Alexey Kardashevskiy wrote: > >>> > >>> > >>> On 12/01/2019 03:58, Eric Auger 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 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> > >>>> --- > >>>> hw/vfio/pci.c | 291 ++++++++++++++++++++++---------------------------- > >>>> 1 file changed, 127 insertions(+), 164 deletions(-) > >>>> > >>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > >>>> index c0cb1ec289..c589a4e666 100644 > >>>> --- a/hw/vfio/pci.c > >>>> +++ b/hw/vfio/pci.c > >>>> @@ -105,6 +105,95 @@ static void vfio_intx_eoi(VFIODevice *vbasedev) > >>>> vfio_unmask_single_irqindex(vbasedev, VFIO_PCI_INTX_IRQ_INDEX); > >>>> } > >>>> > >>>> +/* > >>>> + * vfio_register_event_notifier - 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 to > >>>> + * @target_state: true means notifier needs to be set up > >>>> + * @handler to attach if @target_state is true > >>>> + * @errp error handle > >>>> + */ > >>>> +static int vfio_register_event_notifier(VFIOPCIDevice *vdev, > >>>> + int index, > >>>> + bool target_state, > >>>> + 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) { > >>> > >>> I'd pass the notifier as a parameter as well so index/handler/notifier > >>> would walk together. > >> > >> I tend to agree with Cornelia. moving the notifier out of this helper > >> would remove some factorization and this way, the caller does not have > >> to care about it. > > > > > > Then why pass the handler? It also could go into this switch, > > vfio_register_event_notifier()/vfio_set_event_handler() is never called > > with more than one handler per index (or NULL but then target_state==false). > I don't have any strong opinion here. I will align with the majority's > opinion. If we are sure that the same index/notifier will always use the same handler when setting (i.e., not a different handler if some feature is available), we could also move getting it to the switch statement. OTOH, passing in a handler is a common pattern. Don't really have a strong opinion here.
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index c0cb1ec289..c589a4e666 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -105,6 +105,95 @@ static void vfio_intx_eoi(VFIODevice *vbasedev) vfio_unmask_single_irqindex(vbasedev, VFIO_PCI_INTX_IRQ_INDEX); } +/* + * vfio_register_event_notifier - 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 to + * @target_state: true means notifier needs to be set up + * @handler to attach if @target_state is true + * @errp error handle + */ +static int vfio_register_event_notifier(VFIOPCIDevice *vdev, + int index, + bool target_state, + 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: + return -EINVAL; + } + + 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 -EINVAL; + } + + if (target_state) { + ret = event_notifier_init(notifier, 0); + if (ret) { + error_setg_errno(errp, -ret, + "Unable to init event notifier for irq index %d", + index); + return ret; + } + } + + 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) { + return -EINVAL; + } + + fd = event_notifier_get_fd(notifier); + + if (target_state) { + qemu_set_fd_handler(fd, handler, NULL, vdev); + *pfd = fd; + } else { + qemu_set_fd_handler(fd, NULL, NULL, vdev); + event_notifier_cleanup(notifier); + *pfd = -1; + } + + ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set); + g_free(irq_set); + + if (ret) { + error_setg_errno(errp, -ret, + "vfio: Failed to %s eventfd signalling for index %d", + *pfd < 0 ? "set up" : "tear down", index); + } + return ret; +} + static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev, Error **errp) { #ifdef CONFIG_KVM @@ -2621,86 +2710,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 +2725,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 +2998,29 @@ 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) { + /* + * 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. + */ + vdev->pci_aer = + !vfio_register_event_notifier(vdev, VFIO_PCI_ERR_IRQ_INDEX, true, + vfio_err_notifier_handler, &err); + if (err) { + warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name); + } + } + + if (vdev->features & VFIO_FEATURE_ENABLE_REQ) { + vdev->req_enabled = + !vfio_register_event_notifier(vdev, VFIO_PCI_REQ_IRQ_INDEX, true, + vfio_req_notifier_handler, &err); + if (err) { + warn_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name); + } + } vfio_setup_resetfn_quirk(vdev); return; @@ -3106,9 +3056,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_register_event_notifier(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_register_event_notifier(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) {
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 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> --- hw/vfio/pci.c | 291 ++++++++++++++++++++++---------------------------- 1 file changed, 127 insertions(+), 164 deletions(-)