[Qemu-devel] [PATCH 0/2] device_tree: add qemu_devtree_setprop_sized_cells()

[Qemu-devel] [PATCH 0/2] device_tree: add qemu_devtree_setprop_sized_cells()

[Peter Maydell]

These patches add a new function qemu_devtree_setprop_sized_cells()
to device_tree.c and use it in arm/boot.c.

We already have a qemu_devtree_setprop_cells() which sets a dtb
property to an array of cells whose values are specified by varargs.
However for the fairly common case of setting a property to a list
of addresses or of address,size pairs the number of cells used by
each element in the list depends on the parent's #address-cells
and #size-cells properties. To make this easier we provide an analogous
qemu_devtree_setprop_sized_cells() function which allows the number
of cells used by each element to be specified.

The arm/boot.c use is fairly straightforward; I also have some
virtio related patches brewing which are going to need to use this
to set their 'regs' property values. I figured it was probably better
to send this out for review now rather than wait for those, though.

The arm/boot bit of the patch depends (textually at least) on
http://patchwork.ozlabs.org/patch/251377/
  (arm/boot: Free dtb blob memory after use); it will apply cleanly
to git://git.linaro.org/people/pmaydell/qemu-arm.git arm-devs.next.

Peter Maydell (2):
  device_tree: Add qemu_devtree_setprop_sized_cells() utility function
  arm/boot: Use qemu_devtree_setprop_sized_cells()

 device_tree.c                |   48 ++++++++++++++++++++++++++++++++++++++++++
 hw/arm/boot.c                |   28 ++++++------------------
 include/sysemu/device_tree.h |   29 +++++++++++++++++++++++++
 3 files changed, 84 insertions(+), 21 deletions(-)

-- 
1.7.9.5

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

[Peter Maydell]

We already have a qemu_devtree_setprop_cells() which sets a dtb
property to an array of cells whose values are specified by varargs.
However for the fairly common case of setting a property to a list
of addresses or of address,size pairs the number of cells used by
each element in the list depends on the parent's #address-cells
and #size-cells properties. To make this easier we provide an analogous
qemu_devtree_setprop_sized_cells() function which allows the number
of cells used by each element to be specified.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 device_tree.c                |   48 ++++++++++++++++++++++++++++++++++++++++++
 include/sysemu/device_tree.h |   29 +++++++++++++++++++++++++
 2 files changed, 77 insertions(+)

diff --git a/device_tree.c b/device_tree.c
index 69be9da..15a081c 100644
--- a/device_tree.c
+++ b/device_tree.c
@@ -319,3 +319,51 @@ void qemu_devtree_dumpdtb(void *fdt, int size)
     }
 
 }
+
+int qemu_devtree_setprop_sized_cells(void *fdt, const char *node_path,
+                                     const char *property, ...)
+{
+    uint32_t *propcells;
+    uint32_t propsize = 0;
+    va_list ap;
+    uint32_t ncells;
+    uint64_t value;
+    int i, argnum;
+
+    va_start(ap, property);
+    for (;;) {
+        ncells = va_arg(ap, uint32_t);
+        if (!ncells) {
+            break;
+        }
+        assert(ncells < 3);
+        propsize += ncells;
+        value = va_arg(ap, uint64_t);
+    }
+    va_end(ap);
+
+    propcells = g_new0(uint32_t, propsize);
+
+    i = 0;
+    va_start(ap, property);
+    for (argnum = 1; ; argnum++) {
+        uint32_t hival;
+
+        ncells = va_arg(ap, uint32_t);
+        if (!ncells) {
+            break;
+        }
+        value = va_arg(ap, uint64_t);
+        hival = cpu_to_be32(value >> 32);
+        if (ncells > 1) {
+            propcells[i++] = hival;
+        } else if (hival != 0) {
+            return argnum;
+        }
+        propcells[i++] = cpu_to_be32(value);
+    }
+    va_end(ap);
+
+    return qemu_devtree_setprop(fdt, node_path, property, propcells,
+                                propsize * sizeof(uint32_t));
+}
diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
index f0b3f35..d7691f9 100644
--- a/include/sysemu/device_tree.h
+++ b/include/sysemu/device_tree.h
@@ -51,4 +51,33 @@ int qemu_devtree_add_subnode(void *fdt, const char *name);
 
 void qemu_devtree_dumpdtb(void *fdt, int size);
 
