From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43712) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YD5Lu-0007qR-Pg for qemu-devel@nongnu.org; Mon, 19 Jan 2015 00:58:22 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YD5Lp-00028h-Oo for qemu-devel@nongnu.org; Mon, 19 Jan 2015 00:58:18 -0500 Date: Mon, 19 Jan 2015 16:15:28 +1100 From: David Gibson Message-ID: <20150119051528.GT5297@voom.fritz.box> References: <1419337831-16552-1-git-send-email-mdroth@linux.vnet.ibm.com> <1419337831-16552-11-git-send-email-mdroth@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="4fNq9Po2wJlmxAaR" Content-Disposition: inline In-Reply-To: <1419337831-16552-11-git-send-email-mdroth@linux.vnet.ibm.com> Subject: Re: [Qemu-devel] [PATCH v4 10/17] spapr_drc: add spapr_drc_populate_dt() 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 --4fNq9Po2wJlmxAaR Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Dec 23, 2014 at 06:30:24AM -0600, Michael Roth wrote: > This function handles generation of ibm,drc-* array device tree > properties to describe DRC topology to guests. This will by used > by the guest to direct RTAS calls to manage any dynamic resources > we associate with a particular DR Connector as part of > hotplug/unplug. >=20 > Since general management of boot-time device trees are handled > outside of sPAPRDRConnector, we insert these values blindly given > an FDT and offset. A mask of sPAPRDRConnector types is given to > instruct us on what types of connectors entries should be generated > for, since descriptions for different connectors may live in > different parts of the device tree. >=20 > Based on code originally written by Nathan Fontenot. >=20 > Signed-off-by: Michael Roth > --- > hw/ppc/spapr_drc.c | 225 +++++++++++++++++++++++++++++++++++++++= ++++++ > include/hw/ppc/spapr_drc.h | 3 +- > 2 files changed, 227 insertions(+), 1 deletion(-) >=20 > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c > index f81c6d1..b162184 100644 > --- a/hw/ppc/spapr_drc.c > +++ b/hw/ppc/spapr_drc.c > @@ -501,3 +501,228 @@ sPAPRDRConnector *spapr_dr_connector_by_id(sPAPRDRC= onnectorType type, > (get_type_shift(type) << DRC_INDEX_TYPE_SHIFT) | > (id & DRC_INDEX_ID_MASK)); > } > + > +/* internal helper to gather up DRC info specific to populating DRC > + * topology information in the device tree. > + */ > +typedef struct DRConnectorDTInfo { > + char drc_type[64]; > + char drc_name[64]; > + uint32_t drc_index; > + uint32_t drc_power_domain; > +} DRConnectorDTInfo; > + > +/* generate a string the describes the DRC to encode into the > + * device tree. > + * > + * as documented by PAPR+ v2.7, 13.5.2.6 and C.6.1 > + */ > +static void spapr_drc_get_type_str(char *buf, sPAPRDRConnectorType type) > +{ > + switch (type) { > + case SPAPR_DR_CONNECTOR_TYPE_CPU: > + sprintf(buf, "CPU"); > + break; > + case SPAPR_DR_CONNECTOR_TYPE_PHB: > + sprintf(buf, "PHB"); > + break; > + case SPAPR_DR_CONNECTOR_TYPE_VIO: > + sprintf(buf, "SLOT"); > + break; > + case SPAPR_DR_CONNECTOR_TYPE_PCI: > + sprintf(buf, "28"); > + break; > + case SPAPR_DR_CONNECTOR_TYPE_LMB: > + sprintf(buf, "MEM"); > + break; > + default: > + g_assert(false); > + } So this case is simple enough that you can probably get away with it, but still - interfaces that involve writing to a buffer without any length checks make me very nervous. > +} > + > +/* generate a human-readable name for a DRC to encode into the DT > + * description. this is mainly only used within a guest in place > + * of the unique DRC index. > + * > + * in the case of VIO/PCI devices, it corresponds to a > + * "location code" that maps a logical device/function (DRC index) > + * to a physical (or virtual in the case of VIO) location in the > + * system by chaining together the "location label" for each > + * encapsulating component. > + * > + * since this is more to do with diagnosing physical hardware > + * issues than guest compatibility, we choose location codes/DRC > + * names that adhere to the documented format, but avoid encoding > + * the entire topology information into the label/code, instead > + * just using the location codes based on the labels for the > + * endpoints (VIO/PCI adaptor connectors), which is basically > + * just "C" followed by an integer ID. Hrm.. would it make sense to include here the qemu "id" value on the DRC device? That will make names which are matchable to specific elements on the qemu command line, which about as close an equivalent to a physical location as I can think of. > + * DRC names as documented by PAPR+ v2.7, 13.5.2.4 > + * location codes as documented by PAPR+ v2.7, 12.3.1.5 > + */ > +static void spapr_drc_get_name_str(char *buf, > + sPAPRDRConnectorType type, > + uint32_t drc_index) > +{ > + uint32_t id =3D drc_index & DRC_INDEX_ID_MASK; > + > + switch (type) { > + case SPAPR_DR_CONNECTOR_TYPE_CPU: > + sprintf(buf, "CPU %d", id); > + break; > + case SPAPR_DR_CONNECTOR_TYPE_PHB: > + sprintf(buf, "PHB %d", id); > + break; > + case SPAPR_DR_CONNECTOR_TYPE_VIO: > + case SPAPR_DR_CONNECTOR_TYPE_PCI: > + sprintf(buf, "C%d", id); > + break; > + case SPAPR_DR_CONNECTOR_TYPE_LMB: > + sprintf(buf, "LMB %d", id); > + break; > + default: > + g_assert(false); > + } > +} > + > +static DRConnectorDTInfo *spapr_dr_connector_get_info(uint32_t drc_type_= mask, > + unsigned int *coun= t) > +{ > + Object *root_container; > + ObjectProperty *prop; > + GArray *drc_info_list =3D g_array_new(false, true, > + sizeof(DRConnectorDTInfo)); > + > + /* aliases for all DRConnector objects will be rooted in QOM > + * composition tree at /dr-connector > + */ > + root_container =3D container_get(object_get_root(), "/dr-connector"); > + > + QTAILQ_FOREACH(prop, &root_container->properties, node) { > + Object *obj; > + sPAPRDRConnector *drc; > + sPAPRDRConnectorClass *drck; > + DRConnectorDTInfo drc_info; > + > + if (!strstart(prop->type, "link<", NULL)) { > + continue; > + } > + > + obj =3D object_property_get_link(root_container, prop->name, NUL= L); > + drc =3D SPAPR_DR_CONNECTOR(obj); > + drck =3D SPAPR_DR_CONNECTOR_GET_CLASS(drc); > + > + if ((drc->type & drc_type_mask) =3D=3D 0) { > + continue; > + } > + > + drc_info.drc_index =3D drck->get_index(drc); > + drc_info.drc_power_domain =3D -1; > + spapr_drc_get_type_str(drc_info.drc_type, drc->type); > + spapr_drc_get_name_str(drc_info.drc_name, drc->type, > + drck->get_index(drc)); > + g_array_append_val(drc_info_list, drc_info); > + } > + > + if (count) { > + *count =3D drc_info_list->len; > + } > + > + /* if count is zero, free everything, including internal storage > + * for array > + */ > + return (DRConnectorDTInfo *)g_array_free(drc_info_list, count =3D=3D= 0); > +} > + > +/** > + * spapr_drc_populate_dt > + * > + * @fdt: libfdt device tree > + * @path: path in the DT to generate properties > + * @drc_type_mask: mask of sPAPRDRConnectorType values corresponding > + * to the types of DRCs to generate entries for > + * > + * generate OF properties to describe DRC topology/indices to guests > + * > + * as documented in PAPR+ v2.1, 13.5.2 > + */ > +int spapr_drc_populate_dt(void *fdt, int fdt_offset, uint32_t drc_type_m= ask) > +{ > + DRConnectorDTInfo *drc_info_list; > + unsigned int i, count; > + char *char_buf; > + uint32_t *char_buf_count; > + uint32_t *int_buf; > + int char_buf_offset, ret; > + > + drc_info_list =3D > + spapr_dr_connector_get_info(drc_type_mask, &count); This is the only call to spapr_dt_connector_get_info(). I don't see a lot of point in splitting it out from this function, since it involves a not particular easy to work with array encoding of the information. Why not go direct from the drc objects to the fdt. > + if (!count) { > + return 0; > + } > + > + int_buf =3D g_new0(uint32_t, count + 1); > + int_buf[0] =3D cpu_to_be32(count); > + char_buf =3D g_new0(char, count * 128 + sizeof(uint32_t)); > + char_buf_count =3D (uint32_t *)&char_buf[0]; > + *char_buf_count =3D cpu_to_be32(count); > + > + /* ibm,drc-indexes */ > + for (i =3D 0; i < count; i++) { > + int_buf[i + 1] =3D cpu_to_be32(drc_info_list[i].drc_index); > + } > + ret =3D fdt_setprop(fdt, fdt_offset, "ibm,drc-indexes", int_buf, > + (count + 1) * sizeof(uint32_t)); > + if (ret) { > + fprintf(stderr, "Couldn't create ibm,drc-indexes property\n"); > + goto out; > + } > + > + /* ibm,drc-power-domains */ > + for (i =3D 0; i < count; i++) { > + int_buf[i + 1] =3D cpu_to_be32(drc_info_list[i].drc_power_domain= ); > + } > + ret =3D fdt_setprop(fdt, fdt_offset, "ibm,drc-power-domains", int_bu= f, > + (count + 1) * sizeof(uint32_t)); > + if (ret) { > + fprintf(stderr, "Couldn't finalize ibm,drc-power-domains propert= y\n"); > + goto out; > + } > + > + /* ibm,drc-names */ > + char_buf_offset =3D sizeof(uint32_t); > + > + for (i =3D 0; i < count; i++) { > + strcpy(char_buf + char_buf_offset, drc_info_list[i].drc_name); > + char_buf_offset +=3D strlen(drc_info_list[i].drc_name) + 1; > + } > + ret =3D fdt_setprop(fdt, fdt_offset, "ibm,drc-names", char_buf, > + char_buf_offset); > + if (ret) { > + fprintf(stderr, "Couldn't finalize ibm,drc-names property\n"); > + goto out; > + } > + > + /* ibm,drc-types */ > + char_buf_offset =3D sizeof(uint32_t); > + > + for (i =3D 0; i < count; i++) { > + strcpy(char_buf + char_buf_offset, drc_info_list[i].drc_type); > + char_buf_offset +=3D strlen(drc_info_list[i].drc_type) + 1; > + } > + > + ret =3D fdt_setprop(fdt, fdt_offset, "ibm,drc-types", char_buf, > + char_buf_offset); > + if (ret) { > + fprintf(stderr, "Couldn't finalize ibm,drc-types property\n"); > + goto out; > + } > + > +out: > + g_free(int_buf); > + g_free(char_buf); > + g_free(drc_info_list); > + return ret; > +} > diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h > index 63ec687..5c70140 100644 > --- a/include/hw/ppc/spapr_drc.h > +++ b/include/hw/ppc/spapr_drc.h > @@ -193,9 +193,10 @@ typedef struct sPAPRDRConnectorClass { > =20 > sPAPRDRConnector *spapr_dr_connector_new(Object *owner, > sPAPRDRConnectorType type, > - uint32_t token); > + uint32_t id); > sPAPRDRConnector *spapr_dr_connector_by_index(uint32_t index); > sPAPRDRConnector *spapr_dr_connector_by_id(sPAPRDRConnectorType type, > uint32_t id); > +int spapr_drc_populate_dt(void *fdt, int fdt_offset, uint32_t drc_type_m= ask); > =20 > #endif /* __HW_SPAPR_DRC_H__ */ --=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 --4fNq9Po2wJlmxAaR Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJUvJLvAAoJEGw4ysog2bOSnAkQAJkfwe+6DKZCAWGSsIKjfble Q09nG2dJZRgOgP/ksPgKKf7emwRCa1QXYkh9uOAuRy8SoxkZePZJCRZpni7RVFKH oEGbirzZxkfVUs/OvPJtYmBCbH4KTQV2U9ygARF8VQ4/lSxjt46ncynh80AciDvr WAcH1hYMjPg5HxFnTjagYgNkdv3bl4wh+hM8RRYVsbUtxEBaxGxcK9U9ITp6zWSS NfjcGekaI6hWHBkW5/9us6w6ccyYpWx7/n0049YU/VrNYr2K1xqe3t4W/5I2HFeI RifUkdb5bucrGLy6XHisDfQzyP7ND4QjWWsMOmuroo10+oU59PexsHRs6REcWvW7 ovHmG/HAqXqQbG1uXzrjM3piRpl7/TyhqvzaOBD0ehF2Ji0rSNIPm2n1cqlkiTZM TWLwK1DdSRNoB9Oa3oas0t148X2wr18DY/jjHXoqHIv8raIqkvYeVEutNNrGTHes PzSCRBE6/OpcZ4E3g8yGX3/jp4z4a/i2Y7PB3/CEKOZXKNxU+X5S4+aRh40tHRav Soqmw1C9xn5fXEE+17XZuC2e37uX2iUUKIOt3sIWB0nL+jCvWVjAjiuqt6HcI5FB 3quyyMsTutjfKf8cdoJ7sYHn26oUTakjpcviPXQxUirQjx/q99P1tVnX9fWJuRgi CPAyJhOggFSxwRwk/oUW =cffU -----END PGP SIGNATURE----- --4fNq9Po2wJlmxAaR--