Patchwork [1/2] pci: Automatically patch PCI vendor id and device id in PCI ROM

login
register
mail settings
Submitter Stefan Weil
Date Oct. 18, 2010, 5:55 p.m.
Message ID <1287424511-22021-1-git-send-email-weil@mail.berlios.de>
Download mbox | patch
Permalink /patch/68220/
State New
Headers show

Comments

Stefan Weil - Oct. 18, 2010, 5:55 p.m.
PCI devices with different vendor or device ids sometimes share
the same rom code. Only the ids and the checksum
Michael S. Tsirkin - Oct. 18, 2010, 5:58 p.m.
On Mon, Oct 18, 2010 at 07:55:11PM +0200, Stefan Weil wrote:
> PCI devices with different vendor or device ids sometimes share
> the same rom code. Only the ids and the checksum
> differs in a boot rom for such devices.
> 
> The i825xx ethernet controller family is a typical example
> which is implemented in hw/eepro100.c. It uses at least
> 3 different device ids, so normally 3 boot roms would be needed.
> 
> By automatically patching vendor id and device id (and the checksum)
> in qemu, all emulated family members can share the same boot rom.
> 
> VGA bios roms are another example with different vendor and device ids.
> 
> v2:
> 
> * Patch also the vendor id (and remove the sanity check for vendor id).
> 
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Stefan Weil <weil@mail.berlios.de>
> ---
>  hw/pci.c |   58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 58 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index 1280d4d..139eb24 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -1797,6 +1797,62 @@ static void pci_map_option_rom(PCIDevice *pdev, int region_num, pcibus_t addr, p
>      cpu_register_physical_memory(addr, size, pdev->rom_offset);
>  }
>  
> +/* 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, int size)

let's return an error code on malformed roms so management can detect errors?
Anthony Liguori - Oct. 18, 2010, 6:42 p.m.
On 10/18/2010 12:58 PM, Michael S. Tsirkin wrote:
> On Mon, Oct 18, 2010 at 07:55:11PM +0200, Stefan Weil wrote:
>    
>> PCI devices with different vendor or device ids sometimes share
>> the same rom code. Only the ids and the checksum
>> differs in a boot rom for such devices.
>>
>> The i825xx ethernet controller family is a typical example
>> which is implemented in hw/eepro100.c. It uses at least
>> 3 different device ids, so normally 3 boot roms would be needed.
>>
>> By automatically patching vendor id and device id (and the checksum)
>> in qemu, all emulated family members can share the same boot rom.
>>
>> VGA bios roms are another example with different vendor and device ids.
>>
>> v2:
>>
>> * Patch also the vendor id (and remove the sanity check for vendor id).
>>
>> Cc: Gerd Hoffmann<kraxel@redhat.com>
>> Cc: Markus Armbruster<armbru@redhat.com>
>> Cc: Michael S. Tsirkin<mst@redhat.com>
>> Signed-off-by: Stefan Weil<weil@mail.berlios.de>
>> ---
>>   hw/pci.c |   58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 files changed, 58 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/pci.c b/hw/pci.c
>> index 1280d4d..139eb24 100644
>> --- a/hw/pci.c
>> +++ b/hw/pci.c
>> @@ -1797,6 +1797,62 @@ static void pci_map_option_rom(PCIDevice *pdev, int region_num, pcibus_t addr, p
>>       cpu_register_physical_memory(addr, size, pdev->rom_offset);
>>   }
>>
>> +/* 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, int size)
>>      
> let's return an error code on malformed roms so management can detect errors?
>    

A bad/missing PnP header does not mean it's an invalid ROM.

Regards,

Anthony Liguori
Anthony Liguori - Oct. 18, 2010, 6:44 p.m.
On 10/18/2010 12:55 PM, Stefan Weil wrote:
> PCI devices with different vendor or device ids sometimes share
> the same rom code. Only the ids and the checksum
> differs in a boot rom for such devices.
>
> The i825xx ethernet controller family is a typical example
> which is implemented in hw/eepro100.c. It uses at least
> 3 different device ids, so normally 3 boot roms would be needed.
>
> By automatically patching vendor id and device id (and the checksum)
> in qemu, all emulated family members can share the same boot rom.
>
> VGA bios roms are another example with different vendor and device ids.
>
> v2:
>
> * Patch also the vendor id (and remove the sanity check for vendor id).
>
> Cc: Gerd Hoffmann<kraxel@redhat.com>
> Cc: Markus Armbruster<armbru@redhat.com>
> Cc: Michael S. Tsirkin<mst@redhat.com>
> Signed-off-by: Stefan Weil<weil@mail.berlios.de>
>    

I get very nervous about patching a ROM.  Who's to say that the ROM 
doesn't somehow depend on the contents of its header?  Maybe it has an 
internal CRC built into it or something like that.

Regards,

Anthony Liguori

> ---
>   hw/pci.c |   58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 files changed, 58 insertions(+), 0 deletions(-)
>
> diff --git a/hw/pci.c b/hw/pci.c
> index 1280d4d..139eb24 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -1797,6 +1797,62 @@ static void pci_map_option_rom(PCIDevice *pdev, int region_num, pcibus_t addr, p
>       cpu_register_physical_memory(addr, size, pdev->rom_offset);
>   }
>
> +/* 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, int size)
> +{
> +    uint16_t vendor_id;
> +    uint16_t device_id;
> +    uint16_t rom_vendor_id;
> +    uint16_t rom_device_id;
> +    uint16_t rom_magic;
> +    uint16_t pcir_offset;
> +    uint8_t checksum;
> +
> +    /* Words in rom data are little endian (like in PCI configuration),
> +       so they can be read / written with pci_get_word / pci_set_word. */
> +
> +    /* Only a valid rom will be patched. */
> +    rom_magic = pci_get_word(ptr);
> +    if (rom_magic != 0xaa55) {
> +        PCI_DPRINTF("Bad ROM magic %04x\n", rom_magic);
> +        return;
> +    }
> +    pcir_offset = pci_get_word(ptr + 0x18);
> +    if (pcir_offset + 8>= size || memcmp(ptr + pcir_offset, "PCIR", 4)) {
> +        PCI_DPRINTF("Bad PCIR offset 0x%x or signature\n", pcir_offset);
> +        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);
> +    rom_device_id = pci_get_word(ptr + pcir_offset + 6);
> +
> +    PCI_DPRINTF("ROM id %04x%04x / PCI id %04x%04x\n",
> +                vendor_id, device_id, rom_vendor_id, rom_device_id);
> +
> +    checksum = ptr[6];
> +
> +    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);
> +        PCI_DPRINTF("ROM checksum %02x / %02x\n", ptr[6], checksum);
> +        ptr[6] = checksum;
> +        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);
> +        PCI_DPRINTF("ROM checksum %02x / %02x\n", ptr[6], checksum);
> +        ptr[6] = checksum;
> +        pci_set_word(ptr + pcir_offset + 6, device_id);
> +    }
> +}
> +
>   /* Add an option rom for the device */
>   static int pci_add_option_rom(PCIDevice *pdev)
>   {
> @@ -1849,6 +1905,8 @@ static int pci_add_option_rom(PCIDevice *pdev)
>       load_image(path, ptr);
>       qemu_free(path);
>
> +    pci_patch_ids(pdev, ptr, size);
> +
>       pci_register_bar(pdev, PCI_ROM_SLOT, size,
>                        0, pci_map_option_rom);
>
>
Anthony Liguori - Oct. 18, 2010, 6:53 p.m.
On 10/18/2010 01:44 PM, Anthony Liguori wrote:
> On 10/18/2010 12:55 PM, Stefan Weil wrote:
>> PCI devices with different vendor or device ids sometimes share
>> the same rom code. Only the ids and the checksum
>> differs in a boot rom for such devices.
>>
>> The i825xx ethernet controller family is a typical example
>> which is implemented in hw/eepro100.c. It uses at least
>> 3 different device ids, so normally 3 boot roms would be needed.
>>
>> By automatically patching vendor id and device id (and the checksum)
>> in qemu, all emulated family members can share the same boot rom.
>>
>> VGA bios roms are another example with different vendor and device ids.
>>
>> v2:
>>
>> * Patch also the vendor id (and remove the sanity check for vendor id).
>>
>> Cc: Gerd Hoffmann<kraxel@redhat.com>
>> Cc: Markus Armbruster<armbru@redhat.com>
>> Cc: Michael S. Tsirkin<mst@redhat.com>
>> Signed-off-by: Stefan Weil<weil@mail.berlios.de>
>
> I get very nervous about patching a ROM.  Who's to say that the ROM 
> doesn't somehow depend on the contents of its header?  Maybe it has an 
> internal CRC built into it or something like that.

As part of PMM, ROMs typically reduce their size by decompressing and 
removing code or something of that nature and then rewrite themselves in 
scratch RAM.  The BIOS then copies the resulting ROM (using the ROM size 
in the base header as an indication of how much to copy) into the option 
ROM space.

So the likelihood of depending on the contents of the header seems 
non-trivial to me.

Regards,

Anthony Liguori

> Regards,
>
> Anthony Liguori
>
>> ---
>>   hw/pci.c |   58 
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 files changed, 58 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/pci.c b/hw/pci.c
>> index 1280d4d..139eb24 100644
>> --- a/hw/pci.c
>> +++ b/hw/pci.c
>> @@ -1797,6 +1797,62 @@ static void pci_map_option_rom(PCIDevice 
>> *pdev, int region_num, pcibus_t addr, p
>>       cpu_register_physical_memory(addr, size, pdev->rom_offset);
>>   }
>>
>> +/* 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, int size)
>> +{
>> +    uint16_t vendor_id;
>> +    uint16_t device_id;
>> +    uint16_t rom_vendor_id;
>> +    uint16_t rom_device_id;
>> +    uint16_t rom_magic;
>> +    uint16_t pcir_offset;
>> +    uint8_t checksum;
>> +
>> +    /* Words in rom data are little endian (like in PCI configuration),
>> +       so they can be read / written with pci_get_word / 
>> pci_set_word. */
>> +
>> +    /* Only a valid rom will be patched. */
>> +    rom_magic = pci_get_word(ptr);
>> +    if (rom_magic != 0xaa55) {
>> +        PCI_DPRINTF("Bad ROM magic %04x\n", rom_magic);
>> +        return;
>> +    }
>> +    pcir_offset = pci_get_word(ptr + 0x18);
>> +    if (pcir_offset + 8>= size || memcmp(ptr + pcir_offset, "PCIR", 
>> 4)) {
>> +        PCI_DPRINTF("Bad PCIR offset 0x%x or signature\n", 
>> pcir_offset);
>> +        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);
>> +    rom_device_id = pci_get_word(ptr + pcir_offset + 6);
>> +
>> +    PCI_DPRINTF("ROM id %04x%04x / PCI id %04x%04x\n",
>> +                vendor_id, device_id, rom_vendor_id, rom_device_id);
>> +
>> +    checksum = ptr[6];
>> +
>> +    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);
>> +        PCI_DPRINTF("ROM checksum %02x / %02x\n", ptr[6], checksum);
>> +        ptr[6] = checksum;
>> +        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);
>> +        PCI_DPRINTF("ROM checksum %02x / %02x\n", ptr[6], checksum);
>> +        ptr[6] = checksum;
>> +        pci_set_word(ptr + pcir_offset + 6, device_id);
>> +    }
>> +}
>> +
>>   /* Add an option rom for the device */
>>   static int pci_add_option_rom(PCIDevice *pdev)
>>   {
>> @@ -1849,6 +1905,8 @@ static int pci_add_option_rom(PCIDevice *pdev)
>>       load_image(path, ptr);
>>       qemu_free(path);
>>
>> +    pci_patch_ids(pdev, ptr, size);
>> +
>>       pci_register_bar(pdev, PCI_ROM_SLOT, size,
>>                        0, pci_map_option_rom);
>>
>
>
Michael S. Tsirkin - Oct. 18, 2010, 7:03 p.m.
On Mon, Oct 18, 2010 at 01:42:06PM -0500, Anthony Liguori wrote:
> >>+/* 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, int size)
> >let's return an error code on malformed roms so management can detect errors?
> 
> A bad/missing PnP header does not mean it's an invalid ROM.

I don't see this as a generic capability - rather a specific
hack that helps reduce some duplication for eepro100 and friends.
As such, if we can't patch the id we know it's an invalid file.
Stefan Weil - Oct. 18, 2010, 7:11 p.m.
Am 18.10.2010 20:53, schrieb Anthony Liguori:
> On 10/18/2010 01:44 PM, Anthony Liguori wrote:
>> On 10/18/2010 12:55 PM, Stefan Weil wrote:
>>> PCI devices with different vendor or device ids sometimes share
>>> the same rom code. Only the ids and the checksum
>>> differs in a boot rom for such devices.
>>>
>>> The i825xx ethernet controller family is a typical example
>>> which is implemented in hw/eepro100.c. It uses at least
>>> 3 different device ids, so normally 3 boot roms would be needed.
>>>
>>> By automatically patching vendor id and device id (and the checksum)
>>> in qemu, all emulated family members can share the same boot rom.
>>>
>>> VGA bios roms are another example with different vendor and device ids.
>>>
>>> v2:
>>>
>>> * Patch also the vendor id (and remove the sanity check for vendor id).
>>>
>>> Cc: Gerd Hoffmann<kraxel@redhat.com>
>>> Cc: Markus Armbruster<armbru@redhat.com>
>>> Cc: Michael S. Tsirkin<mst@redhat.com>
>>> Signed-off-by: Stefan Weil<weil@mail.berlios.de>
>>
>> I get very nervous about patching a ROM.  Who's to say that the ROM 
>> doesn't somehow depend on the contents of its header?  Maybe it has 
>> an internal CRC built into it or something like that.
>
> As part of PMM, ROMs typically reduce their size by decompressing and 
> removing code or something of that nature and then rewrite themselves 
> in scratch RAM.  The BIOS then copies the resulting ROM (using the ROM 
> size in the base header as an indication of how much to copy) into the 
> option ROM space.
>
> So the likelihood of depending on the contents of the header seems 
> non-trivial to me.
>
> Regards,
>
> Anthony Liguori
>
[snip]

Etherboot uses compressed code and always fixes the checksum by modifying
the byte at relative address 6, so for etherboot there is no problem.
The etherboot distribution even includes a perl script which can be used
to patch vendor/device ids. I thought about using that script in QEMU's
make but then decided against this alternate solution.

VGA bios seems to work, too (practical test still is missing).

What could happen for other kinds of roms? Either there is nothing
to patch (the 99 % standard case), or they work, or they don't work.
QEMU must only make sure that patching of the supported roms
with supported devices work.

Regards,

Stefan Weil
Stefan Weil - Oct. 18, 2010, 7:36 p.m.
Am 18.10.2010 21:03, schrieb Michael S. Tsirkin:
> On Mon, Oct 18, 2010 at 01:42:06PM -0500, Anthony Liguori wrote:
>>>> +/* 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, int size)
>>> let's return an error code on malformed roms so management can 
>>> detect errors?
>>
>> A bad/missing PnP header does not mean it's an invalid ROM.
>
> I don't see this as a generic capability - rather a specific
> hack that helps reduce some duplication for eepro100 and friends.
> As such, if we can't patch the id we know it's an invalid file.

There is already some kind of error feedback: the rom will not work.
For etherboot roms, booting from network won't work.

This is a qemu internal error, so more error handling is not needed.

Users who configure a device with their own rom file don't
need an id patch, and their rom data will not be patched
because they normally specify a rom file with correct ids.
For the rare case where they configure a rom with a "wrong"
id, their rom data will be patched (something they don't expect)
or not modified because of the sanity checks (then the rom
is ignored by the bios).

Maybe a more perfect solution would only patch the preconfigured
rom files but not user configured files, but I don't think we
need this degree of perfection.

Regards,
Stefan
Anthony Liguori - Oct. 18, 2010, 7:56 p.m.
On 10/18/2010 02:03 PM, Michael S. Tsirkin wrote:
> On Mon, Oct 18, 2010 at 01:42:06PM -0500, Anthony Liguori wrote:
>    
>>>> +/* 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, int size)
>>>>          
>>> let's return an error code on malformed roms so management can detect errors?
>>>        
>> A bad/missing PnP header does not mean it's an invalid ROM.
>>      
> I don't see this as a generic capability - rather a specific
> hack that helps reduce some duplication for eepro100 and friends.
> As such, if we can't patch the id we know it's an invalid file.
>    

This code is unconditional in the pci option rom loading path.

If it's restricted to a qdev property that's defaulted to enabled for 
the eepro cards, that would be a reasonable argument to make.

Regards,

Anthony Liguori
Anthony Liguori - Oct. 18, 2010, 7:59 p.m.
On 10/18/2010 02:36 PM, Stefan Weil wrote:
> Maybe a more perfect solution would only patch the preconfigured
> rom files but not user configured files, but I don't think we
> need this degree of perfection.

Generally speaking, patching third-party code is not something that we 
should get in the habit of doing unless we're very very sure that it's 
okay and we have as many checks in place as possible to avoid bad things 
from happening.

There are so many bad things that can happen.  If attempted to support 
attestation in QEMU and prepopulated a virtual TPM with checksums from 
the BIOS and ROMs, when the virtual BIOS attempts to measure itself if 
we've patched the ROM underneath of it, then the measurements will fail.

In the very least, if we go this route, it has to be an optional feature.

Regards,

Anthony Liguori

> Regards,
> Stefan
>
Gerd Hoffmann - Oct. 19, 2010, 6:40 a.m.
On 10/18/10 21:36, Stefan Weil wrote:
> There is already some kind of error feedback: the rom will not work.
> For etherboot roms, booting from network won't work.

VGA works, after hacking the vgabios to not have the PCI ID hardcoded 
elsewhere.

Nevertheless /me gets the feeling that we better should not take that 
route.  vgabios needs special patching to work.  etherboot does not work 
as-is.  Even if we make it work now it always will be fragile.  The next 
rom update might break it again.  The ID automagically adapting doesn't 
happen on real hardware ...

cheers,
   Gerd
Michael S. Tsirkin - Oct. 19, 2010, 8:37 a.m.
On Mon, Oct 18, 2010 at 09:11:55PM +0200, Stefan Weil wrote:
> QEMU must only make sure that patching of the supported roms
> with supported devices work.

I think that's what Anthony was saying too - make this depend
on a qdev property and set it only in eepro100 for now.
Stefan Weil - Oct. 19, 2010, 9:15 p.m.
Am 19.10.2010 10:37, schrieb Michael S. Tsirkin:
> On Mon, Oct 18, 2010 at 09:11:55PM +0200, Stefan Weil wrote:
>> QEMU must only make sure that patching of the supported roms
>> with supported devices work.
>
> I think that's what Anthony was saying too - make this depend
> on a qdev property and set it only in eepro100 for now.
>

My new patch v3 implements something similar and does not need
a new qdev property:

Don't patch because the rom file was defined by the user:

qemu -L pc-bios -boot n -netdev user,id=internet \
     -device i82801,netdev=internet,romfile=gpxe-eepro100-80861209.rom

Patch because we work with the built-in default rom file:

qemu -L pc-bios -boot n -netdev user,id=internet \
     -device i82801,netdev=internet

This is a safe solution which respects user's rom data
without adding much more complexity.

Regards,
Stefan
Anthony Liguori - Oct. 19, 2010, 9:22 p.m.
On 10/19/2010 04:15 PM, Stefan Weil wrote:
> Am 19.10.2010 10:37, schrieb Michael S. Tsirkin:
>> On Mon, Oct 18, 2010 at 09:11:55PM +0200, Stefan Weil wrote:
>>> QEMU must only make sure that patching of the supported roms
>>> with supported devices work.
>>
>> I think that's what Anthony was saying too - make this depend
>> on a qdev property and set it only in eepro100 for now.
>>
>
> My new patch v3 implements something similar and does not need
> a new qdev property:

I prefer to have an explicit property in case a user actually wants to 
use this functionality.   That said, if Michael's happy with the 
approach, I'm okay with it too.

Regards,

Anthony Liguori

> Don't patch because the rom file was defined by the user:
>
> qemu -L pc-bios -boot n -netdev user,id=internet \
>     -device i82801,netdev=internet,romfile=gpxe-eepro100-80861209.rom
>
> Patch because we work with the built-in default rom file:
>
> qemu -L pc-bios -boot n -netdev user,id=internet \
>     -device i82801,netdev=internet
>
> This is a safe solution which respects user's rom data
> without adding much more complexity.
>
> Regards,
> Stefan
>
Michael S. Tsirkin - Oct. 19, 2010, 9:25 p.m.
On Tue, Oct 19, 2010 at 04:22:23PM -0500, Anthony Liguori wrote:
> On 10/19/2010 04:15 PM, Stefan Weil wrote:
> >Am 19.10.2010 10:37, schrieb Michael S. Tsirkin:
> >>On Mon, Oct 18, 2010 at 09:11:55PM +0200, Stefan Weil wrote:
> >>>QEMU must only make sure that patching of the supported roms
> >>>with supported devices work.
> >>
> >>I think that's what Anthony was saying too - make this depend
> >>on a qdev property and set it only in eepro100 for now.
> >>
> >
> >My new patch v3 implements something similar and does not need
> >a new qdev property:
> 
> I prefer to have an explicit property in case a user actually wants
> to use this functionality.   That said, if Michael's happy with the
> approach, I'm okay with it too.
> 
> Regards,
> 
> Anthony Liguori

I currently feel kind of the same, just to make this explicit ...
if you like, let me sleep on it.

> >Don't patch because the rom file was defined by the user:
> >
> >qemu -L pc-bios -boot n -netdev user,id=internet \
> >    -device i82801,netdev=internet,romfile=gpxe-eepro100-80861209.rom
> >
> >Patch because we work with the built-in default rom file:
> >
> >qemu -L pc-bios -boot n -netdev user,id=internet \
> >    -device i82801,netdev=internet
> >
> >This is a safe solution which respects user's rom data
> >without adding much more complexity.
> >
> >Regards,
> >Stefan
> >

Patch

differs in a boot rom for such devices.

The i825xx ethernet controller family is a typical example
which is implemented in hw/eepro100.c. It uses at least
3 different device ids, so normally 3 boot roms would be needed.

By automatically patching vendor id and device id (and the checksum)
in qemu, all emulated family members can share the same boot rom.

VGA bios roms are another example with different vendor and device ids.

v2:

* Patch also the vendor id (and remove the sanity check for vendor id).

Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 hw/pci.c |   58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 58 insertions(+), 0 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index 1280d4d..139eb24 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -1797,6 +1797,62 @@  static void pci_map_option_rom(PCIDevice *pdev, int region_num, pcibus_t addr, p
     cpu_register_physical_memory(addr, size, pdev->rom_offset);
 }
 
+/* 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, int size)
+{
+    uint16_t vendor_id;
+    uint16_t device_id;
+    uint16_t rom_vendor_id;
+    uint16_t rom_device_id;
+    uint16_t rom_magic;
+    uint16_t pcir_offset;
+    uint8_t checksum;
+
+    /* Words in rom data are little endian (like in PCI configuration),
+       so they can be read / written with pci_get_word / pci_set_word. */
+
+    /* Only a valid rom will be patched. */
+    rom_magic = pci_get_word(ptr);
+    if (rom_magic != 0xaa55) {
+        PCI_DPRINTF("Bad ROM magic %04x\n", rom_magic);
+        return;
+    }
+    pcir_offset = pci_get_word(ptr + 0x18);
+    if (pcir_offset + 8 >= size || memcmp(ptr + pcir_offset, "PCIR", 4)) {
+        PCI_DPRINTF("Bad PCIR offset 0x%x or signature\n", pcir_offset);
+        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);
+    rom_device_id = pci_get_word(ptr + pcir_offset + 6);
+
+    PCI_DPRINTF("ROM id %04x%04x / PCI id %04x%04x\n",
+                vendor_id, device_id, rom_vendor_id, rom_device_id);
+
+    checksum = ptr[6];
+
+    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);
+        PCI_DPRINTF("ROM checksum %02x / %02x\n", ptr[6], checksum);
+        ptr[6] = checksum;
+        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);
+        PCI_DPRINTF("ROM checksum %02x / %02x\n", ptr[6], checksum);
+        ptr[6] = checksum;
+        pci_set_word(ptr + pcir_offset + 6, device_id);
+    }
+}
+
 /* Add an option rom for the device */
 static int pci_add_option_rom(PCIDevice *pdev)
 {
@@ -1849,6 +1905,8 @@  static int pci_add_option_rom(PCIDevice *pdev)
     load_image(path, ptr);
     qemu_free(path);
 
+    pci_patch_ids(pdev, ptr, size);
+
     pci_register_bar(pdev, PCI_ROM_SLOT, size,
                      0, pci_map_option_rom);