From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59218) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YTSnh-0001eL-1E for qemu-devel@nongnu.org; Thu, 05 Mar 2015 05:14:42 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YTSnd-0002vB-Ux for qemu-devel@nongnu.org; Thu, 05 Mar 2015 05:14:40 -0500 Date: Thu, 5 Mar 2015 15:30:40 +1100 From: David Gibson Message-ID: <20150305043040.GL18072@voom.fritz.box> References: <1425006675-19976-1-git-send-email-mdroth@linux.vnet.ibm.com> <1425006675-19976-8-git-send-email-mdroth@linux.vnet.ibm.com> <20150302070246.GH29409@voom.fritz.box> <20150303044016.27171.3218@loki> <20150303053339.GN29409@voom.fritz.box> <20150304055034.27171.34590@loki> <20150304133708.27171.49509@loki> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="V3eawNQxI9TAjvgi" Content-Disposition: inline In-Reply-To: <20150304133708.27171.49509@loki> Subject: Re: [Qemu-devel] [PATCH v6 07/15] 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 --V3eawNQxI9TAjvgi Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Mar 04, 2015 at 07:37:08AM -0600, Michael Roth wrote: > Quoting Michael Roth (2015-03-03 23:50:34) > > Quoting David Gibson (2015-03-02 23:33:39) > > > On Mon, Mar 02, 2015 at 10:40:16PM -0600, Michael Roth wrote: > > > > Quoting David Gibson (2015-03-02 01:02:46) > > > > > On Thu, Feb 26, 2015 at 09:11:07PM -0600, Michael Roth wrote: > > > > > > This interface is used to fetch an OF device-tree nodes that de= scribes a > > > > > > newly-attached device to guest. It is called multiple times to = walk the > > > > > > device-tree node and fetch individual properties into a 'workar= ea'/buffer > > > > > > provided by the guest. > > > > > >=20 > > > > > > The device-tree is generated by QEMU and passed to an sPAPRDRCo= nnector during > > > > > > the initial hotplug operation, and the state of these RTAS call= s is tracked by > > > > > > the sPAPRDRConnector. When the last of these properties is succ= essfully > > > > > > fetched, we report as special return value to the guest and tra= nsition > > > > > > 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 > > > > >=20 > > > > >=20 > > > > > So, actually, here's probably the best place to explain what I ha= d in > > > > > mind for changing the internal interface for this stuff. I was > > > > > thinking something like this pseudocode: > > > > >=20 > > > > > struct DRCCCState { > > > > > void *fdt; > > > > > int offset; > > > > > int depth; > > > > > }; > > > > >=20 > > > > > rtas_configure_connector() > > > > > { > > > > > ... > > > > > DRCCCState *ccstate; > > > > > ... > > > > >=20 > > > > > /* check parameters, retrieve drc */ > > > > > ccstate =3D drc->ccstate; > > > > >=20 > > > > > if (!ccstate) { > > > > > /* Haven't started configuring yet */ > > > > > ccstate =3D malloc(...); > > > > > /* Retrieve the dt fragment from the backend */ > > > > > ccstate->fdt =3D drck->get_dt(...); > > > > > ccstate->offset =3D 0; > > > > > } > > > > >=20 > > > > > while (get next tag from fdt) { > > > > > switch (tag) > > > > > case FDT_PROPERTY: > > > > > /* Translate property into rtas return va= lues */ > > > > > return SPAPR_DR_CC_RESPONSE_NEXT_PROPERTY; > > > > >=20 > > > > > /* other cases ... */ > > > > > } > > > > > =20 > > > > > /* Fall through only if we've completed streaming out the= dt > > > > > */ > > > > >=20 > > > > > /* Tell the back end we've finished configuring */ > > > > > drck->cc_completed(...); > > > > > return SPAPR_DR_CC_RESPONSE_SUCCESS; > > > > > } > > > > >=20 > > > > > On reset, or anything else which interrupts the configuration pro= cess, > > > > > just blow away drc->ccstate. > > > >=20 > > > > Ok, that seems reasonable. I took a stab at it here: > > > >=20 > > > > https://github.com/mdroth/qemu/commit/79ce372743da1b63a6fa33e3d= e1f1daba8ea1fdc > > > > https://github.com/mdroth/qemu/commits/spapr-hotplug-pci > > >=20 > > > It's looking pretty close now, thanks for the rework. > > >=20 > > > > It exposes the ccstate as you suggested, via drck->get_cc_state(), = and in > > > > place of drck->cc_completed() I have drck->set_configured() which s= erves > > > > roughly the same purpose I think. I opted not to let RTAS handle > > > > allocation, since it seemed to imply RTAS owns it and not the DRC. > > >=20 > > > So, that was intentional; basically RTAS *does* own the CCstate. But > > > for convenience of index we need connect it to the DRC. Think of it > > > like an rtas_priv field in the DRC. > > >=20 > > > In particular I think the CCstate should be opaque to everything > > > except the RTAS code itself, which means initializing the offset and > > > depth in RTAS, not in a drck callback. As far as the drck callback > > > is concerned, it's supplying a dt fragment, but it doesn't care about > > > the details of how the upper layer communicates that through to the > > > guest. > >=20 > > Ah ok, so it was about moving the CCState out of DRC, and not just the > > awkward interface that wraps FDT traversal. So I went ahead and did it > > as you suggested, but also making it actually opaque, and relying on > > a couple callbacks that configure-connector passes to > > drc->begin_configure_connector to handle init/reset of the CCState > > fields (such as the fdt, and the start offset (which isn't necessarilly= 0)): > >=20 > > https://github.com/mdroth/qemu/commits/spapr-hotplug-pci > > https://github.com/mdroth/qemu/commit/732aa10fa2e41951c396373e7df7d31= 861322531 > >=20 > > I think I have all your other comments addressed, so if that looks ok > > I'll post v7 soon. Thanks! >=20 > Yikes, just noticed a use-after-free in the new code. Fixed here: >=20 > https://github.com/mdroth/qemu/commit/3fd03f649dc5cd34aa6e2544d38855dd0= f8b3708 Ok, I'm now getting myself a bit tangled in the various revisions. However looking at https://github.com/mdroth/qemu/commit/732aa10fa2e41951c396373e7df7d31861322= 531 The ->begin_configure_connector stuff seems unnecessarily complicated. Couldn't you just have begin_configure_connector() return the fdt, then initialize ccs in rtas_ibm_configure_connector() itself, avoiding the callback-from-a-callback. I'm also not sure that reset_ccs is worth abstracting. I think it would be reasonable just to say that freeing and setting to NULL the ccs link is sufficient. That said, the current reset_ccs doesn't appear to be quite right, since it frees the ccs structure, but not the fdt fragment it points to. I'm not sure how awkward it would be to force them into a common allocation to avoid that. --=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 --V3eawNQxI9TAjvgi Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJU99vwAAoJEGw4ysog2bOSFg8QAJxtxZC4BNbHUEAdJYKxR/CP 1rosh7ykwVpB6v8b1F1QQ4YFhwXcbs4m9DfsqZFFm9OpgUT7dISTRlkH678oc13T 1TG9PM9k3U1xIHToikz7H07UU0p71m+lzyP1ymBH4Nqrsm9fb8Fz0wSE+M52rpk4 4kDu2AnsiaNNf5VDF1ccY7sb9Zk2s8pth6QdU4j0p95BzbF+qz4bdmZBmiPnVs5l /JoJrCn4kViNWCJclF6YC90Q2yxe+HzUZa4ESZeq/e+HdVrEPIPULO06PnvS5sry kvq1n4xYuML6mdkEgTKsj8A1qhr4gaDPWZTGxzNNe2B6OeQNCQyC8rCvmLlHiogZ FytmQaf0kT2Q2t6giMfvTNy1v7OcNBgIICbPgX1BhLshg2MbEP5VtnM3rqQPOMye SudEQu7igeP4wpMEEOEPkjlyvZ9wQaSwz4KxzANutvkQvvkrFe84RRH9i3/8NtwM fK0Z4gbLIQHWX2kwsdL0jPiSf5vcpVi6dvJo2N2OufcfN+hIqmD4kRNrrYSNZKIl s2fhBvIrKSKThOxUJKK1R3ltFHnEsh4DZ4x+dcQa/8uhisZ/Ib6XebEZFEDt+bgS 1prUcMElfl6fkzlP8ifB1aA74dnS5oMTh0XPNJdoXQsv9SkQVn7e689vY7NHxkSh XG7Z2EhLYoCjwRJIYLKf =d8S3 -----END PGP SIGNATURE----- --V3eawNQxI9TAjvgi--