[PATCH RFC v1 0/3] Support dynamic MSI-X allocation

[PATCH RFC v1 0/3] Support dynamic MSI-X allocation

[Jing Liu]

Before kernel v6.5, dynamic allocation of MSI-X interrupts was not
supported. Qemu therefore when allocating a new interrupt, should first
release all previously allocated interrupts (including disable of MSI-X)
and re-allocate all interrupts that includes the new one.

The kernel series [1] adds the support of dynamic MSI-X allocation to
vfio-pci and uses the existing flag VFIO_IRQ_INFO_NORESIZE to guide user
space, that when dynamic MSI-X is supported the flag is cleared.

This series makes the behavior for VFIO PCI devices when dynamic MSI-X
allocation is supported. When guest unmasks an interrupt, Qemu can
directly allocate an interrupt on host for this and has nothing to do
with the previously allocated ones. Therefore, host only allocates
interrupts for those unmasked (enabled) interrupts inside guest when
dynamic MSI-X allocation is supported by device.

During migration restore, Qemu calls vfio_enable_vectors() to enable
MSI-X and interrupts. Since the API causes that a number of irqs set to
host kernel are all allocated when enabling MSI-X, to avoid this, one
possible way is that Qemu first sets vector 0 to host kernel to enable
MSI-X with an invalid fd. After MSI-X enabling, the API can decide which
should be allocated via the event fd value. In this way, the interrupts
allocation on target would be the same as migration source.

Jing Liu (2):
  vfio/pci: enable vector on dynamic MSI-X allocation
  vfio/pci: dynamic MSI-X allocation in interrupt restoring

Reinette Chatre (1):
  vfio/pci: detect the support of dynamic MSI-X allocation

 hw/vfio/pci.c        | 84 +++++++++++++++++++++++++++++++++++++-------
 hw/vfio/pci.h        |  1 +
 hw/vfio/trace-events |  2 ++
 3 files changed, 74 insertions(+), 13 deletions(-)

-- 
2.27.0

[PATCH RFC v1 1/3] vfio/pci: detect the support of dynamic MSI-X allocation

[Jing Liu]

From: Reinette Chatre <reinette.chatre@intel.com>

Kernel provides the guidance of dynamic MSI-X allocation support of
passthrough device, by clearing the VFIO_IRQ_INFO_NORESIZE flag to
guide user space.

Fetch and store the flags from host for later use to determine if
specific flags are set.

Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
Signed-off-by: Jing Liu <jing2.liu@intel.com>
---
 hw/vfio/pci.c        | 12 ++++++++++++
 hw/vfio/pci.h        |  1 +
 hw/vfio/trace-events |  2 ++
 3 files changed, 15 insertions(+)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index a205c6b1130f..0c4ac0873d40 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1572,6 +1572,7 @@ static void vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp)
 
 static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
 {
+    struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info) };
     int ret;
     Error *err = NULL;
 
@@ -1624,6 +1625,17 @@ static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
         memory_region_set_enabled(&vdev->pdev.msix_table_mmio, false);
     }
 
+    irq_info.index = VFIO_PCI_MSIX_IRQ_INDEX;
+    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_GET_IRQ_INFO, &irq_info);
+    if (ret) {
+        /* This can fail for an old kernel or legacy PCI dev */
+        trace_vfio_msix_setup_get_irq_info_failure(strerror(errno));
+    } else {
+        vdev->msix->irq_info_flags = irq_info.flags;
+    }
+    trace_vfio_msix_setup_irq_info_flags(vdev->vbasedev.name,
+                                         vdev->msix->irq_info_flags);
+
     return 0;
 }
 
diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
index a2771b9ff3cc..ad34ec56d0ae 100644
--- a/hw/vfio/pci.h
+++ b/hw/vfio/pci.h
@@ -113,6 +113,7 @@ typedef struct VFIOMSIXInfo {
     uint32_t table_offset;
     uint32_t pba_offset;
     unsigned long *pending;
+    uint32_t irq_info_flags;
 } VFIOMSIXInfo;
 
 #define TYPE_VFIO_PCI "vfio-pci"
diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
index ee7509e68e4f..7d4a398f044d 100644
--- a/hw/vfio/trace-events
+++ b/hw/vfio/trace-events
@@ -28,6 +28,8 @@ vfio_pci_read_config(const char *name, int addr, int len, int val) " (%s, @0x%x,
 vfio_pci_write_config(const char *name, int addr, int val, int len) " (%s, @0x%x, 0x%x, len=0x%x)"
 vfio_msi_setup(const char *name, int pos) "%s PCI MSI CAP @0x%x"
 vfio_msix_early_setup(const char *name, int pos, int table_bar, int offset, int entries) "%s PCI MSI-X CAP @0x%x, BAR %d, offset 0x%x, entries %d"
+vfio_msix_setup_get_irq_info_failure(const char *errstr) "VFIO_DEVICE_GET_IRQ_INFO failure: %s"
+vfio_msix_setup_irq_info_flags(const char *name, uint32_t flags) " (%s) MSI-X irq info flags 0x%x"
 vfio_check_pcie_flr(const char *name) "%s Supports FLR via PCIe cap"
 vfio_check_pm_reset(const char *name) "%s Supports PM reset"
 vfio_check_af_flr(const char *name) "%s Supports FLR via AF cap"
-- 
2.27.0

[PATCH RFC v1 2/3] vfio/pci: enable vector on dynamic MSI-X allocation

[Jing Liu]

The vector_use callback is used to enable vector that is unmasked in
guest. The kernel used to only support static MSI-X allocation. When
allocating a new interrupt using "static MSI-X allocation" kernels,
Qemu first disables all previously allocated vectors and then
re-allocates all including the new one. The nr_vectors of VFIOPCIDevice
indicates that all vectors from 0 to nr_vectors are allocated (and may
be enabled), which is used to to loop all the possibly used vectors
When, e.g., disabling MSI-X interrupts.

Extend the vector_use function to support dynamic MSI-X allocation when
host supports the capability. Qemu therefore can individually allocate
and enable a new interrupt without affecting others or causing interrupts
lost during runtime.

Utilize nr_vectors to calculate the upper bound of enabled vectors in
dynamic MSI-X allocation mode since looping all msix_entries_nr is not
efficient and unnecessary.

