| Message ID | 1418087585-27601-3-git-send-email-lersek@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 9 December 2014 at 01:13, Laszlo Ersek <lersek@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 <lersek@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? > @@ -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 ?) > > 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
On 12/12/14 13:49, Peter Maydell wrote: > On 9 December 2014 at 01:13, Laszlo Ersek <lersek@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 <lersek@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 >
On 12 December 2014 at 13:39, Laszlo Ersek <lersek@redhat.com> wrote: > 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. Yeah, I don't think we need to make the combined case work, but it does seem worth at least making it fail cleanly if anybody tries it, rather than silently doing the wrong thing. -- PMM
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c index 23ea0fe..6073f16 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,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); 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(), }; 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: v3: - new in v3 [Drew Jones] hw/nvram/fw_cfg.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-)