+/**
+ * qemu_devtree_setprop_sized_cells:
+ * @fdt: device tree blob
+ * @node_path: node to set property on
+ * @property: property to set
+ * @...: 0-terminated list of uint32_t number-of-cells, uint64_t value pairs
+ *
+ * Set the specified property on the specified node in the device tree
+ * to be an array of cells. The values of the cells are specified via
+ * the varargs list, which alternates between "number of cells used by
+ * this value" and "value" (terminated when number-of-cells is zero).
+ * number-of-cells must be either 1 or 2 (other values will assert()).
+ *
+ * This function is useful because device tree nodes often have cell arrays
+ * which are either lists of addresses or lists of address,size tuples, but
+ * the number of cells used for each element vary depending on the
+ * #address-cells and #size-cells properties of their parent node.
+ * If you know all your cell elements are one cell wide you can use the
+ * simpler qemu_devtree_setprop_cells().
+ *
+ * Return value: 0 on success, <0 if setting the property failed,
+ * n (for n>0) if value n wouldn't fit in the required number of cells.
+ * (This slightly odd convention is for the benefit of callers who might
+ * wish to print different error messages depending on which value
+ * was too large to fit, since values might be user-specified.)
+ */
+int qemu_devtree_setprop_sized_cells(void *fdt, const char *node_path,
+                                     const char *property, ...);
+
 #endif /* __DEVICE_TREE_H__ */
-- 
1.7.9.5

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

[Alexander Graf]

On 24.06.2013, at 12:22, Peter Maydell wrote:

> We already have a qemu_devtree_setprop_cells() which sets a dtb
> property to an array of cells whose values are specified by varargs.
> However for the fairly common case of setting a property to a list
> of addresses or of address,size pairs the number of cells used by
> each element in the list depends on the parent's #address-cells
> and #size-cells properties. To make this easier we provide an analogous
> qemu_devtree_setprop_sized_cells() function which allows the number
> of cells used by each element to be specified.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

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? There's got to be an easier way to implement that.


Alex

> ---
> device_tree.c                |   48 ++++++++++++++++++++++++++++++++++++++++++
> include/sysemu/device_tree.h |   29 +++++++++++++++++++++++++
> 2 files changed, 77 insertions(+)
> 
> diff --git a/device_tree.c b/device_tree.c
> index 69be9da..15a081c 100644
> --- a/device_tree.c
> +++ b/device_tree.c
> @@ -319,3 +319,51 @@ void qemu_devtree_dumpdtb(void *fdt, int size)
>     }
> 
> }
> +
> +int qemu_devtree_setprop_sized_cells(void *fdt, const char *node_path,
> +                                     const char *property, ...)
> +{
> +    uint32_t *propcells;
> +    uint32_t propsize = 0;
> +    va_list ap;
> +    uint32_t ncells;
> +    uint64_t value;
> +    int i, argnum;
> +
> +    va_start(ap, property);
> +    for (;;) {
> +        ncells = va_arg(ap, uint32_t);
> +        if (!ncells) {
> +            break;
> +        }
> +        assert(ncells < 3);
> +        propsize += ncells;
> +        value = va_arg(ap, uint64_t);
> +    }
> +    va_end(ap);
> +
> +    propcells = g_new0(uint32_t, propsize);
> +
> +    i = 0;
> +    va_start(ap, property);
> +    for (argnum = 1; ; argnum++) {
> +        uint32_t hival;
> +
> +        ncells = va_arg(ap, uint32_t);
> +        if (!ncells) {
> +            break;
> +        }
> +        value = va_arg(ap, uint64_t);
> +        hival = cpu_to_be32(value >> 32);
> +        if (ncells > 1) {
> +            propcells[i++] = hival;
> +        } else if (hival != 0) {
> +            return argnum;
> +        }
> +        propcells[i++] = cpu_to_be32(value);
> +    }
> +    va_end(ap);
> +
> +    return qemu_devtree_setprop(fdt, node_path, property, propcells,
> +                                propsize * sizeof(uint32_t));
> +}
> diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
> index f0b3f35..d7691f9 100644
> --- a/include/sysemu/device_tree.h
> +++ b/include/sysemu/device_tree.h
> @@ -51,4 +51,33 @@ int qemu_devtree_add_subnode(void *fdt, const char *name);
> 
> void qemu_devtree_dumpdtb(void *fdt, int size);
> 
> +/**
> + * qemu_devtree_setprop_sized_cells:
> + * @fdt: device tree blob
> + * @node_path: node to set property on
> + * @property: property to set
> + * @...: 0-terminated list of uint32_t number-of-cells, uint64_t value pairs
> + *
> + * Set the specified property on the specified node in the device tree
> + * to be an array of cells. The values of the cells are specified via
> + * the varargs list, which alternates between "number of cells used by
> + * this value" and "value" (terminated when number-of-cells is zero).
> + * number-of-cells must be either 1 or 2 (other values will assert()).
> + *
> + * This function is useful because device tree nodes often have cell arrays
> + * which are either lists of addresses or lists of address,size tuples, but
> + * the number of cells used for each element vary depending on the
> + * #address-cells and #size-cells properties of their parent node.
> + * If you know all your cell elements are one cell wide you can use the
> + * simpler qemu_devtree_setprop_cells().
> + *
> + * Return value: 0 on success, <0 if setting the property failed,
> + * n (for n>0) if value n wouldn't fit in the required number of cells.
> + * (This slightly odd convention is for the benefit of callers who might
> + * wish to print different error messages depending on which value
> + * was too large to fit, since values might be user-specified.)
> + */
> +int qemu_devtree_setprop_sized_cells(void *fdt, const char *node_path,
> +                                     const char *property, ...);
> +
> #endif /* __DEVICE_TREE_H__ */
> -- 
> 1.7.9.5
> 

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

