On 07/07/17 14:26, Eduardo Habkost wrote: (cut)
>> However this causes us a problem: if you instantiate the fw_cfg yourself >> with qdev_create()...qdev_init_nofail() then you can potentially access >> the underlying MemoryRegions directly and wire up the device without >> attaching it to the QOM tree. This is an invalid configuration but >> wouldn't be detected with fw_cfg_find(). > > Why is it an invalid configuration? All we need is that the > device get wired correctly and that fw_cfg_find() find the device > correctly. Your patch 3/6 makes fw_cfg_find() work on any path, > making fw_cfg_unattached_at_realize() unnecessary. That's a good point actually. The fw_cfg_find() change came in fairly recently but from memory that on its own wasn't enough to prevent multiple fw_cfg devices in my local tests which is why added fw_cfg_unattached_at_realize() (sadly I can't remember exactly which test case failed). So actually perhaps the reason I got sidetracked here was because I was actually hitting the issue pointed out by Igor whereby with ambiguous set to NULL you can miss detecting multiple instances? >> >> The correct way to handle this is to wire up the device yourself with >> object_property_add_child() but then you find the situation whereby the >> people who know that you have to call object_property_add_child() when >> adding the fw_cfg device are the ones who don't need the error message. >> >> Therefore the solution was to enforce that the fw_cfg device has been >> added to the QOM tree before realize because it solves all the problems: >> >> 1) The fw_cfg device can now be placed anywhere in the QOM tree, meaning >> that the QOM tree can be a topologically correct representation of the >> machine > > While attaching the device explicitly is a good idea, we don't > need to make it a fatal error. > >> >> 2) By enforcing that the fw_cfg device has been parented, we guarantee >> that the fw_cfg_find() check is correct and it is impossible to access a >> fw_cfg device that hasn't been wired up to the machine > > This is solved by patch 3/6. > >> >> 3) Since these checks are done at realize time, we can provide nice >> friendly messages back to the developer to tell them what needs to be fixed > > I don't see what needs to be fixed. It is not a bug to leave > fw_cfg in /machine/unattached, as long as fw_cfg_find() works > properly. Yeah. I wonder if I've been leading myself astray down the wrong path here? Let me do some more local tests without fw_cfg_unattached_at_realize() and with an ambiguous argument in fw_cfg_find(). ATB, Mark.
