Re: [Qemu-devel] [PATCH v3 2/7] fw_cfg: introduce the "data_memwidth" property

[Peter Maydell]

On 9 December 2014 at 01:13, Laszlo Ersek <address@hidden> 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>

Mostly looks good to me. A few nits:

> +    qdev_prop_set_uint32(dev, "data_memwidth",
> +                         fw_cfg_data_mem_ops.valid.max_access_size);

Why not just make this the default value of the property, rather
than setting the default value to -1 and always manually overriding it?

> @@ -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,9 +619,20 @@ 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;
>
> +    if (s->data_memwidth > data_mem_ops->valid.max_access_size) {
> +        MemoryRegionOps *ops;
> +
> +        ops = g_memdup(data_mem_ops, sizeof(*data_mem_ops));
> +        ops->valid.max_access_size = s->data_memwidth;
> +        data_mem_ops = ops;
> +    }
> +    memory_region_init_io(&s->data_iomem, OBJECT(s), data_mem_ops, s,
> +                          "fwcfg.data", data_mem_ops->valid.max_access_size);
> +    sysbus_init_mmio(sbd, &s->data_iomem);

The property changes the width of the data port, but only in
the case where it's not combined with the control port
(there the data width remains always 1). Is it worth throwing
an error in realize if the caller tried to set data_memwidth
and also use the combined-port? (Possibly even if the caller
set data_memwidth and tried to use data_iobase at all? Does
it make sense to define an AWAP I/O port ?)

>
>      if (s->ctl_iobase + 1 == s->data_iobase) {
>          sysbus_add_io(sbd, s->ctl_iobase, &s->comb_iomem);
>      } else {
> @@ -637,8 +647,9 @@ static void fw_cfg_realize(DeviceState *dev, Error **errp)
>
>  static Property fw_cfg_properties[] = {
>      DEFINE_PROP_UINT32("ctl_iobase", FWCfgState, ctl_iobase, -1),
>      DEFINE_PROP_UINT32("data_iobase", FWCfgState, data_iobase, -1),
> +    DEFINE_PROP_UINT32("data_memwidth", FWCfgState, data_memwidth, -1),
>      DEFINE_PROP_END_OF_LIST(),
>  };

thanks
-- PMM