Signed-off-by: Jing Liu <jing2.liu@intel.com>
Tested-by: Reinette Chatre <reinette.chatre@intel.com>
---
 hw/vfio/pci.c | 40 +++++++++++++++++++++++++++-------------
 1 file changed, 27 insertions(+), 13 deletions(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 0c4ac0873d40..8c485636445c 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -512,12 +512,20 @@ static int vfio_msix_vector_do_use(PCIDevice *pdev, unsigned int nr,
     }
 
     /*
-     * We don't want to have the host allocate all possible MSI vectors
-     * for a device if they're not in use, so we shutdown and incrementally
-     * increase them as needed.
+     * When dynamic allocation is not supported, we don't want to have the
+     * host allocate all possible MSI vectors for a device if they're not
+     * in use, so we shutdown and incrementally increase them as needed.
+     * And nr_vectors stands for the number of vectors being allocated.
+     *
+     * When dynamic allocation is supported, let the host only allocate
+     * and enable a vector when it is in use in guest. nr_vectors stands
+     * for the upper bound of vectors being enabled (but not all of the
+     * ranges is allocated or enabled).
      */
-    if (vdev->nr_vectors < nr + 1) {
+    if ((vdev->msix->irq_info_flags & VFIO_IRQ_INFO_NORESIZE) &&
+        (vdev->nr_vectors < nr + 1)) {
         vdev->nr_vectors = nr + 1;
+
         if (!vdev->defer_kvm_irq_routing) {
             vfio_disable_irqindex(&vdev->vbasedev, VFIO_PCI_MSIX_IRQ_INDEX);
             ret = vfio_enable_vectors(vdev, true);
@@ -529,16 +537,22 @@ static int vfio_msix_vector_do_use(PCIDevice *pdev, unsigned int nr,
         Error *err = NULL;
         int32_t fd;
 
-        if (vector->virq >= 0) {
-            fd = event_notifier_get_fd(&vector->kvm_interrupt);
-        } else {
-            fd = event_notifier_get_fd(&vector->interrupt);
-        }
+        if (!vdev->defer_kvm_irq_routing) {
+            if (vector->virq >= 0) {
+                fd = event_notifier_get_fd(&vector->kvm_interrupt);
+            } else {
+                fd = event_notifier_get_fd(&vector->interrupt);
+            }
 
-        if (vfio_set_irq_signaling(&vdev->vbasedev,
-                                     VFIO_PCI_MSIX_IRQ_INDEX, nr,
-                                     VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err)) {
-            error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
+            if (vfio_set_irq_signaling(&vdev->vbasedev,
+                                       VFIO_PCI_MSIX_IRQ_INDEX, nr,
+                                       VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err)) {
+                error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
+            }
+        }
+        /* Increase for dynamic allocation case. */
+        if (vdev->nr_vectors < nr + 1) {
+            vdev->nr_vectors = nr + 1;
         }
     }
 
-- 
2.27.0

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

[Jing Liu]

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.
+     *
+     * 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)) {
+        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;
+        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;
+        }
+    }
+
     argsz = sizeof(*irq_set) + (vdev->nr_vectors * sizeof(*fds));
 
     irq_set = g_malloc0(argsz);
-- 
2.27.0

Re: [PATCH RFC v1 1/3] vfio/pci: detect the support of dynamic MSI-X allocation

[Cédric Le Goater]

Hello Jing,

On 7/27/23 09:24, Jing Liu wrote:
> From: Reinette Chatre <reinette.chatre@intel.com>
> 
> Kernel provides the guidance of dynamic MSI-X allocation support of
> passthrough device, by clearing the VFIO_IRQ_INFO_NORESIZE flag to
> guide user space.
> 
> Fetch and store the flags from host for later use to determine if
> specific flags are set.
> 
> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
> Signed-off-by: Jing Liu <jing2.liu@intel.com>
> ---
>   hw/vfio/pci.c        | 12 ++++++++++++
>   hw/vfio/pci.h        |  1 +
>   hw/vfio/trace-events |  2 ++
>   3 files changed, 15 insertions(+)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index a205c6b1130f..0c4ac0873d40 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -1572,6 +1572,7 @@ static void vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp)
>   
>   static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
>   {
> +    struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info) };
>       int ret;
>       Error *err = NULL;
>   
> @@ -1624,6 +1625,17 @@ static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
>           memory_region_set_enabled(&vdev->pdev.msix_table_mmio, false);
>       }
>   
> +    irq_info.index = VFIO_PCI_MSIX_IRQ_INDEX;
> +    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_GET_IRQ_INFO, &irq_info);
> +    if (ret) {
> +        /* This can fail for an old kernel or legacy PCI dev */
> +        trace_vfio_msix_setup_get_irq_info_failure(strerror(errno));

Is it possible to detect the error reported by a kernel (< 6.4) when
dynamic MSI-X are not supported. Looking at vfio_pci_ioctl_get_irq_info()
in the kernel, could info.flags be tested against VFIO_IRQ_INFO_NORESIZE ?

In that case, QEMU should report an error and the trace event is not
needed.

Thanks,

C.

> +    } else {
> +        vdev->msix->irq_info_flags = irq_info.flags;
> +    }
> +    trace_vfio_msix_setup_irq_info_flags(vdev->vbasedev.name,
> +                                         vdev->msix->irq_info_flags);
> +
>       return 0;
>   }
>   
> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> index a2771b9ff3cc..ad34ec56d0ae 100644
> --- a/hw/vfio/pci.h
> +++ b/hw/vfio/pci.h
> @@ -113,6 +113,7 @@ typedef struct VFIOMSIXInfo {
>       uint32_t table_offset;
>       uint32_t pba_offset;
>       unsigned long *pending;
> +    uint32_t irq_info_flags;
>   } VFIOMSIXInfo;
>   
>   #define TYPE_VFIO_PCI "vfio-pci"
> diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
> index ee7509e68e4f..7d4a398f044d 100644
> --- a/hw/vfio/trace-events
> +++ b/hw/vfio/trace-events
> @@ -28,6 +28,8 @@ vfio_pci_read_config(const char *name, int addr, int len, int val) " (%s, @0x%x,
>   vfio_pci_write_config(const char *name, int addr, int val, int len) " (%s, @0x%x, 0x%x, len=0x%x)"
>   vfio_msi_setup(const char *name, int pos) "%s PCI MSI CAP @0x%x"
>   vfio_msix_early_setup(const char *name, int pos, int table_bar, int offset, int entries) "%s PCI MSI-X CAP @0x%x, BAR %d, offset 0x%x, entries %d"
> +vfio_msix_setup_get_irq_info_failure(const char *errstr) "VFIO_DEVICE_GET_IRQ_INFO failure: %s"
> +vfio_msix_setup_irq_info_flags(const char *name, uint32_t flags) " (%s) MSI-X irq info flags 0x%x"
>   vfio_check_pcie_flr(const char *name) "%s Supports FLR via PCIe cap"
>   vfio_check_pm_reset(const char *name) "%s Supports PM reset"
>   vfio_check_af_flr(const char *name) "%s Supports FLR via AF cap"

Re: [PATCH RFC v1 2/3] vfio/pci: enable vector on dynamic MSI-X allocation

[Cédric Le Goater]

On 7/27/23 09:24, Jing Liu wrote:
> The vector_use callback is used to enable vector that is unmasked in
> guest. The kernel used to only support static MSI-X allocation. When
> allocating a new interrupt using "static MSI-X allocation" kernels,
> Qemu first disables all previously allocated vectors and then
> re-allocates all including the new one. The nr_vectors of VFIOPCIDevice
> indicates that all vectors from 0 to nr_vectors are allocated (and may
> be enabled), which is used to to loop all the possibly used vectors
> When, e.g., disabling MSI-X interrupts.
> 
> Extend the vector_use function to support dynamic MSI-X allocation when
> host supports the capability. Qemu therefore can individually allocate
> and enable a new interrupt without affecting others or causing interrupts
> lost during runtime.
> 
> Utilize nr_vectors to calculate the upper bound of enabled vectors in
> dynamic MSI-X allocation mode since looping all msix_entries_nr is not
> efficient and unnecessary.
> 
> Signed-off-by: Jing Liu <jing2.liu@intel.com>
> Tested-by: Reinette Chatre <reinette.chatre@intel.com>
> ---
>   hw/vfio/pci.c | 40 +++++++++++++++++++++++++++-------------
>   1 file changed, 27 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 0c4ac0873d40..8c485636445c 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -512,12 +512,20 @@ static int vfio_msix_vector_do_use(PCIDevice *pdev, unsigned int nr,
>       }
>   
>       /*
> -     * We don't want to have the host allocate all possible MSI vectors
> -     * for a device if they're not in use, so we shutdown and incrementally
> -     * increase them as needed.
> +     * When dynamic allocation is not supported, we don't want to have the
> +     * host allocate all possible MSI vectors for a device if they're not
> +     * in use, so we shutdown and incrementally increase them as needed.
> +     * And nr_vectors stands for the number of vectors being allocated.
> +     *
> +     * When dynamic allocation is supported, let the host only allocate
> +     * and enable a vector when it is in use in guest. nr_vectors stands
> +     * for the upper bound of vectors being enabled (but not all of the
> +     * ranges is allocated or enabled).
>        */
> -    if (vdev->nr_vectors < nr + 1) {
> +    if ((vdev->msix->irq_info_flags & VFIO_IRQ_INFO_NORESIZE) &&

I would add a small helper for this test: vfio_msix_can_alloc_dyn(vdev)

Thanks,

C.


> +        (vdev->nr_vectors < nr + 1)) {
>           vdev->nr_vectors = nr + 1;
> +
>           if (!vdev->defer_kvm_irq_routing) {
>               vfio_disable_irqindex(&vdev->vbasedev, VFIO_PCI_MSIX_IRQ_INDEX);
>               ret = vfio_enable_vectors(vdev, true);
> @@ -529,16 +537,22 @@ static int vfio_msix_vector_do_use(PCIDevice *pdev, unsigned int nr,
>           Error *err = NULL;
>           int32_t fd;
>   
> -        if (vector->virq >= 0) {
> -            fd = event_notifier_get_fd(&vector->kvm_interrupt);
> -        } else {
> -            fd = event_notifier_get_fd(&vector->interrupt);
> -        }
> +        if (!vdev->defer_kvm_irq_routing) {
> +            if (vector->virq >= 0) {
> +                fd = event_notifier_get_fd(&vector->kvm_interrupt);
> +            } else {
> +                fd = event_notifier_get_fd(&vector->interrupt);
> +            }
>   
> -        if (vfio_set_irq_signaling(&vdev->vbasedev,
> -                                     VFIO_PCI_MSIX_IRQ_INDEX, nr,
> -                                     VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err)) {
> -            error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
> +            if (vfio_set_irq_signaling(&vdev->vbasedev,
> +                                       VFIO_PCI_MSIX_IRQ_INDEX, nr,
> +                                       VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err)) {
> +                error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
> +            }
> +        }
> +        /* Increase for dynamic allocation case. */
> +        if (vdev->nr_vectors < nr + 1) {
> +            vdev->nr_vectors = nr + 1;
>           }
>       }
>   

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

[Alex Williamson]

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);

Re: [PATCH RFC v1 1/3] vfio/pci: detect the support of dynamic MSI-X allocation

[Alex Williamson]

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

> From: Reinette Chatre <reinette.chatre@intel.com>
> 
> Kernel provides the guidance of dynamic MSI-X allocation support of
> passthrough device, by clearing the VFIO_IRQ_INFO_NORESIZE flag to
> guide user space.
> 
> Fetch and store the flags from host for later use to determine if
> specific flags are set.
> 
> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
> Signed-off-by: Jing Liu <jing2.liu@intel.com>
> ---
>  hw/vfio/pci.c        | 12 ++++++++++++
>  hw/vfio/pci.h        |  1 +
>  hw/vfio/trace-events |  2 ++
>  3 files changed, 15 insertions(+)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index a205c6b1130f..0c4ac0873d40 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -1572,6 +1572,7 @@ static void vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp)
>  
>  static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
>  {
> +    struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info) };
>      int ret;
>      Error *err = NULL;
>  
> @@ -1624,6 +1625,17 @@ static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
>          memory_region_set_enabled(&vdev->pdev.msix_table_mmio, false);
>      }
>  
> +    irq_info.index = VFIO_PCI_MSIX_IRQ_INDEX;
> +    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_GET_IRQ_INFO, &irq_info);
> +    if (ret) {
> +        /* This can fail for an old kernel or legacy PCI dev */
> +        trace_vfio_msix_setup_get_irq_info_failure(strerror(errno));

We only call vfio_msix_setup() if the device has an MSI-X capability,
so the "legacy PCI" portion of this comment seems unjustified.
Otherwise the GET_IRQ_INFO ioctl has always existed, so I'd also
question the "old kernel" part of this comment.  We don't currently
sanity test the device exposed MSI-X info versus that reported by
GET_IRQ_INFO, but it seems valid to do so.  I'd expect this to happen
in vfio_msix_early_setup() though, especially since that's where the
remainder of VFIOMSIXInfo is setup.

> +    } else {
> +        vdev->msix->irq_info_flags = irq_info.flags;
> +    }
> +    trace_vfio_msix_setup_irq_info_flags(vdev->vbasedev.name,
> +                                         vdev->msix->irq_info_flags);
> +
>      return 0;
>  }
>  
> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> index a2771b9ff3cc..ad34ec56d0ae 100644
> --- a/hw/vfio/pci.h
> +++ b/hw/vfio/pci.h
> @@ -113,6 +113,7 @@ typedef struct VFIOMSIXInfo {
>      uint32_t table_offset;
>      uint32_t pba_offset;
>      unsigned long *pending;
> +    uint32_t irq_info_flags;

Why not simply pull out a "noresize" bool?  Thanks,

Alex

>  } VFIOMSIXInfo;
>  
>  #define TYPE_VFIO_PCI "vfio-pci"
> diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
> index ee7509e68e4f..7d4a398f044d 100644
> --- a/hw/vfio/trace-events
> +++ b/hw/vfio/trace-events
> @@ -28,6 +28,8 @@ vfio_pci_read_config(const char *name, int addr, int len, int val) " (%s, @0x%x,
>  vfio_pci_write_config(const char *name, int addr, int val, int len) " (%s, @0x%x, 0x%x, len=0x%x)"
>  vfio_msi_setup(const char *name, int pos) "%s PCI MSI CAP @0x%x"
>  vfio_msix_early_setup(const char *name, int pos, int table_bar, int offset, int entries) "%s PCI MSI-X CAP @0x%x, BAR %d, offset 0x%x, entries %d"
> +vfio_msix_setup_get_irq_info_failure(const char *errstr) "VFIO_DEVICE_GET_IRQ_INFO failure: %s"
> +vfio_msix_setup_irq_info_flags(const char *name, uint32_t flags) " (%s) MSI-X irq info flags 0x%x"
>  vfio_check_pcie_flr(const char *name) "%s Supports FLR via PCIe cap"
>  vfio_check_pm_reset(const char *name) "%s Supports PM reset"
>  vfio_check_af_flr(const char *name) "%s Supports FLR via AF cap"

