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

[Michael Roth <mdroth@linux.vnet.ibm.com>]

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!

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