[PATCH v3 00/10] vfio/igd: Decoupling quirks with legacy mode

[PATCH v3 00/10] vfio/igd: Decoupling quirks with legacy mode

[Tomita Moeko]

This patchset intends to decouple existing quirks from legacy mode.
Currently all quirks depends on legacy mode (except x-igd-opregion),
which includes following conditions:
* Machine type is i440fx
* IGD device is at guest BDF 00:02.0
* VBIOS in ROM BAR or file
* VGA IO/MMIO ranges are claimed by IGD
* OpRegion
* Same LPC bridge and Host bridge VID/DID/SVID/SSID as host

If one of the condition is not met, the quirks will not be applied.
However, for recent generations, espcially Gen 11+ devices that removed
VBIOS support, not all the conditions are required. For example, on EFI-
based systems, VBIOS ROM is unnecessary, and VGA ranges are not used.

To have better support on newer UEFI-based VMs, this patchset makes the
quirks independent of legacy mode. The BDSM and GGC register quirks are
applied to all supported IGD devices, new x-igd-lpc option for the LPC
bridge / Host bridge ID quirk is introduced for possible Q35 support.
It also prepares for supporting IGD passthrough when it is not primary
display later (kernel change will be merged in 6.15).

To maintain backward compatbility with exising configuration, legacy
mode will automatically be enabled when:
* Machine type is i440fx
* IGD device is at guest BDF 00:02.0
If the legacy mode behavior is unwanted, option x-igd-legacy-mode=off
is provided for users to disable it. Setting x-igd-legacy-mode=on checks
if the condition above are met, and set up all the required quirks.

The default value is x-igd-legacy-mode=auto. When the above conditions
are all met, it acts as before and continues on any error. Otherwise it
is equivalent to x-igd-legacy-mode=off.

When x-igd-legacy-mode is set to on or off, QEMU will fail immediately
on error.

The first 2 patches of this patchset was taken from a previous one,
details can be found at:
https://lore.kernel.org/all/20250124191245.12464-1-tomitamoeko@gmail.com/

This patchest was mainly tested on Alder Lake UHD770, with Debian 12
(kernel 6.1), Windows 11 (driver 32.0.101.6458) and Intel GOP driver
17.0.1081.

Due to I caught COVID these days, I am unable to deliver the proposed
documentation update before QEMU 10.0 soft-freeze next week. Please help
decide whether to make this patch series in 10.0 or next release.

Changelog:
v3:
* Use OnOffAuto for x-igd-legacy-mode option, default is auto, to keep
  current behavior.
* Added a new patch to solve the possible KVMGT/GVT-g fail.
Link: https://lore.kernel.org/all/20250303175220.74917-1-tomitamoeko@gmail.com/

v2:
* Keep legacy mode for compatbility
* Renamed from "vfio/igd: Remove legacy mode"
Link: https://lore.kernel.org/all/20250224182927.31519-1-tomitamoeko@gmail.com/

Tomita Moeko (10):
  vfio/igd: Remove GTT write quirk in IO BAR 4
  vfio/igd: Do not include GTT stolen size in etc/igd-bdsm-size
  vfio/igd: Consolidate OpRegion initialization into a single function
  vfio/igd: Move LPC bridge initialization to a separate function
  vfio/pci: Add placeholder for device-specific config space quirks
  vfio/igd: Refactor vfio_probe_igd_bar4_quirk into pci config quirk
  vfio/igd: Decouple common quirks from legacy mode
  vfio/igd: Handle x-igd-opregion option in config quirk
  vfio/igd: Introduce x-igd-lpc option for LPC bridge ID quirk
  vfio/igd: Fix broken KVMGT OpRegion support

 hw/vfio/igd.c        | 529 +++++++++++++++++++------------------------
 hw/vfio/pci-quirks.c |  51 +----
 hw/vfio/pci.c        |  33 +--
 hw/vfio/pci.h        |  11 +-
 4 files changed, 248 insertions(+), 376 deletions(-)

-- 
2.47.2

[PATCH v3 01/10] vfio/igd: Remove GTT write quirk in IO BAR 4

[Tomita Moeko]

The IO BAR4 of IGD devices contains a pair of 32-bit address/data
registers, MMIO_Index (0x0) and MMIO_Data (0x4), which provide access
to the MMIO BAR0 (GTTMMADR) from IO space. These registers are probably
only used by the VBIOS, and are not documented by intel. The observed
layout of MMIO_Index register is:
 31                                                   2   1      0
+-------------------------------------------------------------------+
|                        Offset                        | Rsvd | Sel |
+-------------------------------------------------------------------+
- Offset: Byte offset in specified region, 4-byte aligned.
- Sel: Region selector
       0: MMIO register region (first half of MMIO BAR0)
       1: GTT region (second half of MMIO BAR0). Pre Gen11 only.

Currently, QEMU implements a quirk that adjusts the guest Data Stolen
Memory (DSM) region address to be (addr - host BDSM + guest BDSM) when
programming GTT entries via IO BAR4, assuming guest still programs GTT
with host DSM address, which is not the case. Guest's BDSM register is
emulated and initialized to 0 at startup by QEMU, then SeaBIOS programs
its value[1]. As result, the address programmed to GTT entries by VBIOS
running in guest are valid GPA, and this unnecessary adjustment brings
inconsistency.

[1] https://gitlab.com/qemu-project/seabios/-/blob/1.12-stable/src/fw/pciinit.c#L319-332

Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
---
 hw/vfio/igd.c | 191 +-------------------------------------------------
 1 file changed, 1 insertion(+), 190 deletions(-)

diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
index b1a237edd6..ca3a32f4f2 100644
--- a/hw/vfio/igd.c
+++ b/hw/vfio/igd.c
@@ -106,12 +106,6 @@ static int igd_gen(VFIOPCIDevice *vdev)
     return -1;
 }
 
-typedef struct VFIOIGDQuirk {
-    struct VFIOPCIDevice *vdev;
-    uint32_t index;
-    uint64_t bdsm;
-} VFIOIGDQuirk;
-
 #define IGD_GMCH 0x50 /* Graphics Control Register */
 #define IGD_BDSM 0x5c /* Base Data of Stolen Memory */
 #define IGD_BDSM_GEN11 0xc0 /* Base Data of Stolen Memory of gen 11 and later */
@@ -300,129 +294,6 @@ static int vfio_pci_igd_lpc_init(VFIOPCIDevice *vdev,
     return ret;
 }
 
-/*
- * IGD Gen8 and newer support up to 8MB for the GTT and use a 64bit PTE
- * entry, older IGDs use 2MB and 32bit.  Each PTE maps a 4k page.  Therefore
- * we either have 2M/4k * 4 = 2k or 8M/4k * 8 = 16k as the maximum iobar index
- * for programming the GTT.
- *
- * See linux:include/drm/i915_drm.h for shift and mask values.
- */
-static int vfio_igd_gtt_max(VFIOPCIDevice *vdev)
-{
-    uint32_t gmch = vfio_pci_read_config(&vdev->pdev, IGD_GMCH, sizeof(gmch));
-    int gen = igd_gen(vdev);
-    uint64_t ggms_size = igd_gtt_memory_size(gen, gmch);
-
-    return (ggms_size / (4 * KiB)) * (gen < 8 ? 4 : 8);
-}
-
-/*
- * The IGD ROM will make use of stolen memory (GGMS) for support of VESA modes.
- * Somehow the host stolen memory range is used for this, but how the ROM gets
- * it is a mystery, perhaps it's hardcoded into the ROM.  Thankfully though, it
- * reprograms the GTT through the IOBAR where we can trap it and transpose the
- * programming to the VM allocated buffer.  That buffer gets reserved by the VM
- * firmware via the fw_cfg entry added below.  Here we're just monitoring the
- * IOBAR address and data registers to detect a write sequence targeting the
- * GTTADR.  This code is developed by observed behavior and doesn't have a
- * direct spec reference, unfortunately.
- */
-static uint64_t vfio_igd_quirk_data_read(void *opaque,
-                                         hwaddr addr, unsigned size)
-{
-    VFIOIGDQuirk *igd = opaque;
-    VFIOPCIDevice *vdev = igd->vdev;
-
-    igd->index = ~0;
-
-    return vfio_region_read(&vdev->bars[4].region, addr + 4, size);
-}
-
-static void vfio_igd_quirk_data_write(void *opaque, hwaddr addr,
-                                      uint64_t data, unsigned size)
-{
-    VFIOIGDQuirk *igd = opaque;
-    VFIOPCIDevice *vdev = igd->vdev;
-    uint64_t val = data;
-    int gen = igd_gen(vdev);
-
-    /*
-     * Programming the GGMS starts at index 0x1 and uses every 4th index (ie.
-     * 0x1, 0x5, 0x9, 0xd,...).  For pre-Gen8 each 4-byte write is a whole PTE
-     * entry, with 0th bit enable set.  For Gen8 and up, PTEs are 64bit, so
-     * entries 0x5 & 0xd are the high dword, in our case zero.  Each PTE points
-     * to a 4k page, which we translate to a page from the VM allocated region,
-     * pointed to by the BDSM register.  If this is not set, we fail.
-     *
-     * We trap writes to the full configured GTT size, but we typically only
-     * see the vBIOS writing up to (nearly) the 1MB barrier.  In fact it often
-     * seems to miss the last entry for an even 1MB GTT.  Doing a gratuitous
-     * write of that last entry does work, but is hopefully unnecessary since
-     * we clear the previous GTT on initialization.
-     */
-    if ((igd->index % 4 == 1) && igd->index < vfio_igd_gtt_max(vdev)) {
-        if (gen < 8 || (igd->index % 8 == 1)) {
-            uint64_t base;
-
-            if (gen < 11) {
-                base = pci_get_long(vdev->pdev.config + IGD_BDSM);
-            } else {
-                base = pci_get_quad(vdev->pdev.config + IGD_BDSM_GEN11);
-            }
-            if (!base) {
-                hw_error("vfio-igd: Guest attempted to program IGD GTT before "
-                         "BIOS reserved stolen memory.  Unsupported BIOS?");
-            }
-
-            val = data - igd->bdsm + base;
-        } else {
-            val = 0; /* upper 32bits of pte, we only enable below 4G PTEs */
-        }
-
-        trace_vfio_pci_igd_bar4_write(vdev->vbasedev.name,
-                                      igd->index, data, val);
-    }
-
-    vfio_region_write(&vdev->bars[4].region, addr + 4, val, size);
-
-    igd->index = ~0;
-}
-
-static const MemoryRegionOps vfio_igd_data_quirk = {
-    .read = vfio_igd_quirk_data_read,
-    .write = vfio_igd_quirk_data_write,
-    .endianness = DEVICE_LITTLE_ENDIAN,
-};
-
-static uint64_t vfio_igd_quirk_index_read(void *opaque,
-                                          hwaddr addr, unsigned size)
-{
-    VFIOIGDQuirk *igd = opaque;
-    VFIOPCIDevice *vdev = igd->vdev;
-
-    igd->index = ~0;
-
-    return vfio_region_read(&vdev->bars[4].region, addr, size);
-}
-
-static void vfio_igd_quirk_index_write(void *opaque, hwaddr addr,
-                                       uint64_t data, unsigned size)
-{
-    VFIOIGDQuirk *igd = opaque;
-    VFIOPCIDevice *vdev = igd->vdev;
-
-    igd->index = data;
-
-    vfio_region_write(&vdev->bars[4].region, addr, data, size);
-}
-
-static const MemoryRegionOps vfio_igd_index_quirk = {
-    .read = vfio_igd_quirk_index_read,
-    .write = vfio_igd_quirk_index_write,
-    .endianness = DEVICE_LITTLE_ENDIAN,
-};
-
 #define IGD_GGC_MMIO_OFFSET     0x108040
 #define IGD_BDSM_MMIO_OFFSET    0x1080C0
 
@@ -494,14 +365,11 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
     g_autofree struct vfio_region_info *opregion = NULL;
     g_autofree struct vfio_region_info *host = NULL;
     g_autofree struct vfio_region_info *lpc = NULL;
-    VFIOQuirk *quirk;
-    VFIOIGDQuirk *igd;
     PCIDevice *lpc_bridge;
-    int i, ret, gen;
+    int ret, gen;
     uint64_t ggms_size, gms_size;
     uint64_t *bdsm_size;
     uint32_t gmch;
-    uint16_t cmd_orig, cmd;
     Error *err = NULL;
 
     /*
@@ -634,32 +502,6 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
         return;
     }
 
-    /* Setup our quirk to munge GTT addresses to the VM allocated buffer */
-    quirk = vfio_quirk_alloc(2);
-    igd = quirk->data = g_malloc0(sizeof(*igd));
-    igd->vdev = vdev;
-    igd->index = ~0;
-    if (gen < 11) {
-        igd->bdsm = vfio_pci_read_config(&vdev->pdev, IGD_BDSM, 4);
-    } else {
-        igd->bdsm = vfio_pci_read_config(&vdev->pdev, IGD_BDSM_GEN11, 4);
-        igd->bdsm |=
-            (uint64_t)vfio_pci_read_config(&vdev->pdev, IGD_BDSM_GEN11 + 4, 4) << 32;
-    }
-    igd->bdsm &= ~((1 * MiB) - 1); /* 1MB aligned */
-
-    memory_region_init_io(&quirk->mem[0], OBJECT(vdev), &vfio_igd_index_quirk,
-                          igd, "vfio-igd-index-quirk", 4);
-    memory_region_add_subregion_overlap(vdev->bars[nr].region.mem,
-                                        0, &quirk->mem[0], 1);
-
-    memory_region_init_io(&quirk->mem[1], OBJECT(vdev), &vfio_igd_data_quirk,
-                          igd, "vfio-igd-data-quirk", 4);
-    memory_region_add_subregion_overlap(vdev->bars[nr].region.mem,
-                                        4, &quirk->mem[1], 1);
-
-    QLIST_INSERT_HEAD(&vdev->bars[nr].quirks, quirk, next);
-
     /*
      * Allow user to override dsm size using x-igd-gms option, in multiples of
      * 32MiB. This option should only be used when the desired size cannot be
@@ -717,37 +559,6 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
         pci_set_quad(vdev->emulated_config_bits + IGD_BDSM_GEN11, ~0);
     }
 
-    /*
-     * This IOBAR gives us access to GTTADR, which allows us to write to
-     * the GTT itself.  So let's go ahead and write zero to all the GTT
-     * entries to avoid spurious DMA faults.  Be sure I/O access is enabled
-     * before talking to the device.
-     */
-    if (pread(vdev->vbasedev.fd, &cmd_orig, sizeof(cmd_orig),
-              vdev->config_offset + PCI_COMMAND) != sizeof(cmd_orig)) {
-        error_report("IGD device %s - failed to read PCI command register",
-                     vdev->vbasedev.name);
-    }
-
-    cmd = cmd_orig | PCI_COMMAND_IO;
-
-    if (pwrite(vdev->vbasedev.fd, &cmd, sizeof(cmd),
-               vdev->config_offset + PCI_COMMAND) != sizeof(cmd)) {
-        error_report("IGD device %s - failed to write PCI command register",
-                     vdev->vbasedev.name);
-    }
-
-    for (i = 1; i < vfio_igd_gtt_max(vdev); i += 4) {
-        vfio_region_write(&vdev->bars[4].region, 0, i, 4);
-        vfio_region_write(&vdev->bars[4].region, 4, 0, 4);
-    }
-
-    if (pwrite(vdev->vbasedev.fd, &cmd_orig, sizeof(cmd_orig),
-               vdev->config_offset + PCI_COMMAND) != sizeof(cmd_orig)) {
-        error_report("IGD device %s - failed to restore PCI command register",
-                     vdev->vbasedev.name);
-    }
-
     trace_vfio_pci_igd_bdsm_enabled(vdev->vbasedev.name,
                                     (ggms_size + gms_size) / MiB);
 }
-- 
2.47.2

[PATCH v3 02/10] vfio/igd: Do not include GTT stolen size in etc/igd-bdsm-size

[Tomita Moeko]

Though GTT Stolen Memory (GSM) is right below Data Stolen Memory (DSM)
in host address space, direct access to GSM is prohibited, and it is
not mapped to guest address space. Both host and guest accesses GSM
indirectly through the second half of MMIO BAR0 (GTTMMADR).

