Re: [Qemu-devel] [PATCH v2 4/4] vfio/quirks: Enable ioeventfd quirks to be handled by vfio directly

[Peter Xu <peterx@redhat.com>]

On Tue, May 01, 2018 at 10:43:46AM -0600, Alex Williamson wrote:

[...]

> -static void vfio_ioeventfd_exit(VFIOIOEventFD *ioeventfd)
> +static void vfio_ioeventfd_exit(VFIOPCIDevice *vdev, VFIOIOEventFD *ioeventfd)
>  {
>      QLIST_REMOVE(ioeventfd, next);
> +
>      memory_region_del_eventfd(ioeventfd->mr, ioeventfd->addr, ioeventfd->size,
>                                ioeventfd->match_data, ioeventfd->data,
>                                &ioeventfd->e);
> -    qemu_set_fd_handler(event_notifier_get_fd(&ioeventfd->e), NULL, NULL, NULL);
> +
> +    if (ioeventfd->vfio) {
> +        struct vfio_device_ioeventfd vfio_ioeventfd;
> +
> +        vfio_ioeventfd.argsz = sizeof(vfio_ioeventfd);
> +        vfio_ioeventfd.flags = ioeventfd->size;
> +        vfio_ioeventfd.data = ioeventfd->data;
> +        vfio_ioeventfd.offset = ioeventfd->region->fd_offset +
> +                                ioeventfd->region_addr;
> +        vfio_ioeventfd.fd = -1;
> +
> +        ioctl(vdev->vbasedev.fd, VFIO_DEVICE_IOEVENTFD, &vfio_ioeventfd);

(If the series is going to respin, I am thinking whether it would
 worth it to capture this error to dump something.  But it's for sure
 optional since even error happened we should have something in dmesg
 so it only matters on whether we also want something to be dumped
 from QEMU side too... After all there aren't much we can do)

> +
> +    } else {
> +        qemu_set_fd_handler(event_notifier_get_fd(&ioeventfd->e),
> +                            NULL, NULL, NULL);
> +    }
> +
>      event_notifier_cleanup(&ioeventfd->e);
>      trace_vfio_ioeventfd_exit(memory_region_name(ioeventfd->mr),
>                                (uint64_t)ioeventfd->addr, ioeventfd->size,

[...]

> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index ba1239551115..84e27c7bb2d1 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3177,6 +3177,8 @@ static Property vfio_pci_dev_properties[] = {
>                       no_geforce_quirks, false),
>      DEFINE_PROP_BOOL("x-no-kvm-ioeventfd", VFIOPCIDevice, no_kvm_ioeventfd,
>                       false),
> +    DEFINE_PROP_BOOL("x-no-vfio-ioeventfd", VFIOPCIDevice, no_vfio_ioeventfd,
> +                     false),

Here it looks more like a tri-state for me, so we can either:

- disable the acceleration in general, or
- enable QEMU-side acceleration only, or
- enable kernel-side acceleration

In other words, IIUC x-no-vfio-ioeventfd won't matter much if
x-no-kvm-ioeventfd is already set.  So not sure whether a single
parameter would be nicer.

Anyway, I'm even fine with two-parameter way, so I'll leave the
decision to Alex and other reviewers:

Reviewed-by: Peter Xu <peterx@redhat.com>

Thanks,

-- 
Peter Xu