From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58735) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YQQZi-0000zv-M1 for qemu-devel@nongnu.org; Tue, 24 Feb 2015 20:15:46 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YQQZU-0006ma-IF for qemu-devel@nongnu.org; Tue, 24 Feb 2015 20:15:39 -0500 Date: Wed, 25 Feb 2015 11:48:23 +1100 From: David Gibson Message-ID: <20150225004823.GA15695@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> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="J/dobhs11T7y2rNN" Content-Disposition: inline In-Reply-To: <20150224204345.31752.42274@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 --J/dobhs11T7y2rNN Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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: > > > This interface is used to fetch an OF device-tree nodes that describe= s a > > > newly-attached device to guest. It is called multiple times to walk t= he > > > device-tree node and fetch individual properties into a 'workarea'/bu= ffer > > > provided by the guest. > > >=20 > > > The device-tree is generated by QEMU and passed to an sPAPRDRConnecto= r during > > > the initial hotplug operation, and the state of these RTAS calls is t= racked by > > > the sPAPRDRConnector. When the last of these properties is successful= ly > > > fetched, we report as special return value to the guest and transition > > > the device to a 'configured' state on the QEMU/DRC side. > > >=20 > > > See docs/specs/ppc-spapr-hotplug.txt for a complete description of > > > this interface. > > >=20 > > > Signed-off-by: Michael Roth > > > --- > > > hw/ppc/spapr_rtas.c | 81 +++++++++++++++++++++++++++++++++++++++++++= ++++++++++ > > > 1 file changed, 81 insertions(+) > > >=20 > > > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c > > > index f80beb2..b8551b0 100644 > > > --- a/hw/ppc/spapr_rtas.c > > > +++ b/hw/ppc/spapr_rtas.c > > > @@ -418,6 +418,85 @@ static void rtas_get_sensor_state(PowerPCCPU *cp= u, sPAPREnvironment *spapr, > > > rtas_st(rets, 1, entity_sense); > > > } > > > =20 > > > +/* configure-connector work area offsets, int32_t units for field > > > + * indexes, bytes for field offset/len values. > > > + * > > > + * as documented by PAPR+ v2.7, 13.5.3.5 > > > + */ > > > +#define CC_IDX_NODE_NAME_OFFSET 2 > > > +#define CC_IDX_PROP_NAME_OFFSET 2 > > > +#define CC_IDX_PROP_LEN 3 > > > +#define CC_IDX_PROP_DATA_OFFSET 4 > > > +#define CC_VAL_DATA_OFFSET ((CC_IDX_PROP_DATA_OFFSET + 1) * 4) > > > +#define CC_WA_LEN 4096 > > > + > > > +static void rtas_ibm_configure_connector(PowerPCCPU *cpu, > > > + sPAPREnvironment *spapr, > > > + uint32_t token, uint32_t na= rgs, > > > + target_ulong args, uint32_t= nret, > > > + target_ulong rets) > > > +{ > > > + uint64_t wa_addr =3D ((uint64_t)rtas_ld(args, 1) << 32) | rtas_l= d(args, 0); > >=20 > > You need to check nargs and nret first. >=20 > Could've sworn I'd fixed all those cases. Will make sure to take care of = it on > the next one. >=20 > >=20 > > > + 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/DRC in= dex: %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 transitions > 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/device > 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 immedi= ate > device_del, but since the 'configured' transition must occur before the l= atter, > it becomes unambiguous. >=20 > It's also possible that a guest might be reset in the middle of a series = of > calls to configure-connector, in which case that state needs to be reset.= 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 configure-co= nnector > state, which is kinda messy. We'd also need a list of some sort, keyed by > the DRC indexes, to handle the subsequent call-per-iteration's (no guaran= tee > only one device configuration is 'in-flight' at a time), so we end up > duplicating a lot of tracking/functionality. 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 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 --J/dobhs11T7y2rNN Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJU7RvXAAoJEGw4ysog2bOScqsQALjQp5IxcVhXnjaJHtkOYj8z yYqYNGU0Pi/dywBfaK7s4mzBxYhSmqaAtEkIUxuttMjNM/1yhDMSH9QVXpg9OVym utbZErMbU4wB/tX4+Kmki/cRrowXvK3YdjgcY9C/v57wqSEuvA74QxG9yVpoVU9g 0JqAVKstWm4wTveVk9KvzIQvw4uWqoGMHEzKBs80z3us+QzgqfQc5aqqWs3vGWo8 7A3K3HdfeKaAZJvV0R5NYQhoCWNAuOwdBDZitTERldC4SpcQOmWLutDy/FS/snyq PmIyWIWEk146hJ494VOuHD6/mAul0k5LmtYLD2mBXXzQZ4o2k752QkITVOt8Ea7I VZx5CXeBR4+yNB9C16Fhl5xS4+vx6AlyVt0mJuYc7c20RF1VuFlKTKNhKfgHbvik 70ESEkShUw8srFoaG2L487CQz/m8Of8Y/UY4PYWB7WnCn+rE8j+c2/DwpkfgWZfF RGD6BM1q7yUx5KLdDUsndaMlyMiuge+SWqPJYIw1JfpMF7EqJzUEIVI+wbU1LrQQ v9knjFRAwynf0COE54+TxLbLJh10j2nr5w+SwahLmZVpwVEKWXRNiiaxQV3FD/3Q IHaNlE2wAvHElKeELfyT/ZMRAvnQ3vxZqCbL/AJQPHIy4a4Kf18gbgFR5NFGBpkg XhDPOEM/NktcHJRYA/HR =dnhw -----END PGP SIGNATURE----- --J/dobhs11T7y2rNN--