Re: [PATCH v6 2/4] dtc: Plugin and fixup support

[David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>]

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

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-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Signed-off-by: Jan Luebbe <jlu-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> ---
>  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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]