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

On Mon, Nov 28, 2016 at 02:10:35PM +0200, Pantelis Antoniou wrote:

quoted

On Nov 28, 2016, at 06:12 , David Gibson [off-list ref] wrote:

On Fri, Nov 25, 2016 at 02:32:10PM +0200, Pantelis Antoniou wrote:

quoted

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 <redacted>
Signed-off-by: Jan Luebbe <redacted>
---
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.

quoted

quoted

%}

%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.

quoted

quoted

+					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