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

[Alexander Graf]

On 26.06.2013, at 10:49, Peter Maydell wrote:

> On 26 June 2013 00:38, David Gibson <address@hidden> wrote:
>> On Mon, Jun 24, 2013 at 12:02:39PM +0100, Peter Maydell wrote:
>>> On 24 June 2013 11:56, Alexander Graf <address@hidden> 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
> 
>> 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?

I think it makes sense to make this API special-purpose for "reg". We currently 
have a generic "put any number of 32bit values into the property" function 
(qemu_devtree_setprop_cells).

Can't we also just add a qemu_devtree_setprop_reg() that walks the tree 
downwards in search for #address-cells and #size-cells and assembles the 
correct reg property from a list of 64bit arguments?

  qemu_devtree_setprop_reg(fdt, "/foo/bar", region[0].offset, region[0].size, 
region[1].offset, region[1].size);

Maybe we could even make it more generic to also cover "ranges".


Alex