Re: [Qemu-devel] [RFC for-2.7 01/11] qdt: IEEE1275-style device tree utility code

[David Gibson]

On Tue, Apr 26, 2016 at 01:00:06PM +0200, Thomas Huth wrote:
> On 20.04.2016 04:33, David Gibson wrote:
> ...
> > This patch introduces a new utility library "qdt" for runtime
> > manipulation of device trees.  The intention is that machine type code
> > can use these routines to construct the device tree conveniently,
> > using a pointer-based representation doesn't have the limitations
> > above.  They can then use qdt_flatten() to convert the completed tree
> > to fdt format as a single O(n) operation to pass to the guest.
> 
> Good idea, the FDT format itself is really not very well suited for
> dynamic manipulations...
> 
> ...
> > diff --git a/util/qdt.c b/util/qdt.c
> > new file mode 100644
> > index 0000000..e3a449a
> > --- /dev/null
> > +++ b/util/qdt.c
> > @@ -0,0 +1,262 @@
> > +/*
> > + * Functions for manipulating IEEE1275 (Open Firmware) style device
> > + * trees.
> 
> What does QDT stand for? Maybe add that in the description here.

"QEMU Device Tree" I guess?  Really I was just looking for something
similar but not the same as fdt, and short.

> > + * Copyright David Gibson, Red Hat Inc. 2016
> > + *
> > + * This work is licensed unter the GNU GPL version 2 or (at your
> > + * option) any later version.
> > + */
> > +
> > +#include <libfdt.h>
> > +#include <stdbool.h>
> > +
> > +#include "qemu/osdep.h"
> > +#include "qapi/error.h"
> > +#include "qemu/qdt.h"
> > +#include "qemu/error-report.h"
> > +
> > +/*
> > + * Node functions
> > + */
> > +
> > +QDTNode *qdt_new_node(const gchar *name)
> > +{
> > +    QDTNode *node = g_new0(QDTNode, 1);
> > +
> > +    g_assert(!strchr(name, '/'));
> > +
> > +    node->name = g_strdup(name);
> > +    QTAILQ_INIT(&node->properties);
> > +    QTAILQ_INIT(&node->children);
> > +
> > +    return node;
> > +}
> > +
> > +static QDTNode *get_subnode(QDTNode *parent, const gchar *name, size_t 
> > namelen)
> > +{
> > +    QDTNode *child;
> > +
> > +    g_assert(!memchr(name, '/', namelen));
> > +
> > +    QTAILQ_FOREACH(child, &parent->children, sibling) {
> > +        if ((strlen(child->name) == namelen)
> > +            && (memcmp(child->name, name, namelen) == 0)) {
> 
> Too many parenthesis for my taste ;-)
> 
> > +            return child;
> > +        }
> > +    }
> > +
> > +    return NULL;
> > +}
> > +
> > +QDTNode *qdt_get_node_relative(QDTNode *node, const gchar *path)
> > +{
> > +    const gchar *slash;
> > +    gsize seglen;
> > +
> > +    do {
> > +        slash = strchr(path, '/');
> > +        seglen = slash ? slash - path : strlen(path);
> > +
> > +        node = get_subnode(node, path, seglen);
> > +        path += seglen + 1;
> > +    } while (node && slash);
> > +
> > +    return node;
> > +}
> > +
> > +QDTNode *qdt_get_node(QDTNode *root, const gchar *path)
> > +{
> > +    g_assert(!root->parent);
> > +    g_assert(path[0] == '/');
> > +    return qdt_get_node_relative(root, path + 1);
> > +}
> > +
> > +QDTNode *qdt_add_subnode(QDTNode *parent, const gchar *name)
> > +{
> > +    QDTNode *new = qdt_new_node(name);
> > +
> > +    new->parent = parent;
> > +    QTAILQ_INSERT_TAIL(&parent->children, new, sibling);
> > +    return new;
> > +}
> 
> In case somebody ever tries to compile this with a C++ compiler ... it's
> maybe better avoid using "new" as name for a variable.

Good point, will change.

