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

I plan to squash these into v7, but wanted to run the patch by you first.
Let me know if you'd rather I just post v7.

Thanks!

> 
> 
> > ---
> >  hw/ppc/spapr_rtas.c | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 88 insertions(+)
> > 
> > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> > index f80beb2..31ad35f 100644
> > --- a/hw/ppc/spapr_rtas.c
> > +++ b/hw/ppc/spapr_rtas.c
> > @@ -418,6 +418,92 @@ 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 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;
> > +
> > +    if (nargs != 2 || nret != 1) {
> > +        rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> > +        return;
> > +    }
> > +
> > +    wa_addr = ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 0);
> > +
> > +    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);
> > +
> > +    switch (resp) {
> > +    case SPAPR_DR_CC_RESPONSE_NEXT_CHILD:
> > +        /* provide the name of the next OF node */
> > +        wa_offset = CC_VAL_DATA_OFFSET;
> > +        rtas_st(wa_addr, CC_IDX_NODE_NAME_OFFSET, wa_offset);
> > +        rtas_st_buffer_direct(wa_addr + wa_offset, CC_WA_LEN - wa_offset,
> > +                              (uint8_t *)prop_name, strlen(prop_name) + 1);
> > +        break;
> > +    case SPAPR_DR_CC_RESPONSE_NEXT_PROPERTY:
> > +        /* provide the name of the next OF property */
> > +        wa_offset = CC_VAL_DATA_OFFSET;
> > +        rtas_st(wa_addr, CC_IDX_PROP_NAME_OFFSET, wa_offset);
> > +        rtas_st_buffer_direct(wa_addr + wa_offset, CC_WA_LEN - wa_offset,
> > +                              (uint8_t *)prop_name, strlen(prop_name) + 1);
> > +
> > +        /* provide the length and value of the OF property. data gets placed
> > +         * immediately after NULL terminator of the OF property's name string
> > +         */
> > +        wa_offset += strlen(prop_name) + 1,
> > +        rtas_st(wa_addr, CC_IDX_PROP_LEN, prop_len);
> > +        rtas_st(wa_addr, CC_IDX_PROP_DATA_OFFSET, wa_offset);
> > +        rtas_st_buffer_direct(wa_addr + wa_offset, CC_WA_LEN - wa_offset,
> > +                              (uint8_t *)((struct fdt_property *)prop)->data,
> > +                              prop_len);
> > +        break;
> > +    case SPAPR_DR_CC_RESPONSE_PREV_PARENT:
> > +    case SPAPR_DR_CC_RESPONSE_ERROR:
> > +    case SPAPR_DR_CC_RESPONSE_SUCCESS:
> > +        break;
> > +    default:
> > +        /* drck->configure_connector() should not return anything else */
> > +        g_assert(false);
> > +    }
> > +
> > +    rc = resp;
> > +out:
> > +    g_free(prop_name);
> > +    rtas_st(rets, 0, rc);
> > +}
> > +
> >  static struct rtas_call {
> >      const char *name;
> >      spapr_rtas_fn fn;
> > @@ -551,6 +637,8 @@ static void core_rtas_register_types(void)
> >                          rtas_set_indicator);
> >      spapr_rtas_register(RTAS_GET_SENSOR_STATE, "get-sensor-state",
> >                          rtas_get_sensor_state);
> > +    spapr_rtas_register(RTAS_IBM_CONFIGURE_CONNECTOR, "ibm,configure-connector",
> > +                        rtas_ibm_configure_connector);
> >  }
> >  
> >  type_init(core_rtas_register_types)
> 
> -- 
> 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