[PATCH v2] vfio/pci: update igd matching conditions

[PATCH v2] vfio/pci: update igd matching conditions

[Tomita Moeko]

igd device can either expose as a VGA controller or display controller
depending on whether it is configured as the primary display device in
BIOS. In both cases, the OpRegion may be present. Also checks if the
device is at bdf 00:02.0 to avoid setting up igd-specific regions on
Intel discrete GPUs.

Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
---
Changelog:
v2:
Fix misuse of pci_get_domain_bus_and_slot(), now only compares bdf
without touching device reference count.
Link: https://lore.kernel.org/all/20241229155140.7434-1-tomitamoeko@gmail.com/

 drivers/vfio/pci/vfio_pci.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index e727941f589d..906a1db46d15 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -111,9 +111,11 @@ static int vfio_pci_open_device(struct vfio_device *core_vdev)
 	if (ret)
 		return ret;
 
-	if (vfio_pci_is_vga(pdev) &&
-	    pdev->vendor == PCI_VENDOR_ID_INTEL &&
-	    IS_ENABLED(CONFIG_VFIO_PCI_IGD)) {
+	if (IS_ENABLED(CONFIG_VFIO_PCI_IGD) &&
+	    (pdev->vendor == PCI_VENDOR_ID_INTEL) &&
+	    (((pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA) ||
+	     ((pdev->class >> 8) == PCI_CLASS_DISPLAY_OTHER)) &&
+	    (pci_dev_id(pdev) == PCI_DEVID(0, PCI_DEVFN(2, 0)))) {
 		ret = vfio_pci_igd_init(vdev);
 		if (ret && ret != -ENODEV) {
 			pci_warn(pdev, "Failed to setup Intel IGD regions\n");
-- 
2.45.2

Re: [PATCH v2] vfio/pci: update igd matching conditions

[Alex Williamson]

On Tue, 31 Dec 2024 00:10:54 +0800
Tomita Moeko <tomitamoeko@gmail.com> wrote:

> igd device can either expose as a VGA controller or display controller
> depending on whether it is configured as the primary display device in
> BIOS. In both cases, the OpRegion may be present. Also checks if the
> device is at bdf 00:02.0 to avoid setting up igd-specific regions on
> Intel discrete GPUs.
> 
> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
> ---
> Changelog:
> v2:
> Fix misuse of pci_get_domain_bus_and_slot(), now only compares bdf
> without touching device reference count.
> Link: https://lore.kernel.org/all/20241229155140.7434-1-tomitamoeko@gmail.com/
> 
>  drivers/vfio/pci/vfio_pci.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index e727941f589d..906a1db46d15 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -111,9 +111,11 @@ static int vfio_pci_open_device(struct vfio_device *core_vdev)
>  	if (ret)
>  		return ret;
>  
> -	if (vfio_pci_is_vga(pdev) &&
> -	    pdev->vendor == PCI_VENDOR_ID_INTEL &&
> -	    IS_ENABLED(CONFIG_VFIO_PCI_IGD)) {
> +	if (IS_ENABLED(CONFIG_VFIO_PCI_IGD) &&
> +	    (pdev->vendor == PCI_VENDOR_ID_INTEL) &&
> +	    (((pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA) ||
> +	     ((pdev->class >> 8) == PCI_CLASS_DISPLAY_OTHER)) &&
> +	    (pci_dev_id(pdev) == PCI_DEVID(0, PCI_DEVFN(2, 0)))) {

Sorry I wasn't available to reply on previous thread before v2 was
posted, but given that we have vfio_pci_is_vga() we should use it
rather than duplicate the contents.  I think that suggests we should
create a similar helper for display_other.  Alternatively we should
maybe consider if it's sufficient to use just the base class.

The DEVID of course does not include the domain, which make it a rather
suspect check already.  What do the discrete cards report at 0xfc in
config space?  If it's zero or -1 or points to something that we can't
memremap() or points to contents that doesn't include the opregion
signature, then we'll already exit out of vfio_pci_igd_init().  Is
there actually a case that we're actually configuring IGD specific
regions for a discrete card?  Thanks,

Alex

>  		ret = vfio_pci_igd_init(vdev);
>  		if (ret && ret != -ENODEV) {
>  			pci_warn(pdev, "Failed to setup Intel IGD regions\n");

Re: [PATCH v2] vfio/pci: update igd matching conditions

[Tomita Moeko]

On 1/4/25 01:44, Alex Williamson wrote:
> On Tue, 31 Dec 2024 00:10:54 +0800
> Tomita Moeko <tomitamoeko@gmail.com> wrote:
> 
>> igd device can either expose as a VGA controller or display controller
>> depending on whether it is configured as the primary display device in
>> BIOS. In both cases, the OpRegion may be present. Also checks if the
>> device is at bdf 00:02.0 to avoid setting up igd-specific regions on
>> Intel discrete GPUs.
>>
>> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
>> ---
>> Changelog:
>> v2:
>> Fix misuse of pci_get_domain_bus_and_slot(), now only compares bdf
>> without touching device reference count.
>> Link: https://lore.kernel.org/all/20241229155140.7434-1-tomitamoeko@gmail.com/
>>
>>  drivers/vfio/pci/vfio_pci.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
>> index e727941f589d..906a1db46d15 100644
>> --- a/drivers/vfio/pci/vfio_pci.c
>> +++ b/drivers/vfio/pci/vfio_pci.c
>> @@ -111,9 +111,11 @@ static int vfio_pci_open_device(struct vfio_device *core_vdev)
>>  	if (ret)
>>  		return ret;
>>  
>> -	if (vfio_pci_is_vga(pdev) &&
>> -	    pdev->vendor == PCI_VENDOR_ID_INTEL &&
>> -	    IS_ENABLED(CONFIG_VFIO_PCI_IGD)) {
>> +	if (IS_ENABLED(CONFIG_VFIO_PCI_IGD) &&
>> +	    (pdev->vendor == PCI_VENDOR_ID_INTEL) &&
>> +	    (((pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA) ||
>> +	     ((pdev->class >> 8) == PCI_CLASS_DISPLAY_OTHER)) &&
>> +	    (pci_dev_id(pdev) == PCI_DEVID(0, PCI_DEVFN(2, 0)))) {
> 
> Sorry I wasn't available to reply on previous thread before v2 was
> posted, but given that we have vfio_pci_is_vga() we should use it
> rather than duplicate the contents.  I think that suggests we should
> create a similar helper for display_other.  Alternatively we should
> maybe consider if it's sufficient to use just the base class.

I think using the base class is okay here. AFAIK intel doesn't has
any devices reported as XGA or 3D controller.

> The DEVID of course does not include the domain, which make it a rather
> suspect check already.  What do the discrete cards report at 0xfc in
> config space?  If it's zero or -1 or points to something that we can't
> memremap() or points to contents that doesn't include the opregion
> signature, then we'll already exit out of vfio_pci_igd_init().  Is
> there actually a case that we're actually configuring IGD specific
> regions for a discrete card?  Thanks,
>
> Alex

Checking (pci_domain_nr(pdev->bus) == 0) seems okay. I tried on a
discrate Arc A770, there is 0 at 0xFC, so vfio_pci_igd_init() returns
with -NODEV and the device is skipped. Shall I remove the BDF check?
It seems to be unnecessary, my intention is to ensure it is really an
igd since it can only be at 0000:00:02.0.
 
>>  		ret = vfio_pci_igd_init(vdev);
>>  		if (ret && ret != -ENODEV) {
>>  			pci_warn(pdev, "Failed to setup Intel IGD regions\n");
>