Hi Alex,
On 1/22/19 8:51 PM, Alex Williamson wrote:
> On Thu, 17 Jan 2019 22:06:31 +0100
> Eric Auger <eric.au...@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.au...@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) {
>
