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

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

[Shivaprasad G Bhat]

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

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

[Alex Williamson]

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

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

[Shivaprasad G Bhat]

Hi Alex,

Thanks and Sorry about the delay in responding. I had to figure out
many nuances for answering your questions.

Replies inline..

On 2/1/25 3:32 AM, Alex Williamson wrote:
> 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.
I posted the kernel fix for this here [1]
> 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?

You are right, I figured the LINE value is not really dependable even within

different PPC machines.

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

I verified atleast on PPC it doesn't help as the device tree properties

are given precedence over the PIN value during probe.  Trying to see

how to handle this, wonder at the same time if this wont have any impact

on other architectures.

> 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,

I see the IRQ_NOTCONNECTED convention is not followed by PPC and

hard to adapt now with many arch specific drivers defaulting on irq = 0

for the same. So, my kernel fix[1] is checking for irq = 0 only for PPC and

not IRQ_NOTCONNECTED.

I have posted my v2 here [2] with the IRQ_INFO check as suggested.

References:

[1] : 
https://lore.kernel.org/all/174231895238.2295.12586708771396482526.stgit@linux.ibm.com/ 


[2] : 174232032506.3739.465958546360660842.stgit@linux.ibm.com

Thanks and Regards,

Shiva