Re: [PATCH v2 1/7] hw/pci: Recalculate option ROM checksum before patching ID

[Alex Williamson <alex@shazbot.org>]

On Mon,  8 Jun 2026 21:45:52 +0800
Tomita Moeko <tomitamoeko@gmail.com> wrote:

> 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 cec065d108..742917f79d 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -2479,6 +2479,21 @@ 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;
> +        }

If we remove this continue branch...

> +        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)
> @@ -2514,25 +2529,27 @@ 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;
> +    }

Then this just becomes:

    if (checksum) {
        trace_pci_rom_checksum_change(ptr[6], ptr[6] - checksum);
        ptr[6] -= checksum;
    }

The result is the same, but this avoids the uint8_t casts where
checksum is promoted to an int for comparison.

Patch 7 would require an equivalent change:

  -    ((uint8_t *)vdev->rom)[6] = (uint8_t)-checksum;
  +    ((uint8_t *)vdev->rom)[6] -= checksum;

Minor change, slightly better form.  Thanks,

Alex