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

[Eduardo Habkost]

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?

> 
> I can see that device_set_realized() will always set the device parent
> to /machine/unattached before calling the realize function if the device
> doesn't have a parent. So is it even possible to add the device via
> object_property_add_child() to the machine during realize? Or could it
> be done by making /machine/fw_cfg an alias to its real location in the
> QOM tree at realize time without breaking the object_resolve_path_type()
> check?

Well, if we can't do object_property_add_child() on ->instance_init()
and doing it on ->realize() would require a more complex solution
involving QOM links, I believe the simplest solution is to provide a
helper function.

> 
> The other interesting option to explore is that since fw_cfg already has
> a machine_ready notifier, the check could be moved there similar to as
> done in hw/core/machine.c's error_on_sysbus_device() if the check
> shouldn't be present in realize. That still doesn't answer the question
> as to how to enforce that the device is correctly linked to the machine
> though.

I think both manually mapping+linking from board code or calling a
helper function from board code would be acceptable.

-- 
Eduardo