Re: [Qemu-devel] [PATCH v6 07/15] spapr_rtas: add ibm, configure-connector RTAS interface

[Michael Roth <mdroth@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 describes a
> > > > > newly-attached device to guest. It is called multiple times to walk 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 sPAPRDRConnector 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.
> > > > > 
> > > > > See docs/specs/ppc-spapr-hotplug.txt for a complete description of
> > > > > this interface.
> > > > > 
> > > > > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > > > 
> > > > 
> > > > 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 = drc->ccstate;
> > > > 
> > > >         if (!ccstate) {
> > > >                 /* Haven't started configuring yet */
> > > >                 ccstate = malloc(...);
> > > >                 /* Retrieve the dt fragment from the backend */
> > > >                 ccstate->fdt = drck->get_dt(...);
> > > >                 ccstate->offset = 0;
> > > >         }
> > > > 
> > > >         while (get next tag from fdt) {
> > > >                 switch (tag)
> > > >                 case FDT_PROPERTY:
> > > >                         /* Translate property into rtas return values */
> > > >                         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 process,
> > > > just blow away drc->ccstate.
> > > 
> > > Ok, that seems reasonable. I took a stab at it here:
> > > 
> > >     https://github.com/mdroth/qemu/commit/79ce372743da1b63a6fa33e3de1f1daba8ea1fdc
> > >     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(), and in
> > > place of drck->cc_completed() I have drck->set_configured() which 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 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/732aa10fa2e41951c396373e7df7d31861322531
> 
> 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/3fd03f649dc5cd34aa6e2544d38855dd0f8b3708

> 
> > 
> > -- 
> > 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