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:
> > > 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>
> > > ---
> > > hw/ppc/spapr_rtas.c | 81
> > > +++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > 1 file changed, 81 insertions(+)
> > >
> > > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> > > index f80beb2..b8551b0 100644
> > > --- a/hw/ppc/spapr_rtas.c
> > > +++ b/hw/ppc/spapr_rtas.c
> > > @@ -418,6 +418,85 @@ static void rtas_get_sensor_state(PowerPCCPU *cpu,
> > > sPAPREnvironment *spapr,
> > > rtas_st(rets, 1, entity_sense);
> > > }
> > >
> > > +/* configure-connector work area offsets, int32_t units for field
> > > + * indexes, bytes for field offset/len values.
> > > + *
> > > + * as documented by PAPR+ v2.7, 13.5.3.5
> > > + */
> > > +#define CC_IDX_NODE_NAME_OFFSET 2
> > > +#define CC_IDX_PROP_NAME_OFFSET 2
> > > +#define CC_IDX_PROP_LEN 3
> > > +#define CC_IDX_PROP_DATA_OFFSET 4
> > > +#define CC_VAL_DATA_OFFSET ((CC_IDX_PROP_DATA_OFFSET + 1) * 4)
> > > +#define CC_WA_LEN 4096
> > > +
> > > +static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
> > > + sPAPREnvironment *spapr,
> > > + uint32_t token, uint32_t nargs,
> > > + target_ulong args, uint32_t
> > > nret,
> > > + target_ulong rets)
> > > +{
> > > + uint64_t wa_addr = ((uint64_t)rtas_ld(args, 1) << 32) |
> > > rtas_ld(args, 0);
> >
> > You need to check nargs and nret first.
>
> Could've sworn I'd fixed all those cases. Will make sure to take care of it on
> the next one.
>
> >
> > > + 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.
--
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
pgpBSpmJ5FhpP.pgp
Description: PGP signature
