On Thu, May 05, 2016 at 10:48:42PM +0300, Pantelis Antoniou wrote:
> This patch enable the generation of symbols & local fixup information
> for trees compiled with the -@ (--symbols) option.
>
> Using this patch labels in the tree and their users emit information
> in __symbols__ and __local_fixups__ nodes.
>
> The __fixups__ node make possible the dynamic resolution of phandle
> references which are present in the plugin tree but lie in the
> tree that are applying the overlay against.
>
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@xxxxxxxxxxxx>
> Signed-off-by: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
> Signed-off-by: Jan Luebbe <jlu@xxxxxxxxxxxxxx>
> ---
> Documentation/manual.txt | 16 ++++
> checks.c | 8 +-
> dtc-lexer.l | 5 ++
> dtc-parser.y | 42 +++++++--
> dtc.c | 23 ++++-
> dtc.h | 28 +++++-
> flattree.c | 2 +-
> fstree.c | 2 +-
> livetree.c | 217 ++++++++++++++++++++++++++++++++++++++++++++++-
> 9 files changed, 329 insertions(+), 14 deletions(-)
>
> diff --git a/Documentation/manual.txt b/Documentation/manual.txt
> index 398de32..3e16c24 100644
> --- a/Documentation/manual.txt
> +++ b/Documentation/manual.txt
> @@ -119,6 +119,20 @@ Options:
> Make space for <number> reserve map entries
> Relevant for dtb and asm output only.
>
> + -@
> + Generates a __symbols__ node at the root node of the resulting blob
> + for any node labels used, and for any local references using phandles
> + it also generates a __local_fixups__ node that tracks them.
> +
> + When using the /plugin/ tag all unresolved label references to
> + be tracked in the __fixups__ node, making dynamic resolution possible.
> +
> + -A
> + Generate automatically aliases for all node labels. This is similar to
> + the -@ option (the __symbols__ node contain identical information) but
> + the semantics are slightly different since no phandles are automatically
> + generated for labeled nodes.
> +
> -S <bytes>
> Ensure the blob at least <bytes> long, adding additional
> space if needed.
> @@ -153,6 +167,8 @@ Here is a very rough overview of the layout of a DTS source file:
>
> devicetree: '/' nodedef
>
> + plugindecl: '/' 'plugin' '/' ';'
You added the new non-terminal definitiom, but didn't use it anywhere.
> nodedef: '{' list_of_property list_of_subnode '}' ';'
>
> property: label PROPNAME '=' propdata ';'
> diff --git a/checks.c b/checks.c
> index 386f956..3d4c3c6 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -490,8 +490,12 @@ static void fixup_phandle_references(struct check *c, struct node *dt,
>
> refnode = get_node_by_ref(dt, m->ref);
> if (! refnode) {
> - FAIL(c, "Reference to non-existent node or label \"%s\"\n",
> - m->ref);
> + if (!(tree_get_versionflags(dt) & VF_PLUGIN))
> + FAIL(c, "Reference to non-existent node or "
> + "label \"%s\"\n", m->ref);
> + else /* mark the entry as unresolved */
> + *((cell_t *)(prop->val.val + m->offset)) =
> + cpu_to_fdt32(0xffffffff);
> continue;
> }
>
> diff --git a/dtc-lexer.l b/dtc-lexer.l
> index 790fbf6..40bbc87 100644
> --- a/dtc-lexer.l
> +++ b/dtc-lexer.l
> @@ -121,6 +121,11 @@ static void lexical_error(const char *fmt, ...);
> return DT_V1;
> }
>
> +<*>"/plugin/" {
> + DPRINT("Keyword: /plugin/\n");
> + return DT_PLUGIN;
> + }
> +
> <*>"/memreserve/" {
> DPRINT("Keyword: /memreserve/\n");
> BEGIN_DEFAULT();
> diff --git a/dtc-parser.y b/dtc-parser.y
> index 000873f..2d3399e 100644
> --- a/dtc-parser.y
> +++ b/dtc-parser.y
> @@ -19,6 +19,7 @@
> */
> %{
> #include <stdio.h>
> +#include <inttypes.h>
>
> #include "dtc.h"
> #include "srcpos.h"
> @@ -52,9 +53,11 @@ extern bool treesource_error;
> struct node *nodelist;
> struct reserve_info *re;
> uint64_t integer;
> + unsigned int flags;
> }
>
> %token DT_V1
> +%token DT_PLUGIN
> %token DT_MEMRESERVE
> %token DT_LSHIFT DT_RSHIFT DT_LE DT_GE DT_EQ DT_NE DT_AND DT_OR
> %token DT_BITS
> @@ -71,6 +74,8 @@ extern bool treesource_error;
>
> %type <data> propdata
> %type <data> propdataprefix
> +%type <flags> versioninfo
> +%type <flags> plugindecl
> %type <re> memreserve
> %type <re> memreserves
> %type <array> arrayprefix
> @@ -101,13 +106,31 @@ extern bool treesource_error;
> %%
>
> sourcefile:
> - DT_V1 ';' memreserves devicetree
> + versioninfo ';' memreserves devicetree
> {
> - the_boot_info = build_boot_info($3, $4,
> + the_boot_info = build_boot_info($1, $3, $4,
> guess_boot_cpuid($4));
> }
> ;
>
> +versioninfo:
> + DT_V1 plugindecl
> + {
> + $$ = VF_DT_V1 | $2;
> + }
> + ;
> +
> +plugindecl:
> + DT_PLUGIN
> + {
> + $$ = VF_PLUGIN;
> + }
> + | /* empty */
> + {
> + $$ = 0;
> + }
> + ;
> +
> memreserves:
> /* empty */
> {
> @@ -156,10 +179,14 @@ devicetree:
> {
> struct node *target = get_node_by_ref($1, $2);
>
> - if (target)
> + if (target) {
> merge_nodes(target, $3);
> - else
> - ERROR(&@2, "Label or path %s not found", $2);
> + } else {
> + if (symbol_fixup_support)
Surely this should depend on the /plugin/ tag, rather than the -@ option..?
> + add_orphan_node($1, $3, $2);
> + else
> + ERROR(&@2, "Label or path %s not found", $2);
> + }
> $$ = $1;
> }
> | devicetree DT_DEL_NODE DT_REF ';'
> @@ -174,6 +201,11 @@ devicetree:
>
> $$ = $1;
> }
> + | /* empty */
> + {
> + /* build empty node */
> + $$ = name_node(build_node(NULL, NULL), "");
> + }
> ;
>
> nodedef:
> diff --git a/dtc.c b/dtc.c
> index 5fa23c4..f8277fb 100644
> --- a/dtc.c
> +++ b/dtc.c
> @@ -31,6 +31,8 @@ int reservenum; /* Number of memory reservation slots */
> int minsize; /* Minimum blob size */
> int padsize; /* Additional padding to blob */
> int phandle_format = PHANDLE_BOTH; /* Use linux,phandle or phandle properties */
> +int symbol_fixup_support;
> +int auto_label_aliases;
>
> static void fill_fullpaths(struct node *tree, const char *prefix)
> {
> @@ -53,7 +55,7 @@ static void fill_fullpaths(struct node *tree, const char *prefix)
> #define FDT_VERSION(version) _FDT_VERSION(version)
> #define _FDT_VERSION(version) #version
> static const char usage_synopsis[] = "dtc [options] <input file>";
> -static const char usage_short_opts[] = "qI:O:o:V:d:R:S:p:fb:i:H:sW:E:hv";
> +static const char usage_short_opts[] = "qI:O:o:V:d:R:S:p:fb:i:H:sW:E:@Ahv";
> static struct option const usage_long_opts[] = {
> {"quiet", no_argument, NULL, 'q'},
> {"in-format", a_argument, NULL, 'I'},
> @@ -71,6 +73,8 @@ static struct option const usage_long_opts[] = {
> {"phandle", a_argument, NULL, 'H'},
> {"warning", a_argument, NULL, 'W'},
> {"error", a_argument, NULL, 'E'},
> + {"symbols", no_argument, NULL, '@'},
> + {"auto-alias", no_argument, NULL, 'A'},
> {"help", no_argument, NULL, 'h'},
> {"version", no_argument, NULL, 'v'},
> {NULL, no_argument, NULL, 0x0},
> @@ -101,6 +105,8 @@ static const char * const usage_opts_help[] = {
> "\t\tboth - Both \"linux,phandle\" and \"phandle\" properties",
> "\n\tEnable/disable warnings (prefix with \"no-\")",
> "\n\tEnable/disable errors (prefix with \"no-\")",
> + "\n\tEnable symbols/fixup support",
> + "\n\tEnable auto-alias of labels",
> "\n\tPrint this help and exit",
> "\n\tPrint version and exit",
> NULL,
> @@ -233,7 +239,12 @@ int main(int argc, char *argv[])
> case 'E':
> parse_checks_option(false, true, optarg);
> break;
> -
> + case '@':
> + symbol_fixup_support = 1;
> + break;
> + case 'A':
> + auto_label_aliases = 1;
> + break;
> case 'h':
> usage(NULL);
> default:
> @@ -294,6 +305,14 @@ int main(int argc, char *argv[])
> if (sort)
> sort_tree(bi);
>
> + if (auto_label_aliases)
> + generate_label_tree(bi->dt, "aliases", false);
> +
> + if (symbol_fixup_support) {
> + generate_label_tree(bi->dt, "__symbols__", true);
> + generate_fixups_tree(bi->dt);
> + }
> +
These should go before the sort_tree() - since they add nodes, they
could result in something that *isn't* sorted when that was requested.
> if (streq(outname, "-")) {
> outf = stdout;
> } else {
> diff --git a/dtc.h b/dtc.h
> index 56212c8..68cb109 100644
> --- a/dtc.h
> +++ b/dtc.h
> @@ -20,7 +20,6 @@
> * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307
> * USA
> */
> -
Unrelated whitespace change.
> #include <stdio.h>
> #include <string.h>
> #include <stdlib.h>
> @@ -54,6 +53,12 @@ extern int reservenum; /* Number of memory reservation slots */
> extern int minsize; /* Minimum blob size */
> extern int padsize; /* Additional padding to blob */
> extern int phandle_format; /* Use linux,phandle or phandle properties */
> +extern int symbol_fixup_support;/* enable symbols & fixup support */
> +extern int auto_label_aliases; /* auto generate labels -> aliases */
> +
> +/*
> + * Tree source globals
> + */
>
> #define PHANDLE_LEGACY 0x1
> #define PHANDLE_EPAPR 0x2
> @@ -158,6 +163,9 @@ struct node {
> int addr_cells, size_cells;
>
> struct label *labels;
> +
> + /* only for the root (parent == NULL) */
> + struct boot_info *bi;
Ugh.. I hate creating circular references unless we really can't avoid
them.
> };
>
> #define for_each_label_withdel(l0, l) \
> @@ -194,6 +202,7 @@ struct node *build_node_delete(void);
> struct node *name_node(struct node *node, char *name);
> struct node *chain_node(struct node *first, struct node *list);
> struct node *merge_nodes(struct node *old_node, struct node *new_node);
> +void add_orphan_node(struct node *old_node, struct node *new_node, char *ref);
>
> void add_property(struct node *node, struct property *prop);
> void delete_property_by_name(struct node *node, char *name);
> @@ -236,14 +245,29 @@ struct reserve_info *add_reserve_entry(struct reserve_info *list,
>
>
> struct boot_info {
> + unsigned int versionflags;
> struct reserve_info *reservelist;
> struct node *dt; /* the device tree */
> uint32_t boot_cpuid_phys;
> };
>
> -struct boot_info *build_boot_info(struct reserve_info *reservelist,
> +/* version flags definitions */
> +#define VF_DT_V1 0x0001 /* /dts-v1/ */
Is there any point to this, since dtc doesn't support v0 dts files any
more anyway?
> +#define VF_PLUGIN 0x0002 /* /plugin/ */
> +
> +static inline unsigned int tree_get_versionflags(struct node *dt)
> +{
> + if (!dt || !dt->bi)
> + return 0;
> + return dt->bi->versionflags;
> +}
> +
> +struct boot_info *build_boot_info(unsigned int versionflags,
> + struct reserve_info *reservelist,
> struct node *tree, uint32_t boot_cpuid_phys);
> void sort_tree(struct boot_info *bi);
> +void generate_label_tree(struct node *dt, char *gen_node_name, bool allocph);
> +void generate_fixups_tree(struct node *dt);
>
> /* Checks */
>
> diff --git a/flattree.c b/flattree.c
> index ec14954..5dde832 100644
> --- a/flattree.c
> +++ b/flattree.c
> @@ -929,5 +929,5 @@ struct boot_info *dt_from_blob(const char *fname)
>
> fclose(f);
>
> - return build_boot_info(reservelist, tree, boot_cpuid_phys);
> + return build_boot_info(VF_DT_V1, reservelist, tree, boot_cpuid_phys);
> }
> diff --git a/fstree.c b/fstree.c
> index 6d1beec..54f520b 100644
> --- a/fstree.c
> +++ b/fstree.c
> @@ -86,6 +86,6 @@ struct boot_info *dt_from_fs(const char *dirname)
> tree = read_fstree(dirname);
> tree = name_node(tree, "");
>
> - return build_boot_info(NULL, tree, guess_boot_cpuid(tree));
> + return build_boot_info(VF_DT_V1, NULL, tree, guess_boot_cpuid(tree));
> }
>
> diff --git a/livetree.c b/livetree.c
> index e229b84..e7d0fe5 100644
> --- a/livetree.c
> +++ b/livetree.c
> @@ -18,6 +18,7 @@
> * USA
> */
>
> +#define _GNU_SOURCE
Ugh. I mean, I know the work I did to get dtc working on BSD has
bitrotted already, but can we avoid gratuitously introducing
non-portability.
> #include "dtc.h"
>
> /*
> @@ -216,6 +217,34 @@ struct node *merge_nodes(struct node *old_node, struct node *new_node)
> return old_node;
> }
>
> +void add_orphan_node(struct node *dt, struct node *new_node, char *ref)
> +{
> + static unsigned int next_orphan_fragment = 0;
> + struct node *node = xmalloc(sizeof(*node));
> + struct property *p;
> + struct data d = empty_data;
> + char *name;
> + int ret;
> +
> + memset(node, 0, sizeof(*node));
> +
> + d = data_add_marker(d, REF_PHANDLE, ref);
> + d = data_append_integer(d, 0xffffffff, 32);
> +
> + p = build_property("target", d);
> + add_property(node, p);
> +
> + ret = asprintf(&name, "fragment@%u",
> + next_orphan_fragment++);
> + if (ret == -1)
> + die("asprintf() failed\n");
> + name_node(node, name);
> + name_node(new_node, "__overlay__");
> +
> + add_child(dt, node);
> + add_child(node, new_node);
> +}
> struct node *chain_node(struct node *first, struct node *list)
> {
> assert(first->next_sibling == NULL);
> @@ -335,15 +364,19 @@ struct reserve_info *add_reserve_entry(struct reserve_info *list,
> return list;
> }
>
> -struct boot_info *build_boot_info(struct reserve_info *reservelist,
> +struct boot_info *build_boot_info(unsigned int versionflags,
> + struct reserve_info *reservelist,
> struct node *tree, uint32_t boot_cpuid_phys)
> {
> struct boot_info *bi;
>
> bi = xmalloc(sizeof(*bi));
> + bi->versionflags = versionflags;
> bi->reservelist = reservelist;
> bi->dt = tree;
> bi->boot_cpuid_phys = boot_cpuid_phys;
> + /* link back */
> + tree->bi = bi;
>
> return bi;
> }
> @@ -709,3 +742,185 @@ void sort_tree(struct boot_info *bi)
> sort_reserve_entries(bi);
> sort_node(bi->dt);
> }
> +
> +/* utility helper to avoid code duplication */
> +static struct node *build_and_name_child_node(struct node *parent, char *name)
> +{
> + struct node *node;
> +
> + node = build_node(NULL, NULL);
> + name_node(node, xstrdup(name));
> + add_child(parent, node);
> +
> + return node;
> +}
> +
> +static void generate_label_tree_internal(struct node *dt, struct node *node,
> + char *gen_node_name, bool allocph)
> +{
> + struct node *c, *an;
> + struct property *p;
> + struct label *l;
> +
> + /* if if there are labels */
> + if (node->labels) {
> + /* an is the aliases/__symbols__ node */
> + an = get_subnode(dt, gen_node_name);
You can factor this out to the caller and just pass 'an' in.
> + /* if no node exists, create it */
Comment does not match the code
> + if (!an)
> + die("Could not get label node\n");
> +
> + /* now add the label in the node */
> + for_each_label(node->labels, l) {
> + /* check whether the label already exists */
> + p = get_property(an, l->label);
> + if (p) {
> + fprintf(stderr, "WARNING: label %s already"
> + " exists in /%s", l->label,
> + gen_node_name);
> + continue;
> + }
> +
> + /* insert it */
> + p = build_property(l->label,
> + data_copy_escape_string(node->fullpath,
> + strlen(node->fullpath)));
> + add_property(an, p);
> + }
> +
> + /* force allocation of a phandle for this node */
> + if (allocph)
> + (void)get_node_phandle(dt, node);
> + }
> +
> + for_each_child(node, c)
> + generate_label_tree_internal(dt, c, gen_node_name, allocph);
> +}
> +
> +void generate_label_tree(struct node *dt, char *gen_node_name, bool allocph)
> +{
> + build_and_name_child_node(dt, gen_node_name);
> + generate_label_tree_internal(dt, dt, gen_node_name, allocph);
> +}
> +
> +static char *fixups_name = "__fixups__";
> +static char *local_fixups_name = "__local_fixups__";
> +
> +static void add_fixup_entry(struct node *dt, struct node *node,
> + struct property *prop, struct marker *m)
> +{
> + struct node *fn; /* local fixup node */
Comment is misleading since this is __fixups__ rather than
__local_fixups__.
> + struct property *p;
> + struct data d;
> + char *entry;
> + int ret;
> +
> + /* m->ref can only be a REF_PHANDLE, but check anyway */
> + if (m->type != REF_PHANDLE)
> + die("Fixup entry can only be a ref to a phandle\n");
If it's not a REF_PHANDLE, that indicates a bug elsewhere, yes? In
which case assert() would be more appropriate.
> + /* fn is the node we're putting entries in */
> + fn = get_subnode(dt, fixups_name);
> + if (!fn)
> + die("Can't get fixups node\n");
Again, assert() would be better.
> + /* there shouldn't be any ':' in the arguments */
> + if (strchr(node->fullpath, ':') || strchr(prop->name, ':'))
> + die("arguments should not contain ':'\n");
> +
> + ret = asprintf(&entry, "%s:%s:%u",
> + node->fullpath, prop->name, m->offset);
> + if (ret == -1)
> + die("asprintf() failed\n");
Looks like we really should put in an xasprintf() utility function,
both to address the BSD portability, and avoid clunky checks for
allocation failures.
> + p = get_property(fn, m->ref);
> + if (p) {
> + d = data_append_data(p->val, entry, strlen(entry) + 1);
> + p->val = d;
> + } else {
> + d = data_append_data(empty_data, entry, strlen(entry) + 1);
> + add_property(fn, build_property(m->ref, d));
> + }
> +}
> +
> +static void add_local_fixup_entry(struct node *dt, struct node *node,
> + struct property *prop, struct marker *m,
> + struct node *refnode)
> +{
> + struct node *lfn, *wn, *nwn; /* local fixup node, walk node, new */
> + struct property *p;
> + struct data d;
> + char *s, *e, *comp;
> + int len;
> +
> + /* fn is the node we're putting entries in */
> + lfn = get_subnode(dt, local_fixups_name);
> + if (!lfn)
> + die("Can't get local fixups node\n");
Again, assert().
> + /* walk the path components creating nodes if they don't exist */
> + comp = xmalloc(strlen(node->fullpath) + 1);
> + /* start skipping the first / */
> + s = node->fullpath + 1;
> + wn = lfn;
> + while (*s) {
Given the break; below is the loop condition redundant?
> + /* retrieve path component */
> + e = strchr(s, '/');
> + if (e == NULL)
> + e = s + strlen(s);
> + len = e - s;
> + memcpy(comp, s, len);
> + comp[len] = '\0';
> +
> + /* if no node exists, create it */
> + nwn = get_subnode(wn, comp);
> + if (!nwn)
> + nwn = build_and_name_child_node(wn, comp);
> + wn = nwn;
> +
> + /* last path component */
> + if (!*e)
> + break;
> +
> + /* next path component */
> + s = e + 1;
> + }
> + free(comp);
> +
> + p = get_property(wn, prop->name);
> + d = data_append_cell(p ? p->val : empty_data, (cell_t)m->offset);
> + if (!p)
> + add_property(wn, build_property(prop->name, d));
> + else
> + p->val = d;
Hmm.. am append_to_property helper would simplify both this and the
other fixup function.
> +}
> +
> +static void generate_fixups_tree_internal(struct node *dt, struct node *node)
> +{
> + struct node *c;
> + struct property *prop;
> + struct marker *m;
> + struct node *refnode;
> +
> + for_each_property(node, prop) {
> + m = prop->val.markers;
> + for_each_marker_of_type(m, REF_PHANDLE) {
> + refnode = get_node_by_ref(dt, m->ref);
> + if (!refnode)
> + add_fixup_entry(dt, node, prop, m);
> + else
> + add_local_fixup_entry(dt, node, prop, m,
> + refnode);
> + }
> + }
> +
> + for_each_child(node, c)
> + generate_fixups_tree_internal(dt, c);
> +}
> +
> +void generate_fixups_tree(struct node *dt)
> +{
> + build_and_name_child_node(dt, fixups_name);
> + build_and_name_child_node(dt, local_fixups_name);
> + generate_fixups_tree_internal(dt, dt);
> +}
--
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:
signature.asc
Description: PGP signature
