Re: [PATCH v4 2/4] vfio/igd: Refactor option ROM patching

[Alex Williamson <alex@shazbot.org>]

On Thu,  2 Jul 2026 02:20:33 +0800
Tomita Moeko <tomitamoeko@gmail.com> wrote:

> Currently the IGD-specific option ROM patching logic is embedded in
> vfio_pci_load_rom() rather than igd.c where IGD-specific code lives.
> Move this logic into a dedicated vfio_igd_legacy_rom_quirk() in igd.c.
> The refactored quirk patches the device ID and checksum on Gen 6~9
> devices with legacy VBIOS as option ROM. A new trace event
> vfio_pci_igd_vbios_patched is also introduced.
> 
> Arguments of vfio_igd_legacy_rom_quirk() are aligned with romfile_fixup
> of PCIDevice so that same logic can be reused on romfile later.

The patching in vfio_pci_load_rom() is mis-characterized as
IGD-specific.  It certainly has an IGD use case, but it's intentionally
generic and handles all cases where the device ID in the ROM doesn't
match the device ID in config space, such as through use of the
x-pci-device-id option.  This change MAKES that code IGD specific and
changes the behavior for all other devices that might have relied on
it, a clear behavioral change.  Nak.  Thanks,

Alex
 
> Reported-by: K S Maan <kirandeepmaan45@gmail.com>
> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
> ---
>  hw/vfio/igd-stubs.c  |  5 +++++
>  hw/vfio/igd.c        | 48 ++++++++++++++++++++++++++++++++++++++++++++
>  hw/vfio/pci-quirks.c |  5 +++++
>  hw/vfio/pci.c        | 30 +--------------------------
>  hw/vfio/pci.h        |  3 +++
>  hw/vfio/trace-events |  1 +
>  6 files changed, 63 insertions(+), 29 deletions(-)
> 
> diff --git a/hw/vfio/igd-stubs.c b/hw/vfio/igd-stubs.c
> index f7687d9091..29110f7568 100644
> --- a/hw/vfio/igd-stubs.c
> +++ b/hw/vfio/igd-stubs.c
> @@ -18,3 +18,8 @@ bool vfio_probe_igd_config_quirk(VFIOPCIDevice *vdev, Error **errp)
>  {
>      return true;
>  }
> +
> +void vfio_igd_legacy_rom_quirk(PCIDevice *pdev, uint8_t *ptr, uint32_t size)
> +{
> +    return;
> +}
> diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c
> index e091f21b6a..f597218164 100644
> --- a/hw/vfio/igd.c
> +++ b/hw/vfio/igd.c
> @@ -724,3 +724,51 @@ bool vfio_probe_igd_config_quirk(VFIOPCIDevice *vdev, Error **errp)
>  
>      return vfio_pci_igd_config_quirk(vdev, errp);
>  }
> +
> +void vfio_igd_legacy_rom_quirk(PCIDevice *pdev, uint8_t *ptr, uint32_t size)
> +{
> +    VFIOPCIDevice *vdev = VFIO_PCI_DEVICE(pdev);
> +    int gen;
> +    uint16_t pcir_offset;
> +    uint8_t checksum = 0;
> +    uint32_t i;
> +
> +    if (!vfio_pci_is(vdev, PCI_VENDOR_ID_INTEL, PCI_ANY_ID) ||
> +        !vfio_is_vga(vdev) || !vdev->vga) {
> +        return;
> +    }
> +
> +    /* Only Gen 6~9 devices have legacy VBIOS as Option ROM */
> +    gen = igd_gen(vdev);
> +    if (gen < 6 || gen > 9) {
> +        return;
> +    }
> +
> +    if (pci_get_word(ptr) != 0xaa55) {
> +        return;
> +    }
> +
> +    /* Must be a legacy ROM */
> +    pcir_offset = pci_get_word(ptr + 0x18);
> +    if (pcir_offset + 0x14 >= size || memcmp(ptr + pcir_offset, "PCIR", 4) ||
> +        pci_get_byte(ptr + pcir_offset + 0x14) != 0x00) {
> +        return;
> +    }
> +
> +    /*
> +     * Patch device ID as multiple IGD devices share the same rom with possible
> +     * non-matching IDs.
> +     */
> +    pci_set_word(ptr + pcir_offset + 6, vdev->device_id);
> +
> +    /*
> +     * IGD roms are known to have bogus checksums. No matter we changed the
> +     * device ID or not, we need to recalculate the checksum and patch it.
> +     */
> +    for (i = 0; i < size; i++) {
> +        checksum += ptr[i];
> +    }
> +    ((uint8_t *)ptr)[6] -= checksum;
> +
> +    trace_vfio_pci_igd_vbios_patched(vdev->vbasedev.name);
> +}
> diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c
> index bccf31751f..496a79a3ca 100644
> --- a/hw/vfio/pci-quirks.c
> +++ b/hw/vfio/pci-quirks.c
> @@ -1592,3 +1592,8 @@ bool vfio_add_virt_caps(VFIOPCIDevice *vdev, Error **errp)
>  
>      return true;
>  }
> +
> +void vfio_rom_quirk_setup(VFIOPCIDevice *vdev)
> +{
> +    vfio_igd_legacy_rom_quirk(PCI_DEVICE(vdev), vdev->rom, vdev->rom_size);
> +}
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 9c06b25e63..d321e6160a 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -1084,35 +1084,7 @@ static void vfio_pci_load_rom(VFIOPCIDevice *vdev)
>          }
>      }
>  
> -    /*
> -     * Test the ROM signature against our device, if the vendor is correct
> -     * but the device ID doesn't match, store the correct device ID and
> -     * recompute the checksum.  Intel IGD devices need this and are known
> -     * to have bogus checksums so we can't simply adjust the checksum.
> -     */
> -    if (pci_get_word(vdev->rom) == 0xaa55 &&
> -        pci_get_word(vdev->rom + 0x18) + 8 < vdev->rom_size &&
> -        !memcmp(vdev->rom + pci_get_word(vdev->rom + 0x18), "PCIR", 4)) {
> -        uint16_t vid, did;
> -
> -        vid = pci_get_word(vdev->rom + pci_get_word(vdev->rom + 0x18) + 4);
> -        did = pci_get_word(vdev->rom + pci_get_word(vdev->rom + 0x18) + 6);
> -
> -        if (vid == vdev->vendor_id && did != vdev->device_id) {
> -            int i;
> -            uint8_t csum, *data = vdev->rom;
> -
> -            pci_set_word(vdev->rom + pci_get_word(vdev->rom + 0x18) + 6,
> -                         vdev->device_id);
> -            data[6] = 0;
> -
> -            for (csum = 0, i = 0; i < vdev->rom_size; i++) {
> -                csum += data[i];
> -            }
> -
> -            data[6] = -csum;
> -        }
> -    }
> +    vfio_rom_quirk_setup(vdev);
>  }
>  
>  /* "Raw" read of underlying config space. */
> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> index c3a1f53d35..620baddda2 100644
> --- a/hw/vfio/pci.h
> +++ b/hw/vfio/pci.h
> @@ -251,10 +251,13 @@ void vfio_bar_quirk_exit(VFIOPCIDevice *vdev, int nr);
>  void vfio_bar_quirk_finalize(VFIOPCIDevice *vdev, int nr);
>  void vfio_setup_resetfn_quirk(VFIOPCIDevice *vdev);
>  bool vfio_add_virt_caps(VFIOPCIDevice *vdev, Error **errp);
> +void vfio_rom_quirk_setup(VFIOPCIDevice *vdev);
>  void vfio_quirk_reset(VFIOPCIDevice *vdev);
>  VFIOQuirk *vfio_quirk_alloc(int nr_mem);
> +
>  void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int nr);
>  bool vfio_probe_igd_config_quirk(VFIOPCIDevice *vdev, Error **errp);
> +void vfio_igd_legacy_rom_quirk(PCIDevice *pdev, uint8_t *ptr, uint32_t size);
>  
>  extern const PropertyInfo qdev_prop_nv_gpudirect_clique;
>  
> diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
> index 2049159015..7dc334ccb3 100644
> --- a/hw/vfio/trace-events
> +++ b/hw/vfio/trace-events
> @@ -90,6 +90,7 @@ vfio_pci_igd_bar4_write(const char *name, uint32_t index, uint32_t data, uint32_
>  vfio_pci_igd_bdsm_enabled(const char *name, int size) "%s %dMB"
>  vfio_pci_igd_host_bridge_enabled(const char *name) "%s"
>  vfio_pci_igd_lpc_bridge_enabled(const char *name) "%s"
> +vfio_pci_igd_vbios_patched(const char *name) "%s"
>  
>  # listener.c
>  vfio_iommu_map_notify(const char *op, uint64_t iova_start, uint64_t iova_end) "iommu %s @ 0x%"PRIx64" - 0x%"PRIx64