From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34971) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YQMLm-0001eh-Io for qemu-devel@nongnu.org; Tue, 24 Feb 2015 15:45:10 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YQMLd-0006p7-Mr for qemu-devel@nongnu.org; Tue, 24 Feb 2015 15:45:02 -0500 Received: from e33.co.us.ibm.com ([32.97.110.151]:37519) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YQMLd-0006or-Ec for qemu-devel@nongnu.org; Tue, 24 Feb 2015 15:44:53 -0500 Received: from /spool/local by e33.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 24 Feb 2015 13:44:52 -0700 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable From: Michael Roth In-Reply-To: <20150224064032.GQ4536@voom.redhat.com> References: <1424096872-29868-1-git-send-email-mdroth@linux.vnet.ibm.com> <1424096872-29868-8-git-send-email-mdroth@linux.vnet.ibm.com> <20150224064032.GQ4536@voom.redhat.com> Message-ID: <20150224204345.31752.42274@loki> Date: Tue, 24 Feb 2015 14:43:45 -0600 Subject: Re: [Qemu-devel] [PATCH v5 07/16] 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-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'/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 > > --- > > 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 narg= s, > > + target_ulong args, uint32_t n= ret, > > + target_ulong rets) > > +{ > > + uint64_t wa_addr =3D ((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 =3D NULL; > > + char *prop_name =3D NULL; > > + int prop_len, rc; > > + > > + 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); > = > 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 lat= ter, 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. T= his 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-conn= ector 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. > = > = > > + 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 +630,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