On Thu, Mar 17, 2016 at 11:41:45AM -0500, Corey Minyard wrote:
> On 03/17/2016 11:02 AM, Michael S. Tsirkin wrote:
> >On Wed, Mar 16, 2016 at 01:47:08PM -0500, miny...@acm.org wrote:
> >>From: Gerd Hoffmann <kra...@redhat.com>
> >>
> >>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 <kra...@redhat.com>
> >>
> >>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 <cminy...@mvista.com>
> >>---
> >>
> >>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
I didn't realize this.
OK.
>From layering point of view, I think I would
like machines to call rom_set_order_override().
And then the fw cfg one should get fw cfg argument.
> >
> >> 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. */
Pls rephrase for readability.
> >>+ 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
> >>
