From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42575) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YT9UO-0002Iv-DS for qemu-devel@nongnu.org; Wed, 04 Mar 2015 08:37:29 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YT9UG-0004DZ-G8 for qemu-devel@nongnu.org; Wed, 04 Mar 2015 08:37:28 -0500 Sender: fluxion Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable From: Michael Roth In-Reply-To: <20150304055034.27171.34590@loki> 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> Message-ID: <20150304133708.27171.49509@loki> Date: Wed, 04 Mar 2015 07:37:08 -0600 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: David Gibson 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 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 desc= ribes a > > > > > newly-attached device to guest. It is called multiple times to wa= lk the > > > > > device-tree node and fetch individual properties into a 'workarea= '/buffer > > > > > provided by the guest. > > > > > = > > > > > The device-tree is generated by QEMU and passed to an sPAPRDRConn= ector during > > > > > the initial hotplug operation, and the state of these RTAS calls = is tracked by > > > > > the sPAPRDRConnector. When the last of these properties is succes= sfully > > > > > fetched, we report as special return value to the guest and trans= ition > > > > > the device to a 'configured' state on the QEMU/DRC side. > > > > > = > > > > > See docs/specs/ppc-spapr-hotplug.txt for a complete description of > > > > > this interface. > > > > > = > > > > > Signed-off-by: Michael Roth > > > > = > > > > = > > > > So, actually, here's probably the best place to explain what I had = in > > > > mind for changing the internal interface for this stuff. I was > > > > thinking something like this pseudocode: > > > > = > > > > struct DRCCCState { > > > > void *fdt; > > > > int offset; > > > > int depth; > > > > }; > > > > = > > > > rtas_configure_connector() > > > > { > > > > ... > > > > DRCCCState *ccstate; > > > > ... > > > > = > > > > /* check parameters, retrieve drc */ > > > > ccstate =3D drc->ccstate; > > > > = > > > > 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; > > > > } > > > > = > > > > while (get next tag from fdt) { > > > > switch (tag) > > > > case FDT_PROPERTY: > > > > /* Translate property into rtas return valu= es */ > > > > return SPAPR_DR_CC_RESPONSE_NEXT_PROPERTY; > > > > = > > > > /* other cases ... */ > > > > } > > > > = > > > > /* Fall through only if we've completed streaming out the dt > > > > */ > > > > = > > > > /* Tell the back end we've finished configuring */ > > > > drck->cc_completed(...); > > > > return SPAPR_DR_CC_RESPONSE_SUCCESS; > > > > } > > > > = > > > > On reset, or anything else which interrupts the configuration proce= ss, > > > > just blow away drc->ccstate. > > > = > > > Ok, that seems reasonable. I took a stab at it here: > > > = > > > https://github.com/mdroth/qemu/commit/79ce372743da1b63a6fa33e3de1= f1daba8ea1fdc > > > https://github.com/mdroth/qemu/commits/spapr-hotplug-pci > > = > > It's looking pretty close now, thanks for the rework. > > = > > > It exposes the ccstate as you suggested, via drck->get_cc_state(), an= d in > > > place of drck->cc_completed() I have drck->set_configured() which ser= ves > > > 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. > > = > > 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. > > = > > 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. > = > 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= )): > = > https://github.com/mdroth/qemu/commits/spapr-hotplug-pci > https://github.com/mdroth/qemu/commit/732aa10fa2e41951c396373e7df7d3186= 1322531 > = > I think I have all your other comments addressed, so if that looks ok > I'll post v7 soon. Thanks! Yikes, just noticed a use-after-free in the new code. Fixed here: https://github.com/mdroth/qemu/commit/3fd03f649dc5cd34aa6e2544d38855dd0f8= b3708 > = > > = > > -- = > > David Gibson | I'll have my music baroque, and my co= de > > david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _ot= her_ > > | _way_ _around_! > > http://www.ozlabs.org/~dgibson