Re: [PATCH RFC v1 2/3] vfio/pci: enable vector on dynamic MSI-X allocation

[Alex Williamson]

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

> The vector_use callback is used to enable vector that is unmasked in
> guest. The kernel used to only support static MSI-X allocation. When
> allocating a new interrupt using "static MSI-X allocation" kernels,
> Qemu first disables all previously allocated vectors and then
> re-allocates all including the new one. The nr_vectors of VFIOPCIDevice
> indicates that all vectors from 0 to nr_vectors are allocated (and may
> be enabled), which is used to to loop all the possibly used vectors
> When, e.g., disabling MSI-X interrupts.
> 
> Extend the vector_use function to support dynamic MSI-X allocation when
> host supports the capability. Qemu therefore can individually allocate
> and enable a new interrupt without affecting others or causing interrupts
> lost during runtime.
> 
> Utilize nr_vectors to calculate the upper bound of enabled vectors in
> dynamic MSI-X allocation mode since looping all msix_entries_nr is not
> efficient and unnecessary.
> 
> Signed-off-by: Jing Liu <jing2.liu@intel.com>
> Tested-by: Reinette Chatre <reinette.chatre@intel.com>
> ---
>  hw/vfio/pci.c | 40 +++++++++++++++++++++++++++-------------
>  1 file changed, 27 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 0c4ac0873d40..8c485636445c 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -512,12 +512,20 @@ static int vfio_msix_vector_do_use(PCIDevice *pdev, unsigned int nr,
>      }
>  
>      /*
> -     * We don't want to have the host allocate all possible MSI vectors
> -     * for a device if they're not in use, so we shutdown and incrementally
> -     * increase them as needed.
> +     * When dynamic allocation is not supported, we don't want to have the
> +     * host allocate all possible MSI vectors for a device if they're not
> +     * in use, so we shutdown and incrementally increase them as needed.
> +     * And nr_vectors stands for the number of vectors being allocated.

"nr_vectors represents the total number of vectors allocated."

> +     *
> +     * When dynamic allocation is supported, let the host only allocate
> +     * and enable a vector when it is in use in guest. nr_vectors stands
> +     * for the upper bound of vectors being enabled (but not all of the
> +     * ranges is allocated or enabled).

s/stands for/represents/

>       */
> -    if (vdev->nr_vectors < nr + 1) {
> +    if ((vdev->msix->irq_info_flags & VFIO_IRQ_INFO_NORESIZE) &&

Testing vdev->msix->noresize would be cleaner.

> +        (vdev->nr_vectors < nr + 1)) {
>          vdev->nr_vectors = nr + 1;
> +
>          if (!vdev->defer_kvm_irq_routing) {
>              vfio_disable_irqindex(&vdev->vbasedev, VFIO_PCI_MSIX_IRQ_INDEX);
>              ret = vfio_enable_vectors(vdev, true);
> @@ -529,16 +537,22 @@ static int vfio_msix_vector_do_use(PCIDevice *pdev, unsigned int nr,
>          Error *err = NULL;
>          int32_t fd;
>  
> -        if (vector->virq >= 0) {
> -            fd = event_notifier_get_fd(&vector->kvm_interrupt);
> -        } else {
> -            fd = event_notifier_get_fd(&vector->interrupt);
> -        }
> +        if (!vdev->defer_kvm_irq_routing) {
> +            if (vector->virq >= 0) {
> +                fd = event_notifier_get_fd(&vector->kvm_interrupt);
> +            } else {
> +                fd = event_notifier_get_fd(&vector->interrupt);
> +            }
>  
> -        if (vfio_set_irq_signaling(&vdev->vbasedev,
> -                                     VFIO_PCI_MSIX_IRQ_INDEX, nr,
> -                                     VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err)) {
> -            error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
> +            if (vfio_set_irq_signaling(&vdev->vbasedev,
> +                                       VFIO_PCI_MSIX_IRQ_INDEX, nr,
> +                                       VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err)) {
> +                error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
> +            }
> +        }
> +        /* Increase for dynamic allocation case. */
> +        if (vdev->nr_vectors < nr + 1) {
> +            vdev->nr_vectors = nr + 1;
>          }

We now have two branches where the bulk of the code is skipped when
defer_kvm_irq_routing is enabled and doing effectively the same update
to nr_vectors otherwise.  This suggests we should move the
defer_kvm_irq_routing test out and create a common place to update
nr_vectors.  Thanks,

Alex

RE: [PATCH RFC v1 1/3] vfio/pci: detect the support of dynamic MSI-X allocation

[Liu, Jing2]

Hi Alex,

Thanks very much for reviewing the patches.

> On July 28, 2023 1:25 AM, Alex Williamson <alex.williamson@redhat.com> wrote:
> 
> On Thu, 27 Jul 2023 03:24:08 -0400
> Jing Liu <jing2.liu@intel.com> wrote:
> 
> > From: Reinette Chatre <reinette.chatre@intel.com>
> >
> > Kernel provides the guidance of dynamic MSI-X allocation support of
> > passthrough device, by clearing the VFIO_IRQ_INFO_NORESIZE flag to
> > guide user space.
> >
> > Fetch and store the flags from host for later use to determine if
> > specific flags are set.
> >
> > Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
> > Signed-off-by: Jing Liu <jing2.liu@intel.com>
> > ---
> >  hw/vfio/pci.c        | 12 ++++++++++++
> >  hw/vfio/pci.h        |  1 +
> >  hw/vfio/trace-events |  2 ++
> >  3 files changed, 15 insertions(+)
> >
> > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index
> > a205c6b1130f..0c4ac0873d40 100644
> > --- a/hw/vfio/pci.c
> > +++ b/hw/vfio/pci.c
> > @@ -1572,6 +1572,7 @@ static void vfio_msix_early_setup(VFIOPCIDevice
> > *vdev, Error **errp)
> >
> >  static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error
> > **errp)  {
> > +    struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info) };
> >      int ret;
> >      Error *err = NULL;
> >
> > @@ -1624,6 +1625,17 @@ static int vfio_msix_setup(VFIOPCIDevice *vdev, int
> pos, Error **errp)
> >          memory_region_set_enabled(&vdev->pdev.msix_table_mmio, false);
> >      }
> >
> > +    irq_info.index = VFIO_PCI_MSIX_IRQ_INDEX;
> > +    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_GET_IRQ_INFO, &irq_info);
> > +    if (ret) {
> > +        /* This can fail for an old kernel or legacy PCI dev */
> > +        trace_vfio_msix_setup_get_irq_info_failure(strerror(errno));
> 
> We only call vfio_msix_setup() if the device has an MSI-X capability, so the
> "legacy PCI" portion of this comment seems unjustified.
> Otherwise the GET_IRQ_INFO ioctl has always existed, so I'd also question the
> "old kernel" part of this comment.  

Oh, yes, I just realize that only VFIO_PCI_ERR_IRQ_INDEX and 
VFIO_PCI_REQ_IRQ_INDEX were added later in include/uapi/linux/vfio.h. Thus,
this ioctl() with MSIX index would not fail by the old-kernel or legacy-PCI reason.
Thanks for pointing out this to me.

We don't currently sanity test the device
> exposed MSI-X info versus that reported by GET_IRQ_INFO, but it seems valid to
> do so.  

Do we want to keep the check of possible failure from kernel (e.g., -EFAULT) and report 
the error code back to caller? Maybe like this,

static void vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp)
{
    ....
    msix->entries = (ctrl & PCI_MSIX_FLAGS_QSIZE) + 1;

    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_GET_IRQ_INFO, &irq_info);
    if (ret < 0) {
        error_setg_errno(errp, -ret, "failed to get MSI-X IRQ INFO");
        return;
    } else {
        vdev->msix->noresize = !!(irq_info.flags & VFIO_IRQ_INFO_NORESIZE);
    }
    ...
    trace_vfio_msix_early_setup(vdev->vbasedev.name, pos, msix->table_bar,
                                msix->table_offset, msix->entries, vdev->msix->noresize);
    ....
}

