From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46589) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZcbTb-0006Vf-AM for qemu-devel@nongnu.org; Thu, 17 Sep 2015 11:52:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZcbTY-00053d-1N for qemu-devel@nongnu.org; Thu, 17 Sep 2015 11:51:59 -0400 Received: from e31.co.us.ibm.com ([32.97.110.149]:38113) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZcbTX-00053R-P0 for qemu-devel@nongnu.org; Thu, 17 Sep 2015 11:51:55 -0400 Received: from /spool/local by e31.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 17 Sep 2015 09:51:55 -0600 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable From: Michael Roth In-Reply-To: <20150916031635.GY2547@voom.fritz.box> References: <1442194913-26545-1-git-send-email-david@gibson.dropbear.id.au> <1442194913-26545-2-git-send-email-david@gibson.dropbear.id.au> <55F683CB.6000508@ozlabs.ru> <20150914114503.GP2547@voom.fritz.box> <55F6B989.8010900@redhat.com> <55F6D479.4010301@ozlabs.ru> <55F6D897.1090100@redhat.com> <20150916031635.GY2547@voom.fritz.box> Message-ID: <20150917155053.27212.55263@loki> Date: Thu, 17 Sep 2015 10:50:53 -0500 Subject: Re: [Qemu-devel] [RFCv2 1/2] spapr: Remove unnecessary owner field from sPAPRDRConnector List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson , Paolo Bonzini Cc: Alexey Kardashevskiy , qemu-devel@nongnu.org, qemu-ppc@nongnu.org, agraf@suse.de, bharata@linux.vnet.ibm.com 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: > > >>>>> > > >>>>> =3D=3D=3D * There is no way for a child to determine what its par= ent > > >>>>> is. It is not * a bidirectional relationship. This is by > > >>>>> design. =3D=3D=3D > > >>>>> > > >>>>> 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 =3D object_property_get_link(root_container, prop->name, = NULL); > > > drc =3D SPAPR_DR_CONNECTOR(obj); > > > drck =3D SPAPR_DR_CONNECTOR_GET_CLASS(drc); > > > = > > > if (owner && (drc->owner !=3D 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 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_ _othe= r_ > | _way_ _around_! > http://www.ozlabs.org/~dgibson