On 06/23/17 13:50, Eduardo Habkost wrote: > On Fri, Jun 23, 2017 at 09:12:01AM +0100, Mark Cave-Ayland wrote: >> 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 is a good point, but I have a question about that: will something > break if a machine decides to create two fw_cfg objects and map them to > different addresses? If it won't break, I see no reason to try to > enforce a single instance in the device code. If it will break, then a > check in realize is still a hack, but might be a good enough solution. >
(1) For the guest, it makes no sense to encounter two fw_cfg devices. Also, a lot of the existent code in QEMU assumes at most one fw_cfg device (for example, there is some related ACPI generation). (2) Relatedly, the fw_cfg_find() helper function is used quite widely, and it shouldn't break -- either due to more-than-one device instances, or due to the one fw_cfg device being linked under a path that is different from FW_CFG_PATH. Thanks Laszlo