[Peter Maydell]

On 24 June 2013 11:56, Alexander Graf <agraf@suse.de> wrote:
> On 24.06.2013, at 12:22, Peter Maydell wrote:
>> We already have a qemu_devtree_setprop_cells() which sets a dtb
>> property to an array of cells whose values are specified by varargs.
>> However for the fairly common case of setting a property to a list
>> of addresses or of address,size pairs the number of cells used by
>> each element in the list depends on the parent's #address-cells
>> and #size-cells properties. To make this easier we provide an analogous
>> qemu_devtree_setprop_sized_cells() function which allows the number
>> of cells used by each element to be specified.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>
> 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. It's pretty complicated because the device tree
spec is pretty complicated (if it had just mandated
64 bit addr/size everywhere then this would be easy).

-- PMM

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

[David Gibson]

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

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:
> > On 24.06.2013, at 12:22, Peter Maydell wrote:
> >> We already have a qemu_devtree_setprop_cells() which sets a dtb
> >> property to an array of cells whose values are specified by varargs.
> >> However for the fairly common case of setting a property to a list
> >> of addresses or of address,size pairs the number of cells used by
> >> each element in the list depends on the parent's #address-cells
> >> and #size-cells properties. To make this easier we provide an analogous
> >> qemu_devtree_setprop_sized_cells() function which allows the number
> >> of cells used by each element to be specified.
> >>
> >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> >
> > 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.

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 :(.

> It's pretty complicated because the device tree
> spec is pretty complicated (if it had just mandated
> 64 bit addr/size everywhere then this would be easy).

Actually, no, because #address-cells and #size-cells cover things
other than "memory-like" addresses.  e.g. #address-cells is 3 for PCI,
which wouldn't be covered by 64-bit addressing.

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

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

[Peter Maydell]

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

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

thanks
-- PMM

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

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

[Peter Maydell]

On 26 June 2013 11:31, Alexander Graf <agraf@suse.de> wrote:
> 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).

Yes, but that doesn't work for things that aren't simple arrays
of 32 bit values, so I think that a generic way to deal
with those too would be useful. If you wanted to write a
"ranges" property you'd need this too, so it doesn't just
apply to "reg".

I think we could avoid the "varargs doesn't promote" problem
by using a varargs-macro wrapper:

#define qemu_devtree_setprop_sized_cells(fdt, node, prop, ...) \
    do {   \
        uint64_t args[] = { __VA_ARGS__ }; \
        do_qemu_devtree_setprop_sized_cells(fdt, node, prop, \
            args, sizeof(args));
    } while (0)

which will promote everything (including the size arguments,
harmlessly) to uint64_t, and avoids having a varargs function.

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

