qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Michael Roth
Subject: Re: [Qemu-devel] [RFCv2 1/2] spapr: Remove unnecessary owner field from sPAPRDRConnector
Date: Thu, 17 Sep 2015 10:50:53 -0500
User-agent: alot/0.3.6

Quoting David Gibson (2015-09-15 22:16:35)
> On Mon, Sep 14, 2015 at 04:24:23PM +0200, Paolo Bonzini wrote:
> > 
> > 
> > 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.
> 
> So, yes, iterating over the connectors under the owner rather than
> going via the global links is another cleanup I've considered but
> haven't gotten around to.

We still need globals for RTAS lookups. I think QOM is our most
mature/well-tested interface for managing inter-device
relationships/lookups, but I can understand if using
root_container/link<DRC> seems off. To me it seem like a nice
"freebie" we get from using QOM, and gave us nice guarantees like
globally unique paths to correspond to globally unique DRC indexes.

But it's a simple enough task that it can live elsewhere. It will
still be global though (RTAS + device_add + FDT generation all
needs to reference it), and I don't think adding more global
interfaces to do things already provided by QOM is worth it.

> 
> Except, I've been thinking further, and I'm not sure it makes sense to
> keep these DR connectors around as full QOM objects anyway.  Having
> them here at least potentially exposes the DR connector stuff to
> users, when it's really an internal of how the hypervisor communicates
> with the guest about hotplug.

I think QOM should manage a machine's composition tree. The fact that
there's a side-effect of exposing things to users shouldn't prevent
us from making use of it. We expose all sorts of state about gpio
pins, memory regions, etc. currently. If there's a need to hide things
from external users, or better manage things like read/write permissions,
I think that logic should be added to QOM rather than used as an argument
against it.

Whether the current composition tree makes sense is tricky to answer.
pseries is guest-only, so everything is sort of a hypervisor internal,
but if a theoretical baremetal pseries existed, I think you could easily
end up with, in the case of PCI anyway, an SHPC-like device that's wired
up to a bus of some sort, addressed by DRC index, with registers to set
indicator/isolation states and internal logic to control state
transitions like SHPC has.

SHPCs are exposed to guests via PCI bus, but pseries calls for RTAS,
and SHPCs are modeled as an internal of the bus controller, whereas
things like memory/cpu don't really have a bus we can piggy-back on,
so we have something a little ad-hoc where DRCs are modeled as
self-contained devices.

FWIW, the initial implementation of hotplug did not use QOM, and was
rewritten to use it at the suggestion of Alex. I think the pieces
fall into place much more logically taking the latter approach and
modeling DRCs as devices. The alternative is to make DRC an internal
detail of PCI buses, and non-existant memory/cpu buses, which I think
will ultimately result is self-contained 'devices' that simple aren't
present in the composition tree. As a result I think things like
managing DRC creation/teardown (think PHB hotplug of hotplug capable
PCI buses) will become more tedious/ad-hoc since we lose the
parent/child life-cycle management that QOM gives us for free.

> 
> I'm going to have to talk to more QOM-experienced people to thrash out
> the details, but I'm thinking a better approach would be to add a "DR
> connector array" QOM interface to the existing PCI host bridge objects
> and.. to something for CPU and memory.  That interface would allow the
> necessary lookups specifically for DR hotplug events and avoid
> creating thousands of extra QOM objects.

qdev can create up to 256*32 under /machine/unattached for PCI machines,
and for pseries as many as 256*32*32. It would've likely even done so
using the "[*]" interface if that was around at the time (but it does use
it for gpio's). I don't think the number of objects should limit what we
use QOM for (and don't think it does really, it's just the "[*]"
implementation that seems to be problematic)

> 
> Either way, it makes this little cleanup pretty irrelevant, so I'll
> drop it for now.
> 
> -- 
> David Gibson                    | I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
>                                 | _way_ _around_!
> http://www.ozlabs.org/~dgibson




reply via email to

[Prev in Thread] Current Thread [Next in Thread]