From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41452) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UrzuX-0007Gi-Tw for qemu-devel@nongnu.org; Wed, 26 Jun 2013 20:18:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UrzuS-0005AY-GZ for qemu-devel@nongnu.org; Wed, 26 Jun 2013 20:18:05 -0400 Received: from ozlabs.org ([2402:b800:7003:1:1::1]:42338) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UrzuR-00059u-SI for qemu-devel@nongnu.org; Wed, 26 Jun 2013 20:18:00 -0400 Date: Thu, 27 Jun 2013 10:10:32 +1000 From: David Gibson Message-ID: <20130627001032.GA10614@voom.fritz.box> References: <1372069376-30640-1-git-send-email-peter.maydell@linaro.org> <1372069376-30640-2-git-send-email-peter.maydell@linaro.org> <3708A7FB-9BEF-494C-B5A2-B1A97C1F78C4@suse.de> <20130625233850.GG25265@voom.fritz.box> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="3MwIy2ne0vdjdPXF" Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH 1/2] device_tree: Add qemu_devtree_setprop_sized_cells() utility function List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: Peter Crosthwaite , patches@linaro.org, Alexander Graf , qemu-devel@nongnu.org --3MwIy2ne0vdjdPXF Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Jun 26, 2013 at 09:49:51AM +0100, Peter Maydell wrote: > On 26 June 2013 00:38, David Gibson wrote: > > On Mon, Jun 24, 2013 at 12:02:39PM +0100, Peter Maydell wrote: > >> On 24 June 2013 11:56, Alexander Graf wrote: > >> > This looks pretty complicated for something actually quite > >> > simple: All you want is to pass in a number of 64bit values, > >> > rather than 32bit ones, right? > >> > >> Nope. If the device tree blob says #address-cells is 1 > >> and #size-cells is 1, then I want to pass in values to > >> go in 32 bit cells. If it says #address-cells is 2 but > >> #size-cells is still 1, then I want to pass in a value > >> for a 64 bit cell then one for a 32 bit cell. If they're > >> both 2 then I want to pass in values for two 64 bit > >> cells. > > > > Hmm.. the property certainly needs to be constructed that way. But I > > think Alex's point is that you could make the arguments all 64-bit, > > and then truncate them in the generated property. >=20 > Er, the arguments *are* all 64 bits and truncated > in the generated property: > + * @...: 0-terminated list of uint32_t number-of-cells, uint64_t value p= airs Duh, sorry, misread. That's even worse for the point below. uint32_t / uint64_t pairs, which will sometimes work if you mess that up, until you get the wrong platform / parameter combination. And the uint32_ts are things that could naturally be in just a plain old int or long, which might be 64-bit by default on some ABIs, and the uint64_ts could be addresses in 32-bit space which would naturally be stored in a 32-bit variable, but *must* be upcast to 64-bit or again this interface will break subtly on certain platforms. This is hair-tearing frustration waiting to happen. > > There's a bigger problem, though, that exists with both versions. In > > general people expect integer arguments like this not to care too much > > about the exact integer type, because it will be promoted to the right > > thing. Except with varargs it won't. So if you ever have a > > notionally 64-bit address that happens to fit in a 32-bit variable and > > you pass that it, this function will be broken. And the worst of it > > is, it'll work most of the time, until you happen to hit the wrong ABI > > and parameter combination :(. >=20 > Do you have a suggested improvement to the API to avoid this? After some thought, yes, though it will need gcc extensions: /* Big fat comment saying not to use this function directly */ int __qemu_fdt_pack_ints(void *fdt, int n, uint64_t[]) { /* Error if n is not even */ /* take the u64s from the array in pairs, packing as the previous version */ } #define qemu_fdt_pack_ints(fdt, ...) \ ({ \ uint64_t _tmp[] =3D { __VA_ARGS__ }; \ __qemu_fdt_pack_ints((fdt), ARRAY_SIZE(_tmp), _tmp); \ }) --=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 --3MwIy2ne0vdjdPXF Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.13 (GNU/Linux) iEYEARECAAYFAlHLgvgACgkQaILKxv3ab8ar/wCZASYG3J6f08YuI/ePzEITLWg/ DikAnibRKRsbByQSh//CKvtI/QMdlUML =c59s -----END PGP SIGNATURE----- --3MwIy2ne0vdjdPXF--