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

login
register
mail settings
Submitter Stefan Weil
Date Oct. 15, 2010, 8:51 p.m.
Message ID <1287175867-7757-1-git-send-email-weil@mail.berlios.de>
Download mbox | patch
Permalink /patch/67998/
State New
Headers show

Comments

Stefan Weil - Oct. 15, 2010, 8:51 p.m.
PCI device with different device ids sometimes share
the same rom code. Only the device id and the checksum
Anthony Liguori - Oct. 15, 2010, 9:05 p.m.
On 10/15/2010 03:51 PM, Stefan Weil wrote:
> PCI device with different device ids sometimes share
> the same rom code. Only the device id and the checksum
> differ in a boot rom for such devices.
>    

BTW, SeaBIOS doesn't reject ROMs when they're loaded via rombar, only 
when they're loaded via romfile.

Maybe it's better to use fw_cfg to explicitly tell SeaBIOS to ignore the 
PCI device id in the rom header for a certain device?

Regards,

Anthony Liguori

> 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 the device id (and the checksum)
> in qemu, all emulated family members can share the same
> boot rom.
>
> 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 |   53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 files changed, 53 insertions(+), 0 deletions(-)
>
> diff --git a/hw/pci.c b/hw/pci.c
> index 1280d4d..c1f8218 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -1797,6 +1797,57 @@ 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 device id 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_device_id(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;
> +
> +    /* 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);
> +
> +    /* Don't patch a rom with wrong vendor id (might be changed if needed). */
> +    if (vendor_id != rom_vendor_id) {
> +        return;
> +    }
> +
> +    PCI_DPRINTF("ROM id %04x%04x / PCI id %04x%04x\n",
> +                vendor_id, device_id, rom_vendor_id, rom_device_id);
> +
> +    if (device_id != rom_device_id) {
> +        /* Patch device id and checksum (at offset 6 for etherboot roms). */
> +        uint8_t checksum;
> +        checksum = ptr[6];
> +        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 +1900,8 @@ static int pci_add_option_rom(PCIDevice *pdev)
>       load_image(path, ptr);
>       qemu_free(path);
>
> +    pci_patch_device_id(pdev, ptr, size);
> +
>       pci_register_bar(pdev, PCI_ROM_SLOT, size,
>                        0, pci_map_option_rom);
>
>
Gerd Hoffmann - Oct. 18, 2010, 10:04 a.m.
Hi,

> +    /* Don't patch a rom with wrong vendor id (might be changed if needed). */
> +    if (vendor_id != rom_vendor_id) {
> +        return;
> +    }

Yes, please drop that one.  If this is accepted I'd like to use this for
vga roms too, so we have to carry only two of them instead of four.

> +    if (device_id != rom_device_id) {
> +        /* Patch device id and checksum (at offset 6 for etherboot roms). */

Does this offset work for all roms?

>   /* Add an option rom for the device */
>   static int pci_add_option_rom(PCIDevice *pdev)
>   {
> @@ -1849,6 +1900,8 @@ static int pci_add_option_rom(PCIDevice *pdev)
>       load_image(path, ptr);
>       qemu_free(path);
>
> +    pci_patch_device_id(pdev, ptr, size);
> +

I'd prefer this being opt-in per driver instead of being applied 
globally (and maybe also pass in a flag whenever a vendor mismatch is 
fine or not).

cheers,
   Gerd
Gerd Hoffmann - Oct. 18, 2010, 10:09 a.m.
On 10/15/10 23:05, Anthony Liguori wrote:
> On 10/15/2010 03:51 PM, Stefan Weil wrote:
>> PCI device with different device ids sometimes share
>> the same rom code. Only the device id and the checksum
>> differ in a boot rom for such devices.
>
> BTW, SeaBIOS doesn't reject ROMs when they're loaded via rombar, only
> when they're loaded via romfile.

SeaBIOS rejects them when loaded from the rom bar and doesn't reject 
them when loaded via fw_cfg.

Using the rom bar is the prefered way though, fw_cfg is only there for 
compatibility with older versions.

> Maybe it's better to use fw_cfg to explicitly tell SeaBIOS to ignore the
> PCI device id in the rom header for a certain device?

Patching the rom is fine IMHO.  Why create + use a separate 
communication path when we can use a much simpler approach?

cheers,
   Gerd
Stefan Weil - Oct. 18, 2010, 11:16 a.m.
Am 18.10.2010 12:04, schrieb Gerd Hoffmann:
>   Hi,
>
>> +    /* Don't patch a rom with wrong vendor id (might be changed if 
>> needed). */
>> +    if (vendor_id != rom_vendor_id) {
>> +        return;
>> +    }
>
> Yes, please drop that one.  If this is accepted I'd like to use this for
> vga roms too, so we have to carry only two of them instead of four.
>
>> +    if (device_id != rom_device_id) {
>> +        /* Patch device id and checksum (at offset 6 for etherboot 
>> roms). */
>
> Does this offset work for all roms?


As far as I know there is no well-defined checksum offset.
The checksum is simply set by modifying any byte (which
normally should be unused).

Etherboot has some unused bytes at the beginning of rom data
and always uses the same offset 6.

For other roms which also don't use the byte at offset 6, this approach
will work, too. If they store code or vital data at that location,
we destroy that data, so it won't work.

The VGA bios roms have a sequence of several bytes of zero
starting at offset 6, so maybe this data is not important and
we may change the byte at offset 6, but that should be checked
before using this mechanism.

>
>>   /* Add an option rom for the device */
>>   static int pci_add_option_rom(PCIDevice *pdev)
>>   {
>> @@ -1849,6 +1900,8 @@ static int pci_add_option_rom(PCIDevice *pdev)
>>       load_image(path, ptr);
>>       qemu_free(path);
>>
>> +    pci_patch_device_id(pdev, ptr, size);
>> +
>
> I'd prefer this being opt-in per driver instead of being applied 
> globally (and maybe also pass in a flag whenever a vendor mismatch is 
> fine or not).
>
> cheers,
>   Gerd

As long as the driver specifies the romfile name,
we get an implicitly defined behaviour: either the
rom matches and nothing special is done, or it doesn't
and the id(s) will be fixed.

So neither flag nor opt-in seems to be needed.
Gerd Hoffmann - Oct. 18, 2010, 11:54 a.m.
Hi,

> As far as I know there is no well-defined checksum offset.
> The checksum is simply set by modifying any byte (which
> normally should be unused).
>
> Etherboot has some unused bytes at the beginning of rom data
> and always uses the same offset 6.

Ah, so you don't actually update the checksum but change some unused 
byte to make the checksum stay the same, right?

> For other roms which also don't use the byte at offset 6, this approach
> will work, too. If they store code or vital data at that location,
> we destroy that data, so it won't work.
>
> The VGA bios roms have a sequence of several bytes of zero
> starting at offset 6, so maybe this data is not important and
> we may change the byte at offset 6, but that should be checked
> before using this mechanism.

 From vgabios:

     .org 0

     vgabios_start:
     .byte  0x55, 0xaa	/* BIOS signature */
     .byte  0x40		/* BIOS extension length */

     vgabios_entry_point:
           jmp vgabios_init_func

 From seabios:

     struct rom_header {
         u16 signature;
         u8 size;
         u8 initVector[4];
         u8 reserved[17];
         u16 pcioffset;
         u16 pnpoffset;
     } PACKED;

Hmm.  So offset 6 is the last byte of initVector.  If (and only if) you 
happen to know that the jump instruction takes 3 bytes only it is save 
to modify the unused 4th byte.  Seems to be true for both vgabios and 
etherboot/gPXE.  We can't assume this in general, although it is quite 
likely given that there hardly would be anything but a 16bit jump.

> As long as the driver specifies the romfile name,
> we get an implicitly defined behaviour: either the
> rom matches and nothing special is done, or it doesn't
> and the id(s) will be fixed.

> So neither flag nor opt-in seems to be needed.

When following this argumentation the vendor id sanity check shouldn't 
be there in the first place ;)

Note that romfile is a pci bus property, so it isn't fully under the 
drivers control because it can be overridden from the command line for 
every pci device.

cheers,
   Gerd
Stefan Weil - Oct. 18, 2010, 1:16 p.m.
Hi,

Am 18.10.2010 13:54, schrieb Gerd Hoffmann:
>   Hi,
>
>> As far as I know there is no well-defined checksum offset.
>> The checksum is simply set by modifying any byte (which
>> normally should be unused).
>>
>> Etherboot has some unused bytes at the beginning of rom data
>> and always uses the same offset 6.
>
> Ah, so you don't actually update the checksum but change some unused 
> byte to make the checksum stay the same, right?

Right. The sum of all bytes modulo 255 must be 0.
Any byte can be modified to achieve this.

>
>> For other roms which also don't use the byte at offset 6, this approach
>> will work, too. If they store code or vital data at that location,
>> we destroy that data, so it won't work.
>>
>> The VGA bios roms have a sequence of several bytes of zero
>> starting at offset 6, so maybe this data is not important and
>> we may change the byte at offset 6, but that should be checked
>> before using this mechanism.
>
> From vgabios:
>
>     .org 0
>
>     vgabios_start:
>     .byte  0x55, 0xaa    /* BIOS signature */
>     .byte  0x40        /* BIOS extension length */
>
>     vgabios_entry_point:
>           jmp vgabios_init_func
>
> From seabios:
>
>     struct rom_header {
>         u16 signature;
>         u8 size;
>         u8 initVector[4];
>         u8 reserved[17];
>         u16 pcioffset;
>         u16 pnpoffset;
>     } PACKED;
>
> Hmm.  So offset 6 is the last byte of initVector.  If (and only if) 
> you happen to know that the jump instruction takes 3 bytes only it is 
> save to modify the unused 4th byte.  Seems to be true for both vgabios 
> and etherboot/gPXE.  We can't assume this in general, although it is 
> quite likely given that there hardly would be anything but a 16bit jump.

I agree. So it would work with vga bios, too.

It looks like vgabios uses the last byte to fix the checksum
(rom data ends with a sequence of 0xff, only last byte is different).


>
>> As long as the driver specifies the romfile name,
>> we get an implicitly defined behaviour: either the
>> rom matches and nothing special is done, or it doesn't
>> and the id(s) will be fixed.
>
>> So neither flag nor opt-in seems to be needed.
>
> When following this argumentation the vendor id sanity check shouldn't 
> be there in the first place ;)

The sanity check is simply there because I had no test case
which patches the vendor id. How could I test with vga bios?

>
> Note that romfile is a pci bus property, so it isn't fully under the 
> drivers control because it can be overridden from the command line for 
> every pci device.

Maybe this is an argument why the driver should not include any flags
for id patching. A user who overrides the rom name from the command line
should know what she/he does.

>
> cheers,
>   Gerd
>


Regards,
Stefan
Gerd Hoffmann - Oct. 18, 2010, 1:30 p.m.
Hi,

>> When following this argumentation the vendor id sanity check shouldn't
>> be there in the first place ;)
>
> The sanity check is simply there because I had no test case
> which patches the vendor id. How could I test with vga bios?

No trivial way as the vgabios needs to be patched to handle that.

The vgabios searches for its hardware, right now the IDs are 
compile-time constants (same constants are compiled into the pci 
header).  Needs to be changed to lookup the ID at runtime in the pci header.

cheers,
   Gerd
Gerd Hoffmann - Oct. 18, 2010, 3:50 p.m.
On 10/18/10 15:30, Gerd Hoffmann wrote:
>   Hi,
>
>>> When following this argumentation the vendor id sanity check shouldn't
>>> be there in the first place ;)
>>
>> The sanity check is simply there because I had no test case
>> which patches the vendor id. How could I test with vga bios?
>
> No trivial way as the vgabios needs to be patched to handle that.

patchrom branches available now:

http://cgit.freedesktop.org/~kraxel/vgabios/log/
http://cgit.freedesktop.org/spice/qemu/log/?h=patchrom

very short instructions:

(1) fetch+compile vgabios, copy new vgabios-pci binary
     so qemu can find it.
(2) fetch qemu, apply/merge id patching, compile qemu
(3) both standard and vmware vga should happily work
     with the same vgabios binary now, including vesa
     graphic modes.

cheers,
   Gerd
Stefan Weil - Oct. 18, 2010, 5:54 p.m.
Am 18.10.2010 17:50, schrieb Gerd Hoffmann:
> On 10/18/10 15:30, Gerd Hoffmann wrote:
>>   Hi,
>>
>>>> When following this argumentation the vendor id sanity check shouldn't
>>>> be there in the first place ;)
>>>
>>> The sanity check is simply there because I had no test case
>>> which patches the vendor id. How could I test with vga bios?
>>
>> No trivial way as the vgabios needs to be patched to handle that.
>
> patchrom branches available now:
>
> http://cgit.freedesktop.org/~kraxel/vgabios/log/
> http://cgit.freedesktop.org/spice/qemu/log/?h=patchrom
>
> very short instructions:
>
> (1) fetch+compile vgabios, copy new vgabios-pci binary
>     so qemu can find it.
> (2) fetch qemu, apply/merge id patching, compile qemu
> (3) both standard and vmware vga should happily work
>     with the same vgabios binary now, including vesa
>     graphic modes.
>
> cheers,
>   Gerd
>

Hi Gerd,

a new patch which also modifies the vendor id will follow
immediately. Perhaps you can try it with your modified vga bios.

Cheers,
Stefan
Anthony Liguori - Oct. 18, 2010, 6:39 p.m.
On 10/18/2010 05:09 AM, Gerd Hoffmann wrote:
> On 10/15/10 23:05, Anthony Liguori wrote:
>> On 10/15/2010 03:51 PM, Stefan Weil wrote:
>>> PCI device with different device ids sometimes share
>>> the same rom code. Only the device id and the checksum
>>> differ in a boot rom for such devices.
>>
>> BTW, SeaBIOS doesn't reject ROMs when they're loaded via rombar, only
>> when they're loaded via romfile.
>
> SeaBIOS rejects them when loaded from the rom bar and doesn't reject 
> them when loaded via fw_cfg.

What I meant was, rombar=0 in qdev.  Sometimes my fingers don't work the 
same way my brain does :-)