Guest firmware only need to reserve a memory region for DSM and program
the BDSM register with the base address of that region, that's actually
what both SeaBIOS[1] and IgdAssignmentDxe does now.

[1] https://gitlab.com/qemu-project/seabios/-/blob/1.12-stable/src/fw/pciinit.c#L319-332

Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
---
 hw/vfio/igd.c | 28 +++-------------------------
 1 file changed, 3 insertions(+), 25 deletions(-)

diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
index ca3a32f4f2..dda4c7bb5d 100644
--- a/hw/vfio/igd.c
+++ b/hw/vfio/igd.c
@@ -112,28 +112,8 @@ static int igd_gen(VFIOPCIDevice *vdev)
 
 #define IGD_GMCH_GEN6_GMS_SHIFT     3       /* SNB_GMCH in i915 */
 #define IGD_GMCH_GEN6_GMS_MASK      0x1f
-#define IGD_GMCH_GEN6_GGMS_SHIFT    8
-#define IGD_GMCH_GEN6_GGMS_MASK     0x3
 #define IGD_GMCH_GEN8_GMS_SHIFT     8       /* BDW_GMCH in i915 */
 #define IGD_GMCH_GEN8_GMS_MASK      0xff
-#define IGD_GMCH_GEN8_GGMS_SHIFT    6
-#define IGD_GMCH_GEN8_GGMS_MASK     0x3
-
-static uint64_t igd_gtt_memory_size(int gen, uint16_t gmch)
-{
-    uint64_t ggms;
-
-    if (gen < 8) {
-        ggms = (gmch >> IGD_GMCH_GEN6_GGMS_SHIFT) & IGD_GMCH_GEN6_GGMS_MASK;
-    } else {
-        ggms = (gmch >> IGD_GMCH_GEN8_GGMS_SHIFT) & IGD_GMCH_GEN8_GGMS_MASK;
-        if (ggms != 0) {
-            ggms = 1ULL << ggms;
-        }
-    }
-
-    return ggms * MiB;
-}
 
 static uint64_t igd_stolen_memory_size(int gen, uint32_t gmch)
 {
@@ -367,7 +347,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
     g_autofree struct vfio_region_info *lpc = NULL;
     PCIDevice *lpc_bridge;
     int ret, gen;
-    uint64_t ggms_size, gms_size;
+    uint64_t gms_size;
     uint64_t *bdsm_size;
     uint32_t gmch;
     Error *err = NULL;
@@ -527,7 +507,6 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
         }
     }
 
-    ggms_size = igd_gtt_memory_size(gen, gmch);
     gms_size = igd_stolen_memory_size(gen, gmch);
 
     /*
@@ -539,7 +518,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
      * config offset 0x5C.
      */
     bdsm_size = g_malloc(sizeof(*bdsm_size));
-    *bdsm_size = cpu_to_le64(ggms_size + gms_size);
+    *bdsm_size = cpu_to_le64(gms_size);
     fw_cfg_add_file(fw_cfg_find(), "etc/igd-bdsm-size",
                     bdsm_size, sizeof(*bdsm_size));
 
@@ -559,6 +538,5 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
         pci_set_quad(vdev->emulated_config_bits + IGD_BDSM_GEN11, ~0);
     }
 
-    trace_vfio_pci_igd_bdsm_enabled(vdev->vbasedev.name,
-                                    (ggms_size + gms_size) / MiB);
+    trace_vfio_pci_igd_bdsm_enabled(vdev->vbasedev.name, (gms_size / MiB));
 }
-- 
2.47.2

[PATCH v3 03/10] vfio/igd: Consolidate OpRegion initialization into a single function

[Tomita Moeko]

Both x-igd-opregion option and legacy mode require identical steps to
set up OpRegion for IGD devices. Consolidate these steps into a single
vfio_pci_igd_setup_opregion function.

The function call in pci.c is wrapped with ifdef temporarily to prevent
build error for non-x86 archs, it will be removed after we decouple it
from legacy mode.

Additionally, move vfio_pci_igd_opregion_init to igd.c to prevent it
from being compiled in non-x86 builds.

Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
---
 hw/vfio/igd.c        | 101 +++++++++++++++++++++++++++++++++++--------
 hw/vfio/pci-quirks.c |  50 ---------------------
 hw/vfio/pci.c        |  22 ++--------
 hw/vfio/pci.h        |   4 +-
 4 files changed, 88 insertions(+), 89 deletions(-)

diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
index dda4c7bb5d..50e4007abe 100644
--- a/hw/vfio/igd.c
+++ b/hw/vfio/igd.c
@@ -106,6 +106,7 @@ static int igd_gen(VFIOPCIDevice *vdev)
     return -1;
 }
 
+#define IGD_ASLS 0xfc /* ASL Storage Register */
 #define IGD_GMCH 0x50 /* Graphics Control Register */
 #define IGD_BDSM 0x5c /* Base Data of Stolen Memory */
 #define IGD_BDSM_GEN11 0xc0 /* Base Data of Stolen Memory of gen 11 and later */
@@ -138,6 +139,82 @@ static uint64_t igd_stolen_memory_size(int gen, uint32_t gmch)
     return 0;
 }
 
+/*
+ * The OpRegion includes the Video BIOS Table, which seems important for
+ * telling the driver what sort of outputs it has.  Without this, the device
+ * may work in the guest, but we may not get output.  This also requires BIOS
+ * support to reserve and populate a section of guest memory sufficient for
+ * the table and to write the base address of that memory to the ASLS register
+ * of the IGD device.
+ */
+static bool vfio_pci_igd_opregion_init(VFIOPCIDevice *vdev,
+                                       struct vfio_region_info *info,
+                                       Error **errp)
+{
+    int ret;
+
+    vdev->igd_opregion = g_malloc0(info->size);
+    ret = pread(vdev->vbasedev.fd, vdev->igd_opregion,
+                info->size, info->offset);
+    if (ret != info->size) {
+        error_setg(errp, "failed to read IGD OpRegion");
+        g_free(vdev->igd_opregion);
+        vdev->igd_opregion = NULL;
+        return false;
+    }
+
+    /*
+     * Provide fw_cfg with a copy of the OpRegion which the VM firmware is to
+     * allocate 32bit reserved memory for, copy these contents into, and write
+     * the reserved memory base address to the device ASLS register at 0xFC.
+     * Alignment of this reserved region seems flexible, but using a 4k page
+     * alignment seems to work well.  This interface assumes a single IGD
+     * device, which may be at VM address 00:02.0 in legacy mode or another
+     * address in UPT mode.
+     *
+     * NB, there may be future use cases discovered where the VM should have
+     * direct interaction with the host OpRegion, in which case the write to
+     * the ASLS register would trigger MemoryRegion setup to enable that.
+     */
+    fw_cfg_add_file(fw_cfg_find(), "etc/igd-opregion",
+                    vdev->igd_opregion, info->size);
+
+    trace_vfio_pci_igd_opregion_enabled(vdev->vbasedev.name);
+
+    pci_set_long(vdev->pdev.config + IGD_ASLS, 0);
+    pci_set_long(vdev->pdev.wmask + IGD_ASLS, ~0);
+    pci_set_long(vdev->emulated_config_bits + IGD_ASLS, ~0);
+
+    return true;
+}
+
+bool vfio_pci_igd_setup_opregion(VFIOPCIDevice *vdev, Error **errp)
+{
+    g_autofree struct vfio_region_info *opregion = NULL;
+    int ret;
+
+    /* Hotplugging is not supprted for opregion access */
+    if (vdev->pdev.qdev.hotplugged) {
+        error_setg(errp, "IGD OpRegion is not supported on hotplugged device");
+        return false;
+    }
+
+    ret = vfio_get_dev_region_info(&vdev->vbasedev,
+                    VFIO_REGION_TYPE_PCI_VENDOR_TYPE | PCI_VENDOR_ID_INTEL,
+                    VFIO_REGION_SUBTYPE_INTEL_IGD_OPREGION, &opregion);
+    if (ret) {
+        error_setg_errno(errp, -ret,
+                         "Device does not supports IGD OpRegion feature");
+        return false;
+    }
+
+    if (!vfio_pci_igd_opregion_init(vdev, opregion, errp)) {
+        return false;
+    }
+
+    return true;
+}
+
 /*
  * The rather short list of registers that we copy from the host devices.
  * The LPC/ISA bridge values are definitely needed to support the vBIOS, the
@@ -342,7 +419,6 @@ void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int nr)
 void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
 {
     g_autofree struct vfio_region_info *rom = NULL;
-    g_autofree struct vfio_region_info *opregion = NULL;
     g_autofree struct vfio_region_info *host = NULL;
     g_autofree struct vfio_region_info *lpc = NULL;
     PCIDevice *lpc_bridge;
@@ -418,15 +494,6 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
      * Check whether we have all the vfio device specific regions to
      * support legacy mode (added in Linux v4.6).  If not, bail.
      */
-    ret = vfio_get_dev_region_info(&vdev->vbasedev,
-                        VFIO_REGION_TYPE_PCI_VENDOR_TYPE | PCI_VENDOR_ID_INTEL,
-                        VFIO_REGION_SUBTYPE_INTEL_IGD_OPREGION, &opregion);
-    if (ret) {
-        error_report("IGD device %s does not support OpRegion access,"
-                     "legacy mode disabled", vdev->vbasedev.name);
-        return;
-    }
-
     ret = vfio_get_dev_region_info(&vdev->vbasedev,
                         VFIO_REGION_TYPE_PCI_VENDOR_TYPE | PCI_VENDOR_ID_INTEL,
                         VFIO_REGION_SUBTYPE_INTEL_IGD_HOST_CFG, &host);
@@ -459,6 +526,13 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
         return;
     }
 
+    /* Setup OpRegion access */
+    if (!vfio_pci_igd_setup_opregion(vdev, &err)) {
+        error_append_hint(&err, "IGD legacy mode disabled\n");
+        error_report_err(err);
+        return;
+    }
+
     /* Create our LPC/ISA bridge */
     ret = vfio_pci_igd_lpc_init(vdev, lpc);
     if (ret) {
@@ -475,13 +549,6 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
         return;
     }
 
-    /* Setup OpRegion access */
-    if (!vfio_pci_igd_opregion_init(vdev, opregion, &err)) {
-        error_append_hint(&err, "IGD legacy mode disabled\n");
-        error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
-        return;
-    }
-
     /*
      * Allow user to override dsm size using x-igd-gms option, in multiples of
      * 32MiB. This option should only be used when the desired size cannot be
diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c
index fbe43b0a79..cac0aa1910 100644
--- a/hw/vfio/pci-quirks.c
+++ b/hw/vfio/pci-quirks.c
@@ -1114,56 +1114,6 @@ static void vfio_probe_rtl8168_bar2_quirk(VFIOPCIDevice *vdev, int nr)
     trace_vfio_quirk_rtl8168_probe(vdev->vbasedev.name);
 }
 
-#define IGD_ASLS 0xfc /* ASL Storage Register */
-
-/*
- * The OpRegion includes the Video BIOS Table, which seems important for
- * telling the driver what sort of outputs it has.  Without this, the device
- * may work in the guest, but we may not get output.  This also requires BIOS
- * support to reserve and populate a section of guest memory sufficient for
- * the table and to write the base address of that memory to the ASLS register
- * of the IGD device.
- */
-bool vfio_pci_igd_opregion_init(VFIOPCIDevice *vdev,
-                                struct vfio_region_info *info, Error **errp)
-{
-    int ret;
-
-    vdev->igd_opregion = g_malloc0(info->size);
-    ret = pread(vdev->vbasedev.fd, vdev->igd_opregion,
-                info->size, info->offset);
-    if (ret != info->size) {
-        error_setg(errp, "failed to read IGD OpRegion");
-        g_free(vdev->igd_opregion);
-        vdev->igd_opregion = NULL;
-        return false;
-    }
-
-    /*
-     * Provide fw_cfg with a copy of the OpRegion which the VM firmware is to
-     * allocate 32bit reserved memory for, copy these contents into, and write
-     * the reserved memory base address to the device ASLS register at 0xFC.
-     * Alignment of this reserved region seems flexible, but using a 4k page
-     * alignment seems to work well.  This interface assumes a single IGD
-     * device, which may be at VM address 00:02.0 in legacy mode or another
-     * address in UPT mode.
-     *
-     * NB, there may be future use cases discovered where the VM should have
-     * direct interaction with the host OpRegion, in which case the write to
-     * the ASLS register would trigger MemoryRegion setup to enable that.
-     */
-    fw_cfg_add_file(fw_cfg_find(), "etc/igd-opregion",
-                    vdev->igd_opregion, info->size);
-
-    trace_vfio_pci_igd_opregion_enabled(vdev->vbasedev.name);
-
-    pci_set_long(vdev->pdev.config + IGD_ASLS, 0);
-    pci_set_long(vdev->pdev.wmask + IGD_ASLS, ~0);
-    pci_set_long(vdev->emulated_config_bits + IGD_ASLS, ~0);
-
-    return true;
-}
-
 /*
  * Common quirk probe entry points.
  */
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 89d900e9cf..098d17b866 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3132,30 +3132,14 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
         vfio_bar_quirk_setup(vdev, i);
     }
 
