Re: [RFC PATCH v2 6/7] util/vfio-helpers: Allow to set EventNotifier to particular IRQ

[Alex Williamson <alex.williamson@redhat.com>]

On Thu, 13 Aug 2020 19:29:56 +0200
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> Let qemu_vfio_pci_init_irq() take an 'index' argument, so we can
> set the EventNotifier to a specific IRQ.
> Add a safety check. Since our helper is limited to one single IRQ
> we are safe.
> 
> Our only user is the NVMe block driver, update it (also safe because
> it only uses the first IRQ).
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  include/qemu/vfio-helpers.h |  2 +-
>  block/nvme.c                |  2 +-
>  util/vfio-helpers.c         | 11 +++++++++--
>  3 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/include/qemu/vfio-helpers.h b/include/qemu/vfio-helpers.h
> index 728f40922b..5c2d8ee5b3 100644
> --- a/include/qemu/vfio-helpers.h
> +++ b/include/qemu/vfio-helpers.h
> @@ -28,6 +28,6 @@ void *qemu_vfio_pci_map_bar(QEMUVFIOState *s, int index,
>  void qemu_vfio_pci_unmap_bar(QEMUVFIOState *s, int index, void *bar,
>                               uint64_t offset, uint64_t size);
>  int qemu_vfio_pci_init_irq(QEMUVFIOState *s, EventNotifier *e,
> -                           Error **errp);
> +                           int irq_index, Error **errp);
>  
>  #endif
> diff --git a/block/nvme.c b/block/nvme.c
> index 21b0770c02..a5ef571492 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -785,7 +785,7 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
>          }
>      }
>  
> -    ret = qemu_vfio_pci_init_irq(s->vfio, &s->irq_notifier, errp);
> +    ret = qemu_vfio_pci_init_irq(s->vfio, &s->irq_notifier, INDEX_ADMIN, errp);
>      if (ret) {
>          goto out;
>      }
> diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
> index 5781e4f066..7a934d1a1b 100644
> --- a/util/vfio-helpers.c
> +++ b/util/vfio-helpers.c
> @@ -180,13 +180,20 @@ void qemu_vfio_pci_unmap_bar(QEMUVFIOState *s, int index, void *bar,
>   * Initialize device IRQ with @irq_type and and register an event notifier.
>   */
>  int qemu_vfio_pci_init_irq(QEMUVFIOState *s, EventNotifier *e,
> -                           Error **errp)
> +                           int irq_index, Error **errp)
>  {
>      int r;
>      struct vfio_irq_set *irq_set;
>      size_t irq_set_size;
>      struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info) };
>  
> +    if (irq_index >= s->irq_count) {
> +        error_setg(errp,
> +                   "Illegal interrupt %d (device initialized for %zu in total)",
> +                   irq_index, s->irq_count);
> +        return -EINVAL;
> +    }
> +
>      irq_info.index = s->irq_type;
>      if (ioctl(s->device, VFIO_DEVICE_GET_IRQ_INFO, &irq_info)) {
>          error_setg_errno(errp, errno, "Failed to get device interrupt info");
> @@ -196,7 +203,7 @@ int qemu_vfio_pci_init_irq(QEMUVFIOState *s, EventNotifier *e,
>          error_setg(errp, "Device interrupt doesn't support eventfd");
>          return -EINVAL;
>      }
> -    s->eventfd[0] = event_notifier_get_fd(e);
> +    s->eventfd[irq_index] = event_notifier_get_fd(e);

This can't work.  For each fd in the array provided the kernel is going
to try to get that fd and configure it as an eventfd.  For each call
until we set all eventfd index {0..irq_count}, this SET_IRQS ioctl will
fail.  I would probably make that pre-configure function I referred to
earlier and create a single spurious interrupt eventfd and configure
all of the vectors to signal that one eventfd.  You could then have
this per vector callback swap the eventfd with the caller provided one
for the given vector.

NB, I don't know if you're going to run into trouble with this scheme
with the fact that devices can behave differently based on the number
of vectors they have enabled.  You're creating an interface for a
driver, so presumably that driver knows, for example, that as soon as
vector N is enabled, signaling for event foo moves from vector 0 to
vector N.  Thanks,

Alex

>  
>      irq_set_size = sizeof(*irq_set) + s->irq_count * sizeof(int32_t);
>      irq_set = g_malloc0(irq_set_size);