Re: [PATCH] vfio/pci: Skip enabling INTx if the IRQ line is also unassgined

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

On Fri, 31 Jan 2025 17:15:01 +0000
Shivaprasad G Bhat <sbhat@linux.ibm.com> wrote:

> Currently, the PCI_INTERRUPT_PIN alone is checked before enabling
> the INTx. Its also necessary to have the IRQ Lines assigned for
> the INTx to work. So, check the PCI_INTERRUPT_LINE against 0xff
> indicates no connection.
> 
> The problem was observed on Power10 systems which primarily use
> MSI-X, and LSI lines are not connected on all devices under a
> PCIe switch. In this configuration where the PIN is non-zero
> but the LINE was 0xff, the VFIO_DEVICE_SET_IRQS was failing as
> it was trying to map the irqfd for the LSI of the device.
> 
> Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
> ---
>  hw/vfio/pci.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index ab17a98ee5..69a519d143 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -264,12 +264,12 @@ static void vfio_irqchip_change(Notifier *notify, void *data)
>  static bool vfio_intx_enable(VFIOPCIDevice *vdev, Error **errp)
>  {
>      uint8_t pin = vfio_pci_read_config(&vdev->pdev, PCI_INTERRUPT_PIN, 1);
> +    uint8_t line = vfio_pci_read_config(&vdev->pdev, PCI_INTERRUPT_LINE, 1);
>      Error *err = NULL;
>      int32_t fd;
>      int ret;
>  
> -
> -    if (!pin) {
> +    if (!pin || (line == 0xFF)) {
>          return true;
>      }
>  

vfio_intx_enable() should be the backup that catches users trying to
configure INTx, but we should also be indicating there's no INTx
through the IRQ_INFO ioctl.  The value in the line register is also not
defined by the PCI spec, it's implementation specific, therefore for
what architectures is this interpretation of the line register valid?

Should we at the same time virtualize the pin register as zero?

Maybe it's also time to implement similar checking for dev->irq ==
IRQ_NOTCONNECTED.  Do Power systems make use of the IRQ_NOTCONNECTED
convention?  Thanks,

Alex