> I'd expect this to happen in vfio_msix_early_setup() though, especially
> since that's where the remainder of VFIOMSIXInfo is setup.

> 
> > +    } else {
> > +        vdev->msix->irq_info_flags = irq_info.flags;
> > +    }
> > +    trace_vfio_msix_setup_irq_info_flags(vdev->vbasedev.name,
> > +                                         vdev->msix->irq_info_flags);
> > +
> >      return 0;
> >  }
> >
> > diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h index
> > a2771b9ff3cc..ad34ec56d0ae 100644
> > --- a/hw/vfio/pci.h
> > +++ b/hw/vfio/pci.h
> > @@ -113,6 +113,7 @@ typedef struct VFIOMSIXInfo {
> >      uint32_t table_offset;
> >      uint32_t pba_offset;
> >      unsigned long *pending;
> > +    uint32_t irq_info_flags;
> 
> Why not simply pull out a "noresize" bool?  Thanks,
> 
Will change to a bool type.

Thanks,
Jing

> Alex
> 
> >  } VFIOMSIXInfo;
> >
> >  #define TYPE_VFIO_PCI "vfio-pci"
> > diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events index
> > ee7509e68e4f..7d4a398f044d 100644
> > --- a/hw/vfio/trace-events
> > +++ b/hw/vfio/trace-events
> > @@ -28,6 +28,8 @@ vfio_pci_read_config(const char *name, int addr, int
> > len, int val) " (%s, @0x%x,  vfio_pci_write_config(const char *name, int addr, int
> val, int len) " (%s, @0x%x, 0x%x, len=0x%x)"
> >  vfio_msi_setup(const char *name, int pos) "%s PCI MSI CAP @0x%x"
> >  vfio_msix_early_setup(const char *name, int pos, int table_bar, int offset, int
> entries) "%s PCI MSI-X CAP @0x%x, BAR %d, offset 0x%x, entries %d"
> > +vfio_msix_setup_get_irq_info_failure(const char *errstr)
> "VFIO_DEVICE_GET_IRQ_INFO failure: %s"
> > +vfio_msix_setup_irq_info_flags(const char *name, uint32_t flags) " (%s) MSI-X
> irq info flags 0x%x"
> >  vfio_check_pcie_flr(const char *name) "%s Supports FLR via PCIe cap"
> >  vfio_check_pm_reset(const char *name) "%s Supports PM reset"
> >  vfio_check_af_flr(const char *name) "%s Supports FLR via AF cap"

Re: [PATCH RFC v1 1/3] vfio/pci: detect the support of dynamic MSI-X allocation

[Cédric Le Goater]

On 7/28/23 10:09, Liu, Jing2 wrote:
> Hi Alex,
> 
> Thanks very much for reviewing the patches.
> 
>> On July 28, 2023 1:25 AM, Alex Williamson <alex.williamson@redhat.com> wrote:
>>
>> On Thu, 27 Jul 2023 03:24:08 -0400
>> Jing Liu <jing2.liu@intel.com> wrote:
>>
>>> From: Reinette Chatre <reinette.chatre@intel.com>
>>>
>>> Kernel provides the guidance of dynamic MSI-X allocation support of
>>> passthrough device, by clearing the VFIO_IRQ_INFO_NORESIZE flag to
>>> guide user space.
>>>
>>> Fetch and store the flags from host for later use to determine if
>>> specific flags are set.
>>>
>>> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
>>> Signed-off-by: Jing Liu <jing2.liu@intel.com>
>>> ---
>>>   hw/vfio/pci.c        | 12 ++++++++++++
>>>   hw/vfio/pci.h        |  1 +
>>>   hw/vfio/trace-events |  2 ++
>>>   3 files changed, 15 insertions(+)
>>>
>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index
>>> a205c6b1130f..0c4ac0873d40 100644
>>> --- a/hw/vfio/pci.c
>>> +++ b/hw/vfio/pci.c
>>> @@ -1572,6 +1572,7 @@ static void vfio_msix_early_setup(VFIOPCIDevice
>>> *vdev, Error **errp)
>>>
>>>   static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error
>>> **errp)  {
>>> +    struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info) };
>>>       int ret;
>>>       Error *err = NULL;
>>>
>>> @@ -1624,6 +1625,17 @@ static int vfio_msix_setup(VFIOPCIDevice *vdev, int
>> pos, Error **errp)
>>>           memory_region_set_enabled(&vdev->pdev.msix_table_mmio, false);
>>>       }
>>>
>>> +    irq_info.index = VFIO_PCI_MSIX_IRQ_INDEX;
>>> +    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_GET_IRQ_INFO, &irq_info);
>>> +    if (ret) {
>>> +        /* This can fail for an old kernel or legacy PCI dev */
>>> +        trace_vfio_msix_setup_get_irq_info_failure(strerror(errno));
>>
>> We only call vfio_msix_setup() if the device has an MSI-X capability, so the
>> "legacy PCI" portion of this comment seems unjustified.
>> Otherwise the GET_IRQ_INFO ioctl has always existed, so I'd also question the
>> "old kernel" part of this comment.
> 
> Oh, yes, I just realize that only VFIO_PCI_ERR_IRQ_INDEX and
> VFIO_PCI_REQ_IRQ_INDEX were added later in include/uapi/linux/vfio.h. Thus,
> this ioctl() with MSIX index would not fail by the old-kernel or legacy-PCI reason.
> Thanks for pointing out this to me.
> 
> We don't currently sanity test the device
>> exposed MSI-X info versus that reported by GET_IRQ_INFO, but it seems valid to
>> do so.
> 
> Do we want to keep the check of possible failure from kernel (e.g., -EFAULT) and report
> the error code back to caller? Maybe like this,
> 
> static void vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp)
> {
>      ....
>      msix->entries = (ctrl & PCI_MSIX_FLAGS_QSIZE) + 1;
> 
>      ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_GET_IRQ_INFO, &irq_info);
>      if (ret < 0) {
>          error_setg_errno(errp, -ret, "failed to get MSI-X IRQ INFO");
>          return;
>      } else {
>          vdev->msix->noresize = !!(irq_info.flags & VFIO_IRQ_INFO_NORESIZE);
>      }
>      ...
>      trace_vfio_msix_early_setup(vdev->vbasedev.name, pos, msix->table_bar,
>                                  msix->table_offset, msix->entries, vdev->msix->noresize);

