Re: [Qemu-devel] [PATCH v2 1/8] device_tree: Add qemu_devtree_setprop_sized_cells() utility functions

[Peter Crosthwaite]

Hi Peter,

On Sat, Jul 13, 2013 at 6:36 AM, Peter Maydell <address@hidden> 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() macro which allows the number
> of cells used by each element to be specified. This is implemented
> using an underlying qemu_devtree_setprop_sized_cells_from_array()
> function which takes the values and sizes as an array; this may
> also be directly useful for cases where the cell contents are
> constructed programmatically.
>
> Signed-off-by: Peter Maydell <address@hidden>
> ---
>  device_tree.c                |   31 +++++++++++++++++++++
>  include/sysemu/device_tree.h |   62 
> ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 93 insertions(+)
>
> diff --git a/device_tree.c b/device_tree.c
> index 10cf3d0..308205c 100644
> --- a/device_tree.c
> +++ b/device_tree.c
> @@ -308,3 +308,34 @@ void qemu_devtree_dumpdtb(void *fdt, int size)
>          exit(g_file_set_contents(dumpdtb, fdt, size, NULL) ? 0 : 1);
>      }
>  }
> +
> +int qemu_devtree_setprop_sized_cells_from_array(void *fdt,
> +                                                const char *node_path,
> +                                                const char *property,
> +                                                int numvalues,
> +                                                uint64_t *values)
> +{
> +    uint32_t *propcells;
> +    uint64_t value;
> +    int cellnum, vnum, ncells;
> +    uint32_t hival;
> +
> +    propcells = g_new0(uint32_t, numvalues * 2);
> +
> +    cellnum = 0;
> +    for (vnum = 0; vnum < numvalues; vnum++) {
> +        ncells = values[vnum * 2];
> +        assert(ncells == 1 || ncells == 2);
> +        value = values[vnum * 2 + 1];
> +        hival = cpu_to_be32(value >> 32);
> +        if (ncells > 1) {
> +            propcells[cellnum++] = hival;
> +        } else if (hival != 0) {
> +            return vnum + 1;
> +        }
> +        propcells[cellnum++] = cpu_to_be32(value);
> +    }
> +
> +    return qemu_devtree_setprop(fdt, node_path, property, propcells,
> +                                cellnum * sizeof(uint32_t));
> +}
> diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h
> index f0b3f35..8189ebc 100644
> --- a/include/sysemu/device_tree.h
> +++ b/include/sysemu/device_tree.h
> @@ -51,4 +51,66 @@ int qemu_devtree_add_subnode(void *fdt, const char *name);
>
>  void qemu_devtree_dumpdtb(void *fdt, int size);
>
> +/**
> + * qemu_devtree_setprop_sized_cells_from_array:
> + * @fdt: device tree blob
> + * @node_path: node to set property on
> + * @property: property to set
> + * @numvalues: number of values
> + * @values: array of number-of-cells, 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 values list, which alternates between "number of cells used by
> + * this value" and "value".
> + * 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(). If you're not setting up the
> + * array programmatically, qemu_devtree_setprop_sized_cells may be more
> + * convenient.
> + *
> + * 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.)

I have patches on list converting the other "setprop" APIs to Error ** based
reporting (as per your suggestion). Once that change happens we will have
two separate error schemes for the one fn.

For sanity checking user specified values (e.g. command line args) cant it
just be done explicitly as required? It seems to me that this will be the
exception not the rule, as for DTB generation from machine model a fail
here will be a fatal anyway (better handled by Error **).

if (acells < 2 && addr >= (1ull << 32)) {
   fprintf("your address is too big for this dtb");
}

LOC looks to be similar and this API gets a little cleaner.

Regards,
Peter