On 12/12/14 13:49, Peter Maydell wrote:
> On 9 December 2014 at 01:13, Laszlo Ersek <ler...@redhat.com> 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 <ler...@redhat.com>
>
> 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
>
