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

[Laszlo Ersek <lersek@redhat.com>]

On 12/16/14 13:42, Laszlo Ersek wrote:
> 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 <lersek@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).

To elaborate on the above -- the fw_cfg device appears to be
undestructible at the moment. It has no unrealize callback. If it were
destructible, then the above leak would be the smallest of concerns --
it doesn't unmap nor destroy the memory regions that implement the
various registers.

So, I think the above is not an actual leak, because the result of
g_memdup() can never become unreferenced.

Thanks,
Laszlo