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 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 makes me think that I'm still misunderstanding something, so I'd be
grateful for any further suggestions.

>> One last question which might avoid a future v8 revision: does the error
>> handling in eddedb5 for the fw_cfg_*_realize() functions look correct?
> 
> The error handling looks correct to me.

Thanks for the review, in that case I will leave it in its current form.


ATB,

Mark.