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:12:19PM +0100, Mark Cave-Ayland wrote:
> On 07/07/17 12:33, Igor Mammedov wrote:
> 
> > On Tue, 4 Jul 2017 19:08:44 +0100
> > Mark Cave-Ayland <address@hidden> wrote:
> > 
> >> On 03/07/17 10:39, Igor Mammedov wrote:
> >>
> >>> On Thu, 29 Jun 2017 15:07:19 +0100
> >>> Mark Cave-Ayland <address@hidden> wrote:
> >>>   
> >>>> When looking to instantiate a TYPE_FW_CFG_MEM or TYPE_FW_CFG_IO device 
> >>>> to be
> >>>> able to wire it up differently, it is much more convenient for the 
> >>>> caller to
> >>>> instantiate the device and have the fw_cfg default files already 
> >>>> preloaded
> >>>> during realize.
> >>>>
> >>>> Move fw_cfg_init1() to the end of both the fw_cfg_mem_realize() and
> >>>> fw_cfg_io_realize() functions so it no longer needs to be called manually
> >>>> when instantiating the device, and also rename it to 
> >>>> fw_cfg_common_realize()
> >>>> which better describes its new purpose.
> >>>>
> >>>> Since it is now the responsibility of the machine to wire up the fw_cfg 
> >>>> device
> >>>> it is necessary to introduce a object_property_add_child() call into
> >>>> fw_cfg_init_io() and fw_cfg_init_mem() to link the fw_cfg device to the 
> >>>> root
> >>>> machine object as before.
> >>>>
> >>>> Finally we can now convert the asserts() preventing multiple fw_cfg 
> >>>> devices
> >>>> and unparented fw_cfg devices being instantiated and replace them with 
> >>>> proper
> >>>> error reporting at realize time. This allows us to remove FW_CFG_NAME and
> >>>> FW_CFG_PATH since they are no longer required.
> >>>>
> >>>> Signed-off-by: Mark Cave-Ayland <address@hidden>
> >>>> ---
> >>>>  hw/nvram/fw_cfg.c |   41 +++++++++++++++++++++++++++++------------
> >>>>  1 file changed, 29 insertions(+), 12 deletions(-)
> >>>>
> >>>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> >>>> index 2291121..31029ac 100644
> >>>> --- a/hw/nvram/fw_cfg.c
> >>>> +++ b/hw/nvram/fw_cfg.c
> >>>> @@ -37,9 +37,6 @@
> >>>>  
> >>>>  #define FW_CFG_FILE_SLOTS_DFLT 0x20
> >>>>  
> >>>> -#define FW_CFG_NAME "fw_cfg"
> >>>> -#define FW_CFG_PATH "/machine/" FW_CFG_NAME
> >>>> -
> >>>>  #define TYPE_FW_CFG     "fw_cfg"
> >>>>  #define TYPE_FW_CFG_IO  "fw_cfg_io"
> >>>>  #define TYPE_FW_CFG_MEM "fw_cfg_mem"
> >>>> @@ -920,19 +917,22 @@ static int fw_cfg_unattached_at_realize(void)
> >>>>  }
> >>>>  
> >>>>  
> >>>> -static void fw_cfg_init1(DeviceState *dev)
> >>>> +static void fw_cfg_common_realize(DeviceState *dev, Error **errp)
> >>>>  {
> >>>>      FWCfgState *s = FW_CFG(dev);
> >>>>      MachineState *machine = MACHINE(qdev_get_machine());
> >>>>      uint32_t version = FW_CFG_VERSION;
> >>>>  
> >>>> -    assert(!object_resolve_path(FW_CFG_PATH, NULL));
> >>>> -
> >>>> -    object_property_add_child(OBJECT(machine), FW_CFG_NAME, OBJECT(s), 
> >>>> NULL);
> >>>> -
> >>>> -    qdev_init_nofail(dev);
> >>>> +    if (!fw_cfg_find()) {  
> >>> maybe add comment that here, that fw_cfg_find() will return NULL if more 
> >>> than
> >>> 1 device is found.  
> >>
> >> This should be the same behaviour as before, i.e. a NULL means that the
> >> fw_cfg device couldn't be found. It seems intuitive to me from the name
> >> of the function exactly what it does, so I don't find the lack of
> >> comment too confusing. Does anyone else have any thoughts here?
> >
> > intuitive meaning from the function name would be return NULL if nothing 
> > were found,
> > however it's not the case here.
> 
> The function with its current name has always existed, so all I'm doing
> here is reusing it as per your comment on an earlier patchset.
> 
> > taking in account that fwcfg in not user creatable device how about:
> > 
> > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> > index 316fca9..8f6eef8 100644
> > --- a/hw/nvram/fw_cfg.c
> > +++ b/hw/nvram/fw_cfg.c
> > @@ -1014,7 +1014,10 @@ FWCfgState *fw_cfg_init_mem(hwaddr ctl_addr, hwaddr 
> > data_addr)
> >  
> >  FWCfgState *fw_cfg_find(void)
> >  {
> > -    return FW_CFG(object_resolve_path(FW_CFG_PATH, NULL));
> > +    bool ambig = false;
> > +    FWCfgState *f = FW_CFG(object_resolve_path_type("", TYPE_FW_CFG, 
> > &ambig));
> > +    assert(!ambig);
> > +    return f;
> >  }
> > 
> > or if we must to print user friendly error and fail realize gracefully
> > (not sure why) just add errp argument to function so it could report
> > error back to caller, then places that do not care much about graceful
> > exit would use error_abort as argument and realize would use
> > its local_error/errp argument.
> 
> My understanding from the thread was that we should only use assert()s
> where there is no other choice so that any failures can be handled in a
> more friendly manner.
> 
> Now as Laszlo pointed out, fw_cfg_find() is used externally to locate
> the fw_cfg device in other parts of the QEMU codebase. Yes I agree that
> it is possible to change the way in which it returns, however I would
> argue that changing those semantics are outside of the scope of this patch.
> 
> >    
> >>>> +        error_setg(errp, "at most one %s device is permitted", 
> >>>> TYPE_FW_CFG);  
> >>> s/TYPE_FW_CFG/object_get_typename()/
> >>> so it would print leaf type name   
> >>
> >> Previously the code would add the device to the machine at FW_CFG_PATH
> >> which was fixed at /machine/fw_cfg regardless of whether it was
> >> fw_cfg_io or fw_cfg_mem type (see the top of fw_cfg.c).
> >>
> >> This was a deliberate attempt to preserve the existing behaviour and if
> >> we were to give the leaf type name I think this would be misleading,
> >> since it implies you could have one fw_cfg_io and one fw_cfg_mem which
> >> isn't correct.
> > I don't have strong preference here, considering that it's
> > hardcoded in board (not user specified) device,
> > developer that stumbles upon error should be able to dig out which
> > concrete type caused it.
> 
> Okay if no-one else comments then I'll leave it as-is in order to
> preserve the existing behaviour as much as possible.
> 
> >>>> +        return;
> >>>> +    }
> >>>>  
> >>>> -    assert(!fw_cfg_unattached_at_realize());
> >>>> +    if (fw_cfg_unattached_at_realize()) {  
> >>> as I pointed out in v6, this condition will always be false,
> >>> I suggest to drop 4/6 patch and this hunk here so it won't to confuse
> >>> readers with assumption that condition might succeed.  
> >>
> >> Definitely look more closely at the fw_cfg_unattached_at_realize()
> >> implementation in patch 4. You'll see that the check to determine if the
> >> device is attached does not check obj->parent but checks to see if the
> >> device exists under /machine/unattached which is what the
> >> device_set_realised() does if the device doesn't have a parent.
> >
> > looking more fw_cfg_unattached_at_realize(),
> >  returns true if fwcfg is direct child of/machine/unattached
> > meaning implicit parent assignment.
> > 
> > As result, above condition essentially forbids having fwcfg under
> > /machine/unattached and forces user to explicitly set parent
> > outside of /machine/unattached or be a child of some other device.
> > 
> > Is this your intent (why)?
> 
> Yes that is entirely correct. All current fw_cfg users setup the device
> using fw_cfg_init_io() and fw_cfg_init_mem() which is fine for those
> cases because these functions attach the fw_cfg device directly to the
> machine at /machine/fw_cfg. This makes it trivial to determine whether
> or not an existing fw_cfg has been instantiated and prevent any more
> instances, which Laszlo has stated is an underlying assumption for
> fw_cfg_find().
> 
> In my particular use case for SPARC64, I need to move the fw_cfg device
> behind a PCI bridge. Therefore in order to allow the QOM tree to reflect
> the actual hardware DT then the fw_cfg device needs to be attached to
> the PCI bridge and not the machine. Hence the check for an existing
> device at /machine/fw_cfg is no longer good enough to determine if a
> fw_cfg device already exists since if they do, they can be in several
> different locations in the QOM tree.
> 
> This explains the change to fw_cfg_find() to make sure that we find any
> other fw_cfg instances, no matter where they are in the QOM tree.
> 
> 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.

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

-- 
Eduardo