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

[David Gibson]

On Thu, Feb 26, 2015 at 04:21:55PM -0600, Michael Roth wrote:
> Quoting David Gibson (2015-02-24 18:48:23)
> > On Tue, Feb 24, 2015 at 02:43:45PM -0600, Michael Roth wrote:
> > > Quoting David Gibson (2015-02-24 00:40:32)
> > > > On Mon, Feb 16, 2015 at 08:27:43AM -0600, Michael Roth wrote:
[snip]
> > > > > +    uint64_t wa_offset;
> > > > > +    uint32_t drc_index;
> > > > > +    sPAPRDRConnector *drc;
> > > > > +    sPAPRDRConnectorClass *drck;
> > > > > +    sPAPRDRCCResponse resp;
> > > > > +    const struct fdt_property *prop = NULL;
> > > > > +    char *prop_name = NULL;
> > > > > +    int prop_len, rc;
> > > > > +
> > > > > +    drc_index = rtas_ld(wa_addr, 0);
> > > > > +    drc = spapr_dr_connector_by_index(drc_index);
> > > > > +    if (!drc) {
> > > > > +        DPRINTF("rtas_ibm_configure_connector: invalid sensor/DRC 
> > > > > index: %xh\n",
> > > > > +                drc_index);
> > > > > +        rc = RTAS_OUT_PARAM_ERROR;
> > > > > +        goto out;
> > > > > +    }
> > > > > +    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > > > > +    resp = drck->configure_connector(drc, &prop_name, &prop, 
> > > > > &prop_len);
> > > > 
> > > > You may have answered this last time round, but if so I forgot the
> > > > reason.
> > > > 
> > > > Why does the awkward iteration need to go down to the drck callback?
> > > > Coudln't the drck callback part just supply the fdt fragment blob,
> > > > then have generic code which streams it out via iteration?
> > > > 
> > > > Obviously we have to support the horrid PAPR interface, but it would
> > > > be nice to confine the PAPR derived horridness to as small an area as
> > > > we can.
> > > 
> > > That horrid interface percolates all the way up the QEMU stack,
> > > unfortunately :)
> > > 
> > > Upon successfully having it's device tree node received, a DRC transitions
> > > to a 'configured' state that's defined in the DR state machine (PAPR+ 
> > > 13.4).
> > > 
> > > We need to track that state, since it's used to differentiate between
> > > a case where a device is set to 'isolated' as part of entity-sense/device
> > > configuration, as opposed to 'isolated' as part the unplug path. The
> > > overlap between the 2 can occur if we do device_add followed by an 
> > > immediate
> > > device_del, but since the 'configured' transition must occur before the 
> > > latter,
> > > it becomes unambiguous.
> > > 
> > > It's also possible that a guest might be reset in the middle of a series 
> > > of
> > > calls to configure-connector, in which case that state needs to be reset. 
> > > This
> > > is currently handled by sPAPRDRConnector's reset hook, so if we moved that
> > > aspect out I think we'd need to wire up a reset hook for the 
> > > configure-connector
> > > state, which is kinda messy. We'd also need a list of some sort, keyed by
> > > the DRC indexes, to handle the subsequent call-per-iteration's (no 
> > > guarantee
> > > only one device configuration is 'in-flight' at a time), so we end up
> > > duplicating a lot of tracking/functionality.
> > 
> > Hmm.  You should still be able to handle that with just 2 hooks and 1
> > bit of state right?  Say "start_configuration" and "end_configuration"
> > or similar.  start_configuration gets the blob from the backend, then
> > end_configuration is called once RTAS has finished streaming it out to
> > the guest and updates to the cnofigured state.
> 
> {start,end}_configuration callbacks would work for handling
> the normal 'configured' state transitions induced by the call, but we'd also
> need hooks in the "other direction" for a couple cases:
> 
> This scenario for instance:
> 
>       qemu:                         guest:
>       add device0 to drc0
>                                     drmgr0: rtas-configure-connector drc0
>       drc0->start_configuration...
>                                     drmgr0: rtas-configure-connector drc0
>                                     drmgr0: rtas-configure-connector drc0
>                                     ...
>                                     drmgr0: rtas-configure-connector drc0
>       del device0                   <device0 removal pending>
>                                     drmgr0: rtas-configure-connector drc0
>       system_reset
>       device0 removed by drc0
>         reset hook
>       add device1 to drc0           drmgr0: rtas-configure-connector drc0
>                                     <begins fetching stale FDT>
> 
> So I think we'd need at least a reset hook wired up to the RTAS state.
> 
> It's also possible for the guest to force a transition out of the
> configured state by ISOLATE'ing the device. This can happen in the
> middle of the guests configure-connector calls if there's an error. If
> rtas-configure-connector is the one generating the error, it can anticipate
> this and automatically reset the state, but in some cases the error is
> guest internal: the get_node() in src/drmgr/rtas_calls.c can fail for
> memory allocation errors, or unexpected workarea structure, after which
> point it simply stops calling rtas-configure-connector and ISOLATEs the
> device. If we don't track that, a subsequent unplug/plug could also
> result in stale FDT fragments being sent to the guest.
> 
> So we'd the RTAS state tracking code to provide an interface of some
> sort for the DRC code to call into, which results in a lot of
> duplicated state-tracking.
> 
> It's a bit unintuitive, but those FDT bits are closely coupled to the DRC
> state, so in the end I think that ends up being the most straight-forward
> place to manage them.

Hrm.  I'm still not following.  I mean, I see that the back-end needs
to be aware of the three-valued state: CC-not-called /
CC-calls-in-progress / CC-calls-completed, but I'm not seeing why it
needs to be aware of the individual CC calls.

Could we have instead a set_state() callback of some sort, that
handles both some of the existing architected states like ISOLATED,
and also special states like CC-in-progress?

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

pgpp07gvV_Flp.pgp
Description: PGP signature