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

[Eduardo Habkost]

On Fri, Jul 07, 2017 at 02:54:49PM +0100, Mark Cave-Ayland wrote:
> 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?
> 

Possibly.  If ambiguous is NULL, you need to be aware that
fw_cfg_find() will return NULL on realize if there are 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().

Note that if you add assert(!ambiguous) to fw_cfg_find(), you
need to choose what to do when multiple devices are instantiated:
a) Calling fw_cfg_find() on realize, making it abort instead of
   returning an error;
b) Not calling fw_cfg_find() on realize, and returning an error
   using object_resolve_path_type() manually;
c) Not calling fw_cfg_find(), not returning an error, and letting
   QEMU abort when fw_cfg_find() is called by other code.

I think it's simpler to just do like vmgenid does and simply
return NULL when there are multiple devices.  We just need to
document that clearly, and be aware of that when calling
fw_cfg_find() on realize.  But I won't object if you choose
another option.

-- 
Eduardo