| Message ID | 1261134074-11795-3-git-send-email-kraxel@redhat.com |
|---|---|
| State | New |
| Headers | show |
The current names of GPXE romfiles are something like pxe-e1000.bin, pxe-ne2k_pci.bin, pxe-rtl8139.bin. This was adequate when these names were computed by a simple rule using the device name. Today, an ethernet device can be associated to any romfile name. Etherboot's Rom-o-Matic (which creates qemu's romfiles) creates names like gpxe-0.9.9-80861209.rom. I don't think it would be good to use etherboot's names because they contain the gpxe version (0.9.9) which might change. But a modified name without the gpxe version like gpxe-80861209.rom would have some advantages: * gpxe* is better than pxe* because the files contain a gPXE boot ROM - not a proprietary PXE ROM. * The romfiles are ROM files, not undefined binaries, so *.rom looks better than *.bin. * For drivers like eepro100.c which implement several devices, a naming rule based on PCI device and vendor id (80861209) is better than a rule based on device names: devices with same ids can share the same romfile. * Transforming an etherboot romfile name to a qemu romfile name is simple when all you have to do is to remove the version. This would also simplify pc-bios/README. The proposed new names should be used for new romfiles. Optionally, the existing romfiles could be renamed. Please tell me what you think of these suggestions. Regards Stefan Weil PS. Mixing ROM data for different hosts in one directory (as it is done today) could be improved, too: * Separate directories for different hosts. * Include the host in the romfile name (as it is done for the openbios roms). * pc-bios was historically correct but is no longer adequate. As far as I remember, there had already been some discussion on these points some time ago.
On 01/11/2010 03:18 PM, Stefan Weil wrote: > The current names of GPXE romfiles are something like > pxe-e1000.bin, pxe-ne2k_pci.bin, pxe-rtl8139.bin. > > This was adequate when these names were computed > by a simple rule using the device name. > > Today, an ethernet device can be associated to any > romfile name. > > Etherboot's Rom-o-Matic (which creates qemu's romfiles) > creates names like gpxe-0.9.9-80861209.rom. > > I don't think it would be good to use etherboot's names > because they contain the gpxe version (0.9.9) which > might change. > > But a modified name without the gpxe version like > gpxe-80861209.rom would have some advantages: > > * gpxe* is better than pxe* because the files contain > a gPXE boot ROM - not a proprietary PXE ROM. > > * The romfiles are ROM files, not undefined binaries, > so *.rom looks better than *.bin. > > * For drivers like eepro100.c which implement several devices, > a naming rule based on PCI device and vendor id (80861209) > is better than a rule based on device names: > devices with same ids can share the same romfile. > > * Transforming an etherboot romfile name to a qemu romfile name > is simple when all you have to do is to remove the version. > This would also simplify pc-bios/README. > > The proposed new names should be used for new romfiles. > Optionally, the existing romfiles could be renamed. > > Please tell me what you think of these suggestions. > I don't feel strongly either way. Regards, Anthony Liguori
Am 11.01.2010 22:18, schrieb Stefan Weil: > The current names of GPXE romfiles are something like > pxe-e1000.bin, pxe-ne2k_pci.bin, pxe-rtl8139.bin. > > This was adequate when these names were computed > by a simple rule using the device name. > > Today, an ethernet device can be associated to any > romfile name. > > Etherboot's Rom-o-Matic (which creates qemu's romfiles) > creates names like gpxe-0.9.9-80861209.rom. > > I don't think it would be good to use etherboot's names > because they contain the gpxe version (0.9.9) which > might change. > > But a modified name without the gpxe version like > gpxe-80861209.rom would have some advantages: > > * gpxe* is better than pxe* because the files contain > a gPXE boot ROM - not a proprietary PXE ROM. > > * The romfiles are ROM files, not undefined binaries, > so *.rom looks better than *.bin. > > * For drivers like eepro100.c which implement several devices, > a naming rule based on PCI device and vendor id (80861209) > is better than a rule based on device names: > devices with same ids can share the same romfile. I dislike this part of your suggestion. Everyone knows what a pcnet is, but I guess most people don't know its PCI device/vendor ID. So it would add files that most people can't associate with anything specific. Maybe you could name your files gpxe-eepro100-vendor_dev.rom and just leave vendor_dev out for devices with just one ID? I'm fine with the other suggestions, though. Kevin
diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c index b08d2ae..6fe433d 100644 --- a/hw/cirrus_vga.c +++ b/hw/cirrus_vga.c @@ -3209,9 +3209,6 @@ static int pci_cirrus_vga_initfn(PCIDevice *dev) pci_register_bar((PCIDevice *)d, 1, CIRRUS_PNPMMIO_SIZE, PCI_BASE_ADDRESS_SPACE_MEMORY, cirrus_pci_mmio_map); } - - /* ROM BIOS */ - pci_add_option_rom((PCIDevice *)d, VGABIOS_CIRRUS_FILENAME); return 0; } @@ -3226,6 +3223,7 @@ static PCIDeviceInfo cirrus_vga_info = { .qdev.size = sizeof(PCICirrusVGAState), .qdev.vmsd = &vmstate_pci_cirrus_vga, .init = pci_cirrus_vga_initfn, + .romfile = VGABIOS_CIRRUS_FILENAME, .config_write = pci_cirrus_write_config, }; diff --git a/hw/e1000.c b/hw/e1000.c index f795601..33c4bc6 100644 --- a/hw/e1000.c +++ b/hw/e1000.c @@ -1121,14 +1121,6 @@ static int pci_e1000_init(PCIDevice *pci_dev) d->dev.qdev.info->name, d->dev.qdev.id, d); qemu_format_nic_info_str(&d->nic->nc, macaddr); - - if (!pci_dev->qdev.hotplugged) { - static int loaded = 0; - if (!loaded) { - pci_add_option_rom(&d->dev, "pxe-e1000.bin"); - loaded = 1; - } - } return 0; } @@ -1146,6 +1138,7 @@ static PCIDeviceInfo e1000_info = { .qdev.vmsd = &vmstate_e1000, .init = pci_e1000_init, .exit = pci_e1000_uninit, + .romfile = "pxe-e1000.bin", .qdev.props = (Property[]) { DEFINE_NIC_PROPERTIES(E1000State, conf), DEFINE_PROP_END_OF_LIST(), diff --git a/hw/pci.c b/hw/pci.c index dbdfdbf..d54f05e 100644 --- a/hw/pci.c +++ b/hw/pci.c @@ -63,12 +63,14 @@ static struct BusInfo pci_bus_info = { .print_dev = pcibus_dev_print, .props = (Property[]) { DEFINE_PROP_PCI_DEVFN("addr", PCIDevice, devfn, -1), + DEFINE_PROP_STRING("romfile", PCIDevice, romfile), DEFINE_PROP_END_OF_LIST() } }; static void pci_update_mappings(PCIDevice *d); static void pci_set_irq(void *opaque, int irq_num, int level); +static int pci_add_option_rom(PCIDevice *pdev); target_phys_addr_t pci_mem_base; static uint16_t pci_default_sub_vendor_id = PCI_SUBVENDOR_ID_REDHAT_QUMRANET; @@ -1387,6 +1389,12 @@ static int pci_qdev_init(DeviceState *qdev, DeviceInfo *base) rc = info->init(pci_dev); if (rc != 0) return rc; + + /* rom loading */ + if (pci_dev->romfile == NULL && info->romfile != NULL) + pci_dev->romfile = qemu_strdup(info->romfile); + pci_add_option_rom(pci_dev); + if (qdev->hotplugged) bus->hotplug(pci_dev, 1); return 0; @@ -1470,18 +1478,28 @@ static void pci_map_option_rom(PCIDevice *pdev, int region_num, pcibus_t addr, p } /* Add an option rom for the device */ -int pci_add_option_rom(PCIDevice *pdev, const char *name) +static int pci_add_option_rom(PCIDevice *pdev) { int size; char *path; void *ptr; - path = qemu_find_file(QEMU_FILE_TYPE_BIOS, name); + if (!pdev->romfile) + return 0; + if (strlen(pdev->romfile) == 0) + return 0; + + path = qemu_find_file(QEMU_FILE_TYPE_BIOS, pdev->romfile); if (path == NULL) { - path = qemu_strdup(name); + path = qemu_strdup(pdev->romfile); } size = get_image_size(path); + if (size < 0) { + qemu_error("%s: failed to find romfile \"%s\"\n", __FUNCTION__, + pdev->romfile); + return -1; + } if (size & (size - 1)) { size = 1 << qemu_fls(size); } diff --git a/hw/pci.h b/hw/pci.h index 89b3f55..39da7df 100644 --- a/hw/pci.h +++ b/hw/pci.h @@ -245,6 +245,7 @@ struct PCIDevice { int32_t version_id; /* Location of option rom */ + char *romfile; ram_addr_t rom_offset; }; @@ -257,8 +258,6 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num, pcibus_t size, int type, PCIMapIORegionFunc *map_func); -int pci_add_option_rom(PCIDevice *pdev, const char *name); - int pci_add_capability(PCIDevice *pci_dev, uint8_t cap_id, uint8_t cap_size); void pci_del_capability(PCIDevice *pci_dev, uint8_t cap_id, uint8_t cap_size); @@ -386,6 +385,9 @@ typedef struct { /* pcie stuff */ int is_express; /* is this device pci express? */ + + /* rom bar */ + const char *romfile; } PCIDeviceInfo; void pci_qdev_register(PCIDeviceInfo *info); diff --git a/hw/rtl8139.c b/hw/rtl8139.c index 2cee97b..fcdcd1d 100644 --- a/hw/rtl8139.c +++ b/hw/rtl8139.c @@ -3353,14 +3353,6 @@ static int pci_rtl8139_init(PCIDevice *dev) qemu_mod_timer(s->timer, rtl8139_get_next_tctr_time(s,qemu_get_clock(vm_clock))); #endif /* RTL8139_ONBOARD_TIMER */ - - if (!dev->qdev.hotplugged) { - static int loaded = 0; - if (!loaded) { - pci_add_option_rom(&s->dev, "pxe-rtl8139.bin"); - loaded = 1; - } - } return 0; } @@ -3371,6 +3363,7 @@ static PCIDeviceInfo rtl8139_info = { .qdev.vmsd = &vmstate_rtl8139, .init = pci_rtl8139_init, .exit = pci_rtl8139_uninit, + .romfile = "pxe-rtl8139.bin", .qdev.props = (Property[]) { DEFINE_NIC_PROPERTIES(RTL8139State, conf), DEFINE_PROP_END_OF_LIST(), diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c index 85f14a2..62b46bd 100644 --- a/hw/virtio-pci.c +++ b/hw/virtio-pci.c @@ -518,14 +518,6 @@ static int virtio_net_init_pci(PCIDevice *pci_dev) /* make the actual value visible */ proxy->nvectors = vdev->nvectors; - - if (!pci_dev->qdev.hotplugged) { - static int loaded = 0; - if (!loaded) { - pci_add_option_rom(pci_dev, "pxe-virtio.bin"); - loaded = 1; - } - } return 0; } @@ -569,6 +561,7 @@ static PCIDeviceInfo virtio_info[] = { .qdev.size = sizeof(VirtIOPCIProxy), .init = virtio_net_init_pci, .exit = virtio_net_exit_pci, + .romfile = "pxe-virtio.bin", .qdev.props = (Property[]) { DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 3), DEFINE_NIC_PROPERTIES(VirtIOPCIProxy, nic),
This patch adds a romfile property to the pci bus. It allows to specify a romfile to load into the rom bar of the pci device. The default value comes from a new field in PCIDeviceInfo. The property allows to change the file and also to disable the rom loading using an empty string. Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- hw/cirrus_vga.c | 4 +--- hw/e1000.c | 9 +-------- hw/pci.c | 24 +++++++++++++++++++++--- hw/pci.h | 6 ++++-- hw/rtl8139.c | 9 +-------- hw/virtio-pci.c | 9 +-------- 6 files changed, 29 insertions(+), 32 deletions(-)