Re: [Qemu-devel] [PATCH v4 3/8] fw_cfg: introduce the "data_memwidth" property

[Alexander Graf]

On 12.12.14 16:58, Laszlo Ersek wrote:
> The "data_memwidth" property is capable of changing the maximum valid
> access size to the MMIO data register, and (corresponding to the previous
> patch) resizes the memory region similarly, at device realization time.
> 
> (Because "data_iomem" is configured and installed dynamically now, we must
> delay those steps to the realize callback.)
> 
> The default value of "data_memwidth" is set so that we don't yet diverge
> from "fw_cfg_data_mem_ops".
> 
> Most of the fw_cfg users will stick with the default, and for them we
> should continue using the statically allocated "fw_cfg_data_mem_ops". This
> is beneficial for debugging because gdb can resolve pointers referencing
> static objects to the names of those objects.
> 
> Signed-off-by: Laszlo Ersek <address@hidden>
> ---
> 
> Notes:
>     v4:
>     - reject I/O port combining if data register is wider than 1 byte
>       [Peter]
>     
>     v3:
>     - new in v3 [Drew Jones]
> 
>  hw/nvram/fw_cfg.c | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index eb0ad83..0947136 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -50,8 +50,9 @@ struct FWCfgState {
>      /*< public >*/
>  
>      MemoryRegion ctl_iomem, data_iomem, comb_iomem;
>      uint32_t ctl_iobase, data_iobase;
> +    uint32_t data_memwidth;
>      FWCfgEntry entries[2][FW_CFG_MAX_ENTRY];
>      FWCfgFiles *files;
>      uint16_t cur_entry;
>      uint32_t cur_offset;
> @@ -569,8 +570,10 @@ FWCfgState *fw_cfg_init(uint32_t ctl_port, uint32_t 
> data_port,
>  
>      dev = qdev_create(NULL, TYPE_FW_CFG);
>      qdev_prop_set_uint32(dev, "ctl_iobase", ctl_port);
>      qdev_prop_set_uint32(dev, "data_iobase", data_port);
> +    qdev_prop_set_uint32(dev, "data_memwidth",
> +                         fw_cfg_data_mem_ops.valid.max_access_size);
>      d = SYS_BUS_DEVICE(dev);
>  
>      s = FW_CFG(dev);
>  
> @@ -607,12 +610,8 @@ static void fw_cfg_initfn(Object *obj)
>  
>      memory_region_init_io(&s->ctl_iomem, OBJECT(s), &fw_cfg_ctl_mem_ops, s,
>                            "fwcfg.ctl", FW_CFG_SIZE);
>      sysbus_init_mmio(sbd, &s->ctl_iomem);
> -    memory_region_init_io(&s->data_iomem, OBJECT(s), &fw_cfg_data_mem_ops, s,
> -                          "fwcfg.data",
> -                          fw_cfg_data_mem_ops.valid.max_access_size);
> -    sysbus_init_mmio(sbd, &s->data_iomem);
>      /* In case ctl and data overlap: */
>      memory_region_init_io(&s->comb_iomem, OBJECT(s), &fw_cfg_comb_mem_ops, s,
>                            "fwcfg", FW_CFG_SIZE);
>  }
> @@ -620,19 +619,31 @@ static void fw_cfg_initfn(Object *obj)
>  static void fw_cfg_realize(DeviceState *dev, Error **errp)
>  {
>      FWCfgState *s = FW_CFG(dev);
>      SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> +    const MemoryRegionOps *data_mem_ops = &fw_cfg_data_mem_ops;
>      uint32_t ctl_io_last;
>      uint32_t data_io_end;
>  
> +    if (s->data_memwidth > data_mem_ops->valid.max_access_size) {
> +        MemoryRegionOps *ops;
> +
> +        ops = g_memdup(data_mem_ops, sizeof(*data_mem_ops));

Hrm, this memory will leak if the device gets destroyed after realize,
right?

I see 2 options around this:

  1) Free it on destruction
  2) Add the RegionOps as field into FWCfgState. Then it gets allocated
and free'd automatically

Option 2 is easier (and more failure proof) but will waste a few bytes
of ram for data_memwidth=1 users. I don't think we need to bother about
the few bytes and rather go with safety :).


Alex