| Message ID | 1287175867-7757-1-git-send-email-weil@mail.berlios.de |
|---|---|
| State | New |
| Headers | show |
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); > >
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
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
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.
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
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
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
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
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
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 >
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?
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
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.
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);