[v4,2/7] fw-cfg: turn FW_CFG_FILE_SLOTS into a device property

Commit Message

Laszlo Ersek Dec. 1, 2016, 5:06 p.m. UTC

We'd like to raise the value of FW_CFG_FILE_SLOTS. Doing it naively could
lead to problems with backward migration: a more recent QEMU (running an
older machine type) would allow the guest, in fw_cfg_select(), to select a
high key value that is unavailable in the same machine type implemented by
the older (target) QEMU. On the target host, fw_cfg_data_read() for
example could dereference nonexistent entries.

As first step, size the FWCfgState.entries[*] and FWCfgState.entry_order
arrays dynamically. All three array sizes will be influenced by the new
field (and device property) FWCfgState.file_slots.

Make the following changes:

- Replace the FW_CFG_FILE_SLOTS macro with FW_CFG_FILE_SLOTS_TRAD
  (traditional count of fw_cfg file slots) in the header file. The value
  remains 0x10.

- Replace all uses of FW_CFG_FILE_SLOTS with a helper function called
  fw_cfg_file_slots(), returning the new property.

- Eliminate the macro FW_CFG_MAX_ENTRY, and replace all its uses with a
  helper function called fw_cfg_max_entry().

- In the MMIO- and IO-mapped realize functions both, allocate all three
  arrays dynamically, based on the new property.

- The new property defaults to 0x20; however at the moment we forcibly set
  it to FW_CFG_FILE_SLOTS_TRAD on all code paths available to board code
  (namely in the fw_cfg_init_io_dma() and fw_cfg_init_mem_wide() helper
  functions). This is going to be customized in the following patches.

Cc: "Gabriel L. Somlo" <[e-mail]>
Cc: "Michael S. Tsirkin" <[e-mail]>
Cc: Gerd Hoffmann <[e-mail]>
Cc: Igor Mammedov <[e-mail]>
Cc: Paolo Bonzini <[e-mail]>
Signed-off-by: Laszlo Ersek <[e-mail]>
---

Notes:
    I know that upstream doesn't care about backward migration, but some
    downstreams might.

 docs/specs/fw_cfg.txt          |  2 +-
 include/hw/nvram/fw_cfg_keys.h |  3 +-
 hw/nvram/fw_cfg.c              | 85 ++++++++++++++++++++++++++++++++++++++----
 3 files changed, 79 insertions(+), 11 deletions(-)

Comments

Gerd Hoffmann Dec. 2, 2016, 11:10 a.m. UTC | #1

On Do, 2016-12-01 at 18:06 +0100, Laszlo Ersek wrote:
> We'd like to raise the value of FW_CFG_FILE_SLOTS. Doing it naively could
> lead to problems with backward migration: a more recent QEMU (running an
> older machine type) would allow the guest, in fw_cfg_select(), to select a
> high key value that is unavailable in the same machine type implemented by
> the older (target) QEMU.

I don't think we need this.  fw_cfg changes are guest-visible so they
must not happen for a given machine type.  So if current machine types
don't hit the limit that should continue to be the case even if we
simply raise FW_CFG_FILE_SLOTS.

But we have to take care that new files show up on new machine types
only.

cheers,
  Gerd

Laszlo Ersek Dec. 2, 2016, 11:48 a.m. UTC | #2

On 12/02/16 12:10, Gerd Hoffmann wrote:
> On Do, 2016-12-01 at 18:06 +0100, Laszlo Ersek wrote:
>> We'd like to raise the value of FW_CFG_FILE_SLOTS. Doing it naively could
>> lead to problems with backward migration: a more recent QEMU (running an
>> older machine type) would allow the guest, in fw_cfg_select(), to select a
>> high key value that is unavailable in the same machine type implemented by
>> the older (target) QEMU.
> 
> I don't think we need this.  fw_cfg changes are guest-visible so they
> must not happen for a given machine type.

Agreed.

> So if current machine types
> don't hit the limit

Please check one of the links in the blurb, under which Paolo noted that
we're already above the limit in the worst (theoretical) case.

In practice they don't hit the limit, indeed.

> that should continue to be the case even if we
> simply raise FW_CFG_FILE_SLOTS.

