On Tue, Jan 27, 2026 at 3:19 AM David Gibson <[email protected]> wrote: > > On Mon, Jan 26, 2026 at 06:42:48PM +0100, Ruslan Ruslichenko wrote: > > From: Ruslan Ruslichenko <[email protected]> > > > > The patch adds few utility functions for parsing FDT nodes. > > The helpers are required for upcoming Hardware device tree > > feature. > > > > Signed-off-by: Ruslan Ruslichenko <[email protected]> > > --- > > include/system/device_tree.h | 30 ++++++ > > system/device_tree.c | 203 +++++++++++++++++++++++++++++++++++ > > 2 files changed, 233 insertions(+) > > > > diff --git a/include/system/device_tree.h b/include/system/device_tree.h > > index f34b8b7ef9..458dbb74b4 100644 > > --- a/include/system/device_tree.h > > +++ b/include/system/device_tree.h > > @@ -90,6 +90,13 @@ int qemu_fdt_setprop_string_array(void *fdt, const char > > *node_path, > > int qemu_fdt_setprop_phandle(void *fdt, const char *node_path, > > const char *property, > > const char *target_node_path); > > + > > +uint64_t qemu_fdt_getprop_sized_cell(void *fdt, const char *node_path, > > + const char *property, int offset, > > + int size, Error **errp); > > +const char *qemu_fdt_getprop_string(void *fdt, const char*node_path, > > + const char *property, int cell, > > + bool inherit, Error **errp); > > /** > > * qemu_fdt_getprop: retrieve the value of a given property > > * @fdt: pointer to the device tree blob > > @@ -193,9 +200,32 @@ int qemu_fdt_setprop_sized_cells_from_array(void *fdt, > > qdt_tmp); \ > > }) > > > > +typedef struct QEMUDevtreeProp { > > + char *name; > > + int len; > > + void *value; > > +} QEMUDevtreeProp; > > + > > +/* node queries */ > > + > > +char *qemu_devtree_get_node_name(void *fdt, const char *node_path); > > +int qemu_devtree_get_num_children(void *fdt, const char *node_path, int > > depth); > > +char **qemu_devtree_get_children(void *fdt, const char *node_path, int > > depth); > > +int qemu_devtree_num_props(void *fdt, const char *node_path); > > +QEMUDevtreeProp *qemu_devtree_get_props(void *fdt, const char *node_path); > > > > +/* node getters */ > > + > > +int qemu_devtree_get_node_by_name(void *fdt, char *node_path, > > + const char *cmpname); > > +int qemu_devtree_get_node_by_phandle(void *fdt, char *node_path, int > > phandle); > > int qemu_devtree_getparent(void *fdt, char *node_path, > > const char *current); > > +int qemu_devtree_get_root_node(void *fdt, char *node_path); > > + > > +/* misc */ > > + > > +int devtree_get_num_nodes(void *fdt); > > > > #define DT_PATH_LENGTH 1024 > > > > diff --git a/system/device_tree.c b/system/device_tree.c > > index 41bde0ba5a..7091a4928e 100644 > > --- a/system/device_tree.c > > +++ b/system/device_tree.c > > @@ -451,6 +451,50 @@ const void *qemu_fdt_getprop(void *fdt, const char > > *node_path, > > return r; > > } > > > > +const char *qemu_fdt_getprop_string(void *fdt, const char*node_path, > > + const char *property, int cell, > > + bool inherit, Error **errp) > > +{ > > + int len; > > + const void *prop; > > + Error *err = NULL; > > + > > + if (!errp) { > > + errp = &err; > > + } > > + > > + prop = qemu_fdt_getprop(fdt, node_path, property, &len, inherit, errp); > > + if (*errp) { > > + return NULL; > > + } > > + while (cell) { > > In DT context, "cell" refers specifically to a 32-bit integer value. > It's not the right word for which string in a stringlist you retrieve. > > Also, most of this is re-implementing fdt_stringlist_get() from libfdt. > > > + void *term = memchr(prop, '\0', len); > > + size_t diff; > > + > > + if (!term) { > > + error_setg(errp, "%s: Couldn't get %s/%s: %s", __func__, > > + node_path, property, fdt_strerror(len)); > > + return NULL; > > + } > > + diff = term - prop + 1; > > + len -= diff; > > + assert(len >= 0); > > + prop += diff; > > + cell--; > > + } > > + > > + if (!len) { > > + return NULL; > > + } > > + > > + if (!*(char *)prop) { > > + error_setg(errp, "%s: Couldn't get %s/%s: %s", __func__, > > + node_path, property, fdt_strerror(len)); > > + return NULL; > > + } > > + return prop; > > +} > > + > > uint32_t qemu_fdt_getprop_cell(void *fdt, const char *node_path, > > const char *property, int offset, > > bool inherit, Error **errp) > > @@ -471,6 +515,22 @@ uint32_t qemu_fdt_getprop_cell(void *fdt, const char > > *node_path, > > return be32_to_cpu(p[offset]); > > } > > > > +uint64_t qemu_fdt_getprop_sized_cell(void *fdt, const char *node_path, > > + const char *property, int offset, > > + int size, Error **errp) > > +{ > > Again, "cell" is 32-bits in DT context, you need a different word. > > > + uint64_t ret = 0; > > Since ret is a u64, 'size' must be either 1 or 2. So you might as > well just have a get_u64() helper (getprop_cell already does u32). > > > + for (; size; size--) { > > + ret <<= 32; > > + ret |= qemu_fdt_getprop_cell(fdt, node_path, property, offset++, > > false, > > + errp); > > + if (errp && *errp) { > > + return 0; > > + } > > + } > > + return ret; > > +} > > + > > uint32_t qemu_fdt_get_phandle(void *fdt, const char *path) > > { > > uint32_t r; > > @@ -638,6 +698,117 @@ out: > > return ret; > > } > > > > +char *qemu_devtree_get_node_name(void *fdt, const char *node_path) > > +{ > > + const char *ret = fdt_get_name(fdt, fdt_path_offset(fdt, node_path), > > NULL); > > This does a full scan of the fdt, which may be unexpected. > > > + return ret ? strdup(ret) : NULL; > > +} > > + > > +int qemu_devtree_num_props(void *fdt, const char *node_path) > > +{ > > + int offset = fdt_path_offset(fdt, node_path); > > + int ret = 0; > > + > > + for (offset = fdt_first_property_offset(fdt, offset); > > + offset != -FDT_ERR_NOTFOUND; > > + offset = fdt_next_property_offset(fdt, offset)) { > > + ret++; > > + } > > + return ret; > > +} > > + > > +QEMUDevtreeProp *qemu_devtree_get_props(void *fdt, const char *node_path) > > +{ > > + QEMUDevtreeProp *ret = g_new0(QEMUDevtreeProp, > > + qemu_devtree_num_props(fdt, node_path) > > + 1); > > + int offset = fdt_path_offset(fdt, node_path); > > + int i = 0; > > + > > + for (offset = fdt_first_property_offset(fdt, offset); > > + offset != -FDT_ERR_NOTFOUND; > > + offset = fdt_next_property_offset(fdt, offset)) { > > + const char *propname; > > + const void *val = fdt_getprop_by_offset(fdt, offset, &propname, > > + &ret[i].len); > > + > > + ret[i].name = g_strdup(propname); > > + ret[i].value = g_memdup2(val, ret[i].len); > > + i++; > > + } > > + return ret; > > How does the caller free the structure? Using g_free() will leak all > the copied names and values. > > > +} > > + > > +static void qemu_devtree_children_info(void *fdt, const char *node_path, > > + int depth, int *num, char **returned_paths) { > > + int offset = fdt_path_offset(fdt, node_path); > > + int root_depth = fdt_node_depth(fdt, offset); > > "root" implies the root of the whole tree, not of the node where you > started. Also, two full scans of the tree above. > > > + int cur_depth = root_depth; > > + > > + if (num) { > > + *num = 0; > > + } > > + for (;;) { > > + offset = fdt_next_node(fdt, offset, &cur_depth); > > + if (cur_depth <= root_depth) { > > + break; > > + } > > + if (cur_depth <= root_depth + depth || depth == 0) { > > + if (returned_paths) { > > + returned_paths[*num] = g_malloc0(DT_PATH_LENGTH); > > No test to avoid overrunning the length of the returned_paths[] array > here. > > > + fdt_get_path(fdt, offset, returned_paths[*num], > > DT_PATH_LENGTH); > > Another full tree scan for every entry here. > > > + } > > + if (num) { > > + (*num)++; > > + } > > + } > > + } > > +} > > + > > +char **qemu_devtree_get_children(void *fdt, const char *node_path, int > > depth) > > +{ > > + int num_children = qemu_devtree_get_num_children(fdt, node_path, > > depth); > > + char **ret = g_malloc0(sizeof(*ret) * num_children); > > + > > + qemu_devtree_children_info(fdt, node_path, depth, &num_children, ret); > > + return ret; > > +} > > + > > +int qemu_devtree_get_num_children(void *fdt, const char *node_path, int > > depth) > > +{ > > + int ret; > > + > > + qemu_devtree_children_info(fdt, node_path, depth, &ret, NULL); > > + return ret; > > +} > > + > > +int qemu_devtree_get_node_by_name(void *fdt, char *node_path, > > + const char *cmpname) { > > The semantics of this make no sense to me. It finds the first node > with the name after the given node, whether it's a child, sibling or > unrelated node. I'm guessing you want something based on > fdt_subnode_offset() instead. > > > + int offset = 0; > > + char *name = NULL; > > + > > + do { > > + char *at; > > + > > + offset = fdt_next_node(fdt, offset, NULL); > > No test for error here, before using 'offset'. > > > + name = (void *)fdt_get_name(fdt, offset, NULL); > > + if (!name) { > > + continue; > > The only errors from fdt_get_name() indicate either a corrupted DT, or > bad parameters. continuing at this point will not do anything useful. > > > + } > > + at = memchr(name, '@', strlen(name)); > > + if (!strncmp(name, cmpname, at ? at - name : strlen(name))) { > > + break; > > + } > > + } while (offset > 0); > > + return offset > 0 ? > > + fdt_get_path(fdt, offset, node_path, DT_PATH_LENGTH) : 1; > > +} > > + > > +int qemu_devtree_get_node_by_phandle(void *fdt, char *node_path, int > > phandle) > > +{ > > + return fdt_get_path(fdt, fdt_node_offset_by_phandle(fdt, phandle), > > + node_path, DT_PATH_LENGTH); > > Two full tree scans. > > > +} > > + > > int qemu_devtree_getparent(void *fdt, char *node_path, const char *current) > > { > > int offset = fdt_path_offset(fdt, current); > > @@ -648,6 +819,38 @@ int qemu_devtree_getparent(void *fdt, char *node_path, > > const char *current) > > fdt_get_path(fdt, parent_offset, node_path, DT_PATH_LENGTH) : 1; > > } > > > > +int qemu_devtree_get_root_node(void *fdt, char *node_path) > > +{ > > + return fdt_get_path(fdt, 0, node_path, DT_PATH_LENGTH); > > The root node's path is "/" by definition, you don't need this > function. > > > +} > > + > > +static void devtree_scan(void *fdt, int *num_nodes) > > "scan" is vague. The name should reflect what this actually does. > > > +{ > > + int depth = 0, offset = 0; > > + > > + if (num_nodes) { > > + *num_nodes = 0; > > + } > > + for (;;) { > > + offset = fdt_next_node(fdt, offset, &depth); > > + if (num_nodes) { > > + (*num_nodes)++; > > + } > > + if (offset <= 0 || depth <= 0) { > > + break; > > + } > > + } > > +} > > + > > +int devtree_get_num_nodes(void *fdt) > > +{ > > + int ret; > > + > > + devtree_scan(fdt, &ret); > > + return ret; > > +} > > + > > + > > void qmp_dumpdtb(const char *filename, Error **errp) > > { > > ERRP_GUARD(); > > -- > > 2.43.0 > > >
Hi David! Thank you for the detailed review for this and previous patches. I will follow up your comments in the v2 patch series. > -- > David Gibson (he or they) | 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
