qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v4 2/7] fw-cfg: turn FW_CFG_FILE_SLOTS into a de


From: Igor Mammedov
Subject: Re: [Qemu-devel] [PATCH v4 2/7] fw-cfg: turn FW_CFG_FILE_SLOTS into a device property
Date: Tue, 6 Dec 2016 11:50:38 +0100

On Thu,  1 Dec 2016 18:06:19 +0100
Laszlo Ersek <address@hidden> 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" <address@hidden>
> Cc: "Michael S. Tsirkin" <address@hidden>
> Cc: Gerd Hoffmann <address@hidden>
> Cc: Igor Mammedov <address@hidden>
> Cc: Paolo Bonzini <address@hidden>
> Signed-off-by: Laszlo Ersek <address@hidden>
> ---
> 
> 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);
>  




reply via email to

[Prev in Thread] Current Thread [Next in Thread]