On 28/06/17 15:12, Igor Mammedov wrote: >>> I don't understand this part. When and why would the check become >>> useless? >> >> Well because when using the standard QEMU pattern of >> qdev_create()...qdev_init_nofail() it is possible to realize the device >> and wire up its MemoryRegions manually as I have done with the sun4u >> (i.e. it will respond to accesses) multiple times without calling the >> helper function and triggering the check. The ingenious part here is >> that only the developers who aren't aware that they have to manually >> call the helper function will be the ones who get caught out trying to >> instantiate the device multiple times ;) >> >> I'm also aware that with QOM we've got 2 conflicting ideas with fw_cfg: >> on the one hand we're saying that QOM hierarchy should at some level >> represent the topology of the machine, i.e. for sun4u the fw_cfg device >> should appear under the ebus device. whereas at the moment we assume a >> fixed path of FW_CFG_PATH. >> >> Based upon this I been thinking along the following lines: >> >> 1) Alter fw_cfg_find() to use object_resolve_path_type("", TYPE_FW_CFG, >> NULL) > I'd make use of the 3rd argument &ambiguous and assert on it
I don't think this is relevant here, as one of the aims of the patchset is to ensure that only one fw_cfg device is realized, since as Laszlo points out this is an underlying assumption in the code. >> This solves the issue of allowing the QOM tree to best represent the >> device topology as it will allow fw_cfg_find() to locate the fw_cfg >> device regardless of its location, and hence it can be placed as >> appropriate for each machine. > looks reasonable, that's what we were doing in a bunch of other cases Okay. >> 2) Add a check at the top of fw_cfg_common_realize() along the lines of: >> >> if (object_unattached(OBJECT(dev)) { >> error_setg(errp, "%s device must be explicitly added as a child " >> object before realize", TYPE_FW_CFG); >> return; >> } > that would be never trigger as ancestor's DEVICE.realize() always sets > parent if it wasn't set manually before child's realize is called. > > in other words it's guarantied that device has parent by the time > realize() is run by device_set_realized() Yes I understand that, although I may not have made it clear enough that this is pseudo code (there is no object_unattached() function in QEMU). If you look below you can see that the criteria I am using to distinguish whether a device is a child or not is to see whether it exists underneath /machine/unattached, since as you correctly point out the parent is already set by the time the device is realized. >> This makes it obvious to the developer that they MUST wire up the fw_cfg >> device to the machine before realize, and then if more than one device >> is instantiated we can still trigger the existing error from my v7 branch: >> >> if (!object_resolve_path_type("", TYPE_FW_CFG, NULL)) { >> error_setg(errp, "at most one %s device is permitted", TYPE_FW_CFG); >> return; >> } > reuse fw_cfg_find() here? Yes that is entirely valid - I've made this change locally. >> I prefer this method because it is impossible for a developer to >> accidentally realize the fw_cfg device without attaching it to the >> machine first (i.e. fw_cfg_find() will always succeed if this is the >> case if altered as per 1) above) and it can only be realized once. And >> following the POLA the device can be wired up using >> object_property_add_child() as per the numerous existing examples >> throughout the QEMU codebase. >> >> Now the minor issue with 2) is that I can't find an easy way to >> determine if the device is unattached at realize time. I've tried >> something like this: >> >> if (!object_resolve_path_type("/machine/unattached", >> TYPE_FW_CFG, NULL)) { >> ... >> ... >> } >> >> However that doesn't work because as the rules differ between partial >> and absolute path resolution, we end up resolving the container itself >> as opposed to iterating down through the QOM tree. Is there an existing >> solution for this or would I need to come up with something that >> iterates over the container children to search for a TYPE_FW_CFG device >> myself? Actually the visitor function isn't too complicated at all and I have something working now. Let me tidy up the patchset and if it looks good, I'll re-post it as a v7. ATB, Mark.