I'm not trying to make it hard for myself :), so in theory I don't
disagree with the idea. However, do consider backwards migration. (As
noted under the patch, I'm aware that upstream doesn't care, but that
shouldn't be reason enough to reject a patch that does care.)

Let's say we start a guest in the pc-q35-2.8 machtype on a new QEMU
release, as source QEMU host, which has the raised FW_CFG_FILE_SLOTS
value. The guest writes a high key value to the selector register, which
is valid under the raised limit, so the key value is stored (i.e.,
fw_cfg_select() won't store FW_CFG_INVALID in cur_entry).

Then the guest is migrated to the older release QEMU, where the value of
cur_entry is larger than or equal to FW_CFG_MAX_ENTRY. The guest then
reads the data register, and fw_cfg_data_read() does this:

    FWCfgEntry *e = (s->cur_entry == FW_CFG_INVALID) ? NULL :
                    &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK];
    uint64_t value = 0;

    assert(size > 0 && size <= sizeof(value));
    if (s->cur_entry != FW_CFG_INVALID && e->data && s->cur_offset <
e->len) {

Just computing the pointer "e" is undefined behavior, let alone
evaluating "e->data".

Once again, I know upstream doesn't care about backward migration, but
we (at Red Hat) do, for specific machine types at least. I would rather
carry this patch in upstream than in downstream only.

As far as I understand, this is the first time in QEMU history that
we're looking into raising FW_CFG_FILE_SLOTS, so I'd rather be careful.
(I definitely don't want to win the privilege to implement the patch in
downstream!)

> But we have to take care that new files show up on new machine types
> only.

The series covers that, yes -- if the SMI host features bitmap that is
returned by the new machtype-specific callback function at board
initialization is zero (or the callback doesn't exist), then the fw_cfg
files are not created. See how ich9_lpc_pm_init() is called, and how it
handles the new "smi_host_features" parameter.

Thanks
Laszlo

Gerd Hoffmann Dec. 2, 2016, 12:47 p.m. UTC | #3

Hi,

> Please check one of the links in the blurb, under which Paolo noted that
> we're already above the limit in the worst (theoretical) case.

Oh, ok.  That changes the picture.

> In practice they don't hit the limit, indeed.

But creating such a case being possible (even if unlikely) is reason
enough to care, from a security point of view.

cheers,
  Gerd

Igor Mammedov Dec. 6, 2016, 10:50 a.m. UTC | #4

On Thu,  1 Dec 2016 18:06:19 +0100
Laszlo Ersek <[e-mail]> wrote:

> We'd like to raise the value of FW_CFG_FILE_SLOTS. Doing it naively could
> lead to problems with backward migration: a more recent QEMU (running an
> older machine type) would allow the guest, in fw_cfg_select(), to select a
> high key value that is unavailable in the same machine type implemented by
> the older (target) QEMU. On the target host, fw_cfg_data_read() for
> example could dereference nonexistent entries.
> 
> As first step, size the FWCfgState.entries[*] and FWCfgState.entry_order
> arrays dynamically. All three array sizes will be influenced by the new
> field (and device property) FWCfgState.file_slots.
> 
> Make the following changes:
> 
> - Replace the FW_CFG_FILE_SLOTS macro with FW_CFG_FILE_SLOTS_TRAD
>   (traditional count of fw_cfg file slots) in the header file. The value
>   remains 0x10.
> 
> - Replace all uses of FW_CFG_FILE_SLOTS with a helper function called
>   fw_cfg_file_slots(), returning the new property.
> 
> - Eliminate the macro FW_CFG_MAX_ENTRY, and replace all its uses with a
>   helper function called fw_cfg_max_entry().
> 
> - In the MMIO- and IO-mapped realize functions both, allocate all three
>   arrays dynamically, based on the new property.
> 
> - The new property defaults to 0x20; however at the moment we forcibly set
>   it to FW_CFG_FILE_SLOTS_TRAD on all code paths available to board code
>   (namely in the fw_cfg_init_io_dma() and fw_cfg_init_mem_wide() helper
>   functions). This is going to be customized in the following patches.
> 
> Cc: "Gabriel L. Somlo" <[e-mail]>
> Cc: "Michael S. Tsirkin" <[e-mail]>
> Cc: Gerd Hoffmann <[e-mail]>
> Cc: Igor Mammedov <[e-mail]>
> Cc: Paolo Bonzini <[e-mail]>
> Signed-off-by: Laszlo Ersek <[e-mail]>
> ---
> 
> Notes:
>     I know that upstream doesn't care about backward migration, but some
>     downstreams might.
> 
>  docs/specs/fw_cfg.txt          |  2 +-
>  include/hw/nvram/fw_cfg_keys.h |  3 +-
>  hw/nvram/fw_cfg.c              | 85 ++++++++++++++++++++++++++++++++++++++----
>  3 files changed, 79 insertions(+), 11 deletions(-)
> 
> diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
> index a19e2adbe1c6..84e2978706f5 100644
> --- a/docs/specs/fw_cfg.txt
> +++ b/docs/specs/fw_cfg.txt
> @@ -154,11 +154,11 @@ Selector Reg.    Range Usage
>  0x8000 - 0xbfff  Arch. Specific (0x0000 - 0x3fff, generally RO, possibly RW
>                                   through the DMA interface in QEMU v2.9+)
>  0xc000 - 0xffff  Arch. Specific (0x0000 - 0x3fff, RW, ignored in v2.4+)
>  
>  In practice, the number of allowed firmware configuration items is given
> -by the value of FW_CFG_MAX_ENTRY (see fw_cfg.h).
> +by the value (FW_CFG_FILE_FIRST + FW_CFG_FILE_SLOTS_TRAD) (see fw_cfg.h).
>  
>  = Guest-side DMA Interface =
>  
>  If bit 1 of the feature bitmap is set, the DMA interface is present. This does
>  not replace the existing fw_cfg interface, it is an add-on. This interface
> diff --git a/include/hw/nvram/fw_cfg_keys.h b/include/hw/nvram/fw_cfg_keys.h
> index 0f3e871884c0..627589793671 100644
> --- a/include/hw/nvram/fw_cfg_keys.h
> +++ b/include/hw/nvram/fw_cfg_keys.h
> @@ -27,12 +27,11 @@
>  #define FW_CFG_SETUP_SIZE       0x17
>  #define FW_CFG_SETUP_DATA       0x18
>  #define FW_CFG_FILE_DIR         0x19
>  
>  #define FW_CFG_FILE_FIRST       0x20
> -#define FW_CFG_FILE_SLOTS       0x10
> -#define FW_CFG_MAX_ENTRY        (FW_CFG_FILE_FIRST + FW_CFG_FILE_SLOTS)
> +#define FW_CFG_FILE_SLOTS_TRAD  0x10
>  
>  #define FW_CFG_WRITE_CHANNEL    0x4000
>  #define FW_CFG_ARCH_LOCAL       0x8000
>  #define FW_CFG_ENTRY_MASK       (~(FW_CFG_WRITE_CHANNEL | FW_CFG_ARCH_LOCAL))
>  
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index e0145c11a19b..2e1441c09750 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -31,10 +31,13 @@
>  #include "hw/sysbus.h"
>  #include "trace.h"
>  #include "qemu/error-report.h"
>  #include "qemu/config-file.h"
>  #include "qemu/cutils.h"
> +#include "qapi/error.h"
> +
> +#define FW_CFG_FILE_SLOTS_DFLT 0x20
>  
>  #define FW_CFG_NAME "fw_cfg"
>  #define FW_CFG_PATH "/machine/" FW_CFG_NAME
>  
>  #define TYPE_FW_CFG     "fw_cfg"
> @@ -69,12 +72,13 @@ typedef struct FWCfgEntry {
>  struct FWCfgState {
>      /*< private >*/
>      SysBusDevice parent_obj;
>      /*< public >*/
>  
> -    FWCfgEntry entries[2][FW_CFG_MAX_ENTRY];
> -    int entry_order[FW_CFG_MAX_ENTRY];
> +    uint32_t file_slots;
should it be uint16_t?
As below you use "uint16_t file_slots_max;" and do some UINT16
to calculate max limit.

> +    FWCfgEntry *entries[2];
> +    int *entry_order;
>      FWCfgFiles *files;
>      uint16_t cur_entry;
>      uint32_t cur_offset;
>      Notifier machine_ready;
>  
> @@ -255,17 +259,27 @@ static void fw_cfg_reboot(FWCfgState *s)
>  static void fw_cfg_write(FWCfgState *s, uint8_t value)
>  {
>      /* nothing, write support removed in QEMU v2.4+ */
>  }
>  
> +static inline uint32_t fw_cfg_file_slots(const FWCfgState *s)
> +{
> +    return s->file_slots;
> +}
so far it doesn't look like this wrapper function is necessary,
I'd prefer accessing field directly as it's used only internally.
Or do you have plans to extend wrapper later?
(then it would be better introduce wrapper at that time)

> +
> +static inline uint32_t fw_cfg_max_entry(const FWCfgState *s)
> +{
> +    return FW_CFG_FILE_FIRST + fw_cfg_file_slots(s);
> +}
> +
>  static int fw_cfg_select(FWCfgState *s, uint16_t key)
>  {
>      int arch, ret;
>      FWCfgEntry *e;
>  
>      s->cur_offset = 0;
> -    if ((key & FW_CFG_ENTRY_MASK) >= FW_CFG_MAX_ENTRY) {
> +    if ((key & FW_CFG_ENTRY_MASK) >= fw_cfg_max_entry(s)) {
>          s->cur_entry = FW_CFG_INVALID;
>          ret = 0;
>      } else {
>          s->cur_entry = key;
>          ret = 1;
> @@ -608,11 +622,11 @@ static void fw_cfg_add_bytes_read_callback(FWCfgState *s, uint16_t key,
>  {
>      int arch = !!(key & FW_CFG_ARCH_LOCAL);
>  
>      key &= FW_CFG_ENTRY_MASK;
>  
> -    assert(key < FW_CFG_MAX_ENTRY && len < UINT32_MAX);
> +    assert(key < fw_cfg_max_entry(s) && len < UINT32_MAX);
>      assert(s->entries[arch][key].data == NULL); /* avoid key conflict */
>  
>      s->entries[arch][key].data = data;
>      s->entries[arch][key].len = (uint32_t)len;
>      s->entries[arch][key].read_callback = callback;
> @@ -626,11 +640,11 @@ static void *fw_cfg_modify_bytes_read(FWCfgState *s, uint16_t key,
>      void *ptr;
>      int arch = !!(key & FW_CFG_ARCH_LOCAL);
>  
>      key &= FW_CFG_ENTRY_MASK;
>  
> -    assert(key < FW_CFG_MAX_ENTRY && len < UINT32_MAX);
> +    assert(key < fw_cfg_max_entry(s) && len < UINT32_MAX);
>  
>      /* return the old data to the function caller, avoid memory leak */
>      ptr = s->entries[arch][key].data;
>      s->entries[arch][key].data = data;
>      s->entries[arch][key].len = len;
> @@ -775,17 +789,17 @@ void fw_cfg_add_file_callback(FWCfgState *s,  const char *filename,
>      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;
> +        dsize = sizeof(uint32_t) + sizeof(FWCfgFile) * fw_cfg_file_slots(s);
>          s->files = g_malloc0(dsize);
>          fw_cfg_add_bytes(s, FW_CFG_FILE_DIR, s->files, dsize);
>      }
>  
>      count = be32_to_cpu(s->files->count);
> -    assert(count < FW_CFG_FILE_SLOTS);
> +    assert(count < fw_cfg_file_slots(s));
>  
>      /* Find the insertion point. */
>      if (mc->legacy_fw_cfg_order) {
>          /*
>           * Sort by order. For files with the same order, we keep them
> @@ -855,11 +869,11 @@ void *fw_cfg_modify_file(FWCfgState *s, const char *filename,
>      void *ptr = NULL;
>  
>      assert(s->files);
>  
>      index = be32_to_cpu(s->files->count);
> -    assert(index < FW_CFG_FILE_SLOTS);
> +    assert(index < fw_cfg_file_slots(s));
>  
>      for (i = 0; i < index; i++) {
>          if (strcmp(filename, s->files->f[i].name) == 0) {
>              ptr = fw_cfg_modify_bytes_read(s, FW_CFG_FILE_FIRST + i,
>                                             data, len);
> @@ -926,10 +940,16 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
>      qdev_prop_set_uint32(dev, "dma_iobase", dma_iobase);
>      if (!dma_requested) {
>          qdev_prop_set_bit(dev, "dma_enabled", false);
>      }
>  
> +    /* Once we expose the "file_slots" property to callers of
> +     * fw_cfg_init_io_dma(), the following setting should become conditional on
> +     * the input parameter being lower than the current value of the property.
> +     */
> +    qdev_prop_set_uint32(dev, "file_slots", FW_CFG_FILE_SLOTS_TRAD);
> +
>      fw_cfg_init1(dev);
>      s = FW_CFG(dev);
>  
>      if (s->dma_enabled) {
>          /* 64 bits for the address field */
> @@ -963,10 +983,17 @@ FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr,
>      qdev_prop_set_uint32(dev, "data_width", data_width);
>      if (!dma_requested) {
>          qdev_prop_set_bit(dev, "dma_enabled", false);
>      }
>  
> +    /* Once we expose the "file_slots" property to callers of
> +     * fw_cfg_init_mem_wide(), the following setting should become conditional
> +     * on the input parameter being lower than the current value of the
> +     * property. Refer to fw_cfg_init_io_dma().
> +     */
> +    qdev_prop_set_uint32(dev, "file_slots", FW_CFG_FILE_SLOTS_TRAD);
both above cases of qdev_prop_set_uint32() could be replaced by
compat property for fwcfg device which would clamp "file_slots"
to old value for old machine types in a cleaner way.

> +
>      fw_cfg_init1(dev);
>  
>      sbd = SYS_BUS_DEVICE(dev);
>      sysbus_mmio_map(sbd, 0, ctl_addr);
>      sysbus_mmio_map(sbd, 1, data_addr);
> @@ -1012,23 +1039,56 @@ static const TypeInfo fw_cfg_info = {
>      .abstract      = true,
>      .instance_size = sizeof(FWCfgState),
>      .class_init    = fw_cfg_class_init,
>  };
>  
> +static void fw_cfg_file_slots_allocate(FWCfgState *s, Error **errp)
> +{
> +    uint16_t file_slots_max;
> +
> +    if (fw_cfg_file_slots(s) < FW_CFG_FILE_SLOTS_TRAD) {
> +        error_setg(errp, "\"file_slots\" must be at least 0x%x",
> +                   FW_CFG_FILE_SLOTS_TRAD);
> +        return;
> +    }
> +
> +    /* (UINT16_MAX & FW_CFG_ENTRY_MASK) is the highest inclusive selector value
> +     * that we permit. The actual (exclusive) value coming from the
> +     * configuration is (FW_CFG_FILE_FIRST + fw_cfg_file_slots(s)). */
> +    file_slots_max = (UINT16_MAX & FW_CFG_ENTRY_MASK) - FW_CFG_FILE_FIRST + 1;
> +    if (fw_cfg_file_slots(s) > file_slots_max) {
> +        error_setg(errp, "\"file_slots\" must not exceed 0x%" PRIx16,
> +                   file_slots_max);
> +        return;
> +    }
> +
> +    s->entries[0] = g_new0(FWCfgEntry, fw_cfg_max_entry(s));
> +    s->entries[1] = g_new0(FWCfgEntry, fw_cfg_max_entry(s));
> +    s->entry_order = g_new0(int, fw_cfg_max_entry(s));
> +}
>  
>  static Property fw_cfg_io_properties[] = {
>      DEFINE_PROP_UINT32("iobase", FWCfgIoState, iobase, -1),
>      DEFINE_PROP_UINT32("dma_iobase", FWCfgIoState, dma_iobase, -1),
>      DEFINE_PROP_BOOL("dma_enabled", FWCfgIoState, parent_obj.dma_enabled,
>                       true),
> +    DEFINE_PROP_UINT32("file_slots", FWCfgIoState, parent_obj.file_slots,
> +                       FW_CFG_FILE_SLOTS_DFLT),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
>  static void fw_cfg_io_realize(DeviceState *dev, Error **errp)
>  {
>      FWCfgIoState *s = FW_CFG_IO(dev);
>      SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> +    Error *local_err = NULL;
> +
> +    fw_cfg_file_slots_allocate(FW_CFG(s), &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  
>      /* when using port i/o, the 8-bit data register ALWAYS overlaps
>       * with half of the 16-bit control register. Hence, the total size
>       * of the i/o region used is FW_CFG_CTL_SIZE */
>      memory_region_init_io(&s->comb_iomem, OBJECT(s), &fw_cfg_comb_mem_ops,
> @@ -1061,18 +1121,27 @@ static const TypeInfo fw_cfg_io_info = {
>  
>  static Property fw_cfg_mem_properties[] = {
>      DEFINE_PROP_UINT32("data_width", FWCfgMemState, data_width, -1),
>      DEFINE_PROP_BOOL("dma_enabled", FWCfgMemState, parent_obj.dma_enabled,
>                       true),
> +    DEFINE_PROP_UINT32("file_slots", FWCfgMemState, parent_obj.file_slots,
> +                       FW_CFG_FILE_SLOTS_DFLT),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
>  static void fw_cfg_mem_realize(DeviceState *dev, Error **errp)
>  {
>      FWCfgMemState *s = FW_CFG_MEM(dev);
>      SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>      const MemoryRegionOps *data_ops = &fw_cfg_data_mem_ops;
> +    Error *local_err = NULL;
> +
> +    fw_cfg_file_slots_allocate(FW_CFG(s), &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  
>      memory_region_init_io(&s->ctl_iomem, OBJECT(s), &fw_cfg_ctl_mem_ops,
>                            FW_CFG(s), "fwcfg.ctl", FW_CFG_CTL_SIZE);
>      sysbus_init_mmio(sbd, &s->ctl_iomem);
>

Laszlo Ersek Dec. 6, 2016, 11:43 a.m. UTC | #5

On 12/06/16 11:50, Igor Mammedov wrote:
> On Thu,  1 Dec 2016 18:06:19 +0100
> Laszlo Ersek <[e-mail]> wrote:
> 
>> We'd like to raise the value of FW_CFG_FILE_SLOTS. Doing it naively could
>> lead to problems with backward migration: a more recent QEMU (running an
>> older machine type) would allow the guest, in fw_cfg_select(), to select a
>> high key value that is unavailable in the same machine type implemented by
>> the older (target) QEMU. On the target host, fw_cfg_data_read() for
>> example could dereference nonexistent entries.
>>
>> As first step, size the FWCfgState.entries[*] and FWCfgState.entry_order
>> arrays dynamically. All three array sizes will be influenced by the new
>> field (and device property) FWCfgState.file_slots.
>>
>> Make the following changes:
>>
>> - Replace the FW_CFG_FILE_SLOTS macro with FW_CFG_FILE_SLOTS_TRAD
>>   (traditional count of fw_cfg file slots) in the header file. The value
>>   remains 0x10.
>>
>> - Replace all uses of FW_CFG_FILE_SLOTS with a helper function called
>>   fw_cfg_file_slots(), returning the new property.
>>
>> - Eliminate the macro FW_CFG_MAX_ENTRY, and replace all its uses with a
>>   helper function called fw_cfg_max_entry().
>>
>> - In the MMIO- and IO-mapped realize functions both, allocate all three
>>   arrays dynamically, based on the new property.
>>
>> - The new property defaults to 0x20; however at the moment we forcibly set
>>   it to FW_CFG_FILE_SLOTS_TRAD on all code paths available to board code
>>   (namely in the fw_cfg_init_io_dma() and fw_cfg_init_mem_wide() helper
>>   functions). This is going to be customized in the following patches.
>>
>> Cc: "Gabriel L. Somlo" <[e-mail]>
>> Cc: "Michael S. Tsirkin" <[e-mail]>
>> Cc: Gerd Hoffmann <[e-mail]>
>> Cc: Igor Mammedov <[e-mail]>
>> Cc: Paolo Bonzini <[e-mail]>
>> Signed-off-by: Laszlo Ersek <[e-mail]>
>> ---
>>
>> Notes:
>>     I know that upstream doesn't care about backward migration, but some
>>     downstreams might.
>>
>>  docs/specs/fw_cfg.txt          |  2 +-
>>  include/hw/nvram/fw_cfg_keys.h |  3 +-
>>  hw/nvram/fw_cfg.c              | 85 ++++++++++++++++++++++++++++++++++++++----
>>  3 files changed, 79 insertions(+), 11 deletions(-)
>>
>> diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
>> index a19e2adbe1c6..84e2978706f5 100644
>> --- a/docs/specs/fw_cfg.txt
>> +++ b/docs/specs/fw_cfg.txt
>> @@ -154,11 +154,11 @@ Selector Reg.    Range Usage
>>  0x8000 - 0xbfff  Arch. Specific (0x0000 - 0x3fff, generally RO, possibly RW
>>                                   through the DMA interface in QEMU v2.9+)
>>  0xc000 - 0xffff  Arch. Specific (0x0000 - 0x3fff, RW, ignored in v2.4+)
>>  
>>  In practice, the number of allowed firmware configuration items is given
>> -by the value of FW_CFG_MAX_ENTRY (see fw_cfg.h).
>> +by the value (FW_CFG_FILE_FIRST + FW_CFG_FILE_SLOTS_TRAD) (see fw_cfg.h).
>>  
>>  = Guest-side DMA Interface =
>>  
>>  If bit 1 of the feature bitmap is set, the DMA interface is present. This does
>>  not replace the existing fw_cfg interface, it is an add-on. This interface
>> diff --git a/include/hw/nvram/fw_cfg_keys.h b/include/hw/nvram/fw_cfg_keys.h
>> index 0f3e871884c0..627589793671 100644
>> --- a/include/hw/nvram/fw_cfg_keys.h
>> +++ b/include/hw/nvram/fw_cfg_keys.h
>> @@ -27,12 +27,11 @@
>>  #define FW_CFG_SETUP_SIZE       0x17
>>  #define FW_CFG_SETUP_DATA       0x18
>>  #define FW_CFG_FILE_DIR         0x19
>>  
>>  #define FW_CFG_FILE_FIRST       0x20
>> -#define FW_CFG_FILE_SLOTS       0x10
>> -#define FW_CFG_MAX_ENTRY        (FW_CFG_FILE_FIRST + FW_CFG_FILE_SLOTS)
>> +#define FW_CFG_FILE_SLOTS_TRAD  0x10
>>  
>>  #define FW_CFG_WRITE_CHANNEL    0x4000
>>  #define FW_CFG_ARCH_LOCAL       0x8000
>>  #define FW_CFG_ENTRY_MASK       (~(FW_CFG_WRITE_CHANNEL | FW_CFG_ARCH_LOCAL))
>>  
>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
>> index e0145c11a19b..2e1441c09750 100644
>> --- a/hw/nvram/fw_cfg.c
>> +++ b/hw/nvram/fw_cfg.c
>> @@ -31,10 +31,13 @@
>>  #include "hw/sysbus.h"
>>  #include "trace.h"
>>  #include "qemu/error-report.h"
>>  #include "qemu/config-file.h"
>>  #include "qemu/cutils.h"
>> +#include "qapi/error.h"
>> +
>> +#define FW_CFG_FILE_SLOTS_DFLT 0x20
>>  
>>  #define FW_CFG_NAME "fw_cfg"
>>  #define FW_CFG_PATH "/machine/" FW_CFG_NAME
>>  
>>  #define TYPE_FW_CFG     "fw_cfg"
>> @@ -69,12 +72,13 @@ typedef struct FWCfgEntry {
>>  struct FWCfgState {
>>      /*< private >*/
>>      SysBusDevice parent_obj;
>>      /*< public >*/
>>  
>> -    FWCfgEntry entries[2][FW_CFG_MAX_ENTRY];
>> -    int entry_order[FW_CFG_MAX_ENTRY];
>> +    uint32_t file_slots;
> should it be uint16_t?
> As below you use "uint16_t file_slots_max;" and do some UINT16
> to calculate max limit.

I think I had a reason for making this uint32_t. I think the argument
was that fw_cfg_max_entry() could theoretically return a value that
doesn't fit in a uint16_t. Looking again at the patch however, I think I
can try to make this a uint16_t for the next version.

> 
>> +    FWCfgEntry *entries[2];
>> +    int *entry_order;
>>      FWCfgFiles *files;
>>      uint16_t cur_entry;
>>      uint32_t cur_offset;
>>      Notifier machine_ready;
>>  
>> @@ -255,17 +259,27 @@ static void fw_cfg_reboot(FWCfgState *s)
>>  static void fw_cfg_write(FWCfgState *s, uint8_t value)
>>  {
>>      /* nothing, write support removed in QEMU v2.4+ */
>>  }
>>  
>> +static inline uint32_t fw_cfg_file_slots(const FWCfgState *s)
>> +{
>> +    return s->file_slots;
>> +}
> so far it doesn't look like this wrapper function is necessary,
> I'd prefer accessing field directly as it's used only internally.
> Or do you have plans to extend wrapper later?
> (then it would be better introduce wrapper at that time)

Originally I used "s->file_slots" in this patch, like you say, without
this small wrapper function,, but the resultant patch was harder to
review. With this wrapper, you have two kinds of changes in the patch:

*     FW_CFG_MAX_ENTRY
  --> fw_cfg_max_entry(s)

*     FW_CFG_FILE_SLOTS
  --> fw_cfg_file_slots(s)

Without the wrapper, the second bullet looks

*     FW_CFG_FILE_SLOTS
  --> s->file_slots

and to me that made the patch harder to verify.

> 
>> +
>> +static inline uint32_t fw_cfg_max_entry(const FWCfgState *s)
>> +{
>> +    return FW_CFG_FILE_FIRST + fw_cfg_file_slots(s);
>> +}
>> +
>>  static int fw_cfg_select(FWCfgState *s, uint16_t key)
>>  {
>>      int arch, ret;
>>      FWCfgEntry *e;
>>  
>>      s->cur_offset = 0;
>> -    if ((key & FW_CFG_ENTRY_MASK) >= FW_CFG_MAX_ENTRY) {
>> +    if ((key & FW_CFG_ENTRY_MASK) >= fw_cfg_max_entry(s)) {
>>          s->cur_entry = FW_CFG_INVALID;
>>          ret = 0;
>>      } else {
>>          s->cur_entry = key;
>>          ret = 1;
>> @@ -608,11 +622,11 @@ static void fw_cfg_add_bytes_read_callback(FWCfgState *s, uint16_t key,
>>  {
>>      int arch = !!(key & FW_CFG_ARCH_LOCAL);
>>  
>>      key &= FW_CFG_ENTRY_MASK;
>>  
>> -    assert(key < FW_CFG_MAX_ENTRY && len < UINT32_MAX);
>> +    assert(key < fw_cfg_max_entry(s) && len < UINT32_MAX);
>>      assert(s->entries[arch][key].data == NULL); /* avoid key conflict */
>>  
>>      s->entries[arch][key].data = data;
>>      s->entries[arch][key].len = (uint32_t)len;
>>      s->entries[arch][key].read_callback = callback;
>> @@ -626,11 +640,11 @@ static void *fw_cfg_modify_bytes_read(FWCfgState *s, uint16_t key,
>>      void *ptr;
>>      int arch = !!(key & FW_CFG_ARCH_LOCAL);
>>  
>>      key &= FW_CFG_ENTRY_MASK;
>>  
>> -    assert(key < FW_CFG_MAX_ENTRY && len < UINT32_MAX);
>> +    assert(key < fw_cfg_max_entry(s) && len < UINT32_MAX);
>>  
>>      /* return the old data to the function caller, avoid memory leak */
>>      ptr = s->entries[arch][key].data;
>>      s->entries[arch][key].data = data;
>>      s->entries[arch][key].len = len;
>> @@ -775,17 +789,17 @@ void fw_cfg_add_file_callback(FWCfgState *s,  const char *filename,
>>      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;
>> +        dsize = sizeof(uint32_t) + sizeof(FWCfgFile) * fw_cfg_file_slots(s);
>>          s->files = g_malloc0(dsize);
>>          fw_cfg_add_bytes(s, FW_CFG_FILE_DIR, s->files, dsize);
>>      }
>>  
>>      count = be32_to_cpu(s->files->count);
>> -    assert(count < FW_CFG_FILE_SLOTS);
>> +    assert(count < fw_cfg_file_slots(s));
>>  
>>      /* Find the insertion point. */
>>      if (mc->legacy_fw_cfg_order) {
>>          /*
>>           * Sort by order. For files with the same order, we keep them
>> @@ -855,11 +869,11 @@ void *fw_cfg_modify_file(FWCfgState *s, const char *filename,
>>      void *ptr = NULL;
>>  
>>      assert(s->files);
>>  
>>      index = be32_to_cpu(s->files->count);
>> -    assert(index < FW_CFG_FILE_SLOTS);
>> +    assert(index < fw_cfg_file_slots(s));
>>  
>>      for (i = 0; i < index; i++) {
>>          if (strcmp(filename, s->files->f[i].name) == 0) {
>>              ptr = fw_cfg_modify_bytes_read(s, FW_CFG_FILE_FIRST + i,
>>                                             data, len);
>> @@ -926,10 +940,16 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
>>      qdev_prop_set_uint32(dev, "dma_iobase", dma_iobase);
>>      if (!dma_requested) {
>>          qdev_prop_set_bit(dev, "dma_enabled", false);
>>      }
>>  
>> +    /* Once we expose the "file_slots" property to callers of
>> +     * fw_cfg_init_io_dma(), the following setting should become conditional on
>> +     * the input parameter being lower than the current value of the property.
>> +     */
>> +    qdev_prop_set_uint32(dev, "file_slots", FW_CFG_FILE_SLOTS_TRAD);
>> +
>>      fw_cfg_init1(dev);
>>      s = FW_CFG(dev);
>>  
>>      if (s->dma_enabled) {
>>          /* 64 bits for the address field */
>> @@ -963,10 +983,17 @@ FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr,
>>      qdev_prop_set_uint32(dev, "data_width", data_width);
>>      if (!dma_requested) {
>>          qdev_prop_set_bit(dev, "dma_enabled", false);
>>      }
>>  
>> +    /* Once we expose the "file_slots" property to callers of
>> +     * fw_cfg_init_mem_wide(), the following setting should become conditional
>> +     * on the input parameter being lower than the current value of the
>> +     * property. Refer to fw_cfg_init_io_dma().
>> +     */
>> +    qdev_prop_set_uint32(dev, "file_slots", FW_CFG_FILE_SLOTS_TRAD);
> both above cases of qdev_prop_set_uint32() could be replaced by
> compat property for fwcfg device which would clamp "file_slots"
> to old value for old machine types in a cleaner way.

The unconditional qdev_prop_set_uint32() call is already replaced, in
fw_cfg_init_io_dma(), in the next patch, and the compat properties are
added in the patch after it.

The current approach is:
- patch #2 adds the property with the raised default, but clamps it
  down in code that calls fw_cfg_init1() directly
- patch #3 propagates the clamping-down a little bit outwards, towards
  board code, but only in the IO-port mapped case,
- patch #4 adds the 2.8 compat props and raises the limit in 2.9 pc
  board code (i.e., 2.9 pc opts in)

The point is that
(a) no old machine type should see any change
(b) even for new machine types, the higher file slot count is opt-in for
    board code (i.e., it shouldn't affect callers of
    fw_cfg_init_mem_wide() and fw_cfg_init_io())

The qdev_prop_set_uint32() call that unconditionally remains in
fw_cfg_init_mem_wide() at the end of this series is for ensuring (b).

The "hw/arm/virt.c" board calls fw_cfg_init_mem_wide(), and that board
should not receive the increased fw_cfg file count even in 2.9+ (to
which the compat props would not apply).

In order to remove the unconditional property setting from
fw_cfg_init_mem_wide() too, I'd either have to modify the
fw_cfg_init_mem_wide() prototype and also the call site in
"hw/arm/virt.c", or we'd have to carry forward the compat property to
2.9 and later, indefinitely.

I'm open to reworking this code, but both goals (a) and (b) should be
considered in any alternative implementation.

Thanks!
Laszlo

> 
>> +
>>      fw_cfg_init1(dev);
>>  
>>      sbd = SYS_BUS_DEVICE(dev);
>>      sysbus_mmio_map(sbd, 0, ctl_addr);
>>      sysbus_mmio_map(sbd, 1, data_addr);
>> @@ -1012,23 +1039,56 @@ static const TypeInfo fw_cfg_info = {
>>      .abstract      = true,
>>      .instance_size = sizeof(FWCfgState),
>>      .class_init    = fw_cfg_class_init,
>>  };
>>  
>> +static void fw_cfg_file_slots_allocate(FWCfgState *s, Error **errp)
>> +{
>> +    uint16_t file_slots_max;
>> +
>> +    if (fw_cfg_file_slots(s) < FW_CFG_FILE_SLOTS_TRAD) {
>> +        error_setg(errp, "\"file_slots\" must be at least 0x%x",
>> +                   FW_CFG_FILE_SLOTS_TRAD);
>> +        return;
>> +    }
>> +
>> +    /* (UINT16_MAX & FW_CFG_ENTRY_MASK) is the highest inclusive selector value
>> +     * that we permit. The actual (exclusive) value coming from the
>> +     * configuration is (FW_CFG_FILE_FIRST + fw_cfg_file_slots(s)). */
>> +    file_slots_max = (UINT16_MAX & FW_CFG_ENTRY_MASK) - FW_CFG_FILE_FIRST + 1;
>> +    if (fw_cfg_file_slots(s) > file_slots_max) {
>> +        error_setg(errp, "\"file_slots\" must not exceed 0x%" PRIx16,
>> +                   file_slots_max);
>> +        return;
>> +    }
>> +
>> +    s->entries[0] = g_new0(FWCfgEntry, fw_cfg_max_entry(s));
>> +    s->entries[1] = g_new0(FWCfgEntry, fw_cfg_max_entry(s));
>> +    s->entry_order = g_new0(int, fw_cfg_max_entry(s));
>> +}
>>  
>>  static Property fw_cfg_io_properties[] = {
>>      DEFINE_PROP_UINT32("iobase", FWCfgIoState, iobase, -1),
>>      DEFINE_PROP_UINT32("dma_iobase", FWCfgIoState, dma_iobase, -1),
>>      DEFINE_PROP_BOOL("dma_enabled", FWCfgIoState, parent_obj.dma_enabled,
>>                       true),
>> +    DEFINE_PROP_UINT32("file_slots", FWCfgIoState, parent_obj.file_slots,
>> +                       FW_CFG_FILE_SLOTS_DFLT),
>>      DEFINE_PROP_END_OF_LIST(),
>>  };
>>  
>>  static void fw_cfg_io_realize(DeviceState *dev, Error **errp)
>>  {
>>      FWCfgIoState *s = FW_CFG_IO(dev);
>>      SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>> +    Error *local_err = NULL;
>> +
>> +    fw_cfg_file_slots_allocate(FW_CFG(s), &local_err);
>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>> +        return;
>> +    }
>>  
>>      /* when using port i/o, the 8-bit data register ALWAYS overlaps
>>       * with half of the 16-bit control register. Hence, the total size
>>       * of the i/o region used is FW_CFG_CTL_SIZE */
>>      memory_region_init_io(&s->comb_iomem, OBJECT(s), &fw_cfg_comb_mem_ops,
>> @@ -1061,18 +1121,27 @@ static const TypeInfo fw_cfg_io_info = {
>>  
>>  static Property fw_cfg_mem_properties[] = {
>>      DEFINE_PROP_UINT32("data_width", FWCfgMemState, data_width, -1),
>>      DEFINE_PROP_BOOL("dma_enabled", FWCfgMemState, parent_obj.dma_enabled,
>>                       true),
>> +    DEFINE_PROP_UINT32("file_slots", FWCfgMemState, parent_obj.file_slots,
>> +                       FW_CFG_FILE_SLOTS_DFLT),
>>      DEFINE_PROP_END_OF_LIST(),
>>  };
>>  
>>  static void fw_cfg_mem_realize(DeviceState *dev, Error **errp)
>>  {
>>      FWCfgMemState *s = FW_CFG_MEM(dev);
>>      SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>>      const MemoryRegionOps *data_ops = &fw_cfg_data_mem_ops;
>> +    Error *local_err = NULL;
>> +
>> +    fw_cfg_file_slots_allocate(FW_CFG(s), &local_err);
>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>> +        return;
>> +    }
>>  
>>      memory_region_init_io(&s->ctl_iomem, OBJECT(s), &fw_cfg_ctl_mem_ops,
>>                            FW_CFG(s), "fwcfg.ctl", FW_CFG_CTL_SIZE);
>>      sysbus_init_mmio(sbd, &s->ctl_iomem);
>>  
>

Igor Mammedov Dec. 6, 2016, 12:02 p.m. UTC | #6

On Tue, 6 Dec 2016 12:43:06 +0100
Laszlo Ersek <[e-mail]> wrote:

> On 12/06/16 11:50, Igor Mammedov wrote:
> > On Thu,  1 Dec 2016 18:06:19 +0100
> > Laszlo Ersek <[e-mail]> wrote:
> >   
> >> We'd like to raise the value of FW_CFG_FILE_SLOTS. Doing it naively could
> >> lead to problems with backward migration: a more recent QEMU (running an
> >> older machine type) would allow the guest, in fw_cfg_select(), to select a
> >> high key value that is unavailable in the same machine type implemented by
> >> the older (target) QEMU. On the target host, fw_cfg_data_read() for
> >> example could dereference nonexistent entries.
> >>
> >> As first step, size the FWCfgState.entries[*] and FWCfgState.entry_order
> >> arrays dynamically. All three array sizes will be influenced by the new
> >> field (and device property) FWCfgState.file_slots.
> >>
> >> Make the following changes:
> >>
> >> - Replace the FW_CFG_FILE_SLOTS macro with FW_CFG_FILE_SLOTS_TRAD
> >>   (traditional count of fw_cfg file slots) in the header file. The value
> >>   remains 0x10.
> >>
> >> - Replace all uses of FW_CFG_FILE_SLOTS with a helper function called
> >>   fw_cfg_file_slots(), returning the new property.
> >>
> >> - Eliminate the macro FW_CFG_MAX_ENTRY, and replace all its uses with a
> >>   helper function called fw_cfg_max_entry().
> >>
> >> - In the MMIO- and IO-mapped realize functions both, allocate all three
> >>   arrays dynamically, based on the new property.
> >>
> >> - The new property defaults to 0x20; however at the moment we forcibly set
> >>   it to FW_CFG_FILE_SLOTS_TRAD on all code paths available to board code
> >>   (namely in the fw_cfg_init_io_dma() and fw_cfg_init_mem_wide() helper
> >>   functions). This is going to be customized in the following patches.
> >>
> >> Cc: "Gabriel L. Somlo" <[e-mail]>
> >> Cc: "Michael S. Tsirkin" <[e-mail]>
> >> Cc: Gerd Hoffmann <[e-mail]>
> >> Cc: Igor Mammedov <[e-mail]>
> >> Cc: Paolo Bonzini <[e-mail]>
> >> Signed-off-by: Laszlo Ersek <[e-mail]>
> >> ---
> >>
> >> Notes:
> >>     I know that upstream doesn't care about backward migration, but some
> >>     downstreams might.
> >>
> >>  docs/specs/fw_cfg.txt          |  2 +-
> >>  include/hw/nvram/fw_cfg_keys.h |  3 +-
> >>  hw/nvram/fw_cfg.c              | 85 ++++++++++++++++++++++++++++++++++++++----
> >>  3 files changed, 79 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
> >> index a19e2adbe1c6..84e2978706f5 100644
> >> --- a/docs/specs/fw_cfg.txt
> >> +++ b/docs/specs/fw_cfg.txt
> >> @@ -154,11 +154,11 @@ Selector Reg.    Range Usage
> >>  0x8000 - 0xbfff  Arch. Specific (0x0000 - 0x3fff, generally RO, possibly RW
> >>                                   through the DMA interface in QEMU v2.9+)
> >>  0xc000 - 0xffff  Arch. Specific (0x0000 - 0x3fff, RW, ignored in v2.4+)
> >>  
> >>  In practice, the number of allowed firmware configuration items is given
> >> -by the value of FW_CFG_MAX_ENTRY (see fw_cfg.h).
> >> +by the value (FW_CFG_FILE_FIRST + FW_CFG_FILE_SLOTS_TRAD) (see fw_cfg.h).
> >>  
> >>  = Guest-side DMA Interface =
> >>  
> >>  If bit 1 of the feature bitmap is set, the DMA interface is present. This does
> >>  not replace the existing fw_cfg interface, it is an add-on. This interface
> >> diff --git a/include/hw/nvram/fw_cfg_keys.h b/include/hw/nvram/fw_cfg_keys.h
> >> index 0f3e871884c0..627589793671 100644
> >> --- a/include/hw/nvram/fw_cfg_keys.h
> >> +++ b/include/hw/nvram/fw_cfg_keys.h
> >> @@ -27,12 +27,11 @@
> >>  #define FW_CFG_SETUP_SIZE       0x17
> >>  #define FW_CFG_SETUP_DATA       0x18
> >>  #define FW_CFG_FILE_DIR         0x19
> >>  
> >>  #define FW_CFG_FILE_FIRST       0x20
> >> -#define FW_CFG_FILE_SLOTS       0x10
> >> -#define FW_CFG_MAX_ENTRY        (FW_CFG_FILE_FIRST + FW_CFG_FILE_SLOTS)
> >> +#define FW_CFG_FILE_SLOTS_TRAD  0x10
> >>  
> >>  #define FW_CFG_WRITE_CHANNEL    0x4000
> >>  #define FW_CFG_ARCH_LOCAL       0x8000
> >>  #define FW_CFG_ENTRY_MASK       (~(FW_CFG_WRITE_CHANNEL | FW_CFG_ARCH_LOCAL))
> >>  
> >> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> >> index e0145c11a19b..2e1441c09750 100644
> >> --- a/hw/nvram/fw_cfg.c
> >> +++ b/hw/nvram/fw_cfg.c
> >> @@ -31,10 +31,13 @@
> >>  #include "hw/sysbus.h"
> >>  #include "trace.h"
> >>  #include "qemu/error-report.h"
> >>  #include "qemu/config-file.h"
> >>  #include "qemu/cutils.h"
> >> +#include "qapi/error.h"
> >> +
> >> +#define FW_CFG_FILE_SLOTS_DFLT 0x20
> >>  
> >>  #define FW_CFG_NAME "fw_cfg"
> >>  #define FW_CFG_PATH "/machine/" FW_CFG_NAME
> >>  
> >>  #define TYPE_FW_CFG     "fw_cfg"
> >> @@ -69,12 +72,13 @@ typedef struct FWCfgEntry {
> >>  struct FWCfgState {
> >>      /*< private >*/
> >>      SysBusDevice parent_obj;
> >>      /*< public >*/
> >>  
> >> -    FWCfgEntry entries[2][FW_CFG_MAX_ENTRY];
> >> -    int entry_order[FW_CFG_MAX_ENTRY];
> >> +    uint32_t file_slots;  
> > should it be uint16_t?
> > As below you use "uint16_t file_slots_max;" and do some UINT16
> > to calculate max limit.  
> 
> I think I had a reason for making this uint32_t. I think the argument
> was that fw_cfg_max_entry() could theoretically return a value that
> doesn't fit in a uint16_t. Looking again at the patch however, I think I
> can try to make this a uint16_t for the next version.
> 
> >   
> >> +    FWCfgEntry *entries[2];
> >> +    int *entry_order;
> >>      FWCfgFiles *files;
> >>      uint16_t cur_entry;
> >>      uint32_t cur_offset;
> >>      Notifier machine_ready;
> >>  
> >> @@ -255,17 +259,27 @@ static void fw_cfg_reboot(FWCfgState *s)
> >>  static void fw_cfg_write(FWCfgState *s, uint8_t value)
> >>  {
> >>      /* nothing, write support removed in QEMU v2.4+ */
> >>  }
> >>  
> >> +static inline uint32_t fw_cfg_file_slots(const FWCfgState *s)
> >> +{
> >> +    return s->file_slots;
> >> +}  
> > so far it doesn't look like this wrapper function is necessary,
> > I'd prefer accessing field directly as it's used only internally.
> > Or do you have plans to extend wrapper later?
> > (then it would be better introduce wrapper at that time)  
> 
> Originally I used "s->file_slots" in this patch, like you say, without
> this small wrapper function,, but the resultant patch was harder to
> review. With this wrapper, you have two kinds of changes in the patch:
> 
> *     FW_CFG_MAX_ENTRY
>   --> fw_cfg_max_entry(s)  
> 
> *     FW_CFG_FILE_SLOTS
>   --> fw_cfg_file_slots(s)  
> 
> Without the wrapper, the second bullet looks
> 
> *     FW_CFG_FILE_SLOTS
>   --> s->file_slots  
> 
> and to me that made the patch harder to verify.
to me this variant look clearer as I don't have to recall that 
fw_cfg_file_slots(s) is s->file_slots  and does nothing more.
But if you prefer wrapper I'm also fine with it.

> 
> >   
> >> +
> >> +static inline uint32_t fw_cfg_max_entry(const FWCfgState *s)
> >> +{
> >> +    return FW_CFG_FILE_FIRST + fw_cfg_file_slots(s);
> >> +}
> >> +
> >>  static int fw_cfg_select(FWCfgState *s, uint16_t key)
> >>  {
> >>      int arch, ret;
> >>      FWCfgEntry *e;
> >>  
> >>      s->cur_offset = 0;
> >> -    if ((key & FW_CFG_ENTRY_MASK) >= FW_CFG_MAX_ENTRY) {
> >> +    if ((key & FW_CFG_ENTRY_MASK) >= fw_cfg_max_entry(s)) {
> >>          s->cur_entry = FW_CFG_INVALID;
> >>          ret = 0;
> >>      } else {
> >>          s->cur_entry = key;
> >>          ret = 1;
> >> @@ -608,11 +622,11 @@ static void fw_cfg_add_bytes_read_callback(FWCfgState *s, uint16_t key,
> >>  {
> >>      int arch = !!(key & FW_CFG_ARCH_LOCAL);
> >>  
> >>      key &= FW_CFG_ENTRY_MASK;
> >>  
> >> -    assert(key < FW_CFG_MAX_ENTRY && len < UINT32_MAX);
> >> +    assert(key < fw_cfg_max_entry(s) && len < UINT32_MAX);
> >>      assert(s->entries[arch][key].data == NULL); /* avoid key conflict */
> >>  
> >>      s->entries[arch][key].data = data;
> >>      s->entries[arch][key].len = (uint32_t)len;
> >>      s->entries[arch][key].read_callback = callback;
> >> @@ -626,11 +640,11 @@ static void *fw_cfg_modify_bytes_read(FWCfgState *s, uint16_t key,
> >>      void *ptr;
> >>      int arch = !!(key & FW_CFG_ARCH_LOCAL);
> >>  
> >>      key &= FW_CFG_ENTRY_MASK;
> >>  
> >> -    assert(key < FW_CFG_MAX_ENTRY && len < UINT32_MAX);
> >> +    assert(key < fw_cfg_max_entry(s) && len < UINT32_MAX);
> >>  
> >>      /* return the old data to the function caller, avoid memory leak */
> >>      ptr = s->entries[arch][key].data;
> >>      s->entries[arch][key].data = data;
> >>      s->entries[arch][key].len = len;
> >> @@ -775,17 +789,17 @@ void fw_cfg_add_file_callback(FWCfgState *s,  const char *filename,
> >>      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;
> >> +        dsize = sizeof(uint32_t) + sizeof(FWCfgFile) * fw_cfg_file_slots(s);
> >>          s->files = g_malloc0(dsize);
> >>          fw_cfg_add_bytes(s, FW_CFG_FILE_DIR, s->files, dsize);
> >>      }
> >>  
> >>      count = be32_to_cpu(s->files->count);
> >> -    assert(count < FW_CFG_FILE_SLOTS);
> >> +    assert(count < fw_cfg_file_slots(s));
> >>  
> >>      /* Find the insertion point. */
> >>      if (mc->legacy_fw_cfg_order) {
> >>          /*
> >>           * Sort by order. For files with the same order, we keep them
> >> @@ -855,11 +869,11 @@ void *fw_cfg_modify_file(FWCfgState *s, const char *filename,
> >>      void *ptr = NULL;
> >>  
> >>      assert(s->files);
> >>  
> >>      index = be32_to_cpu(s->files->count);
> >> -    assert(index < FW_CFG_FILE_SLOTS);
> >> +    assert(index < fw_cfg_file_slots(s));
> >>  
> >>      for (i = 0; i < index; i++) {
> >>          if (strcmp(filename, s->files->f[i].name) == 0) {
> >>              ptr = fw_cfg_modify_bytes_read(s, FW_CFG_FILE_FIRST + i,
> >>                                             data, len);
> >> @@ -926,10 +940,16 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
> >>      qdev_prop_set_uint32(dev, "dma_iobase", dma_iobase);
> >>      if (!dma_requested) {
> >>          qdev_prop_set_bit(dev, "dma_enabled", false);
> >>      }
> >>  
> >> +    /* Once we expose the "file_slots" property to callers of
> >> +     * fw_cfg_init_io_dma(), the following setting should become conditional on
> >> +     * the input parameter being lower than the current value of the property.
> >> +     */
> >> +    qdev_prop_set_uint32(dev, "file_slots", FW_CFG_FILE_SLOTS_TRAD);
> >> +
> >>      fw_cfg_init1(dev);
> >>      s = FW_CFG(dev);
> >>  
> >>      if (s->dma_enabled) {
> >>          /* 64 bits for the address field */
> >> @@ -963,10 +983,17 @@ FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr,
> >>      qdev_prop_set_uint32(dev, "data_width", data_width);
> >>      if (!dma_requested) {
> >>          qdev_prop_set_bit(dev, "dma_enabled", false);
> >>      }
> >>  
> >> +    /* Once we expose the "file_slots" property to callers of
> >> +     * fw_cfg_init_mem_wide(), the following setting should become conditional
> >> +     * on the input parameter being lower than the current value of the
> >> +     * property. Refer to fw_cfg_init_io_dma().
> >> +     */
> >> +    qdev_prop_set_uint32(dev, "file_slots", FW_CFG_FILE_SLOTS_TRAD);  
> > both above cases of qdev_prop_set_uint32() could be replaced by
> > compat property for fwcfg device which would clamp "file_slots"
> > to old value for old machine types in a cleaner way.  
> 
> The unconditional qdev_prop_set_uint32() call is already replaced, in
> fw_cfg_init_io_dma(), in the next patch, and the compat properties are
> added in the patch after it.
> 
> The current approach is:
> - patch #2 adds the property with the raised default, but clamps it
>   down in code that calls fw_cfg_init1() directly
> - patch #3 propagates the clamping-down a little bit outwards, towards
>   board code, but only in the IO-port mapped case,
> - patch #4 adds the 2.8 compat props and raises the limit in 2.9 pc
>   board code (i.e., 2.9 pc opts in)
> 
> The point is that
> (a) no old machine type should see any change
> (b) even for new machine types, the higher file slot count is opt-in for
>     board code (i.e., it shouldn't affect callers of
>     fw_cfg_init_mem_wide() and fw_cfg_init_io())
I'm not sure that we need (b) (i.e. optin for new machine versions)
Maybe all new machine types should use new default and we should
clamp it down for all old machine types the same way (compat prop).
It would be uniform and less confusing possibly making series simpler.

> 
> The qdev_prop_set_uint32() call that unconditionally remains in
> fw_cfg_init_mem_wide() at the end of this series is for ensuring (b).
> 
> The "hw/arm/virt.c" board calls fw_cfg_init_mem_wide(), and that board
> should not receive the increased fw_cfg file count even in 2.9+ (to
> which the compat props would not apply).
> 
> In order to remove the unconditional property setting from
> fw_cfg_init_mem_wide() too, I'd either have to modify the
> fw_cfg_init_mem_wide() prototype and also the call site in
> "hw/arm/virt.c", or we'd have to carry forward the compat property to
> 2.9 and later, indefinitely.
> 
> I'm open to reworking this code, but both goals (a) and (b) should be
> considered in any alternative implementation.
> 
> Thanks!
> Laszlo
> 
> >   
> >> +
> >>      fw_cfg_init1(dev);
> >>  
> >>      sbd = SYS_BUS_DEVICE(dev);
> >>      sysbus_mmio_map(sbd, 0, ctl_addr);
> >>      sysbus_mmio_map(sbd, 1, data_addr);
> >> @@ -1012,23 +1039,56 @@ static const TypeInfo fw_cfg_info = {
> >>      .abstract      = true,
> >>      .instance_size = sizeof(FWCfgState),
> >>      .class_init    = fw_cfg_class_init,
> >>  };
> >>  
> >> +static void fw_cfg_file_slots_allocate(FWCfgState *s, Error **errp)
> >> +{
> >> +    uint16_t file_slots_max;
> >> +
> >> +    if (fw_cfg_file_slots(s) < FW_CFG_FILE_SLOTS_TRAD) {
> >> +        error_setg(errp, "\"file_slots\" must be at least 0x%x",
> >> +                   FW_CFG_FILE_SLOTS_TRAD);
> >> +        return;
> >> +    }
> >> +
> >> +    /* (UINT16_MAX & FW_CFG_ENTRY_MASK) is the highest inclusive selector value
> >> +     * that we permit. The actual (exclusive) value coming from the
> >> +     * configuration is (FW_CFG_FILE_FIRST + fw_cfg_file_slots(s)). */
> >> +    file_slots_max = (UINT16_MAX & FW_CFG_ENTRY_MASK) - FW_CFG_FILE_FIRST + 1;
> >> +    if (fw_cfg_file_slots(s) > file_slots_max) {
> >> +        error_setg(errp, "\"file_slots\" must not exceed 0x%" PRIx16,
> >> +                   file_slots_max);
> >> +        return;
> >> +    }
> >> +
> >> +    s->entries[0] = g_new0(FWCfgEntry, fw_cfg_max_entry(s));
> >> +    s->entries[1] = g_new0(FWCfgEntry, fw_cfg_max_entry(s));
> >> +    s->entry_order = g_new0(int, fw_cfg_max_entry(s));
> >> +}
> >>  
> >>  static Property fw_cfg_io_properties[] = {
> >>      DEFINE_PROP_UINT32("iobase", FWCfgIoState, iobase, -1),
> >>      DEFINE_PROP_UINT32("dma_iobase", FWCfgIoState, dma_iobase, -1),
> >>      DEFINE_PROP_BOOL("dma_enabled", FWCfgIoState, parent_obj.dma_enabled,
> >>                       true),
> >> +    DEFINE_PROP_UINT32("file_slots", FWCfgIoState, parent_obj.file_slots,
> >> +                       FW_CFG_FILE_SLOTS_DFLT),
> >>      DEFINE_PROP_END_OF_LIST(),
> >>  };
> >>  
> >>  static void fw_cfg_io_realize(DeviceState *dev, Error **errp)
> >>  {
> >>      FWCfgIoState *s = FW_CFG_IO(dev);
> >>      SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> >> +    Error *local_err = NULL;
> >> +
> >> +    fw_cfg_file_slots_allocate(FW_CFG(s), &local_err);
> >> +    if (local_err) {
> >> +        error_propagate(errp, local_err);
> >> +        return;
> >> +    }
> >>  
> >>      /* when using port i/o, the 8-bit data register ALWAYS overlaps
> >>       * with half of the 16-bit control register. Hence, the total size
> >>       * of the i/o region used is FW_CFG_CTL_SIZE */
> >>      memory_region_init_io(&s->comb_iomem, OBJECT(s), &fw_cfg_comb_mem_ops,
> >> @@ -1061,18 +1121,27 @@ static const TypeInfo fw_cfg_io_info = {
> >>  
> >>  static Property fw_cfg_mem_properties[] = {
> >>      DEFINE_PROP_UINT32("data_width", FWCfgMemState, data_width, -1),
> >>      DEFINE_PROP_BOOL("dma_enabled", FWCfgMemState, parent_obj.dma_enabled,
> >>                       true),
> >> +    DEFINE_PROP_UINT32("file_slots", FWCfgMemState, parent_obj.file_slots,
> >> +                       FW_CFG_FILE_SLOTS_DFLT),
> >>      DEFINE_PROP_END_OF_LIST(),
> >>  };
> >>  
> >>  static void fw_cfg_mem_realize(DeviceState *dev, Error **errp)
> >>  {
> >>      FWCfgMemState *s = FW_CFG_MEM(dev);
> >>      SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> >>      const MemoryRegionOps *data_ops = &fw_cfg_data_mem_ops;
> >> +    Error *local_err = NULL;
> >> +
> >> +    fw_cfg_file_slots_allocate(FW_CFG(s), &local_err);
> >> +    if (local_err) {
> >> +        error_propagate(errp, local_err);
> >> +        return;
> >> +    }
> >>  
> >>      memory_region_init_io(&s->ctl_iomem, OBJECT(s), &fw_cfg_ctl_mem_ops,
> >>                            FW_CFG(s), "fwcfg.ctl", FW_CFG_CTL_SIZE);
> >>      sysbus_init_mmio(sbd, &s->ctl_iomem);
> >>    
> >   
>

Laszlo Ersek Dec. 6, 2016, 4:22 p.m. UTC | #7

On 12/06/16 13:02, Igor Mammedov wrote:
> On Tue, 6 Dec 2016 12:43:06 +0100
> Laszlo Ersek <[e-mail]> wrote:
> 
>> On 12/06/16 11:50, Igor Mammedov wrote:
>>> On Thu,  1 Dec 2016 18:06:19 +0100
>>> Laszlo Ersek <[e-mail]> wrote:
>>>   
>>>> We'd like to raise the value of FW_CFG_FILE_SLOTS. Doing it naively could
>>>> lead to problems with backward migration: a more recent QEMU (running an
>>>> older machine type) would allow the guest, in fw_cfg_select(), to select a
>>>> high key value that is unavailable in the same machine type implemented by
>>>> the older (target) QEMU. On the target host, fw_cfg_data_read() for
>>>> example could dereference nonexistent entries.
>>>>
>>>> As first step, size the FWCfgState.entries[*] and FWCfgState.entry_order
>>>> arrays dynamically. All three array sizes will be influenced by the new
>>>> field (and device property) FWCfgState.file_slots.
>>>>
>>>> Make the following changes:
>>>>
>>>> - Replace the FW_CFG_FILE_SLOTS macro with FW_CFG_FILE_SLOTS_TRAD
>>>>   (traditional count of fw_cfg file slots) in the header file. The value
>>>>   remains 0x10.
>>>>
>>>> - Replace all uses of FW_CFG_FILE_SLOTS with a helper function called
>>>>   fw_cfg_file_slots(), returning the new property.
>>>>
>>>> - Eliminate the macro FW_CFG_MAX_ENTRY, and replace all its uses with a
>>>>   helper function called fw_cfg_max_entry().
>>>>
>>>> - In the MMIO- and IO-mapped realize functions both, allocate all three
>>>>   arrays dynamically, based on the new property.
>>>>
>>>> - The new property defaults to 0x20; however at the moment we forcibly set
>>>>   it to FW_CFG_FILE_SLOTS_TRAD on all code paths available to board code
>>>>   (namely in the fw_cfg_init_io_dma() and fw_cfg_init_mem_wide() helper
>>>>   functions). This is going to be customized in the following patches.
>>>>
>>>> Cc: "Gabriel L. Somlo" <[e-mail]>
>>>> Cc: "Michael S. Tsirkin" <[e-mail]>
>>>> Cc: Gerd Hoffmann <[e-mail]>
>>>> Cc: Igor Mammedov <[e-mail]>
>>>> Cc: Paolo Bonzini <[e-mail]>
>>>> Signed-off-by: Laszlo Ersek <[e-mail]>
>>>> ---
>>>>
>>>> Notes:
>>>>     I know that upstream doesn't care about backward migration, but some
>>>>     downstreams might.
>>>>
>>>>  docs/specs/fw_cfg.txt          |  2 +-
>>>>  include/hw/nvram/fw_cfg_keys.h |  3 +-
>>>>  hw/nvram/fw_cfg.c              | 85 ++++++++++++++++++++++++++++++++++++++----
>>>>  3 files changed, 79 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
>>>> index a19e2adbe1c6..84e2978706f5 100644
>>>> --- a/docs/specs/fw_cfg.txt
>>>> +++ b/docs/specs/fw_cfg.txt
>>>> @@ -154,11 +154,11 @@ Selector Reg.    Range Usage
>>>>  0x8000 - 0xbfff  Arch. Specific (0x0000 - 0x3fff, generally RO, possibly RW
>>>>                                   through the DMA interface in QEMU v2.9+)
>>>>  0xc000 - 0xffff  Arch. Specific (0x0000 - 0x3fff, RW, ignored in v2.4+)
>>>>  
>>>>  In practice, the number of allowed firmware configuration items is given
>>>> -by the value of FW_CFG_MAX_ENTRY (see fw_cfg.h).
>>>> +by the value (FW_CFG_FILE_FIRST + FW_CFG_FILE_SLOTS_TRAD) (see fw_cfg.h).
>>>>  
>>>>  = Guest-side DMA Interface =
>>>>  
>>>>  If bit 1 of the feature bitmap is set, the DMA interface is present. This does
>>>>  not replace the existing fw_cfg interface, it is an add-on. This interface
>>>> diff --git a/include/hw/nvram/fw_cfg_keys.h b/include/hw/nvram/fw_cfg_keys.h
>>>> index 0f3e871884c0..627589793671 100644
>>>> --- a/include/hw/nvram/fw_cfg_keys.h
>>>> +++ b/include/hw/nvram/fw_cfg_keys.h
>>>> @@ -27,12 +27,11 @@
>>>>  #define FW_CFG_SETUP_SIZE       0x17
>>>>  #define FW_CFG_SETUP_DATA       0x18
>>>>  #define FW_CFG_FILE_DIR         0x19
>>>>  
>>>>  #define FW_CFG_FILE_FIRST       0x20
>>>> -#define FW_CFG_FILE_SLOTS       0x10
>>>> -#define FW_CFG_MAX_ENTRY        (FW_CFG_FILE_FIRST + FW_CFG_FILE_SLOTS)
>>>> +#define FW_CFG_FILE_SLOTS_TRAD  0x10
>>>>  
>>>>  #define FW_CFG_WRITE_CHANNEL    0x4000
>>>>  #define FW_CFG_ARCH_LOCAL       0x8000
>>>>  #define FW_CFG_ENTRY_MASK       (~(FW_CFG_WRITE_CHANNEL | FW_CFG_ARCH_LOCAL))
>>>>  
>>>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
>>>> index e0145c11a19b..2e1441c09750 100644
>>>> --- a/hw/nvram/fw_cfg.c
>>>> +++ b/hw/nvram/fw_cfg.c
>>>> @@ -31,10 +31,13 @@
>>>>  #include "hw/sysbus.h"
>>>>  #include "trace.h"
>>>>  #include "qemu/error-report.h"
>>>>  #include "qemu/config-file.h"
>>>>  #include "qemu/cutils.h"
>>>> +#include "qapi/error.h"
>>>> +
>>>> +#define FW_CFG_FILE_SLOTS_DFLT 0x20
>>>>  
>>>>  #define FW_CFG_NAME "fw_cfg"
>>>>  #define FW_CFG_PATH "/machine/" FW_CFG_NAME
>>>>  
>>>>  #define TYPE_FW_CFG     "fw_cfg"
>>>> @@ -69,12 +72,13 @@ typedef struct FWCfgEntry {
>>>>  struct FWCfgState {
>>>>      /*< private >*/
>>>>      SysBusDevice parent_obj;
>>>>      /*< public >*/
>>>>  
>>>> -    FWCfgEntry entries[2][FW_CFG_MAX_ENTRY];
>>>> -    int entry_order[FW_CFG_MAX_ENTRY];
>>>> +    uint32_t file_slots;  
>>> should it be uint16_t?
>>> As below you use "uint16_t file_slots_max;" and do some UINT16
>>> to calculate max limit.  
>>
>> I think I had a reason for making this uint32_t. I think the argument
>> was that fw_cfg_max_entry() could theoretically return a value that
>> doesn't fit in a uint16_t. Looking again at the patch however, I think I
>> can try to make this a uint16_t for the next version.
>>
>>>   
>>>> +    FWCfgEntry *entries[2];
>>>> +    int *entry_order;
>>>>      FWCfgFiles *files;
>>>>      uint16_t cur_entry;
>>>>      uint32_t cur_offset;
>>>>      Notifier machine_ready;
>>>>  
>>>> @@ -255,17 +259,27 @@ static void fw_cfg_reboot(FWCfgState *s)
>>>>  static void fw_cfg_write(FWCfgState *s, uint8_t value)
>>>>  {
>>>>      /* nothing, write support removed in QEMU v2.4+ */
>>>>  }
>>>>  
>>>> +static inline uint32_t fw_cfg_file_slots(const FWCfgState *s)
>>>> +{
>>>> +    return s->file_slots;
>>>> +}  
>>> so far it doesn't look like this wrapper function is necessary,
>>> I'd prefer accessing field directly as it's used only internally.
>>> Or do you have plans to extend wrapper later?
>>> (then it would be better introduce wrapper at that time)  
>>
>> Originally I used "s->file_slots" in this patch, like you say, without
>> this small wrapper function,, but the resultant patch was harder to
>> review. With this wrapper, you have two kinds of changes in the patch:
>>
>> *     FW_CFG_MAX_ENTRY
>>   --> fw_cfg_max_entry(s)  
>>
>> *     FW_CFG_FILE_SLOTS
>>   --> fw_cfg_file_slots(s)  
>>
>> Without the wrapper, the second bullet looks
>>
>> *     FW_CFG_FILE_SLOTS
>>   --> s->file_slots  
>>
>> and to me that made the patch harder to verify.
> to me this variant look clearer as I don't have to recall that 
> fw_cfg_file_slots(s) is s->file_slots  and does nothing more.
> But if you prefer wrapper I'm also fine with it.
> 
>>
>>>   
>>>> +
>>>> +static inline uint32_t fw_cfg_max_entry(const FWCfgState *s)
>>>> +{
>>>> +    return FW_CFG_FILE_FIRST + fw_cfg_file_slots(s);
>>>> +}
>>>> +
>>>>  static int fw_cfg_select(FWCfgState *s, uint16_t key)
>>>>  {
>>>>      int arch, ret;
>>>>      FWCfgEntry *e;
>>>>  
>>>>      s->cur_offset = 0;
>>>> -    if ((key & FW_CFG_ENTRY_MASK) >= FW_CFG_MAX_ENTRY) {
>>>> +    if ((key & FW_CFG_ENTRY_MASK) >= fw_cfg_max_entry(s)) {
>>>>          s->cur_entry = FW_CFG_INVALID;
>>>>          ret = 0;
>>>>      } else {
>>>>          s->cur_entry = key;
>>>>          ret = 1;
>>>> @@ -608,11 +622,11 @@ static void fw_cfg_add_bytes_read_callback(FWCfgState *s, uint16_t key,
>>>>  {
>>>>      int arch = !!(key & FW_CFG_ARCH_LOCAL);
>>>>  
>>>>      key &= FW_CFG_ENTRY_MASK;
>>>>  
>>>> -    assert(key < FW_CFG_MAX_ENTRY && len < UINT32_MAX);
>>>> +    assert(key < fw_cfg_max_entry(s) && len < UINT32_MAX);
>>>>      assert(s->entries[arch][key].data == NULL); /* avoid key conflict */
>>>>  
>>>>      s->entries[arch][key].data = data;
>>>>      s->entries[arch][key].len = (uint32_t)len;
>>>>      s->entries[arch][key].read_callback = callback;
>>>> @@ -626,11 +640,11 @@ static void *fw_cfg_modify_bytes_read(FWCfgState *s, uint16_t key,
>>>>      void *ptr;
>>>>      int arch = !!(key & FW_CFG_ARCH_LOCAL);
>>>>  
>>>>      key &= FW_CFG_ENTRY_MASK;
>>>>  
>>>> -    assert(key < FW_CFG_MAX_ENTRY && len < UINT32_MAX);
>>>> +    assert(key < fw_cfg_max_entry(s) && len < UINT32_MAX);
>>>>  
>>>>      /* return the old data to the function caller, avoid memory leak */
>>>>      ptr = s->entries[arch][key].data;
>>>>      s->entries[arch][key].data = data;
>>>>      s->entries[arch][key].len = len;
>>>> @@ -775,17 +789,17 @@ void fw_cfg_add_file_callback(FWCfgState *s,  const char *filename,
>>>>      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;
>>>> +        dsize = sizeof(uint32_t) + sizeof(FWCfgFile) * fw_cfg_file_slots(s);
>>>>          s->files = g_malloc0(dsize);
>>>>          fw_cfg_add_bytes(s, FW_CFG_FILE_DIR, s->files, dsize);
>>>>      }
>>>>  
>>>>      count = be32_to_cpu(s->files->count);
>>>> -    assert(count < FW_CFG_FILE_SLOTS);
>>>> +    assert(count < fw_cfg_file_slots(s));
>>>>  
>>>>      /* Find the insertion point. */
>>>>      if (mc->legacy_fw_cfg_order) {
>>>>          /*
>>>>           * Sort by order. For files with the same order, we keep them
>>>> @@ -855,11 +869,11 @@ void *fw_cfg_modify_file(FWCfgState *s, const char *filename,
>>>>      void *ptr = NULL;
>>>>  
>>>>      assert(s->files);
>>>>  
>>>>      index = be32_to_cpu(s->files->count);
>>>> -    assert(index < FW_CFG_FILE_SLOTS);
>>>> +    assert(index < fw_cfg_file_slots(s));
>>>>  
>>>>      for (i = 0; i < index; i++) {
>>>>          if (strcmp(filename, s->files->f[i].name) == 0) {
>>>>              ptr = fw_cfg_modify_bytes_read(s, FW_CFG_FILE_FIRST + i,
>>>>                                             data, len);
>>>> @@ -926,10 +940,16 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
>>>>      qdev_prop_set_uint32(dev, "dma_iobase", dma_iobase);
>>>>      if (!dma_requested) {
>>>>          qdev_prop_set_bit(dev, "dma_enabled", false);
>>>>      }
>>>>  
>>>> +    /* Once we expose the "file_slots" property to callers of
>>>> +     * fw_cfg_init_io_dma(), the following setting should become conditional on
>>>> +     * the input parameter being lower than the current value of the property.
>>>> +     */
>>>> +    qdev_prop_set_uint32(dev, "file_slots", FW_CFG_FILE_SLOTS_TRAD);
>>>> +
>>>>      fw_cfg_init1(dev);
>>>>      s = FW_CFG(dev);
>>>>  
>>>>      if (s->dma_enabled) {
>>>>          /* 64 bits for the address field */
>>>> @@ -963,10 +983,17 @@ FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr,
>>>>      qdev_prop_set_uint32(dev, "data_width", data_width);
>>>>      if (!dma_requested) {
>>>>          qdev_prop_set_bit(dev, "dma_enabled", false);
>>>>      }
>>>>  
>>>> +    /* Once we expose the "file_slots" property to callers of
>>>> +     * fw_cfg_init_mem_wide(), the following setting should become conditional
>>>> +     * on the input parameter being lower than the current value of the
>>>> +     * property. Refer to fw_cfg_init_io_dma().
>>>> +     */
>>>> +    qdev_prop_set_uint32(dev, "file_slots", FW_CFG_FILE_SLOTS_TRAD);  
>>> both above cases of qdev_prop_set_uint32() could be replaced by
>>> compat property for fwcfg device which would clamp "file_slots"
>>> to old value for old machine types in a cleaner way.  
>>
>> The unconditional qdev_prop_set_uint32() call is already replaced, in
>> fw_cfg_init_io_dma(), in the next patch, and the compat properties are
>> added in the patch after it.
>>
>> The current approach is:
>> - patch #2 adds the property with the raised default, but clamps it
>>   down in code that calls fw_cfg_init1() directly
>> - patch #3 propagates the clamping-down a little bit outwards, towards
>>   board code, but only in the IO-port mapped case,
>> - patch #4 adds the 2.8 compat props and raises the limit in 2.9 pc
>>   board code (i.e., 2.9 pc opts in)
>>
>> The point is that
>> (a) no old machine type should see any change
>> (b) even for new machine types, the higher file slot count is opt-in for
>>     board code (i.e., it shouldn't affect callers of
>>     fw_cfg_init_mem_wide() and fw_cfg_init_io())
> I'm not sure that we need (b) (i.e. optin for new machine versions)
> Maybe all new machine types should use new default and we should
> clamp it down for all old machine types the same way (compat prop).
> It would be uniform and less confusing possibly making series simpler.

Works for me if (b) is a non-goal. I'll update the patches.

Thanks!
Laszlo

> 
>>
>> The qdev_prop_set_uint32() call that unconditionally remains in
>> fw_cfg_init_mem_wide() at the end of this series is for ensuring (b).
>>
>> The "hw/arm/virt.c" board calls fw_cfg_init_mem_wide(), and that board
>> should not receive the increased fw_cfg file count even in 2.9+ (to
>> which the compat props would not apply).
>>
>> In order to remove the unconditional property setting from
>> fw_cfg_init_mem_wide() too, I'd either have to modify the
>> fw_cfg_init_mem_wide() prototype and also the call site in
>> "hw/arm/virt.c", or we'd have to carry forward the compat property to
>> 2.9 and later, indefinitely.
>>
>> I'm open to reworking this code, but both goals (a) and (b) should be
>> considered in any alternative implementation.
>>
>> Thanks!
>> Laszlo
>>
>>>   
>>>> +
>>>>      fw_cfg_init1(dev);
>>>>  
>>>>      sbd = SYS_BUS_DEVICE(dev);
>>>>      sysbus_mmio_map(sbd, 0, ctl_addr);
>>>>      sysbus_mmio_map(sbd, 1, data_addr);
>>>> @@ -1012,23 +1039,56 @@ static const TypeInfo fw_cfg_info = {
>>>>      .abstract      = true,
>>>>      .instance_size = sizeof(FWCfgState),
>>>>      .class_init    = fw_cfg_class_init,
>>>>  };
>>>>  
>>>> +static void fw_cfg_file_slots_allocate(FWCfgState *s, Error **errp)
>>>> +{
>>>> +    uint16_t file_slots_max;
>>>> +
>>>> +    if (fw_cfg_file_slots(s) < FW_CFG_FILE_SLOTS_TRAD) {
>>>> +        error_setg(errp, "\"file_slots\" must be at least 0x%x",
>>>> +                   FW_CFG_FILE_SLOTS_TRAD);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    /* (UINT16_MAX & FW_CFG_ENTRY_MASK) is the highest inclusive selector value
>>>> +     * that we permit. The actual (exclusive) value coming from the
>>>> +     * configuration is (FW_CFG_FILE_FIRST + fw_cfg_file_slots(s)). */
>>>> +    file_slots_max = (UINT16_MAX & FW_CFG_ENTRY_MASK) - FW_CFG_FILE_FIRST + 1;
>>>> +    if (fw_cfg_file_slots(s) > file_slots_max) {
>>>> +        error_setg(errp, "\"file_slots\" must not exceed 0x%" PRIx16,
>>>> +                   file_slots_max);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    s->entries[0] = g_new0(FWCfgEntry, fw_cfg_max_entry(s));
>>>> +    s->entries[1] = g_new0(FWCfgEntry, fw_cfg_max_entry(s));
>>>> +    s->entry_order = g_new0(int, fw_cfg_max_entry(s));
>>>> +}
>>>>  
>>>>  static Property fw_cfg_io_properties[] = {
>>>>      DEFINE_PROP_UINT32("iobase", FWCfgIoState, iobase, -1),
>>>>      DEFINE_PROP_UINT32("dma_iobase", FWCfgIoState, dma_iobase, -1),
>>>>      DEFINE_PROP_BOOL("dma_enabled", FWCfgIoState, parent_obj.dma_enabled,
>>>>                       true),
>>>> +    DEFINE_PROP_UINT32("file_slots", FWCfgIoState, parent_obj.file_slots,
>>>> +                       FW_CFG_FILE_SLOTS_DFLT),
>>>>      DEFINE_PROP_END_OF_LIST(),
>>>>  };
>>>>  
>>>>  static void fw_cfg_io_realize(DeviceState *dev, Error **errp)
>>>>  {
>>>>      FWCfgIoState *s = FW_CFG_IO(dev);
>>>>      SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>>>> +    Error *local_err = NULL;
>>>> +
>>>> +    fw_cfg_file_slots_allocate(FW_CFG(s), &local_err);
>>>> +    if (local_err) {
>>>> +        error_propagate(errp, local_err);
>>>> +        return;
>>>> +    }
>>>>  
>>>>      /* when using port i/o, the 8-bit data register ALWAYS overlaps
>>>>       * with half of the 16-bit control register. Hence, the total size
>>>>       * of the i/o region used is FW_CFG_CTL_SIZE */
>>>>      memory_region_init_io(&s->comb_iomem, OBJECT(s), &fw_cfg_comb_mem_ops,
>>>> @@ -1061,18 +1121,27 @@ static const TypeInfo fw_cfg_io_info = {
>>>>  
>>>>  static Property fw_cfg_mem_properties[] = {
>>>>      DEFINE_PROP_UINT32("data_width", FWCfgMemState, data_width, -1),
>>>>      DEFINE_PROP_BOOL("dma_enabled", FWCfgMemState, parent_obj.dma_enabled,
>>>>                       true),
>>>> +    DEFINE_PROP_UINT32("file_slots", FWCfgMemState, parent_obj.file_slots,
>>>> +                       FW_CFG_FILE_SLOTS_DFLT),
>>>>      DEFINE_PROP_END_OF_LIST(),
>>>>  };
>>>>  
>>>>  static void fw_cfg_mem_realize(DeviceState *dev, Error **errp)
>>>>  {
>>>>      FWCfgMemState *s = FW_CFG_MEM(dev);
>>>>      SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>>>>      const MemoryRegionOps *data_ops = &fw_cfg_data_mem_ops;
>>>> +    Error *local_err = NULL;
>>>> +
>>>> +    fw_cfg_file_slots_allocate(FW_CFG(s), &local_err);
>>>> +    if (local_err) {
>>>> +        error_propagate(errp, local_err);
>>>> +        return;
>>>> +    }
>>>>  
>>>>      memory_region_init_io(&s->ctl_iomem, OBJECT(s), &fw_cfg_ctl_mem_ops,
>>>>                            FW_CFG(s), "fwcfg.ctl", FW_CFG_CTL_SIZE);
>>>>      sysbus_init_mmio(sbd, &s->ctl_iomem);
>>>>    
>>>   
>>
>

Michael S. Tsirkin Jan. 10, 2017, 3:02 p.m. UTC | #8

On Thu, Dec 01, 2016 at 06:06:19PM +0100, Laszlo Ersek wrote:
> We'd like to raise the value of FW_CFG_FILE_SLOTS. Doing it naively could
> lead to problems with backward migration: a more recent QEMU (running an
> older machine type) would allow the guest, in fw_cfg_select(), to select a
> high key value that is unavailable in the same machine type implemented by
> the older (target) QEMU. On the target host, fw_cfg_data_read() for
> example could dereference nonexistent entries.
> 
> As first step, size the FWCfgState.entries[*] and FWCfgState.entry_order
> arrays dynamically. All three array sizes will be influenced by the new
> field (and device property) FWCfgState.file_slots.
> 
> Make the following changes:
> 
> - Replace the FW_CFG_FILE_SLOTS macro with FW_CFG_FILE_SLOTS_TRAD
>   (traditional count of fw_cfg file slots) in the header file. The value
>   remains 0x10.
> 
> - Replace all uses of FW_CFG_FILE_SLOTS with a helper function called
>   fw_cfg_file_slots(), returning the new property.
> 
> - Eliminate the macro FW_CFG_MAX_ENTRY, and replace all its uses with a
>   helper function called fw_cfg_max_entry().
> 
> - In the MMIO- and IO-mapped realize functions both, allocate all three
>   arrays dynamically, based on the new property.
> 
> - The new property defaults to 0x20; however at the moment we forcibly set
>   it to FW_CFG_FILE_SLOTS_TRAD on all code paths available to board code
>   (namely in the fw_cfg_init_io_dma() and fw_cfg_init_mem_wide() helper
>   functions). This is going to be customized in the following patches.
> 
> Cc: "Gabriel L. Somlo" <[e-mail]>
> Cc: "Michael S. Tsirkin" <[e-mail]>
> Cc: Gerd Hoffmann <[e-mail]>
> Cc: Igor Mammedov <[e-mail]>
> Cc: Paolo Bonzini <[e-mail]>
> Signed-off-by: Laszlo Ersek <[e-mail]>

Reviewed-by: Michael S. Tsirkin <[e-mail]>

> ---
> 
> Notes:
>     I know that upstream doesn't care about backward migration, but some
>     downstreams might.
> 
>  docs/specs/fw_cfg.txt          |  2 +-
>  include/hw/nvram/fw_cfg_keys.h |  3 +-
>  hw/nvram/fw_cfg.c              | 85 ++++++++++++++++++++++++++++++++++++++----
>  3 files changed, 79 insertions(+), 11 deletions(-)
> 
> diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
> index a19e2adbe1c6..84e2978706f5 100644
> --- a/docs/specs/fw_cfg.txt
> +++ b/docs/specs/fw_cfg.txt
> @@ -154,11 +154,11 @@ Selector Reg.    Range Usage
>  0x8000 - 0xbfff  Arch. Specific (0x0000 - 0x3fff, generally RO, possibly RW
>                                   through the DMA interface in QEMU v2.9+)
>  0xc000 - 0xffff  Arch. Specific (0x0000 - 0x3fff, RW, ignored in v2.4+)
>  
>  In practice, the number of allowed firmware configuration items is given
> -by the value of FW_CFG_MAX_ENTRY (see fw_cfg.h).
> +by the value (FW_CFG_FILE_FIRST + FW_CFG_FILE_SLOTS_TRAD) (see fw_cfg.h).
>  
>  = Guest-side DMA Interface =
>  
>  If bit 1 of the feature bitmap is set, the DMA interface is present. This does
>  not replace the existing fw_cfg interface, it is an add-on. This interface
> diff --git a/include/hw/nvram/fw_cfg_keys.h b/include/hw/nvram/fw_cfg_keys.h
> index 0f3e871884c0..627589793671 100644
> --- a/include/hw/nvram/fw_cfg_keys.h
> +++ b/include/hw/nvram/fw_cfg_keys.h
> @@ -27,12 +27,11 @@
>  #define FW_CFG_SETUP_SIZE       0x17
>  #define FW_CFG_SETUP_DATA       0x18
>  #define FW_CFG_FILE_DIR         0x19
>  
>  #define FW_CFG_FILE_FIRST       0x20
> -#define FW_CFG_FILE_SLOTS       0x10
> -#define FW_CFG_MAX_ENTRY        (FW_CFG_FILE_FIRST + FW_CFG_FILE_SLOTS)
> +#define FW_CFG_FILE_SLOTS_TRAD  0x10
>  
>  #define FW_CFG_WRITE_CHANNEL    0x4000
>  #define FW_CFG_ARCH_LOCAL       0x8000
>  #define FW_CFG_ENTRY_MASK       (~(FW_CFG_WRITE_CHANNEL | FW_CFG_ARCH_LOCAL))
>  
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index e0145c11a19b..2e1441c09750 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -31,10 +31,13 @@
>  #include "hw/sysbus.h"
>  #include "trace.h"
>  #include "qemu/error-report.h"
>  #include "qemu/config-file.h"
>  #include "qemu/cutils.h"
> +#include "qapi/error.h"
> +
> +#define FW_CFG_FILE_SLOTS_DFLT 0x20
>  
>  #define FW_CFG_NAME "fw_cfg"
>  #define FW_CFG_PATH "/machine/" FW_CFG_NAME
>  
>  #define TYPE_FW_CFG     "fw_cfg"
> @@ -69,12 +72,13 @@ typedef struct FWCfgEntry {
>  struct FWCfgState {
>      /*< private >*/
>      SysBusDevice parent_obj;
>      /*< public >*/
>  
> -    FWCfgEntry entries[2][FW_CFG_MAX_ENTRY];
> -    int entry_order[FW_CFG_MAX_ENTRY];
> +    uint32_t file_slots;
> +    FWCfgEntry *entries[2];
> +    int *entry_order;
>      FWCfgFiles *files;
>      uint16_t cur_entry;
>      uint32_t cur_offset;
>      Notifier machine_ready;
>  
> @@ -255,17 +259,27 @@ static void fw_cfg_reboot(FWCfgState *s)
>  static void fw_cfg_write(FWCfgState *s, uint8_t value)
>  {
>      /* nothing, write support removed in QEMU v2.4+ */
>  }
>  
> +static inline uint32_t fw_cfg_file_slots(const FWCfgState *s)
> +{
> +    return s->file_slots;
> +}
> +
> +static inline uint32_t fw_cfg_max_entry(const FWCfgState *s)
> +{
> +    return FW_CFG_FILE_FIRST + fw_cfg_file_slots(s);
> +}
> +
>  static int fw_cfg_select(FWCfgState *s, uint16_t key)
>  {
>      int arch, ret;
>      FWCfgEntry *e;
>  
>      s->cur_offset = 0;
> -    if ((key & FW_CFG_ENTRY_MASK) >= FW_CFG_MAX_ENTRY) {
> +    if ((key & FW_CFG_ENTRY_MASK) >= fw_cfg_max_entry(s)) {
>          s->cur_entry = FW_CFG_INVALID;
>          ret = 0;
>      } else {
>          s->cur_entry = key;
>          ret = 1;
> @@ -608,11 +622,11 @@ static void fw_cfg_add_bytes_read_callback(FWCfgState *s, uint16_t key,
>  {
>      int arch = !!(key & FW_CFG_ARCH_LOCAL);
>  
>      key &= FW_CFG_ENTRY_MASK;
>  
> -    assert(key < FW_CFG_MAX_ENTRY && len < UINT32_MAX);
> +    assert(key < fw_cfg_max_entry(s) && len < UINT32_MAX);
>      assert(s->entries[arch][key].data == NULL); /* avoid key conflict */
>  
>      s->entries[arch][key].data = data;
>      s->entries[arch][key].len = (uint32_t)len;
>      s->entries[arch][key].read_callback = callback;
> @@ -626,11 +640,11 @@ static void *fw_cfg_modify_bytes_read(FWCfgState *s, uint16_t key,
>      void *ptr;
>      int arch = !!(key & FW_CFG_ARCH_LOCAL);
>  
>      key &= FW_CFG_ENTRY_MASK;
>  
> -    assert(key < FW_CFG_MAX_ENTRY && len < UINT32_MAX);
> +    assert(key < fw_cfg_max_entry(s) && len < UINT32_MAX);
>  
>      /* return the old data to the function caller, avoid memory leak */
>      ptr = s->entries[arch][key].data;
>      s->entries[arch][key].data = data;
>      s->entries[arch][key].len = len;
> @@ -775,17 +789,17 @@ void fw_cfg_add_file_callback(FWCfgState *s,  const char *filename,
>      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;
> +        dsize = sizeof(uint32_t) + sizeof(FWCfgFile) * fw_cfg_file_slots(s);
>          s->files = g_malloc0(dsize);
>          fw_cfg_add_bytes(s, FW_CFG_FILE_DIR, s->files, dsize);
>      }
>  
>      count = be32_to_cpu(s->files->count);
> -    assert(count < FW_CFG_FILE_SLOTS);
> +    assert(count < fw_cfg_file_slots(s));
>  
>      /* Find the insertion point. */
>      if (mc->legacy_fw_cfg_order) {
>          /*
>           * Sort by order. For files with the same order, we keep them
> @@ -855,11 +869,11 @@ void *fw_cfg_modify_file(FWCfgState *s, const char *filename,
>      void *ptr = NULL;
>  
>      assert(s->files);
>  
>      index = be32_to_cpu(s->files->count);
> -    assert(index < FW_CFG_FILE_SLOTS);
> +    assert(index < fw_cfg_file_slots(s));
>  
>      for (i = 0; i < index; i++) {
>          if (strcmp(filename, s->files->f[i].name) == 0) {
>              ptr = fw_cfg_modify_bytes_read(s, FW_CFG_FILE_FIRST + i,
>                                             data, len);
> @@ -926,10 +940,16 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
>      qdev_prop_set_uint32(dev, "dma_iobase", dma_iobase);
>      if (!dma_requested) {
>          qdev_prop_set_bit(dev, "dma_enabled", false);
>      }
>  
> +    /* Once we expose the "file_slots" property to callers of
> +     * fw_cfg_init_io_dma(), the following setting should become conditional on
> +     * the input parameter being lower than the current value of the property.
> +     */
> +    qdev_prop_set_uint32(dev, "file_slots", FW_CFG_FILE_SLOTS_TRAD);
> +
>      fw_cfg_init1(dev);
>      s = FW_CFG(dev);
>  
>      if (s->dma_enabled) {
>          /* 64 bits for the address field */
> @@ -963,10 +983,17 @@ FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr,
>      qdev_prop_set_uint32(dev, "data_width", data_width);
>      if (!dma_requested) {
>          qdev_prop_set_bit(dev, "dma_enabled", false);
>      }
>  
> +    /* Once we expose the "file_slots" property to callers of
> +     * fw_cfg_init_mem_wide(), the following setting should become conditional
> +     * on the input parameter being lower than the current value of the
> +     * property. Refer to fw_cfg_init_io_dma().
> +     */
> +    qdev_prop_set_uint32(dev, "file_slots", FW_CFG_FILE_SLOTS_TRAD);
> +
>      fw_cfg_init1(dev);
>  
>      sbd = SYS_BUS_DEVICE(dev);
>      sysbus_mmio_map(sbd, 0, ctl_addr);
>      sysbus_mmio_map(sbd, 1, data_addr);
> @@ -1012,23 +1039,56 @@ static const TypeInfo fw_cfg_info = {
>      .abstract      = true,
>      .instance_size = sizeof(FWCfgState),
>      .class_init    = fw_cfg_class_init,
>  };
>  
> +static void fw_cfg_file_slots_allocate(FWCfgState *s, Error **errp)
> +{
> +    uint16_t file_slots_max;
> +
> +    if (fw_cfg_file_slots(s) < FW_CFG_FILE_SLOTS_TRAD) {
> +        error_setg(errp, "\"file_slots\" must be at least 0x%x",
> +                   FW_CFG_FILE_SLOTS_TRAD);
> +        return;
> +    }
> +
> +    /* (UINT16_MAX & FW_CFG_ENTRY_MASK) is the highest inclusive selector value
> +     * that we permit. The actual (exclusive) value coming from the
> +     * configuration is (FW_CFG_FILE_FIRST + fw_cfg_file_slots(s)). */
> +    file_slots_max = (UINT16_MAX & FW_CFG_ENTRY_MASK) - FW_CFG_FILE_FIRST + 1;
> +    if (fw_cfg_file_slots(s) > file_slots_max) {
> +        error_setg(errp, "\"file_slots\" must not exceed 0x%" PRIx16,
> +                   file_slots_max);
> +        return;
> +    }
> +
> +    s->entries[0] = g_new0(FWCfgEntry, fw_cfg_max_entry(s));
> +    s->entries[1] = g_new0(FWCfgEntry, fw_cfg_max_entry(s));
> +    s->entry_order = g_new0(int, fw_cfg_max_entry(s));
> +}
>  
>  static Property fw_cfg_io_properties[] = {
>      DEFINE_PROP_UINT32("iobase", FWCfgIoState, iobase, -1),
>      DEFINE_PROP_UINT32("dma_iobase", FWCfgIoState, dma_iobase, -1),
>      DEFINE_PROP_BOOL("dma_enabled", FWCfgIoState, parent_obj.dma_enabled,
>                       true),
> +    DEFINE_PROP_UINT32("file_slots", FWCfgIoState, parent_obj.file_slots,
> +                       FW_CFG_FILE_SLOTS_DFLT),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
>  static void fw_cfg_io_realize(DeviceState *dev, Error **errp)
>  {
>      FWCfgIoState *s = FW_CFG_IO(dev);
>      SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> +    Error *local_err = NULL;
> +
> +    fw_cfg_file_slots_allocate(FW_CFG(s), &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  
>      /* when using port i/o, the 8-bit data register ALWAYS overlaps
>       * with half of the 16-bit control register. Hence, the total size
>       * of the i/o region used is FW_CFG_CTL_SIZE */
>      memory_region_init_io(&s->comb_iomem, OBJECT(s), &fw_cfg_comb_mem_ops,
> @@ -1061,18 +1121,27 @@ static const TypeInfo fw_cfg_io_info = {
>  
>  static Property fw_cfg_mem_properties[] = {
>      DEFINE_PROP_UINT32("data_width", FWCfgMemState, data_width, -1),
>      DEFINE_PROP_BOOL("dma_enabled", FWCfgMemState, parent_obj.dma_enabled,
>                       true),
> +    DEFINE_PROP_UINT32("file_slots", FWCfgMemState, parent_obj.file_slots,
> +                       FW_CFG_FILE_SLOTS_DFLT),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
>  static void fw_cfg_mem_realize(DeviceState *dev, Error **errp)
>  {
>      FWCfgMemState *s = FW_CFG_MEM(dev);
>      SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>      const MemoryRegionOps *data_ops = &fw_cfg_data_mem_ops;
> +    Error *local_err = NULL;
> +
> +    fw_cfg_file_slots_allocate(FW_CFG(s), &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>  
>      memory_region_init_io(&s->ctl_iomem, OBJECT(s), &fw_cfg_ctl_mem_ops,
>                            FW_CFG(s), "fwcfg.ctl", FW_CFG_CTL_SIZE);
>      sysbus_init_mmio(sbd, &s->ctl_iomem);
>  
> -- 
> 2.9.2
>

Patch

diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt
index a19e2adbe1c6..84e2978706f5 100644
--- a/docs/specs/fw_cfg.txt
+++ b/docs/specs/fw_cfg.txt
@@ -154,11 +154,11 @@  Selector Reg.    Range Usage
 0x8000 - 0xbfff  Arch. Specific (0x0000 - 0x3fff, generally RO, possibly RW
                                  through the DMA interface in QEMU v2.9+)
 0xc000 - 0xffff  Arch. Specific (0x0000 - 0x3fff, RW, ignored in v2.4+)
 
 In practice, the number of allowed firmware configuration items is given
-by the value of FW_CFG_MAX_ENTRY (see fw_cfg.h).
+by the value (FW_CFG_FILE_FIRST + FW_CFG_FILE_SLOTS_TRAD) (see fw_cfg.h).
 
 = Guest-side DMA Interface =
 
 If bit 1 of the feature bitmap is set, the DMA interface is present. This does
 not replace the existing fw_cfg interface, it is an add-on. This interface
diff --git a/include/hw/nvram/fw_cfg_keys.h b/include/hw/nvram/fw_cfg_keys.h
index 0f3e871884c0..627589793671 100644
--- a/include/hw/nvram/fw_cfg_keys.h
+++ b/include/hw/nvram/fw_cfg_keys.h
@@ -27,12 +27,11 @@ 
 #define FW_CFG_SETUP_SIZE       0x17
 #define FW_CFG_SETUP_DATA       0x18
 #define FW_CFG_FILE_DIR         0x19
 
 #define FW_CFG_FILE_FIRST       0x20
-#define FW_CFG_FILE_SLOTS       0x10
-#define FW_CFG_MAX_ENTRY        (FW_CFG_FILE_FIRST + FW_CFG_FILE_SLOTS)
+#define FW_CFG_FILE_SLOTS_TRAD  0x10
 
 #define FW_CFG_WRITE_CHANNEL    0x4000
 #define FW_CFG_ARCH_LOCAL       0x8000
 #define FW_CFG_ENTRY_MASK       (~(FW_CFG_WRITE_CHANNEL | FW_CFG_ARCH_LOCAL))
 
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index e0145c11a19b..2e1441c09750 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -31,10 +31,13 @@ 
 #include "hw/sysbus.h"
 #include "trace.h"
 #include "qemu/error-report.h"
 #include "qemu/config-file.h"
 #include "qemu/cutils.h"
+#include "qapi/error.h"
+
+#define FW_CFG_FILE_SLOTS_DFLT 0x20
 
 #define FW_CFG_NAME "fw_cfg"
 #define FW_CFG_PATH "/machine/" FW_CFG_NAME
 
 #define TYPE_FW_CFG     "fw_cfg"
@@ -69,12 +72,13 @@  typedef struct FWCfgEntry {
 struct FWCfgState {
     /*< private >*/
     SysBusDevice parent_obj;
     /*< public >*/
 
-    FWCfgEntry entries[2][FW_CFG_MAX_ENTRY];
-    int entry_order[FW_CFG_MAX_ENTRY];
+    uint32_t file_slots;
+    FWCfgEntry *entries[2];
+    int *entry_order;
     FWCfgFiles *files;
     uint16_t cur_entry;
     uint32_t cur_offset;
     Notifier machine_ready;
 
@@ -255,17 +259,27 @@  static void fw_cfg_reboot(FWCfgState *s)
 static void fw_cfg_write(FWCfgState *s, uint8_t value)
 {
     /* nothing, write support removed in QEMU v2.4+ */
 }
 
+static inline uint32_t fw_cfg_file_slots(const FWCfgState *s)
+{
+    return s->file_slots;
+}
+
+static inline uint32_t fw_cfg_max_entry(const FWCfgState *s)
+{
+    return FW_CFG_FILE_FIRST + fw_cfg_file_slots(s);
+}
+
 static int fw_cfg_select(FWCfgState *s, uint16_t key)
 {
     int arch, ret;
     FWCfgEntry *e;
 
     s->cur_offset = 0;
-    if ((key & FW_CFG_ENTRY_MASK) >= FW_CFG_MAX_ENTRY) {
+    if ((key & FW_CFG_ENTRY_MASK) >= fw_cfg_max_entry(s)) {
         s->cur_entry = FW_CFG_INVALID;
         ret = 0;
     } else {
         s->cur_entry = key;
         ret = 1;
@@ -608,11 +622,11 @@  static void fw_cfg_add_bytes_read_callback(FWCfgState *s, uint16_t key,
 {
     int arch = !!(key & FW_CFG_ARCH_LOCAL);
 
     key &= FW_CFG_ENTRY_MASK;
 
-    assert(key < FW_CFG_MAX_ENTRY && len < UINT32_MAX);
+    assert(key < fw_cfg_max_entry(s) && len < UINT32_MAX);
     assert(s->entries[arch][key].data == NULL); /* avoid key conflict */
 
     s->entries[arch][key].data = data;
     s->entries[arch][key].len = (uint32_t)len;
     s->entries[arch][key].read_callback = callback;
@@ -626,11 +640,11 @@  static void *fw_cfg_modify_bytes_read(FWCfgState *s, uint16_t key,
     void *ptr;
     int arch = !!(key & FW_CFG_ARCH_LOCAL);
 
     key &= FW_CFG_ENTRY_MASK;
 
-    assert(key < FW_CFG_MAX_ENTRY && len < UINT32_MAX);
+    assert(key < fw_cfg_max_entry(s) && len < UINT32_MAX);
 
     /* return the old data to the function caller, avoid memory leak */
     ptr = s->entries[arch][key].data;
     s->entries[arch][key].data = data;
     s->entries[arch][key].len = len;
@@ -775,17 +789,17 @@  void fw_cfg_add_file_callback(FWCfgState *s,  const char *filename,
     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;
+        dsize = sizeof(uint32_t) + sizeof(FWCfgFile) * fw_cfg_file_slots(s);
         s->files = g_malloc0(dsize);
         fw_cfg_add_bytes(s, FW_CFG_FILE_DIR, s->files, dsize);
     }
 
     count = be32_to_cpu(s->files->count);
-    assert(count < FW_CFG_FILE_SLOTS);
+    assert(count < fw_cfg_file_slots(s));
 
     /* Find the insertion point. */
     if (mc->legacy_fw_cfg_order) {
         /*
          * Sort by order. For files with the same order, we keep them
@@ -855,11 +869,11 @@  void *fw_cfg_modify_file(FWCfgState *s, const char *filename,
     void *ptr = NULL;
 
     assert(s->files);
 
     index = be32_to_cpu(s->files->count);
-    assert(index < FW_CFG_FILE_SLOTS);
+    assert(index < fw_cfg_file_slots(s));
 
     for (i = 0; i < index; i++) {
         if (strcmp(filename, s->files->f[i].name) == 0) {
             ptr = fw_cfg_modify_bytes_read(s, FW_CFG_FILE_FIRST + i,
                                            data, len);
@@ -926,10 +940,16 @@  FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t dma_iobase,
     qdev_prop_set_uint32(dev, "dma_iobase", dma_iobase);
     if (!dma_requested) {
         qdev_prop_set_bit(dev, "dma_enabled", false);
     }
 
+    /* Once we expose the "file_slots" property to callers of
+     * fw_cfg_init_io_dma(), the following setting should become conditional on
+     * the input parameter being lower than the current value of the property.
+     */
+    qdev_prop_set_uint32(dev, "file_slots", FW_CFG_FILE_SLOTS_TRAD);
+
     fw_cfg_init1(dev);
     s = FW_CFG(dev);
 
     if (s->dma_enabled) {
         /* 64 bits for the address field */
@@ -963,10 +983,17 @@  FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr,
     qdev_prop_set_uint32(dev, "data_width", data_width);
     if (!dma_requested) {
         qdev_prop_set_bit(dev, "dma_enabled", false);
     }
 
+    /* Once we expose the "file_slots" property to callers of
+     * fw_cfg_init_mem_wide(), the following setting should become conditional
+     * on the input parameter being lower than the current value of the
+     * property. Refer to fw_cfg_init_io_dma().
+     */
+    qdev_prop_set_uint32(dev, "file_slots", FW_CFG_FILE_SLOTS_TRAD);
+
     fw_cfg_init1(dev);
 
     sbd = SYS_BUS_DEVICE(dev);
     sysbus_mmio_map(sbd, 0, ctl_addr);
     sysbus_mmio_map(sbd, 1, data_addr);
@@ -1012,23 +1039,56 @@  static const TypeInfo fw_cfg_info = {
     .abstract      = true,
     .instance_size = sizeof(FWCfgState),
     .class_init    = fw_cfg_class_init,
 };
 
+static void fw_cfg_file_slots_allocate(FWCfgState *s, Error **errp)
+{
+    uint16_t file_slots_max;
+
+    if (fw_cfg_file_slots(s) < FW_CFG_FILE_SLOTS_TRAD) {
+        error_setg(errp, "\"file_slots\" must be at least 0x%x",
+                   FW_CFG_FILE_SLOTS_TRAD);
+        return;
+    }
+
+    /* (UINT16_MAX & FW_CFG_ENTRY_MASK) is the highest inclusive selector value
+     * that we permit. The actual (exclusive) value coming from the
+     * configuration is (FW_CFG_FILE_FIRST + fw_cfg_file_slots(s)). */
+    file_slots_max = (UINT16_MAX & FW_CFG_ENTRY_MASK) - FW_CFG_FILE_FIRST + 1;
+    if (fw_cfg_file_slots(s) > file_slots_max) {
+        error_setg(errp, "\"file_slots\" must not exceed 0x%" PRIx16,
+                   file_slots_max);
+        return;
+    }
+
+    s->entries[0] = g_new0(FWCfgEntry, fw_cfg_max_entry(s));
+    s->entries[1] = g_new0(FWCfgEntry, fw_cfg_max_entry(s));
+    s->entry_order = g_new0(int, fw_cfg_max_entry(s));
+}
 
 static Property fw_cfg_io_properties[] = {
     DEFINE_PROP_UINT32("iobase", FWCfgIoState, iobase, -1),
     DEFINE_PROP_UINT32("dma_iobase", FWCfgIoState, dma_iobase, -1),
     DEFINE_PROP_BOOL("dma_enabled", FWCfgIoState, parent_obj.dma_enabled,
                      true),
+    DEFINE_PROP_UINT32("file_slots", FWCfgIoState, parent_obj.file_slots,
+                       FW_CFG_FILE_SLOTS_DFLT),
     DEFINE_PROP_END_OF_LIST(),
 };
 
 static void fw_cfg_io_realize(DeviceState *dev, Error **errp)
 {
     FWCfgIoState *s = FW_CFG_IO(dev);
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+    Error *local_err = NULL;
+
+    fw_cfg_file_slots_allocate(FW_CFG(s), &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
 
     /* when using port i/o, the 8-bit data register ALWAYS overlaps
      * with half of the 16-bit control register. Hence, the total size
      * of the i/o region used is FW_CFG_CTL_SIZE */
     memory_region_init_io(&s->comb_iomem, OBJECT(s), &fw_cfg_comb_mem_ops,
@@ -1061,18 +1121,27 @@  static const TypeInfo fw_cfg_io_info = {
 
 static Property fw_cfg_mem_properties[] = {
     DEFINE_PROP_UINT32("data_width", FWCfgMemState, data_width, -1),
     DEFINE_PROP_BOOL("dma_enabled", FWCfgMemState, parent_obj.dma_enabled,
                      true),
+    DEFINE_PROP_UINT32("file_slots", FWCfgMemState, parent_obj.file_slots,
+                       FW_CFG_FILE_SLOTS_DFLT),
     DEFINE_PROP_END_OF_LIST(),
 };
 
 static void fw_cfg_mem_realize(DeviceState *dev, Error **errp)
 {
     FWCfgMemState *s = FW_CFG_MEM(dev);
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
     const MemoryRegionOps *data_ops = &fw_cfg_data_mem_ops;
+    Error *local_err = NULL;
+
+    fw_cfg_file_slots_allocate(FW_CFG(s), &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
 
     memory_region_init_io(&s->ctl_iomem, OBJECT(s), &fw_cfg_ctl_mem_ops,
                           FW_CFG(s), "fwcfg.ctl", FW_CFG_CTL_SIZE);
     sysbus_init_mmio(sbd, &s->ctl_iomem);