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

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

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

On Tue, Nov 29, 2016 at 01:09:11PM +0200, Pantelis Antoniou wrote:
> Hi David,
> 
> > On Nov 29, 2016, at 04:10 , David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > 
> > 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__.
> > 
> 
> It is easily done. Although using /plugin/ as an auto-magic option does both
> just fine.

Sorry, I don't follow what you're saying.

> >> 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.
> > 
> 
> Hmm, yeah.
> 
> 
> Regards
> 
> — Pantelis
> 

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