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

[Tomita Moeko <tomitamoeko@gmail.com>]

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");
>