Do we have an actual use for this? It seems pretty complicated.
I would expect that in practice there are two major use cases:
 (a) create your own fdt from scratch (in which case you can
     just make everything 64 bits and in any case will know
     when creating nodes what the #address-cells etc are)
 (b) modify an existing fdt, in which case you definitely don't
     want to go poking around too deeply in the tree; anything
     more than just "put an extra node in the root" is starting
     to get pretty chancy.

thanks
-- PMM

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

[Alexander Graf]

On 26.06.2013, at 12:50, Peter Maydell wrote:

> On 26 June 2013 11:31, Alexander Graf <agraf@suse.de> wrote:
>> 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).
> 
> Yes, but that doesn't work for things that aren't simple arrays
> of 32 bit values, so I think that a generic way to deal
> with those too would be useful. If you wanted to write a
> "ranges" property you'd need this too, so it doesn't just
> apply to "reg".
> 
> I think we could avoid the "varargs doesn't promote" problem
> by using a varargs-macro wrapper:
> 
> #define qemu_devtree_setprop_sized_cells(fdt, node, prop, ...) \
>    do {   \
>        uint64_t args[] = { __VA_ARGS__ }; \
>        do_qemu_devtree_setprop_sized_cells(fdt, node, prop, \
>            args, sizeof(args));
>    } while (0)
> 
> which will promote everything (including the size arguments,
> harmlessly) to uint64_t, and avoids having a varargs function.

That would work, yes :).

> 
>> 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?
> 
> Do we have an actual use for this? It seems pretty complicated.
> I would expect that in practice there are two major use cases:
> (a) create your own fdt from scratch (in which case you can
>     just make everything 64 bits and in any case will know
>     when creating nodes what the #address-cells etc are)
> (b) modify an existing fdt, in which case you definitely don't
>     want to go poking around too deeply in the tree; anything
>     more than just "put an extra node in the root" is starting
>     to get pretty chancy.

Well, though I do agree it would mimic exactly what the interpreter will do when reading those values, ensuring consistency, no?


Alex

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

[Peter Crosthwaite]

Hi,

On Wed, Jun 26, 2013 at 8:50 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 26 June 2013 11:31, Alexander Graf <agraf@suse.de> wrote:
>> 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).
>
> Yes, but that doesn't work for things that aren't simple arrays
> of 32 bit values, so I think that a generic way to deal
> with those too would be useful. If you wanted to write a
> "ranges" property you'd need this too, so it doesn't just
> apply to "reg".
>

+1. And wouldn't an implementation of such a reg-specific function
back onto Peter's new function quite nicely anyway?

> I think we could avoid the "varargs doesn't promote" problem
> by using a varargs-macro wrapper:
>
> #define qemu_devtree_setprop_sized_cells(fdt, node, prop, ...) \
>     do {   \
>         uint64_t args[] = { __VA_ARGS__ }; \
>         do_qemu_devtree_setprop_sized_cells(fdt, node, prop, \
>             args, sizeof(args));
>     } while (0)
>

Are statement expressions sanctioned? Or do we need to give up the
nice caller accessible return codes?

And can we factor out common functionality (mainly the error checking)
with existing set_prop_cells to make the interfaces consistent? (we
need to get rid of those aborts sooner or later)

> which will promote everything (including the size arguments,
> harmlessly) to uint64_t, and avoids having a varargs function.
>
>> 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?
>

I have a patch in my tree that generalises qemu_devtree_getprop* to
allow you walk parents for properties (as per the #foo-cells
semantic). I use it for interrupt cells however, which kinda suggests
that this wish for parent traversal is more generic than just
populating reg. I think that Peters patch, along with a parent search
friendly property search will be enough to be able to do whatever you
want in only a handful of lines.

