From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53726) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YVzRh-0001KP-Gw for qemu-devel@nongnu.org; Thu, 12 Mar 2015 05:30:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YVzRa-0001Ex-Ls for qemu-devel@nongnu.org; Thu, 12 Mar 2015 05:30:25 -0400 Date: Thu, 12 Mar 2015 16:52:10 +1100 From: David Gibson Message-ID: <20150312055210.GT11973@voom.redhat.com> 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> <20150305043040.GL18072@voom.fritz.box> <20150305141258.2674.35847@loki> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="TPGKQYD3WP7vMswh" Content-Disposition: inline In-Reply-To: <20150305141258.2674.35847@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 --TPGKQYD3WP7vMswh Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Mar 05, 2015 at 08:12:58AM -0600, Michael Roth wrote: > Quoting David Gibson (2015-03-04 22:30:40) > > 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 tha= t describes a > > > > > > > > newly-attached device to guest. It is called multiple times= to walk the > > > > > > > > device-tree node and fetch individual properties into a 'wo= rkarea'/buffer > > > > > > > > provided by the guest. > > > > > > > >=20 > > > > > > > > The device-tree is generated by QEMU and passed to an sPAPR= DRConnector during > > > > > > > > the initial hotplug operation, and the state of these RTAS = calls is tracked by > > > > > > > > the sPAPRDRConnector. When the last of these properties is = successfully > > > > > > > > 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 descrip= tion of > > > > > > > > this interface. > > > > > > > >=20 > > > > > > > > Signed-off-by: Michael Roth > > > > > > >=20 > > > > > > >=20 > > > > > > > So, actually, here's probably the best place to explain what = I had in > > > > > > > mind for changing the internal interface for this stuff. I w= as > > > > > > > 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 retur= n values */ > > > > > > > return SPAPR_DR_CC_RESPONSE_NEXT_PROP= ERTY; > > > > > > >=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= process, > > > > > > > just blow away drc->ccstate. > > > > > >=20 > > > > > > Ok, that seems reasonable. I took a stab at it here: > > > > > >=20 > > > > > > https://github.com/mdroth/qemu/commit/79ce372743da1b63a6fa3= 3e3de1f1daba8ea1fdc > > > > > > 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() whi= ch serves > > > > > > 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 D= RC. > > > > >=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 callba= ck > > > > > is concerned, it's supplying a dt fragment, but it doesn't care a= bout > > > > > the details of how the upper layer communicates that through to t= he > > > > > 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 necessar= illy 0)): > > > >=20 > > > > https://github.com/mdroth/qemu/commits/spapr-hotplug-pci > > > > https://github.com/mdroth/qemu/commit/732aa10fa2e41951c396373e7df= 7d31861322531 > > > >=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/3fd03f649dc5cd34aa6e2544d3885= 5dd0f8b3708 > >=20 > > Ok, I'm now getting myself a bit tangled in the various revisions. > > However looking at > >=20 > > https://github.com/mdroth/qemu/commit/732aa10fa2e41951c396373e7df7d3186= 1322531 > >=20 > > 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. >=20 > We need the fdt, as well as the fdt starting offset, to initialize the CC= S. Do you actually have a use-case for a non-zero starting offset? Or could you simplify by having the individual PCI device always create its fdt fragment at offset 0. > I think it's a matter a of taste whether that's those are returned separa= tely, > or through a callback passed via begin_configure_connector. The approach I > took just seemed a bit more instructive about what data was needed, > and why. > drck->get_fdt() and drck->get_fdt_starting_offset() instead of the > callback seemed a bit much too specific in purpose to warrant a general > interface, and it since we seem to need a reset_ccs anyway (see below), > init_ccs seemed like a good place to contain those values. Um.. I'm a bit confused by this. You could return both the fdt pointert and offset as one call using pointers or a structure return value without needing to invoke a callback-from-a-callback. > I am fine with just initializing ccs via get_fdt()/get_fdt_starting_offse= t() > beforehand though, but I do think we're stuck with a reset_ccs callback > if we're agreed on drck->get_configure_connector_state() =3D=3D NULL being > the primary means to invalidate CCS state. Hm. I'll have to take another look. I'd really like to keep things to a single set of callbacks if possible, rather than having both callbacks and counter-callbacks, or whatever you want to call them. > > 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. >=20 > But after allocation, rtas_configure_connector hands over the ccs link > to DRC, and it's local copy goes out of scope. The only way to retrieve > it is via get_configure_connector_state(), so if the idea is to return > NULL open reset, we have no way to free the ccs structure. If we simply > have DRC free it, we violate the idea that ccs state is opaque. So given > the init_ccs callback above, it made sense to handle the free via a > reset_ccs. >=20 > >=20 > > 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 > You mean freeing the actual FDT data? In this case the FDT pointer is > simply a pointer to the copy the DRC has, and the lifecycle of the FDT > is tied to the device lifecycle, and spans beyond that of a CCS (since > we can configure/unconfigure the same device multiple times without > unplugging in between) Oh, ok. Why do you need a copy in ccstate then? The rtas code has access to the drc structure as well. --=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 --TPGKQYD3WP7vMswh Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJVASmKAAoJEGw4ysog2bOS42QP+gMSlxq2ysJeypooG7o+P4N0 W9bYnDUpFX+9syZ5hUTn4+S7ZcpYjMiUBFeY+7Kwj82Of8vr1MYi4TilWELWHmST UloPMhxIuRMNOXIhYvDrgcznpdna6NQvsIXtPOWa4Va0xG23lqZBrF+9AMEoZ2j4 GlomY2xAiqbqroANEstbAPnhuwJHfX0ulaQkQFvfmGU9NcFbyLHgeLQ7tQQ8AY/m i1/FvrS9RW0/ELVdSQdmdfH/E1I1k+Ppk6b6adfaLQNgo/KojY3bpwVk8gR35GqL 2wCx2udmDQDk8lzAEI7WCQTXWvrVx6zKCu/vhmbEzZ43jUbJb0lk/maEf0UmKUZ/ KY7oZrv3jfu96rNxc/5I8J9IvGWhoqFuuRlJLuFEsInTZ9VtN5dUQ89pofmttKUa y1+fW4PwdQue2eW7jJcF9e+EBmvqGgk0rPj9/z5iVJmGIkyBgDNhz9sSJsT7L+wR Lsg4etnlOH6PueazokgD5nn0+yfWW3AvzsJtHKItFdws2qFdBe06IPSEj5M53ULk hlIh+iYwmNL0e7BPwuK3b1PpZFrpwr04Q0ksH09TDeiUTurcIeaEq4/NCOwV6/ZS 0yzhieJrVDYtYJDGH2NZnWFZrgYN07EAtSvx98GNGp4yVamAPGf1ZchVZCAHOdhn EPD/Bqtvcagv2MN+hoj0 =X3zV -----END PGP SIGNATURE----- --TPGKQYD3WP7vMswh--