On 1/4/25 01:44, Alex Williamson wrote: > On Tue, 31 Dec 2024 00:10:54 +0800 > Tomita Moeko <tomitamoeko@xxxxxxxxx> 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@xxxxxxxxx> >> --- >> 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@xxxxxxxxx/ >> >> 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"); >