> Do we have an actual use for this? It seems pretty complicated.
> I would expect that in practice there are two major use cases:
>  (a) create your own fdt from scratch (in which case you can
>      just make everything 64 bits and in any case will know
>      when creating nodes what the #address-cells etc are)
>  (b) modify an existing fdt, in which case you definitely don't
>      want to go poking around too deeply in the tree; anything
>      more than just "put an extra node in the root" is starting
>      to get pretty chancy.
>

Looking to the future, what about -device adding a periph then having
qemu add it to the dtb before boot?

Regards,
Peter

> thanks
> -- PMM

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

[Alexander Graf]

On 06/26/2013 02:38 PM, Peter Crosthwaite wrote:
> Hi,
>
> On Wed, Jun 26, 2013 at 8:50 PM, Peter Maydell<peter.maydell@linaro.org>  wrote:
>> On 26 June 2013 11:31, Alexander Graf<agraf@suse.de>  wrote:
>>> 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).
>> Yes, but that doesn't work for things that aren't simple arrays
>> of 32 bit values, so I think that a generic way to deal
>> with those too would be useful. If you wanted to write a
>> "ranges" property you'd need this too, so it doesn't just
>> apply to "reg".
>>
> +1. And wouldn't an implementation of such a reg-specific function
> back onto Peter's new function quite nicely anyway?
>
>> I think we could avoid the "varargs doesn't promote" problem
>> by using a varargs-macro wrapper:
>>
>> #define qemu_devtree_setprop_sized_cells(fdt, node, prop, ...) \
>>      do {   \
>>          uint64_t args[] = { __VA_ARGS__ }; \
>>          do_qemu_devtree_setprop_sized_cells(fdt, node, prop, \
>>              args, sizeof(args));
>>      } while (0)
>>
> Are statement expressions sanctioned? Or do we need to give up the
> nice caller accessible return codes?
>
> And can we factor out common functionality (mainly the error checking)
> with existing set_prop_cells to make the interfaces consistent? (we
> need to get rid of those aborts sooner or later)
>
>> which will promote everything (including the size arguments,
>> harmlessly) to uint64_t, and avoids having a varargs function.
>>
>>> 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?
> I have a patch in my tree that generalises qemu_devtree_getprop* to
> allow you walk parents for properties (as per the #foo-cells
> semantic). I use it for interrupt cells however, which kinda suggests
> that this wish for parent traversal is more generic than just
> populating reg. I think that Peters patch, along with a parent search
> friendly property search will be enough to be able to do whatever you
> want in only a handful of lines.
>
>> Do we have an actual use for this? It seems pretty complicated.
>> I would expect that in practice there are two major use cases:
>>   (a) create your own fdt from scratch (in which case you can
>>       just make everything 64 bits and in any case will know
>>       when creating nodes what the #address-cells etc are)
>>   (b) modify an existing fdt, in which case you definitely don't
>>       want to go poking around too deeply in the tree; anything
>>       more than just "put an extra node in the root" is starting
>>       to get pretty chancy.
>>
> Looking to the future, what about -device adding a periph then having
> qemu add it to the dtb before boot?

I've had a lengthy discussion about that with Anthony a while ago. His 
take was that this is perfectly reasonable, as long as the device tree 
generation code stays within the machine model. The machine would just 
traverse the QOM hierachy and generate device tree nodes for everything 
it knows.


Alex

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

[David Gibson]

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

On Wed, Jun 26, 2013 at 02:44:17PM +0200, Alexander Graf wrote:
> On 06/26/2013 02:38 PM, Peter Crosthwaite wrote:
> >Hi,
> >
> >On Wed, Jun 26, 2013 at 8:50 PM, Peter Maydell<peter.maydell@linaro.org>  wrote:
> >>On 26 June 2013 11:31, Alexander Graf<agraf@suse.de>  wrote:
> >>>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).
> >>Yes, but that doesn't work for things that aren't simple arrays
> >>of 32 bit values, so I think that a generic way to deal
> >>with those too would be useful. If you wanted to write a
> >>"ranges" property you'd need this too, so it doesn't just
> >>apply to "reg".
> >>
> >+1. And wouldn't an implementation of such a reg-specific function
> >back onto Peter's new function quite nicely anyway?
> >
> >>I think we could avoid the "varargs doesn't promote" problem
> >>by using a varargs-macro wrapper:
> >>
> >>#define qemu_devtree_setprop_sized_cells(fdt, node, prop, ...) \
> >>     do {   \
> >>         uint64_t args[] = { __VA_ARGS__ }; \
> >>         do_qemu_devtree_setprop_sized_cells(fdt, node, prop, \
> >>             args, sizeof(args));
> >>     } while (0)
> >>
> >Are statement expressions sanctioned? Or do we need to give up the
> >nice caller accessible return codes?
> >
> >And can we factor out common functionality (mainly the error checking)
> >with existing set_prop_cells to make the interfaces consistent? (we
> >need to get rid of those aborts sooner or later)
> >
> >>which will promote everything (including the size arguments,
> >>harmlessly) to uint64_t, and avoids having a varargs function.
> >>
> >>>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?
> >I have a patch in my tree that generalises qemu_devtree_getprop* to
> >allow you walk parents for properties (as per the #foo-cells
> >semantic). I use it for interrupt cells however, which kinda suggests
> >that this wish for parent traversal is more generic than just
> >populating reg. I think that Peters patch, along with a parent search
> >friendly property search will be enough to be able to do whatever you
> >want in only a handful of lines.
> >
> >>Do we have an actual use for this? It seems pretty complicated.
> >>I would expect that in practice there are two major use cases:
> >>  (a) create your own fdt from scratch (in which case you can
> >>      just make everything 64 bits and in any case will know
> >>      when creating nodes what the #address-cells etc are)
> >>  (b) modify an existing fdt, in which case you definitely don't
> >>      want to go poking around too deeply in the tree; anything
> >>      more than just "put an extra node in the root" is starting
> >>      to get pretty chancy.
> >>
> >Looking to the future, what about -device adding a periph then having
> >qemu add it to the dtb before boot?
> 
> I've had a lengthy discussion about that with Anthony a while ago.
> His take was that this is perfectly reasonable, as long as the
> device tree generation code stays within the machine model. The
> machine would just traverse the QOM hierachy and generate device
> tree nodes for everything it knows.

I also talked with Anthony about this.  Although he's insistent on the
fdt generation staying within the machine, I think it would make sense
to have some shared helpers for this between the fdt platforms.

Note that spapr already contains a half-arsed implementation of this.

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

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

[Anthony Liguori]

On Wed, Jun 26, 2013 at 7:17 PM, David Gibson
<david@gibson.dropbear.id.au> wrote:
> On Wed, Jun 26, 2013 at 02:44:17PM +0200, Alexander Graf wrote:
>> I've had a lengthy discussion about that with Anthony a while ago.
>> His take was that this is perfectly reasonable, as long as the
>> device tree generation code stays within the machine model. The
>> machine would just traverse the QOM hierachy and generate device
>> tree nodes for everything it knows.
>
> I also talked with Anthony about this.  Although he's insistent on the
> fdt generation staying within the machine, I think it would make sense
> to have some shared helpers for this between the fdt platforms.

FDT generation should stay in the machine but having helpers for
platforms to share makes all the sense in the world.

What I don't think makes sense is to move FDT generation logic to
qdev*.c or anything like that.

But if 99% of ARM/PPC just call fdt_build_generic_table() and then
customize from there, that's fine by me.

Regards,

Anthony Liguori

> Note that spapr already contains a half-arsed implementation of this.
>
> --
> 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

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

[Peter Maydell]

On 26 June 2013 13:38, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> On Wed, Jun 26, 2013 at 8:50 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> I think we could avoid the "varargs doesn't promote" problem
>> by using a varargs-macro wrapper:
>>
>> #define qemu_devtree_setprop_sized_cells(fdt, node, prop, ...) \
>>     do {   \
>>         uint64_t args[] = { __VA_ARGS__ }; \
>>         do_qemu_devtree_setprop_sized_cells(fdt, node, prop, \
>>             args, sizeof(args));
>>     } while (0)
>>
>
> Are statement expressions sanctioned? Or do we need to give up the
> nice caller accessible return codes?

I just borrowed the syntax from the existing qemu_devtree_setprop_cells
macro to illustrate how the varargs bit would work; I'll tweak it to
actually get the return code right for the patch proper.

> And can we factor out common functionality (mainly the error checking)
> with existing set_prop_cells to make the interfaces consistent? (we
> need to get rid of those aborts sooner or later)

Maybe we should at some point switch to using Error** for error
reporting in our wrapper APIs here? It is a bit odd that half
our device tree functions die on error and the other half
report a failure code.

>> which will promote everything (including the size arguments,
>> harmlessly) to uint64_t, and avoids having a varargs function.
>>
>>> 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?
>>
>
> I have a patch in my tree that generalises qemu_devtree_getprop* to
> allow you walk parents for properties (as per the #foo-cells
> semantic). I use it for interrupt cells however, which kinda suggests
> that this wish for parent traversal is more generic than just
> populating reg. I think that Peters patch, along with a parent search
> friendly property search will be enough to be able to do whatever you
> want in only a handful of lines.

Sounds good to me.

-- PMM

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

[Peter Maydell]

On 26 June 2013 13:38, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> Are statement expressions sanctioned?

..and to answer this specific question which I missed first time
round, we already use them (eg container_of, MAKE_TCGV_PTR).

We seem to mark some but not all of them with __extension__;
I think on balance that's probably unnecessary given we aren't
going to be trying to compile with -pedantic any time soon.

-- PMM

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

[David Gibson]

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

On Wed, Jun 26, 2013 at 11:50:26AM +0100, Peter Maydell wrote:
> On 26 June 2013 11:31, Alexander Graf <agraf@suse.de> wrote:
> > 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).
> 
> Yes, but that doesn't work for things that aren't simple arrays
> of 32 bit values, so I think that a generic way to deal
> with those too would be useful. If you wanted to write a
> "ranges" property you'd need this too, so it doesn't just
> apply to "reg".
> 
> I think we could avoid the "varargs doesn't promote" problem
> by using a varargs-macro wrapper:
> 
> #define qemu_devtree_setprop_sized_cells(fdt, node, prop, ...) \
>     do {   \
>         uint64_t args[] = { __VA_ARGS__ }; \
>         do_qemu_devtree_setprop_sized_cells(fdt, node, prop, \
>             args, sizeof(args));
>     } while (0)
> 
> which will promote everything (including the size arguments,
> harmlessly) to uint64_t, and avoids having a varargs function.

