From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53366) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1akZTi-0003Cd-Ef for qemu-devel@nongnu.org; Mon, 28 Mar 2016 11:53:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1akZTb-0008Fu-MF for qemu-devel@nongnu.org; Mon, 28 Mar 2016 11:53:18 -0400 Received: from mx1.redhat.com ([209.132.183.28]:48155) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1akZTb-0008Ei-EL for qemu-devel@nongnu.org; Mon, 28 Mar 2016 11:53:11 -0400 Date: Mon, 28 Mar 2016 18:53:03 +0300 From: "Michael S. Tsirkin" Message-ID: <20160328185217-mutt-send-email-mst@redhat.com> References: <1459176138-18665-1-git-send-email-minyard@acm.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1459176138-18665-1-git-send-email-minyard@acm.org> Subject: Re: [Qemu-devel] [PATCH v5] Sort the fw_cfg file list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: minyard@acm.org Cc: Paolo Bonzini , Corey Minyard , Gerd Hoffmann , qemu-devel@nongnu.org On Mon, Mar 28, 2016 at 09:42:18AM -0500, minyard@acm.org wrote: > From: Gerd Hoffmann > > Entries are inserted in filename order instead of being > appended to the end in case sorting is enabled. > > This will avoid any future issues of moving the file creation > around, it doesn't matter what order they are created now, > the will always be in filename order. > > Signed-off-by: Gerd Hoffmann > > Added machine type handling for compatibility. This was > a fairly complex change, this will preserve the order of fw_cfg > for older versions no matter what order the firmware files > actually come in. A list is kept of the correct legacy order > and the entries will be inserted based upon their order in > the list. Except that some entries are ordered (in a specific > area of the list) based upon what order they appear on the > command line. Special handling is added for those entries. > > Signed-off-by: Corey Minyard Reviewed-by: Michael S. Tsirkin Gerd, will you pick this up, or should I? > --- > > Resend, as I left out the v5 in the header. > > Changes from v4: > > * Add a space in fw_cfg.h to improve readability. > > * Move the set order override for user file out of the main function > in vl.c to parse_fw_cfg(). That way it has the fw_cfg and can > call the fww_cfg_set_order_override() function directly. > > * Added a ROM I missed: genroms/kvmvapic.bin. I looked around > some more and I couldn't find anything else. I spent some time > trying lots of different configurations and didn't find anything > new. > > I'm not sure who should merge this. > > hw/core/loader.c | 10 ++++ > hw/i386/pc.c | 4 ++ > hw/i386/pc_piix.c | 1 + > hw/i386/pc_q35.c | 1 + > hw/nvram/fw_cfg.c | 131 +++++++++++++++++++++++++++++++++++++++++++--- > include/hw/boards.h | 3 +- > include/hw/loader.h | 2 + > include/hw/nvram/fw_cfg.h | 8 +++ > vl.c | 10 +++- > 9 files changed, 159 insertions(+), 11 deletions(-) > > diff --git a/hw/core/loader.c b/hw/core/loader.c > index 8e8031c..8a3d518 100644 > --- a/hw/core/loader.c > +++ b/hw/core/loader.c > @@ -1051,6 +1051,16 @@ void rom_set_fw(FWCfgState *f) > fw_cfg = f; > } > > +void rom_set_order_override(int order) > +{ > + fw_cfg_set_order_override(fw_cfg, order); > +} > + > +void rom_reset_order_override(void) > +{ > + fw_cfg_reset_order_override(fw_cfg); > +} > + > static Rom *find_rom(hwaddr addr) > { > Rom *rom; > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index 56ec6cd..aa3f4f3 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -1406,6 +1406,7 @@ DeviceState *pc_vga_init(ISABus *isa_bus, PCIBus *pci_bus) > { > DeviceState *dev = NULL; > > + rom_set_order_override(FW_CFG_ORDER_OVERRIDE_VGA); > if (pci_bus) { > PCIDevice *pcidev = pci_vga_init(pci_bus); > dev = pcidev ? &pcidev->qdev : NULL; > @@ -1413,6 +1414,7 @@ DeviceState *pc_vga_init(ISABus *isa_bus, PCIBus *pci_bus) > ISADevice *isadev = isa_vga_init(isa_bus); > dev = isadev ? DEVICE(isadev) : NULL; > } > + rom_reset_order_override(); > return dev; > } > > @@ -1541,6 +1543,7 @@ void pc_nic_init(ISABus *isa_bus, PCIBus *pci_bus) > { > int i; > > + rom_set_order_override(FW_CFG_ORDER_OVERRIDE_NIC); > for (i = 0; i < nb_nics; i++) { > NICInfo *nd = &nd_table[i]; > > @@ -1550,6 +1553,7 @@ void pc_nic_init(ISABus *isa_bus, PCIBus *pci_bus) > pci_nic_init_nofail(nd, pci_bus, "e1000", NULL); > } > } > + rom_reset_order_override(); > } > > void pc_pci_device_init(PCIBus *pci_bus) > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c > index 6f8c2cd..447703b 100644 > --- a/hw/i386/pc_piix.c > +++ b/hw/i386/pc_piix.c > @@ -429,6 +429,7 @@ static void pc_i440fx_2_5_machine_options(MachineClass *m) > m->alias = NULL; > m->is_default = 0; > pcmc->save_tsc_khz = false; > + m->legacy_fw_cfg_order = 1; > SET_MACHINE_COMPAT(m, PC_COMPAT_2_5); > } > > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c > index 46522c9..04f3609 100644 > --- a/hw/i386/pc_q35.c > +++ b/hw/i386/pc_q35.c > @@ -291,6 +291,7 @@ static void pc_q35_2_5_machine_options(MachineClass *m) > pc_q35_2_6_machine_options(m); > m->alias = NULL; > pcmc->save_tsc_khz = false; > + m->legacy_fw_cfg_order = 1; > SET_MACHINE_COMPAT(m, PC_COMPAT_2_5); > } > > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c > index 7866248..a58efe4 100644 > --- a/hw/nvram/fw_cfg.c > +++ b/hw/nvram/fw_cfg.c > @@ -28,6 +28,7 @@ > #include "hw/isa/isa.h" > #include "hw/nvram/fw_cfg.h" > #include "hw/sysbus.h" > +#include "hw/boards.h" > #include "trace.h" > #include "qemu/error-report.h" > #include "qemu/config-file.h" > @@ -68,11 +69,14 @@ struct FWCfgState { > /*< public >*/ > > FWCfgEntry entries[2][FW_CFG_MAX_ENTRY]; > + int entry_order[FW_CFG_MAX_ENTRY]; > FWCfgFiles *files; > uint16_t cur_entry; > uint32_t cur_offset; > Notifier machine_ready; > > + int fw_cfg_order_override; > + > bool dma_enabled; > dma_addr_t dma_addr; > AddressSpace *dma_as; > @@ -664,12 +668,87 @@ void fw_cfg_add_i64(FWCfgState *s, uint16_t key, uint64_t value) > fw_cfg_add_bytes(s, key, copy, sizeof(value)); > } > > +void fw_cfg_set_order_override(FWCfgState *s, int order) > +{ > + assert(s->fw_cfg_order_override == 0); > + s->fw_cfg_order_override = order; > +} > + > +void fw_cfg_reset_order_override(FWCfgState *s) > +{ > + assert(s->fw_cfg_order_override != 0); > + s->fw_cfg_order_override = 0; > +} > + > +/* > + * This is the legacy order list. For legacy systems, files are in > + * the fw_cfg in the order defined below, by the "order" value. Note > + * that some entries (VGA ROMs, NIC option ROMS, etc.) go into a > + * specific area, but there may be more than one and they occur in the > + * order that the user specifies them on the command line. Those are > + * handled in a special manner, using the order override above. > + * > + * For non-legacy, the files are sorted by filename to avoid this kind > + * of complexity in the future. > + * > + * This is only for x86, other arches don't implement versioning so > + * they won't set legacy mode. > + */ > +static struct { > + const char *name; > + int order; > +} fw_cfg_order[] = { > + { "etc/boot-menu-wait", 10 }, > + { "bootsplash.jpg", 11 }, > + { "bootsplash.bmp", 12 }, > + { "etc/boot-fail-wait", 15 }, > + { "etc/smbios/smbios-tables", 20 }, > + { "etc/smbios/smbios-anchor", 30 }, > + { "etc/e820", 40 }, > + { "etc/reserved-memory-end", 50 }, > + { "genroms/kvmvapic.bin", 55 }, > + { "genroms/linuxboot.bin", 60 }, > + { }, /* VGA ROMs from pc_vga_init come here, 70. */ > + { }, /* NIC option ROMs from pc_nic_init come here, 80. */ > + { "etc/system-states", 90 }, > + { }, /* User ROMs come here, 100. */ > + { }, /* Device FW comes here, 110. */ > + { "etc/extra-pci-roots", 120 }, > + { "etc/acpi/tables", 130 }, > + { "etc/table-loader", 140 }, > + { "etc/tpm/log", 150 }, > + { "etc/acpi/rsdp", 160 }, > + { "bootorder", 170 }, > + > +#define FW_CFG_ORDER_OVERRIDE_LAST 200 > +}; > + > +static int get_fw_cfg_order(FWCfgState *s, const char *name) > +{ > + int i; > + > + if (s->fw_cfg_order_override > 0) > + return s->fw_cfg_order_override; > + > + for (i = 0; i < ARRAY_SIZE(fw_cfg_order); i++) { > + if (fw_cfg_order[i].name == NULL) > + continue; > + if (strcmp(name, fw_cfg_order[i].name) == 0) > + return fw_cfg_order[i].order; > + } > + /* Stick unknown stuff at the end. */ > + error_report("warning: Unknown firmware file in legacy mode: %s\n", name); > + return FW_CFG_ORDER_OVERRIDE_LAST; > +} > + > void fw_cfg_add_file_callback(FWCfgState *s, const char *filename, > FWCfgReadCallback callback, void *callback_opaque, > void *data, size_t len) > { > - int i, index; > + int i, index, count; > size_t dsize; > + MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine()); > + int order = 0; > > if (!s->files) { > dsize = sizeof(uint32_t) + sizeof(FWCfgFile) * FW_CFG_FILE_SLOTS; > @@ -677,13 +756,48 @@ void fw_cfg_add_file_callback(FWCfgState *s, const char *filename, > fw_cfg_add_bytes(s, FW_CFG_FILE_DIR, s->files, dsize); > } > > - index = be32_to_cpu(s->files->count); > - assert(index < FW_CFG_FILE_SLOTS); > + count = be32_to_cpu(s->files->count); > + assert(count < FW_CFG_FILE_SLOTS); > > - pstrcpy(s->files->f[index].name, sizeof(s->files->f[index].name), > - filename); > - for (i = 0; i < index; i++) { > - if (strcmp(s->files->f[index].name, s->files->f[i].name) == 0) { > + /* Find the insertion point. */ > + if (mc->legacy_fw_cfg_order) { > + /* > + * Sort by order. For files with the same order, we keep them > + * in the sequence in which they were added. > + */ > + order = get_fw_cfg_order(s, filename); > + for (index = count; > + index > 0 && order < s->entry_order[index - 1]; > + index--); > + } else { > + /* Sort by file name. */ > + for (index = count; > + index > 0 && strcmp(filename, s->files->f[index - 1].name) < 0; > + index--); > + } > + > + /* > + * Move all the entries from the index point and after down one > + * to create a slot for the new entry. Because calculations are > + * being done with the index, make it so that "i" is the current > + * index and "i - 1" is the one being copied from, thus the > + * unusual start and end in the for statement. > + */ > + for (i = count + 1; i > index; i--) { > + s->files->f[i] = s->files->f[i - 1]; > + s->files->f[i].select = cpu_to_be16(FW_CFG_FILE_FIRST + i); > + s->entries[0][FW_CFG_FILE_FIRST + i] = > + s->entries[0][FW_CFG_FILE_FIRST + i - 1]; > + s->entry_order[i] = s->entry_order[i - 1]; > + } > + > + memset(&s->files->f[index], 0, sizeof(FWCfgFile)); > + memset(&s->entries[0][FW_CFG_FILE_FIRST + index], 0, sizeof(FWCfgEntry)); > + > + pstrcpy(s->files->f[index].name, sizeof(s->files->f[index].name), filename); > + for (i = 0; i <= count; i++) { > + if (i != index && > + strcmp(s->files->f[index].name, s->files->f[i].name) == 0) { > error_report("duplicate fw_cfg file name: %s", > s->files->f[index].name); > exit(1); > @@ -695,9 +809,10 @@ void fw_cfg_add_file_callback(FWCfgState *s, const char *filename, > > s->files->f[index].size = cpu_to_be32(len); > s->files->f[index].select = cpu_to_be16(FW_CFG_FILE_FIRST + index); > + s->entry_order[index] = order; > trace_fw_cfg_add_file(s, index, s->files->f[index].name, len); > > - s->files->count = cpu_to_be32(index+1); > + s->files->count = cpu_to_be32(count+1); > } > > void fw_cfg_add_file(FWCfgState *s, const char *filename, > diff --git a/include/hw/boards.h b/include/hw/boards.h > index b5d7eae..b6567f7 100644 > --- a/include/hw/boards.h > +++ b/include/hw/boards.h > @@ -84,7 +84,8 @@ struct MachineClass { > no_cdrom:1, > no_sdcard:1, > has_dynamic_sysbus:1, > - pci_allow_0_address:1; > + pci_allow_0_address:1, > + legacy_fw_cfg_order:1; > int is_default; > const char *default_machine_opts; > const char *default_boot_order; > diff --git a/include/hw/loader.h b/include/hw/loader.h > index 0ba7808..e0fd5e8 100644 > --- a/include/hw/loader.h > +++ b/include/hw/loader.h > @@ -128,6 +128,8 @@ int rom_add_elf_program(const char *name, void *data, size_t datasize, > size_t romsize, hwaddr addr); > int rom_check_and_register_reset(void); > void rom_set_fw(FWCfgState *f); > +void rom_set_order_override(int order); > +void rom_reset_order_override(void); > int rom_copy(uint8_t *dest, hwaddr addr, size_t size); > void *rom_ptr(hwaddr addr); > void hmp_info_roms(Monitor *mon, const QDict *qdict); > diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h > index 4315f4e..d56c969 100644 > --- a/include/hw/nvram/fw_cfg.h > +++ b/include/hw/nvram/fw_cfg.h > @@ -57,6 +57,14 @@ typedef struct FWCfgFile { > char name[FW_CFG_MAX_FILE_PATH]; > } FWCfgFile; > > +#define FW_CFG_ORDER_OVERRIDE_VGA 70 > +#define FW_CFG_ORDER_OVERRIDE_NIC 80 > +#define FW_CFG_ORDER_OVERRIDE_USER 100 > +#define FW_CFG_ORDER_OVERRIDE_DEVICE 110 > + > +void fw_cfg_set_order_override(FWCfgState *fw_cfg, int order); > +void fw_cfg_reset_order_override(FWCfgState *fw_cfg); > + > typedef struct FWCfgFiles { > uint32_t count; > FWCfgFile f[]; > diff --git a/vl.c b/vl.c > index 7a28982..736d3e7 100644 > --- a/vl.c > +++ b/vl.c > @@ -2296,8 +2296,9 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp) > gchar *buf; > size_t size; > const char *name, *file, *str; > + FWCfgState *fw_cfg = (FWCfgState *) opaque; > > - if (opaque == NULL) { > + if (fw_cfg == NULL) { > error_report("fw_cfg device not available"); > return -1; > } > @@ -2331,7 +2332,10 @@ static int parse_fw_cfg(void *opaque, QemuOpts *opts, Error **errp) > return -1; > } > } > - fw_cfg_add_file((FWCfgState *)opaque, name, buf, size); > + /* For legacy, keep user files in a specific global order. */ > + fw_cfg_set_order_override(fw_cfg, FW_CFG_ORDER_OVERRIDE_USER); > + fw_cfg_add_file(fw_cfg, name, buf, size); > + fw_cfg_reset_order_override(fw_cfg); > return 0; > } > > @@ -4535,10 +4539,12 @@ int main(int argc, char **argv, char **envp) > igd_gfx_passthru(); > > /* init generic devices */ > + rom_set_order_override(FW_CFG_ORDER_OVERRIDE_DEVICE); > if (qemu_opts_foreach(qemu_find_opts("device"), > device_init_func, NULL, NULL)) { > exit(1); > } > + rom_reset_order_override(); > > /* Did we create any drives that we failed to create a device for? */ > drive_check_orphaned(); > -- > 2.5.0