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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 7/6/20 2:04 PM, Stefan Hajnoczi wrote:
> 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.

Indeed.

> 
>> @@ -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.

I'll look into that. Cc'ing VFIO experts meanwhile...

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