On 21/06/17 14:23, Eduardo Habkost wrote: >>>>> I now have a v7 patchset ready to go (currently hosted at >>>>> https://github.com/mcayland/qemu/tree/fwcfg7 for the curious). Laszlo, >>>>> I've currently left off your Tested-by tag since I'm not sure it's still >>>>> valid for less-than-trivial changes - if you're happy for me to re-add >>>>> it before I send the v7 patchset to the list, please let me know. >>>> >>>> I intend to test v7 when you post it. >>> >>> I still see the instance_init assert() in that branch (commit >>> 17d75643f880). Is that correct? >> >> Yes that was the intention. In 17d75643f880 both the assert() and >> object_property_add_child() are moved into the instance_init() function, >> and then in the follow-up commit eddedb5 the assert() is removed from >> instance_init() and the object_resolve_path_type() check added into >> fw_cfg_init1() as part of its conversion into the >> fw_cfg_common_realize() function. > > We can't move assert() + linking to instance_init even if it's > just temporary, as it makes device-list-properties crash. > > Just linking in instance_init is also a problem, because > instance_init should never affect global QEMU state. > > You could move linking to realize as well, but: just like you > already moved sysbus_add_io() calls outside realize, I believe > linking belongs outside realize too. I need to re-read the > discussion threads to understand the motivation behind that.
Ultimately the question we're trying to answer is "has someone instantiated another fw_cfg device for this machine?" and the way it works currently is that fw_cfg_init_io() and fw_cfg_init_mem() attach the fw_cfg device to the /machine object and then check after realize whether a /machine/fw_cfg device already exists, aborting if it does. So in the current implementation we're not actually concerned with the placement of fw_cfg within the model itself, and indeed with a fixed location in the QOM tree it's already completely wrong. If you take a look at the QOM tree for the sparc/sparc64/ppc machines you'll see that they really are very far from reality. For me the use of object_resolve_path_type() during realize is a good solution since regardless of the location of the fw_cfg we can always detect whether we have more than one device instance. However what seems unappealing about this is that while all existing users which use fw_cfg_init_io() and fw_cfg_init_mem() are fine, in the case where I am wiring up the device myself then for my sun4u example the code looks like this: dev = qdev_create(NULL, TYPE_FW_CFG_IO); ... qdev_init_nofail(dev); memory_region_add_subregion(pci_address_space_io(ebus), BIOS_CFG_IOPORT, &FW_CFG_IO(dev)->comb_iomem); Here you can see that the device is active because it is mapped into the correct IO address space, but notice I have forgotten to link it to the QOM /machine object myself. Hence I can instantiate and map as many instances as I like and never trigger the duplicate instance check which makes the check fairly ineffective. This makes me think that I'm still misunderstanding something, so I'd be grateful for any further suggestions. >> One last question which might avoid a future v8 revision: does the error >> handling in eddedb5 for the fw_cfg_*_realize() functions look correct? > > The error handling looks correct to me. Thanks for the review, in that case I will leave it in its current form. ATB, Mark.