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

[Laszlo Ersek]

On 12/12/14 13:49, Peter Maydell wrote:
> 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?

This hunk just prepares the ground for the next patch, where the
property will be set from a new function parameter. Ultimately I wanted
to leave the default value of the property at -1, for consistency with
the other two properties.

> 
>> @@ -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 ?)

I considered "locking down" the new interface, and preventing callers
from passing in any IO ports when they want a wider MMIO data register.
I didn't do that because someone might want a wider ioport mapping at
some point (although no current such user exists and I couldn't name
what the advantage would be in it). Unless you use the combined thing,
the wide data register should work with the ioport mapping too.

The combined case I thought to leave simply undefined.

If you want, I can set an error, but then I'd prefer to prevent callers
from passing IO ports through the new data_memwidth-taking functions.

Thanks
Laszlo

> 
>>
>>      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
>