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 27/06/17 01:49, Eduardo Habkost wrote:

> On Sun, Jun 25, 2017 at 07:58:04PM +0100, Mark Cave-Ayland wrote:
>> On 23/06/17 19:50, Eduardo Habkost wrote:
>>
>>>> Really, please go back to the earlier discussion around fw_cfg_init1()
>>>> and you'll see my original point (which matches what you just voiced).
>>>
>>> Yep.  I was just not sure validation on realize was necessary or
>>> convenient.  It looks like we agree it is just convenient.
>>>
>>>
>>>>
>>>>> All that said, I don't have a strong argument against doing it on
>>>>> realize, except my gut feeling that this is not how qdev was
>>>>> designed[1].  If doing it on realize is the simplest way to do it, I
>>>>> won't be the one complaining.
>>>>>
>>>>> [1] I believe the original intent was to make every single device
>>>>>     user-creatable and define boards in a declarative way in config
>>>>>     files.  We are very far from that goal.
>>>>
>>>> I'm fine either way, I just wanted to accommodate Mark's preference,
>>>> because he seemed to strongly dislike having to call any helper
>>>> functions from board code, beyond initing and realizing the device.
>>>>
>>>> This is my recollection of the earlier discussion anyway.
>>>
>>> I think we are in agreement, then.  If there are no objections from
>>> others against doing object_property_add_child() on realize, I'm also OK
>>> with that.
>>
>> Just to clarify here that my objection wasn't so much about calling
>> helper functions from board code, it was that as the current patch
>> exposes the MemoryRegions in TYPE_FW_CFG_IO and TYPE_FW_CFG_MEM then as
>> per my example where the machine link is omitted then the check becomes
>> useless.
> 
> 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)

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.

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;
}

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;
}

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?


ATB,

Mark.