Re: [Qemu-devel] [RFCv2 1/2] spapr: Remove unnecessary owner field from sPAPRDRConnector

[Michael Roth <mdroth@linux.vnet.ibm.com>]

Quoting Paolo Bonzini (2015-09-14 09:24:23)
> 
> 
> On 14/09/2015 16:06, Alexey Kardashevskiy wrote:
> >>>>>
> >>>>> === * There is no way for a child to determine what its parent
> >>>>> is.  It is not * a bidirectional relationship.  This is by
> >>>>> design. ===
> >>>>>
> >>>>> This part always confused me as there is "Object *parent" in
> >>>>> the "struct Object". So there is way to determine but it must
> >>>>> not be used? Is it debug only?
> >>>>>
> >>>>> Anyway, all members of the Object class are under /*< private
> >>>>>> */ so they should not be accesses in sPAPR code, I believe.
> >>> Ah, good point, I missed that.  I guess we have to keep the owner
> >>> field, redundant though it seems.  Blech.
> >>
> >> I think the comment is wrong or at least inaccurate; it only applies
> >> to the external QOM interface.
> > 
> > Is this case external?
> 
> I meant external as in qom-get, qom-set, qom-list.  There isn't a ".."
> property.
> 
> > Originally I was looking for a object_get_parent() but it is not there
> > so I decided that the comment is correct or I just fail to understand it :)
> 
> Yes, we can add such an API.
> 
> Let's look also at what ->owner is used for.
> 
> > object_property_add_alias(root_container, link_name,
> >                           drc->owner, child_name, &err);
> 
> This can be rewritten as
> 
>      object_property_add_const_link(root_container, link_name,
>                                     drc, &err);
> 
> >     QTAILQ_FOREACH(prop, &root_container->properties, node) {
> >         Object *obj;
> >         sPAPRDRConnector *drc;
> >         sPAPRDRConnectorClass *drck;
> >         uint32_t drc_index, drc_power_domain;
> > 
> >         if (!strstart(prop->type, "link<", NULL)) {
> >             continue;
> >         }
> > 
> >         obj = object_property_get_link(root_container, prop->name, NULL);
> >         drc = SPAPR_DR_CONNECTOR(obj);
> >         drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > 
> >         if (owner && (drc->owner != owner)) {
> 
> Could the PCI host bridge instead store the DR connectors when it
> creates them with spapr_dr_connector_new?  Then you can just call
> spapr_drc_populate_dt directly with the right objects, and avoid another
> O(n^2) loop.

It could be done I think, but in some cases the DRC lists can include a mix
of DRC types/indexes from multiple parents, so we'd need to add some logic
to modify/append to existing paths in FDT.

We end up needing a global registry of some sort anyway, since RTAS
calls from the guest address the DRCs purely by the DRC index, so I
think if we can make use of that same registry to simply the above case
there's no reason not to.

> 
> Paolo
>