Hi Alex,
On 1/23/19 5:00 PM, Alex Williamson wrote:
> On Wed, 23 Jan 2019 16:28:50 +0100
> Auger Eric <eric.au...@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.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.
>
> 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
>
