vfio/pci: Clear MSI-X IRQ index always

[PATCH] vfio/pci: Clear MSI-X IRQ index always

Posted by Cédric Le Goater 2 years ago

When doing device assignment of a physical device, MSI-X can be
enabled with no vectors enabled and this sets the IRQ index to
VFIO_PCI_MSIX_IRQ_INDEX. However, when MSI-X is disabled, the IRQ
index is left untouched if no vectors are in use. Then, when INTx
is enabled, the IRQ index value is considered incompatible (set to
MSI-X) and VFIO_DEVICE_SET_IRQS fails. QEMU complains with :

qemu-system-x86_64: vfio 0000:08:00.0: Failed to set up TRIGGER eventfd signaling for interrupt INTX-0: VFIO_DEVICE_SET_IRQS failure: Invalid argument

To avoid that, unconditionaly clear the IRQ index when MSI-X is
disabled.

Buglink: https://issues.redhat.com/browse/RHEL-21293
Fixes: 5ebffa4e87e7 ("vfio/pci: use an invalid fd to enable MSI-X")
Cc: Jing Liu <jing2.liu@intel.com>
Cc: Alex Williamson <alex.williamson@redhat.com>
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 hw/vfio/pci.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index d7fe06715c4b9cde66a68c31aaf405315921b0d6..4fa387f0430d62ca2ba1b5ae5b7037f8f06b33f9 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -826,9 +826,11 @@ static void vfio_msix_disable(VFIOPCIDevice *vdev)
         }
     }
 
-    if (vdev->nr_vectors) {
-        vfio_disable_irqindex(&vdev->vbasedev, VFIO_PCI_MSIX_IRQ_INDEX);
-    }
+    /*
+     * Always clear MSI-X IRQ index. A PF device could have enabled
+     * MSI-X with no vectors. See vfio_msix_enable().
+     */
+    vfio_disable_irqindex(&vdev->vbasedev, VFIO_PCI_MSIX_IRQ_INDEX);
 
     vfio_msi_disable_common(vdev);
     vfio_intx_enable(vdev, &err);
-- 
2.43.0

Re: [PATCH] vfio/pci: Clear MSI-X IRQ index always

Posted by Michael Tokarev 2 years ago

25.01.2024 17:42, Cédric Le Goater:
> When doing device assignment of a physical device, MSI-X can be
> enabled with no vectors enabled and this sets the IRQ index to
> VFIO_PCI_MSIX_IRQ_INDEX. However, when MSI-X is disabled, the IRQ
> index is left untouched if no vectors are in use. Then, when INTx
> is enabled, the IRQ index value is considered incompatible (set to
> MSI-X) and VFIO_DEVICE_SET_IRQS fails. QEMU complains with :
> 
> qemu-system-x86_64: vfio 0000:08:00.0: Failed to set up TRIGGER eventfd signaling for interrupt INTX-0: VFIO_DEVICE_SET_IRQS failure: Invalid argument
> 
> To avoid that, unconditionaly clear the IRQ index when MSI-X is
> disabled.
> 
> Buglink: https://issues.redhat.com/browse/RHEL-21293
> Fixes: 5ebffa4e87e7 ("vfio/pci: use an invalid fd to enable MSI-X")
> Cc: Jing Liu <jing2.liu@intel.com>
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>

This seems to be a stable material too, for stable-8.2 series.
Please let me know if it is not.
Please Cc: qemu-stable@ for other stable-worthy changes.

Thanks,

/mjt

> ---
>   hw/vfio/pci.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index d7fe06715c4b9cde66a68c31aaf405315921b0d6..4fa387f0430d62ca2ba1b5ae5b7037f8f06b33f9 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -826,9 +826,11 @@ static void vfio_msix_disable(VFIOPCIDevice *vdev)
>           }
>       }
>   
> -    if (vdev->nr_vectors) {
> -        vfio_disable_irqindex(&vdev->vbasedev, VFIO_PCI_MSIX_IRQ_INDEX);
> -    }
> +    /*
> +     * Always clear MSI-X IRQ index. A PF device could have enabled
> +     * MSI-X with no vectors. See vfio_msix_enable().
> +     */
> +    vfio_disable_irqindex(&vdev->vbasedev, VFIO_PCI_MSIX_IRQ_INDEX);
>   
>       vfio_msi_disable_common(vdev);
>       vfio_intx_enable(vdev, &err);

