From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50998) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1agazf-0006HL-QY for qemu-devel@nongnu.org; Thu, 17 Mar 2016 12:41:53 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1agaze-0008AF-1a for qemu-devel@nongnu.org; Thu, 17 Mar 2016 12:41:51 -0400 Received: from mail-pf0-x231.google.com ([2607:f8b0:400e:c00::231]:32990) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1agazd-0008A8-Le for qemu-devel@nongnu.org; Thu, 17 Mar 2016 12:41:49 -0400 Received: by mail-pf0-x231.google.com with SMTP id 124so128061919pfg.0 for ; Thu, 17 Mar 2016 09:41:48 -0700 (PDT) References: <1458154028-4934-1-git-send-email-minyard@acm.org> <20160317180120-mutt-send-email-mst@redhat.com> From: Corey Minyard Message-ID: <56EADE49.8030803@mvista.com> Date: Thu, 17 Mar 2016 11:41:45 -0500 MIME-Version: 1.0 In-Reply-To: <20160317180120-mutt-send-email-mst@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3] Sort the fw_cfg file list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" , minyard@acm.org Cc: Paolo Bonzini , qemu-devel@nongnu.org, Gerd Hoffmann On 03/17/2016 11:02 AM, Michael S. Tsirkin wrote: > On Wed, Mar 16, 2016 at 01:47:08PM -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 >> --- >> >> A new version of the sorted fw_cfg, with new backwards >> compatibility code. This is bigger than I would like, but >> I can't think of a good way to split it up that would make >> much sense. >> >> This is more for review, anyway, I'm sure there are issues with >> this. >> >> I'm not too excited about fw_cfg_set_order_override(), but I can't >> think of a better way to handle this. You can't use filename or >> prefixes, the same filenames appear in different places for VGA >> ROMs depending on if they are PCI or ISA. > I don't see another good solution either. > However, I would like to make these NOPs with > new machine types. > > This way down the road we can drop this mess. They should be NOPs with new machine types. These only have an effect if legacy_fw_cfg_order is set in the machine class, and that's only set for older machines. -corey > >> hw/i386/pc.c | 4 ++ >> hw/i386/pc_piix.c | 1 + >> hw/i386/pc_q35.c | 1 + >> hw/nvram/fw_cfg.c | 127 +++++++++++++++++++++++++++++++++++++++++++--- >> include/hw/boards.h | 3 +- >> include/hw/nvram/fw_cfg.h | 7 +++ >> vl.c | 4 ++ >> 7 files changed, 138 insertions(+), 9 deletions(-) >> >> diff --git a/hw/i386/pc.c b/hw/i386/pc.c >> index 56ec6cd..707753b 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; >> >> + fw_cfg_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; >> } >> + fw_cfg_reset_order_override(); >> return dev; >> } >> >> @@ -1541,6 +1543,7 @@ void pc_nic_init(ISABus *isa_bus, PCIBus *pci_bus) >> { >> int i; >> >> + fw_cfg_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); >> } >> } >> + fw_cfg_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..1693f26 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,6 +69,7 @@ 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; >> @@ -664,12 +666,88 @@ void fw_cfg_add_i64(FWCfgState *s, uint16_t key, uint64_t value) >> fw_cfg_add_bytes(s, key, copy, sizeof(value)); >> } >> >> +static int fw_cfg_order_override = -1; >> + >> +void fw_cfg_set_order_override(int order) >> +{ >> + assert(fw_cfg_order_override == -1); >> + fw_cfg_order_override = order; >> +} >> + >> +void fw_cfg_reset_order_override(void) >> +{ >> + assert(fw_cfg_order_override != -1); >> + fw_cfg_order_override = -1; >> +} >> + >> +/* >> + * 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/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(const char *name) >> +{ >> + int i; >> + >> + if (fw_cfg_order_override >= 0) >> + return 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 +755,45 @@ 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, and keep insertion order for the same order. */ >> + order = get_fw_cfg_order(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 +805,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/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h >> index 4315f4e..38bbfca 100644 >> --- a/include/hw/nvram/fw_cfg.h >> +++ b/include/hw/nvram/fw_cfg.h >> @@ -57,6 +57,13 @@ 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(int order); >> +void fw_cfg_reset_order_override(void); >> + >> typedef struct FWCfgFiles { >> uint32_t count; >> FWCfgFile f[]; >> diff --git a/vl.c b/vl.c >> index 7a28982..6fad844 100644 >> --- a/vl.c >> +++ b/vl.c >> @@ -4520,10 +4520,12 @@ int main(int argc, char **argv, char **envp) >> >> numa_post_machine_init(); >> >> + fw_cfg_set_order_override(FW_CFG_ORDER_OVERRIDE_USER); >> if (qemu_opts_foreach(qemu_find_opts("fw_cfg"), >> parse_fw_cfg, fw_cfg_find(), NULL) != 0) { >> exit(1); >> } >> + fw_cfg_reset_order_override(); >> >> /* init USB devices */ >> if (usb_enabled()) { >> @@ -4535,10 +4537,12 @@ int main(int argc, char **argv, char **envp) >> igd_gfx_passthru(); >> >> /* init generic devices */ >> + fw_cfg_set_order_override(FW_CFG_ORDER_OVERRIDE_DEVICE); >> if (qemu_opts_foreach(qemu_find_opts("device"), >> device_init_func, NULL, NULL)) { >> exit(1); >> } >> + fw_cfg_reset_order_override(); >> >> /* Did we create any drives that we failed to create a device for? */ >> drive_check_orphaned(); >> -- >> 2.5.0 >>