On 4/23/25 14:54, Corvin Köhne wrote:
> On Tue, 2025-04-22 at 00:31 +0800, Tomita Moeko wrote:
>> CAUTION: External Email!!
>> There is currently no straightforward way to distinguish if a Intel
>> graphics device is IGD or discrete GPU. However, only IGD devices expose
>> OpRegion. Use the presence of VFIO_REGION_SUBTYPE_INTEL_IGD_OPREGION
>> to identify IGD devices.
>>
>> Signed-off-by: Tomita Moeko <[email protected]>
>> ---
>> hw/vfio/igd.c | 26 ++++++++++++++++++--------
>> 1 file changed, 18 insertions(+), 8 deletions(-)
>>
>> diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
>> index 36316e50ea..7a7c7735c1 100644
>> --- a/hw/vfio/igd.c
>> +++ b/hw/vfio/igd.c
>> @@ -479,6 +479,7 @@ void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int
>> nr)
>>
>> static bool vfio_pci_igd_config_quirk(VFIOPCIDevice *vdev, Error **errp)
>> {
>> + g_autofree struct vfio_region_info *opregion = NULL;
>> int ret, gen;
>> uint64_t gms_size;
>> uint64_t *bdsm_size;
>> @@ -486,16 +487,20 @@ 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.
>> - */
>
> Why do you remove this comment? Yes, the comment is not correct. Some OS
> driver
> and the UEFI GOP depend on address 00:02.0 too. Wouldn't it be better to
> improve
> the comment instead of removing it? This restriction looks a bit odd and IMO a
> comment would help future reader to understand it easier.
The restriction is documented in doc/igd-assign.txt, keeping the comment here
seems misleading IMO. That's why I decided to remove it here.
>> if (!vfio_pci_is(vdev, PCI_VENDOR_ID_INTEL, PCI_ANY_ID) ||
>> !vfio_is_vga(vdev)) {
>> return true;
>> }
>>
>> + /* IGD device always comes with OpRegion */
>> + ret = vfio_device_get_region_info_type(&vdev->vbasedev,
>> + VFIO_REGION_TYPE_PCI_VENDOR_TYPE | PCI_VENDOR_ID_INTEL,
>> + VFIO_REGION_SUBTYPE_INTEL_IGD_OPREGION, &opregion);
>> + if (ret) {
>> + return true;
>> + }
>> + info_report("OpRegion detected on Intel display %x.", vdev->device_id);
>> +
>> /*
>> * IGD is not a standard, they like to change their specs often. We
>> * only attempt to support back to SandBridge and we hope that newer
>> @@ -570,9 +575,14 @@ static bool vfio_pci_igd_config_quirk(VFIOPCIDevice
>> *vdev, Error **errp)
>> }
>>
>> /* Setup OpRegion access */
>> - if ((vdev->features & VFIO_FEATURE_ENABLE_IGD_OPREGION) &&
>> - !vfio_pci_igd_setup_opregion(vdev, errp)) {
>> - goto error;
>> + if ((vdev->features & VFIO_FEATURE_ENABLE_IGD_OPREGION)) {
>> + if (vdev->pdev.qdev.hotplugged) {
>> + error_setg(errp, "OpRegion is not supported on hotplugged
>> device");
>> + goto error;
>> + }
>> + if (!vfio_pci_igd_opregion_init(vdev, opregion, errp)) {
>> + goto error;
>> + }
>
> How is this part related to "Detect IGD device by OpRegion"?
As Alex suggested, this part will be refactored in v2.
Thanks,
Moeko
>> }
>>
>> /* Setup LPC bridge / Host bridge PCI IDs */
>
