Re: [PATCH RFC v1 3/3] vfio/pci: dynamic MSI-X allocation in interrupt restoring

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

On Thu, 27 Jul 2023 03:24:10 -0400
Jing Liu <jing2.liu@intel.com> wrote:

> During migration restoring, vfio_enable_vectors() is called to restore
> enabling MSI-X interrupts for assigned devices. It sets the range from 0
> to nr_vectors to kernel to enable MSI-X and the vectors unmasked in
> guest. During the MSI-X enabling, all the vectors within the range are
> allocated according to the ioctl().
> 
> When dynamic MSI-X allocation is supported, we only want the guest
> unmasked vectors being allocated and enabled. Therefore, Qemu can first
> set vector 0 to enable MSI-X and after that, all the vectors can be
> allocated in need.
> 
> Signed-off-by: Jing Liu <jing2.liu@intel.com>
> ---
>  hw/vfio/pci.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 8c485636445c..43ffacd5b36a 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -375,6 +375,38 @@ static int vfio_enable_vectors(VFIOPCIDevice *vdev, bool msix)
>      int ret = 0, i, argsz;
>      int32_t *fds;
>  
> +    /*
> +     * If dynamic MSI-X allocation is supported, the vectors to be allocated
> +     * and enabled can be scattered. Before kernel enabling MSI-X, setting
> +     * nr_vectors causes all these vectors being allocated on host.

s/being/to be/

> +     *
> +     * To keep allocation as needed, first setup vector 0 with an invalid
> +     * fd to make MSI-X enabled, then enable vectors by setting all so that
> +     * kernel allocates and enables interrupts only when enabled in guest.
> +     */
> +    if (msix && !(vdev->msix->irq_info_flags & VFIO_IRQ_INFO_NORESIZE)) {

!vdev->msix->noresize again seems cleaner.

> +        argsz = sizeof(*irq_set) + sizeof(*fds);
> +
> +        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 = msix ? VFIO_PCI_MSIX_IRQ_INDEX :
> +                         VFIO_PCI_MSI_IRQ_INDEX;

Why are we testing msix again within a branch that requires msix?

> +        irq_set->start = 0;
> +        irq_set->count = 1;
> +        fds = (int32_t *)&irq_set->data;
> +        fds[0] = -1;
> +
> +        ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
> +
> +        g_free(irq_set);
> +
> +        if (ret) {
> +            return ret;
> +        }
> +    }

So your goal here is simply to get the kernel to call vfio_msi_enable()
with nvec = 1 to get MSI-X enabled on the device, which then allows the
kernel to use the dynamic expansion when we call SET_IRQS again with a
potentially sparse set of eventfds to vector mappings.  This seems very
similar to the nr_vectors == 0 branch of vfio_msix_enable() where it
uses a do_use and release call to accomplish getting MSI-X enabled.  We
should consolidate, probably by pulling this out into a function since
it seems cleaner to use the fd = -1 trick than to setup userspace
triggering and immediately release.  Thanks,

Alex

> +
>      argsz = sizeof(*irq_set) + (vdev->nr_vectors * sizeof(*fds));
>  
>      irq_set = g_malloc0(argsz);