Heh.  Oops, didn't read this before I suggested the same thing.
Although a statement expression will let you actually get any fdt
error codes out.

> > 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?
> 
> Do we have an actual use for this? It seems pretty complicated.
> I would expect that in practice there are two major use cases:
>  (a) create your own fdt from scratch (in which case you can
>      just make everything 64 bits and in any case will know
>      when creating nodes what the #address-cells etc are)
>  (b) modify an existing fdt, in which case you definitely don't
>      want to go poking around too deeply in the tree; anything
>      more than just "put an extra node in the root" is starting
>      to get pretty chancy.

So, I tend to think that "reg" and "ranges" specific helpers would be
nicer than having to add the size for every parameter.  But I was
thinking of passing in the address-cells and size-cells values rather
than having the function dig around in the tree to get them.

The other way to avoid varargs would be to have a helper that just
adds one entry at a time, using fdt_appendprop().

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

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

[David Gibson]

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

[Qemu-devel] [PATCH 2/2] arm/boot: Use qemu_devtree_setprop_sized_cells()

[Peter Maydell]

Replace the opencoded assembly of the reg property array for the
/memory node with a call to qemu_devtree_setprop_sized_cells().

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/boot.c |   28 +++++++---------------------
 1 file changed, 7 insertions(+), 21 deletions(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 7c0090f..7f98497 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -227,12 +227,10 @@ static void set_kernel_args_old(const struct arm_boot_info *info)
 
 static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo)
 {
-    uint32_t *mem_reg_property;
-    uint32_t mem_reg_propsize;
     void *fdt = NULL;
     char *filename;
     int size, rc;
-    uint32_t acells, scells, hival;
+    uint32_t acells, scells;
 
     filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, binfo->dtb_filename);
     if (!filename) {
@@ -255,30 +253,18 @@ static int load_dtb(hwaddr addr, const struct arm_boot_info *binfo)
         goto fail;
     }
 
-    mem_reg_propsize = acells + scells;
-    mem_reg_property = g_new0(uint32_t, mem_reg_propsize);
-    mem_reg_property[acells - 1] = cpu_to_be32(binfo->loader_start);
-    hival = cpu_to_be32(binfo->loader_start >> 32);
-    if (acells > 1) {
-        mem_reg_property[acells - 2] = hival;
-    } else if (hival != 0) {
+    rc = qemu_devtree_setprop_sized_cells(fdt, "/memory", "reg",
+                                          acells, binfo->loader_start,
+                                          scells, binfo->ram_size, 0);
+    if (rc == 1) {
         fprintf(stderr, "qemu: dtb file not compatible with "
                 "RAM start address > 4GB\n");
         goto fail;
-    }
-    mem_reg_property[acells + scells - 1] = cpu_to_be32(binfo->ram_size);
-    hival = cpu_to_be32(binfo->ram_size >> 32);
-    if (scells > 1) {
-        mem_reg_property[acells + scells - 2] = hival;
-    } else if (hival != 0) {
+    } else if (rc == 2) {
         fprintf(stderr, "qemu: dtb file not compatible with "
                 "RAM size > 4GB\n");
         goto fail;
-    }
-
-    rc = qemu_devtree_setprop(fdt, "/memory", "reg", mem_reg_property,
-                              mem_reg_propsize * sizeof(uint32_t));
-    if (rc < 0) {
+    } else if (rc != 0) {
         fprintf(stderr, "couldn't set /memory/reg\n");
         goto fail;
     }
-- 
1.7.9.5