Re: [PATCH] vfio/igd: Check host PCI address when probing

[Tomita Moeko <tomitamoeko@gmail.com>]

On 4/10/25 15:34, Cédric Le Goater wrote:
> + Corvin
> 
> On 4/9/25 19:18, Alex Williamson wrote:
>> On Wed, 26 Mar 2025 01:22:39 +0800
>> Tomita Moeko <tomitamoeko@gmail.com> wrote:
>>
>>> So far, all Intel VGA adapters, including discrete GPUs like A770 and
>>> B580, were treated as IGD devices. While this had no functional impact,
>>> a error about "unsupported IGD device" will be printed when passthrough
>>> Intel discrete GPUs.
>>>
>>> Since IGD devices must be at "0000:00:02.0", let's check the host PCI
>>> address when probing.
>>>
>>> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
>>> ---
>>>   hw/vfio/igd.c | 23 +++++++++--------------
>>>   1 file changed, 9 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
>>> index 265fffc2aa..ff250017b0 100644
>>> --- a/hw/vfio/igd.c
>>> +++ b/hw/vfio/igd.c
>>> @@ -53,6 +53,13 @@
>>>    * headless setup is desired, the OpRegion gets in the way of that.
>>>    */
>>>   +static bool vfio_is_igd(VFIOPCIDevice *vdev)
>>> +{
>>> +    return vfio_pci_is(vdev, PCI_VENDOR_ID_INTEL, PCI_ANY_ID) &&
>>> +           vfio_is_vga(vdev) &&
>>> +           vfio_pci_host_match(&vdev->host, "0000:00:02.0");
>>> +}
>>
>> vfio-pci devices can also be specified via sysfsdev= rather than host=,
>> so at a minimum I think we'd need to test against vdev->vbasedev.name,
>> as other callers of vfio_pci_host_match do.  For example building a
>> local PCIHostDeviceAddress and comparing it to name.  This is also not
>> foolproof though if we start taking advantage of devices passed by fd.
>>
>> Could we instead rely PCIe capabilities?  A discrete GPU should
>> identify as either an endpoint or legacy endpoint and IGD should
>> identify as a root complex integrated endpoint, or maybe older versions
>> would lack the PCIe capability altogether.
> 
> Maintaining a list of PCI IDs for Intel GPU devices as Corvin was
> proposing in [1] is not a viable solution ?
> 
> Thanks,
> 
> C.
> 
> [1] https://lore.kernel.org/qemu-devel/20250206121341.118337-1-corvin.koehne@gmail.com/

I checked Intel doc, probably maintaining an device ID list is the only
possible way. But given that intel is moving to xe driver, generation
becomes unclear, I'd like to propose a list with quirk flags for igd.

static const struct igd_device igd_devices[] = {
    INTEL_SNB_IDS(IGD_DEVICE, OPREGION_QUIRK | BDSM_QUIRK),
    INTEL_TGL_IDS(IGD_DEVICE, OPREGION_QUIRK | BDSM64_QUIRK),
}

Matching in the list is more time consuming than current switch-case,
it's better to have a new field to cache it.

I will go with Corvin's first 2 patches with reordering suggested by
Cornelia.

Thanks,
Moeko
  
>> Also I think the comments that were dropped below are still valid and
>> useful to transfer to this new helper.  I think those are actually
>> referring to the guest address of 00:02.0 though, which should maybe be
>> a test as well.  Thanks,
>>
>> Alex
>>
>>> +
>>>   /*
>>>    * This presumes the device is already known to be an Intel VGA device, so we
>>>    * take liberties in which device ID bits match which generation.  This should
>>> @@ -427,13 +434,7 @@ void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int nr)
>>>       VFIOConfigMirrorQuirk *ggc_mirror, *bdsm_mirror;
>>>       int gen;
>>>   -    /*
>>> -     * This must be an Intel VGA device at address 00:02.0 for us to even
>>> -     * consider enabling legacy mode. Some driver have dependencies on the PCI
>>> -     * bus address.
>>> -     */
>>> -    if (!vfio_pci_is(vdev, PCI_VENDOR_ID_INTEL, PCI_ANY_ID) ||
>>> -        !vfio_is_vga(vdev) || nr != 0) {
>>> +    if (nr != 0 || !vfio_is_igd(vdev)) {
>>>           return;
>>>       }
>>>   @@ -490,13 +491,7 @@ static bool vfio_pci_igd_config_quirk(VFIOPCIDevice *vdev, Error **errp)
>>>       bool legacy_mode_enabled = false;
>>>       Error *err = NULL;
>>>   -    /*
>>> -     * This must be an Intel VGA device at address 00:02.0 for us to even
>>> -     * consider enabling legacy mode.  The vBIOS has dependencies on the
>>> -     * PCI bus address.
>>> -     */
>>> -    if (!vfio_pci_is(vdev, PCI_VENDOR_ID_INTEL, PCI_ANY_ID) ||
>>> -        !vfio_is_vga(vdev)) {
>>> +    if (!vfio_is_igd(vdev)) {
>>>           return true;
>>>       }
>>>   
>>
>