Re: [Qemu-devel] [PATCHv7 5/6] fw_cfg: move qdev_init_nofail() from fw_cfg_init1() to callers

[Mark Cave-Ayland]

On 07/07/17 14:26, Eduardo Habkost wrote:

(cut)

>> However this causes us a problem: if you instantiate the fw_cfg yourself
>> with qdev_create()...qdev_init_nofail() then you can potentially access
>> the underlying MemoryRegions directly and wire up the device without
>> attaching it to the QOM tree. This is an invalid configuration but
>> wouldn't be detected with fw_cfg_find().
> 
> Why is it an invalid configuration?  All we need is that the
> device get wired correctly and that fw_cfg_find() find the device
> correctly.  Your patch 3/6 makes fw_cfg_find() work on any path,
> making fw_cfg_unattached_at_realize() unnecessary.

That's a good point actually. The fw_cfg_find() change came in fairly
recently but from memory that on its own wasn't enough to prevent
multiple fw_cfg devices in my local tests which is why added
fw_cfg_unattached_at_realize() (sadly I can't remember exactly which
test case failed).

So actually perhaps the reason I got sidetracked here was because I was
actually hitting the issue pointed out by Igor whereby with ambiguous
set to NULL you can miss detecting multiple instances?

>>
>> The correct way to handle this is to wire up the device yourself with
>> object_property_add_child() but then you find the situation whereby the
>> people who know that you have to call object_property_add_child() when
>> adding the fw_cfg device are the ones who don't need the error message.
>>
>> Therefore the solution was to enforce that the fw_cfg device has been
>> added to the QOM tree before realize because it solves all the problems:
>>
>> 1) The fw_cfg device can now be placed anywhere in the QOM tree, meaning
>> that the QOM tree can be a topologically correct representation of the
>> machine
> 
> While attaching the device explicitly is a good idea, we don't
> need to make it a fatal error.
> 
>>
>> 2) By enforcing that the fw_cfg device has been parented, we guarantee
>> that the fw_cfg_find() check is correct and it is impossible to access a
>> fw_cfg device that hasn't been wired up to the machine
> 
> This is solved by patch 3/6.
> 
>>
>> 3) Since these checks are done at realize time, we can provide nice
>> friendly messages back to the developer to tell them what needs to be fixed
> 
> I don't see what needs to be fixed.  It is not a bug to leave
> fw_cfg in /machine/unattached, as long as fw_cfg_find() works
> properly.

Yeah. I wonder if I've been leading myself astray down the wrong path
here? Let me do some more local tests without
fw_cfg_unattached_at_realize() and with an ambiguous argument in
fw_cfg_find().


ATB,

Mark.