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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3881 bytes --]

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.

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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]