Re: [PATCH] vfio/pci: Clear MSI-X IRQ index always

Posted by Michael Tokarev 2 years ago

29.01.2024 23:10, Michael Tokarev пишет:

> This seems to be a stable material too, for stable-8.2 series.
> Please let me know if it is not.
> Please Cc: qemu-stable@ for other stable-worthy changes.

Ahh, this is already Cc'ed to qemu-stable@, - please excuse me for the noise!

/mjt

Re: [PATCH] vfio/pci: Clear MSI-X IRQ index always

Posted by Cédric Le Goater 2 years ago

On 1/25/24 15:42, Cédric Le Goater wrote:
> When doing device assignment of a physical device, MSI-X can be
> enabled with no vectors enabled and this sets the IRQ index to
> VFIO_PCI_MSIX_IRQ_INDEX. However, when MSI-X is disabled, the IRQ
> index is left untouched if no vectors are in use. Then, when INTx
> is enabled, the IRQ index value is considered incompatible (set to
> MSI-X) and VFIO_DEVICE_SET_IRQS fails. QEMU complains with :
> 
> qemu-system-x86_64: vfio 0000:08:00.0: Failed to set up TRIGGER eventfd signaling for interrupt INTX-0: VFIO_DEVICE_SET_IRQS failure: Invalid argument
> 
> To avoid that, unconditionaly clear the IRQ index when MSI-X is
> disabled.


Applied to vfio-next.

Thanks,

C.

Re: [PATCH] vfio/pci: Clear MSI-X IRQ index always

Posted by Alex Williamson 2 years ago

On Thu, 25 Jan 2024 15:42:20 +0100
Cédric Le Goater <clg@redhat.com> wrote:

> When doing device assignment of a physical device, MSI-X can be
> enabled with no vectors enabled and this sets the IRQ index to
> VFIO_PCI_MSIX_IRQ_INDEX. However, when MSI-X is disabled, the IRQ
> index is left untouched if no vectors are in use. Then, when INTx
> is enabled, the IRQ index value is considered incompatible (set to
> MSI-X) and VFIO_DEVICE_SET_IRQS fails. QEMU complains with :
> 
> qemu-system-x86_64: vfio 0000:08:00.0: Failed to set up TRIGGER eventfd signaling for interrupt INTX-0: VFIO_DEVICE_SET_IRQS failure: Invalid argument
> 
> To avoid that, unconditionaly clear the IRQ index when MSI-X is
> disabled.
> 
> Buglink: https://issues.redhat.com/browse/RHEL-21293
> Fixes: 5ebffa4e87e7 ("vfio/pci: use an invalid fd to enable MSI-X")
> Cc: Jing Liu <jing2.liu@intel.com>
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>  hw/vfio/pci.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index d7fe06715c4b9cde66a68c31aaf405315921b0d6..4fa387f0430d62ca2ba1b5ae5b7037f8f06b33f9 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -826,9 +826,11 @@ static void vfio_msix_disable(VFIOPCIDevice *vdev)
>          }
>      }
>  
> -    if (vdev->nr_vectors) {
> -        vfio_disable_irqindex(&vdev->vbasedev, VFIO_PCI_MSIX_IRQ_INDEX);
> -    }
> +    /*
> +     * Always clear MSI-X IRQ index. A PF device could have enabled
> +     * MSI-X with no vectors. See vfio_msix_enable().
> +     */
> +    vfio_disable_irqindex(&vdev->vbasedev, VFIO_PCI_MSIX_IRQ_INDEX);
>  
>      vfio_msi_disable_common(vdev);
>      vfio_intx_enable(vdev, &err);

LGTM, at worst the SET_IRQS ioctl return an errno if we try to disable
an unconfigured interrupt index, so seems safe to call unconditionally
here.

Reviewed-by: Alex Williamson <alex.williamson@redhat.com>