qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCHv6 3/5] fw_cfg: move assert() and linking of fw_c


From: Laszlo Ersek
Subject: Re: [Qemu-devel] [PATCHv6 3/5] fw_cfg: move assert() and linking of fw_cfg device to the machine into instance_init()
Date: Fri, 23 Jun 2017 17:52:03 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.0

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

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

Thanks
Laszlo



reply via email to

[Prev in Thread] Current Thread [Next in Thread]