From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58479) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YRDnF-0006zE-IZ for qemu-devel@nongnu.org; Fri, 27 Feb 2015 00:48:59 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YRDnE-0002zS-4T for qemu-devel@nongnu.org; Fri, 27 Feb 2015 00:48:57 -0500 Date: Fri, 27 Feb 2015 16:31:20 +1100 From: David Gibson Message-ID: <20150227053120.GD29409@voom.fritz.box> References: <1424096872-29868-1-git-send-email-mdroth@linux.vnet.ibm.com> <1424096872-29868-8-git-send-email-mdroth@linux.vnet.ibm.com> <20150224064032.GQ4536@voom.redhat.com> <20150224204345.31752.42274@loki> <20150225004823.GA15695@voom.fritz.box> <20150226222155.31752.51742@loki> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="F8dlzb82+Fcn6AgP" Content-Disposition: inline In-Reply-To: <20150226222155.31752.51742@loki> Subject: Re: [Qemu-devel] [PATCH v5 07/16] spapr_rtas: add ibm, configure-connector RTAS interface List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Michael Roth Cc: aik@ozlabs.ru, qemu-devel@nongnu.org, agraf@suse.de, ncmike@ncultra.org, qemu-ppc@nongnu.org, tyreld@linux.vnet.ibm.com, bharata.rao@gmail.com, nfont@linux.vnet.ibm.com --F8dlzb82+Fcn6AgP Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Feb 26, 2015 at 04:21:55PM -0600, Michael Roth wrote: > Quoting David Gibson (2015-02-24 18:48:23) > > On Tue, Feb 24, 2015 at 02:43:45PM -0600, Michael Roth wrote: > > > Quoting David Gibson (2015-02-24 00:40:32) > > > > On Mon, Feb 16, 2015 at 08:27:43AM -0600, Michael Roth wrote: [snip] > > > > > + uint64_t wa_offset; > > > > > + uint32_t drc_index; > > > > > + sPAPRDRConnector *drc; > > > > > + sPAPRDRConnectorClass *drck; > > > > > + sPAPRDRCCResponse resp; > > > > > + const struct fdt_property *prop =3D NULL; > > > > > + char *prop_name =3D NULL; > > > > > + int prop_len, rc; > > > > > + > > > > > + drc_index =3D rtas_ld(wa_addr, 0); > > > > > + drc =3D spapr_dr_connector_by_index(drc_index); > > > > > + if (!drc) { > > > > > + DPRINTF("rtas_ibm_configure_connector: invalid sensor/DR= C index: %xh\n", > > > > > + drc_index); > > > > > + rc =3D RTAS_OUT_PARAM_ERROR; > > > > > + goto out; > > > > > + } > > > > > + drck =3D SPAPR_DR_CONNECTOR_GET_CLASS(drc); > > > > > + resp =3D drck->configure_connector(drc, &prop_name, &prop, &= prop_len); > > > >=20 > > > > You may have answered this last time round, but if so I forgot the > > > > reason. > > > >=20 > > > > Why does the awkward iteration need to go down to the drck callback? > > > > Coudln't the drck callback part just supply the fdt fragment blob, > > > > then have generic code which streams it out via iteration? > > > >=20 > > > > Obviously we have to support the horrid PAPR interface, but it would > > > > be nice to confine the PAPR derived horridness to as small an area = as > > > > we can. > > >=20 > > > That horrid interface percolates all the way up the QEMU stack, > > > unfortunately :) > > >=20 > > > Upon successfully having it's device tree node received, a DRC transi= tions > > > to a 'configured' state that's defined in the DR state machine (PAPR+= 13.4). > > >=20 > > > We need to track that state, since it's used to differentiate between > > > a case where a device is set to 'isolated' as part of entity-sense/de= vice > > > configuration, as opposed to 'isolated' as part the unplug path. The > > > overlap between the 2 can occur if we do device_add followed by an im= mediate > > > device_del, but since the 'configured' transition must occur before t= he latter, > > > it becomes unambiguous. > > >=20 > > > It's also possible that a guest might be reset in the middle of a ser= ies of > > > calls to configure-connector, in which case that state needs to be re= set. This > > > is currently handled by sPAPRDRConnector's reset hook, so if we moved= that > > > aspect out I think we'd need to wire up a reset hook for the configur= e-connector > > > state, which is kinda messy. We'd also need a list of some sort, keye= d by > > > the DRC indexes, to handle the subsequent call-per-iteration's (no gu= arantee > > > only one device configuration is 'in-flight' at a time), so we end up > > > duplicating a lot of tracking/functionality. > >=20 > > Hmm. You should still be able to handle that with just 2 hooks and 1 > > bit of state right? Say "start_configuration" and "end_configuration" > > or similar. start_configuration gets the blob from the backend, then > > end_configuration is called once RTAS has finished streaming it out to > > the guest and updates to the cnofigured state. >=20 > {start,end}_configuration callbacks would work for handling > the normal 'configured' state transitions induced by the call, but we'd a= lso > need hooks in the "other direction" for a couple cases: >=20 > This scenario for instance: >=20 > qemu: guest: > add device0 to drc0 > drmgr0: rtas-configure-connector drc0 > drc0->start_configuration... > drmgr0: rtas-configure-connector drc0 > drmgr0: rtas-configure-connector drc0 > ... > drmgr0: rtas-configure-connector drc0 > del device0 > drmgr0: rtas-configure-connector drc0 > system_reset > device0 removed by drc0 > reset hook > add device1 to drc0 drmgr0: rtas-configure-connector drc0 > >=20 > So I think we'd need at least a reset hook wired up to the RTAS state. >=20 > It's also possible for the guest to force a transition out of the > configured state by ISOLATE'ing the device. This can happen in the > middle of the guests configure-connector calls if there's an error. If > rtas-configure-connector is the one generating the error, it can anticipa= te > this and automatically reset the state, but in some cases the error is > guest internal: the get_node() in src/drmgr/rtas_calls.c can fail for > memory allocation errors, or unexpected workarea structure, after which > point it simply stops calling rtas-configure-connector and ISOLATEs the > device. If we don't track that, a subsequent unplug/plug could also > result in stale FDT fragments being sent to the guest. >=20 > So we'd the RTAS state tracking code to provide an interface of some > sort for the DRC code to call into, which results in a lot of > duplicated state-tracking. >=20 > It's a bit unintuitive, but those FDT bits are closely coupled to the DRC > state, so in the end I think that ends up being the most straight-forward > place to manage them. Hrm. I'm still not following. I mean, I see that the back-end needs to be aware of the three-valued state: CC-not-called / CC-calls-in-progress / CC-calls-completed, but I'm not seeing why it needs to be aware of the individual CC calls. Could we have instead a set_state() callback of some sort, that handles both some of the existing architected states like ISOLATED, and also special states like CC-in-progress? --=20 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 --F8dlzb82+Fcn6AgP Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJU8AEoAAoJEGw4ysog2bOSwYcP+waZHl4bypb/N5AB5f3/1Db4 vvXd5ev3PB5xTp/sKF8RjwL618sL4ym94dSQGjPwZMCZHfTEZU98+mCOPr0opWGr BpR/3myPvd6zMBIG6AiqaonYWZrfA6ech7xjpJXCCXu83Xsuk0ZP2VXAzqO2YVpj ND0dOgLIiKw9vhpe743W8ayoftCBFWyG7CfbbV8Imq+oPnE9KsppU6mMEzcvUZDI pWYf3u7U0aiAOgZXppqF33fdg0/czFozIUq6eKNtT8qU46TI7XkSzgHKRIPGsUVi OTbQ7og6JF2/BmvxUTbbazFA1NlzQvt6/IQVuCUJ/Ay2AqTrErlQwxUsy9h4l137 67YRTohuWwGU6kyiCncHY06lq2wGJZGxRKmfCQ4iGGEnjA+DsMY/yQrw5+O8LhXC 9ds6jbGy5CpY2o+GUuX/Qx9I0gnewRXV/LMWHmKSwx48JxLoBhIJvUQBd9mbumkm YZ7SxuYEuZNBXhJIr9qb+RGSN8WFF3JZjtbcrjWhHytHfGgk0d+nkF45BDw7QvbS d226gRaL6aeCZtrVVZOwhw7OIZXyJRHXIbjy38soBTqsGfrtgHI9MsiX8z3xXpt7 vVnmfOQn/dbKKED3YkEuzz9quiMmBWkbF/RqXqEh0afuOV4j8jxFP7dpJVu/tYw/ 7y17rCuTdk/PeCNdnfKL =ZaEr -----END PGP SIGNATURE----- --F8dlzb82+Fcn6AgP--