In the trace event, please ouput irq_info.flags since it gives more
information on the value returned by the kernel.

>      ....
> }
> 
>> I'd expect this to happen in vfio_msix_early_setup() though, especially
>> since that's where the remainder of VFIOMSIXInfo is setup.
> 
>>
>>> +    } else {
>>> +        vdev->msix->irq_info_flags = irq_info.flags;
>>> +    }
>>> +    trace_vfio_msix_setup_irq_info_flags(vdev->vbasedev.name,
>>> +                                         vdev->msix->irq_info_flags);
>>> +
>>>       return 0;
>>>   }
>>>
>>> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h index
>>> a2771b9ff3cc..ad34ec56d0ae 100644
>>> --- a/hw/vfio/pci.h
>>> +++ b/hw/vfio/pci.h
>>> @@ -113,6 +113,7 @@ typedef struct VFIOMSIXInfo {
>>>       uint32_t table_offset;
>>>       uint32_t pba_offset;
>>>       unsigned long *pending;
>>> +    uint32_t irq_info_flags;
>>
>> Why not simply pull out a "noresize" bool?  Thanks,
>>
> Will change to a bool type.

I would simply cache the KVM flags value under VFIOMSIXInfo as you
did and add an helper. Both work the same but the intial proposal
keeps more information. This is minor.

Thanks,

C.
  
> 
> Thanks,
> Jing
> 
>> Alex
>>
>>>   } VFIOMSIXInfo;
>>>
>>>   #define TYPE_VFIO_PCI "vfio-pci"
>>> diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events index
>>> ee7509e68e4f..7d4a398f044d 100644
>>> --- a/hw/vfio/trace-events
>>> +++ b/hw/vfio/trace-events
>>> @@ -28,6 +28,8 @@ vfio_pci_read_config(const char *name, int addr, int
>>> len, int val) " (%s, @0x%x,  vfio_pci_write_config(const char *name, int addr, int
>> val, int len) " (%s, @0x%x, 0x%x, len=0x%x)"
>>>   vfio_msi_setup(const char *name, int pos) "%s PCI MSI CAP @0x%x"
>>>   vfio_msix_early_setup(const char *name, int pos, int table_bar, int offset, int
>> entries) "%s PCI MSI-X CAP @0x%x, BAR %d, offset 0x%x, entries %d"
>>> +vfio_msix_setup_get_irq_info_failure(const char *errstr)
>> "VFIO_DEVICE_GET_IRQ_INFO failure: %s"
>>> +vfio_msix_setup_irq_info_flags(const char *name, uint32_t flags) " (%s) MSI-X
>> irq info flags 0x%x"
>>>   vfio_check_pcie_flr(const char *name) "%s Supports FLR via PCIe cap"
>>>   vfio_check_pm_reset(const char *name) "%s Supports PM reset"
>>>   vfio_check_af_flr(const char *name) "%s Supports FLR via AF cap"
> 

RE: [PATCH RFC v1 1/3] vfio/pci: detect the support of dynamic MSI-X allocation

[Liu, Jing2]

Hi C.,

Thanks very much for reviewing the patches.

> On July 28, 2023 12:58 AM, Cédric Le Goater <clg@redhat.com> wrote:
> 
> Hello Jing,
> 
> On 7/27/23 09:24, Jing Liu wrote:
> > From: Reinette Chatre <reinette.chatre@intel.com>
> >
> > Kernel provides the guidance of dynamic MSI-X allocation support of
> > passthrough device, by clearing the VFIO_IRQ_INFO_NORESIZE flag to
> > guide user space.
> >
> > Fetch and store the flags from host for later use to determine if
> > specific flags are set.
> >
> > Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
> > Signed-off-by: Jing Liu <jing2.liu@intel.com>
> > ---
> >   hw/vfio/pci.c        | 12 ++++++++++++
> >   hw/vfio/pci.h        |  1 +
> >   hw/vfio/trace-events |  2 ++
> >   3 files changed, 15 insertions(+)
> >
> > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index
> > a205c6b1130f..0c4ac0873d40 100644
> > --- a/hw/vfio/pci.c
> > +++ b/hw/vfio/pci.c
> > @@ -1572,6 +1572,7 @@ static void vfio_msix_early_setup(VFIOPCIDevice
> > *vdev, Error **errp)
> >
> >   static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error **errp)
> >   {
> > +    struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info) };
> >       int ret;
> >       Error *err = NULL;
> >
> > @@ -1624,6 +1625,17 @@ static int vfio_msix_setup(VFIOPCIDevice *vdev, int
> pos, Error **errp)
> >           memory_region_set_enabled(&vdev->pdev.msix_table_mmio, false);
> >       }
> >
> > +    irq_info.index = VFIO_PCI_MSIX_IRQ_INDEX;
> > +    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_GET_IRQ_INFO, &irq_info);
> > +    if (ret) {
> > +        /* This can fail for an old kernel or legacy PCI dev */
> > +        trace_vfio_msix_setup_get_irq_info_failure(strerror(errno));
> 
> Is it possible to detect the error reported by a kernel (< 6.4) when dynamic MSI-X
> are not supported. 

I just realized that ioctl(VFIO_DEVICE_GET_IRQ_INFO) with VFIO_PCI_MSIX_IRQ_INDEX
should always exists and would not cause failure for older kernel reason, after reading
Alex's comment on this patch. (If my understanding is correct)

So the possible failure might be other reason except old kernel or legacy PCI dev.

Looking at vfio_pci_ioctl_get_irq_info() in the kernel, could
> info.flags be tested against VFIO_IRQ_INFO_NORESIZE ?
> 
Sorry I didn't quite understand "info.flags be tested against VFIO_IRQ_INFO_NORESIZE".
I saw kernel < 6.4 simply added NORESIZE to info.flags and latest kernel adds if has_dyn_msix.
Would you please kindly describe more on your point?

> In that case, QEMU should report an error and the trace event is not needed.

I replied an email with new error handling draft code based on my understanding, which 
reports the error and need no trace. Could you please help review if that is what we want?

Thanks,
Jing

> 
> Thanks,
> 
> C.
> 
> > +    } else {
> > +        vdev->msix->irq_info_flags = irq_info.flags;
> > +    }
> > +    trace_vfio_msix_setup_irq_info_flags(vdev->vbasedev.name,
> > +                                         vdev->msix->irq_info_flags);
> > +
> >       return 0;
> >   }
> >
> > diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h index
> > a2771b9ff3cc..ad34ec56d0ae 100644
> > --- a/hw/vfio/pci.h
> > +++ b/hw/vfio/pci.h
> > @@ -113,6 +113,7 @@ typedef struct VFIOMSIXInfo {
> >       uint32_t table_offset;
> >       uint32_t pba_offset;
> >       unsigned long *pending;
> > +    uint32_t irq_info_flags;
> >   } VFIOMSIXInfo;
> >
> >   #define TYPE_VFIO_PCI "vfio-pci"
> > diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events index
> > ee7509e68e4f..7d4a398f044d 100644
> > --- a/hw/vfio/trace-events
> > +++ b/hw/vfio/trace-events
> > @@ -28,6 +28,8 @@ vfio_pci_read_config(const char *name, int addr, int len,
> int val) " (%s, @0x%x,
> >   vfio_pci_write_config(const char *name, int addr, int val, int len) " (%s,
> @0x%x, 0x%x, len=0x%x)"
> >   vfio_msi_setup(const char *name, int pos) "%s PCI MSI CAP @0x%x"
> >   vfio_msix_early_setup(const char *name, int pos, int table_bar, int offset, int
> entries) "%s PCI MSI-X CAP @0x%x, BAR %d, offset 0x%x, entries %d"
> > +vfio_msix_setup_get_irq_info_failure(const char *errstr)
> "VFIO_DEVICE_GET_IRQ_INFO failure: %s"
> > +vfio_msix_setup_irq_info_flags(const char *name, uint32_t flags) " (%s) MSI-X
> irq info flags 0x%x"
> >   vfio_check_pcie_flr(const char *name) "%s Supports FLR via PCIe cap"
> >   vfio_check_pm_reset(const char *name) "%s Supports PM reset"
> >   vfio_check_af_flr(const char *name) "%s Supports FLR via AF cap"

Re: [PATCH RFC v1 1/3] vfio/pci: detect the support of dynamic MSI-X allocation

[Cédric Le Goater]

[ ... ]

> Sorry I didn't quite understand "info.flags be tested against VFIO_IRQ_INFO_NORESIZE".
> I saw kernel < 6.4 simply added NORESIZE to info.flags and latest kernel adds if has_dyn_msix.
> Would you please kindly describe more on your point?

I was trying to find the conditions to detect safely that the kernel didn't
have dynamic MSI-X support. Testing VFIO_IRQ_INFO_NORESIZE seems enough.

