From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38508) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YSedG-0003fc-1g for qemu-devel@nongnu.org; Mon, 02 Mar 2015 23:40:35 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YSed9-0003MA-Ru for qemu-devel@nongnu.org; Mon, 02 Mar 2015 23:40:33 -0500 Received: from e9.ny.us.ibm.com ([32.97.182.139]:47199) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YSed9-0003Ln-MV for qemu-devel@nongnu.org; Mon, 02 Mar 2015 23:40:27 -0500 Received: from /spool/local by e9.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 2 Mar 2015 23:40:26 -0500 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable From: Michael Roth In-Reply-To: <20150302070246.GH29409@voom.fritz.box> References: <1425006675-19976-1-git-send-email-mdroth@linux.vnet.ibm.com> <1425006675-19976-8-git-send-email-mdroth@linux.vnet.ibm.com> <20150302070246.GH29409@voom.fritz.box> Message-ID: <20150303044016.27171.3218@loki> Date: Mon, 02 Mar 2015 22:40:16 -0600 Subject: Re: [Qemu-devel] [PATCH v6 07/15] spapr_rtas: add ibm, configure-connector RTAS interface List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson Cc: aik@ozlabs.ru, qemu-devel@nongnu.org, agraf@suse.de, ncmike@ncultra.org, qemu-ppc@nongnu.org, tyreld@linux.vnet.ibm.com, bharata.rao@gmail.com, nfont@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'/buff= er > > 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 tra= cked 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 > = > = > 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 =3D drc->ccstate; > = > if (!ccstate) { > /* Haven't started configuring yet */ > ccstate =3D malloc(...); > /* Retrieve the dt fragment from the backend */ > ccstate->fdt =3D drck->get_dt(...); > ccstate->offset =3D 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/79ce372743da1b63a6fa33e3de1f1daba= 8ea1fdc 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 narg= s, > > + target_ulong args, uint32_t n= ret, > > + 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 =3D NULL; > > + char *prop_name =3D NULL; > > + int prop_len, rc; > > + > > + if (nargs !=3D 2 || nret !=3D 1) { > > + rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); > > + return; > > + } > > + > > + wa_addr =3D ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(args, 0); > > + > > + drc_index =3D rtas_ld(wa_addr, 0); > > + drc =3D spapr_dr_connector_by_index(drc_index); > > + if (!drc) { > > + DPRINTF("rtas_ibm_configure_connector: invalid sensor/DRC inde= x: %xh\n", > > + drc_index); > > + rc =3D RTAS_OUT_PARAM_ERROR; > > + goto out; > > + } > > + drck =3D SPAPR_DR_CONNECTOR_GET_CLASS(drc); > > + resp =3D drck->configure_connector(drc, &prop_name, &prop, &prop_l= en); > > + > > + switch (resp) { > > + case SPAPR_DR_CC_RESPONSE_NEXT_CHILD: > > + /* provide the name of the next OF node */ > > + wa_offset =3D 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_offs= et, > > + (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 =3D 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_offs= et, > > + (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 +=3D 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_offs= et, > > + (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 =3D 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-c= onnector", > > + 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_ _othe= r_ > | _way_ _around_! > http://www.ozlabs.org/~dgibson