> Using the rom bar is the prefered way though, fw_cfg is only there for 
> compatibility with older versions.
>
>> Maybe it's better to use fw_cfg to explicitly tell SeaBIOS to ignore the
>> PCI device id in the rom header for a certain device?
>
> Patching the rom is fine IMHO.  Why create + use a separate 
> communication path when we can use a much simpler approach?

How does this interact with PCI device passthrough?

We clearly can't patch in that case whereas if we had a hint to SeaBIOS, 
it would still work.

Regards,

Anthony Liguori

> cheers,
>   Gerd
>
Avi Kivity - Oct. 21, 2010, 10:09 a.m.
On 10/18/2010 08:39 PM, Anthony Liguori wrote:
>> SeaBIOS rejects them when loaded from the rom bar and doesn't reject 
>> them when loaded via fw_cfg.
>
>
> What I meant was, rombar=0 in qdev.  Sometimes my fingers don't work 
> the same way my brain does :-)


Are you using qmp or the human monitor protocol?
Anthony Liguori - Oct. 21, 2010, 1:14 p.m.
On 10/21/2010 05:09 AM, Avi Kivity wrote:
>  On 10/18/2010 08:39 PM, Anthony Liguori wrote:
>>> SeaBIOS rejects them when loaded from the rom bar and doesn't reject 
>>> them when loaded via fw_cfg.
>>
>>
>> What I meant was, rombar=0 in qdev.  Sometimes my fingers don't work 
>> the same way my brain does :-)
>
>
> Are you using qmp or the human monitor protocol?