>> In that case, QEMU should report an error and the trace event is not needed.
> 
> I replied an email with new error handling draft code based on my understanding, which
> reports the error and need no trace. Could you please help review if that is what we want?

yes. It looked good. Please send a v1 !

Thanks,

Cédric.

Re: [PATCH RFC v1 1/3] vfio/pci: detect the support of dynamic MSI-X allocation

[Alex Williamson]

On Fri, 28 Jul 2023 10:27:17 +0200
Cédric Le Goater <clg@redhat.com> wrote:

> On 7/28/23 10:09, Liu, Jing2 wrote:
> > Hi Alex,
> > 
> > Thanks very much for reviewing the patches.
> >   
> >> On July 28, 2023 1:25 AM, Alex Williamson <alex.williamson@redhat.com> wrote:
> >>
> >> On Thu, 27 Jul 2023 03:24:08 -0400
> >> Jing Liu <jing2.liu@intel.com> wrote:
> >>  
> >>> From: Reinette Chatre <reinette.chatre@intel.com>
> >>>
> >>> Kernel provides the guidance of dynamic MSI-X allocation support of
> >>> passthrough device, by clearing the VFIO_IRQ_INFO_NORESIZE flag to
> >>> guide user space.
> >>>
> >>> Fetch and store the flags from host for later use to determine if
> >>> specific flags are set.
> >>>
> >>> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
> >>> Signed-off-by: Jing Liu <jing2.liu@intel.com>
> >>> ---
> >>>   hw/vfio/pci.c        | 12 ++++++++++++
> >>>   hw/vfio/pci.h        |  1 +
> >>>   hw/vfio/trace-events |  2 ++
> >>>   3 files changed, 15 insertions(+)
> >>>
> >>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index
> >>> a205c6b1130f..0c4ac0873d40 100644
> >>> --- a/hw/vfio/pci.c
> >>> +++ b/hw/vfio/pci.c
> >>> @@ -1572,6 +1572,7 @@ static void vfio_msix_early_setup(VFIOPCIDevice
> >>> *vdev, Error **errp)
> >>>
> >>>   static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error
> >>> **errp)  {
> >>> +    struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info) };
> >>>       int ret;
> >>>       Error *err = NULL;
> >>>
> >>> @@ -1624,6 +1625,17 @@ static int vfio_msix_setup(VFIOPCIDevice *vdev, int  
> >> pos, Error **errp)  
> >>>           memory_region_set_enabled(&vdev->pdev.msix_table_mmio, false);
> >>>       }
> >>>
> >>> +    irq_info.index = VFIO_PCI_MSIX_IRQ_INDEX;
> >>> +    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_GET_IRQ_INFO, &irq_info);
> >>> +    if (ret) {
> >>> +        /* This can fail for an old kernel or legacy PCI dev */
> >>> +        trace_vfio_msix_setup_get_irq_info_failure(strerror(errno));  
> >>
> >> We only call vfio_msix_setup() if the device has an MSI-X capability, so the
> >> "legacy PCI" portion of this comment seems unjustified.
> >> Otherwise the GET_IRQ_INFO ioctl has always existed, so I'd also question the
> >> "old kernel" part of this comment.  
> > 
> > Oh, yes, I just realize that only VFIO_PCI_ERR_IRQ_INDEX and
> > VFIO_PCI_REQ_IRQ_INDEX were added later in include/uapi/linux/vfio.h. Thus,
> > this ioctl() with MSIX index would not fail by the old-kernel or legacy-PCI reason.
> > Thanks for pointing out this to me.
> > 
> > We don't currently sanity test the device  
> >> exposed MSI-X info versus that reported by GET_IRQ_INFO, but it seems valid to
> >> do so.  
> > 
> > Do we want to keep the check of possible failure from kernel (e.g., -EFAULT) and report
> > the error code back to caller? Maybe like this,
> > 
> > static void vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp)
> > {
> >      ....
> >      msix->entries = (ctrl & PCI_MSIX_FLAGS_QSIZE) + 1;
> > 
> >      ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_GET_IRQ_INFO, &irq_info);
> >      if (ret < 0) {
> >          error_setg_errno(errp, -ret, "failed to get MSI-X IRQ INFO");
> >          return;

Yes.

> >      } else {
> >          vdev->msix->noresize = !!(irq_info.flags & VFIO_IRQ_INFO_NORESIZE);
> >      }

No 'else' required here since the error branch returns.

