On 12/16/14 13:06, Alexander Graf wrote:
>
>
> 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 <ler...@redhat.com>
>> ---
>>
>> 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?
How do you destroy the fw_cfg device after it is successfully realized?
I wouldn't introduce such a blatant leak out of oversight.
> I see 2 options around this:
>
> 1) Free it on destruction
Does that mean an unrealize callback?
> 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 :).
I wanted to keep the static ops object for the common user, because it
is very convenient when debugging in gdb -- the address is automatically
resolved to the name of the static object. I guess I can do (1) (if that
means an unrealize callback).
Thanks
Laszlo