I'm still running on a DCE/RPC implementation from the early 80s.

Regards,

Anthony Liguori
Avi Kivity - Oct. 21, 2010, 3:34 p.m.
On 10/21/2010 03:14 PM, Anthony Liguori wrote:
> On 10/21/2010 05:09 AM, Avi Kivity wrote:
>>  On 10/18/2010 08:39 PM, Anthony Liguori wrote:
>>>> SeaBIOS rejects them when loaded from the rom bar and doesn't 
>>>> reject them when loaded via fw_cfg.
>>>
>>>
>>> What I meant was, rombar=0 in qdev.  Sometimes my fingers don't work 
>>> the same way my brain does :-)
>>
>>
>> Are you using qmp or the human monitor protocol?
>
> I'm still running on a DCE/RPC implementation from the early 80s.
>

Well, I hope you keep it up to date.  I wouldn't want a vulnerability 
inserted into qemu by an attacker controlling a maintainer's hands.

Patch

differ 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 the device id (and the checksum)
in qemu, all emulated family members can share the same
boot rom.

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 |   53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 53 insertions(+), 0 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index 1280d4d..c1f8218 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -1797,6 +1797,57 @@  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 device id 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_device_id(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;
+
+    /* 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);
+
+    /* Don't patch a rom with wrong vendor id (might be changed if needed). */
+    if (vendor_id != rom_vendor_id) {
+        return;
+    }
+
+    PCI_DPRINTF("ROM id %04x%04x / PCI id %04x%04x\n",
+                vendor_id, device_id, rom_vendor_id, rom_device_id);
+
+    if (device_id != rom_device_id) {
+        /* Patch device id and checksum (at offset 6 for etherboot roms). */
+        uint8_t checksum;
+        checksum = ptr[6];
+        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 +1900,8 @@  static int pci_add_option_rom(PCIDevice *pdev)
     load_image(path, ptr);
     qemu_free(path);
 
+    pci_patch_device_id(pdev, ptr, size);
+
     pci_register_bar(pdev, PCI_ROM_SLOT, size,
                      0, pci_map_option_rom);