Re: [PATCH v10 3/4] dtc: Plugin and fixup support

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

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

On Mon, Nov 28, 2016 at 02:10:35PM +0200, Pantelis Antoniou wrote:
> 
> > On Nov 28, 2016, at 06:12 , David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > 
> > On Fri, Nov 25, 2016 at 02:32:10PM +0200, 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.
> >> 
> >> While there is a new magic number for dynamic device tree/overlays blobs
> >> it is by default enabled. Remember to use -M to generate compatible
> >> blobs.
> >> 
> >> 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 |  25 +++++-
> >> checks.c                 |   8 +-
> >> dtc-lexer.l              |   5 ++
> >> dtc-parser.y             |  50 +++++++++--
> >> dtc.c                    |  39 +++++++-
> >> dtc.h                    |  20 ++++-
> >> fdtdump.c                |   2 +-
> >> flattree.c               |  17 ++--
> >> fstree.c                 |   2 +-
> >> libfdt/fdt.c             |   2 +-
> >> libfdt/fdt.h             |   3 +-
> >> livetree.c               | 225 ++++++++++++++++++++++++++++++++++++++++++++++-
> >> tests/mangle-layout.c    |   7 +-
> >> 13 files changed, 375 insertions(+), 30 deletions(-)
> >> 
> >> diff --git a/Documentation/manual.txt b/Documentation/manual.txt
> >> index 398de32..094893b 100644
> >> --- a/Documentation/manual.txt
> >> +++ b/Documentation/manual.txt
> >> @@ -119,6 +119,24 @@ 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.
> >> +
> >> +    -M
> >> +	Generate blobs with the old FDT magic number for device tree objects.
> >> +	By default blobs use the DTBO FDT magic number instead.
> >> +
> >>     -S <bytes>
> >> 	Ensure the blob at least <bytes> long, adding additional
> >> 	space if needed.
> >> @@ -146,13 +164,18 @@ Additionally, dtc performs various sanity checks on the tree.
> >> Here is a very rough overview of the layout of a DTS source file:
> >> 
> >> 
> >> -    sourcefile:   list_of_memreserve devicetree
> >> +    sourcefile:   versioninfo plugindecl list_of_memreserve devicetree
> >> 
> >>     memreserve:   label 'memreserve' ADDR ADDR ';'
> >> 		| label 'memreserve' ADDR '-' ADDR ';'
> >> 
> >>     devicetree:   '/' nodedef
> >> 
> >> +    versioninfo:  '/' 'dts-v1' '/' ';'
> >> +
> >> +    plugindecl:   '/' 'plugin' '/' ';'
> >> +                | /* empty */
> >> +
> >>     nodedef:      '{' list_of_property list_of_subnode '}' ';'
> >> 
> >>     property:     label PROPNAME '=' propdata ';'
> >> diff --git a/checks.c b/checks.c
> >> index 2bd27a4..4292f4b 100644
> >> --- a/checks.c
> >> +++ b/checks.c
> >> @@ -487,8 +487,12 @@ static void fixup_phandle_references(struct check *c, struct boot_info *bi,
> >> 
> >> 			refnode = get_node_by_ref(dt, m->ref);
> >> 			if (! refnode) {
> >> -				FAIL(c, "Reference to non-existent node or label \"%s\"\n",
> >> -				     m->ref);
> >> +				if (!(bi->versionflags & 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 14aaf2e..1a1f660 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"
> >> @@ -33,6 +34,7 @@ extern void yyerror(char const *s);
> >> 
> >> extern struct boot_info *the_boot_info;
> >> extern bool treesource_error;
> >> +
> > 
> > Extraneous whitespace change here
> > 
> 
> OK.
> 
> >> %}
> >> 
> >> %union {
> >> @@ -52,9 +54,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 +75,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,16 +107,34 @@ extern bool treesource_error;
> >> %%
> >> 
> >> sourcefile:
> >> -	  v1tag memreserves devicetree
> >> +	  versioninfo plugindecl memreserves devicetree
> >> +		{
> >> +			the_boot_info = build_boot_info($1 | $2, $3, $4,
> >> +							guess_boot_cpuid($4));
> >> +		}
> >> +	;
> >> +
> >> +versioninfo:
> >> +	v1tag
> >> 		{
> >> -			the_boot_info = build_boot_info($2, $3,
> >> -							guess_boot_cpuid($3));
> >> +			$$ = VF_DT_V1;
> >> 		}
> >> 	;
> >> 
> >> v1tag:
> >> 	  DT_V1 ';'
> >> +	| DT_V1
> >> 	| DT_V1 ';' v1tag
> >> +
> >> +plugindecl:
> >> +	DT_PLUGIN ';'
> >> +		{
> >> +			$$ = VF_PLUGIN;
> >> +		}
> >> +	| /* empty */
> >> +		{
> >> +			$$ = 0;
> >> +		}
> >> 	;
> >> 
> >> memreserves:
> >> @@ -161,10 +185,19 @@ 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 {
> >> +				/*
> >> +				 * We rely on the rule being always:
> >> +				 *   versioninfo plugindecl memreserves devicetree
> >> +				 * so $-1 is what we want (plugindecl)
> >> +				 */
> >> +				if ($<flags>-1 & VF_PLUGIN)
> > 
> > o_O... ok.  I've never seen negative value references before.  Can you
> > provide a link to some documentation saying this is actually supported
> > usage in bison?  I wasn't able to find it when I looked.
> > 
> 
> There is a section about inherited attributes in the flex & bison book by O’Reily.
> 
> https://books.google.gr/books?id=3Sr1V5J9_qMC&lpg=PP1&dq=flex%20bison&hl=el&pg=PP1#v=onepage&q=flex%20bison&f=false
> 
> There’s a direct link to the 2nd Edition of lex & yacc:
> 
> https://books.google.gr/books?id=fMPxfWfe67EC&lpg=PA183&ots=RcRSji2NAT&dq=yacc%20inherited%20attributes&hl=el&pg=PA183#v=onepage&q=yacc%20inherited%20attributes&f=false

Thanks for the link.  I still think moving the fragment assembly out
of the parser will be a better idea long term, but this does address
the main concern I had, so it will do for now.

> >> +					add_orphan_node($1, $3, $2);
> >> +				else
> >> +					ERROR(&@2, "Label or path %s not found", $2);
> >> +			}
> >> 			$$ = $1;
> >> 		}
> >> 	| devicetree DT_DEL_NODE DT_REF ';'
> >> @@ -179,6 +212,11 @@ devicetree:
> >> 
> >> 			$$ = $1;
> >> 		}
> >> +	| /* empty */
> >> +		{
> >> +			/* build empty node */
> >> +			$$ = name_node(build_node(NULL, NULL), "");
> >> +		}
> >> 	;
> >> 
> >> nodedef:
> >> diff --git a/dtc.c b/dtc.c
> >> index 9dcf640..06e91bc 100644
> >> --- a/dtc.c
> >> +++ b/dtc.c
> >> @@ -32,6 +32,9 @@ int minsize;		/* Minimum blob size */
> >> int padsize;		/* Additional padding to blob */
> >> int alignsize;		/* Additional padding to blob accroding to the alignsize */
> >> int phandle_format = PHANDLE_BOTH;	/* Use linux,phandle or phandle properties */
> >> +int symbol_fixup_support;	/* enable symbols & fixup support */
> >> +int auto_label_aliases;		/* auto generate labels -> aliases */
> >> +int no_dtbo_magic;		/* use old FDT magic values for objects */
> >> 
> >> static int is_power_of_2(int x)
> >> {
> >> @@ -59,7 +62,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:a:fb:i:H:sW:E:hv";
> >> +static const char usage_short_opts[] = "qI:O:o:V:d:R:S:p:a:fb:i:H:sW:E:@AMhv";
> >> static struct option const usage_long_opts[] = {
> >> 	{"quiet",            no_argument, NULL, 'q'},
> >> 	{"in-format",         a_argument, NULL, 'I'},
> >> @@ -78,6 +81,9 @@ 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'},
> >> +	{"no-dtbo-magic",    no_argument, NULL, 'M'},
> >> 	{"help",             no_argument, NULL, 'h'},
> >> 	{"version",          no_argument, NULL, 'v'},
> >> 	{NULL,               no_argument, NULL, 0x0},
> >> @@ -109,6 +115,9 @@ 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\tDo not use DTBO magic value for plugin objects",
> >> 	"\n\tPrint this help and exit",
> >> 	"\n\tPrint version and exit",
> >> 	NULL,
> >> @@ -153,7 +162,7 @@ static const char *guess_input_format(const char *fname, const char *fallback)
> >> 	fclose(f);
> >> 
> >> 	magic = fdt32_to_cpu(magic);
> >> -	if (magic == FDT_MAGIC)
> >> +	if (magic == FDT_MAGIC || magic == FDT_MAGIC_DTBO)
> >> 		return "dtb";
> >> 
> >> 	return guess_type_by_name(fname, fallback);
> >> @@ -172,6 +181,7 @@ int main(int argc, char *argv[])
> >> 	FILE *outf = NULL;
> >> 	int outversion = DEFAULT_FDT_VERSION;
> >> 	long long cmdline_boot_cpuid = -1;
> >> +	fdt32_t out_magic = FDT_MAGIC;
> >> 
> >> 	quiet      = 0;
> >> 	reservenum = 0;
> >> @@ -249,6 +259,16 @@ int main(int argc, char *argv[])
> >> 			parse_checks_option(false, true, optarg);
> >> 			break;
> >> 
> >> +		case '@':
> >> +			symbol_fixup_support = 1;
> >> +			break;
> >> +		case 'A':
> >> +			auto_label_aliases = 1;
> >> +			break;
> >> +		case 'M':
> >> +			no_dtbo_magic = 1;
> >> +			break;
> >> +
> >> 		case 'h':
> >> 			usage(NULL);
> >> 		default:
> >> @@ -306,6 +326,14 @@ int main(int argc, char *argv[])
> >> 	fill_fullpaths(bi->dt, "");
> >> 	process_checks(force, 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);
> > 
> > Hang on.. this doesn't seem right.  I thought -@ controlled the
> > __symbols__ side (i.e. the part upon which we overlay) rather than the
> > fixups side (the part which overlays).  A dtbo could certainly have
> > both, of course, but for base trees, wouldn't you have symbols without
> > fixups?  And should it be illegal to try to build a /plugin/ without
> > -@?
> 
> It does control both for now. For base trees having the fixup nodes
> will allow us to do probe order dependency tracking in the future.

Erm.. how?

> For plugins we need the __symbols__ node to support stacked overlays, i.e.
> overlays referring label that were introduced by a previous overlay.

Yes, I realise that an overlay may well want __symbols__ as well.  But
they still seem conceptually different.  I think -@ should control
__symbols__ whereas /plugin/ should control __fixups__.

> For plugins there is no requirement for now to actually contain references to
> be resolved. It can easily be enforced though.

Sure, but I don't see the relevance of that here.  You could just omit
the __fixups__ node if there's nothing to go into them.

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