Re: [Qemu-devel] [PATCH 1/2] device_tree: Add qemu_devtree_setprop_sized_cells() utility function

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3088 bytes --]

On Wed, Jun 26, 2013 at 09:49:51AM +0100, Peter Maydell wrote:
> On 26 June 2013 00:38, David Gibson <david@gibson.dropbear.id.au> wrote:
> > On Mon, Jun 24, 2013 at 12:02:39PM +0100, Peter Maydell wrote:
> >> On 24 June 2013 11:56, Alexander Graf <agraf@suse.de> 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.
> 
> 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 pairs

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 :(.
> 
> 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[] = { __VA_ARGS__ }; \
		__qemu_fdt_pack_ints((fdt), ARRAY_SIZE(_tmp), _tmp); \
	})

-- 
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

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]