On 5/21/25 04:46, edmund.raile wrote:
> Hi Moeko,
>> On 5/19/25 16:03, edmund.raile wrote:
>>> Restore SR-IOV Intel iGPU VF passthrough capability:
>>> Check x-igd-opregion=off parameter in vfio_pci_igd_config_quirk and
>>> vfio_pci_kvmgt_config_quirk to ensure x-igd-opregion=off is
>>> respected despite subsequent attempt of automatic
>>> IGD opregion detection.
>>>
>>> Fixes: c0273e77f2d7 ("vfio/igd: Detect IGD device by OpRegion")
>>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2968
>>> Signed-off-by: Edmund Raile <edmund.ra...@protonmail.com>
>>> ---
>>> This patch fixes a regression in QEMU’s VFIO IGD quirk handling that
>>> established automatic IGD opregion detection which ignores
>>> x-igd-opregion=off necessary for SR-IOV VF passthrough of
>>> Intel iGPUs using i915-sriov-dkms.
>>>
>>> Please review and provide feedback.
>>> Let me know if additional testing or changes are needed.
>>>
>>> Kind regards,
>>> Edmund Raile.
>>
>> Hi, Edmund
>>
>> I did a quick test with x-igd-opregion=off with c0273e77f2d7 included on
>> SRIOV PF, setting x-igd-opregion=off works as expected on my linux
>> guest. Per my understanding, SRIOV PF should not have OpRegion address
>> in its config space 0xfc, kernel returns -ENODEV when accessing, and
>> QEMU continues after vfio_pci_igd_opregion_detect() fails by returing
>> true. Could you please share more details about this issue?
>
> This is with the i915-sriov-dkms module creating a virtual function
> from my iGPU:
> 00:02.0 VGA compatible controller: Intel Corporation AlderLake-S GT1 (rev 0c)
> 00:02.1 VGA compatible controller: Intel Corporation AlderLake-S GT1 (rev 0c)
> Which I pass through to the VM, allowing both the host and the guest
> to use the same iGPU.
>
>>
>> [ 0.655035] i915 0000:00:02.0: [drm:intel_opregion_setup [i915]] graphic
>> opregion physical addr: 0x0
>> [ 0.655490] i915 0000:00:02.0: [drm:intel_opregion_setup [i915]] ACPI
>> OpRegion not supported!
>> [ 0.656088] i915 0000:00:02.0: Invalid PCI ROM header signature:
>> expecting 0xaa55, got 0x0000
>> [ 0.656462] i915 0000:00:02.0: [drm] Failed to find VBIOS tables (VBT)
>
> Where are you getting these logs from?
> These aren't kernel messages, are they?
Hi, Edmund
This is get printed by guest kernel, with `drm.debug=0x2` in cmdline.
>> If you are mentioning the log "Device does not supports IGD OpRegion
>> feature", it is a false error and can be ignored I think. Maybe we can
>> improve this, making it more clear?
>
> The exact error I am getting without my patch is this:
> qemu-system-x86_64: -device
> vfio-pci,host=0000:00:02.1,romfile=/path/to/intelgopdriver_desktop.bin,x-vga=on,x-igd-opregion=off:
> Device does not supports IGD OpRegion feature: No such device
> after which QEMU immediately exits.
> Maybe the issue is that the parrent device (the iGPU) DOES support
> OpRegion but the virtual function child does not.
> The intent is to give the option of disabling it back to the user.
I did some further debugging, and found it was caused by a mistake in
error handling. In vfio_pci_igd_opregion_detect(), `errp` is set when
OpRegion is not found. However, in pci_qdev_realize(), the caller of
vfio_realize(), it checks if the errp is set for failure, rather than
the return value.
hw/pci/pci.c:2268
> if (pc->realize) {
> pc->realize(pci_dev, &local_err);
> if (local_err) {
> error_propagate(errp, local_err);
> do_pci_unregister_device(pci_dev);
> return;
> }
> }
I will submit a patch later to fix error handling.
>>> hw/vfio/igd.c | 10 ++++++++++
>>> 1 file changed, 10 insertions(+)
>>>
>>> diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
>>> index e7952d15a0..e54a2a2f00 100644
>>> --- a/hw/vfio/igd.c
>>> +++ b/hw/vfio/igd.c
>>> @@ -523,6 +523,11 @@ static bool vfio_pci_igd_config_quirk(VFIOPCIDevice
>>> *vdev, Error **errp)
>>> return true;
>>> }
>>>
>>> + /* Respect x-igd-opregion=off by skipping OpRegion handling */
>>> + if (!vdev->igd_opregion) {
>>> + return true;
>>> + }
>>> +
>>
>> Here (vdev->igd_opregion == NULL) is always true, the pointer is zero-
>> initialized and only get assigned in vfio_pci_igd_opregion_init().
>> Enabling OpRegion or not is by VFIO_FEATURE_ENABLE_IGD_OPREGION bit.
>>
>
> I thought `vdev->igd_opregion` would already contain the
> `x-igd-opregion=` option state of the `-device vfio-pci`
> parameter but you're right, it's not assigned here yet,
> so is this the correct way to access the state of the
> parameter option:
> (vdev->features & VFIO_FEATURE_ENABLE_IGD_OPREGION)
> Which would then make it
> + if (!(vdev->features & VFIO_FEATURE_ENABLE_IGD_OPREGION)) {
> + return true;
> + }
Yes that's right.
Thanks,
Moeko
>>> /* IGD device always comes with OpRegion */
>>> if (!vfio_pci_igd_opregion_detect(vdev, &opregion, errp)) {
>>> return true;
>>> @@ -689,6 +694,11 @@ static bool vfio_pci_kvmgt_config_quirk(VFIOPCIDevice
>>> *vdev, Error **errp)
>>> return true;
>>> }
>>>
>>> + /* Respect x-igd-opregion=off by skipping OpRegion handling */
>>> + if (!vdev->igd_opregion) {
>>> + return true;
>>> + }
>>> +
>>
>> As I mentioned in a comment below, GVT-g vGPU always emulate OpRegion,
>> so let QEMU fail immediately if OpRegion is not avaliable here.
>
> Agreed, you are right, it shouldn't be necessary here,
> I'll remove it in v3.
>
>>
>> Thanks,
>> Moeko
>
> Thank you for taking the time to review (and contribute in the
> first place).
>
> Kind regards,
> Edmund.
>