+#ifdef CONFIG_VFIO_IGD
     if (!vdev->igd_opregion &&
         vdev->features & VFIO_FEATURE_ENABLE_IGD_OPREGION) {
-        g_autofree struct vfio_region_info *opregion = NULL;
-
-        if (vdev->pdev.qdev.hotplugged) {
-            error_setg(errp,
-                       "cannot support IGD OpRegion feature on hotplugged "
-                       "device");
-            goto out_unset_idev;
-        }
-
-        ret = vfio_get_dev_region_info(vbasedev,
-                        VFIO_REGION_TYPE_PCI_VENDOR_TYPE | PCI_VENDOR_ID_INTEL,
-                        VFIO_REGION_SUBTYPE_INTEL_IGD_OPREGION, &opregion);
-        if (ret) {
-            error_setg_errno(errp, -ret,
-                             "does not support requested IGD OpRegion feature");
-            goto out_unset_idev;
-        }
-
-        if (!vfio_pci_igd_opregion_init(vdev, opregion, errp)) {
+        if (!vfio_pci_igd_setup_opregion(vdev, errp)) {
             goto out_unset_idev;
         }
     }
+#endif
 
     /* QEMU emulates all of MSI & MSIX */
     if (pdev->cap_present & QEMU_PCI_CAP_MSIX) {
diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
index 43c166680a..4763f14415 100644
--- a/hw/vfio/pci.h
+++ b/hw/vfio/pci.h
@@ -228,9 +228,7 @@ int vfio_pci_get_pci_hot_reset_info(VFIOPCIDevice *vdev,
 
 bool vfio_populate_vga(VFIOPCIDevice *vdev, Error **errp);
 
-bool vfio_pci_igd_opregion_init(VFIOPCIDevice *vdev,
-                                struct vfio_region_info *info,
-                                Error **errp);
+bool vfio_pci_igd_setup_opregion(VFIOPCIDevice *vdev, Error **errp);
 
 void vfio_display_reset(VFIOPCIDevice *vdev);
 bool vfio_display_probe(VFIOPCIDevice *vdev, Error **errp);
-- 
2.47.2

[PATCH v3 04/10] vfio/igd: Move LPC bridge initialization to a separate function

[Tomita Moeko]

A new option will soon be introduced to decouple the LPC bridge/Host
bridge ID quirk from legacy mode. To prepare for this, move the LPC
bridge initialization into a separate function.

Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
---
 hw/vfio/igd.c | 122 +++++++++++++++++++++++++++++---------------------
 1 file changed, 70 insertions(+), 52 deletions(-)

diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
index 50e4007abe..9d2b761d1d 100644
--- a/hw/vfio/igd.c
+++ b/hw/vfio/igd.c
@@ -351,6 +351,72 @@ static int vfio_pci_igd_lpc_init(VFIOPCIDevice *vdev,
     return ret;
 }
 
+static bool vfio_pci_igd_setup_lpc_bridge(VFIOPCIDevice *vdev, Error **errp)
+{
+    g_autofree struct vfio_region_info *host = NULL;
+    g_autofree struct vfio_region_info *lpc = NULL;
+    PCIDevice *lpc_bridge;
+    int ret;
+
+    /*
+     * Copying IDs or creating new devices are not supported on hotplug
+     */
+    if (vdev->pdev.qdev.hotplugged) {
+        error_setg(errp, "IGD LPC is not supported on hotplugged device");
+        return false;
+    }
+
+    /*
+     * We need to create an LPC/ISA bridge at PCI bus address 00:1f.0 that we
+     * can stuff host values into, so if there's already one there and it's not
+     * one we can hack on, this quirk is no-go.  Sorry Q35.
+     */
+    lpc_bridge = pci_find_device(pci_device_root_bus(&vdev->pdev),
+                                 0, PCI_DEVFN(0x1f, 0));
+    if (lpc_bridge && !object_dynamic_cast(OBJECT(lpc_bridge),
+                                           "vfio-pci-igd-lpc-bridge")) {
+        error_setg(errp,
+                   "Cannot create LPC bridge due to existing device at 1f.0");
+        return false;
+    }
+
+    /*
+     * Check whether we have all the vfio device specific regions to
+     * support LPC quirk (added in Linux v4.6).
+     */
+    ret = vfio_get_dev_region_info(&vdev->vbasedev,
+                        VFIO_REGION_TYPE_PCI_VENDOR_TYPE | PCI_VENDOR_ID_INTEL,
+                        VFIO_REGION_SUBTYPE_INTEL_IGD_LPC_CFG, &lpc);
+    if (ret) {
+        error_setg(errp, "IGD LPC bridge access is not supported by kernel");
+        return false;
+    }
+
+    ret = vfio_get_dev_region_info(&vdev->vbasedev,
+                        VFIO_REGION_TYPE_PCI_VENDOR_TYPE | PCI_VENDOR_ID_INTEL,
+                        VFIO_REGION_SUBTYPE_INTEL_IGD_HOST_CFG, &host);
+    if (ret) {
+        error_setg(errp, "IGD host bridge access is not supported by kernel");
+        return false;
+    }
+
+    /* Create/modify LPC bridge */
+    ret = vfio_pci_igd_lpc_init(vdev, lpc);
+    if (ret) {
+        error_setg(errp, "Failed to create/modify LPC bridge for IGD");
+        return false;
+    }
+
+    /* Stuff some host values into the VM PCI host bridge */
+    ret = vfio_pci_igd_host_init(vdev, host);
+    if (ret) {
+        error_setg(errp, "Failed to modify host bridge for IGD");
+        return false;
+    }
+
+    return true;
+}
+
 #define IGD_GGC_MMIO_OFFSET     0x108040
 #define IGD_BDSM_MMIO_OFFSET    0x1080C0
 
@@ -419,9 +485,6 @@ void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int nr)
 void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
 {
     g_autofree struct vfio_region_info *rom = NULL;
-    g_autofree struct vfio_region_info *host = NULL;
-    g_autofree struct vfio_region_info *lpc = NULL;
-    PCIDevice *lpc_bridge;
     int ret, gen;
     uint64_t gms_size;
     uint64_t *bdsm_size;
@@ -440,20 +503,6 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
         return;
     }
 
-    /*
-     * We need to create an LPC/ISA bridge at PCI bus address 00:1f.0 that we
-     * can stuff host values into, so if there's already one there and it's not
-     * one we can hack on, legacy mode is no-go.  Sorry Q35.
-     */
-    lpc_bridge = pci_find_device(pci_device_root_bus(&vdev->pdev),
-                                 0, PCI_DEVFN(0x1f, 0));
-    if (lpc_bridge && !object_dynamic_cast(OBJECT(lpc_bridge),
-                                           "vfio-pci-igd-lpc-bridge")) {
-        error_report("IGD device %s cannot support legacy mode due to existing "
-                     "devices at address 1f.0", vdev->vbasedev.name);
-        return;
-    }
-
     /*
      * 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
@@ -490,28 +539,6 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
         return;
     }
 
-    /*
-     * Check whether we have all the vfio device specific regions to
-     * support legacy mode (added in Linux v4.6).  If not, bail.
-     */
-    ret = vfio_get_dev_region_info(&vdev->vbasedev,
-                        VFIO_REGION_TYPE_PCI_VENDOR_TYPE | PCI_VENDOR_ID_INTEL,
-                        VFIO_REGION_SUBTYPE_INTEL_IGD_HOST_CFG, &host);
-    if (ret) {
-        error_report("IGD device %s does not support host bridge access,"
-                     "legacy mode disabled", vdev->vbasedev.name);
-        return;
-    }
-
-    ret = vfio_get_dev_region_info(&vdev->vbasedev,
-                        VFIO_REGION_TYPE_PCI_VENDOR_TYPE | PCI_VENDOR_ID_INTEL,
-                        VFIO_REGION_SUBTYPE_INTEL_IGD_LPC_CFG, &lpc);
-    if (ret) {
-        error_report("IGD device %s does not support LPC bridge access,"
-                     "legacy mode disabled", vdev->vbasedev.name);
-        return;
-    }
-
     gmch = vfio_pci_read_config(&vdev->pdev, IGD_GMCH, 4);
 
     /*
@@ -533,19 +560,10 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
         return;
     }
 
-    /* Create our LPC/ISA bridge */
-    ret = vfio_pci_igd_lpc_init(vdev, lpc);
-    if (ret) {
-        error_report("IGD device %s failed to create LPC bridge, "
-                     "legacy mode disabled", vdev->vbasedev.name);
-        return;
-    }
-
-    /* Stuff some host values into the VM PCI host bridge */
-    ret = vfio_pci_igd_host_init(vdev, host);
-    if (ret) {
-        error_report("IGD device %s failed to modify host bridge, "
-                     "legacy mode disabled", vdev->vbasedev.name);
+    /* Setup LPC bridge / Host bridge PCI IDs */
+    if (!vfio_pci_igd_setup_lpc_bridge(vdev, &err)) {
+        error_append_hint(&err, "IGD legacy mode disabled\n");
+        error_report_err(err);
         return;
     }
 
-- 
2.47.2

[PATCH v3 05/10] vfio/pci: Add placeholder for device-specific config space quirks

[Tomita Moeko]

IGD devices require device-specific quirk to be applied to their PCI
config space. Currently, it is put in the BAR4 quirk that does nothing
to BAR4 itself. Add a placeholder for PCI config space quirks to hold
that quirk later.

Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
---
 hw/vfio/pci-quirks.c | 5 +++++
 hw/vfio/pci.c        | 4 ++++
 hw/vfio/pci.h        | 1 +
 3 files changed, 10 insertions(+)

diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c
index cac0aa1910..8a81c60400 100644
--- a/hw/vfio/pci-quirks.c
+++ b/hw/vfio/pci-quirks.c
@@ -1117,6 +1117,11 @@ static void vfio_probe_rtl8168_bar2_quirk(VFIOPCIDevice *vdev, int nr)
 /*
  * Common quirk probe entry points.
  */
+bool vfio_config_quirk_setup(VFIOPCIDevice *vdev, Error **errp)
+{
+    return true;
+}
+
 void vfio_vga_quirk_setup(VFIOPCIDevice *vdev)
 {
     vfio_vga_probe_ati_3c3_quirk(vdev);
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 098d17b866..a58d555934 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3124,6 +3124,10 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
         goto out_unset_idev;
     }
 
+    if (!vfio_config_quirk_setup(vdev, errp)) {
+        goto out_unset_idev;
+    }
+
     if (vdev->vga) {
         vfio_vga_quirk_setup(vdev);
     }
diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
index 4763f14415..d54e43764b 100644
--- a/hw/vfio/pci.h
+++ b/hw/vfio/pci.h
@@ -205,6 +205,7 @@ uint64_t vfio_vga_read(void *opaque, hwaddr addr, unsigned size);
 void vfio_vga_write(void *opaque, hwaddr addr, uint64_t data, unsigned size);
 
 bool vfio_opt_rom_in_denylist(VFIOPCIDevice *vdev);
+bool vfio_config_quirk_setup(VFIOPCIDevice *vdev, Error **errp);
 void vfio_vga_quirk_setup(VFIOPCIDevice *vdev);
 void vfio_vga_quirk_exit(VFIOPCIDevice *vdev);
 void vfio_vga_quirk_finalize(VFIOPCIDevice *vdev);
-- 
2.47.2

[PATCH v3 06/10] vfio/igd: Refactor vfio_probe_igd_bar4_quirk into pci config quirk

[Tomita Moeko]

The actual IO BAR4 write quirk in vfio_probe_igd_bar4_quirk was removed
in previous change, leaving the function not matching its name, so move
it into the newly introduced vfio_config_quirk_setup. There is no
functional change in this commit.

For now, to align with current legacy mode behavior, it returns and
proceeds on error. Later it will fail on error after decoupling the
quirks from legacy mode.

Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
---
 hw/vfio/igd.c        | 21 ++++++++++++---------
 hw/vfio/pci-quirks.c |  6 +++++-
 hw/vfio/pci.h        |  2 +-
 3 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
index 9d2b761d1d..f5e19f1241 100644
--- a/hw/vfio/igd.c
+++ b/hw/vfio/igd.c
@@ -482,7 +482,8 @@ void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int nr)
     QLIST_INSERT_HEAD(&vdev->bars[nr].quirks, bdsm_quirk, next);
 }
 
-void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
+bool vfio_probe_igd_config_quirk(VFIOPCIDevice *vdev,
+                                 Error **errp G_GNUC_UNUSED)
 {
     g_autofree struct vfio_region_info *rom = NULL;
     int ret, gen;
@@ -497,10 +498,10 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
      * PCI bus address.
      */
     if (!vfio_pci_is(vdev, PCI_VENDOR_ID_INTEL, PCI_ANY_ID) ||
-        !vfio_is_vga(vdev) || nr != 4 ||
+        !vfio_is_vga(vdev) ||
         &vdev->pdev != pci_find_device(pci_device_root_bus(&vdev->pdev),
                                        0, PCI_DEVFN(0x2, 0))) {
-        return;
+        return true;
     }
 
     /*
@@ -512,7 +513,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
     if (gen == -1) {
         error_report("IGD device %s is unsupported in legacy mode, "
                      "try SandyBridge or newer", vdev->vbasedev.name);
-        return;
+        return true;
     }
 
     /*
@@ -525,7 +526,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
     if ((ret || !rom->size) && !vdev->pdev.romfile) {
         error_report("IGD device %s has no ROM, legacy mode disabled",
                      vdev->vbasedev.name);
-        return;
+        return true;
     }
 
     /*
@@ -536,7 +537,7 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
         error_report("IGD device %s hotplugged, ROM disabled, "
                      "legacy mode disabled", vdev->vbasedev.name);
         vdev->rom_read_failed = true;
-        return;
+        return true;
     }
 
     gmch = vfio_pci_read_config(&vdev->pdev, IGD_GMCH, 4);
@@ -550,21 +551,21 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
         error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
         error_report("IGD device %s failed to enable VGA access, "
                      "legacy mode disabled", vdev->vbasedev.name);
-        return;
+        return true;
     }
 
     /* Setup OpRegion access */
     if (!vfio_pci_igd_setup_opregion(vdev, &err)) {
         error_append_hint(&err, "IGD legacy mode disabled\n");
         error_report_err(err);
-        return;
+        return true;
     }
 
     /* Setup LPC bridge / Host bridge PCI IDs */
     if (!vfio_pci_igd_setup_lpc_bridge(vdev, &err)) {
         error_append_hint(&err, "IGD legacy mode disabled\n");
         error_report_err(err);
-        return;
+        return true;
     }
 
     /*
@@ -624,4 +625,6 @@ void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr)
     }
 
     trace_vfio_pci_igd_bdsm_enabled(vdev->vbasedev.name, (gms_size / MiB));
+
+    return true;
 }
diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c
index 8a81c60400..f2b37910f0 100644
--- a/hw/vfio/pci-quirks.c
+++ b/hw/vfio/pci-quirks.c
@@ -1119,6 +1119,11 @@ static void vfio_probe_rtl8168_bar2_quirk(VFIOPCIDevice *vdev, int nr)
  */
 bool vfio_config_quirk_setup(VFIOPCIDevice *vdev, Error **errp)
 {
+#ifdef CONFIG_VFIO_IGD
+    if (!vfio_probe_igd_config_quirk(vdev, errp)) {
+        return false;
+    }
+#endif
     return true;
 }
 
@@ -1170,7 +1175,6 @@ void vfio_bar_quirk_setup(VFIOPCIDevice *vdev, int nr)
     vfio_probe_rtl8168_bar2_quirk(vdev, nr);
 #ifdef CONFIG_VFIO_IGD
     vfio_probe_igd_bar0_quirk(vdev, nr);
-    vfio_probe_igd_bar4_quirk(vdev, nr);
 #endif
 }
 
diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
index d54e43764b..2e81f9eb19 100644
--- a/hw/vfio/pci.h
+++ b/hw/vfio/pci.h
@@ -217,7 +217,7 @@ bool vfio_add_virt_caps(VFIOPCIDevice *vdev, Error **errp);
 void vfio_quirk_reset(VFIOPCIDevice *vdev);
 VFIOQuirk *vfio_quirk_alloc(int nr_mem);
 void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int nr);
-void vfio_probe_igd_bar4_quirk(VFIOPCIDevice *vdev, int nr);
+bool vfio_probe_igd_config_quirk(VFIOPCIDevice *vdev, Error **errp);
 
 extern const PropertyInfo qdev_prop_nv_gpudirect_clique;
 
-- 
2.47.2

[PATCH v3 07/10] vfio/igd: Decouple common quirks from legacy mode

[Tomita Moeko]

So far, IGD-specific quirks all require enabling legacy mode, which is
toggled by assigning IGD to 00:02.0. However, some quirks, like the BDSM
and GGC register quirks, should be applied to all supported IGD devices.
A new config option, x-igd-legacy-mode=[on|off|auto], is introduced to
control the legacy mode only quirks. The default value is "auto", which
keeps current behavior that enables legacy mode implicitly and continues
on error when all following conditions are met.
* Machine type is i440fx
* IGD device is at guest BDF 00:02.0

If any one of the conditions above is not met, the default behavior is
equivalent to "off", QEMU will fail immediately if any error occurs.

Users can also use "on" to force enabling legacy mode. It checks if all
the conditions above are met and set up legacy mode. QEMU will also fail
immediately on error in this case.

Additionally, the hotplug check in legacy mode is removed as hotplugging
IGD device is never supported, and it will be checked when enabling the
OpRegion quirk.

Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
---
 hw/vfio/igd.c | 127 +++++++++++++++++++++++++++++---------------------
 hw/vfio/pci.c |   2 +
 hw/vfio/pci.h |   1 +
 3 files changed, 77 insertions(+), 53 deletions(-)

diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
index f5e19f1241..ac096e2eb5 100644
--- a/hw/vfio/igd.c
+++ b/hw/vfio/igd.c
@@ -15,6 +15,7 @@
 #include "qemu/error-report.h"
 #include "qapi/error.h"
 #include "qapi/qmp/qerror.h"
+#include "hw/boards.h"
 #include "hw/hw.h"
 #include "hw/nvram/fw_cfg.h"
 #include "pci.h"
@@ -432,9 +433,7 @@ void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int nr)
      * bus address.
      */
     if (!vfio_pci_is(vdev, PCI_VENDOR_ID_INTEL, PCI_ANY_ID) ||
-        !vfio_is_vga(vdev) || nr != 0 ||
-        &vdev->pdev != pci_find_device(pci_device_root_bus(&vdev->pdev),
-                                       0, PCI_DEVFN(0x2, 0))) {
+        !vfio_is_vga(vdev) || nr != 0) {
         return;
     }
 
@@ -482,14 +481,13 @@ void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int nr)
     QLIST_INSERT_HEAD(&vdev->bars[nr].quirks, bdsm_quirk, next);
 }
 
