Re: [Qemu-devel] [PATCHv6 3/5] fw_cfg: move assert() and linking of fw_cfg device to the machine into instance_init()

[Mark Cave-Ayland]

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.