Re: [PATCH v3 16/16] block/nvme: Use per-queuepair IRQ notifier and AIO context

From: Stefan Hajnoczi <stefanha@redhat.com>
To: "Philippe Mathieu-Daudé" <philmd@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>, Fam Zheng <fam@euphon.net>,
	qemu-block@nongnu.org, qemu-devel@nongnu.org,
	Maxim Levitsky <mlevitsk@redhat.com>,
	Max Reitz <mreitz@redhat.com>
Subject: Re: [PATCH v3 16/16] block/nvme: Use per-queuepair IRQ notifier and AIO context
Date: Mon, 6 Jul 2020 13:04:53 +0100	[thread overview]
Message-ID: <20200706120453.GE342708@stefanha-x1.localdomain> (raw)
In-Reply-To: <20200704213051.19749-17-philmd@redhat.com>

[-- Attachment #1: Type: text/plain, Size: 1609 bytes --]

On Sat, Jul 04, 2020 at 11:30:51PM +0200, Philippe Mathieu-Daudé wrote:
> @@ -683,6 +676,17 @@ static bool nvme_add_io_queue(BlockDriverState *bs, Error **errp)
>      s->queues = g_renew(NVMeQueuePair *, s->queues, n + 1);
>      s->queues[n] = q;
>      s->nr_queues++;
> +
> +    ret = qemu_vfio_pci_init_irq(s->vfio,
> +                                 &s->queues[n]->irq_notifier,
> +                                 VFIO_PCI_MSIX_IRQ_INDEX, errp);
> +    if (ret) {
> +        goto out_error;
> +    }
> +    aio_set_event_notifier(aio_context,
> +                           &s->queues[n]->irq_notifier,
> +                           false, nvme_handle_event, nvme_poll_cb);

s->queues[n] can be replaced with q to make the code easier to read.

> @@ -784,12 +782,14 @@ static int nvme_init(BlockDriverState *bs, const char *device, int namespace,
>          }
>      }
>  
> -    ret = qemu_vfio_pci_init_irq(s->vfio, &s->irq_notifier,
> +    ret = qemu_vfio_pci_init_irq(s->vfio,
> +                                 &s->queues[QUEUE_INDEX_ADMIN]->irq_notifier,
>                                   VFIO_PCI_MSIX_IRQ_INDEX, errp);

QEMU is setting up only 1 MSI-X vector that is shared by the admin and
all io queues?

I'm not very familiar with the VFIO ioctls but I guess this call
replaces the admin queue's irq_notifier registration with VFIO. So now
the queue's irq_notifier is signalled on admin cq events. The admin
irq_notifier is no longer signalled. This seems broken.

If there are multiple irq_notifiers then multiple MSI-X vectors are
needed.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]