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

[Laszlo Ersek]

On 06/23/17 18:10, Eduardo Habkost wrote:
> On Fri, Jun 23, 2017 at 05:52:03PM +0200, Laszlo Ersek wrote:
>> On 06/23/17 13:50, Eduardo Habkost wrote:
>>> On Fri, Jun 23, 2017 at 09:12:01AM +0100, Mark Cave-Ayland wrote:
>>>> On 21/06/17 14:23, Eduardo Habkost wrote:
>>>>
>>>>>>>>> I now have a v7 patchset ready to go (currently hosted at
>>>>>>>>> https://github.com/mcayland/qemu/tree/fwcfg7 for the curious). Laszlo,
>>>>>>>>> I've currently left off your Tested-by tag since I'm not sure it's 
>>>>>>>>> still
>>>>>>>>> valid for less-than-trivial changes - if you're happy for me to re-add
>>>>>>>>> it before I send the v7 patchset to the list, please let me know.
>>>>>>>>
>>>>>>>> I intend to test v7 when you post it.
>>>>>>>
>>>>>>> I still see the instance_init assert() in that branch (commit
>>>>>>> 17d75643f880).  Is that correct?
>>>>>>
>>>>>> Yes that was the intention. In 17d75643f880 both the assert() and
>>>>>> object_property_add_child() are moved into the instance_init() function,
>>>>>> and then in the follow-up commit eddedb5 the assert() is removed from
>>>>>> instance_init() and the object_resolve_path_type() check added into
>>>>>> fw_cfg_init1() as part of its conversion into the
>>>>>> fw_cfg_common_realize() function.
>>>>>
>>>>> We can't move assert() + linking to instance_init even if it's
>>>>> just temporary, as it makes device-list-properties crash.
>>>>>
>>>>> Just linking in instance_init is also a problem, because
>>>>> instance_init should never affect global QEMU state.
>>>>>
>>>>> You could move linking to realize as well, but: just like you
>>>>> already moved sysbus_add_io() calls outside realize, I believe
>>>>> linking belongs outside realize too.  I need to re-read the
>>>>> discussion threads to understand the motivation behind that.
>>>>
>>>> Ultimately the question we're trying to answer is "has someone
>>>> instantiated another fw_cfg device for this machine?" and the way it
>>>> works currently is that fw_cfg_init_io() and fw_cfg_init_mem() attach
>>>> the fw_cfg device to the /machine object and then check after realize
>>>> whether a /machine/fw_cfg device already exists, aborting if it does.
>>>>
>>>> So in the current implementation we're not actually concerned with the
>>>> placement of fw_cfg within the model itself, and indeed with a fixed
>>>> location in the QOM tree it's already completely wrong. If you take a
>>>> look at the QOM tree for the sparc/sparc64/ppc machines you'll see that
>>>> they really are very far from reality.
>>>>
>>>> For me the use of object_resolve_path_type() during realize is a good
>>>> solution since regardless of the location of the fw_cfg we can always
>>>> detect whether we have more than one device instance.
>>>>
>>>> However what seems unappealing about this is that while all existing
>>>> users which use fw_cfg_init_io() and fw_cfg_init_mem() are fine, in the
>>>> case where I am wiring up the device myself then for my sun4u example
>>>> the code looks like this:
>>>>
>>>> dev = qdev_create(NULL, TYPE_FW_CFG_IO);
>>>> ...
>>>> qdev_init_nofail(dev);
>>>> memory_region_add_subregion(pci_address_space_io(ebus), BIOS_CFG_IOPORT,
>>>>                             &FW_CFG_IO(dev)->comb_iomem);
>>>>
>>>> Here you can see that the device is active because it is mapped into the
>>>> correct IO address space, but notice I have forgotten to link it to the
>>>> QOM /machine object myself. Hence I can instantiate and map as many
>>>> instances as I like and never trigger the duplicate instance check which
>>>> makes the check fairly ineffective.
>>>
>>> This is a good point, but I have a question about that: will something
>>> break if a machine decides to create two fw_cfg objects and map them to
>>> different addresses?  If it won't break, I see no reason to try to
>>> enforce a single instance in the device code.  If it will break, then a
>>> check in realize is still a hack, but might be a good enough solution.
>>>
>>
>> (1) For the guest, it makes no sense to encounter two fw_cfg devices.
>> Also, a lot of the existent code in QEMU assumes at most one fw_cfg
>> device (for example, there is some related ACPI generation).
> 
> This is an argument for making board code ensure there's only one
> device, and possibly for providing a helper that board code can use.
> But it doesn't require validation on realize.
> 
>>
>> (2) Relatedly, the fw_cfg_find() helper function is used quite widely,
>> and it shouldn't break -- either due to more-than-one device instances,
>> or due to the one fw_cfg device being linked under a path that is
>> different from FW_CFG_PATH.
> 
> This is also an argument for providing a helper that ensures
> fw_cfg_find() will work, but doesn't require validation on realize.

I agree.

If you read back the discussion threads under the earlier versions of
this patch set, you'll find that I argued for keeping the unicity stuff
-- and possibly some other stuff -- that currently resides in the
fw_cfg_init1() *helper* function outside of device code (realize or
otherwise).

I didn't disagree with the reorganization of the code, but I suggested
to preserve this stuff in helper functions, which board code could call.
This would clearly place the same burden on Mark's new sun4u board code
-- i.e., that new board code would *also* have to call these helper
function(s). Mark disliked this board requirement (he only wanted to
instantiate the device and be done with it, in board code); and we went
from there.

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).

> 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.

Thanks
Laszlo