On 06/12/17 23:21, Mark Cave-Ayland wrote:
> As indicated by Laszlo it is a QOM bug for the realize() method to actually
> map the device. Set up the IO regions with sysbus_init_mmio() and defer
> the mapping to the caller, as already done in fw_cfg_init_mem_wide().
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk>
> ---
> hw/nvram/fw_cfg.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 316fca9..be5b04e 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -936,6 +936,7 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t
> dma_iobase,
> AddressSpace *dma_as)
> {
> DeviceState *dev;
> + SysBusDevice *sbd;
> FWCfgState *s;
> uint32_t version = FW_CFG_VERSION;
> bool dma_requested = dma_iobase && dma_as;
> @@ -948,12 +949,17 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase,
> uint32_t dma_iobase,
> }
>
> fw_cfg_init1(dev);
> +
> + sbd = SYS_BUS_DEVICE(dev);
> + sysbus_add_io(sbd, iobase, sysbus_mmio_get_region(sbd, 0));
> +
> s = FW_CFG(dev);
>
> if (s->dma_enabled) {
> /* 64 bits for the address field */
> s->dma_as = dma_as;
> s->dma_addr = 0;
> + sysbus_add_io(sbd, dma_iobase, sysbus_mmio_get_region(sbd, 1));
>
> version |= FW_CFG_VERSION_DMA;
> }
> @@ -1085,13 +1091,13 @@ static void fw_cfg_io_realize(DeviceState *dev, Error
> **errp)
> * of the i/o region used is FW_CFG_CTL_SIZE */
> memory_region_init_io(&s->comb_iomem, OBJECT(s), &fw_cfg_comb_mem_ops,
> FW_CFG(s), "fwcfg", FW_CFG_CTL_SIZE);
> - sysbus_add_io(sbd, s->iobase, &s->comb_iomem);
> + sysbus_init_mmio(sbd, &s->comb_iomem);
>
> if (FW_CFG(s)->dma_enabled) {
> memory_region_init_io(&FW_CFG(s)->dma_iomem, OBJECT(s),
> &fw_cfg_dma_mem_ops, FW_CFG(s), "fwcfg.dma",
> sizeof(dma_addr_t));
> - sysbus_add_io(sbd, s->dma_iobase, &FW_CFG(s)->dma_iomem);
> + sysbus_init_mmio(sbd, &FW_CFG(s)->dma_iomem);
> }
> }
>
>
This does mostly what I had in mind, but why are the sysbus_init_mmio()
"replacements" necessary?
This is what sysbus_init_mmio() does:
assert(dev->num_mmio < QDEV_MAX_MMIO);
n = dev->num_mmio++;
dev->mmio[n].addr = -1;
dev->mmio[n].memory = memory;
But we don't have MMIO regions here, we have IO ports. This is all what
sysbus_add_io() does:
memory_region_add_subregion(get_system_io(), addr, mem);
It doesn't do anything related to SysBusDevice.mmio[] and mmio.num_mmio.
This patch, as written, changes "num_mmio" from zero to nonzero, and
that could have a bunch of unexpected consequences for
"hw/core/sysbus.c":
- sysbus_has_mmio() would return true
- sysbus_dev_print() produces different output
- sysbus_get_fw_dev_path() formats a different OpenFW device path
fragment
Instead, can we just move the sysbus_add_io() function calls *without*
replacements in fw_cfg_io_realize()?
In fw_cfg_init_io_dma(), you know the object type exactly -- you just
created it with TYPE_FW_CFG_IO --, so after device realization, you can
cast the type as narrowly as necessary, and refer to the fields by name.
Something like (build-tested only):
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 316fca9bc1da..a28ce1eacd63 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -96,7 +96,6 @@ struct FWCfgIoState {
> /*< public >*/
>
> MemoryRegion comb_iomem;
> - uint32_t iobase, dma_iobase;
> };
>
> struct FWCfgMemState {
> @@ -936,24 +935,29 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase,
> uint32_t dma_iobase,
> AddressSpace *dma_as)
> {
> DeviceState *dev;
> + SysBusDevice *sbd;
> FWCfgState *s;
> + FWCfgIoState *ios;
> uint32_t version = FW_CFG_VERSION;
> bool dma_requested = dma_iobase && dma_as;
>
> dev = qdev_create(NULL, TYPE_FW_CFG_IO);
> - qdev_prop_set_uint32(dev, "iobase", iobase);
> - qdev_prop_set_uint32(dev, "dma_iobase", dma_iobase);
> if (!dma_requested) {
> qdev_prop_set_bit(dev, "dma_enabled", false);
> }
>
> fw_cfg_init1(dev);
> + sbd = SYS_BUS_DEVICE(dev);
> s = FW_CFG(dev);
> + ios = FW_CFG_IO(dev);
> +
> + sysbus_add_io(sbd, iobase, &ios->comb_iomem);
>
> if (s->dma_enabled) {
> /* 64 bits for the address field */
> s->dma_as = dma_as;
> s->dma_addr = 0;
> + sysbus_add_io(sbd, dma_iobase, &s->dma_iomem);
>
> version |= FW_CFG_VERSION_DMA;
> }
> @@ -1059,8 +1063,6 @@ static void fw_cfg_file_slots_allocate(FWCfgState *s,
> Error **errp)
> }
>
> static Property fw_cfg_io_properties[] = {
> - DEFINE_PROP_UINT32("iobase", FWCfgIoState, iobase, -1),
> - DEFINE_PROP_UINT32("dma_iobase", FWCfgIoState, dma_iobase, -1),
> DEFINE_PROP_BOOL("dma_enabled", FWCfgIoState, parent_obj.dma_enabled,
> true),
> DEFINE_PROP_UINT16("x-file-slots", FWCfgIoState, parent_obj.file_slots,
> @@ -1071,7 +1073,6 @@ static Property fw_cfg_io_properties[] = {
> static void fw_cfg_io_realize(DeviceState *dev, Error **errp)
> {
> FWCfgIoState *s = FW_CFG_IO(dev);
> - SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> Error *local_err = NULL;
>
> fw_cfg_file_slots_allocate(FW_CFG(s), &local_err);
> @@ -1085,13 +1086,11 @@ static void fw_cfg_io_realize(DeviceState *dev, Error
> **errp)
> * of the i/o region used is FW_CFG_CTL_SIZE */
> memory_region_init_io(&s->comb_iomem, OBJECT(s), &fw_cfg_comb_mem_ops,
> FW_CFG(s), "fwcfg", FW_CFG_CTL_SIZE);
> - sysbus_add_io(sbd, s->iobase, &s->comb_iomem);
>
> if (FW_CFG(s)->dma_enabled) {
> memory_region_init_io(&FW_CFG(s)->dma_iomem, OBJECT(s),
> &fw_cfg_dma_mem_ops, FW_CFG(s), "fwcfg.dma",
> sizeof(dma_addr_t));
> - sysbus_add_io(sbd, s->dma_iobase, &FW_CFG(s)->dma_iomem);
> }
> }
>
It turns out that we introduced the "iobase" and "dma_iobase" properties
*solely* so that we could pass arguments to fw_cfg_io_realize(). But
fw_cfg_io_realize() only needed those parameters for the *wrong* purpose
(namely calling sysbus_add_io()). By eliminating the sysbus_add_io()
calls from fw_cfg_io_realize(), the related properties become
unnecessary too.
(NB, setting the "dma_enabled" property in fw_cfg_init_io_dma(), and
setting "data_width" and "dma_enabled" in fw_cfg_init_mem_wide(), remain
necessary, because those aren't related to region mapping.)
What do you think?
Thanks!
Laszlo
