[PATCH] vfio: pci: Advertise INTx only if LINE is connected

[PATCH] vfio: pci: Advertise INTx only if LINE is connected

[Shivaprasad G Bhat]

On POWER systems, when the device is behind the io expander,
not all PCI slots would have the PCI_INTERRUPT_LINE connected.
The firmware assigns a valid PCI_INTERRUPT_PIN though. In such
configuration, the irq_info ioctl currently advertizes the
irq count as 1 as the PCI_INTERRUPT_PIN is valid.

The patch adds the additional check[1] if the irq is assigned
for the PIN which is done iff the LINE is connected.

[1]: https://lore.kernel.org/qemu-devel/20250131150201.048aa3bf.alex.williamson@redhat.com/

Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
Suggested-By: Alex Williamson <alex.williamson@redhat.com>
---
 drivers/vfio/pci/vfio_pci_core.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
index 586e49efb81b..4ce70f05b4a8 100644
--- a/drivers/vfio/pci/vfio_pci_core.c
+++ b/drivers/vfio/pci/vfio_pci_core.c
@@ -734,6 +734,10 @@ static int vfio_pci_get_irq_count(struct vfio_pci_core_device *vdev, int irq_typ
 			return 0;
 
 		pci_read_config_byte(vdev->pdev, PCI_INTERRUPT_PIN, &pin);
+#if IS_ENABLED(CONFIG_PPC64)
+		if (!vdev->pdev->irq)
+			pin = 0;
+#endif
 
 		return pin ? 1 : 0;
 	} else if (irq_type == VFIO_PCI_MSI_IRQ_INDEX) {

Re: [PATCH] vfio: pci: Advertise INTx only if LINE is connected

[Alex Williamson]

On Tue, 18 Mar 2025 17:29:21 +0000
Shivaprasad G Bhat <sbhat@linux.ibm.com> wrote:

> On POWER systems, when the device is behind the io expander,
> not all PCI slots would have the PCI_INTERRUPT_LINE connected.
> The firmware assigns a valid PCI_INTERRUPT_PIN though. In such
> configuration, the irq_info ioctl currently advertizes the
> irq count as 1 as the PCI_INTERRUPT_PIN is valid.
> 
> The patch adds the additional check[1] if the irq is assigned
> for the PIN which is done iff the LINE is connected.
> 
> [1]: https://lore.kernel.org/qemu-devel/20250131150201.048aa3bf.alex.williamson@redhat.com/
> 
> Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
> Suggested-By: Alex Williamson <alex.williamson@redhat.com>
> ---
>  drivers/vfio/pci/vfio_pci_core.c |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index 586e49efb81b..4ce70f05b4a8 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -734,6 +734,10 @@ static int vfio_pci_get_irq_count(struct vfio_pci_core_device *vdev, int irq_typ
>  			return 0;
>  
>  		pci_read_config_byte(vdev->pdev, PCI_INTERRUPT_PIN, &pin);
> +#if IS_ENABLED(CONFIG_PPC64)
> +		if (!vdev->pdev->irq)
> +			pin = 0;
> +#endif
>  
>  		return pin ? 1 : 0;
>  	} else if (irq_type == VFIO_PCI_MSI_IRQ_INDEX) {
> 
> 

See:

https://lore.kernel.org/all/20250311230623.1264283-1-alex.williamson@redhat.com/

Do we need to expand that to test !vdev->pdev->irq in
vfio_config_init()?  We don't allow a zero irq to be enabled in
vfio_intx_enable(), so we might as well not report it as supported.  I
don't see why any of this needs to be POWER specific.  Thanks,

Alex

Re: [PATCH] vfio: pci: Advertise INTx only if LINE is connected

[Shivaprasad G Bhat]

On 3/18/25 11:28 PM, Alex Williamson wrote:
> On Tue, 18 Mar 2025 17:29:21 +0000
> Shivaprasad G Bhat <sbhat@linux.ibm.com> wrote:
>
>> On POWER systems, when the device is behind the io expander,
>> not all PCI slots would have the PCI_INTERRUPT_LINE connected.
>> The firmware assigns a valid PCI_INTERRUPT_PIN though. In such
>> configuration, the irq_info ioctl currently advertizes the
>> irq count as 1 as the PCI_INTERRUPT_PIN is valid.
>>
>> The patch adds the additional check[1] if the irq is assigned
>> for the PIN which is done iff the LINE is connected.
>>
>> [1]: https://lore.kernel.org/qemu-devel/20250131150201.048aa3bf.alex.williamson@redhat.com/
>>
>> Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
>> Suggested-By: Alex Williamson <alex.williamson@redhat.com>
>> ---
>>   drivers/vfio/pci/vfio_pci_core.c |    4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
>> index 586e49efb81b..4ce70f05b4a8 100644
>> --- a/drivers/vfio/pci/vfio_pci_core.c
>> +++ b/drivers/vfio/pci/vfio_pci_core.c
>> @@ -734,6 +734,10 @@ static int vfio_pci_get_irq_count(struct vfio_pci_core_device *vdev, int irq_typ
>>   			return 0;
>>   
>>   		pci_read_config_byte(vdev->pdev, PCI_INTERRUPT_PIN, &pin);
>> +#if IS_ENABLED(CONFIG_PPC64)
>> +		if (!vdev->pdev->irq)
>> +			pin = 0;
>> +#endif
>>   
>>   		return pin ? 1 : 0;
>>   	} else if (irq_type == VFIO_PCI_MSI_IRQ_INDEX) {
>>
>>
> See:
>
> https://lore.kernel.org/all/20250311230623.1264283-1-alex.williamson@redhat.com/
>
> Do we need to expand that to test !vdev->pdev->irq in
> vfio_config_init()?

Yes. Looks to be the better option. I did try this and it works.


I see your patch has already got Reviewed-by. Are you planning

for v2 Or want me to post a separate patch with this new check?


> We don't allow a zero irq to be enabled in
> vfio_intx_enable(), so we might as well not report it as supported.

Yes. I agree.


Thank you!


Regards,

Shivaprasad

Re: [PATCH] vfio: pci: Advertise INTx only if LINE is connected

[Alex Williamson]

On Thu, 20 Mar 2025 23:24:49 +0530
Shivaprasad G Bhat <sbhat@linux.ibm.com> wrote:

> On 3/18/25 11:28 PM, Alex Williamson wrote:
> > On Tue, 18 Mar 2025 17:29:21 +0000
> > Shivaprasad G Bhat <sbhat@linux.ibm.com> wrote:
> >  
> >> On POWER systems, when the device is behind the io expander,
> >> not all PCI slots would have the PCI_INTERRUPT_LINE connected.
> >> The firmware assigns a valid PCI_INTERRUPT_PIN though. In such
> >> configuration, the irq_info ioctl currently advertizes the
> >> irq count as 1 as the PCI_INTERRUPT_PIN is valid.
> >>
> >> The patch adds the additional check[1] if the irq is assigned
> >> for the PIN which is done iff the LINE is connected.
> >>
> >> [1]: https://lore.kernel.org/qemu-devel/20250131150201.048aa3bf.alex.williamson@redhat.com/
> >>
> >> Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
> >> Suggested-By: Alex Williamson <alex.williamson@redhat.com>
> >> ---
> >>   drivers/vfio/pci/vfio_pci_core.c |    4 ++++
> >>   1 file changed, 4 insertions(+)
> >>
> >> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> >> index 586e49efb81b..4ce70f05b4a8 100644
> >> --- a/drivers/vfio/pci/vfio_pci_core.c
> >> +++ b/drivers/vfio/pci/vfio_pci_core.c
> >> @@ -734,6 +734,10 @@ static int vfio_pci_get_irq_count(struct vfio_pci_core_device *vdev, int irq_typ
> >>   			return 0;
> >>   
> >>   		pci_read_config_byte(vdev->pdev, PCI_INTERRUPT_PIN, &pin);
> >> +#if IS_ENABLED(CONFIG_PPC64)
> >> +		if (!vdev->pdev->irq)
> >> +			pin = 0;
> >> +#endif
> >>   
> >>   		return pin ? 1 : 0;
> >>   	} else if (irq_type == VFIO_PCI_MSI_IRQ_INDEX) {
> >>
> >>  
> > See:
> >
> > https://lore.kernel.org/all/20250311230623.1264283-1-alex.williamson@redhat.com/
> >
> > Do we need to expand that to test !vdev->pdev->irq in
> > vfio_config_init()?  
> 
> Yes. Looks to be the better option. I did try this and it works.
> 
> 
> I see your patch has already got Reviewed-by. Are you planning
> 
> for v2 Or want me to post a separate patch with this new check?

It seems worth noting this as an additional vector for virtualizing the
PIN register since we'd often expect the PIN is already zero if
pdev->irq is zero.  I posted a patch[1], please review/test.  Thanks,

Alex

[1]https://lore.kernel.org/all/20250320194145.2816379-1-alex.williamson@redhat.com/

Re: [PATCH] vfio: pci: Advertise INTx only if LINE is connected

[Christophe Leroy]

Le 18/03/2025 à 18:29, Shivaprasad G Bhat a écrit :
> On POWER systems, when the device is behind the io expander,
> not all PCI slots would have the PCI_INTERRUPT_LINE connected.
> The firmware assigns a valid PCI_INTERRUPT_PIN though. In such
> configuration, the irq_info ioctl currently advertizes the
> irq count as 1 as the PCI_INTERRUPT_PIN is valid.
> 
> The patch adds the additional check[1] if the irq is assigned
> for the PIN which is done iff the LINE is connected.
> 
> [1]: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fqemu-devel%2F20250131150201.048aa3bf.alex.williamson%40redhat.com%2F&data=05%7C02%7Cchristophe.leroy2%40cs-soprasteria.com%7Ce0fb1d4bf2064e115ce408dd6642796b%7C8b87af7d86474dc78df45f69a2011bb5%7C0%7C0%7C638779157886704638%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=egZuT5CZsC6S%2Bd7bZTuO4RcKL8IJREPbxIMGZZkZeMQ%3D&reserved=0
> 
> Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
> Suggested-By: Alex Williamson <alex.williamson@redhat.com>
> ---
>   drivers/vfio/pci/vfio_pci_core.c |    4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index 586e49efb81b..4ce70f05b4a8 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -734,6 +734,10 @@ static int vfio_pci_get_irq_count(struct vfio_pci_core_device *vdev, int irq_typ
>   			return 0;
>   
>   		pci_read_config_byte(vdev->pdev, PCI_INTERRUPT_PIN, &pin);
> +#if IS_ENABLED(CONFIG_PPC64)
> +		if (!vdev->pdev->irq)
> +			pin = 0;
> +#endif

I see no reason for #ifdef here, please instead do:

	if (IS_ENABLED(CONFIG_PPC64) && !vdev->pdev->irq)

See 
https://docs.kernel.org/process/coding-style.html#conditional-compilation

>   
>   		return pin ? 1 : 0;
>   	} else if (irq_type == VFIO_PCI_MSI_IRQ_INDEX) {
> 
> 
>