On 03/07/17 10:39, Igor Mammedov wrote: > On Thu, 29 Jun 2017 15:07:19 +0100 > Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk> 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, and also rename it to fw_cfg_common_realize() >> which better describes its new purpose. >> >> Since it is now the responsibility of the machine to wire up the fw_cfg >> device >> it is necessary to introduce a object_property_add_child() call into >> fw_cfg_init_io() and fw_cfg_init_mem() to link the fw_cfg device to the root >> machine object as before. >> >> Finally we can now convert the asserts() preventing multiple fw_cfg devices >> and unparented fw_cfg devices being instantiated and replace them with proper >> error reporting at realize time. This allows us to remove FW_CFG_NAME and >> FW_CFG_PATH since they are no longer required. >> >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk> >> --- >> hw/nvram/fw_cfg.c | 41 +++++++++++++++++++++++++++++------------ >> 1 file changed, 29 insertions(+), 12 deletions(-) >> >> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c >> index 2291121..31029ac 100644 >> --- a/hw/nvram/fw_cfg.c >> +++ b/hw/nvram/fw_cfg.c >> @@ -37,9 +37,6 @@ >> >> #define FW_CFG_FILE_SLOTS_DFLT 0x20 >> >> -#define FW_CFG_NAME "fw_cfg" >> -#define FW_CFG_PATH "/machine/" FW_CFG_NAME >> - >> #define TYPE_FW_CFG "fw_cfg" >> #define TYPE_FW_CFG_IO "fw_cfg_io" >> #define TYPE_FW_CFG_MEM "fw_cfg_mem" >> @@ -920,19 +917,22 @@ static int fw_cfg_unattached_at_realize(void) >> } >> >> >> -static void fw_cfg_init1(DeviceState *dev) >> +static void fw_cfg_common_realize(DeviceState *dev, Error **errp) >> { >> FWCfgState *s = FW_CFG(dev); >> MachineState *machine = MACHINE(qdev_get_machine()); >> uint32_t version = FW_CFG_VERSION; >> >> - assert(!object_resolve_path(FW_CFG_PATH, NULL)); >> - >> - object_property_add_child(OBJECT(machine), FW_CFG_NAME, OBJECT(s), >> NULL); >> - >> - qdev_init_nofail(dev); >> + if (!fw_cfg_find()) { > maybe add comment that here, that fw_cfg_find() will return NULL if more than > 1 device is found.
This should be the same behaviour as before, i.e. a NULL means that the fw_cfg device couldn't be found. It seems intuitive to me from the name of the function exactly what it does, so I don't find the lack of comment too confusing. Does anyone else have any thoughts here? > >> + error_setg(errp, "at most one %s device is permitted", TYPE_FW_CFG); > s/TYPE_FW_CFG/object_get_typename()/ > so it would print leaf type name Previously the code would add the device to the machine at FW_CFG_PATH which was fixed at /machine/fw_cfg regardless of whether it was fw_cfg_io or fw_cfg_mem type (see the top of fw_cfg.c). This was a deliberate attempt to preserve the existing behaviour and if we were to give the leaf type name I think this would be misleading, since it implies you could have one fw_cfg_io and one fw_cfg_mem which isn't correct. > >> + return; >> + } >> >> - assert(!fw_cfg_unattached_at_realize()); >> + if (fw_cfg_unattached_at_realize()) { > as I pointed out in v6, this condition will always be false, > I suggest to drop 4/6 patch and this hunk here so it won't to confuse > readers with assumption that condition might succeed. Definitely look more closely at the fw_cfg_unattached_at_realize() implementation in patch 4. You'll see that the check to determine if the device is attached does not check obj->parent but checks to see if the device exists under /machine/unattached which is what the device_set_realised() does if the device doesn't have a parent. I can confirm that I've given this a fairly good test with parented and non-parented objects and it seems to work well here. Does it not work for you in testing? > >> + error_setg(errp, "%s device must be added as a child device before " >> + "realize", TYPE_FW_CFG); >> + return; >> + } >> >> fw_cfg_add_bytes(s, FW_CFG_SIGNATURE, (char *)"QEMU", 4); >> fw_cfg_add_bytes(s, FW_CFG_UUID, &qemu_uuid, 16); >> @@ -965,7 +965,9 @@ 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); >> + object_property_add_child(OBJECT(qdev_get_machine()), TYPE_FW_CFG, >> + OBJECT(dev), NULL); >> + qdev_init_nofail(dev); >> >> sbd = SYS_BUS_DEVICE(dev); >> ios = FW_CFG_IO(dev); >> @@ -1003,7 +1005,9 @@ FWCfgState *fw_cfg_init_mem_wide(hwaddr ctl_addr, >> qdev_prop_set_bit(dev, "dma_enabled", false); >> } >> >> - fw_cfg_init1(dev); >> + object_property_add_child(OBJECT(qdev_get_machine()), TYPE_FW_CFG, >> + OBJECT(dev), NULL); >> + qdev_init_nofail(dev); >> >> sbd = SYS_BUS_DEVICE(dev); >> sysbus_mmio_map(sbd, 0, ctl_addr); >> @@ -1033,6 +1037,7 @@ FWCfgState *fw_cfg_find(void) >> return FW_CFG(object_resolve_path_type("", TYPE_FW_CFG, NULL)); >> } >> >> + >> static void fw_cfg_class_init(ObjectClass *klass, void *data) >> { >> DeviceClass *dc = DEVICE_CLASS(klass); >> @@ -1104,6 +1109,12 @@ static void fw_cfg_io_realize(DeviceState *dev, Error >> **errp) >> &fw_cfg_dma_mem_ops, FW_CFG(s), "fwcfg.dma", >> sizeof(dma_addr_t)); >> } >> + >> + fw_cfg_common_realize(dev, &local_err); >> + if (local_err) { >> + error_propagate(errp, local_err); >> + return; >> + } >> } >> >> static void fw_cfg_io_class_init(ObjectClass *klass, void *data) >> @@ -1170,6 +1181,12 @@ 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_common_realize(dev, &local_err); >> + if (local_err) { >> + error_propagate(errp, local_err); >> + return; >> + } >> } >> >> static void fw_cfg_mem_class_init(ObjectClass *klass, void *data) > patch looks good, with above comments fixed: Great, thanks! I think there is some misunderstanding about the points above so I'd be interested to hear your further comments. > Reviewed-by: Igor Mammedov <imamm...@redhat.com> ATB, Mark.