Re: [Qemu-devel] [PATCH v4] Sort the fw_cfg file list

[Michael S. Tsirkin]

On Mon, Mar 21, 2016 at 01:48:14PM -0500, address@hidden wrote:
> From: Gerd Hoffmann <address@hidden>
> 
> 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 <address@hidden>
> 
> 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 <address@hidden>


Reviewed-by: Michael S. Tsirkin <address@hidden>

Some minor comments below.

Who's merging this? Gerd or myself?

> ---
> 
> I'm going to assume my interpretation of Michael's commends is correct
> and send a new version...
> 
> Changes from v3:
> 
> Add some layering to the previous patch, add rom_set_order_override()
> that then calls the fw_config_set_order_override() functions.
> 
> Reword a rather hard to parse comment.
> 
>  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         | 130 
> +++++++++++++++++++++++++++++++++++++++++++---
>  include/hw/boards.h       |   3 +-
>  include/hw/loader.h       |   2 +
>  include/hw/nvram/fw_cfg.h |   7 +++
>  vl.c                      |   4 ++
>  9 files changed, 153 insertions(+), 9 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..1525078 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,86 @@ 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/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 +755,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 +808,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..8335fc0 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

Add an empty line here pls.

> +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..e8a06d8 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4520,10 +4520,12 @@ int main(int argc, char **argv, char **envp)
>  
>      numa_post_machine_init();
>  
> +    rom_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);
>      }
> +    rom_reset_order_override();
>  
>      /* init USB devices */
>      if (usb_enabled()) {


Can this be moved within parse_fw_cfg?
And then it can use the fw cfg APIs since it gets the
fw cfg pointer.

> @@ -4535,10 +4537,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