> > +/*
> > + * Property functions
> > + */
> > +
> > +QDTProperty *qdt_new_property(const gchar *name, gconstpointer val, gsize 
> > len)
> > +{
> > +    QDTProperty *prop = g_malloc0(sizeof(*prop) + len);
> > +
> > +    prop->name = g_strdup(name);
> > +    prop->len = len;
> > +    memcpy(prop->val, val, len);
> > +    return prop;
> > +}
> > +
> > +static QDTProperty *getprop_(const QDTNode *node, const gchar *name)
> 
> Underscore at the end looks somewhat strange ... can't you simply drop that?

Well.. the idea was that the _ versions are the "internal" ones,
whereas external users will generally use the non-underscore version
(in this case the only difference is that the external one returns a
const pointer).

I don't particularly like that convention, so feel free to suggest
something better.

> > +{
> > +    QDTProperty *prop;
> > +
> > +    QTAILQ_FOREACH(prop, &node->properties, list) {
> > +        if (strcmp(prop->name, name) == 0) {
> > +            return prop;
> > +        }
> > +    }
> > +    return NULL;
> > +}
> > +
> > +const QDTProperty *qdt_getprop(const QDTNode *node, const gchar *name)
> > +{
> > +    return getprop_(node, name);
> > +}
> > +
> > +void qdt_delprop(QDTNode *node, const gchar *name)
> > +{
> > +    QDTProperty *prop = getprop_(node, name);
> > +
> > +    if (prop) {
> > +        QTAILQ_REMOVE(&node->properties, prop, list);
> > +        g_free(prop->name);
> > +        g_free(prop);
> > +    }
> > +}
> > +
> > +const QDTProperty *qdt_setprop(QDTNode *node, const gchar *name,
> > +                               gconstpointer val, gsize len)
> > +{
> > +    QDTProperty *prop;
> > +
> > +    qdt_delprop(node, name);
> > +
> > +    prop = g_malloc0(sizeof(*prop) + len);
> > +    prop->name = g_strdup(name);
> > +    prop->len = len;
> > +    memcpy(prop->val, val, len);
> 
> Can you replace the above four lines with qdt_new_property ?

Actually, qdt_new_property() is a leftover from an approach I ended up
not liking, I should just remove it.

> > +    QTAILQ_INSERT_TAIL(&node->properties, prop, list);
> > +    return prop;
> > +}
> > +
> > +const QDTProperty *qdt_setprop_string(QDTNode *node, const gchar *name,
> > +                                      const gchar *val)
> > +{
> > +    return qdt_setprop(node, name, val, strlen(val) + 1);
> > +}
> > +
> > +const QDTProperty *qdt_setprop_cells_(QDTNode *node, const gchar *name,
> > +                                      const uint32_t *val, gsize len)
> > +{
> > +    uint32_t swapval[len];
> > +    gsize i;
> > +
> > +    for (i = 0; i < len; i++) {
> > +        swapval[i] = cpu_to_fdt32(val[i]);
> > +    }
> > +    return qdt_setprop(node, name, swapval, sizeof(swapval));
> > +}
> > +
> > +const QDTProperty *qdt_setprop_u64s_(QDTNode *node, const char *name,
> > +                                     const uint64_t *val, gsize len)
> > +{
> > +    uint64_t swapval[len];
> > +    gsize i;
> > +
> > +    for (i = 0; i < len; i++) {
> > +        swapval[i] = cpu_to_fdt64(val[i]);
> > +    }
> > +    return qdt_setprop(node, name, swapval, sizeof(swapval));
> > +}
> > +
> > +void qdt_set_phandle(QDTNode *node, uint32_t phandle)
> > +{
> > +    g_assert((phandle != 0) && (phandle != (uint32_t)-1));
> > +    qdt_setprop_cells(node, "linux,phandle", phandle);
> 
> Do we still need "linux,phandle" nowadays? ... maybe that would be a
> good point in time now to slowly get rid of this?
> At least the ARM code seems to work already without that, and pseries
> should also be fine without it (since SLOF replaces "phandle" and
> "linux,phandle" anyway).

Erm.. maybe.  IIRC the addition of support for the new property to the
early boot kernel code on Power was surprisingly late, so it seems
safer to include it, but, we can check.

> > +    qdt_setprop_cells(node, "phandle", phandle);
> > +}
> > +
> > +/*
> > + * Whole tree functions
> > + */
> > +
> > +static void qdt_flatten_node(void *fdt, QDTNode *node, Error **errp)
> > +{
> > +    QDTProperty *prop;
> > +    QDTNode *subnode;
> > +    Error *local_err = NULL;
> > +    int ret;
> > +
> > +    ret = fdt_begin_node(fdt, node->name);
> > +    if (ret < 0) {
> > +        error_setg(errp, "Error flattening device tree: fdt_begin_node(): 
> > %s",
> > +                   fdt_strerror(ret));
> > +        return;
> > +    }
> > +
> > +    QTAILQ_FOREACH(prop, &node->properties, list) {
> > +        ret = fdt_property(fdt, prop->name, prop->val, prop->len);
> > +        if (ret < 0) {
> > +            error_setg(errp, "Error flattening device tree: 
> > fdt_property(): %s",
> > +                       fdt_strerror(ret));
> > +            return;
> > +        }
> > +    }
> > +
> > +    QTAILQ_FOREACH(subnode, &node->children, sibling) {
> > +        qdt_flatten_node(fdt, subnode, &local_err);
> > +        if (local_err) {
> > +            error_propagate(errp, local_err);
> > +            return;
> > +        }
> > +    }
> > +
> > +    ret = fdt_end_node(fdt);
> > +    if (ret < 0) {
> > +        error_setg(errp, "Error flattening device tree: fdt_end_node(): 
> > %s",
> > +                   fdt_strerror(ret));
> > +        return;
> > +    }
> > +}
> > +
> > +void *qdt_flatten(QDTNode *root, gsize bufsize, Error **errp)
> > +{
> > +    void *fdt = g_malloc0(bufsize);
> > +    Error *local_err = NULL;
> > +    int ret;
> 
> So the caller has already to know here, how big the flattened device
> tree will be at max? That's a somewhat cumbersome interface. Would it be
> feasible to determine the size automatically somehow?

Yes, I was planningto add an autosizing flatten interface, but I
wanted to get the basic concept reviewed before I bothered
implementing that.  It shouldn't be that hard - just realloc()
whenever one of the FDT calls returns -FDT_ERR_NOSPACE.

> Or maybe at least make the caller provide the buffer to the fdt array...
> then the caller provides both, buffer pointer and size. That would be
> somewhat more consistent interface, I think.

Well, I was thinking that we could have the autosizing interface in
the same function by redefining that as a max size, and having 0 or -1
mean no maximum.

> > +    assert(!root->parent); /* Should be a root node */
> > +
> > +    ret = fdt_create(fdt, bufsize);
> > +    if (ret < 0) {
> > +        error_setg(errp, "Error flattening device tree: fdt_create(): %s",
> > +                   fdt_strerror(ret));
> > +        goto fail;
> > +    }
> > +
> > +    ret = fdt_finish_reservemap(fdt);
> > +    if (ret < 0) {
> > +        error_setg(errp,
> > +                   "Error flattening device tree: fdt_finish_reservemap(): 
> > %s",
> > +                   fdt_strerror(ret));
> > +        goto fail;
> > +    }
> > +
> > +    qdt_flatten_node(fdt, root, &local_err);
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> > +        goto fail;
> > +    }
> > +
> > +    ret = fdt_finish(fdt);
> > +    if (ret < 0) {
> > +        error_setg(errp, "Error flattening device tree: fdt_finish(): %s",
> > +                   fdt_strerror(ret));
> > +        goto fail;
> > +    }
> 
> Maybe add a sanity check a la "fdt_totalsize(fdt) < bufsize" here and
> abort on buffer overflow?

There's really no point - the fdt functions can't succeed if they
overflow the buffer space, so we will have already tripped an error.
The only libfdt functions which will *ever* change fdt_totalsize() are
fdt_create(), fdt_open_int(), fdt_finish() and fdt_pack() and the last
two can only ever reduce it, never increase it.

> (or move such a check even into qdt_flatten_node ?)
> 
> > +    return fdt;
> > +
> > +fail:
> > +    g_free(fdt);
> > +    return NULL;
> > +}
> 
>  Thomas
> 

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

signature.asc
Description: PGP signature