| Message ID | 1418399932-7658-4-git-send-email-lersek@redhat.com |
|---|---|
| State | New |
| Headers | show |
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? I see 2 options around this: 1) Free it on destruction 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 :). Alex
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). Thanks Laszlo
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
On 16 December 2014 at 16:59, Laszlo Ersek <lersek@redhat.com> wrote: > 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. True, and we have a lot of device that are in this same category of "can't ever be destroyed". However it is setting up a minor beartrap for ourselves in future if we have allocations which aren't tracked via a field in the device's state structure, because the obvious future implementation of destruction for a device is "just free/destroy everything that is in the state struct". NB: I think what Alex had in mind with his option (2) was just to have a "MemoryRegionOps ops;" field in the state struct, and then use "s->ops = data_mem_ops;" rather than the memdup. That retains the use of the static field for the non-variable-width case, it's just that instead of allocating off the heap for the var-width setup we use an inline lump of memory in a struct we're already allocing. I don't think I care very much about this, but Alex's suggestion 2 is slightly nicer I guess. Adding a whole unrealize callback is definitely vastly overkill. -- PMM
On 12/16/14 18:10, Peter Maydell wrote: > On 16 December 2014 at 16:59, Laszlo Ersek <lersek@redhat.com> wrote: >> 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. > True, and we have a lot of device that are in this same > category of "can't ever be destroyed". However it is setting > up a minor beartrap for ourselves in future if we have > allocations which aren't tracked via a field in the device's > state structure, because the obvious future implementation of > destruction for a device is "just free/destroy everything > that is in the state struct". > > NB: I think what Alex had in mind with his option (2) was > just to have a "MemoryRegionOps ops;" field in the state > struct, and then use "s->ops = data_mem_ops;" rather than > the memdup. That retains the use of the static field for > the non-variable-width case, it's just that instead of > allocating off the heap for the var-width setup we use > an inline lump of memory in a struct we're already allocing. > > I don't think I care very much about this, but Alex's > suggestion 2 is slightly nicer I guess. Adding a whole > unrealize callback is definitely vastly overkill. Yeah, it's exactly what I meant. Sorry for not being as clear. By moving the dynamically created struct into the device struct we're just making the whole allocation flow easier. But if this is the only nitpick, there's no need for a respin just for that. Alex
On 12/16/14 18:20, Alexander Graf wrote: > On 12/16/14 18:10, Peter Maydell wrote: >> On 16 December 2014 at 16:59, Laszlo Ersek <lersek@redhat.com> wrote: >>> 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. >> True, and we have a lot of device that are in this same >> category of "can't ever be destroyed". However it is setting >> up a minor beartrap for ourselves in future if we have >> allocations which aren't tracked via a field in the device's >> state structure, because the obvious future implementation of >> destruction for a device is "just free/destroy everything >> that is in the state struct". >> >> NB: I think what Alex had in mind with his option (2) was >> just to have a "MemoryRegionOps ops;" field in the state >> struct, and then use "s->ops = data_mem_ops;" rather than >> the memdup. That retains the use of the static field for >> the non-variable-width case, it's just that instead of >> allocating off the heap for the var-width setup we use >> an inline lump of memory in a struct we're already allocing. >> >> I don't think I care very much about this, but Alex's >> suggestion 2 is slightly nicer I guess. Adding a whole >> unrealize callback is definitely vastly overkill. > > Yeah, it's exactly what I meant. Sorry for not being as clear. By moving > the dynamically created struct into the device struct we're just making > the whole allocation flow easier. > > But if this is the only nitpick, there's no need for a respin just for > that. Okay, I understand it now. Thanks. Laszlo
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)); + 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); + if (s->ctl_iobase == 0 && s->data_iobase == 0) { return; } ctl_io_last = s->ctl_iobase + FW_CFG_SIZE - 1; - data_io_end = s->data_iobase + fw_cfg_data_mem_ops.valid.max_access_size; + data_io_end = s->data_iobase + data_mem_ops->valid.max_access_size; if (ctl_io_last >= s->data_iobase && ctl_io_last < data_io_end) { - if (fw_cfg_data_mem_ops.valid.max_access_size == 1) { + if (data_mem_ops->valid.max_access_size == 1) { sysbus_add_io(sbd, s->ctl_iobase, &s->comb_iomem); } else { error_setg(errp, "only a byte-wide data I/O port can be combined"); return; @@ -649,8 +660,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(), }; FWCfgState *fw_cfg_find(void)
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(-)