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 <mdr...@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
