On Fri, Jul 07, 2017 at 04:44:53PM +0200, Igor Mammedov wrote:
[...]
> > > 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.
> the rule I use to decide assert vs nice error handling:
> 1: try to avoid crash on hotplug path
> 2: if error could be induced by end user on startup, try to print nice error
> before dying
> 3: when error should not happen just assert or use error_abort
> which would print nice error message before dying.
I would add this to the list:
If returning error instead of aborting is easy, return an error
and let the caller decide if it should use &error_abort or not.
>
> > 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.
> I'd just kill qemu in fw_cfg case, which is small not intrusive change.
>
> [...]
>
> > >>>>
> > >>>> - 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.
> without using ambiguous argument object_resolve_path_type() isn't
> returning NULL in case of duplicates in different leafs of tree.
This doesn't sound right. object_resolve_path_type() should
always return NULL if multiple matches are found. See its
documentation.
>
> for reason, see
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg460692.html
> or look at object_resolve_partial_path() impl.
>
[...]
> > Finally for reference here is the current version of the code in my
> > outstanding sun4u patchset which wires up the fw_cfg device behind a PCI
> > bridge in hw/sparc64/sun4u:
> >
> > dev = qdev_create(NULL, TYPE_FW_CFG_IO);
> > qdev_prop_set_bit(dev, "dma_enabled", false);
> > object_property_add_child(OBJECT(ebus), TYPE_FW_CFG, OBJECT(dev),
> > NULL);
> > qdev_init_nofail(dev);
> > memory_region_add_subregion(pci_address_space_io(ebus),
> > BIOS_CFG_IOPORT,
> > &FW_CFG_IO(dev)->comb_iomem);
> looks fine,
>
> so what I'd do is:
> * drop 4/6
Agreed on this point. But:
> * make fw_cfg_find() use ambiguous argument and error_abort if ambiguous ==
> true
> * from fw_cfg_common_realize() just call
>
> // if fw_cfg_find() returns NULL it means that device isn't in QOM tree
> // which shouldn't ever happen, fw_cfg_find() will abort itself if
> // another instance of device present in QOM tree.
> assert(fw_cfg_find());
That would work, but I don't see why doing that if just returning
NULL would: 1) make the code in fw_cfg_find() simpler and
shorter; 2) make realize error handling friendlier (returning
error instead of aborting). We just need to document that
explicitly in fw_cfg_find() (see find_vmgenid_dev() for an
example).
If you still want to make realize abort instead of returning
error, you don't even need assert(ambiguous) on fw_cfg_find().
All you need is this on realize:
assert(fw_cfg_find() == dev);
--
Eduardo
