This series fixes the regression that on IGD passthrough with legacy BIOS boot and VBIOS, the screen is garbled during BIOS POST and GRUB (which uses standard VGA output routines), starting from QEMU 10.0. Though the kernel i915 driver still works, it reports an error about the initial GTT programmed by VBIOS is using invalid address. i915 0000:00:02.0: [drm] *ERROR* Initial plane programming using invalid range, dma_addr=0x00000000db200000 ((null) [0x00000000baf00000-0x00000000beefffff]) With the help of AI disassembling the VBIOS image dumped from host, it is found that the VBIOS itself implements a routine like: uint32_t get_BDSM() { static uint32_t saved = 0; if (saved != 0) { return saved; } return read_pci_config(BDSM_REG); } And the saved value is not cleared after initialization. Given that IGD devices don't have a real ROM BAR, the VBIOS image read by default from host is actually the VBIOS shadow RAM region, containing host-side modifications like the saved BDSM value above during POST. When the image is executed in guest, it still uses the saved host BDSM (HPA) instead of the value programmed by SeaBIOS in config space (GPA). This address mismatch leads to the garbled screen and i915 error. The previous solution, c4c45e943e51 ("vfio/pci: Intel graphics legacy mode assignment"), adjusts GTT entry addresses to (addr - host BDSM + guest BDSM) to workaround that. But it is removed in 5aed8b0f0be2 ("vfio/igd: Remove GTT write quirk in IO BAR 4") due to inconsistent values in MMIO BAR0 and IO BAR4. Considering it's unsafe to expose HPA to guest, a ROM quirk clearing the saved value in VBIOS image is introduced to fix the issue. During debugging, it is also found that IGD VBIOS ROM doesn't always match the actual IGD device ID, due to the fact that IGD of the same CPU family has multiple device IDs but shares the same ROM image. However, SeaBIOS checks the device ID strictly and refuses to run if IDs does not match. Currently only the default path, reading ROM from kernel patches the device ID, but the romfile path doesn't. So the ROM ID patching logic is also refactored in this patch series to also handle the romfile path. These changes are tested on Haswell platform with legacy BIOS boot, by K S Maan. Thanks to K S Maan for continuous help on locating and testing the issue! Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3093 Reported-by: K S Maan <kirandeepmaan45@gmail.com> Tested-by: K S Maan <kirandeepmaan45@gmail.com> Tomita Moeko (6): hw/pci: Recalculate option ROM checksum before patching ID hw/pci: Introduce rom_need_patch_id flag in PCIDevice hw/pci: Promote pci_patch_ids() to public pci_rom_patch_ids() vfio/pci: Use pci_rom_patch_ids() for IGD ROM ID patching vfio/igd: Toggle rom_need_patch_id flag on IGD devices vfio/igd: Clear saved BDSM in legacy VBIOS ROM at load time hw/pci/pci.c | 40 ++++++++--- hw/vfio/igd-stubs.c | 5 ++ hw/vfio/igd.c | 133 ++++++++++++++++++++++++++++++++++-- hw/vfio/pci-quirks.c | 5 ++ hw/vfio/pci.c | 33 ++------- hw/vfio/pci.h | 3 + hw/vfio/trace-events | 1 + include/hw/pci/pci.h | 3 + include/hw/pci/pci_device.h | 1 + 9 files changed, 179 insertions(+), 45 deletions(-) -- 2.53.0 pci_patch_ids() only adjusts checksum based on the new IDs. For an option ROM with invalid checksum, the patched one will still have an invalid checksum. Always calculate the checksum and patch it if necessary to ensure the option ROM is valid. This is intended for fixing the romfile used in IGD passthrough as multiple IGD devices share the same rom with possible non-matching device ID, and its checksum is known to be bogus [1]. A helper function pci_rom_calculate_checksum() is added and exported for reusing in IGD-specific quirk later. [1] hw/vfio/pci.c:1090 Reported-by: K S Maan <kirandeepmaan45@gmail.com> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com> Tested-by: K S Maan <kirandeepmaan45@gmail.com> --- hw/pci/pci.c | 22 +++++++++++++++++++++- include/hw/pci/pci.h | 2 ++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/hw/pci/pci.c b/hw/pci/pci.c index XXXXXXX..XXXXXXX 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -XXX,XX +XXX,XX @@ static uint8_t pci_find_capability_at_offset(PCIDevice *pdev, uint8_t offset) return found; } +uint8_t pci_rom_calculate_checksum(uint8_t *ptr, uint32_t size) +{ + uint8_t checksum = 0; + uint8_t orig_checksum = ptr[6]; + uint32_t i; + + ptr[6] = 0; + for (i = 0; i < size; i++) { + checksum += ptr[i]; + } + ptr[6] = orig_checksum; + + return -checksum; +} + /* Patch the PCI vendor and device ids in a PCI rom image if necessary. This is needed for an option rom which is used for more than one device. */ static void pci_patch_ids(PCIDevice *pdev, uint8_t *ptr, uint32_t size) @@ -XXX,XX +XXX,XX @@ static void pci_patch_ids(PCIDevice *pdev, uint8_t *ptr, uint32_t size) trace_pci_rom_and_pci_ids(pdev->romfile, vendor_id, device_id, rom_vendor_id, rom_device_id); - checksum = ptr[6]; + /* In case the checksum is bogus */ + checksum = pci_rom_calculate_checksum(ptr, size); + if (ptr[6] != checksum) { + trace_pci_rom_checksum_change(ptr[6], checksum); + ptr[6] = checksum; + } if (vendor_id != rom_vendor_id) { /* Patch vendor id and checksum (at offset 6 for etherboot roms). */ diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h index XXXXXXX..XXXXXXX 100644 --- a/include/hw/pci/pci.h +++ b/include/hw/pci/pci.h @@ -XXX,XX +XXX,XX @@ void pci_set_enabled(PCIDevice *pci_dev, bool state); void pci_set_power(PCIDevice *pci_dev, bool state); int pci_pm_init(PCIDevice *pci_dev, uint8_t offset, Error **errp); +uint8_t pci_rom_calculate_checksum(uint8_t *ptr, uint32_t size); + #endif -- 2.53.0 Allow external code (e.g., VFIO IGD passthrough) to request PCI ID patching for a custom romfile, replacing the existing is_default_rom flag for this purpose. Reported-by: K S Maan <kirandeepmaan45@gmail.com> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com> Tested-by: K S Maan <kirandeepmaan45@gmail.com> --- hw/pci/pci.c | 14 ++++++-------- include/hw/pci/pci_device.h | 1 + 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/hw/pci/pci.c b/hw/pci/pci.c index XXXXXXX..XXXXXXX 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -XXX,XX +XXX,XX @@ static const TypeInfo cxl_bus_info = { static void pci_update_mappings(PCIDevice *d); static void pci_irq_handler(void *opaque, int irq_num, int level); -static void pci_add_option_rom(PCIDevice *pdev, bool is_default_rom, Error **); +static void pci_add_option_rom(PCIDevice *pdev, Error **); static void pci_del_option_rom(PCIDevice *pdev); static uint16_t pci_default_sub_vendor_id = PCI_SUBVENDOR_ID_REDHAT_QUMRANET; @@ -XXX,XX +XXX,XX @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp) PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(pci_dev); ObjectClass *klass = OBJECT_CLASS(pc); Error *local_err = NULL; - bool is_default_rom; uint16_t class_id; /* @@ -XXX,XX +XXX,XX @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp) } /* rom loading */ - is_default_rom = false; if (pci_dev->romfile == NULL && pc->romfile != NULL) { + /* using a built-in default rom */ pci_dev->romfile = g_strdup(pc->romfile); - is_default_rom = true; + pci_dev->rom_need_patch_id = true; } - pci_add_option_rom(pci_dev, is_default_rom, &local_err); + pci_add_option_rom(pci_dev, &local_err); if (local_err) { error_propagate(errp, local_err); pci_qdev_unrealize(DEVICE(pci_dev)); @@ -XXX,XX +XXX,XX @@ static void pci_patch_ids(PCIDevice *pdev, uint8_t *ptr, uint32_t size) } /* Add an option rom for the device */ -static void pci_add_option_rom(PCIDevice *pdev, bool is_default_rom, - Error **errp) +static void pci_add_option_rom(PCIDevice *pdev, Error **errp) { int64_t size = 0; g_autofree char *path = NULL; @@ -XXX,XX +XXX,XX @@ static void pci_add_option_rom(PCIDevice *pdev, bool is_default_rom, return; } - if (is_default_rom) { + if (pdev->rom_need_patch_id) { /* Only the default rom images will be patched (if needed). */ pci_patch_ids(pdev, ptr, size); } diff --git a/include/hw/pci/pci_device.h b/include/hw/pci/pci_device.h index XXXXXXX..XXXXXXX 100644 --- a/include/hw/pci/pci_device.h +++ b/include/hw/pci/pci_device.h @@ -XXX,XX +XXX,XX @@ struct PCIDevice { bool has_rom; MemoryRegion rom; int32_t rom_bar; + bool rom_need_patch_id; /* INTx routing notifier */ PCIINTxRoutingNotifier intx_routing_notifier; -- 2.53.0 Remove the static qualifier from pci_patch_ids() and rename it to pci_rom_patch_ids(), adding a declaration in include/hw/pci/pci.h so external callers can reuse it. Reported-by: K S Maan <kirandeepmaan45@gmail.com> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com> Tested-by: K S Maan <kirandeepmaan45@gmail.com> --- hw/pci/pci.c | 4 ++-- include/hw/pci/pci.h | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/hw/pci/pci.c b/hw/pci/pci.c index XXXXXXX..XXXXXXX 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -XXX,XX +XXX,XX @@ uint8_t pci_rom_calculate_checksum(uint8_t *ptr, uint32_t size) /* Patch the PCI vendor and device ids in a PCI rom image if necessary. This is needed for an option rom which is used for more than one device. */ -static void pci_patch_ids(PCIDevice *pdev, uint8_t *ptr, uint32_t size) +void pci_rom_patch_ids(PCIDevice *pdev, uint8_t *ptr, uint32_t size) { uint16_t vendor_id; uint16_t device_id; @@ -XXX,XX +XXX,XX @@ static void pci_add_option_rom(PCIDevice *pdev, Error **errp) if (pdev->rom_need_patch_id) { /* Only the default rom images will be patched (if needed). */ - pci_patch_ids(pdev, ptr, size); + pci_rom_patch_ids(pdev, ptr, size); } } diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h index XXXXXXX..XXXXXXX 100644 --- a/include/hw/pci/pci.h +++ b/include/hw/pci/pci.h @@ -XXX,XX +XXX,XX @@ void pci_set_power(PCIDevice *pci_dev, bool state); int pci_pm_init(PCIDevice *pci_dev, uint8_t offset, Error **errp); uint8_t pci_rom_calculate_checksum(uint8_t *ptr, uint32_t size); +void pci_rom_patch_ids(PCIDevice *pdev, uint8_t *ptr, uint32_t size); #endif -- 2.53.0 Remove the duplicate inline logic in vfio_pci_load_rom() that patched the device ID in an IGD option ROM and replace it with a call to pci_rom_patch_ids(), conditioned on the rom_need_patch_id flag. Reported-by: K S Maan <kirandeepmaan45@gmail.com> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com> Tested-by: K S Maan <kirandeepmaan45@gmail.com> --- hw/vfio/pci.c | 31 +++---------------------------- 1 file changed, 3 insertions(+), 28 deletions(-) diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index XXXXXXX..XXXXXXX 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -XXX,XX +XXX,XX @@ static void vfio_update_msi(VFIOPCIDevice *vdev) static void vfio_pci_load_rom(VFIOPCIDevice *vdev) { + PCIDevice *pdev = PCI_DEVICE(vdev); VFIODevice *vbasedev = &vdev->vbasedev; struct vfio_region_info *reg_info = NULL; uint64_t size; @@ -XXX,XX +XXX,XX @@ 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; - } + if (pdev->rom_need_patch_id) { + pci_rom_patch_ids(pdev, vdev->rom, vdev->rom_size); } } -- 2.53.0 IGD ROMs are known to have wrong device IDs and bogus checksums. Toggle the rom_need_patch_id flag when the IGD has ROM BAR or custom romfile so that pci_rom_patch_ids() will correct the vendor/device IDs and checksum. The ROM BAR availability check is extracted into vfio_pci_igd_has_rom_bar() to avoid duplicating the region info lookup. Reported-by: K S Maan <kirandeepmaan45@gmail.com> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com> Tested-by: K S Maan <kirandeepmaan45@gmail.com> --- hw/vfio/igd.c | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c index XXXXXXX..XXXXXXX 100644 --- a/hw/vfio/igd.c +++ b/hw/vfio/igd.c @@ -XXX,XX +XXX,XX @@ void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int nr) QLIST_INSERT_HEAD(&vdev->bars[nr].quirks, bdsm_quirk, next); } +static bool vfio_pci_igd_has_rom_bar(VFIOPCIDevice *vdev) +{ + struct vfio_region_info *rom = NULL; + int ret; + + ret = vfio_device_get_region_info(&vdev->vbasedev, + VFIO_PCI_ROM_REGION_INDEX, &rom); + + return !ret && rom->size; +} + static bool vfio_pci_igd_config_quirk(VFIOPCIDevice *vdev, Error **errp) { struct vfio_region_info *opregion = NULL; PCIDevice *pdev = PCI_DEVICE(vdev); - int ret, gen; + int gen; uint64_t gms_size = 0; uint64_t *bdsm_size; uint32_t gmch; @@ -XXX,XX +XXX,XX @@ static bool vfio_pci_igd_config_quirk(VFIOPCIDevice *vdev, Error **errp) * - OpRegion * - Same LPC bridge and Host bridge VID/DID/SVID/SSID as host */ - struct vfio_region_info *rom = NULL; - legacy_mode_enabled = true; info_report("IGD legacy mode enabled, " "use x-igd-legacy-mode=off to disable it if unwanted."); @@ -XXX,XX +XXX,XX @@ static bool vfio_pci_igd_config_quirk(VFIOPCIDevice *vdev, Error **errp) * 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_device_get_region_info(&vdev->vbasedev, - VFIO_PCI_ROM_REGION_INDEX, &rom); - if ((ret || !rom->size) && !pdev->romfile) { + if (!vfio_pci_igd_has_rom_bar(vdev) && !pdev->romfile) { error_setg(&err, "Device has no ROM"); goto error; } @@ -XXX,XX +XXX,XX @@ static bool vfio_pci_igd_config_quirk(VFIOPCIDevice *vdev, Error **errp) goto error; } + /* + * IGD are known to have bad checksums and wrong device ID in its rom, + * request to patch it. + */ + if (pdev->romfile || vfio_pci_igd_has_rom_bar(vdev)) { + pdev->rom_need_patch_id = true; + } + /* * ASLS (OpRegion address) is read-only, emulated * It contains HPA, guest firmware need to reprogram it with GPA. -- 2.53.0 IGD does not come with a ROM BAR [1], the ROM BAR read by default from kernel is actually the host VBIOS shadow RAM region that contains host modifications on boot. With AI-assisted reverse engineering on VBIOS binaries, it is observed that VBIOS saves BDSM register value on first access and uses saved value if present. When the image is executed in guest, since there is already a saved HPA in VBIOS, it keeps using that value instead of the GPA programmed by SeaBIOS in BDSM register in PCI config space, causing VBIOS to program GTT entries with wrong address, resulting in garbled output in BIOS POST and the error below detected by i915 driver. i915 0000:00:02.0: [drm] *ERROR* Initial plane programming using invalid range, dma_addr=0x00000000db200000 ((null) [0x00000000baf00000-0x00000000beefffff]) The previous solution, c4c45e943e51 ("vfio/pci: Intel graphics legacy mode assignment"), adjusts GTT entry addresses to (addr - host BDSM + guest BDSM) to workaround that. But it is removed in 5aed8b0f0be2 ("vfio/igd: Remove GTT write quirk in IO BAR 4") due to inconsistent values in MMIO BAR0 and IO BAR4. Considering it's unsafe to expose HPA to guest, a ROM quirk clearing the saved value in VBIOS image is introduced. It searches the BDSM accessor routine by matching a 19-byte signature anchored on the unique `mov $0x105e,%ax` instruction, then locate the offset of saved BDSM and clears it. This makes the routine fall through to the PCI config read on the first call inside the guest. The quirk is invoked in vfio_pci_load_rom(), and is gated on Gen 6-9 IGD devices with VGA access enabled and legacy (non-UEFI) PCIR code type in the ROM header. A new trace event vfio_pci_igd_vbios_patched is also introduced. [1] 3.5.15, 4th Generation Intel Core Processor Family Datasheet Vol. 2 https://www.intel.com/content/dam/www/public/us/en/documents/datasheets/4th-gen-core-family-desktop-vol-2-datasheet.pdf Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3093 Reported-by: K S Maan <kirandeepmaan45@gmail.com> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com> Tested-by: K S Maan <kirandeepmaan45@gmail.com> --- hw/vfio/igd-stubs.c | 5 ++ hw/vfio/igd.c | 106 +++++++++++++++++++++++++++++++++++++++++++ hw/vfio/pci-quirks.c | 5 ++ hw/vfio/pci.c | 2 + hw/vfio/pci.h | 3 ++ hw/vfio/trace-events | 1 + 6 files changed, 122 insertions(+) diff --git a/hw/vfio/igd-stubs.c b/hw/vfio/igd-stubs.c index XXXXXXX..XXXXXXX 100644 --- a/hw/vfio/igd-stubs.c +++ b/hw/vfio/igd-stubs.c @@ -XXX,XX +XXX,XX @@ bool vfio_probe_igd_config_quirk(VFIOPCIDevice *vdev, Error **errp) { return true; } + +void vfio_probe_igd_legacy_rom_quirk(VFIOPCIDevice *vdev) +{ + return; +} diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c index XXXXXXX..XXXXXXX 100644 --- a/hw/vfio/igd.c +++ b/hw/vfio/igd.c @@ -XXX,XX +XXX,XX @@ bool vfio_probe_igd_config_quirk(VFIOPCIDevice *vdev, Error **errp) return vfio_pci_igd_config_quirk(vdev, errp); } + +/* + * IGD ROM BAR read from kernel is actually the host VBIOS shadow RAM region, + * which contains host modifications. In Gen 6-9 VBIOS, the routine below is + * used to get BDSM value when programming the initial GTT. + * xx xx xx xx v: .long ? # saved value + * 66 53 push %ebx + * 66 2e 83 3e xx xx 00 cmpl $0x0,%cs:v # is saved value empty? + * 74 07 je 1f # if zero, go compute + * 66 2e a1 xx xx mov %cs:v,%eax # else return saved value + * eb 0f jmp 2f + * b8 5e 10 1: mov $0x105e,%ax # dev 00:02.0, offset 5E + * e8 xx xx call pci_read_cfg_word + * 66 c1 e0 10 shl $0x10,%eax # left shift 16 bits + * 66 2e a3 xx xx mov %eax,%cs:v # save the result + * 66 5b 2：pop %ebx + * c3 ret + * When running the VBIOS in guest, saved value still reflects the host stolen + * memory base address, which is not correct in guest. So we need to patch the + * VBIOS to clear the saved value. + * + * The unique 19-byte starts at `cmpl $0,%cs:v` and ends at `mov $0x105e,%ax` + * anchors the match to the routine. Both `cs:` displacements must reference + * the same offset. + */ +static int igd_vbios_find_saved_bdsm(const uint8_t *rom, size_t rom_size, + uint16_t *bdsm_offset) +{ + static const uint8_t start[] = { 0x66, 0x2e, 0x83, 0x3e }; + static const uint8_t middle[] = { 0x00, 0x74, 0x07, 0x66, 0x2e, 0xa1 }; + static const uint8_t end[] = { 0xeb, 0x0f, 0xb8, 0x5e, 0x10 }; + size_t i; + bool found = false; + + if (rom_size < 19) { + return -ENOENT; + } + + for (i = 0; i + 19 <= rom_size; i++) { + if (memcmp(rom + i, start, sizeof(start)) != 0 || + memcmp(rom + i + 6, middle, sizeof(middle)) != 0 || + memcmp(rom + i + 14, end, sizeof(end)) != 0) { + continue; + } + + /* same saved value address? */ + if (rom[i + 4] != rom[i + 12] || rom[i + 5] != rom[i + 13]) { + continue; + } + + if (found) { + return -EEXIST; + } + + *bdsm_offset = rom[i + 4] | ((uint16_t)rom[i + 5] << 8); + found = true; + } + + if (!found) { + return -ENOENT; + } + + return 0; +} + +void vfio_probe_igd_legacy_rom_quirk(VFIOPCIDevice *vdev) +{ + int ret, gen; + uint16_t pcir_offset, bdsm_offset = 0; + uint8_t checksum; + + 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(vdev->rom) != 0xaa55) { + return; + } + + /* Must be a legacy ROM */ + pcir_offset = pci_get_word(vdev->rom + 0x18); + if (pcir_offset >= vdev->rom_size || + memcmp(vdev->rom + pcir_offset, "PCIR", 4) || + pci_get_byte(vdev->rom + pcir_offset + 0x14) != 0x00) { + return; + } + + ret = igd_vbios_find_saved_bdsm(vdev->rom, vdev->rom_size, &bdsm_offset); + if (ret < 0) { + return; + } + + memset(vdev->rom + bdsm_offset, 0, sizeof(uint32_t)); + + checksum = pci_rom_calculate_checksum(vdev->rom, vdev->rom_size); + ((uint8_t *)vdev->rom)[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 XXXXXXX..XXXXXXX 100644 --- a/hw/vfio/pci-quirks.c +++ b/hw/vfio/pci-quirks.c @@ -XXX,XX +XXX,XX @@ bool vfio_add_virt_caps(VFIOPCIDevice *vdev, Error **errp) return true; } + +void vfio_rom_quirk_setup(VFIOPCIDevice *vdev) +{ + vfio_probe_igd_legacy_rom_quirk(vdev); +} diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index XXXXXXX..XXXXXXX 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -XXX,XX +XXX,XX @@ static void vfio_pci_load_rom(VFIOPCIDevice *vdev) if (pdev->rom_need_patch_id) { pci_rom_patch_ids(pdev, vdev->rom, vdev->rom_size); } + + vfio_rom_quirk_setup(vdev); } /* "Raw" read of underlying config space. */ diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h index XXXXXXX..XXXXXXX 100644 --- a/hw/vfio/pci.h +++ b/hw/vfio/pci.h @@ -XXX,XX +XXX,XX @@ 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_probe_igd_legacy_rom_quirk(VFIOPCIDevice *vdev); extern const PropertyInfo qdev_prop_nv_gpudirect_clique; diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events index XXXXXXX..XXXXXXX 100644 --- a/hw/vfio/trace-events +++ b/hw/vfio/trace-events @@ -XXX,XX +XXX,XX @@ 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 -- 2.53.0 This series fixes the regression that on IGD passthrough with legacy BIOS boot and VBIOS, the screen is garbled during BIOS POST and GRUB (which uses standard VGA output routines), starting from QEMU 10.0. Though the kernel i915 driver still works, it reports an error about the initial GTT programmed by VBIOS is using invalid address. i915 0000:00:02.0: [drm] *ERROR* Initial plane programming using invalid range, dma_addr=0x00000000db200000 ((null) [0x00000000baf00000-0x00000000beefffff]) With the help of AI disassembling the VBIOS image dumped from host, it is found that the VBIOS itself implements a routine like: uint32_t get_BDSM() { static uint32_t saved = 0; if (saved != 0) { return saved; } return read_pci_config(BDSM_REG); } And the saved value is not cleared after initialization. Given that IGD devices don't have a real ROM BAR, the VBIOS image read by default from host is actually the VBIOS shadow RAM region, containing host-side modifications like the saved BDSM value above during POST. When the image is executed in guest, it still uses the saved host BDSM (HPA) instead of the value programmed by SeaBIOS in config space (GPA). This address mismatch leads to the garbled screen and i915 error. The previous solution, c4c45e943e51 ("vfio/pci: Intel graphics legacy mode assignment"), adjusts GTT entry addresses to (addr - host BDSM + guest BDSM) to workaround that. But it is removed in 5aed8b0f0be2 ("vfio/igd: Remove GTT write quirk in IO BAR 4") due to inconsistent values in MMIO BAR0 and IO BAR4. Considering it's unsafe to expose HPA to guest, a ROM quirk clearing the saved value in VBIOS image is introduced to fix the issue. During debugging, it is also found that IGD VBIOS ROM doesn't always match the actual IGD device ID, due to the fact that IGD of the same CPU family has multiple device IDs but shares the same ROM image. However, SeaBIOS checks the device ID strictly and refuses to run if IDs does not match. Currently only the default path, reading ROM from kernel patches the device ID, but the romfile path doesn't. So the ROM ID patching logic is also refactored in this patch series to also handle the romfile path. These changes are tested on Haswell platform with legacy BIOS boot, by K S Maan. Thanks to K S Maan for continuous help on locating and testing the issue! Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3093 Reported-by: K S Maan <kirandeepmaan45@gmail.com> Changelog: v2: * New patch 2/7 to fix regression with EFI option ROMs * Refine logic in ROM ID and checksum patching * Reorder patch 4 and 5 for cleaner bisection * Address comments from v1 Link: https://lore.kernel.org/all/20260603173355.36121-1-tomitamoeko@gmail.com/t Tomita Moeko (7): hw/pci: Recalculate option ROM checksum before patching ID hw/pci: Skip EFI option ROM in pci_patch_ids() hw/pci: Introduce rom_need_patch_id flag in PCIDevice hw/pci: Promote pci_patch_ids() to public pci_rom_patch_ids() vfio/igd: Toggle rom_need_patch_id flag on IGD devices vfio/pci: Use pci_rom_patch_ids() for IGD ROM ID patching vfio/igd: Clear saved BDSM in legacy VBIOS ROM at load time hw/pci/pci.c | 58 ++++++++++------ hw/vfio/igd-stubs.c | 5 ++ hw/vfio/igd.c | 132 ++++++++++++++++++++++++++++++++++-- hw/vfio/pci-quirks.c | 5 ++ hw/vfio/pci.c | 33 ++------- hw/vfio/pci.h | 3 + hw/vfio/trace-events | 1 + include/hw/pci/pci.h | 3 + include/hw/pci/pci_device.h | 1 + 9 files changed, 188 insertions(+), 53 deletions(-) -- 2.53.0 pci_patch_ids() only adjusts checksum based on the new IDs. For an option ROM with invalid checksum, the patched one will still have an invalid checksum. Always calculate the checksum and patch it if necessary to ensure the option ROM is valid. This is intended for fixing the romfile used in IGD passthrough as multiple IGD devices share the same rom with possible non-matching device ID, and its checksum is known to be bogus [1]. A helper function pci_rom_calculate_checksum() is added and exported for reusing in IGD-specific quirk later. [1] hw/vfio/pci.c:1090 Reported-by: K S Maan <kirandeepmaan45@gmail.com> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com> --- hw/pci/pci.c | 35 ++++++++++++++++++++++++++--------- include/hw/pci/pci.h | 2 ++ 2 files changed, 28 insertions(+), 9 deletions(-) diff --git a/hw/pci/pci.c b/hw/pci/pci.c index XXXXXXX..XXXXXXX 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -XXX,XX +XXX,XX @@ static uint8_t pci_find_capability_at_offset(PCIDevice *pdev, uint8_t offset) return found; } +uint8_t pci_rom_calculate_checksum(const uint8_t *ptr, uint32_t size) +{ + uint8_t checksum = 0; + uint32_t i; + + for (i = 0; i < size; i++) { + if (i == 6) { + continue; + } + checksum += ptr[i]; + } + + return checksum; +} + /* Patch the PCI vendor and device ids in a PCI rom image if necessary. This is needed for an option rom which is used for more than one device. */ static void pci_patch_ids(PCIDevice *pdev, uint8_t *ptr, uint32_t size) @@ -XXX,XX +XXX,XX @@ static void pci_patch_ids(PCIDevice *pdev, uint8_t *ptr, uint32_t size) trace_pci_rom_and_pci_ids(pdev->romfile, vendor_id, device_id, rom_vendor_id, rom_device_id); - checksum = ptr[6]; + /* In case the checksum is bogus */ + checksum = pci_rom_calculate_checksum(ptr, size); if (vendor_id != rom_vendor_id) { /* Patch vendor id and checksum (at offset 6 for etherboot roms). */ - checksum += (uint8_t)rom_vendor_id + (uint8_t)(rom_vendor_id >> 8); - checksum -= (uint8_t)vendor_id + (uint8_t)(vendor_id >> 8); - trace_pci_rom_checksum_change(ptr[6], checksum); - ptr[6] = checksum; + checksum += (uint8_t)vendor_id + (uint8_t)(vendor_id >> 8); + checksum -= (uint8_t)rom_vendor_id + (uint8_t)(rom_vendor_id >> 8); pci_set_word(ptr + pcir_offset + 4, vendor_id); } if (device_id != rom_device_id) { /* Patch device id and checksum (at offset 6 for etherboot roms). */ - checksum += (uint8_t)rom_device_id + (uint8_t)(rom_device_id >> 8); - checksum -= (uint8_t)device_id + (uint8_t)(device_id >> 8); - trace_pci_rom_checksum_change(ptr[6], checksum); - ptr[6] = checksum; + checksum += (uint8_t)device_id + (uint8_t)(device_id >> 8); + checksum -= (uint8_t)rom_device_id + (uint8_t)(rom_device_id >> 8); pci_set_word(ptr + pcir_offset + 6, device_id); } + + if (ptr[6] != (uint8_t)-checksum) { + trace_pci_rom_checksum_change(ptr[6], (uint8_t)-checksum); + ptr[6] = (uint8_t)-checksum; + } } /* Add an option rom for the device */ diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h index XXXXXXX..XXXXXXX 100644 --- a/include/hw/pci/pci.h +++ b/include/hw/pci/pci.h @@ -XXX,XX +XXX,XX @@ void pci_set_enabled(PCIDevice *pci_dev, bool state); void pci_set_power(PCIDevice *pci_dev, bool state); int pci_pm_init(PCIDevice *pci_dev, uint8_t offset, Error **errp); +uint8_t pci_rom_calculate_checksum(const uint8_t *ptr, uint32_t size); + #endif -- 2.53.0 pci_patch_ids() patches the checksum at the reserved 0x06 byte, but for EFI option ROMs the 32 bits at 0x04 are the EFI signature and must be 0x00000EF1. Since OVMF does not check vendor/device IDs in the PCIR header or the checksum, skip patching for EFI ROMs. Reported-by: K S Maan <kirandeepmaan45@gmail.com> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com> --- hw/pci/pci.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/hw/pci/pci.c b/hw/pci/pci.c index XXXXXXX..XXXXXXX 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -XXX,XX +XXX,XX @@ static void pci_patch_ids(PCIDevice *pdev, uint8_t *ptr, uint32_t size) return; } + /* OVMF won't check IDs in PCIR header, skip EFI roms */ + if (pci_get_byte(ptr + pcir_offset + 0x14) == 0x03) { + return; + } + vendor_id = pci_get_word(pdev->config + PCI_VENDOR_ID); device_id = pci_get_word(pdev->config + PCI_DEVICE_ID); rom_vendor_id = pci_get_word(ptr + pcir_offset + 4); -- 2.53.0 Allow external code (e.g., VFIO IGD passthrough) to request PCI ID patching for a custom romfile, replacing the existing is_default_rom flag for this purpose. Reported-by: K S Maan <kirandeepmaan45@gmail.com> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com> --- hw/pci/pci.c | 14 ++++++-------- include/hw/pci/pci_device.h | 1 + 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/hw/pci/pci.c b/hw/pci/pci.c index XXXXXXX..XXXXXXX 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -XXX,XX +XXX,XX @@ static const TypeInfo cxl_bus_info = { static void pci_update_mappings(PCIDevice *d); static void pci_irq_handler(void *opaque, int irq_num, int level); -static void pci_add_option_rom(PCIDevice *pdev, bool is_default_rom, Error **); +static void pci_add_option_rom(PCIDevice *pdev, Error **); static void pci_del_option_rom(PCIDevice *pdev); static uint16_t pci_default_sub_vendor_id = PCI_SUBVENDOR_ID_REDHAT_QUMRANET; @@ -XXX,XX +XXX,XX @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp) PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(pci_dev); ObjectClass *klass = OBJECT_CLASS(pc); Error *local_err = NULL; - bool is_default_rom; uint16_t class_id; /* @@ -XXX,XX +XXX,XX @@ static void pci_qdev_realize(DeviceState *qdev, Error **errp) } /* rom loading */ - is_default_rom = false; if (pci_dev->romfile == NULL && pc->romfile != NULL) { + /* using a built-in default rom */ pci_dev->romfile = g_strdup(pc->romfile); - is_default_rom = true; + pci_dev->rom_need_patch_id = true; } - pci_add_option_rom(pci_dev, is_default_rom, &local_err); + pci_add_option_rom(pci_dev, &local_err); if (local_err) { error_propagate(errp, local_err); pci_qdev_unrealize(DEVICE(pci_dev)); @@ -XXX,XX +XXX,XX @@ static void pci_patch_ids(PCIDevice *pdev, uint8_t *ptr, uint32_t size) } /* Add an option rom for the device */ -static void pci_add_option_rom(PCIDevice *pdev, bool is_default_rom, - Error **errp) +static void pci_add_option_rom(PCIDevice *pdev, Error **errp) { int64_t size = 0; g_autofree char *path = NULL; @@ -XXX,XX +XXX,XX @@ static void pci_add_option_rom(PCIDevice *pdev, bool is_default_rom, return; } - if (is_default_rom) { + if (pdev->rom_need_patch_id) { /* Only the default rom images will be patched (if needed). */ pci_patch_ids(pdev, ptr, size); } diff --git a/include/hw/pci/pci_device.h b/include/hw/pci/pci_device.h index XXXXXXX..XXXXXXX 100644 --- a/include/hw/pci/pci_device.h +++ b/include/hw/pci/pci_device.h @@ -XXX,XX +XXX,XX @@ struct PCIDevice { char *romfile; uint32_t romsize; bool has_rom; + bool rom_need_patch_id; MemoryRegion rom; int32_t rom_bar; -- 2.53.0 Remove the static qualifier from pci_patch_ids() and rename it to pci_rom_patch_ids(), adding a declaration in include/hw/pci/pci.h so external callers can reuse it. Reported-by: K S Maan <kirandeepmaan45@gmail.com> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com> --- hw/pci/pci.c | 4 ++-- include/hw/pci/pci.h | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/hw/pci/pci.c b/hw/pci/pci.c index XXXXXXX..XXXXXXX 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -XXX,XX +XXX,XX @@ uint8_t pci_rom_calculate_checksum(const uint8_t *ptr, uint32_t size) /* Patch the PCI vendor and device ids in a PCI rom image if necessary. This is needed for an option rom which is used for more than one device. */ -static void pci_patch_ids(PCIDevice *pdev, uint8_t *ptr, uint32_t size) +void pci_rom_patch_ids(PCIDevice *pdev, uint8_t *ptr, uint32_t size) { uint16_t vendor_id; uint16_t device_id; @@ -XXX,XX +XXX,XX @@ static void pci_add_option_rom(PCIDevice *pdev, Error **errp) if (pdev->rom_need_patch_id) { /* Only the default rom images will be patched (if needed). */ - pci_patch_ids(pdev, ptr, size); + pci_rom_patch_ids(pdev, ptr, size); } } diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h index XXXXXXX..XXXXXXX 100644 --- a/include/hw/pci/pci.h +++ b/include/hw/pci/pci.h @@ -XXX,XX +XXX,XX @@ void pci_set_power(PCIDevice *pci_dev, bool state); int pci_pm_init(PCIDevice *pci_dev, uint8_t offset, Error **errp); uint8_t pci_rom_calculate_checksum(const uint8_t *ptr, uint32_t size); +void pci_rom_patch_ids(PCIDevice *pdev, uint8_t *ptr, uint32_t size); #endif -- 2.53.0 IGD ROMs are known to have wrong device IDs and bogus checksums. Toggle the rom_need_patch_id flag when the IGD has ROM BAR or custom romfile so that pci_rom_patch_ids() will correct the vendor/device IDs and checksum. Reported-by: K S Maan <kirandeepmaan45@gmail.com> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com> --- hw/vfio/igd.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c index XXXXXXX..XXXXXXX 100644 --- a/hw/vfio/igd.c +++ b/hw/vfio/igd.c @@ -XXX,XX +XXX,XX @@ void vfio_probe_igd_bar0_quirk(VFIOPCIDevice *vdev, int nr) static bool vfio_pci_igd_config_quirk(VFIOPCIDevice *vdev, Error **errp) { struct vfio_region_info *opregion = NULL; + struct vfio_region_info *rom = NULL; PCIDevice *pdev = PCI_DEVICE(vdev); - int ret, gen; + int gen; uint64_t gms_size = 0; uint64_t *bdsm_size; uint32_t gmch; bool legacy_mode_enabled = false; + bool has_rombar = false; Error *err = NULL; if (!vfio_pci_is(vdev, PCI_VENDOR_ID_INTEL, PCI_ANY_ID) || @@ -XXX,XX +XXX,XX @@ static bool vfio_pci_igd_config_quirk(VFIOPCIDevice *vdev, Error **errp) gen = igd_gen(vdev); gmch = vfio_pci_read_config(pdev, IGD_GMCH, 4); + has_rombar = !vfio_device_get_region_info(&vdev->vbasedev, + VFIO_PCI_ROM_REGION_INDEX, + &rom) && rom->size; + /* * For backward compatibility, enable legacy mode when * - Device geneation is 6 to 9 (including both) @@ -XXX,XX +XXX,XX @@ static bool vfio_pci_igd_config_quirk(VFIOPCIDevice *vdev, Error **errp) * - OpRegion * - Same LPC bridge and Host bridge VID/DID/SVID/SSID as host */ - struct vfio_region_info *rom = NULL; - legacy_mode_enabled = true; info_report("IGD legacy mode enabled, " "use x-igd-legacy-mode=off to disable it if unwanted."); @@ -XXX,XX +XXX,XX @@ static bool vfio_pci_igd_config_quirk(VFIOPCIDevice *vdev, Error **errp) * 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_device_get_region_info(&vdev->vbasedev, - VFIO_PCI_ROM_REGION_INDEX, &rom); - if ((ret || !rom->size) && !pdev->romfile) { + if (!has_rombar && !pdev->romfile) { error_setg(&err, "Device has no ROM"); goto error; } @@ -XXX,XX +XXX,XX @@ static bool vfio_pci_igd_config_quirk(VFIOPCIDevice *vdev, Error **errp) goto error; } + /* + * IGD are known to have bad checksums and wrong device ID in its rom, + * request to patch it. + */ + if (has_rombar || pdev->romfile) { + pdev->rom_need_patch_id = true; + } + /* * ASLS (OpRegion address) is read-only, emulated * It contains HPA, guest firmware need to reprogram it with GPA. -- 2.53.0 Remove the duplicate inline logic in vfio_pci_load_rom() that patched the device ID in an IGD option ROM and replace it with a call to pci_rom_patch_ids(), conditioned on the rom_need_patch_id flag. Reported-by: K S Maan <kirandeepmaan45@gmail.com> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com> --- hw/vfio/pci.c | 31 +++---------------------------- 1 file changed, 3 insertions(+), 28 deletions(-) diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index XXXXXXX..XXXXXXX 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -XXX,XX +XXX,XX @@ static void vfio_update_msi(VFIOPCIDevice *vdev) static void vfio_pci_load_rom(VFIOPCIDevice *vdev) { + PCIDevice *pdev = PCI_DEVICE(vdev); VFIODevice *vbasedev = &vdev->vbasedev; struct vfio_region_info *reg_info = NULL; uint64_t size; @@ -XXX,XX +XXX,XX @@ 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; - } + if (pdev->rom_need_patch_id) { + pci_rom_patch_ids(pdev, vdev->rom, vdev->rom_size); } } -- 2.53.0 IGD does not come with a ROM BAR [1], the ROM BAR read by default from kernel is actually the host VBIOS shadow RAM region that contains host modifications on boot. With AI-assisted reverse engineering on VBIOS binaries, it is observed that VBIOS saves BDSM register value on first access and uses saved value if present. When the image is executed in guest, since there is already a saved HPA in VBIOS, it keeps using that value instead of the GPA programmed by SeaBIOS in BDSM register in PCI config space, causing VBIOS to program GTT entries with wrong address, resulting in garbled output in BIOS POST and the error below detected by i915 driver. i915 0000:00:02.0: [drm] *ERROR* Initial plane programming using invalid range, dma_addr=0x00000000db200000 ((null) [0x00000000baf00000-0x00000000beefffff]) The previous solution, c4c45e943e51 ("vfio/pci: Intel graphics legacy mode assignment"), adjusts GTT entry addresses to (addr - host BDSM + guest BDSM) to workaround that. But it is removed in 5aed8b0f0be2 ("vfio/igd: Remove GTT write quirk in IO BAR 4") due to inconsistent values in MMIO BAR0 and IO BAR4. Considering it's unsafe to expose HPA to guest, a ROM quirk clearing the saved value in VBIOS image is introduced. It searches the BDSM accessor routine by matching a 19-byte signature anchored on the unique `mov $0x105e,%ax` instruction, then locate the offset of saved BDSM and clears it. This makes the routine fall through to the PCI config read on the first call inside the guest. The quirk is invoked in vfio_pci_load_rom(), and is gated on Gen 6-9 IGD devices with VGA access enabled and legacy (non-UEFI) PCIR code type in the ROM header. A new trace event vfio_pci_igd_vbios_patched is also introduced. [1] 3.5.15, 4th Generation Intel Core Processor Family Datasheet Vol. 2 https://www.intel.com/content/dam/www/public/us/en/documents/datasheets/4th-gen-core-family-desktop-vol-2-datasheet.pdf Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3093 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 | 110 +++++++++++++++++++++++++++++++++++++++++++ hw/vfio/pci-quirks.c | 5 ++ hw/vfio/pci.c | 2 + hw/vfio/pci.h | 3 ++ hw/vfio/trace-events | 1 + 6 files changed, 126 insertions(+) diff --git a/hw/vfio/igd-stubs.c b/hw/vfio/igd-stubs.c index XXXXXXX..XXXXXXX 100644 --- a/hw/vfio/igd-stubs.c +++ b/hw/vfio/igd-stubs.c @@ -XXX,XX +XXX,XX @@ bool vfio_probe_igd_config_quirk(VFIOPCIDevice *vdev, Error **errp) { return true; } + +void vfio_probe_igd_legacy_rom_quirk(VFIOPCIDevice *vdev) +{ + return; +} diff --git a/hw/vfio/igd.c b/hw/vfio/igd.c index XXXXXXX..XXXXXXX 100644 --- a/hw/vfio/igd.c +++ b/hw/vfio/igd.c @@ -XXX,XX +XXX,XX @@ bool vfio_probe_igd_config_quirk(VFIOPCIDevice *vdev, Error **errp) return vfio_pci_igd_config_quirk(vdev, errp); } + +/* + * IGD ROM BAR read from kernel is actually the host VBIOS shadow RAM region, + * which contains host modifications. In Gen 6-9 VBIOS, the routine below is + * used to get BDSM value when programming the initial GTT. + * xx xx xx xx v: .long ? # saved value + * 66 53 push %ebx + * 66 2e 83 3e xx xx 00 cmpl $0x0,%cs:v # is saved value empty? + * 74 07 je 1f # if zero, go compute + * 66 2e a1 xx xx mov %cs:v,%eax # else return saved value + * eb 0f jmp 2f + * b8 5e 10 1: mov $0x105e,%ax # dev 00:02.0, offset 5E + * e8 xx xx call pci_read_cfg_word + * 66 c1 e0 10 shl $0x10,%eax # left shift 16 bits + * 66 2e a3 xx xx mov %eax,%cs:v # save the result + * 66 5b 2：pop %ebx + * c3 ret + * When running the VBIOS in guest, saved value still reflects the host stolen + * memory base address, which is not correct in guest. So we need to patch the + * VBIOS to clear the saved value. + * + * The unique 19-byte starts at `cmpl $0,%cs:v` and ends at `mov $0x105e,%ax` + * anchors the match to the routine. Both `cs:` displacements must reference + * the same offset. + */ +static int igd_vbios_find_saved_bdsm(const uint8_t *rom, size_t rom_size, + uint16_t *bdsm_offset) +{ + static const uint8_t start[] = { 0x66, 0x2e, 0x83, 0x3e }; + static const uint8_t middle[] = { 0x00, 0x74, 0x07, 0x66, 0x2e, 0xa1 }; + static const uint8_t end[] = { 0xeb, 0x0f, 0xb8, 0x5e, 0x10 }; + uint16_t val; + size_t i; + bool found = false; + + if (rom_size < 19) { + return -ENOENT; + } + + for (i = 0; i + 19 <= rom_size; i++) { + if (memcmp(rom + i, start, sizeof(start)) != 0 || + memcmp(rom + i + 6, middle, sizeof(middle)) != 0 || + memcmp(rom + i + 14, end, sizeof(end)) != 0) { + continue; + } + + /* same saved value address? */ + if (rom[i + 4] != rom[i + 12] || rom[i + 5] != rom[i + 13]) { + continue; + } + + if (found) { + return -EEXIST; + } + + val = rom[i + 4] | ((uint16_t)rom[i + 5] << 8); + if (val < rom_size) { + *bdsm_offset = val; + found = true; + } + } + + if (!found) { + return -ENOENT; + } + + return 0; +} + +void vfio_probe_igd_legacy_rom_quirk(VFIOPCIDevice *vdev) +{ + int ret, gen; + uint16_t pcir_offset, bdsm_offset = 0; + uint8_t checksum; + + 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(vdev->rom) != 0xaa55) { + return; + } + + /* Must be a legacy ROM */ + pcir_offset = pci_get_word(vdev->rom + 0x18); + if (pcir_offset + 0x14 >= vdev->rom_size || + memcmp(vdev->rom + pcir_offset, "PCIR", 4) || + pci_get_byte(vdev->rom + pcir_offset + 0x14) != 0x00) { + return; + } + + ret = igd_vbios_find_saved_bdsm(vdev->rom, vdev->rom_size, &bdsm_offset); + if (ret < 0) { + return; + } + + memset(vdev->rom + bdsm_offset, 0, sizeof(uint32_t)); + + checksum = pci_rom_calculate_checksum(vdev->rom, vdev->rom_size); + ((uint8_t *)vdev->rom)[6] = (uint8_t)-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 XXXXXXX..XXXXXXX 100644 --- a/hw/vfio/pci-quirks.c +++ b/hw/vfio/pci-quirks.c @@ -XXX,XX +XXX,XX @@ bool vfio_add_virt_caps(VFIOPCIDevice *vdev, Error **errp) return true; } + +void vfio_rom_quirk_setup(VFIOPCIDevice *vdev) +{ + vfio_probe_igd_legacy_rom_quirk(vdev); +} diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index XXXXXXX..XXXXXXX 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -XXX,XX +XXX,XX @@ static void vfio_pci_load_rom(VFIOPCIDevice *vdev) if (pdev->rom_need_patch_id) { pci_rom_patch_ids(pdev, vdev->rom, vdev->rom_size); } + + vfio_rom_quirk_setup(vdev); } /* "Raw" read of underlying config space. */ diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h index XXXXXXX..XXXXXXX 100644 --- a/hw/vfio/pci.h +++ b/hw/vfio/pci.h @@ -XXX,XX +XXX,XX @@ 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_probe_igd_legacy_rom_quirk(VFIOPCIDevice *vdev); extern const PropertyInfo qdev_prop_nv_gpudirect_clique; diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events index XXXXXXX..XXXXXXX 100644 --- a/hw/vfio/trace-events +++ b/hw/vfio/trace-events @@ -XXX,XX +XXX,XX @@ 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 -- 2.53.0