> >      ...
> >      trace_vfio_msix_early_setup(vdev->vbasedev.name, pos, msix->table_bar,
> >                                  msix->table_offset, msix->entries, vdev->msix->noresize);  
> 
> In the trace event, please ouput irq_info.flags since it gives more
> information on the value returned by the kernel.
> 
> >      ....
> > }
> >   
> >> I'd expect this to happen in vfio_msix_early_setup() though, especially
> >> since that's where the remainder of VFIOMSIXInfo is setup.  
> >   
> >>  
> >>> +    } else {
> >>> +        vdev->msix->irq_info_flags = irq_info.flags;
> >>> +    }
> >>> +    trace_vfio_msix_setup_irq_info_flags(vdev->vbasedev.name,
> >>> +                                         vdev->msix->irq_info_flags);
> >>> +
> >>>       return 0;
> >>>   }
> >>>
> >>> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h index
> >>> a2771b9ff3cc..ad34ec56d0ae 100644
> >>> --- a/hw/vfio/pci.h
> >>> +++ b/hw/vfio/pci.h
> >>> @@ -113,6 +113,7 @@ typedef struct VFIOMSIXInfo {
> >>>       uint32_t table_offset;
> >>>       uint32_t pba_offset;
> >>>       unsigned long *pending;
> >>> +    uint32_t irq_info_flags;  
> >>
> >> Why not simply pull out a "noresize" bool?  Thanks,
> >>  
> > Will change to a bool type.  
> 
> I would simply cache the KVM flags value under VFIOMSIXInfo as you
> did and add an helper. Both work the same but the intial proposal
> keeps more information. This is minor.

TBH, I'd still prefer that we only store the one field we care about
and avoid an extra helper, regardless of how simple it might be.  Even
if we eventually add masking for MSI-X, we can store it in less space
and more accessibly decoded in the VFIOMSIXInfo struct vs helpers to
access a cached flags value.  Thanks,

Alex

Re: [PATCH RFC v1 1/3] vfio/pci: detect the support of dynamic MSI-X allocation

[Cédric Le Goater]

[ ... ]

>>>>> --- a/hw/vfio/pci.h
>>>>> +++ b/hw/vfio/pci.h
>>>>> @@ -113,6 +113,7 @@ typedef struct VFIOMSIXInfo {
>>>>>        uint32_t table_offset;
>>>>>        uint32_t pba_offset;
>>>>>        unsigned long *pending;
>>>>> +    uint32_t irq_info_flags;
>>>>
>>>> Why not simply pull out a "noresize" bool?  Thanks,
>>>>   
>>> Will change to a bool type.
>>
>> I would simply cache the KVM flags value under VFIOMSIXInfo as you
>> did and add an helper. Both work the same but the intial proposal
>> keeps more information. This is minor.
> 
> TBH, I'd still prefer that we only store the one field we care about
> and avoid an extra helper, regardless of how simple it might be.  Even
> if we eventually add masking for MSI-X, we can store it in less space
> and more accessibly decoded in the VFIOMSIXInfo struct vs helpers to
> access a cached flags value.  Thanks,

Let's use a bool then. np.

Thanks,

C.

RE: [PATCH RFC v1 1/3] vfio/pci: detect the support of dynamic MSI-X allocation

[Liu, Jing2]

Hi Alex,

> On July 28, 2023 11:41 PM, Alex Williamson <alex.williamson@redhat.com> wrote:
> 
> On Fri, 28 Jul 2023 10:27:17 +0200
> Cédric Le Goater <clg@redhat.com> wrote:
> 
> > On 7/28/23 10:09, Liu, Jing2 wrote:
> > > Hi Alex,
> > >
> > > Thanks very much for reviewing the patches.
> > >
> > >> On July 28, 2023 1:25 AM, Alex Williamson
> <alex.williamson@redhat.com> wrote:
> > >>
> > >> On Thu, 27 Jul 2023 03:24:08 -0400
> > >> Jing Liu <jing2.liu@intel.com> wrote:
> > >>
> > >>> From: Reinette Chatre <reinette.chatre@intel.com>
> > >>>
> > >>> Kernel provides the guidance of dynamic MSI-X allocation support
> > >>> of passthrough device, by clearing the VFIO_IRQ_INFO_NORESIZE flag
> > >>> to guide user space.
> > >>>
> > >>> Fetch and store the flags from host for later use to determine if
> > >>> specific flags are set.
> > >>>
> > >>> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
> > >>> Signed-off-by: Jing Liu <jing2.liu@intel.com>
> > >>> ---
> > >>>   hw/vfio/pci.c        | 12 ++++++++++++
> > >>>   hw/vfio/pci.h        |  1 +
> > >>>   hw/vfio/trace-events |  2 ++
> > >>>   3 files changed, 15 insertions(+)
> > >>>
> > >>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index
> > >>> a205c6b1130f..0c4ac0873d40 100644
> > >>> --- a/hw/vfio/pci.c
> > >>> +++ b/hw/vfio/pci.c
> > >>> @@ -1572,6 +1572,7 @@ static void
> > >>> vfio_msix_early_setup(VFIOPCIDevice
> > >>> *vdev, Error **errp)
> > >>>
> > >>>   static int vfio_msix_setup(VFIOPCIDevice *vdev, int pos, Error
> > >>> **errp)  {
> > >>> +    struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info)
> > >>> + };
> > >>>       int ret;
> > >>>       Error *err = NULL;
> > >>>
> > >>> @@ -1624,6 +1625,17 @@ static int vfio_msix_setup(VFIOPCIDevice
> > >>> *vdev, int
> > >> pos, Error **errp)
> > >>>           memory_region_set_enabled(&vdev->pdev.msix_table_mmio,
> false);
> > >>>       }
> > >>>
> > >>> +    irq_info.index = VFIO_PCI_MSIX_IRQ_INDEX;
> > >>> +    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_GET_IRQ_INFO,
> &irq_info);
> > >>> +    if (ret) {
> > >>> +        /* This can fail for an old kernel or legacy PCI dev */
> > >>> +
> > >>> + trace_vfio_msix_setup_get_irq_info_failure(strerror(errno));
> > >>
> > >> We only call vfio_msix_setup() if the device has an MSI-X
> > >> capability, so the "legacy PCI" portion of this comment seems unjustified.
> > >> Otherwise the GET_IRQ_INFO ioctl has always existed, so I'd also
> > >> question the "old kernel" part of this comment.
> > >
> > > Oh, yes, I just realize that only VFIO_PCI_ERR_IRQ_INDEX and
> > > VFIO_PCI_REQ_IRQ_INDEX were added later in
> > > include/uapi/linux/vfio.h. Thus, this ioctl() with MSIX index would not fail by
> the old-kernel or legacy-PCI reason.
> > > Thanks for pointing out this to me.
> > >
> > > We don't currently sanity test the device
> > >> exposed MSI-X info versus that reported by GET_IRQ_INFO, but it
> > >> seems valid to do so.
> > >
> > > Do we want to keep the check of possible failure from kernel (e.g.,
> > > -EFAULT) and report the error code back to caller? Maybe like this,
> > >
> > > static void vfio_msix_early_setup(VFIOPCIDevice *vdev, Error **errp)
> > > {
> > >      ....
> > >      msix->entries = (ctrl & PCI_MSIX_FLAGS_QSIZE) + 1;
> > >
> > >      ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_GET_IRQ_INFO, &irq_info);
> > >      if (ret < 0) {
> > >          error_setg_errno(errp, -ret, "failed to get MSI-X IRQ INFO");
> > >          return;
> 
> Yes.
> 
> > >      } else {
> > >          vdev->msix->noresize = !!(irq_info.flags & VFIO_IRQ_INFO_NORESIZE);
> > >      }
> 
> No 'else' required here since the error branch returns.

Oh, yes. Will remove the "else" and just set the ”noresize“ value in next version.

> 
> > >      ...
> > >      trace_vfio_msix_early_setup(vdev->vbasedev.name, pos, msix-
> >table_bar,
> > >                                  msix->table_offset, msix->entries,
> > > vdev->msix->noresize);
> >
> > In the trace event, please ouput irq_info.flags since it gives more
> > information on the value returned by the kernel.
> >
> > >      ....
> > > }
> > >
> > >> I'd expect this to happen in vfio_msix_early_setup() though,
> > >> especially since that's where the remainder of VFIOMSIXInfo is setup.
> > >
> > >>
> > >>> +    } else {
> > >>> +        vdev->msix->irq_info_flags = irq_info.flags;
> > >>> +    }
> > >>> +    trace_vfio_msix_setup_irq_info_flags(vdev->vbasedev.name,
> > >>> +
> > >>> + vdev->msix->irq_info_flags);
> > >>> +
> > >>>       return 0;
> > >>>   }
> > >>>
> > >>> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h index
> > >>> a2771b9ff3cc..ad34ec56d0ae 100644
> > >>> --- a/hw/vfio/pci.h
> > >>> +++ b/hw/vfio/pci.h
> > >>> @@ -113,6 +113,7 @@ typedef struct VFIOMSIXInfo {
> > >>>       uint32_t table_offset;
> > >>>       uint32_t pba_offset;
> > >>>       unsigned long *pending;
> > >>> +    uint32_t irq_info_flags;
> > >>
> > >> Why not simply pull out a "noresize" bool?  Thanks,
> > >>
> > > Will change to a bool type.
> >
> > I would simply cache the KVM flags value under VFIOMSIXInfo as you did
> > and add an helper. Both work the same but the intial proposal keeps
> > more information. This is minor.
> 
> TBH, I'd still prefer that we only store the one field we care about and avoid an
> extra helper, regardless of how simple it might be.  Even if we eventually add
> masking for MSI-X, we can store it in less space and more accessibly decoded in
> the VFIOMSIXInfo struct vs helpers to access a cached flags value.  Thanks,
> 

Got it, thanks,

Jing

> Alex

RE: [PATCH RFC v1 1/3] vfio/pci: detect the support of dynamic MSI-X allocation

[Liu, Jing2]

Hi C.

> On July 28, 2023 4:44 PM, Cédric Le Goater <clg@redhat.com> wrote:
> 
> [ ... ]
> 
> > Sorry I didn't quite understand "info.flags be tested against
> VFIO_IRQ_INFO_NORESIZE".
> > I saw kernel < 6.4 simply added NORESIZE to info.flags and latest kernel adds
> if has_dyn_msix.
> > Would you please kindly describe more on your point?
> 
> I was trying to find the conditions to detect safely that the kernel didn't have
> dynamic MSI-X support. Testing VFIO_IRQ_INFO_NORESIZE seems enough.
> 
Oh, I see.

> >> In that case, QEMU should report an error and the trace event is not
> needed.
> >
> > I replied an email with new error handling draft code based on my
> > understanding, which reports the error and need no trace. Could you please
> help review if that is what we want?
> 
> yes. It looked good. Please send a v1 !

Thanks for reviewing that. I guess you mean v2 for next version 😊

Jing
> 
> Thanks,
> 
> Cédric.

RE: [PATCH RFC v1 2/3] vfio/pci: enable vector on dynamic MSI-X allocation

[Liu, Jing2]

Hi Alex,

> On July 28, 2023 1:25 AM,  Alex Williamson <alex.williamson@redhat.com> wrote:
> 
> On Thu, 27 Jul 2023 03:24:09 -0400
> Jing Liu <jing2.liu@intel.com> wrote:
> 
> > The vector_use callback is used to enable vector that is unmasked in
> > guest. The kernel used to only support static MSI-X allocation. When
> > allocating a new interrupt using "static MSI-X allocation" kernels,
> > Qemu first disables all previously allocated vectors and then
> > re-allocates all including the new one. The nr_vectors of
> > VFIOPCIDevice indicates that all vectors from 0 to nr_vectors are
> > allocated (and may be enabled), which is used to to loop all the
> > possibly used vectors When, e.g., disabling MSI-X interrupts.
> >
> > Extend the vector_use function to support dynamic MSI-X allocation
> > when host supports the capability. Qemu therefore can individually
> > allocate and enable a new interrupt without affecting others or
> > causing interrupts lost during runtime.
> >
> > Utilize nr_vectors to calculate the upper bound of enabled vectors in
> > dynamic MSI-X allocation mode since looping all msix_entries_nr is not
> > efficient and unnecessary.
> >
> > Signed-off-by: Jing Liu <jing2.liu@intel.com>
> > Tested-by: Reinette Chatre <reinette.chatre@intel.com>
> > ---
> >  hw/vfio/pci.c | 40 +++++++++++++++++++++++++++-------------
> >  1 file changed, 27 insertions(+), 13 deletions(-)
> >
> > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index
> > 0c4ac0873d40..8c485636445c 100644
> > --- a/hw/vfio/pci.c
> > +++ b/hw/vfio/pci.c
> > @@ -512,12 +512,20 @@ static int vfio_msix_vector_do_use(PCIDevice
> *pdev, unsigned int nr,
> >      }
> >
> >      /*
> > -     * We don't want to have the host allocate all possible MSI vectors
> > -     * for a device if they're not in use, so we shutdown and incrementally
> > -     * increase them as needed.
> > +     * When dynamic allocation is not supported, we don't want to have the
> > +     * host allocate all possible MSI vectors for a device if they're not
> > +     * in use, so we shutdown and incrementally increase them as needed.
> > +     * And nr_vectors stands for the number of vectors being allocated.
> 
> "nr_vectors represents the total number of vectors allocated."

Will change.

> 
> > +     *
> > +     * When dynamic allocation is supported, let the host only allocate
> > +     * and enable a vector when it is in use in guest. nr_vectors stands
> > +     * for the upper bound of vectors being enabled (but not all of the
> > +     * ranges is allocated or enabled).
> 
> s/stands for/represents/
Will change.
> 
> >       */
> > -    if (vdev->nr_vectors < nr + 1) {
> > +    if ((vdev->msix->irq_info_flags & VFIO_IRQ_INFO_NORESIZE) &&
> 
> Testing vdev->msix->noresize would be cleaner.
> 
> > +        (vdev->nr_vectors < nr + 1)) {
> >          vdev->nr_vectors = nr + 1;
> > +
> >          if (!vdev->defer_kvm_irq_routing) {
> >              vfio_disable_irqindex(&vdev->vbasedev, VFIO_PCI_MSIX_IRQ_INDEX);
> >              ret = vfio_enable_vectors(vdev, true); @@ -529,16 +537,22
> > @@ static int vfio_msix_vector_do_use(PCIDevice *pdev, unsigned int nr,
> >          Error *err = NULL;
> >          int32_t fd;
> >
> > -        if (vector->virq >= 0) {
> > -            fd = event_notifier_get_fd(&vector->kvm_interrupt);
> > -        } else {
> > -            fd = event_notifier_get_fd(&vector->interrupt);
> > -        }
> > +        if (!vdev->defer_kvm_irq_routing) {
> > +            if (vector->virq >= 0) {
> > +                fd = event_notifier_get_fd(&vector->kvm_interrupt);
> > +            } else {
> > +                fd = event_notifier_get_fd(&vector->interrupt);
> > +            }
> >
> > -        if (vfio_set_irq_signaling(&vdev->vbasedev,
> > -                                     VFIO_PCI_MSIX_IRQ_INDEX, nr,
> > -                                     VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err)) {
> > -            error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
> > +            if (vfio_set_irq_signaling(&vdev->vbasedev,
> > +                                       VFIO_PCI_MSIX_IRQ_INDEX, nr,
> > +                                       VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err)) {
> > +                error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
> > +            }
> > +        }
> > +        /* Increase for dynamic allocation case. */
> > +        if (vdev->nr_vectors < nr + 1) {
> > +            vdev->nr_vectors = nr + 1;
> >          }
> 
> We now have two branches where the bulk of the code is skipped when
> defer_kvm_irq_routing is enabled and doing effectively the same update to
> nr_vectors otherwise.  This suggests we should move the
> defer_kvm_irq_routing test out and create a common place to update
> nr_vectors.  Thanks,

I make a new logic as follows that moves the defer_kvm_irq_routing test out.
Since the vfio_enable_vectors() function need an updated nr_vectors value
so need first update and test the different conditions using old value,
e.g. old_nr_vec.

int old_nr_vec = vdev->nr_vectors;
...
...
if (vdev->nr_vectors < nr + 1) {
    vdev->nr_vectors = nr + 1;
}
if (!vdev->defer_kvm_irq_routing) {
    if (vdev->msix->noresize && (old_nr_vec < nr + 1)) {
            vfio_disable_irqindex(&vdev->vbasedev, VFIO_PCI_MSIX_IRQ_INDEX);
            ret = vfio_enable_vectors(vdev, true);  // use updated nr_vectors
            ...
    } else {
            if (vector->virq >= 0) {
                fd = event_notifier_get_fd(&vector->kvm_interrupt);
            } else {
                fd = event_notifier_get_fd(&vector->interrupt);
            }
            if (vfio_set_irq_signaling(&vdev->vbasedev,
                                       VFIO_PCI_MSIX_IRQ_INDEX, nr,
                                       VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err)) {
                error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
            }        
    }
}
Thanks,
Jing

> 
> Alex

Re: [PATCH RFC v1 1/3] vfio/pci: detect the support of dynamic MSI-X allocation

[Cédric Le Goater]

On 7/31/23 05:57, Liu, Jing2 wrote:
> Hi C.
> 
>> On July 28, 2023 4:44 PM, Cédric Le Goater <clg@redhat.com> wrote:
>>
>> [ ... ]
>>
>>> Sorry I didn't quite understand "info.flags be tested against
>> VFIO_IRQ_INFO_NORESIZE".
>>> I saw kernel < 6.4 simply added NORESIZE to info.flags and latest kernel adds
>> if has_dyn_msix.
>>> Would you please kindly describe more on your point?
>>
>> I was trying to find the conditions to detect safely that the kernel didn't have
>> dynamic MSI-X support. Testing VFIO_IRQ_INFO_NORESIZE seems enough.
>>
> Oh, I see.
> 
>>>> In that case, QEMU should report an error and the trace event is not
>> needed.
>>>
>>> I replied an email with new error handling draft code based on my
>>> understanding, which reports the error and need no trace. Could you please
>> help review if that is what we want?
>>
>> yes. It looked good. Please send a v1 !
> 
> Thanks for reviewing that. I guess you mean v2 for next version 😊

Well, if you remove the RFC status, I think you should, this would
still be a v1.

Thanks,

C.

RE: [PATCH RFC v1 1/3] vfio/pci: detect the support of dynamic MSI-X allocation

[Liu, Jing2]

Hi C.

> On July 31, 2023 3:26 PM, Cédric Le Goater <clg@redhat.com> wrote:
> 
> On 7/31/23 05:57, Liu, Jing2 wrote:
> > Hi C.
> >
> >> On July 28, 2023 4:44 PM, Cédric Le Goater <clg@redhat.com> wrote:
> >>
> >> [ ... ]
> >>
> >>> Sorry I didn't quite understand "info.flags be tested against
> >> VFIO_IRQ_INFO_NORESIZE".
> >>> I saw kernel < 6.4 simply added NORESIZE to info.flags and latest
> >>> kernel adds
> >> if has_dyn_msix.
> >>> Would you please kindly describe more on your point?
> >>
> >> I was trying to find the conditions to detect safely that the kernel
> >> didn't have dynamic MSI-X support. Testing VFIO_IRQ_INFO_NORESIZE
> seems enough.
> >>
> > Oh, I see.
> >
> >>>> In that case, QEMU should report an error and the trace event is
> >>>> not
> >> needed.
> >>>
> >>> I replied an email with new error handling draft code based on my
> >>> understanding, which reports the error and need no trace. Could you
> >>> please
> >> help review if that is what we want?
> >>
> >> yes. It looked good. Please send a v1 !
> >
> > Thanks for reviewing that. I guess you mean v2 for next version 😊
> 
> Well, if you remove the RFC status, I think you should, this would still be a v1.
> 
Oh, got it. Thanks for telling this.

Jing

> Thanks,
> 
> C.

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

[Liu, Jing2]

Hi Alex,

> On July 28, 2023 1:25 AM, Alex Williamson <alex.williamson@redhat.com> wrote:
> 
> 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/
Will change.

> 
> > +     *
> > +     * 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.
Sure, will change.
> 
> > +        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?
Ah, yes. Will remove the test.

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

Yes, that's what I can think out to get MSI-X enabled first. The only question is that,
when getting kernel to call vfio_msi_enable() with nvec=1, kernel will allocate one
interrupt along with enabling MSI-X, which cannot avoid. 

Therefore, if we set vector 0 for example, irq for vec 0 will be allocated in kernel.
And later if vector 0 is unmasked in guest, then enable it as normal; but if vector 0
is always masked in guest, then we leave an allocated irq there (unenabled though)
until MSI-X disable.
I'm not sure if this is okay, but cannot think out other cleaner way.
And I also wonder if it is possible, or vector 0 is always being enabled?


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.  

They are similar. Use a do_use to setup userspace triggering also makes kernel
one allocated irq there. And my understanding is that, the following release function
actually won't release if it is a userspace trigger.

static void vfio_msix_vector_release(PCIDevice *pdev, unsigned int nr)
{
    /*
     * There are still old guests that mask and unmask vectors on every
     * interrupt.  If we're using QEMU bypass with a KVM irqfd, leave all of
     * the KVM setup in place, simply switch VFIO to use the non-bypass
     * eventfd.  We'll then fire the interrupt through QEMU and the MSI-X
     * core will mask the interrupt and set pending bits, allowing it to
     * be re-asserted on unmask.  Nothing to do if already using QEMU mode.
     */
    ...
}

 
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,

Oh, yes, agree that uses fd=-1 trick is cleaner and we don't need depend on the maskable
bit in qemu. According to your suggestion, I will create a function e.g., 
vfio_enable_msix_no_vec(vdev), which only sets vector 0 with fd=-1 to kernel, and 
returns the result back.

Thanks,
Jing
> 
> Alex
> 
> > +
> >      argsz = sizeof(*irq_set) + (vdev->nr_vectors * sizeof(*fds));
> >
> >      irq_set = g_malloc0(argsz);