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