-bool vfio_probe_igd_config_quirk(VFIOPCIDevice *vdev,
-                                 Error **errp G_GNUC_UNUSED)
+bool vfio_probe_igd_config_quirk(VFIOPCIDevice *vdev, Error **errp)
 {
-    g_autofree struct vfio_region_info *rom = NULL;
     int ret, gen;
     uint64_t gms_size;
     uint64_t *bdsm_size;
     uint32_t gmch;
+    bool legacy_mode_enabled = false;
     Error *err = NULL;
 
     /*
@@ -498,9 +496,7 @@ bool vfio_probe_igd_config_quirk(VFIOPCIDevice *vdev,
      * PCI bus address.
      */
     if (!vfio_pci_is(vdev, PCI_VENDOR_ID_INTEL, PCI_ANY_ID) ||
-        !vfio_is_vga(vdev) ||
-        &vdev->pdev != pci_find_device(pci_device_root_bus(&vdev->pdev),
-                                       0, PCI_DEVFN(0x2, 0))) {
+        !vfio_is_vga(vdev)) {
         return true;
     }
 
@@ -516,56 +512,67 @@ bool vfio_probe_igd_config_quirk(VFIOPCIDevice *vdev,
         return true;
     }
 
-    /*
-     * Most of what we're doing here is to enable the ROM to run, so if
-     * there's no ROM, there's no point in setting up this quirk.
-     * NB. We only seem to get BIOS ROMs, so a UEFI VM would need CSM support.
-     */
-    ret = vfio_get_region_info(&vdev->vbasedev,
-                               VFIO_PCI_ROM_REGION_INDEX, &rom);
-    if ((ret || !rom->size) && !vdev->pdev.romfile) {
-        error_report("IGD device %s has no ROM, legacy mode disabled",
-                     vdev->vbasedev.name);
-        return true;
-    }
-
-    /*
-     * Ignore the hotplug corner case, mark the ROM failed, we can't
-     * create the devices we need for legacy mode in the hotplug scenario.
-     */
-    if (vdev->pdev.qdev.hotplugged) {
-        error_report("IGD device %s hotplugged, ROM disabled, "
-                     "legacy mode disabled", vdev->vbasedev.name);
-        vdev->rom_read_failed = true;
-        return true;
-    }
-
     gmch = vfio_pci_read_config(&vdev->pdev, IGD_GMCH, 4);
 
     /*
-     * If IGD VGA Disable is clear (expected) and VGA is not already enabled,
-     * try to enable it.  Probably shouldn't be using legacy mode without VGA,
-     * but also no point in us enabling VGA if disabled in hardware.
+     * For backward compatibilty, enable legacy mode when
+     * - Machine type is i440fx (pc_piix)
+     * - IGD device is at guest BDF 00:02.0
+     * - Not manually disabled by x-igd-legacy-mode=off
      */
-    if (!(gmch & 0x2) && !vdev->vga && !vfio_populate_vga(vdev, &err)) {
-        error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
-        error_report("IGD device %s failed to enable VGA access, "
-                     "legacy mode disabled", vdev->vbasedev.name);
-        return true;
-    }
+    if ((vdev->igd_legacy_mode != ON_OFF_AUTO_OFF) &&
+        !strcmp(MACHINE_GET_CLASS(qdev_get_machine())->family, "pc_piix") &&
+        (&vdev->pdev == pci_find_device(pci_device_root_bus(&vdev->pdev),
+        0, PCI_DEVFN(0x2, 0)))) {
+        /*
+         * IGD legacy mode requires:
+         * - VBIOS in ROM BAR or file
+         * - VGA IO/MMIO ranges are claimed by IGD
+         * - OpRegion
+         * - Same LPC bridge and Host bridge VID/DID/SVID/SSID as host
+         */
+        g_autofree struct vfio_region_info *rom = NULL;
+
+        legacy_mode_enabled = true;
+        warn_report("IGD legacy mode enabled, "
+                    "use x-igd-legacy-mode=off to disable it if unwanted.");
+
+        /*
+         * Most of what we're doing here is to enable the ROM to run, so if
+         * there's no ROM, there's no point in setting up this quirk.
+         * NB. We only seem to get BIOS ROMs, so UEFI VM would need CSM support.
+         */
+        ret = vfio_get_region_info(&vdev->vbasedev,
+                                   VFIO_PCI_ROM_REGION_INDEX, &rom);
+        if ((ret || !rom->size) && !vdev->pdev.romfile) {
+            error_setg(&err, "Device has no ROM");
+            goto error;
+        }
 
-    /* Setup OpRegion access */
-    if (!vfio_pci_igd_setup_opregion(vdev, &err)) {
-        error_append_hint(&err, "IGD legacy mode disabled\n");
-        error_report_err(err);
-        return true;
-    }
+        /*
+         * If IGD VGA Disable is clear (expected) and VGA is not already
+         * enabled, try to enable it. Probably shouldn't be using legacy mode
+         * without VGA, but also no point in us enabling VGA if disabled in
+         * hardware.
+         */
+        if (!(gmch & 0x2) && !vdev->vga && !vfio_populate_vga(vdev, &err)) {
+            error_setg(&err, "Unable to enable VGA access");
+            goto error;
+        }
 
-    /* Setup LPC bridge / Host bridge PCI IDs */
-    if (!vfio_pci_igd_setup_lpc_bridge(vdev, &err)) {
-        error_append_hint(&err, "IGD legacy mode disabled\n");
-        error_report_err(err);
-        return true;
+        /* Setup OpRegion access */
+        if (!vfio_pci_igd_setup_opregion(vdev, &err)) {
+            goto error;
+        }
+
+        /* Setup LPC bridge / Host bridge PCI IDs */
+        if (!vfio_pci_igd_setup_lpc_bridge(vdev, &err)) {
+            goto error;
+        }
+    } else if (vdev->igd_legacy_mode == ON_OFF_AUTO_ON) {
+        error_setg(&err,
+                   "Machine is not i440fx or assigned BDF is not 00:02.0");
+        goto error;
     }
 
     /*
@@ -627,4 +634,18 @@ bool vfio_probe_igd_config_quirk(VFIOPCIDevice *vdev,
     trace_vfio_pci_igd_bdsm_enabled(vdev->vbasedev.name, (gms_size / MiB));
 
     return true;
+
+error:
+    /*
+     * When legacy mode is implicity enabled, continue on error,
+     * to keep compatibility
+     */
+    if (legacy_mode_enabled && (vdev->igd_legacy_mode == ON_OFF_AUTO_AUTO)) {
+        error_report_err(err);
+        error_report("IGD legacy mode disabled");
+        return true;
+    }
+
+    error_propagate(errp, err);
+    return false;
 }
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index a58d555934..d5ff8c1ea8 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3363,6 +3363,8 @@ static const Property vfio_pci_dev_properties[] = {
                     VFIO_FEATURE_ENABLE_REQ_BIT, true),
     DEFINE_PROP_BIT("x-igd-opregion", VFIOPCIDevice, features,
                     VFIO_FEATURE_ENABLE_IGD_OPREGION_BIT, false),
+    DEFINE_PROP_ON_OFF_AUTO("x-igd-legacy-mode", VFIOPCIDevice,
+                            igd_legacy_mode, ON_OFF_AUTO_AUTO),
     DEFINE_PROP_ON_OFF_AUTO("enable-migration", VFIOPCIDevice,
                             vbasedev.enable_migration, ON_OFF_AUTO_AUTO),
     DEFINE_PROP_BOOL("migration-events", VFIOPCIDevice,
diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
index 2e81f9eb19..aa67ceb346 100644
--- a/hw/vfio/pci.h
+++ b/hw/vfio/pci.h
@@ -158,6 +158,7 @@ struct VFIOPCIDevice {
     uint32_t display_xres;
     uint32_t display_yres;
     int32_t bootindex;
+    OnOffAuto igd_legacy_mode;
     uint32_t igd_gms;
     OffAutoPCIBAR msix_relo;
     uint8_t pm_cap;
-- 
2.47.2

[PATCH v3 08/10] vfio/igd: Handle x-igd-opregion option in config quirk

[Tomita Moeko]

Both enable OpRegion option (x-igd-opregion) and legacy mode require
setting up OpRegion copy for IGD devices. As the config quirk no longer
depends on legacy mode, we can now handle x-igd-opregion option there
instead of in vfio_realize.

Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
---
 hw/vfio/igd.c | 14 +++++++++-----
 hw/vfio/pci.c |  9 ---------
 hw/vfio/pci.h |  2 --
 3 files changed, 9 insertions(+), 16 deletions(-)

diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
index ac096e2eb5..4c3862b61e 100644
--- a/hw/vfio/igd.c
+++ b/hw/vfio/igd.c
@@ -189,7 +189,7 @@ static bool vfio_pci_igd_opregion_init(VFIOPCIDevice *vdev,
     return true;
 }
 
-bool vfio_pci_igd_setup_opregion(VFIOPCIDevice *vdev, Error **errp)
+static bool vfio_pci_igd_setup_opregion(VFIOPCIDevice *vdev, Error **errp)
 {
     g_autofree struct vfio_region_info *opregion = NULL;
     int ret;
@@ -560,10 +560,8 @@ bool vfio_probe_igd_config_quirk(VFIOPCIDevice *vdev, Error **errp)
             goto error;
         }
 
-        /* Setup OpRegion access */
-        if (!vfio_pci_igd_setup_opregion(vdev, &err)) {
-            goto error;
-        }
+        /* Enable OpRegion quirk */
+        vdev->features |= VFIO_FEATURE_ENABLE_IGD_OPREGION;
 
         /* Setup LPC bridge / Host bridge PCI IDs */
         if (!vfio_pci_igd_setup_lpc_bridge(vdev, &err)) {
@@ -575,6 +573,12 @@ bool vfio_probe_igd_config_quirk(VFIOPCIDevice *vdev, Error **errp)
         goto error;
     }
 
+    /* Setup OpRegion access */
+    if ((vdev->features & VFIO_FEATURE_ENABLE_IGD_OPREGION) &&
+        !vfio_pci_igd_setup_opregion(vdev, errp)) {
+        goto error;
+    }
+
     /*
      * Allow user to override dsm size using x-igd-gms option, in multiples of
      * 32MiB. This option should only be used when the desired size cannot be
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index d5ff8c1ea8..c9a484fa68 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3136,15 +3136,6 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
         vfio_bar_quirk_setup(vdev, i);
     }
 
-#ifdef CONFIG_VFIO_IGD
-    if (!vdev->igd_opregion &&
-        vdev->features & VFIO_FEATURE_ENABLE_IGD_OPREGION) {
-        if (!vfio_pci_igd_setup_opregion(vdev, errp)) {
-            goto out_unset_idev;
-        }
-    }
-#endif
-
     /* QEMU emulates all of MSI & MSIX */
     if (pdev->cap_present & QEMU_PCI_CAP_MSIX) {
         memset(vdev->emulated_config_bits + pdev->msix_cap, 0xff,
diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
index aa67ceb346..0d0a18136a 100644
--- a/hw/vfio/pci.h
+++ b/hw/vfio/pci.h
@@ -230,8 +230,6 @@ int vfio_pci_get_pci_hot_reset_info(VFIOPCIDevice *vdev,
 
 bool vfio_populate_vga(VFIOPCIDevice *vdev, Error **errp);
 
-bool vfio_pci_igd_setup_opregion(VFIOPCIDevice *vdev, Error **errp);
-
 void vfio_display_reset(VFIOPCIDevice *vdev);
 bool vfio_display_probe(VFIOPCIDevice *vdev, Error **errp);
 void vfio_display_finalize(VFIOPCIDevice *vdev);
-- 
2.47.2

[PATCH v3 09/10] vfio/igd: Introduce x-igd-lpc option for LPC bridge ID quirk

[Tomita Moeko]

The LPC bridge/Host bridge IDs quirk is also not dependent on legacy
mode. Recent Windows driver no longer depends on these IDs, as well as
Linux i915 driver, while UEFI GOP seems still needs them. Make it an
option to allow users enabling and disabling it as needed.

Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
---
 hw/vfio/igd.c | 14 ++++++++------
 hw/vfio/pci.c |  2 ++
 hw/vfio/pci.h |  3 +++
 3 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
index 4c3862b61e..6775ce2eac 100644
--- a/hw/vfio/igd.c
+++ b/hw/vfio/igd.c
@@ -560,13 +560,9 @@ bool vfio_probe_igd_config_quirk(VFIOPCIDevice *vdev, Error **errp)
             goto error;
         }
 
-        /* Enable OpRegion quirk */
+        /* Enable OpRegion and LPC brige quirk */
         vdev->features |= VFIO_FEATURE_ENABLE_IGD_OPREGION;
-
-        /* Setup LPC bridge / Host bridge PCI IDs */
-        if (!vfio_pci_igd_setup_lpc_bridge(vdev, &err)) {
-            goto error;
-        }
+        vdev->features |= VFIO_FEATURE_ENABLE_IGD_LPC;
     } else if (vdev->igd_legacy_mode == ON_OFF_AUTO_ON) {
         error_setg(&err,
                    "Machine is not i440fx or assigned BDF is not 00:02.0");
@@ -579,6 +575,12 @@ bool vfio_probe_igd_config_quirk(VFIOPCIDevice *vdev, Error **errp)
         goto error;
     }
 
+    /* Setup LPC bridge / Host bridge PCI IDs */
+    if ((vdev->features & VFIO_FEATURE_ENABLE_IGD_LPC) &&
+        !vfio_pci_igd_setup_lpc_bridge(vdev, errp)) {
+        goto error;
+     }
+
     /*
      * Allow user to override dsm size using x-igd-gms option, in multiples of
      * 32MiB. This option should only be used when the desired size cannot be
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index c9a484fa68..d16cc5cc90 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3354,6 +3354,8 @@ static const Property vfio_pci_dev_properties[] = {
                     VFIO_FEATURE_ENABLE_REQ_BIT, true),
     DEFINE_PROP_BIT("x-igd-opregion", VFIOPCIDevice, features,
                     VFIO_FEATURE_ENABLE_IGD_OPREGION_BIT, false),
+    DEFINE_PROP_BIT("x-igd-lpc", VFIOPCIDevice, features,
+                    VFIO_FEATURE_ENABLE_IGD_LPC_BIT, false),
     DEFINE_PROP_ON_OFF_AUTO("x-igd-legacy-mode", VFIOPCIDevice,
                             igd_legacy_mode, ON_OFF_AUTO_AUTO),
     DEFINE_PROP_ON_OFF_AUTO("enable-migration", VFIOPCIDevice,
diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
index 0d0a18136a..a87333f3d5 100644
--- a/hw/vfio/pci.h
+++ b/hw/vfio/pci.h
@@ -154,6 +154,9 @@ struct VFIOPCIDevice {
 #define VFIO_FEATURE_ENABLE_IGD_OPREGION_BIT 2
 #define VFIO_FEATURE_ENABLE_IGD_OPREGION \
                                 (1 << VFIO_FEATURE_ENABLE_IGD_OPREGION_BIT)
+#define VFIO_FEATURE_ENABLE_IGD_LPC_BIT 3
+#define VFIO_FEATURE_ENABLE_IGD_LPC \
+                                (1 << VFIO_FEATURE_ENABLE_IGD_LPC_BIT)
     OnOffAuto display;
     uint32_t display_xres;
     uint32_t display_yres;
-- 
2.47.2

[PATCH v3 10/10] vfio/igd: Fix broken KVMGT OpRegion support

[Tomita Moeko]

The KVMGT/GVT-g vGPU also exposes OpRegion. But unlike IGD passthrough,
it only needs the OpRegion quirk. A previous change moved x-igd-opregion
handling to config quirk breaks KVMGT functionality as it brings extra
checks and applied other quirks. Here we check if the device is mdev
(KVMGT) or not (passthrough), and then applies corresponding quirks.

As before, users must manually specify x-igd-opregion=on to enable it
on KVMGT devices. In the future, we may check the VID/DID and enable
OpRegion automatically.

Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
---
 hw/vfio/igd.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
index 6775ce2eac..4c5d6a016b 100644
--- a/hw/vfio/igd.c
+++ b/hw/vfio/igd.c
@@ -481,7 +481,7 @@ void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int nr)
     QLIST_INSERT_HEAD(&vdev->bars[nr].quirks, bdsm_quirk, next);
 }
 
-bool vfio_probe_igd_config_quirk(VFIOPCIDevice *vdev, Error **errp)
+static bool vfio_pci_igd_config_quirk(VFIOPCIDevice *vdev, Error **errp)
 {
     int ret, gen;
     uint64_t gms_size;
@@ -655,3 +655,28 @@ error:
     error_propagate(errp, err);
     return false;
 }
+
+/*
+ * KVMGT/GVT-g vGPU exposes an emulated OpRegion. So far, users have to specify
+ * x-igd-opregion=on to enable the access.
+ * TODO: Check VID/DID and enable opregion access automatically
+ */
+static bool vfio_pci_kvmgt_config_quirk(VFIOPCIDevice *vdev, Error **errp)
+{
+    if ((vdev->features & VFIO_FEATURE_ENABLE_IGD_OPREGION) &&
+        !vfio_pci_igd_setup_opregion(vdev, errp)) {
+        return false;
+    }
+
+    return true;
+}
+
+bool vfio_probe_igd_config_quirk(VFIOPCIDevice *vdev, Error **errp)
+{
+    /* KVMGT/GVT-g vGPU is exposed as mdev */
+    if (vdev->vbasedev.mdev) {
+        return vfio_pci_kvmgt_config_quirk(vdev, errp);
+    }
+
+    return vfio_pci_igd_config_quirk(vdev, errp);
+}
-- 
2.47.2

Re: [PATCH v3 07/10] vfio/igd: Decouple common quirks from legacy mode

[Alex Williamson]

On Fri,  7 Mar 2025 02:01:27 +0800
Tomita Moeko <tomitamoeko@gmail.com> wrote:

> So far, IGD-specific quirks all require enabling legacy mode, which is
> toggled by assigning IGD to 00:02.0. However, some quirks, like the BDSM
> and GGC register quirks, should be applied to all supported IGD devices.
> A new config option, x-igd-legacy-mode=[on|off|auto], is introduced to
> control the legacy mode only quirks. The default value is "auto", which
> keeps current behavior that enables legacy mode implicitly and continues
> on error when all following conditions are met.
> * Machine type is i440fx
> * IGD device is at guest BDF 00:02.0
> 
> If any one of the conditions above is not met, the default behavior is
> equivalent to "off", QEMU will fail immediately if any error occurs.
> 
> Users can also use "on" to force enabling legacy mode. It checks if all
> the conditions above are met and set up legacy mode. QEMU will also fail
> immediately on error in this case.
> 
> Additionally, the hotplug check in legacy mode is removed as hotplugging
> IGD device is never supported, and it will be checked when enabling the
> OpRegion quirk.
> 
> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
> ---
>  hw/vfio/igd.c | 127 +++++++++++++++++++++++++++++---------------------
>  hw/vfio/pci.c |   2 +
>  hw/vfio/pci.h |   1 +
>  3 files changed, 77 insertions(+), 53 deletions(-)
> 
> diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
> index f5e19f1241..ac096e2eb5 100644
> --- a/hw/vfio/igd.c
> +++ b/hw/vfio/igd.c
> @@ -15,6 +15,7 @@
>  #include "qemu/error-report.h"
>  #include "qapi/error.h"
>  #include "qapi/qmp/qerror.h"
> +#include "hw/boards.h"
>  #include "hw/hw.h"
>  #include "hw/nvram/fw_cfg.h"
>  #include "pci.h"
> @@ -432,9 +433,7 @@ void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int nr)
>       * bus address.
>       */
>      if (!vfio_pci_is(vdev, PCI_VENDOR_ID_INTEL, PCI_ANY_ID) ||
> -        !vfio_is_vga(vdev) || nr != 0 ||
> -        &vdev->pdev != pci_find_device(pci_device_root_bus(&vdev->pdev),
> -                                       0, PCI_DEVFN(0x2, 0))) {
> +        !vfio_is_vga(vdev) || nr != 0) {
>          return;
>      }
>  
> @@ -482,14 +481,13 @@ void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int nr)
>      QLIST_INSERT_HEAD(&vdev->bars[nr].quirks, bdsm_quirk, next);
>  }
>  
> -bool vfio_probe_igd_config_quirk(VFIOPCIDevice *vdev,
> -                                 Error **errp G_GNUC_UNUSED)
> +bool vfio_probe_igd_config_quirk(VFIOPCIDevice *vdev, Error **errp)
>  {
> -    g_autofree struct vfio_region_info *rom = NULL;
>      int ret, gen;
>      uint64_t gms_size;
>      uint64_t *bdsm_size;
>      uint32_t gmch;
> +    bool legacy_mode_enabled = false;
>      Error *err = NULL;
>  
>      /*
> @@ -498,9 +496,7 @@ bool vfio_probe_igd_config_quirk(VFIOPCIDevice *vdev,
>       * PCI bus address.
>       */
>      if (!vfio_pci_is(vdev, PCI_VENDOR_ID_INTEL, PCI_ANY_ID) ||
> -        !vfio_is_vga(vdev) ||
> -        &vdev->pdev != pci_find_device(pci_device_root_bus(&vdev->pdev),
> -                                       0, PCI_DEVFN(0x2, 0))) {
> +        !vfio_is_vga(vdev)) {
>          return true;
>      }
>  
> @@ -516,56 +512,67 @@ bool vfio_probe_igd_config_quirk(VFIOPCIDevice *vdev,
>          return true;
>      }
>  
> -    /*
> -     * Most of what we're doing here is to enable the ROM to run, so if
> -     * there's no ROM, there's no point in setting up this quirk.
> -     * NB. We only seem to get BIOS ROMs, so a UEFI VM would need CSM support.
> -     */
> -    ret = vfio_get_region_info(&vdev->vbasedev,
> -                               VFIO_PCI_ROM_REGION_INDEX, &rom);
> -    if ((ret || !rom->size) && !vdev->pdev.romfile) {
> -        error_report("IGD device %s has no ROM, legacy mode disabled",
> -                     vdev->vbasedev.name);
> -        return true;
> -    }
> -
> -    /*
> -     * Ignore the hotplug corner case, mark the ROM failed, we can't
> -     * create the devices we need for legacy mode in the hotplug scenario.
> -     */
> -    if (vdev->pdev.qdev.hotplugged) {
> -        error_report("IGD device %s hotplugged, ROM disabled, "
> -                     "legacy mode disabled", vdev->vbasedev.name);
> -        vdev->rom_read_failed = true;
> -        return true;
> -    }
> -
>      gmch = vfio_pci_read_config(&vdev->pdev, IGD_GMCH, 4);
>  
>      /*
> -     * If IGD VGA Disable is clear (expected) and VGA is not already enabled,
> -     * try to enable it.  Probably shouldn't be using legacy mode without VGA,
> -     * but also no point in us enabling VGA if disabled in hardware.
> +     * For backward compatibilty, enable legacy mode when
> +     * - Machine type is i440fx (pc_piix)
> +     * - IGD device is at guest BDF 00:02.0
> +     * - Not manually disabled by x-igd-legacy-mode=off
>       */
> -    if (!(gmch & 0x2) && !vdev->vga && !vfio_populate_vga(vdev, &err)) {
> -        error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
> -        error_report("IGD device %s failed to enable VGA access, "
> -                     "legacy mode disabled", vdev->vbasedev.name);
> -        return true;
> -    }
> +    if ((vdev->igd_legacy_mode != ON_OFF_AUTO_OFF) &&
> +        !strcmp(MACHINE_GET_CLASS(qdev_get_machine())->family, "pc_piix") &&
> +        (&vdev->pdev == pci_find_device(pci_device_root_bus(&vdev->pdev),
> +        0, PCI_DEVFN(0x2, 0)))) {
> +        /*
> +         * IGD legacy mode requires:
> +         * - VBIOS in ROM BAR or file
> +         * - VGA IO/MMIO ranges are claimed by IGD
> +         * - OpRegion
> +         * - Same LPC bridge and Host bridge VID/DID/SVID/SSID as host
> +         */
> +        g_autofree struct vfio_region_info *rom = NULL;
> +
> +        legacy_mode_enabled = true;
> +        warn_report("IGD legacy mode enabled, "
> +                    "use x-igd-legacy-mode=off to disable it if unwanted.");

info_report() maybe?  These aren't great choices for a user, get a
warning for using the intended mode or silence that warning by
requiring experimental options that taint the VM relative to libvirt.

Also, why do we list all the requirements then only test a few of them
before declaring we're using legacy mode?  It seems like the above
should be moved to the end of this branch.

Given the pending release deadline, maybe these aren't blockers, but
let's follow-up with something that assumes the user wants something
other than what they're doing.  Thanks,

Alex

> +
> +        /*
> +         * Most of what we're doing here is to enable the ROM to run, so if
> +         * there's no ROM, there's no point in setting up this quirk.
> +         * NB. We only seem to get BIOS ROMs, so UEFI VM would need CSM support.
> +         */
> +        ret = vfio_get_region_info(&vdev->vbasedev,
> +                                   VFIO_PCI_ROM_REGION_INDEX, &rom);
> +        if ((ret || !rom->size) && !vdev->pdev.romfile) {
> +            error_setg(&err, "Device has no ROM");
> +            goto error;
> +        }
>  
> -    /* Setup OpRegion access */
> -    if (!vfio_pci_igd_setup_opregion(vdev, &err)) {
> -        error_append_hint(&err, "IGD legacy mode disabled\n");
> -        error_report_err(err);
> -        return true;
> -    }
> +        /*
> +         * If IGD VGA Disable is clear (expected) and VGA is not already
> +         * enabled, try to enable it. Probably shouldn't be using legacy mode
> +         * without VGA, but also no point in us enabling VGA if disabled in
> +         * hardware.
> +         */
> +        if (!(gmch & 0x2) && !vdev->vga && !vfio_populate_vga(vdev, &err)) {
> +            error_setg(&err, "Unable to enable VGA access");
> +            goto error;
> +        }
>  
> -    /* Setup LPC bridge / Host bridge PCI IDs */
> -    if (!vfio_pci_igd_setup_lpc_bridge(vdev, &err)) {
> -        error_append_hint(&err, "IGD legacy mode disabled\n");
> -        error_report_err(err);
> -        return true;
> +        /* Setup OpRegion access */
> +        if (!vfio_pci_igd_setup_opregion(vdev, &err)) {
> +            goto error;
> +        }
> +
> +        /* Setup LPC bridge / Host bridge PCI IDs */
> +        if (!vfio_pci_igd_setup_lpc_bridge(vdev, &err)) {
> +            goto error;
> +        }
> +    } else if (vdev->igd_legacy_mode == ON_OFF_AUTO_ON) {
> +        error_setg(&err,
> +                   "Machine is not i440fx or assigned BDF is not 00:02.0");
> +        goto error;
>      }
>  
>      /*
> @@ -627,4 +634,18 @@ bool vfio_probe_igd_config_quirk(VFIOPCIDevice *vdev,
>      trace_vfio_pci_igd_bdsm_enabled(vdev->vbasedev.name, (gms_size / MiB));
>  
>      return true;
> +
> +error:
> +    /*
> +     * When legacy mode is implicity enabled, continue on error,
> +     * to keep compatibility
> +     */
> +    if (legacy_mode_enabled && (vdev->igd_legacy_mode == ON_OFF_AUTO_AUTO)) {
> +        error_report_err(err);
> +        error_report("IGD legacy mode disabled");
> +        return true;
> +    }
> +
> +    error_propagate(errp, err);
> +    return false;
>  }
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index a58d555934..d5ff8c1ea8 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3363,6 +3363,8 @@ static const Property vfio_pci_dev_properties[] = {
>                      VFIO_FEATURE_ENABLE_REQ_BIT, true),
>      DEFINE_PROP_BIT("x-igd-opregion", VFIOPCIDevice, features,
>                      VFIO_FEATURE_ENABLE_IGD_OPREGION_BIT, false),
> +    DEFINE_PROP_ON_OFF_AUTO("x-igd-legacy-mode", VFIOPCIDevice,
> +                            igd_legacy_mode, ON_OFF_AUTO_AUTO),
>      DEFINE_PROP_ON_OFF_AUTO("enable-migration", VFIOPCIDevice,
>                              vbasedev.enable_migration, ON_OFF_AUTO_AUTO),
>      DEFINE_PROP_BOOL("migration-events", VFIOPCIDevice,
> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> index 2e81f9eb19..aa67ceb346 100644
> --- a/hw/vfio/pci.h
> +++ b/hw/vfio/pci.h
> @@ -158,6 +158,7 @@ struct VFIOPCIDevice {
>      uint32_t display_xres;
>      uint32_t display_yres;
>      int32_t bootindex;
> +    OnOffAuto igd_legacy_mode;
>      uint32_t igd_gms;
>      OffAutoPCIBAR msix_relo;
>      uint8_t pm_cap;

Re: [PATCH v3 00/10] vfio/igd: Decoupling quirks with legacy mode

[Alex Williamson]

On Fri,  7 Mar 2025 02:01:20 +0800
Tomita Moeko <tomitamoeko@gmail.com> wrote:

> This patchset intends to decouple existing quirks from legacy mode.
> Currently all quirks depends on legacy mode (except x-igd-opregion),
> which includes following conditions:
> * Machine type is i440fx
> * IGD device is at guest BDF 00:02.0
> * VBIOS in ROM BAR or file
> * VGA IO/MMIO ranges are claimed by IGD
> * OpRegion
> * Same LPC bridge and Host bridge VID/DID/SVID/SSID as host
> 
> If one of the condition is not met, the quirks will not be applied.
> However, for recent generations, espcially Gen 11+ devices that removed
> VBIOS support, not all the conditions are required. For example, on EFI-
> based systems, VBIOS ROM is unnecessary, and VGA ranges are not used.
> 
> To have better support on newer UEFI-based VMs, this patchset makes the
> quirks independent of legacy mode. The BDSM and GGC register quirks are
> applied to all supported IGD devices, new x-igd-lpc option for the LPC
> bridge / Host bridge ID quirk is introduced for possible Q35 support.
> It also prepares for supporting IGD passthrough when it is not primary
> display later (kernel change will be merged in 6.15).
> 
> To maintain backward compatbility with exising configuration, legacy
> mode will automatically be enabled when:
> * Machine type is i440fx
> * IGD device is at guest BDF 00:02.0
> If the legacy mode behavior is unwanted, option x-igd-legacy-mode=off
> is provided for users to disable it. Setting x-igd-legacy-mode=on checks
> if the condition above are met, and set up all the required quirks.
> 
> The default value is x-igd-legacy-mode=auto. When the above conditions
> are all met, it acts as before and continues on any error. Otherwise it
> is equivalent to x-igd-legacy-mode=off.
> 
> When x-igd-legacy-mode is set to on or off, QEMU will fail immediately
> on error.
> 
> The first 2 patches of this patchset was taken from a previous one,
> details can be found at:
> https://lore.kernel.org/all/20250124191245.12464-1-tomitamoeko@gmail.com/
> 
> This patchest was mainly tested on Alder Lake UHD770, with Debian 12
> (kernel 6.1), Windows 11 (driver 32.0.101.6458) and Intel GOP driver
> 17.0.1081.
> 
> Due to I caught COVID these days, I am unable to deliver the proposed
> documentation update before QEMU 10.0 soft-freeze next week. Please help
> decide whether to make this patch series in 10.0 or next release.
> 
> Changelog:
> v3:
> * Use OnOffAuto for x-igd-legacy-mode option, default is auto, to keep
>   current behavior.
> * Added a new patch to solve the possible KVMGT/GVT-g fail.
> Link: https://lore.kernel.org/all/20250303175220.74917-1-tomitamoeko@gmail.com/

See comment on 07/ but otherwise works for me with GVT-d and GVT-g on
my i7-7700:

Reviewed-by: Alex Williamson <alex.williamson@redhat.com>
Tested-by: Alex Williamson <alex.williamson@redhat.com>

Re: [PATCH v3 00/10] vfio/igd: Decoupling quirks with legacy mode

[Corvin Köhne]

[-- Attachment #1: Type: text/plain, Size: 3200 bytes --]

On Thu, 2025-03-06 at 15:49 -0700, Alex Williamson wrote:
> On Fri,  7 Mar 2025 02:01:20 +0800
> Tomita Moeko <tomitamoeko@gmail.com> wrote:
> 
> > This patchset intends to decouple existing quirks from legacy mode.
> > Currently all quirks depends on legacy mode (except x-igd-opregion),
> > which includes following conditions:
> > * Machine type is i440fx
> > * IGD device is at guest BDF 00:02.0
> > * VBIOS in ROM BAR or file
> > * VGA IO/MMIO ranges are claimed by IGD
> > * OpRegion
> > * Same LPC bridge and Host bridge VID/DID/SVID/SSID as host
> > 
> > If one of the condition is not met, the quirks will not be applied.
> > However, for recent generations, espcially Gen 11+ devices that removed
> > VBIOS support, not all the conditions are required. For example, on EFI-
> > based systems, VBIOS ROM is unnecessary, and VGA ranges are not used.
> > 
> > To have better support on newer UEFI-based VMs, this patchset makes the
> > quirks independent of legacy mode. The BDSM and GGC register quirks are
> > applied to all supported IGD devices, new x-igd-lpc option for the LPC
> > bridge / Host bridge ID quirk is introduced for possible Q35 support.
> > It also prepares for supporting IGD passthrough when it is not primary
> > display later (kernel change will be merged in 6.15).
> > 
> > To maintain backward compatbility with exising configuration, legacy
> > mode will automatically be enabled when:
> > * Machine type is i440fx
> > * IGD device is at guest BDF 00:02.0
> > If the legacy mode behavior is unwanted, option x-igd-legacy-mode=off
> > is provided for users to disable it. Setting x-igd-legacy-mode=on checks
> > if the condition above are met, and set up all the required quirks.
> > 
> > The default value is x-igd-legacy-mode=auto. When the above conditions
> > are all met, it acts as before and continues on any error. Otherwise it
> > is equivalent to x-igd-legacy-mode=off.
> > 
> > When x-igd-legacy-mode is set to on or off, QEMU will fail immediately
> > on error.
> > 
> > The first 2 patches of this patchset was taken from a previous one,
> > details can be found at:
> > https://lore.kernel.org/all/20250124191245.12464-1-tomitamoeko@gmail.com/
> > 
> > This patchest was mainly tested on Alder Lake UHD770, with Debian 12
> > (kernel 6.1), Windows 11 (driver 32.0.101.6458) and Intel GOP driver
> > 17.0.1081.
> > 
> > Due to I caught COVID these days, I am unable to deliver the proposed
> > documentation update before QEMU 10.0 soft-freeze next week. Please help
> > decide whether to make this patch series in 10.0 or next release.
> > 
> > Changelog:
> > v3:
> > * Use OnOffAuto for x-igd-legacy-mode option, default is auto, to keep
> >   current behavior.
> > * Added a new patch to solve the possible KVMGT/GVT-g fail.
> > Link:
> > https://lore.kernel.org/all/20250303175220.74917-1-tomitamoeko@gmail.com/
> 
> See comment on 07/ but otherwise works for me with GVT-d and GVT-g on
> my i7-7700:
> 
> Reviewed-by: Alex Williamson <alex.williamson@redhat.com>
> Tested-by: Alex Williamson <alex.williamson@redhat.com>
> 

Reviewed-by: Corvin Köhne <c.koehne@beckhoff.com>


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 00/10] vfio/igd: Decoupling quirks with legacy mode

[Cédric Le Goater]

On 3/6/25 19:01, Tomita Moeko wrote:
> This patchset intends to decouple existing quirks from legacy mode.
> Currently all quirks depends on legacy mode (except x-igd-opregion),
> which includes following conditions:
> * Machine type is i440fx
> * IGD device is at guest BDF 00:02.0
> * VBIOS in ROM BAR or file
> * VGA IO/MMIO ranges are claimed by IGD
> * OpRegion
> * Same LPC bridge and Host bridge VID/DID/SVID/SSID as host
> 
> If one of the condition is not met, the quirks will not be applied.
> However, for recent generations, espcially Gen 11+ devices that removed
> VBIOS support, not all the conditions are required. For example, on EFI-
> based systems, VBIOS ROM is unnecessary, and VGA ranges are not used.
> 
> To have better support on newer UEFI-based VMs, this patchset makes the
> quirks independent of legacy mode. The BDSM and GGC register quirks are
> applied to all supported IGD devices, new x-igd-lpc option for the LPC
> bridge / Host bridge ID quirk is introduced for possible Q35 support.
> It also prepares for supporting IGD passthrough when it is not primary
> display later (kernel change will be merged in 6.15).
> 
> To maintain backward compatbility with exising configuration, legacy
> mode will automatically be enabled when:
> * Machine type is i440fx
> * IGD device is at guest BDF 00:02.0
> If the legacy mode behavior is unwanted, option x-igd-legacy-mode=off
> is provided for users to disable it. Setting x-igd-legacy-mode=on checks
> if the condition above are met, and set up all the required quirks.
> 
> The default value is x-igd-legacy-mode=auto. When the above conditions
> are all met, it acts as before and continues on any error. Otherwise it
> is equivalent to x-igd-legacy-mode=off.
> 
> When x-igd-legacy-mode is set to on or off, QEMU will fail immediately
> on error.
> 
> The first 2 patches of this patchset was taken from a previous one,
> details can be found at:
> https://lore.kernel.org/all/20250124191245.12464-1-tomitamoeko@gmail.com/
> 
> This patchest was mainly tested on Alder Lake UHD770, with Debian 12
> (kernel 6.1), Windows 11 (driver 32.0.101.6458) and Intel GOP driver
> 17.0.1081.
> 
> Due to I caught COVID these days, I am unable to deliver the proposed
> documentation update before QEMU 10.0 soft-freeze next week. Please help
> decide whether to make this patch series in 10.0 or next release.
> 
> Changelog:
> v3:
> * Use OnOffAuto for x-igd-legacy-mode option, default is auto, to keep
>    current behavior.
> * Added a new patch to solve the possible KVMGT/GVT-g fail.
> Link: https://lore.kernel.org/all/20250303175220.74917-1-tomitamoeko@gmail.com/
> 
> v2:
> * Keep legacy mode for compatbility
> * Renamed from "vfio/igd: Remove legacy mode"
> Link: https://lore.kernel.org/all/20250224182927.31519-1-tomitamoeko@gmail.com/
> 
> Tomita Moeko (10):
>    vfio/igd: Remove GTT write quirk in IO BAR 4
>    vfio/igd: Do not include GTT stolen size in etc/igd-bdsm-size
>    vfio/igd: Consolidate OpRegion initialization into a single function
>    vfio/igd: Move LPC bridge initialization to a separate function
>    vfio/pci: Add placeholder for device-specific config space quirks
>    vfio/igd: Refactor vfio_probe_igd_bar4_quirk into pci config quirk
>    vfio/igd: Decouple common quirks from legacy mode
>    vfio/igd: Handle x-igd-opregion option in config quirk
>    vfio/igd: Introduce x-igd-lpc option for LPC bridge ID quirk
>    vfio/igd: Fix broken KVMGT OpRegion support
> 
>   hw/vfio/igd.c        | 529 +++++++++++++++++++------------------------
>   hw/vfio/pci-quirks.c |  51 +----
>   hw/vfio/pci.c        |  33 +--
>   hw/vfio/pci.h        |  11 +-
>   4 files changed, 248 insertions(+), 376 deletions(-)
> 


Applied to vfio-next.

Thanks,

C.

Re: [PATCH v3 07/10] vfio/igd: Decouple common quirks from legacy mode

[Tomita Moeko]

On 2025/3/7 6:49, Alex Williamson wrote:
> On Fri,  7 Mar 2025 02:01:27 +0800
> Tomita Moeko <tomitamoeko@gmail.com> wrote:
> 
>> So far, IGD-specific quirks all require enabling legacy mode, which is
>> toggled by assigning IGD to 00:02.0. However, some quirks, like the BDSM
>> and GGC register quirks, should be applied to all supported IGD devices.
>> A new config option, x-igd-legacy-mode=[on|off|auto], is introduced to
>> control the legacy mode only quirks. The default value is "auto", which
>> keeps current behavior that enables legacy mode implicitly and continues
>> on error when all following conditions are met.
>> * Machine type is i440fx
>> * IGD device is at guest BDF 00:02.0
>>
>> If any one of the conditions above is not met, the default behavior is
>> equivalent to "off", QEMU will fail immediately if any error occurs.
>>
>> Users can also use "on" to force enabling legacy mode. It checks if all
>> the conditions above are met and set up legacy mode. QEMU will also fail
>> immediately on error in this case.
>>
>> Additionally, the hotplug check in legacy mode is removed as hotplugging
>> IGD device is never supported, and it will be checked when enabling the
>> OpRegion quirk.
>>
>> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
>> ---
>>  hw/vfio/igd.c | 127 +++++++++++++++++++++++++++++---------------------
>>  hw/vfio/pci.c |   2 +
>>  hw/vfio/pci.h |   1 +
>>  3 files changed, 77 insertions(+), 53 deletions(-)
>>
>> diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
>> index f5e19f1241..ac096e2eb5 100644
>> --- a/hw/vfio/igd.c
>> +++ b/hw/vfio/igd.c
>> @@ -15,6 +15,7 @@
>>  #include "qemu/error-report.h"
>>  #include "qapi/error.h"
>>  #include "qapi/qmp/qerror.h"
>> +#include "hw/boards.h"
>>  #include "hw/hw.h"
>>  #include "hw/nvram/fw_cfg.h"
>>  #include "pci.h"
>> @@ -432,9 +433,7 @@ void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int nr)
>>       * bus address.
>>       */
>>      if (!vfio_pci_is(vdev, PCI_VENDOR_ID_INTEL, PCI_ANY_ID) ||
>> -        !vfio_is_vga(vdev) || nr != 0 ||
>> -        &vdev->pdev != pci_find_device(pci_device_root_bus(&vdev->pdev),
>> -                                       0, PCI_DEVFN(0x2, 0))) {
>> +        !vfio_is_vga(vdev) || nr != 0) {
>>          return;
>>      }
>>  
>> @@ -482,14 +481,13 @@ void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int nr)
>>      QLIST_INSERT_HEAD(&vdev->bars[nr].quirks, bdsm_quirk, next);
>>  }
>>  
>> -bool vfio_probe_igd_config_quirk(VFIOPCIDevice *vdev,
>> -                                 Error **errp G_GNUC_UNUSED)
>> +bool vfio_probe_igd_config_quirk(VFIOPCIDevice *vdev, Error **errp)
>>  {
>> -    g_autofree struct vfio_region_info *rom = NULL;
>>      int ret, gen;
>>      uint64_t gms_size;
>>      uint64_t *bdsm_size;
>>      uint32_t gmch;
>> +    bool legacy_mode_enabled = false;
>>      Error *err = NULL;
>>  
>>      /*
>> @@ -498,9 +496,7 @@ bool vfio_probe_igd_config_quirk(VFIOPCIDevice *vdev,
>>       * PCI bus address.
>>       */
>>      if (!vfio_pci_is(vdev, PCI_VENDOR_ID_INTEL, PCI_ANY_ID) ||
>> -        !vfio_is_vga(vdev) ||
>> -        &vdev->pdev != pci_find_device(pci_device_root_bus(&vdev->pdev),
>> -                                       0, PCI_DEVFN(0x2, 0))) {
>> +        !vfio_is_vga(vdev)) {
>>          return true;
>>      }
>>  
>> @@ -516,56 +512,67 @@ bool vfio_probe_igd_config_quirk(VFIOPCIDevice *vdev,
>>          return true;
>>      }
>>  
>> -    /*
>> -     * Most of what we're doing here is to enable the ROM to run, so if
>> -     * there's no ROM, there's no point in setting up this quirk.
>> -     * NB. We only seem to get BIOS ROMs, so a UEFI VM would need CSM support.
>> -     */
>> -    ret = vfio_get_region_info(&vdev->vbasedev,
>> -                               VFIO_PCI_ROM_REGION_INDEX, &rom);
>> -    if ((ret || !rom->size) && !vdev->pdev.romfile) {
>> -        error_report("IGD device %s has no ROM, legacy mode disabled",
>> -                     vdev->vbasedev.name);
>> -        return true;
>> -    }
>> -
>> -    /*
>> -     * Ignore the hotplug corner case, mark the ROM failed, we can't
>> -     * create the devices we need for legacy mode in the hotplug scenario.
>> -     */
>> -    if (vdev->pdev.qdev.hotplugged) {
>> -        error_report("IGD device %s hotplugged, ROM disabled, "
>> -                     "legacy mode disabled", vdev->vbasedev.name);
>> -        vdev->rom_read_failed = true;
>> -        return true;
>> -    }
>> -
>>      gmch = vfio_pci_read_config(&vdev->pdev, IGD_GMCH, 4);
>>  
>>      /*
>> -     * If IGD VGA Disable is clear (expected) and VGA is not already enabled,
>> -     * try to enable it.  Probably shouldn't be using legacy mode without VGA,
>> -     * but also no point in us enabling VGA if disabled in hardware.
>> +     * For backward compatibilty, enable legacy mode when
>> +     * - Machine type is i440fx (pc_piix)
>> +     * - IGD device is at guest BDF 00:02.0
>> +     * - Not manually disabled by x-igd-legacy-mode=off
>>       */
>> -    if (!(gmch & 0x2) && !vdev->vga && !vfio_populate_vga(vdev, &err)) {
>> -        error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
>> -        error_report("IGD device %s failed to enable VGA access, "
>> -                     "legacy mode disabled", vdev->vbasedev.name);
>> -        return true;
>> -    }
>> +    if ((vdev->igd_legacy_mode != ON_OFF_AUTO_OFF) &&
>> +        !strcmp(MACHINE_GET_CLASS(qdev_get_machine())->family, "pc_piix") &&
>> +        (&vdev->pdev == pci_find_device(pci_device_root_bus(&vdev->pdev),
>> +        0, PCI_DEVFN(0x2, 0)))) {
>> +        /*
>> +         * IGD legacy mode requires:
>> +         * - VBIOS in ROM BAR or file
>> +         * - VGA IO/MMIO ranges are claimed by IGD
>> +         * - OpRegion
>> +         * - Same LPC bridge and Host bridge VID/DID/SVID/SSID as host
>> +         */
>> +        g_autofree struct vfio_region_info *rom = NULL;
>> +
>> +        legacy_mode_enabled = true;
>> +        warn_report("IGD legacy mode enabled, "
>> +                    "use x-igd-legacy-mode=off to disable it if unwanted.");
> 
> info_report() maybe?  These aren't great choices for a user, get a
> warning for using the intended mode or silence that warning by
> requiring experimental options that taint the VM relative to libvirt.

Got it. If possible, please help change this to info_report when
applying.

> Also, why do we list all the requirements then only test a few of them
> before declaring we're using legacy mode?  It seems like the above
> should be moved to the end of this branch.

Having i440fx machine type and BDF 00:02.0 are the must-have condition
for considering enabling legacy mode, while others are the requirements
for legacy mode itself. In this verison, legacy mode is equivalent to
    romfile=oprom.bin,x-vga=on,x-igd-opregion=on,x-igd-lpc=on

BDF being 00:02.0 is a requirement of VBIOS, and hacking the LPC DID
currently only works on i440fx. Though for Q35, we can overwrite the
existing ICH9 LPC DID to make IGD driver happy, it won't break ACPI PM
in recent guest kernel, and maybe later making it an option for Q35,
its still too risky to make it an implicit default. That's why I prefer
to check the must-have condtions first, then check other requirements
when setting up them.  

Setting the `legacy_mode_enabled` flag here is for error handling, as
we have to keep the old "continue on error" behavior for legacy mode.

> Given the pending release deadline, maybe these aren't blockers, but
> let's follow-up with something that assumes the user wants something
> other than what they're doing.  Thanks,
> 
> Alex
>

Sure.

Moeko
 
>> +
>> +        /*
>> +         * Most of what we're doing here is to enable the ROM to run, so if
>> +         * there's no ROM, there's no point in setting up this quirk.
>> +         * NB. We only seem to get BIOS ROMs, so UEFI VM would need CSM support.
>> +         */
>> +        ret = vfio_get_region_info(&vdev->vbasedev,
>> +                                   VFIO_PCI_ROM_REGION_INDEX, &rom);
>> +        if ((ret || !rom->size) && !vdev->pdev.romfile) {
>> +            error_setg(&err, "Device has no ROM");
>> +            goto error;
>> +        }
>>  
>> -    /* Setup OpRegion access */
>> -    if (!vfio_pci_igd_setup_opregion(vdev, &err)) {
>> -        error_append_hint(&err, "IGD legacy mode disabled\n");
>> -        error_report_err(err);
>> -        return true;
>> -    }
>> +        /*
>> +         * If IGD VGA Disable is clear (expected) and VGA is not already
>> +         * enabled, try to enable it. Probably shouldn't be using legacy mode
>> +         * without VGA, but also no point in us enabling VGA if disabled in
>> +         * hardware.
>> +         */
>> +        if (!(gmch & 0x2) && !vdev->vga && !vfio_populate_vga(vdev, &err)) {
>> +            error_setg(&err, "Unable to enable VGA access");
>> +            goto error;
>> +        }
>>  
>> -    /* Setup LPC bridge / Host bridge PCI IDs */
>> -    if (!vfio_pci_igd_setup_lpc_bridge(vdev, &err)) {
>> -        error_append_hint(&err, "IGD legacy mode disabled\n");
>> -        error_report_err(err);
>> -        return true;
>> +        /* Setup OpRegion access */
>> +        if (!vfio_pci_igd_setup_opregion(vdev, &err)) {
>> +            goto error;
>> +        }
>> +
>> +        /* Setup LPC bridge / Host bridge PCI IDs */
>> +        if (!vfio_pci_igd_setup_lpc_bridge(vdev, &err)) {
>> +            goto error;
>> +        }
>> +    } else if (vdev->igd_legacy_mode == ON_OFF_AUTO_ON) {
>> +        error_setg(&err,
>> +                   "Machine is not i440fx or assigned BDF is not 00:02.0");
>> +        goto error;
>>      }
>>  
>>      /*
>> @@ -627,4 +634,18 @@ bool vfio_probe_igd_config_quirk(VFIOPCIDevice *vdev,
>>      trace_vfio_pci_igd_bdsm_enabled(vdev->vbasedev.name, (gms_size / MiB));
>>  
>>      return true;
>> +
>> +error:
>> +    /*
>> +     * When legacy mode is implicity enabled, continue on error,
>> +     * to keep compatibility
>> +     */
>> +    if (legacy_mode_enabled && (vdev->igd_legacy_mode == ON_OFF_AUTO_AUTO)) {
>> +        error_report_err(err);
>> +        error_report("IGD legacy mode disabled");
>> +        return true;
>> +    }
>> +
>> +    error_propagate(errp, err);
>> +    return false;
>>  }
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index a58d555934..d5ff8c1ea8 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -3363,6 +3363,8 @@ static const Property vfio_pci_dev_properties[] = {
>>                      VFIO_FEATURE_ENABLE_REQ_BIT, true),
>>      DEFINE_PROP_BIT("x-igd-opregion", VFIOPCIDevice, features,
>>                      VFIO_FEATURE_ENABLE_IGD_OPREGION_BIT, false),
>> +    DEFINE_PROP_ON_OFF_AUTO("x-igd-legacy-mode", VFIOPCIDevice,
>> +                            igd_legacy_mode, ON_OFF_AUTO_AUTO),
>>      DEFINE_PROP_ON_OFF_AUTO("enable-migration", VFIOPCIDevice,
>>                              vbasedev.enable_migration, ON_OFF_AUTO_AUTO),
>>      DEFINE_PROP_BOOL("migration-events", VFIOPCIDevice,
>> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
>> index 2e81f9eb19..aa67ceb346 100644
>> --- a/hw/vfio/pci.h
>> +++ b/hw/vfio/pci.h
>> @@ -158,6 +158,7 @@ struct VFIOPCIDevice {
>>      uint32_t display_xres;
>>      uint32_t display_yres;
>>      int32_t bootindex;
>> +    OnOffAuto igd_legacy_mode;
>>      uint32_t igd_gms;
>>      OffAutoPCIBAR msix_relo;
>>      uint8_t pm_cap;
> 

Re: [PATCH v3 07/10] vfio/igd: Decouple common quirks from legacy mode

[Cédric Le Goater]

Tomita,

On 3/7/25 19:37, Tomita Moeko wrote:
> On 2025/3/7 6:49, Alex Williamson wrote:
>> On Fri,  7 Mar 2025 02:01:27 +0800
>> Tomita Moeko <tomitamoeko@gmail.com> wrote:
>>
>>> So far, IGD-specific quirks all require enabling legacy mode, which is
>>> toggled by assigning IGD to 00:02.0. However, some quirks, like the BDSM
>>> and GGC register quirks, should be applied to all supported IGD devices.
>>> A new config option, x-igd-legacy-mode=[on|off|auto], is introduced to
>>> control the legacy mode only quirks. The default value is "auto", which
>>> keeps current behavior that enables legacy mode implicitly and continues
>>> on error when all following conditions are met.
>>> * Machine type is i440fx
>>> * IGD device is at guest BDF 00:02.0
>>>
>>> If any one of the conditions above is not met, the default behavior is
>>> equivalent to "off", QEMU will fail immediately if any error occurs.
>>>
>>> Users can also use "on" to force enabling legacy mode. It checks if all
>>> the conditions above are met and set up legacy mode. QEMU will also fail
>>> immediately on error in this case.
>>>
>>> Additionally, the hotplug check in legacy mode is removed as hotplugging
>>> IGD device is never supported, and it will be checked when enabling the
>>> OpRegion quirk.
>>>
>>> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
>>> ---
>>>   hw/vfio/igd.c | 127 +++++++++++++++++++++++++++++---------------------
>>>   hw/vfio/pci.c |   2 +
>>>   hw/vfio/pci.h |   1 +
>>>   3 files changed, 77 insertions(+), 53 deletions(-)
>>>
>>> diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
>>> index f5e19f1241..ac096e2eb5 100644
>>> --- a/hw/vfio/igd.c
>>> +++ b/hw/vfio/igd.c
>>> @@ -15,6 +15,7 @@
>>>   #include "qemu/error-report.h"
>>>   #include "qapi/error.h"
>>>   #include "qapi/qmp/qerror.h"
>>> +#include "hw/boards.h"
>>>   #include "hw/hw.h"
>>>   #include "hw/nvram/fw_cfg.h"
>>>   #include "pci.h"
>>> @@ -432,9 +433,7 @@ void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int nr)
>>>        * bus address.
>>>        */
>>>       if (!vfio_pci_is(vdev, PCI_VENDOR_ID_INTEL, PCI_ANY_ID) ||
>>> -        !vfio_is_vga(vdev) || nr != 0 ||
>>> -        &vdev->pdev != pci_find_device(pci_device_root_bus(&vdev->pdev),
>>> -                                       0, PCI_DEVFN(0x2, 0))) {
>>> +        !vfio_is_vga(vdev) || nr != 0) {
>>>           return;
>>>       }
>>>   
>>> @@ -482,14 +481,13 @@ void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int nr)
>>>       QLIST_INSERT_HEAD(&vdev->bars[nr].quirks, bdsm_quirk, next);
>>>   }
>>>   
>>> -bool vfio_probe_igd_config_quirk(VFIOPCIDevice *vdev,
>>> -                                 Error **errp G_GNUC_UNUSED)
>>> +bool vfio_probe_igd_config_quirk(VFIOPCIDevice *vdev, Error **errp)
>>>   {
>>> -    g_autofree struct vfio_region_info *rom = NULL;
>>>       int ret, gen;
>>>       uint64_t gms_size;
>>>       uint64_t *bdsm_size;
>>>       uint32_t gmch;
>>> +    bool legacy_mode_enabled = false;
>>>       Error *err = NULL;
>>>   
>>>       /*
>>> @@ -498,9 +496,7 @@ bool vfio_probe_igd_config_quirk(VFIOPCIDevice *vdev,
>>>        * PCI bus address.
>>>        */
>>>       if (!vfio_pci_is(vdev, PCI_VENDOR_ID_INTEL, PCI_ANY_ID) ||
>>> -        !vfio_is_vga(vdev) ||
>>> -        &vdev->pdev != pci_find_device(pci_device_root_bus(&vdev->pdev),
>>> -                                       0, PCI_DEVFN(0x2, 0))) {
>>> +        !vfio_is_vga(vdev)) {
>>>           return true;
>>>       }
>>>   
>>> @@ -516,56 +512,67 @@ bool vfio_probe_igd_config_quirk(VFIOPCIDevice *vdev,
>>>           return true;
>>>       }
>>>   
>>> -    /*
>>> -     * Most of what we're doing here is to enable the ROM to run, so if
>>> -     * there's no ROM, there's no point in setting up this quirk.
>>> -     * NB. We only seem to get BIOS ROMs, so a UEFI VM would need CSM support.
>>> -     */
>>> -    ret = vfio_get_region_info(&vdev->vbasedev,
>>> -                               VFIO_PCI_ROM_REGION_INDEX, &rom);
>>> -    if ((ret || !rom->size) && !vdev->pdev.romfile) {
>>> -        error_report("IGD device %s has no ROM, legacy mode disabled",
>>> -                     vdev->vbasedev.name);
>>> -        return true;
>>> -    }
>>> -
>>> -    /*
>>> -     * Ignore the hotplug corner case, mark the ROM failed, we can't
>>> -     * create the devices we need for legacy mode in the hotplug scenario.
>>> -     */
>>> -    if (vdev->pdev.qdev.hotplugged) {
>>> -        error_report("IGD device %s hotplugged, ROM disabled, "
>>> -                     "legacy mode disabled", vdev->vbasedev.name);
>>> -        vdev->rom_read_failed = true;
>>> -        return true;
>>> -    }
>>> -
>>>       gmch = vfio_pci_read_config(&vdev->pdev, IGD_GMCH, 4);
>>>   
>>>       /*
>>> -     * If IGD VGA Disable is clear (expected) and VGA is not already enabled,
>>> -     * try to enable it.  Probably shouldn't be using legacy mode without VGA,
>>> -     * but also no point in us enabling VGA if disabled in hardware.
>>> +     * For backward compatibilty, enable legacy mode when
>>> +     * - Machine type is i440fx (pc_piix)
>>> +     * - IGD device is at guest BDF 00:02.0
>>> +     * - Not manually disabled by x-igd-legacy-mode=off
>>>        */
>>> -    if (!(gmch & 0x2) && !vdev->vga && !vfio_populate_vga(vdev, &err)) {
>>> -        error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
>>> -        error_report("IGD device %s failed to enable VGA access, "
>>> -                     "legacy mode disabled", vdev->vbasedev.name);
>>> -        return true;
>>> -    }
>>> +    if ((vdev->igd_legacy_mode != ON_OFF_AUTO_OFF) &&
>>> +        !strcmp(MACHINE_GET_CLASS(qdev_get_machine())->family, "pc_piix") &&
>>> +        (&vdev->pdev == pci_find_device(pci_device_root_bus(&vdev->pdev),
>>> +        0, PCI_DEVFN(0x2, 0)))) {
>>> +        /*
>>> +         * IGD legacy mode requires:
>>> +         * - VBIOS in ROM BAR or file
>>> +         * - VGA IO/MMIO ranges are claimed by IGD
>>> +         * - OpRegion
>>> +         * - Same LPC bridge and Host bridge VID/DID/SVID/SSID as host
>>> +         */
>>> +        g_autofree struct vfio_region_info *rom = NULL;
>>> +
>>> +        legacy_mode_enabled = true;
>>> +        warn_report("IGD legacy mode enabled, "
>>> +                    "use x-igd-legacy-mode=off to disable it if unwanted.");
>>
>> info_report() maybe?  These aren't great choices for a user, get a
>> warning for using the intended mode or silence that warning by
>> requiring experimental options that taint the VM relative to libvirt.
> 
> Got it. If possible, please help change this to info_report when
> applying.

done.


> 
>> Also, why do we list all the requirements then only test a few of them
>> before declaring we're using legacy mode?  It seems like the above
>> should be moved to the end of this branch.
> 
> Having i440fx machine type and BDF 00:02.0 are the must-have condition
> for considering enabling legacy mode, while others are the requirements
> for legacy mode itself. In this verison, legacy mode is equivalent to
>      romfile=oprom.bin,x-vga=on,x-igd-opregion=on,x-igd-lpc=on
> 
> BDF being 00:02.0 is a requirement of VBIOS, and hacking the LPC DID
> currently only works on i440fx. Though for Q35, we can overwrite the
> existing ICH9 LPC DID to make IGD driver happy, it won't break ACPI PM
> in recent guest kernel, and maybe later making it an option for Q35,
> its still too risky to make it an implicit default. That's why I prefer
> to check the must-have condtions first, then check other requirements
> when setting up them.
> 
> Setting the `legacy_mode_enabled` flag here is for error handling, as
> we have to keep the old "continue on error" behavior for legacy mode.

A ducomentation update is welcome ! Could you do that before hard-freeze ?

Thanks,

C.






>> Given the pending release deadline, maybe these aren't blockers, but
>> let's follow-up with something that assumes the user wants something
>> other than what they're doing.  Thanks,
>>
>> Alex
>>
> 
> Sure.
> 
> Moeko
>   
>>> +
>>> +        /*
>>> +         * Most of what we're doing here is to enable the ROM to run, so if
>>> +         * there's no ROM, there's no point in setting up this quirk.
>>> +         * NB. We only seem to get BIOS ROMs, so UEFI VM would need CSM support.
>>> +         */
>>> +        ret = vfio_get_region_info(&vdev->vbasedev,
>>> +                                   VFIO_PCI_ROM_REGION_INDEX, &rom);
>>> +        if ((ret || !rom->size) && !vdev->pdev.romfile) {
>>> +            error_setg(&err, "Device has no ROM");
>>> +            goto error;
>>> +        }
>>>   
>>> -    /* Setup OpRegion access */
>>> -    if (!vfio_pci_igd_setup_opregion(vdev, &err)) {
>>> -        error_append_hint(&err, "IGD legacy mode disabled\n");
>>> -        error_report_err(err);
>>> -        return true;
>>> -    }
>>> +        /*
>>> +         * If IGD VGA Disable is clear (expected) and VGA is not already
>>> +         * enabled, try to enable it. Probably shouldn't be using legacy mode
>>> +         * without VGA, but also no point in us enabling VGA if disabled in
>>> +         * hardware.
>>> +         */
>>> +        if (!(gmch & 0x2) && !vdev->vga && !vfio_populate_vga(vdev, &err)) {
>>> +            error_setg(&err, "Unable to enable VGA access");
>>> +            goto error;
>>> +        }
>>>   
>>> -    /* Setup LPC bridge / Host bridge PCI IDs */
>>> -    if (!vfio_pci_igd_setup_lpc_bridge(vdev, &err)) {
>>> -        error_append_hint(&err, "IGD legacy mode disabled\n");
>>> -        error_report_err(err);
>>> -        return true;
>>> +        /* Setup OpRegion access */
>>> +        if (!vfio_pci_igd_setup_opregion(vdev, &err)) {
>>> +            goto error;
>>> +        }
>>> +
>>> +        /* Setup LPC bridge / Host bridge PCI IDs */
>>> +        if (!vfio_pci_igd_setup_lpc_bridge(vdev, &err)) {
>>> +            goto error;
>>> +        }
>>> +    } else if (vdev->igd_legacy_mode == ON_OFF_AUTO_ON) {
>>> +        error_setg(&err,
>>> +                   "Machine is not i440fx or assigned BDF is not 00:02.0");
>>> +        goto error;
>>>       }
>>>   
>>>       /*
>>> @@ -627,4 +634,18 @@ bool vfio_probe_igd_config_quirk(VFIOPCIDevice *vdev,
>>>       trace_vfio_pci_igd_bdsm_enabled(vdev->vbasedev.name, (gms_size / MiB));
>>>   
>>>       return true;
>>> +
>>> +error:
>>> +    /*
>>> +     * When legacy mode is implicity enabled, continue on error,
>>> +     * to keep compatibility
>>> +     */
>>> +    if (legacy_mode_enabled && (vdev->igd_legacy_mode == ON_OFF_AUTO_AUTO)) {
>>> +        error_report_err(err);
>>> +        error_report("IGD legacy mode disabled");
>>> +        return true;
>>> +    }
>>> +
>>> +    error_propagate(errp, err);
>>> +    return false;
>>>   }
>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>>> index a58d555934..d5ff8c1ea8 100644
>>> --- a/hw/vfio/pci.c
>>> +++ b/hw/vfio/pci.c
>>> @@ -3363,6 +3363,8 @@ static const Property vfio_pci_dev_properties[] = {
>>>                       VFIO_FEATURE_ENABLE_REQ_BIT, true),
>>>       DEFINE_PROP_BIT("x-igd-opregion", VFIOPCIDevice, features,
>>>                       VFIO_FEATURE_ENABLE_IGD_OPREGION_BIT, false),
>>> +    DEFINE_PROP_ON_OFF_AUTO("x-igd-legacy-mode", VFIOPCIDevice,
>>> +                            igd_legacy_mode, ON_OFF_AUTO_AUTO),
>>>       DEFINE_PROP_ON_OFF_AUTO("enable-migration", VFIOPCIDevice,
>>>                               vbasedev.enable_migration, ON_OFF_AUTO_AUTO),
>>>       DEFINE_PROP_BOOL("migration-events", VFIOPCIDevice,
>>> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
>>> index 2e81f9eb19..aa67ceb346 100644
>>> --- a/hw/vfio/pci.h
>>> +++ b/hw/vfio/pci.h
>>> @@ -158,6 +158,7 @@ struct VFIOPCIDevice {
>>>       uint32_t display_xres;
>>>       uint32_t display_yres;
>>>       int32_t bootindex;
>>> +    OnOffAuto igd_legacy_mode;
>>>       uint32_t igd_gms;
>>>       OffAutoPCIBAR msix_relo;
>>>       uint8_t pm_cap;
>>
> 

Re: [PATCH v3 07/10] vfio/igd: Decouple common quirks from legacy mode

[Tomita Moeko]

On 2025/3/10 15:13, Cédric Le Goater wrote:
> Tomita,
> 
> On 3/7/25 19:37, Tomita Moeko wrote:
>> On 2025/3/7 6:49, Alex Williamson wrote:
>>> On Fri,  7 Mar 2025 02:01:27 +0800
>>> Tomita Moeko <tomitamoeko@gmail.com> wrote:
>>>
>>>> So far, IGD-specific quirks all require enabling legacy mode, which is
>>>> toggled by assigning IGD to 00:02.0. However, some quirks, like the BDSM
>>>> and GGC register quirks, should be applied to all supported IGD devices.
>>>> A new config option, x-igd-legacy-mode=[on|off|auto], is introduced to
>>>> control the legacy mode only quirks. The default value is "auto", which
>>>> keeps current behavior that enables legacy mode implicitly and continues
>>>> on error when all following conditions are met.
>>>> * Machine type is i440fx
>>>> * IGD device is at guest BDF 00:02.0
>>>>
>>>> If any one of the conditions above is not met, the default behavior is
>>>> equivalent to "off", QEMU will fail immediately if any error occurs.
>>>>
>>>> Users can also use "on" to force enabling legacy mode. It checks if all
>>>> the conditions above are met and set up legacy mode. QEMU will also fail
>>>> immediately on error in this case.
>>>>
>>>> Additionally, the hotplug check in legacy mode is removed as hotplugging
>>>> IGD device is never supported, and it will be checked when enabling the
>>>> OpRegion quirk.
>>>>
>>>> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
>>>> ---
>>>>   hw/vfio/igd.c | 127 +++++++++++++++++++++++++++++---------------------
>>>>   hw/vfio/pci.c |   2 +
>>>>   hw/vfio/pci.h |   1 +
>>>>   3 files changed, 77 insertions(+), 53 deletions(-)
>>>>
>>>> diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
>>>> index f5e19f1241..ac096e2eb5 100644
>>>> --- a/hw/vfio/igd.c
>>>> +++ b/hw/vfio/igd.c
>>>> @@ -15,6 +15,7 @@
>>>>   #include "qemu/error-report.h"
>>>>   #include "qapi/error.h"
>>>>   #include "qapi/qmp/qerror.h"
>>>> +#include "hw/boards.h"
>>>>   #include "hw/hw.h"
>>>>   #include "hw/nvram/fw_cfg.h"
>>>>   #include "pci.h"
>>>> @@ -432,9 +433,7 @@ void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int nr)
>>>>        * bus address.
>>>>        */
>>>>       if (!vfio_pci_is(vdev, PCI_VENDOR_ID_INTEL, PCI_ANY_ID) ||
>>>> -        !vfio_is_vga(vdev) || nr != 0 ||
>>>> -        &vdev->pdev != pci_find_device(pci_device_root_bus(&vdev->pdev),
>>>> -                                       0, PCI_DEVFN(0x2, 0))) {
>>>> +        !vfio_is_vga(vdev) || nr != 0) {
>>>>           return;
>>>>       }
>>>>   @@ -482,14 +481,13 @@ void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int nr)
>>>>       QLIST_INSERT_HEAD(&vdev->bars[nr].quirks, bdsm_quirk, next);
>>>>   }
>>>>   -bool vfio_probe_igd_config_quirk(VFIOPCIDevice *vdev,
>>>> -                                 Error **errp G_GNUC_UNUSED)
>>>> +bool vfio_probe_igd_config_quirk(VFIOPCIDevice *vdev, Error **errp)
>>>>   {
>>>> -    g_autofree struct vfio_region_info *rom = NULL;
>>>>       int ret, gen;
>>>>       uint64_t gms_size;
>>>>       uint64_t *bdsm_size;
>>>>       uint32_t gmch;
>>>> +    bool legacy_mode_enabled = false;
>>>>       Error *err = NULL;
>>>>         /*
>>>> @@ -498,9 +496,7 @@ bool vfio_probe_igd_config_quirk(VFIOPCIDevice *vdev,
>>>>        * PCI bus address.
>>>>        */
>>>>       if (!vfio_pci_is(vdev, PCI_VENDOR_ID_INTEL, PCI_ANY_ID) ||
>>>> -        !vfio_is_vga(vdev) ||
>>>> -        &vdev->pdev != pci_find_device(pci_device_root_bus(&vdev->pdev),
>>>> -                                       0, PCI_DEVFN(0x2, 0))) {
>>>> +        !vfio_is_vga(vdev)) {
>>>>           return true;
>>>>       }
>>>>   @@ -516,56 +512,67 @@ bool vfio_probe_igd_config_quirk(VFIOPCIDevice *vdev,
>>>>           return true;
>>>>       }
>>>>   -    /*
>>>> -     * Most of what we're doing here is to enable the ROM to run, so if
>>>> -     * there's no ROM, there's no point in setting up this quirk.
>>>> -     * NB. We only seem to get BIOS ROMs, so a UEFI VM would need CSM support.
>>>> -     */
>>>> -    ret = vfio_get_region_info(&vdev->vbasedev,
>>>> -                               VFIO_PCI_ROM_REGION_INDEX, &rom);
>>>> -    if ((ret || !rom->size) && !vdev->pdev.romfile) {
>>>> -        error_report("IGD device %s has no ROM, legacy mode disabled",
>>>> -                     vdev->vbasedev.name);
>>>> -        return true;
>>>> -    }
>>>> -
>>>> -    /*
>>>> -     * Ignore the hotplug corner case, mark the ROM failed, we can't
>>>> -     * create the devices we need for legacy mode in the hotplug scenario.
>>>> -     */
>>>> -    if (vdev->pdev.qdev.hotplugged) {
>>>> -        error_report("IGD device %s hotplugged, ROM disabled, "
>>>> -                     "legacy mode disabled", vdev->vbasedev.name);
>>>> -        vdev->rom_read_failed = true;
>>>> -        return true;
>>>> -    }
>>>> -
>>>>       gmch = vfio_pci_read_config(&vdev->pdev, IGD_GMCH, 4);
>>>>         /*
>>>> -     * If IGD VGA Disable is clear (expected) and VGA is not already enabled,
>>>> -     * try to enable it.  Probably shouldn't be using legacy mode without VGA,
>>>> -     * but also no point in us enabling VGA if disabled in hardware.
>>>> +     * For backward compatibilty, enable legacy mode when
>>>> +     * - Machine type is i440fx (pc_piix)
>>>> +     * - IGD device is at guest BDF 00:02.0
>>>> +     * - Not manually disabled by x-igd-legacy-mode=off
>>>>        */
>>>> -    if (!(gmch & 0x2) && !vdev->vga && !vfio_populate_vga(vdev, &err)) {
>>>> -        error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
>>>> -        error_report("IGD device %s failed to enable VGA access, "
>>>> -                     "legacy mode disabled", vdev->vbasedev.name);
>>>> -        return true;
>>>> -    }
>>>> +    if ((vdev->igd_legacy_mode != ON_OFF_AUTO_OFF) &&
>>>> +        !strcmp(MACHINE_GET_CLASS(qdev_get_machine())->family, "pc_piix") &&
>>>> +        (&vdev->pdev == pci_find_device(pci_device_root_bus(&vdev->pdev),
>>>> +        0, PCI_DEVFN(0x2, 0)))) {
>>>> +        /*
>>>> +         * IGD legacy mode requires:
>>>> +         * - VBIOS in ROM BAR or file
>>>> +         * - VGA IO/MMIO ranges are claimed by IGD
>>>> +         * - OpRegion
>>>> +         * - Same LPC bridge and Host bridge VID/DID/SVID/SSID as host
>>>> +         */
>>>> +        g_autofree struct vfio_region_info *rom = NULL;
>>>> +
>>>> +        legacy_mode_enabled = true;
>>>> +        warn_report("IGD legacy mode enabled, "
>>>> +                    "use x-igd-legacy-mode=off to disable it if unwanted.");
>>>
>>> info_report() maybe?  These aren't great choices for a user, get a
>>> warning for using the intended mode or silence that warning by
>>> requiring experimental options that taint the VM relative to libvirt.
>>
>> Got it. If possible, please help change this to info_report when
>> applying.
> 
> done.
> 
> 
>>
>>> Also, why do we list all the requirements then only test a few of them
>>> before declaring we're using legacy mode?  It seems like the above
>>> should be moved to the end of this branch.
>>
>> Having i440fx machine type and BDF 00:02.0 are the must-have condition
>> for considering enabling legacy mode, while others are the requirements
>> for legacy mode itself. In this verison, legacy mode is equivalent to
>>      romfile=oprom.bin,x-vga=on,x-igd-opregion=on,x-igd-lpc=on
>>
>> BDF being 00:02.0 is a requirement of VBIOS, and hacking the LPC DID
>> currently only works on i440fx. Though for Q35, we can overwrite the
>> existing ICH9 LPC DID to make IGD driver happy, it won't break ACPI PM
>> in recent guest kernel, and maybe later making it an option for Q35,
>> its still too risky to make it an implicit default. That's why I prefer
>> to check the must-have condtions first, then check other requirements
>> when setting up them.
>>
>> Setting the `legacy_mode_enabled` flag here is for error handling, as
>> we have to keep the old "continue on error" behavior for legacy mode.
> 
> A ducomentation update is welcome ! Could you do that before hard-freeze ?
> 
> Thanks,
> 
> C.
> 

Let me try to make it before Mar 18.

Thanks,
Moeko

>>> Given the pending release deadline, maybe these aren't blockers, but
>>> let's follow-up with something that assumes the user wants something
>>> other than what they're doing.  Thanks,
>>>
>>> Alex
>>>
>>
>> Sure.
>>
>> Moeko
>>  
>>>> +
>>>> +        /*
>>>> +         * Most of what we're doing here is to enable the ROM to run, so if
>>>> +         * there's no ROM, there's no point in setting up this quirk.
>>>> +         * NB. We only seem to get BIOS ROMs, so UEFI VM would need CSM support.
>>>> +         */
>>>> +        ret = vfio_get_region_info(&vdev->vbasedev,
>>>> +                                   VFIO_PCI_ROM_REGION_INDEX, &rom);
>>>> +        if ((ret || !rom->size) && !vdev->pdev.romfile) {
>>>> +            error_setg(&err, "Device has no ROM");
>>>> +            goto error;
>>>> +        }
>>>>   -    /* Setup OpRegion access */
>>>> -    if (!vfio_pci_igd_setup_opregion(vdev, &err)) {
>>>> -        error_append_hint(&err, "IGD legacy mode disabled\n");
>>>> -        error_report_err(err);
>>>> -        return true;
>>>> -    }
>>>> +        /*
>>>> +         * If IGD VGA Disable is clear (expected) and VGA is not already
>>>> +         * enabled, try to enable it. Probably shouldn't be using legacy mode
>>>> +         * without VGA, but also no point in us enabling VGA if disabled in
>>>> +         * hardware.
>>>> +         */
>>>> +        if (!(gmch & 0x2) && !vdev->vga && !vfio_populate_vga(vdev, &err)) {
>>>> +            error_setg(&err, "Unable to enable VGA access");
>>>> +            goto error;
>>>> +        }
>>>>   -    /* Setup LPC bridge / Host bridge PCI IDs */
>>>> -    if (!vfio_pci_igd_setup_lpc_bridge(vdev, &err)) {
>>>> -        error_append_hint(&err, "IGD legacy mode disabled\n");
>>>> -        error_report_err(err);
>>>> -        return true;
>>>> +        /* Setup OpRegion access */
>>>> +        if (!vfio_pci_igd_setup_opregion(vdev, &err)) {
>>>> +            goto error;
>>>> +        }
>>>> +
>>>> +        /* Setup LPC bridge / Host bridge PCI IDs */
>>>> +        if (!vfio_pci_igd_setup_lpc_bridge(vdev, &err)) {
>>>> +            goto error;
>>>> +        }
>>>> +    } else if (vdev->igd_legacy_mode == ON_OFF_AUTO_ON) {
>>>> +        error_setg(&err,
>>>> +                   "Machine is not i440fx or assigned BDF is not 00:02.0");
>>>> +        goto error;
>>>>       }
>>>>         /*
>>>> @@ -627,4 +634,18 @@ bool vfio_probe_igd_config_quirk(VFIOPCIDevice *vdev,
>>>>       trace_vfio_pci_igd_bdsm_enabled(vdev->vbasedev.name, (gms_size / MiB));
>>>>         return true;
>>>> +
>>>> +error:
>>>> +    /*
>>>> +     * When legacy mode is implicity enabled, continue on error,
>>>> +     * to keep compatibility
>>>> +     */
>>>> +    if (legacy_mode_enabled && (vdev->igd_legacy_mode == ON_OFF_AUTO_AUTO)) {
>>>> +        error_report_err(err);
>>>> +        error_report("IGD legacy mode disabled");
>>>> +        return true;
>>>> +    }
>>>> +
>>>> +    error_propagate(errp, err);
>>>> +    return false;
>>>>   }
>>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>>>> index a58d555934..d5ff8c1ea8 100644
>>>> --- a/hw/vfio/pci.c
>>>> +++ b/hw/vfio/pci.c
>>>> @@ -3363,6 +3363,8 @@ static const Property vfio_pci_dev_properties[] = {
>>>>                       VFIO_FEATURE_ENABLE_REQ_BIT, true),
>>>>       DEFINE_PROP_BIT("x-igd-opregion", VFIOPCIDevice, features,
>>>>                       VFIO_FEATURE_ENABLE_IGD_OPREGION_BIT, false),
>>>> +    DEFINE_PROP_ON_OFF_AUTO("x-igd-legacy-mode", VFIOPCIDevice,
>>>> +                            igd_legacy_mode, ON_OFF_AUTO_AUTO),
>>>>       DEFINE_PROP_ON_OFF_AUTO("enable-migration", VFIOPCIDevice,
>>>>                               vbasedev.enable_migration, ON_OFF_AUTO_AUTO),
>>>>       DEFINE_PROP_BOOL("migration-events", VFIOPCIDevice,
>>>> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
>>>> index 2e81f9eb19..aa67ceb346 100644
>>>> --- a/hw/vfio/pci.h
>>>> +++ b/hw/vfio/pci.h
>>>> @@ -158,6 +158,7 @@ struct VFIOPCIDevice {
>>>>       uint32_t display_xres;
>>>>       uint32_t display_yres;
>>>>       int32_t bootindex;
>>>> +    OnOffAuto igd_legacy_mode;
>>>>       uint32_t igd_gms;
>>>>       OffAutoPCIBAR msix_relo;
>>>>       uint8_t pm_cap;
>>>
>>
>