[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCHv2 3/4] fw_cfg: move qdev_init_nofail() from fw_c
From: |
Laszlo Ersek |
Subject: |
Re: [Qemu-devel] [PATCHv2 3/4] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers |
Date: |
Tue, 13 Jun 2017 00:50:48 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.1 |
On 06/12/17 23:21, Mark Cave-Ayland wrote:
> When looking to instantiate a TYPE_FW_CFG_MEM or TYPE_FW_CFG_IO device to be
> able to wire it up differently, it is much more convenient for the caller to
> instantiate the device and have the fw_cfg default files already preloaded
> during realize.
>
> Move fw_cfg_init1() to the end of both the fw_cfg_mem_realize() and
> fw_cfg_io_realize() functions so it no longer needs to be called manually
> when instantiating the device.
>
> Signed-off-by: Mark Cave-Ayland <address@hidden>
> ---
> hw/nvram/fw_cfg.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index e1aa4fc..6c21e43 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -920,8 +920,6 @@ static void fw_cfg_init1(DeviceState *dev)
>
> object_property_add_child(OBJECT(machine), FW_CFG_NAME, OBJECT(s), NULL);
>
> - qdev_init_nofail(dev);
> -
> fw_cfg_add_bytes(s, FW_CFG_SIGNATURE, (char *)"QEMU", 4);
> fw_cfg_add_bytes(s, FW_CFG_UUID, &qemu_uuid, 16);
> fw_cfg_add_i16(s, FW_CFG_NOGRAPHIC, (uint16_t)!machine->enable_graphics);
> @@ -954,7 +952,7 @@ FWCfgState *fw_cfg_init_io_dma(uint32_t iobase, uint32_t
> dma_iobase,
> qdev_prop_set_bit(dev, "dma_enabled", false);
> }
>
> - fw_cfg_init1(dev);
> + qdev_init_nofail(dev);
>
> sbd = SYS_BUS_DEVICE(dev);
> sysbus_add_io(sbd, iobase, sysbus_mmio_get_region(sbd, 0));
> @@ -991,7 +989,7 @@ FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr,
> qdev_prop_set_bit(dev, "dma_enabled", false);
> }
>
> - fw_cfg_init1(dev);
> + qdev_init_nofail(dev);
>
> sbd = SYS_BUS_DEVICE(dev);
> sysbus_mmio_map(sbd, 0, ctl_addr);
> @@ -1097,6 +1095,8 @@ static void fw_cfg_io_realize(DeviceState *dev, Error
> **errp)
> sizeof(dma_addr_t));
> sysbus_init_mmio(sbd, &FW_CFG(s)->dma_iomem);
> }
> +
> + fw_cfg_init1(dev);
> }
>
> static void fw_cfg_io_class_init(ObjectClass *klass, void *data)
> @@ -1163,6 +1163,8 @@ static void fw_cfg_mem_realize(DeviceState *dev, Error
> **errp)
> sizeof(dma_addr_t));
> sysbus_init_mmio(sbd, &FW_CFG(s)->dma_iomem);
> }
> +
> + fw_cfg_init1(dev);
> }
>
> static void fw_cfg_mem_class_init(ObjectClass *klass, void *data)
>
This looks good to me generally, but I'm concerned about the part of
fw_cfg_init1() that precedes the original qdev_init_nofail() call, namely
assert(!object_resolve_path(FW_CFG_PATH, NULL));
object_property_add_child(OBJECT(machine), FW_CFG_NAME, OBJECT(s),
NULL);
The object_property_add_child() call creates a machine-global link to
the sole fw-cfg device, so that *other* code can find the fw-cfg device
by calling object_resolve_path(). (The same way that we assert fails
right before the creation, i.e. we don't try to create several fw_cfg
devices.)
I feel that this link creation does not belong in device realize
methods, but to board code. I feel that these two steps should be
factored out to a separate helper function, and then called from:
- fw_cfg_init_io_dma(), just before the new qdev_init_nofail() call site,
- fw_cfg_init_mem_wide(), just before the new qdev_init_nofail() call site,
- before any similar qdev_init_nofail() call sites, such as in
<https://lists.nongnu.org/archive/html/qemu-devel/2017-06/msg02658.html>.
Again this is just a gut feeling, comments / opinions welcome.
Thanks,
Laszlo
- [Qemu-devel] [PATCHv2 4/4] fw_cfg: move QOM type defines into fw_cfg.h, (continued)
- [Qemu-devel] [PATCHv2 4/4] fw_cfg: move QOM type defines into fw_cfg.h, Mark Cave-Ayland, 2017/06/12
- [Qemu-devel] [PATCHv2 1/4] fw_cfg: don't map the fw_cfg IO ports in fw_cfg_io_realize(), Mark Cave-Ayland, 2017/06/12
- [Qemu-devel] [PATCHv2 2/4] fw_cfg: move setting of FW_CFG_VERSION_DMA bit to fw_cfg_init1(), Mark Cave-Ayland, 2017/06/12
- [Qemu-devel] [PATCHv2 3/4] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers, Mark Cave-Ayland, 2017/06/12
- Re: [Qemu-devel] [PATCHv2 3/4] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers,
Laszlo Ersek <=
- Re: [Qemu-devel] [PATCHv2 0/4] fw_cfg: qdev-related tidy-ups, Paolo Bonzini, 2017/06/14