From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52195) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YD3Fy-000119-42 for qemu-devel@nongnu.org; Sun, 18 Jan 2015 22:44:03 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YD3Fv-0008Qi-S0 for qemu-devel@nongnu.org; Sun, 18 Jan 2015 22:44:02 -0500 Date: Mon, 19 Jan 2015 14:44:08 +1100 From: David Gibson Message-ID: <20150119034408.GQ5297@voom.fritz.box> References: <1419337831-16552-1-git-send-email-mdroth@linux.vnet.ibm.com> <1419337831-16552-8-git-send-email-mdroth@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="o99acAvKqrTZeiCU" Content-Disposition: inline In-Reply-To: <1419337831-16552-8-git-send-email-mdroth@linux.vnet.ibm.com> Subject: Re: [Qemu-devel] [PATCH v4 07/17] spapr_rtas: add ibm, configure-connector RTAS interface List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Michael Roth 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 --o99acAvKqrTZeiCU Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Dec 23, 2014 at 06:30:21AM -0600, Michael Roth wrote: This really wants a commit message explaining in outline what configure_connector does. > Signed-off-by: Michael Roth > --- > hw/ppc/spapr_rtas.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++= ++++++ > 1 file changed, 81 insertions(+) >=20 > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c > index 13e6e55..d847f45 100644 > --- a/hw/ppc/spapr_rtas.c > +++ b/hw/ppc/spapr_rtas.c > @@ -422,6 +422,85 @@ static void rtas_get_sensor_state(PowerPCCPU *cpu, s= PAPREnvironment *spapr, > rtas_st(rets, 1, entity_sense); > } > =20 > +/* 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 Any reason not to use a struct for this? > + > +static void rtas_ibm_configure_connector(PowerPCCPU *cpu, > + sPAPREnvironment *spapr, > + uint32_t token, uint32_t nargs, > + target_ulong args, uint32_t nre= t, > + target_ulong rets) > +{ As always, you need to validate nargs and nret first. > + uint64_t wa_addr =3D ((uint64_t)rtas_ld(args, 1) << 32) | rtas_ld(ar= gs, 0); > + 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); I'm trying to work out if this is an abuse of the rtas_ld interface. As written it wasn't intended for accessing anything other than the immediate rtas args and return values. > + drc =3D spapr_dr_connector_by_index(drc_index); > + if (!drc) { > + DPRINTF("rtas_ibm_configure_connector: invalid sensor/DRC index:= %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_len= ); So, the RTAS interface is stuck with the rather clunky iteration through the device tree. But I don't see a good reason to replicate that clunky interface for communication between the DR core and the individual connector drivers. Wouldn't it make more sense for the class callback to just return the fdt fragment, and then do all the iteration in the DR core? > + 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_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 =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_offset, > + (uint8_t *)prop_name, strlen(prop_name) + = 1); > + > + /* provide the length and value of the OF property. data gets pl= aced > + * immediately after NULL terminator of the OF property's name s= tring > + */ > + 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_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 =3D resp; > +out: > + g_free(prop_name); > + rtas_st(rets, 0, rc); > +} > + > static struct rtas_call { > const char *name; > spapr_rtas_fn fn; > @@ -559,6 +638,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-con= nector", > + rtas_ibm_configure_connector); > } > =20 > type_init(core_rtas_register_types) --=20 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 --o99acAvKqrTZeiCU Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJUvH2IAAoJEGw4ysog2bOScgAP/11xCBMQo0pa1kXtmeWsblCK PYCOZDGXoeCT1hRmmRdgN3csPbgFD904MnSq/HkLdQ1fFhrxbegSINMRGqj/ioKn MKReThMuDuf/ZNRfn2tUKM14LSkw4Pv79has3oVfua/gyeZukBzfJD/4oFOJoz/0 /GHl/2vtl2sQYs+So7wIlU+gACTAgchKclWJHgGTb3aAtYAFYQQx1GLusJhmXU+j ASww7dQSMm1en8Epbm050UbKbhKoBOsNEjOd9cJazDEvBon3U5KMIk/i+/uIrdYQ ZRamzKX4stQQmFbtIGPgtSjRa3yynsbm6hBGlooyv8PPPsjC4TmXZRRLaLN5cGfF Rhk6/Cn3I8LU2QH9zqMQ+Dg71tN522Zzm2R5Dkj0DQv7IBO36uxh2nPQZm65zXOs l+ntbKlOX77QiZeKB9t7leqEzzBkY4qEDty7dl74+1Hw0PT8xGd3GK9DAwWxNA5s F1S+9rTEcdoumWnVdr9zQOYZIPX0EfWa0EVBmaTP8VLi+ep3W0U6ftDdWyHI2zxg 5UrcB2pTgMIncUO5MRJ2/EWra6cuuYG/OEa7QX0WBlkGeQgq69TRhNPJT7e42q68 imvwJrpHvkP98nuUBfxKA1uujvPqgqm+MCKII+RSVdjnYJ5UuSmphLumnq7YY+SG oCvFfsevRbX25emCNC8v =Fs+X -----END PGP SIGNATURE----- --o99acAvKqrTZeiCU--