| Message ID | 1497302470-10776-4-git-send-email-mark.cave-ayland@ilande.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <mark.cave-ayland@ilande.co.uk> > --- > 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
On 12/06/17 23:50, Laszlo Ersek wrote: > 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 <mark.cave-ayland@ilande.co.uk> >> --- >> 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. After testing the assert() in a few different scenarios, I much prefer the existing approach with a fixed fw_cfg path at /machine/fw_cfg. The reason for this is that with existing code, any attempt to init a second fw_cfg device will assert immediately, whereas if the board is responsible for wiring up the fw_cfg device then it's possible that a caller will either a) forget to call a helper function or b) wire up 2 fw_cfg in different places in the object tree, e.g. one calling fw_cfg_init_io() and another instantiated directly via QOM as per what I'm trying to do with sun4u. Taking all this into account, I'm working on a v3 patchset that I hope to post to the list later today. ATB, Mark.
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)
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 <mark.cave-ayland@ilande.co.uk> --- hw/nvram/fw_cfg.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)