[PATCH v9 0/4] dtc: Dynamic DT support

[PATCH v9 0/4] dtc: Dynamic DT support

[Pantelis Antoniou]

This patchset adds Dynamic DT support in the DTC compiler
as used in a number of boards like the beaglebone/rpi/chip and others.

The first patch passes an extra boot_info argument to the check methods.
The second patch documents the internals of overlay generation, while
the third one adds dynamic object/overlay support proper.

The last patch simply adds a few overlay tests verifying operation.

This patchset is against DTC mainline and is also available for a pull
request from https://github.com/pantoniou/dtc/tree/overlays

Regards

-- Pantelis

Changes since v8:
* Removed extra member of boot_info in each node; passing boot_info
parameter to the check methods instead.
* Reworked yacc syntax that supports both old and new plugin syntax
* Added handling for new magic number (enabled by 'M' switch).
* Dropped dtbo/asmo formats.
* Added overlay testsuite.
* Addressed last version maintainer comments.

Changes since v7:
* Dropped xasprintf & backward compatibility patch
* Rebased against dgibson's overlay branch
* Minor doc wording fixes.

Changes since v6:
* Introduced xasprintf
* Added append_to_property and used it
* Changed some die()'s to assert
* Reordered node generation to respect sort
* Addressed remaining maintainer changes from v6

Changes since v5:
* Rebase to latest dtc version.
* Addressed all the maintainer requested changes from v5
* Added new magic value for dynamic objects and new format

Changes since v4:
* Rebase to latest dtc version.
* Completely redesigned the generation of resolution data.
Now instead of being generated as part of blob generation
they are created in the live tree.
* Consequently the patchset is much smaller.
* Added -A auto-label alias generation option.
* Addressed maintainer comments.
* Added syntactic sugar for overlays in the form of .dtsi
* Added /dts-v1/ /plugin/ preferred plugin form and deprecate
the previous form (although still works for backward compatibility)

Changes since v3:
* Rebase to latest dtc version.

Changes since v2:
* Split single patch to a patchset.
* Updated to dtc mainline.
* Changed __local_fixups__ format
* Clean up for better legibility.

Pantelis Antoniou (4):
  checks: Pass boot_info to the check methods
  dtc: Document the dynamic plugin internals
  dtc: Plugin and fixup support
  tests: Add overlay tests

 Documentation/dt-object-internal.txt | 318 +++++++++++++++++++++++++++++++++++
 Documentation/manual.txt             |  25 ++-
 checks.c                             | 121 +++++++------
 dtc-lexer.l                          |   5 +
 dtc-parser.y                         |  49 +++++-
 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 +-
 tests/overlay_overlay_dtc.dts        |  76 +--------
 tests/overlay_overlay_dtc.dtsi       |  83 +++++++++
 tests/overlay_overlay_new_dtc.dts    |  11 ++
 tests/overlay_overlay_simple.dts     |  12 ++
 tests/run_tests.sh                   |  20 +++
 treesource.c                         |   7 +-
 20 files changed, 884 insertions(+), 160 deletions(-)
 create mode 100644 Documentation/dt-object-internal.txt
 create mode 100644 tests/overlay_overlay_dtc.dtsi
 create mode 100644 tests/overlay_overlay_new_dtc.dts
 create mode 100644 tests/overlay_overlay_simple.dts

-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v9 1/4] checks: Pass boot_info to the check methods

[Pantelis Antoniou]

As preparation for overlay support we need to pass boot info
as an extra boot_info parameter to each check method.

No other functional changes are made.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
---
 checks.c | 113 +++++++++++++++++++++++++++++++++------------------------------
 1 file changed, 59 insertions(+), 54 deletions(-)

diff --git a/checks.c b/checks.c
index 0381c98..609975a 100644
--- a/checks.c
+++ b/checks.c
@@ -40,7 +40,8 @@ enum checkstatus {
 
 struct check;
 
-typedef void (*check_fn)(struct check *c, struct node *dt, struct node *node);
+typedef void (*check_fn)(struct check *c, struct boot_info *bi,
+			 struct node *dt, struct node *node);
 
 struct check {
 	const char *name;
@@ -97,19 +98,20 @@ static inline void check_msg(struct check *c, const char *fmt, ...)
 		check_msg((c), __VA_ARGS__); \
 	} while (0)
 
-static void check_nodes_props(struct check *c, struct node *dt, struct node *node)
+static void check_nodes_props(struct check *c, struct boot_info *bi,
+			      struct node *dt, struct node *node)
 {
 	struct node *child;
 
 	TRACE(c, "%s", node->fullpath);
 	if (c->fn)
-		c->fn(c, dt, node);
+		c->fn(c, bi, dt, node);
 
 	for_each_child(node, child)
-		check_nodes_props(c, dt, child);
+		check_nodes_props(c, bi, dt, child);
 }
 
-static bool run_check(struct check *c, struct node *dt)
+static bool run_check(struct check *c, struct boot_info *bi, struct node *dt)
 {
 	bool error = false;
 	int i;
@@ -123,7 +125,7 @@ static bool run_check(struct check *c, struct node *dt)
 
 	for (i = 0; i < c->num_prereqs; i++) {
 		struct check *prq = c->prereq[i];
-		error = error || run_check(prq, dt);
+		error = error || run_check(prq, bi, dt);
 		if (prq->status != PASSED) {
 			c->status = PREREQ;
 			check_msg(c, "Failed prerequisite '%s'",
@@ -134,7 +136,7 @@ static bool run_check(struct check *c, struct node *dt)
 	if (c->status != UNCHECKED)
 		goto out;
 
-	check_nodes_props(c, dt, dt);
+	check_nodes_props(c, bi, dt, dt);
 
 	if (c->status == UNCHECKED)
 		c->status = PASSED;
@@ -153,15 +155,15 @@ out:
  */
 
 /* A check which always fails, for testing purposes only */
-static inline void check_always_fail(struct check *c, struct node *dt,
-				     struct node *node)
+static inline void check_always_fail(struct check *c, struct boot_info *bi,
+				     struct node *dt, struct node *node)
 {
 	FAIL(c, "always_fail check");
 }
 CHECK(always_fail, check_always_fail, NULL);
 
-static void check_is_string(struct check *c, struct node *root,
-			    struct node *node)
+static void check_is_string(struct check *c, struct boot_info *bi,
+			    struct node *root, struct node *node)
 {
 	struct property *prop;
 	char *propname = c->data;
@@ -179,8 +181,8 @@ static void check_is_string(struct check *c, struct node *root,
 #define ERROR_IF_NOT_STRING(nm, propname) \
 	ERROR(nm, check_is_string, (propname))
 
-static void check_is_cell(struct check *c, struct node *root,
-			  struct node *node)
+static void check_is_cell(struct check *c, struct boot_info *bi,
+			  struct node *root, struct node *node)
 {
 	struct property *prop;
 	char *propname = c->data;
@@ -202,8 +204,8 @@ static void check_is_cell(struct check *c, struct node *root,
  * Structural check functions
  */
 
-static void check_duplicate_node_names(struct check *c, struct node *dt,
-				       struct node *node)
+static void check_duplicate_node_names(struct check *c, struct boot_info *bi,
+				       struct node *dt, struct node *node)
 {
 	struct node *child, *child2;
 
@@ -217,8 +219,8 @@ static void check_duplicate_node_names(struct check *c, struct node *dt,
 }
 ERROR(duplicate_node_names, check_duplicate_node_names, NULL);
 
-static void check_duplicate_property_names(struct check *c, struct node *dt,
-					   struct node *node)
+static void check_duplicate_property_names(struct check *c, struct boot_info *bi,
+					   struct node *dt, struct node *node)
 {
 	struct property *prop, *prop2;
 
@@ -239,8 +241,8 @@ ERROR(duplicate_property_names, check_duplicate_property_names, NULL);
 #define DIGITS		"0123456789"
 #define PROPNODECHARS	LOWERCASE UPPERCASE DIGITS ",._+*#?-"
 
-static void check_node_name_chars(struct check *c, struct node *dt,
-				  struct node *node)
+static void check_node_name_chars(struct check *c, struct boot_info *bi,
+				  struct node *dt, struct node *node)
 {
 	int n = strspn(node->name, c->data);
 
@@ -250,8 +252,8 @@ static void check_node_name_chars(struct check *c, struct node *dt,
 }
 ERROR(node_name_chars, check_node_name_chars, PROPNODECHARS "@");
 
-static void check_node_name_format(struct check *c, struct node *dt,
-				   struct node *node)
+static void check_node_name_format(struct check *c, struct boot_info *bi,
+				   struct node *dt, struct node *node)
 {
 	if (strchr(get_unitname(node), '@'))
 		FAIL(c, "Node %s has multiple '@' characters in name",
@@ -259,8 +261,8 @@ static void check_node_name_format(struct check *c, struct node *dt,
 }
 ERROR(node_name_format, check_node_name_format, NULL, &node_name_chars);
 
-static void check_unit_address_vs_reg(struct check *c, struct node *dt,
-			     struct node *node)
+static void check_unit_address_vs_reg(struct check *c, struct boot_info *bi,
+				      struct node *dt, struct node *node)
 {
 	const char *unitname = get_unitname(node);
 	struct property *prop = get_property(node, "reg");
@@ -283,8 +285,8 @@ static void check_unit_address_vs_reg(struct check *c, struct node *dt,
 }
 WARNING(unit_address_vs_reg, check_unit_address_vs_reg, NULL);
 
-static void check_property_name_chars(struct check *c, struct node *dt,
-				      struct node *node)
+static void check_property_name_chars(struct check *c, struct boot_info *bi,
+				      struct node *dt, struct node *node)
 {
 	struct property *prop;
 
@@ -305,9 +307,10 @@ ERROR(property_name_chars, check_property_name_chars, PROPNODECHARS);
 	((prop) ? (prop)->name : ""), \
 	((prop) ? "' in " : ""), (node)->fullpath
 
-static void check_duplicate_label(struct check *c, struct node *dt,
-				  const char *label, struct node *node,
-				  struct property *prop, struct marker *mark)
+static void check_duplicate_label(struct check *c, struct boot_info *bi,
+				  struct node *dt, const char *label,
+				  struct node *node, struct property *prop,
+				  struct marker *mark)
 {
 	struct node *othernode = NULL;
 	struct property *otherprop = NULL;
@@ -331,29 +334,30 @@ static void check_duplicate_label(struct check *c, struct node *dt,
 		     DESCLABEL_ARGS(othernode, otherprop, othermark));
 }
 
-static void check_duplicate_label_node(struct check *c, struct node *dt,
-				       struct node *node)
+static void check_duplicate_label_node(struct check *c, struct boot_info *bi,
+				       struct node *dt, struct node *node)
 {
 	struct label *l;
 	struct property *prop;
 
 	for_each_label(node->labels, l)
-		check_duplicate_label(c, dt, l->label, node, NULL, NULL);
+		check_duplicate_label(c, bi, dt, l->label, node, NULL, NULL);
 
 	for_each_property(node, prop) {
 		struct marker *m = prop->val.markers;
 
 		for_each_label(prop->labels, l)
-			check_duplicate_label(c, dt, l->label, node, prop, NULL);
+			check_duplicate_label(c, bi, dt, l->label, node, prop, NULL);
 
 		for_each_marker_of_type(m, LABEL)
-			check_duplicate_label(c, dt, m->ref, node, prop, m);
+			check_duplicate_label(c, bi, dt, m->ref, node, prop, m);
 	}
 }
 ERROR(duplicate_label, check_duplicate_label_node, NULL);
 
-static cell_t check_phandle_prop(struct check *c, struct node *root,
-				 struct node *node, const char *propname)
+static cell_t check_phandle_prop(struct check *c, struct boot_info *bi,
+				 struct node *root, struct node *node,
+				 const char *propname)
 {
 	struct property *prop;
 	struct marker *m;
@@ -398,8 +402,8 @@ static cell_t check_phandle_prop(struct check *c, struct node *root,
 	return phandle;
 }
 
-static void check_explicit_phandles(struct check *c, struct node *root,
-				    struct node *node)
+static void check_explicit_phandles(struct check *c, struct boot_info *bi,
+				    struct node *root, struct node *node)
 {
 	struct node *other;
 	cell_t phandle, linux_phandle;
@@ -407,9 +411,9 @@ static void check_explicit_phandles(struct check *c, struct node *root,
 	/* Nothing should have assigned phandles yet */
 	assert(!node->phandle);
 
-	phandle = check_phandle_prop(c, root, node, "phandle");
+	phandle = check_phandle_prop(c, bi, root, node, "phandle");
 
-	linux_phandle = check_phandle_prop(c, root, node, "linux,phandle");
+	linux_phandle = check_phandle_prop(c, bi, root, node, "linux,phandle");
 
 	if (!phandle && !linux_phandle)
 		/* No valid phandles; nothing further to check */
@@ -433,8 +437,8 @@ static void check_explicit_phandles(struct check *c, struct node *root,
 }
 ERROR(explicit_phandles, check_explicit_phandles, NULL);
 
-static void check_name_properties(struct check *c, struct node *root,
-				  struct node *node)
+static void check_name_properties(struct check *c, struct boot_info *bi,
+				  struct node *root, struct node *node)
 {
 	struct property **pp, *prop = NULL;
 
@@ -467,8 +471,8 @@ ERROR(name_properties, check_name_properties, NULL, &name_is_string);
  * Reference fixup functions
  */
 
-static void fixup_phandle_references(struct check *c, struct node *dt,
-				     struct node *node)
+static void fixup_phandle_references(struct check *c, struct boot_info *bi,
+				     struct node *dt, struct node *node)
 {
 	struct property *prop;
 
@@ -495,8 +499,8 @@ static void fixup_phandle_references(struct check *c, struct node *dt,
 ERROR(phandle_references, fixup_phandle_references, NULL,
       &duplicate_node_names, &explicit_phandles);
 
-static void fixup_path_references(struct check *c, struct node *dt,
-				  struct node *node)
+static void fixup_path_references(struct check *c, struct boot_info *bi,
+				  struct node *dt, struct node *node)
 {
 	struct property *prop;
 
@@ -534,8 +538,8 @@ WARNING_IF_NOT_STRING(device_type_is_string, "device_type");
 WARNING_IF_NOT_STRING(model_is_string, "model");
 WARNING_IF_NOT_STRING(status_is_string, "status");
 
-static void fixup_addr_size_cells(struct check *c, struct node *dt,
-				  struct node *node)
+static void fixup_addr_size_cells(struct check *c, struct boot_info *bi,
+				  struct node *dt, struct node *node)
 {
 	struct property *prop;
 
@@ -558,8 +562,8 @@ WARNING(addr_size_cells, fixup_addr_size_cells, NULL,
 #define node_size_cells(n) \
 	(((n)->size_cells == -1) ? 1 : (n)->size_cells)
 
-static void check_reg_format(struct check *c, struct node *dt,
-			     struct node *node)
+static void check_reg_format(struct check *c, struct boot_info *bi,
+			     struct node *dt, struct node *node)
 {
 	struct property *prop;
 	int addr_cells, size_cells, entrylen;
@@ -587,8 +591,8 @@ static void check_reg_format(struct check *c, struct node *dt,
 }
 WARNING(reg_format, check_reg_format, NULL, &addr_size_cells);
 
-static void check_ranges_format(struct check *c, struct node *dt,
-				struct node *node)
+static void check_ranges_format(struct check *c, struct boot_info *bi,
+				struct node *dt, struct node *node)
 {
 	struct property *prop;
 	int c_addr_cells, p_addr_cells, c_size_cells, p_size_cells, entrylen;
@@ -631,8 +635,8 @@ WARNING(ranges_format, check_ranges_format, NULL, &addr_size_cells);
 /*
  * Style checks
  */
-static void check_avoid_default_addr_size(struct check *c, struct node *dt,
-					  struct node *node)
+static void check_avoid_default_addr_size(struct check *c, struct boot_info *bi,
+					  struct node *dt, struct node *node)
 {
 	struct property *reg, *ranges;
 
@@ -657,6 +661,7 @@ WARNING(avoid_default_addr_size, check_avoid_default_addr_size, NULL,
 	&addr_size_cells);
 
 static void check_obsolete_chosen_interrupt_controller(struct check *c,
+						       struct boot_info *bi,
 						       struct node *dt,
 						       struct node *node)
 {
@@ -773,7 +778,7 @@ void process_checks(bool force, struct boot_info *bi)
 		struct check *c = check_table[i];
 
 		if (c->warn || c->error)
-			error = error || run_check(c, dt);
+			error = error || run_check(c, bi, dt);
 	}
 
 	if (error) {
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v9 2/4] dtc: Document the dynamic plugin internals

[Pantelis Antoniou]

Provides the document explaining the internal mechanics of
plugins and options.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
---
 Documentation/dt-object-internal.txt | 318 +++++++++++++++++++++++++++++++++++
 1 file changed, 318 insertions(+)
 create mode 100644 Documentation/dt-object-internal.txt

diff --git a/Documentation/dt-object-internal.txt b/Documentation/dt-object-internal.txt
new file mode 100644
index 0000000..d5b841e
--- /dev/null
+++ b/Documentation/dt-object-internal.txt
@@ -0,0 +1,318 @@
+Device Tree Dynamic Object format internals
+-------------------------------------------
+
+The Device Tree for most platforms is a static representation of
+the hardware capabilities. This is insufficient for many platforms
+that need to dynamically insert device tree fragments to the
+running kernel's live tree.
+
+This document explains the the device tree object format and the
+modifications made to the device tree compiler, which make it possible.
+
+1. Simplified Problem Definition
+--------------------------------
+
+Assume we have a platform which boots using following simplified device tree.
+
+---- foo.dts -----------------------------------------------------------------
+	/* FOO platform */
+	/ {
+		compatible = "corp,foo";
+
+		/* shared resources */
+		res: res {
+		};
+
+		/* On chip peripherals */
+		ocp: ocp {
+			/* peripherals that are always instantiated */
+			peripheral1 { ... };
+		};
+	};
+---- foo.dts -----------------------------------------------------------------
+
+We have a number of peripherals that after probing (using some undefined method)
+should result in different device tree configuration.
+
+We cannot boot with this static tree because due to the configuration of the
+foo platform there exist multiple conficting peripherals DT fragments.
+
+So for the bar peripheral we would have this:
+
+---- foo+bar.dts -------------------------------------------------------------
+	/* FOO platform + bar peripheral */
+	/ {
+		compatible = "corp,foo";
+
+		/* shared resources */
+		res: res {
+		};
+
+		/* On chip peripherals */
+		ocp: ocp {
+			/* peripherals that are always instantiated */
+			peripheral1 { ... };
+
+			/* bar peripheral */
+			bar {
+				compatible = "corp,bar";
+				... /* various properties and child nodes */
+			};
+		};
+	};
+---- foo+bar.dts -------------------------------------------------------------
+
+While for the baz peripheral we would have this:
+
+---- foo+baz.dts -------------------------------------------------------------
+	/* FOO platform + baz peripheral */
+	/ {
+		compatible = "corp,foo";
+
+		/* shared resources */
+		res: res {
+			/* baz resources */
+			baz_res: res_baz { ... };
+		};
+
+		/* On chip peripherals */
+		ocp: ocp {
+			/* peripherals that are always instantiated */
+			peripheral1 { ... };
+
+			/* baz peripheral */
+			baz {
+				compatible = "corp,baz";
+				/* reference to another point in the tree */
+				ref-to-res = <&baz_res>;
+				... /* various properties and child nodes */
+			};
+		};
+	};
+---- foo+baz.dts -------------------------------------------------------------
+
+We note that the baz case is more complicated, since the baz peripheral needs to
+reference another node in the DT tree.
+
+2. Device Tree Object Format Requirements
+-----------------------------------------
+
+Since the device tree is used for booting a number of very different hardware
+platforms it is imperative that we tread very carefully.
+
+2.a) No changes to the Device Tree binary format for the base tree. We cannot
+modify the tree format at all and all the information we require should be
+encoded using device tree itself. We can add nodes that can be safely ignored
+by both bootloaders and the kernel. The plugin dtb's are optionally tagged
+with a different magic number in the header but otherwise they too are simple
+blobs.
+
+2.b) Changes to the DTS source format should be absolutely minimal, and should
+only be needed for the DT fragment definitions, and not the base boot DT.
+
+2.c) An explicit option should be used to instruct DTC to generate the required
+information needed for object resolution. Platforms that don't use the
+dynamic object format can safely ignore it.
+
+2.d) Finally, DT syntax changes should be kept to a minimum. It should be
+possible to express everything using the existing DT syntax.
+
+3. Implementation
+-----------------
+
+The basic unit of addressing in Device Tree is the phandle. Turns out it's
+relatively simple to extend the way phandles are generated and referenced
+so that it's possible to dynamically convert symbolic references (labels)
+to phandle values. This is a valid assumption as long as the author uses
+reference syntax and does not assign phandle values manually (which might
+be a problem with decompiled source files).
+
+We can roughly divide the operation into two steps.
+
+3.a) Compilation of the base board DTS file using the '-@' option
+generates a valid DT blob with an added __symbols__ node at the root node,
+containing a list of all nodes that are marked with a label.
+
+Using the foo.dts file above the following node will be generated;
+
+$ dtc -@ -O dtb -o foo.dtb -b 0 foo.dts
+$ fdtdump foo.dtb
+...
+/ {
+	...
+	res {
+		...
+		phandle = <0x00000001>;
+		...
+	};
+	ocp {
+		...
+		phandle = <0x00000002>;
+		...
+	};
+	__symbols__ {
+		res="/res";
+		ocp="/ocp";
+	};
+};
+
+Notice that all the nodes that had a label have been recorded, and that
+phandles have been generated for them.
+
+This blob can be used to boot the board normally, the __symbols__ node will
+be safely ignored both by the bootloader and the kernel (the only loss will
+be a few bytes of memory and disk space).
+
+3.b) The Device Tree fragments must be compiled with the same option but they
+must also have a tag (/plugin/) that allows undefined references to nodes
+that are not present at compilation time to be recorded so that the runtime
+loader can fix them.
+
+So the bar peripheral's DTS format would be of the form:
+
+/dts-v1/ /plugin/;	/* allow undefined references and record them */
+/ {
+	....	/* various properties for loader use; i.e. part id etc. */
+	fragment@0 {
+		target = <&ocp>;
+		__overlay__ {
+			/* bar peripheral */
+			bar {
+				compatible = "corp,bar";
+				... /* various properties and child nodes */
+			}
+		};
+	};
+};
+
+Note that there's a target property that specifies the location where the
+contents of the overlay node will be placed, and it references the node
+in the foo.dts file.
+
+$ dtc -@ -O dtb -o bar.dtbo -b 0 bar.dts
+$ fdtdump bar.dtbo
+...
+/ {
+	... /* properties */
+	fragment@0 {
+		target = <0xffffffff>;
+		__overlay__ {
+			bar {
+				compatible = "corp,bar";
+				... /* various properties and child nodes */
+			}
+		};
+	};
+	__fixups__ {
+	    ocp = "/fragment@0:target:0";
+	};
+};
+
+No __symbols__ has been generated (no label in bar.dts).
+Note that the target's ocp label is undefined, so the phandle handle
+value is filled with the illegal value '0xffffffff', while a __fixups__
+node has been generated, which marks the location in the tree where
+the label lookup should store the runtime phandle value of the ocp node.
+
+The format of the __fixups__ node entry is
+
+	<label> = "<local-full-path>:<property-name>:<offset>";
+
+<label> 		Is the label we're referring
+<local-full-path>	Is the full path of the node the reference is
+<property-name>		Is the name of the property containing the
+			reference
+<offset>		The offset (in bytes) of where the property's
+			phandle value is located.
+
+Doing the same with the baz peripheral's DTS format is a little bit more
+involved, since baz contains references to local labels which require
+local fixups.
+
+/dts-v1/ /plugin/;	/* allow undefined label references and record them */
+/ {
+	....	/* various properties for loader use; i.e. part id etc. */
+	fragment@0 {
+		target = <&res>;
+		__overlay__ {
+			/* baz resources */
+			baz_res: res_baz { ... };
+		};
+	};
+	fragment@1 {
+		target = <&ocp>;
+		__overlay__ {
+			/* baz peripheral */
+			baz {
+				compatible = "corp,baz";
+				/* reference to another point in the tree */
+				ref-to-res = <&baz_res>;
+				... /* various properties and child nodes */
+			}
+		};
+	};
+};
+
+Note that &bar_res reference.
+
+$ dtc -@ -O dtb -o baz.dtbo -b 0 baz.dts
+$ fdtdump baz.dtbo
+...
+/ {
+	... /* properties */
+	fragment@0 {
+		target = <0xffffffff>;
+		__overlay__ {
+			res_baz {
+				....
+				phandle = <0x00000001>;
+			};
+		};
+	};
+	fragment@1 {
+		target = <0xffffffff>;
+		__overlay__ {
+			baz {
+				compatible = "corp,baz";
+				... /* various properties and child nodes */
+				ref-to-res = <0x00000001>;
+			}
+		};
+	};
+	__fixups__ {
+		res = "/fragment@0:target:0";
+		ocp = "/fragment@1:target:0";
+	};
+	__local_fixups__ {
+		fragment@1 {
+			__overlay__ {
+				baz {
+					ref-to-res = <0>;
+				};
+			};
+		};
+	};
+};
+
+This is similar to the bar case, but the reference of a local label by the
+baz node generates a __local_fixups__ entry that records the place that the
+local reference is being made. No matter how phandles are allocated from dtc
+the run time loader must apply an offset to each phandle in every dynamic
+DT object loaded. The __local_fixups__ node records the place of every
+local reference so that the loader can apply the offset.
+
+There is an alternative syntax to the expanded form for overlays with phandle
+targets which makes the format similar to the one using in .dtsi include files.
+
+So for the &ocp target example above one can simply write:
+
+/dts-v1/ /plugin/;
+&ocp {
+	/* bar peripheral */
+	bar {
+		compatible = "corp,bar";
+		... /* various properties and child nodes */
+	}
+};
+
+The resulting dtb object is identical.
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Pantelis Antoniou]

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 disabled. This is in order to give time for DT blob
methods to be updated.

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             |  49 +++++++++--
 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 +-
 treesource.c             |   7 +-
 14 files changed, 380 insertions(+), 31 deletions(-)

diff --git a/Documentation/manual.txt b/Documentation/manual.txt
index 398de32..65fbf09 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 new FDT magic number. By default blobs with the
+	standard FDT magic number are generated.
+
     -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 609975a..bc03d42 100644
--- a/checks.c
+++ b/checks.c
@@ -486,8 +486,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..4afc592 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,9 @@ extern void yyerror(char const *s);
 
 extern struct boot_info *the_boot_info;
 extern bool treesource_error;
+
+/* temporary while the tree is not built */
+static unsigned int the_versionflags;
 %}
 
 %union {
@@ -52,9 +56,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 +77,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 +109,36 @@ 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));
+			the_versionflags |= VF_DT_V1;
+			$$ = the_versionflags;
 		}
 	;
 
 v1tag:
 	  DT_V1 ';'
+	| DT_V1
 	| DT_V1 ';' v1tag
+
+plugindecl:
+	DT_PLUGIN ';'
+		{
+			the_versionflags |= VF_PLUGIN;
+			$$ = VF_PLUGIN;
+		}
+	| /* empty */
+		{
+			$$ = 0;
+		}
 	;
 
 memreserves:
@@ -161,10 +189,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 (the_versionflags & VF_PLUGIN)
+					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 +211,11 @@ devicetree:
 
 			$$ = $1;
 		}
+	| /* empty */
+		{
+			/* build empty node */
+			$$ = name_node(build_node(NULL, NULL), "");
+		}
 	;
 
 nodedef:
diff --git a/dtc.c b/dtc.c
index 9dcf640..a654267 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 new_magic;			/* use new 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'},
+	{"new-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\tUse new blog magic value",
 	"\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':
+			new_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);
+	}
+
 	if (sort)
 		sort_tree(bi);
 
@@ -318,12 +346,15 @@ int main(int argc, char *argv[])
 			    outname, strerror(errno));
 	}
 
+	if (new_magic && (bi->versionflags & VF_PLUGIN))
+		out_magic = FDT_MAGIC_DTBO;
+
 	if (streq(outform, "dts")) {
 		dt_to_source(outf, bi);
 	} else if (streq(outform, "dtb")) {
-		dt_to_blob(outf, bi, outversion);
+		dt_to_blob(outf, bi, out_magic, outversion);
 	} else if (streq(outform, "asm")) {
-		dt_to_asm(outf, bi, outversion);
+		dt_to_asm(outf, bi, out_magic, outversion);
 	} else if (streq(outform, "null")) {
 		/* do nothing */
 	} else {
diff --git a/dtc.h b/dtc.h
index 32009bc..889b8f8 100644
--- a/dtc.h
+++ b/dtc.h
@@ -55,6 +55,9 @@ extern int minsize;		/* Minimum blob size */
 extern int padsize;		/* Additional padding to blob */
 extern int alignsize;		/* Additional padding to blob accroding to the alignsize */
 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 */
+extern int new_magic;		/* use new FDT magic values for objects */
 
 #define PHANDLE_LEGACY	0x1
 #define PHANDLE_EPAPR	0x2
@@ -195,6 +198,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);
@@ -202,6 +206,8 @@ void delete_property(struct property *prop);
 void add_child(struct node *parent, struct node *child);
 void delete_node_by_name(struct node *parent, char *name);
 void delete_node(struct node *node);
+void append_to_property(struct node *node,
+			char *name, const void *data, int len);
 
 const char *get_unitname(struct node *node);
 struct property *get_property(struct node *node, const char *propname);
@@ -237,14 +243,22 @@ 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/ */
+#define VF_PLUGIN	0x0002	/* /plugin/ */
+
+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 */
 
@@ -253,8 +267,8 @@ void process_checks(bool force, struct boot_info *bi);
 
 /* Flattened trees */
 
-void dt_to_blob(FILE *f, struct boot_info *bi, int version);
-void dt_to_asm(FILE *f, struct boot_info *bi, int version);
+void dt_to_blob(FILE *f, struct boot_info *bi, fdt32_t magic, int version);
+void dt_to_asm(FILE *f, struct boot_info *bi, fdt32_t magic, int version);
 
 struct boot_info *dt_from_blob(const char *fname);
 
diff --git a/fdtdump.c b/fdtdump.c
index a9a2484..dd63ac2 100644
--- a/fdtdump.c
+++ b/fdtdump.c
@@ -201,7 +201,7 @@ int main(int argc, char *argv[])
 			p = memchr(p, smagic[0], endp - p - FDT_MAGIC_SIZE);
 			if (!p)
 				break;
-			if (fdt_magic(p) == FDT_MAGIC) {
+			if (fdt_magic(p) == FDT_MAGIC || fdt_magic(p) == FDT_MAGIC_DTBO) {
 				/* try and validate the main struct */
 				off_t this_len = endp - p;
 				fdt32_t max_version = 17;
diff --git a/flattree.c b/flattree.c
index a9d9520..57d76cf 100644
--- a/flattree.c
+++ b/flattree.c
@@ -335,6 +335,7 @@ static struct data flatten_reserve_list(struct reserve_info *reservelist,
 }
 
 static void make_fdt_header(struct fdt_header *fdt,
+			    fdt32_t magic,
 			    struct version_info *vi,
 			    int reservesize, int dtsize, int strsize,
 			    int boot_cpuid_phys)
@@ -345,7 +346,7 @@ static void make_fdt_header(struct fdt_header *fdt,
 
 	memset(fdt, 0xff, sizeof(*fdt));
 
-	fdt->magic = cpu_to_fdt32(FDT_MAGIC);
+	fdt->magic = cpu_to_fdt32(magic);
 	fdt->version = cpu_to_fdt32(vi->version);
 	fdt->last_comp_version = cpu_to_fdt32(vi->last_comp_version);
 
@@ -366,7 +367,7 @@ static void make_fdt_header(struct fdt_header *fdt,
 		fdt->size_dt_struct = cpu_to_fdt32(dtsize);
 }
 
-void dt_to_blob(FILE *f, struct boot_info *bi, int version)
+void dt_to_blob(FILE *f, struct boot_info *bi, fdt32_t magic, int version)
 {
 	struct version_info *vi = NULL;
 	int i;
@@ -390,7 +391,7 @@ void dt_to_blob(FILE *f, struct boot_info *bi, int version)
 	reservebuf = flatten_reserve_list(bi->reservelist, vi);
 
 	/* Make header */
-	make_fdt_header(&fdt, vi, reservebuf.len, dtbuf.len, strbuf.len,
+	make_fdt_header(&fdt, magic, vi, reservebuf.len, dtbuf.len, strbuf.len,
 			bi->boot_cpuid_phys);
 
 	/*
@@ -467,7 +468,7 @@ static void dump_stringtable_asm(FILE *f, struct data strbuf)
 	}
 }
 
-void dt_to_asm(FILE *f, struct boot_info *bi, int version)
+void dt_to_asm(FILE *f, struct boot_info *bi, fdt32_t magic, int version)
 {
 	struct version_info *vi = NULL;
 	int i;
@@ -830,6 +831,7 @@ struct boot_info *dt_from_blob(const char *fname)
 	struct node *tree;
 	uint32_t val;
 	int flags = 0;
+	unsigned int versionflags = VF_DT_V1;
 
 	f = srcfile_relative_open(fname, NULL);
 
@@ -845,9 +847,12 @@ struct boot_info *dt_from_blob(const char *fname)
 	}
 
 	magic = fdt32_to_cpu(magic);
-	if (magic != FDT_MAGIC)
+	if (magic != FDT_MAGIC && magic != FDT_MAGIC_DTBO)
 		die("Blob has incorrect magic number\n");
 
+	if (magic == FDT_MAGIC_DTBO)
+		versionflags |= VF_PLUGIN;
+
 	rc = fread(&totalsize, sizeof(totalsize), 1, f);
 	if (ferror(f))
 		die("Error reading DT blob size: %s\n", strerror(errno));
@@ -942,5 +947,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(versionflags, 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/libfdt/fdt.c b/libfdt/fdt.c
index 22286a1..28d422c 100644
--- a/libfdt/fdt.c
+++ b/libfdt/fdt.c
@@ -57,7 +57,7 @@
 
 int fdt_check_header(const void *fdt)
 {
-	if (fdt_magic(fdt) == FDT_MAGIC) {
+	if (fdt_magic(fdt) == FDT_MAGIC || fdt_magic(fdt) == FDT_MAGIC_DTBO) {
 		/* Complete tree */
 		if (fdt_version(fdt) < FDT_FIRST_SUPPORTED_VERSION)
 			return -FDT_ERR_BADVERSION;
diff --git a/libfdt/fdt.h b/libfdt/fdt.h
index 526aedb..493cd55 100644
--- a/libfdt/fdt.h
+++ b/libfdt/fdt.h
@@ -55,7 +55,7 @@
 #ifndef __ASSEMBLY__
 
 struct fdt_header {
-	fdt32_t magic;			 /* magic word FDT_MAGIC */
+	fdt32_t magic;			 /* magic word FDT_MAGIC[|_DTBO] */
 	fdt32_t totalsize;		 /* total size of DT block */
 	fdt32_t off_dt_struct;		 /* offset to structure */
 	fdt32_t off_dt_strings;		 /* offset to strings */
@@ -93,6 +93,7 @@ struct fdt_property {
 #endif /* !__ASSEMBLY */
 
 #define FDT_MAGIC	0xd00dfeed	/* 4: version, 4: total size */
+#define FDT_MAGIC_DTBO	0xd00dfdb0	/* DTBO magic */
 #define FDT_TAGSIZE	sizeof(fdt32_t)
 
 #define FDT_BEGIN_NODE	0x1		/* Start node: full name */
diff --git a/livetree.c b/livetree.c
index 3dc7559..1a2f4b1 100644
--- a/livetree.c
+++ b/livetree.c
@@ -216,6 +216,31 @@ 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;
+
+	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);
+
+	xasprintf(&name, "fragment@%u",
+			next_orphan_fragment++);
+	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);
@@ -296,6 +321,23 @@ void delete_node(struct node *node)
 	delete_labels(&node->labels);
 }
 
+void append_to_property(struct node *node,
+				    char *name, const void *data, int len)
+{
+	struct data d;
+	struct property *p;
+
+	p = get_property(node, name);
+	if (p) {
+		d = data_append_data(p->val, data, len);
+		p->val = d;
+	} else {
+		d = data_append_data(empty_data, data, len);
+		p = build_property(name, d);
+		add_property(node, p);
+	}
+}
+
 struct reserve_info *build_reserve_entry(uint64_t address, uint64_t size)
 {
 	struct reserve_info *new = xmalloc(sizeof(*new));
@@ -335,12 +377,14 @@ 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;
@@ -709,3 +753,182 @@ 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,
+					 struct node *an, bool allocph)
+{
+	struct node *c;
+	struct property *p;
+	struct label *l;
+
+	/* if if there are labels */
+	if (node->labels) {
+		/* 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,
+					an->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, an, allocph);
+}
+
+void generate_label_tree(struct node *dt, char *gen_node_name, bool allocph)
+{
+	struct node *an;
+
+	for_each_child(dt, an)
+		if (streq(gen_node_name, an->name))
+			break;
+
+	if (!an)
+		an = build_and_name_child_node(dt, gen_node_name);
+	if (!an)
+		die("Could not build label node /%s\n", gen_node_name);
+
+	generate_label_tree_internal(dt, dt, an, 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;	/* fixup node */
+	char *entry;
+
+	/* m->ref can only be a REF_PHANDLE, but check anyway */
+	assert(m->type == REF_PHANDLE);
+
+	/* fn is the node we're putting entries in */
+	fn = get_subnode(dt, fixups_name);
+	assert(fn != NULL);
+
+	/* there shouldn't be any ':' in the arguments */
+	if (strchr(node->fullpath, ':') || strchr(prop->name, ':'))
+		die("arguments should not contain ':'\n");
+
+	xasprintf(&entry, "%s:%s:%u",
+			node->fullpath, prop->name, m->offset);
+	append_to_property(fn, m->ref, entry, strlen(entry) + 1);
+}
+
+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 */
+	uint32_t value_32;
+	char *s, *e, *comp;
+	int len;
+
+	/* fn is the node we're putting entries in */
+	lfn = get_subnode(dt, local_fixups_name);
+	assert(lfn != NULL);
+
+	/* 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) {
+		/* 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);
+
+	value_32 = cpu_to_fdt32(m->offset);
+	append_to_property(wn, prop->name, &value_32, sizeof(value_32));
+}
+
+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)
+{
+	struct node *an;
+
+	for_each_child(dt, an)
+		if (streq(fixups_name, an->name))
+			break;
+
+	if (!an)
+		build_and_name_child_node(dt, fixups_name);
+
+	for_each_child(dt, an)
+		if (streq(local_fixups_name, an->name))
+			break;
+
+	if (!an)
+		build_and_name_child_node(dt, local_fixups_name);
+
+	generate_fixups_tree_internal(dt, dt);
+}
diff --git a/tests/mangle-layout.c b/tests/mangle-layout.c
index a76e51e..d29ebc6 100644
--- a/tests/mangle-layout.c
+++ b/tests/mangle-layout.c
@@ -42,7 +42,8 @@ static void expand_buf(struct bufstate *buf, int newsize)
 	buf->size = newsize;
 }
 
-static void new_header(struct bufstate *buf, int version, const void *fdt)
+static void new_header(struct bufstate *buf, fdt32_t magic, int version,
+		       const void *fdt)
 {
 	int hdrsize;
 
@@ -56,7 +57,7 @@ static void new_header(struct bufstate *buf, int version, const void *fdt)
 	expand_buf(buf, hdrsize);
 	memset(buf->buf, 0, hdrsize);
 
-	fdt_set_magic(buf->buf, FDT_MAGIC);
+	fdt_set_magic(buf->buf, magic);
 	fdt_set_version(buf->buf, version);
 	fdt_set_last_comp_version(buf->buf, 16);
 	fdt_set_boot_cpuid_phys(buf->buf, fdt_boot_cpuid_phys(fdt));
@@ -145,7 +146,7 @@ int main(int argc, char *argv[])
 	if (fdt_version(fdt) < 17)
 		CONFIG("Input tree must be v17");
 
-	new_header(&buf, version, fdt);
+	new_header(&buf, FDT_MAGIC, version, fdt);
 
 	while (*blockorder) {
 		add_block(&buf, version, *blockorder, fdt);
diff --git a/treesource.c b/treesource.c
index a55d1d1..75e920d 100644
--- a/treesource.c
+++ b/treesource.c
@@ -267,7 +267,12 @@ void dt_to_source(FILE *f, struct boot_info *bi)
 {
 	struct reserve_info *re;
 
-	fprintf(f, "/dts-v1/;\n\n");
+	fprintf(f, "/dts-v1/");
+
+	if (bi->versionflags & VF_PLUGIN)
+		fprintf(f, " /plugin/");
+
+	fprintf(f, ";\n\n");
 
 	for (re = bi->reservelist; re; re = re->next) {
 		struct label *l;
-- 
2.1.4

[PATCH v9 4/4] tests: Add overlay tests

[Pantelis Antoniou]

Add a number of tests for dynamic objects/overlays.

Re-use the original test by moving the contents to a .dtsi include

Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
---
 tests/overlay_overlay_dtc.dts     | 76 +----------------------------------
 tests/overlay_overlay_dtc.dtsi    | 83 +++++++++++++++++++++++++++++++++++++++
 tests/overlay_overlay_new_dtc.dts | 11 ++++++
 tests/overlay_overlay_simple.dts  | 12 ++++++
 tests/run_tests.sh                | 20 ++++++++++
 5 files changed, 127 insertions(+), 75 deletions(-)
 create mode 100644 tests/overlay_overlay_dtc.dtsi
 create mode 100644 tests/overlay_overlay_new_dtc.dts
 create mode 100644 tests/overlay_overlay_simple.dts

diff --git a/tests/overlay_overlay_dtc.dts b/tests/overlay_overlay_dtc.dts
index 30d2362..ca943ea 100644
--- a/tests/overlay_overlay_dtc.dts
+++ b/tests/overlay_overlay_dtc.dts
@@ -8,78 +8,4 @@
 /dts-v1/;
 /plugin/;
 
-/ {
-	/* Test that we can change an int by another */
-	fragment@0 {
-		target = <&test>;
-
-		__overlay__ {
-			test-int-property = <43>;
-		};
-	};
-
-	/* Test that we can replace a string by a longer one */
-	fragment@1 {
-		target = <&test>;
-
-		__overlay__ {
-			test-str-property = "foobar";
-		};
-	};
-
-	/* Test that we add a new property */
-	fragment@2 {
-		target = <&test>;
-
-		__overlay__ {
-			test-str-property-2 = "foobar2";
-		};
-	};
-
-	/* Test that we add a new node (by phandle) */
-	fragment@3 {
-		target = <&test>;
-
-		__overlay__ {
-			new-node {
-				new-property;
-			};
-		};
-	};
-
-	fragment@5 {
-		target = <&test>;
-
-		__overlay__ {
-			local: new-local-node {
-				new-property;
-			};
-		};
-	};
-
-	fragment@6 {
-		target = <&test>;
-
-		__overlay__ {
-			test-phandle = <&test>, <&local>;
-		};
-	};
-
-	fragment@7 {
-		target = <&test>;
-
-		__overlay__ {
-			test-several-phandle = <&local>, <&local>;
-		};
-	};
-
-	fragment@8 {
-		target = <&test>;
-
-		__overlay__ {
-			sub-test-node {
-				new-sub-test-property;
-			};
-		};
-	};
-};
+/include/ "overlay_overlay_dtc.dtsi"
diff --git a/tests/overlay_overlay_dtc.dtsi b/tests/overlay_overlay_dtc.dtsi
new file mode 100644
index 0000000..8ea8d5d
--- /dev/null
+++ b/tests/overlay_overlay_dtc.dtsi
@@ -0,0 +1,83 @@
+/*
+ * Copyright (c) 2016 NextThing Co
+ * Copyright (c) 2016 Free Electrons
+ * Copyright (c) 2016 Konsulko Inc.
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+/ {
+	/* Test that we can change an int by another */
+	fragment@0 {
+		target = <&test>;
+
+		__overlay__ {
+			test-int-property = <43>;
+		};
+	};
+
+	/* Test that we can replace a string by a longer one */
+	fragment@1 {
+		target = <&test>;
+
+		__overlay__ {
+			test-str-property = "foobar";
+		};
+	};
+
+	/* Test that we add a new property */
+	fragment@2 {
+		target = <&test>;
+
+		__overlay__ {
+			test-str-property-2 = "foobar2";
+		};
+	};
+
+	/* Test that we add a new node (by phandle) */
+	fragment@3 {
+		target = <&test>;
+
+		__overlay__ {
+			new-node {
+				new-property;
+			};
+		};
+	};
+
+	fragment@5 {
+		target = <&test>;
+
+		__overlay__ {
+			local: new-local-node {
+				new-property;
+			};
+		};
+	};
+
+	fragment@6 {
+		target = <&test>;
+
+		__overlay__ {
+			test-phandle = <&test>, <&local>;
+		};
+	};
+
+	fragment@7 {
+		target = <&test>;
+
+		__overlay__ {
+			test-several-phandle = <&local>, <&local>;
+		};
+	};
+
+	fragment@8 {
+		target = <&test>;
+
+		__overlay__ {
+			sub-test-node {
+				new-sub-test-property;
+			};
+		};
+	};
+};
diff --git a/tests/overlay_overlay_new_dtc.dts b/tests/overlay_overlay_new_dtc.dts
new file mode 100644
index 0000000..14d3f54
--- /dev/null
+++ b/tests/overlay_overlay_new_dtc.dts
@@ -0,0 +1,11 @@
+/*
+ * Copyright (c) 2016 NextThing Co
+ * Copyright (c) 2016 Free Electrons
+ * Copyright (c) 2016 Konsulko Inc.
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+/dts-v1/ /plugin/;
+
+/include/ "overlay_overlay_dtc.dtsi"
diff --git a/tests/overlay_overlay_simple.dts b/tests/overlay_overlay_simple.dts
new file mode 100644
index 0000000..8657e1e
--- /dev/null
+++ b/tests/overlay_overlay_simple.dts
@@ -0,0 +1,12 @@
+/*
+ * Copyright (c) 2016 Konsulko Inc.
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+/dts-v1/;
+/plugin/;
+
+&test {
+	test-int-property = <43>;
+};
diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index e4139dd..99c09bc 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -181,6 +181,26 @@ overlay_tests () {
         run_dtc_test -@ -I dts -O dtb -o overlay_base_with_symbols.test.dtb overlay_base.dts
         run_dtc_test -@ -I dts -O dtb -o overlay_overlay_with_symbols.test.dtb overlay_overlay_dtc.dts
         run_test overlay overlay_base_with_symbols.test.dtb overlay_overlay_with_symbols.test.dtb
+
+        # new /plugin/ format
+        run_dtc_test -@ -I dts -O dtb -o overlay_overlay_new_with_symbols.test.dtb overlay_overlay_new_dtc.dts
+
+        # test new magic option
+        run_dtc_test -M@ -I dts -O dtb -o overlay_overlay_with_symbols_new_magic.test.dtb overlay_overlay_dtc.dts
+
+        # test plugin source to dtb and back
+        run_dtc_test -@ -I dtb -O dts -o overlay_overlay_dtc.test.dts overlay_overlay_with_symbols.test.dtb
+        run_dtc_test -@ -I dts -O dtb -o overlay_overlay_with_symbols.test.test.dtb overlay_overlay_dtc.test.dts
+        run_test dtbs_equal_ordered overlay_overlay_with_symbols.test.dtb overlay_overlay_with_symbols.test.test.dtb
+
+	# test simplified plugin syntax
+        run_dtc_test -@ -I dts -O dtb -o overlay_overlay_simple.dtb overlay_overlay_simple.dts
+
+	# test plugin source to dtb and back (with new magic)
+        run_dtc_test -@ -I dtb -O dts -o overlay_overlay_dtc_new_magic.test.dts overlay_overlay_with_symbols_new_magic.test.dtb
+        run_dtc_test -@ -I dts -O dtb -o overlay_overlay_with_symbols_new_magic.test.test.dtb overlay_overlay_dtc_new_magic.test.dts
+        run_test dtbs_equal_ordered overlay_overlay_with_symbols_new_magic.test.dtb overlay_overlay_with_symbols_new_magic.test.test.dtb
+
     fi
 
     # Bad fixup tests
-- 
2.1.4

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

[Phil Elwell]

On 24/11/2016 12:31, 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 disabled. This is in order to give time for DT blob
> methods to be updated.
>
> 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>
It's great to see this work about to come to fruition, but I have a
reservation about this patch. Like previous versions, this
implementation generates __fixups__, __local_fixups__  and __symbols__
whenever the "-@" command line parameter is given; without it you get
none. In my opinion, this logic causes the DTBs to be unnecessarily
large with no obvious benefit.

You can divide the compiled outputs from DTC into two categories - fully
resolved DTBs (what I would call base DTBs), and overlays. Base DTBs
should include __symbols__ to allow overlays to be applied, and it is
right that this should be controlled by the "-@" flag, but since base
DTBs are fully resolved I think there is no reason to include either
__fixups__ or __local_fixups__. Therefore I think both kinds of fixups
should only be omitted when the "/plugin/ " directive is used. This was
the purpose of one of the patches I provided you with.

Phil Elwell, Raspberry Pi

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

[Pantelis Antoniou]

Hi Phil,

> On Nov 24, 2016, at 15:49 , Phil Elwell <phil-FnsA7b+Nu9XbIbC87yuRow@public.gmane.org> wrote:
> 
> On 24/11/2016 12:31, 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 disabled. This is in order to give time for DT blob
>> methods to be updated.
>> 
>> 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>
> It's great to see this work about to come to fruition, but I have a
> reservation about this patch. Like previous versions, this
> implementation generates __fixups__, __local_fixups__  and __symbols__
> whenever the "-@" command line parameter is given; without it you get
> none. In my opinion, this logic causes the DTBs to be unnecessarily
> large with no obvious benefit.
> 
> You can divide the compiled outputs from DTC into two categories - fully
> resolved DTBs (what I would call base DTBs), and overlays. Base DTBs
> should include __symbols__ to allow overlays to be applied, and it is
> right that this should be controlled by the "-@" flag, but since base
> DTBs are fully resolved I think there is no reason to include either
> __fixups__ or __local_fixups__. Therefore I think both kinds of fixups
> should only be omitted when the "/plugin/ " directive is used. This was
> the purpose of one of the patches I provided you with.
> 

Yes, I’m quite aware of that. There is a reason for generating every
resolve node for the base tree. It contains information about the
dependencies of every hardware device due to phandle references.
We can utilize that to re-order the device probe order to eliminate
-EPROBE_DEFER cycles upon boot.

It is important we get the core support in and then you can add extra
switches for every special platform case.

What kind of problems do you have with larger device tree blobs?
I do carry a hashed phandle patch on my mainline tracking tree which
should help with larger DTBs. 

> Phil Elwell, Raspberry Pi

Regards

— Pantelis

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Phil Elwell]

On 24/11/2016 13:58, Pantelis Antoniou wrote:
> Hi Phil,
>
>> On Nov 24, 2016, at 15:49 , Phil Elwell <phil-FnsA7b+Nu9XbIbC87yuRow@public.gmane.org> wrote:
>>
>> On 24/11/2016 12:31, 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 disabled. This is in order to give time for DT blob
>>> methods to be updated.
>>>
>>> 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>
>> It's great to see this work about to come to fruition, but I have a
>> reservation about this patch. Like previous versions, this
>> implementation generates __fixups__, __local_fixups__  and __symbols__
>> whenever the "-@" command line parameter is given; without it you get
>> none. In my opinion, this logic causes the DTBs to be unnecessarily
>> large with no obvious benefit.
>>
>> You can divide the compiled outputs from DTC into two categories - fully
>> resolved DTBs (what I would call base DTBs), and overlays. Base DTBs
>> should include __symbols__ to allow overlays to be applied, and it is
>> right that this should be controlled by the "-@" flag, but since base
>> DTBs are fully resolved I think there is no reason to include either
>> __fixups__ or __local_fixups__. Therefore I think both kinds of fixups
>> should only be omitted when the "/plugin/ " directive is used. This was
>> the purpose of one of the patches I provided you with.
>>
> Yes, I’m quite aware of that. There is a reason for generating every
> resolve node for the base tree. It contains information about the
> dependencies of every hardware device due to phandle references.
> We can utilize that to re-order the device probe order to eliminate
> -EPROBE_DEFER cycles upon boot.
>
> It is important we get the core support in and then you can add extra
> switches for every special platform case.
>
> What kind of problems do you have with larger device tree blobs?
> I do carry a hashed phandle patch on my mainline tracking tree which
> should help with larger DTBs. 
Early Raspberry Pi DT support used to load the DTBs to 0x100, Although
the ARMv6/7 kernel starts at 0x8000, something (the decompressor?) would
use the space between 0x4000 and the 0x8000, which gave us a practical
limit of 16KB on the DTB size. This used to be sufficient for a base DTB
and a few small overlays, but with the patch all Pi DTBs are over 16KB.
The practical limit was overcome a long time ago, but it made me aware
of the DTB contents, and there are may be some situations where it is
desirable to reduce the footprint as much as possible.

I would have thought that all DTBs already contain enough dependency
information in the form of the phandles themselves. One of the first
things the kernel does is to unflatten the DTB, and that is the obvious
point to resolve the phandles and generate the necessary dependencies.
Can you explain how both __fixups__ and __local_fixups__ aid this
process? Ideally they wouldn't duplicate any information already in the
tree, since then you have to cope with the possibility of malformed DTBs
where the two don't actually match.

Phil

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

[Pantelis Antoniou]

Hi Phil,

> On Nov 24, 2016, at 16:14 , Phil Elwell <phil-FnsA7b+Nu9XbIbC87yuRow@public.gmane.org> wrote:
> 
> On 24/11/2016 13:58, Pantelis Antoniou wrote:
>> Hi Phil,
>> 
>>> On Nov 24, 2016, at 15:49 , Phil Elwell <phil-FnsA7b+Nu9XbIbC87yuRow@public.gmane.org> wrote:
>>> 
>>> On 24/11/2016 12:31, 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 disabled. This is in order to give time for DT blob
>>>> methods to be updated.
>>>> 
>>>> 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>
>>> It's great to see this work about to come to fruition, but I have a
>>> reservation about this patch. Like previous versions, this
>>> implementation generates __fixups__, __local_fixups__  and __symbols__
>>> whenever the "-@" command line parameter is given; without it you get
>>> none. In my opinion, this logic causes the DTBs to be unnecessarily
>>> large with no obvious benefit.
>>> 
>>> You can divide the compiled outputs from DTC into two categories - fully
>>> resolved DTBs (what I would call base DTBs), and overlays. Base DTBs
>>> should include __symbols__ to allow overlays to be applied, and it is
>>> right that this should be controlled by the "-@" flag, but since base
>>> DTBs are fully resolved I think there is no reason to include either
>>> __fixups__ or __local_fixups__. Therefore I think both kinds of fixups
>>> should only be omitted when the "/plugin/ " directive is used. This was
>>> the purpose of one of the patches I provided you with.
>>> 
>> Yes, I’m quite aware of that. There is a reason for generating every
>> resolve node for the base tree. It contains information about the
>> dependencies of every hardware device due to phandle references.
>> We can utilize that to re-order the device probe order to eliminate
>> -EPROBE_DEFER cycles upon boot.
>> 
>> It is important we get the core support in and then you can add extra
>> switches for every special platform case.
>> 
>> What kind of problems do you have with larger device tree blobs?
>> I do carry a hashed phandle patch on my mainline tracking tree which
>> should help with larger DTBs. 
> Early Raspberry Pi DT support used to load the DTBs to 0x100, Although
> the ARMv6/7 kernel starts at 0x8000, something (the decompressor?) would
> use the space between 0x4000 and the 0x8000, which gave us a practical
> limit of 16KB on the DTB size. This used to be sufficient for a base DTB
> and a few small overlays, but with the patch all Pi DTBs are over 16KB.
> The practical limit was overcome a long time ago, but it made me aware
> of the DTB contents, and there are may be some situations where it is
> desirable to reduce the footprint as much as possible.
> 

I see now why you care about this. If you can regenerate your patch against
what I’ve posted I’d be happy to carry it along.

But DTBs are getting pretty large even without the extra fixup nodes. It is
not uncommon to see them at a few hundred KBs now.

> I would have thought that all DTBs already contain enough dependency
> information in the form of the phandles themselves. One of the first
> things the kernel does is to unflatten the DTB, and that is the obvious
> point to resolve the phandles and generate the necessary dependencies.
> Can you explain how both __fixups__ and __local_fixups__ aid this
> process? Ideally they wouldn't duplicate any information already in the
> tree, since then you have to cope with the possibility of malformed DTBs
> where the two don't actually match.

No, the DTBs by themselves do not contain enough information to build the
probe dependency graph, because phandles are simply converted to 32 bit
cell values on compile.

For instance take a case of one node using the other:

foo_label: foo { };
bar { use = <&foo_label>; };

A standard compile would generate a bar node as bar { use = <1>; };

While using the -@ switch you’d get a local ref that says that there is
a phandle cell value at offset 0 of bar/use property. Looking up the
phandle value you can see that this property references the foo node.

So when building the probe order graph the foo node should be probed
before bar.

> 
> Phil

Regards

— Pantelis

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Phil Elwell]

On 24/11/2016 14:23, Pantelis Antoniou wrote:
> Hi Phil,
>> I would have thought that all DTBs already contain enough dependency
>> information in the form of the phandles themselves. One of the first
>> things the kernel does is to unflatten the DTB, and that is the obvious
>> point to resolve the phandles and generate the necessary dependencies.
>> Can you explain how both __fixups__ and __local_fixups__ aid this
>> process? Ideally they wouldn't duplicate any information already in the
>> tree, since then you have to cope with the possibility of malformed DTBs
>> where the two don't actually match.
> No, the DTBs by themselves do not contain enough information to build the
> probe dependency graph, because phandles are simply converted to 32 bit
> cell values on compile.
>
> For instance take a case of one node using the other:
>
> foo_label: foo { };
> bar { use = <&foo_label>; };
>
> A standard compile would generate a bar node as bar { use = <1>; };
>
> While using the -@ switch you’d get a local ref that says that there is
> a phandle cell value at offset 0 of bar/use property. Looking up the
> phandle value you can see that this property references the foo node.
>
> So when building the probe order graph the foo node should be probed
> before bar.
Yes, you are quite right. During compilation the phandleness of a
phandle reference gets lost - it's just an integer. All you really need
is one bit per word, and you could dream up clever coding schemes for
that sparse map, but extra information is needed.

I still think that __fixups__ is unnecessary for base DTBs, but since it
will be empty anyway it just looks a bit awkward rather than taking up
much space.

Seemingly random load order of device nodes has been a cause of
confusion and annoyance, so I'm happy that there is a plan to eliminate
them. If the DTBs have to grow to make that possible then so be it.

Phil

Re: [PATCH v9 1/4] checks: Pass boot_info to the check methods

[David Gibson]

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

On Thu, Nov 24, 2016 at 02:31:30PM +0200, Pantelis Antoniou wrote:
> As preparation for overlay support we need to pass boot info
> as an extra boot_info parameter to each check method.
> 
> No other functional changes are made.
> 
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>

You don't really need to pass both bi and dt, since bi->dt will get
you the same thing.

> ---
>  checks.c | 113 +++++++++++++++++++++++++++++++++------------------------------
>  1 file changed, 59 insertions(+), 54 deletions(-)
> 
> diff --git a/checks.c b/checks.c
> index 0381c98..609975a 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -40,7 +40,8 @@ enum checkstatus {
>  
>  struct check;
>  
> -typedef void (*check_fn)(struct check *c, struct node *dt, struct node *node);
> +typedef void (*check_fn)(struct check *c, struct boot_info *bi,
> +			 struct node *dt, struct node *node);
>  
>  struct check {
>  	const char *name;
> @@ -97,19 +98,20 @@ static inline void check_msg(struct check *c, const char *fmt, ...)
>  		check_msg((c), __VA_ARGS__); \
>  	} while (0)
>  
> -static void check_nodes_props(struct check *c, struct node *dt, struct node *node)
> +static void check_nodes_props(struct check *c, struct boot_info *bi,
> +			      struct node *dt, struct node *node)
>  {
>  	struct node *child;
>  
>  	TRACE(c, "%s", node->fullpath);
>  	if (c->fn)
> -		c->fn(c, dt, node);
> +		c->fn(c, bi, dt, node);
>  
>  	for_each_child(node, child)
> -		check_nodes_props(c, dt, child);
> +		check_nodes_props(c, bi, dt, child);
>  }
>  
> -static bool run_check(struct check *c, struct node *dt)
> +static bool run_check(struct check *c, struct boot_info *bi, struct node *dt)
>  {
>  	bool error = false;
>  	int i;
> @@ -123,7 +125,7 @@ static bool run_check(struct check *c, struct node *dt)
>  
>  	for (i = 0; i < c->num_prereqs; i++) {
>  		struct check *prq = c->prereq[i];
> -		error = error || run_check(prq, dt);
> +		error = error || run_check(prq, bi, dt);
>  		if (prq->status != PASSED) {
>  			c->status = PREREQ;
>  			check_msg(c, "Failed prerequisite '%s'",
> @@ -134,7 +136,7 @@ static bool run_check(struct check *c, struct node *dt)
>  	if (c->status != UNCHECKED)
>  		goto out;
>  
> -	check_nodes_props(c, dt, dt);
> +	check_nodes_props(c, bi, dt, dt);
>  
>  	if (c->status == UNCHECKED)
>  		c->status = PASSED;
> @@ -153,15 +155,15 @@ out:
>   */
>  
>  /* A check which always fails, for testing purposes only */
> -static inline void check_always_fail(struct check *c, struct node *dt,
> -				     struct node *node)
> +static inline void check_always_fail(struct check *c, struct boot_info *bi,
> +				     struct node *dt, struct node *node)
>  {
>  	FAIL(c, "always_fail check");
>  }
>  CHECK(always_fail, check_always_fail, NULL);
>  
> -static void check_is_string(struct check *c, struct node *root,
> -			    struct node *node)
> +static void check_is_string(struct check *c, struct boot_info *bi,
> +			    struct node *root, struct node *node)
>  {
>  	struct property *prop;
>  	char *propname = c->data;
> @@ -179,8 +181,8 @@ static void check_is_string(struct check *c, struct node *root,
>  #define ERROR_IF_NOT_STRING(nm, propname) \
>  	ERROR(nm, check_is_string, (propname))
>  
> -static void check_is_cell(struct check *c, struct node *root,
> -			  struct node *node)
> +static void check_is_cell(struct check *c, struct boot_info *bi,
> +			  struct node *root, struct node *node)
>  {
>  	struct property *prop;
>  	char *propname = c->data;
> @@ -202,8 +204,8 @@ static void check_is_cell(struct check *c, struct node *root,
>   * Structural check functions
>   */
>  
> -static void check_duplicate_node_names(struct check *c, struct node *dt,
> -				       struct node *node)
> +static void check_duplicate_node_names(struct check *c, struct boot_info *bi,
> +				       struct node *dt, struct node *node)
>  {
>  	struct node *child, *child2;
>  
> @@ -217,8 +219,8 @@ static void check_duplicate_node_names(struct check *c, struct node *dt,
>  }
>  ERROR(duplicate_node_names, check_duplicate_node_names, NULL);
>  
> -static void check_duplicate_property_names(struct check *c, struct node *dt,
> -					   struct node *node)
> +static void check_duplicate_property_names(struct check *c, struct boot_info *bi,
> +					   struct node *dt, struct node *node)
>  {
>  	struct property *prop, *prop2;
>  
> @@ -239,8 +241,8 @@ ERROR(duplicate_property_names, check_duplicate_property_names, NULL);
>  #define DIGITS		"0123456789"
>  #define PROPNODECHARS	LOWERCASE UPPERCASE DIGITS ",._+*#?-"
>  
> -static void check_node_name_chars(struct check *c, struct node *dt,
> -				  struct node *node)
> +static void check_node_name_chars(struct check *c, struct boot_info *bi,
> +				  struct node *dt, struct node *node)
>  {
>  	int n = strspn(node->name, c->data);
>  
> @@ -250,8 +252,8 @@ static void check_node_name_chars(struct check *c, struct node *dt,
>  }
>  ERROR(node_name_chars, check_node_name_chars, PROPNODECHARS "@");
>  
> -static void check_node_name_format(struct check *c, struct node *dt,
> -				   struct node *node)
> +static void check_node_name_format(struct check *c, struct boot_info *bi,
> +				   struct node *dt, struct node *node)
>  {
>  	if (strchr(get_unitname(node), '@'))
>  		FAIL(c, "Node %s has multiple '@' characters in name",
> @@ -259,8 +261,8 @@ static void check_node_name_format(struct check *c, struct node *dt,
>  }
>  ERROR(node_name_format, check_node_name_format, NULL, &node_name_chars);
>  
> -static void check_unit_address_vs_reg(struct check *c, struct node *dt,
> -			     struct node *node)
> +static void check_unit_address_vs_reg(struct check *c, struct boot_info *bi,
> +				      struct node *dt, struct node *node)
>  {
>  	const char *unitname = get_unitname(node);
>  	struct property *prop = get_property(node, "reg");
> @@ -283,8 +285,8 @@ static void check_unit_address_vs_reg(struct check *c, struct node *dt,
>  }
>  WARNING(unit_address_vs_reg, check_unit_address_vs_reg, NULL);
>  
> -static void check_property_name_chars(struct check *c, struct node *dt,
> -				      struct node *node)
> +static void check_property_name_chars(struct check *c, struct boot_info *bi,
> +				      struct node *dt, struct node *node)
>  {
>  	struct property *prop;
>  
> @@ -305,9 +307,10 @@ ERROR(property_name_chars, check_property_name_chars, PROPNODECHARS);
>  	((prop) ? (prop)->name : ""), \
>  	((prop) ? "' in " : ""), (node)->fullpath
>  
> -static void check_duplicate_label(struct check *c, struct node *dt,
> -				  const char *label, struct node *node,
> -				  struct property *prop, struct marker *mark)
> +static void check_duplicate_label(struct check *c, struct boot_info *bi,
> +				  struct node *dt, const char *label,
> +				  struct node *node, struct property *prop,
> +				  struct marker *mark)
>  {
>  	struct node *othernode = NULL;
>  	struct property *otherprop = NULL;
> @@ -331,29 +334,30 @@ static void check_duplicate_label(struct check *c, struct node *dt,
>  		     DESCLABEL_ARGS(othernode, otherprop, othermark));
>  }
>  
> -static void check_duplicate_label_node(struct check *c, struct node *dt,
> -				       struct node *node)
> +static void check_duplicate_label_node(struct check *c, struct boot_info *bi,
> +				       struct node *dt, struct node *node)
>  {
>  	struct label *l;
>  	struct property *prop;
>  
>  	for_each_label(node->labels, l)
> -		check_duplicate_label(c, dt, l->label, node, NULL, NULL);
> +		check_duplicate_label(c, bi, dt, l->label, node, NULL, NULL);
>  
>  	for_each_property(node, prop) {
>  		struct marker *m = prop->val.markers;
>  
>  		for_each_label(prop->labels, l)
> -			check_duplicate_label(c, dt, l->label, node, prop, NULL);
> +			check_duplicate_label(c, bi, dt, l->label, node, prop, NULL);
>  
>  		for_each_marker_of_type(m, LABEL)
> -			check_duplicate_label(c, dt, m->ref, node, prop, m);
> +			check_duplicate_label(c, bi, dt, m->ref, node, prop, m);
>  	}
>  }
>  ERROR(duplicate_label, check_duplicate_label_node, NULL);
>  
> -static cell_t check_phandle_prop(struct check *c, struct node *root,
> -				 struct node *node, const char *propname)
> +static cell_t check_phandle_prop(struct check *c, struct boot_info *bi,
> +				 struct node *root, struct node *node,
> +				 const char *propname)
>  {
>  	struct property *prop;
>  	struct marker *m;
> @@ -398,8 +402,8 @@ static cell_t check_phandle_prop(struct check *c, struct node *root,
>  	return phandle;
>  }
>  
> -static void check_explicit_phandles(struct check *c, struct node *root,
> -				    struct node *node)
> +static void check_explicit_phandles(struct check *c, struct boot_info *bi,
> +				    struct node *root, struct node *node)
>  {
>  	struct node *other;
>  	cell_t phandle, linux_phandle;
> @@ -407,9 +411,9 @@ static void check_explicit_phandles(struct check *c, struct node *root,
>  	/* Nothing should have assigned phandles yet */
>  	assert(!node->phandle);
>  
> -	phandle = check_phandle_prop(c, root, node, "phandle");
> +	phandle = check_phandle_prop(c, bi, root, node, "phandle");
>  
> -	linux_phandle = check_phandle_prop(c, root, node, "linux,phandle");
> +	linux_phandle = check_phandle_prop(c, bi, root, node, "linux,phandle");
>  
>  	if (!phandle && !linux_phandle)
>  		/* No valid phandles; nothing further to check */
> @@ -433,8 +437,8 @@ static void check_explicit_phandles(struct check *c, struct node *root,
>  }
>  ERROR(explicit_phandles, check_explicit_phandles, NULL);
>  
> -static void check_name_properties(struct check *c, struct node *root,
> -				  struct node *node)
> +static void check_name_properties(struct check *c, struct boot_info *bi,
> +				  struct node *root, struct node *node)
>  {
>  	struct property **pp, *prop = NULL;
>  
> @@ -467,8 +471,8 @@ ERROR(name_properties, check_name_properties, NULL, &name_is_string);
>   * Reference fixup functions
>   */
>  
> -static void fixup_phandle_references(struct check *c, struct node *dt,
> -				     struct node *node)
> +static void fixup_phandle_references(struct check *c, struct boot_info *bi,
> +				     struct node *dt, struct node *node)
>  {
>  	struct property *prop;
>  
> @@ -495,8 +499,8 @@ static void fixup_phandle_references(struct check *c, struct node *dt,
>  ERROR(phandle_references, fixup_phandle_references, NULL,
>        &duplicate_node_names, &explicit_phandles);
>  
> -static void fixup_path_references(struct check *c, struct node *dt,
> -				  struct node *node)
> +static void fixup_path_references(struct check *c, struct boot_info *bi,
> +				  struct node *dt, struct node *node)
>  {
>  	struct property *prop;
>  
> @@ -534,8 +538,8 @@ WARNING_IF_NOT_STRING(device_type_is_string, "device_type");
>  WARNING_IF_NOT_STRING(model_is_string, "model");
>  WARNING_IF_NOT_STRING(status_is_string, "status");
>  
> -static void fixup_addr_size_cells(struct check *c, struct node *dt,
> -				  struct node *node)
> +static void fixup_addr_size_cells(struct check *c, struct boot_info *bi,
> +				  struct node *dt, struct node *node)
>  {
>  	struct property *prop;
>  
> @@ -558,8 +562,8 @@ WARNING(addr_size_cells, fixup_addr_size_cells, NULL,
>  #define node_size_cells(n) \
>  	(((n)->size_cells == -1) ? 1 : (n)->size_cells)
>  
> -static void check_reg_format(struct check *c, struct node *dt,
> -			     struct node *node)
> +static void check_reg_format(struct check *c, struct boot_info *bi,
> +			     struct node *dt, struct node *node)
>  {
>  	struct property *prop;
>  	int addr_cells, size_cells, entrylen;
> @@ -587,8 +591,8 @@ static void check_reg_format(struct check *c, struct node *dt,
>  }
>  WARNING(reg_format, check_reg_format, NULL, &addr_size_cells);
>  
> -static void check_ranges_format(struct check *c, struct node *dt,
> -				struct node *node)
> +static void check_ranges_format(struct check *c, struct boot_info *bi,
> +				struct node *dt, struct node *node)
>  {
>  	struct property *prop;
>  	int c_addr_cells, p_addr_cells, c_size_cells, p_size_cells, entrylen;
> @@ -631,8 +635,8 @@ WARNING(ranges_format, check_ranges_format, NULL, &addr_size_cells);
>  /*
>   * Style checks
>   */
> -static void check_avoid_default_addr_size(struct check *c, struct node *dt,
> -					  struct node *node)
> +static void check_avoid_default_addr_size(struct check *c, struct boot_info *bi,
> +					  struct node *dt, struct node *node)
>  {
>  	struct property *reg, *ranges;
>  
> @@ -657,6 +661,7 @@ WARNING(avoid_default_addr_size, check_avoid_default_addr_size, NULL,
>  	&addr_size_cells);
>  
>  static void check_obsolete_chosen_interrupt_controller(struct check *c,
> +						       struct boot_info *bi,
>  						       struct node *dt,
>  						       struct node *node)
>  {
> @@ -773,7 +778,7 @@ void process_checks(bool force, struct boot_info *bi)
>  		struct check *c = check_table[i];
>  
>  		if (c->warn || c->error)
> -			error = error || run_check(c, dt);
> +			error = error || run_check(c, bi, dt);
>  	}
>  
>  	if (error) {

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

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

[David Gibson]

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

On Thu, Nov 24, 2016 at 02:31:32PM +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 disabled. This is in order to give time for DT blob
> methods to be updated.
> 
> 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             |  49 +++++++++--
>  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 +-
>  treesource.c             |   7 +-
>  14 files changed, 380 insertions(+), 31 deletions(-)
> 
> diff --git a/Documentation/manual.txt b/Documentation/manual.txt
> index 398de32..65fbf09 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 new FDT magic number. By default blobs with the
> +	standard FDT magic number are generated.

First, this description is incomplete since -M *only* affects the
magic number for /plugin/ input, not in other cases.  Second, the
default is the wrong way around.  If we make old-style the default,
then new-style will never be used, which defeats the purpose.

> +
>      -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 609975a..bc03d42 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -486,8 +486,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..4afc592 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,9 @@ extern void yyerror(char const *s);
>  
>  extern struct boot_info *the_boot_info;
>  extern bool treesource_error;
> +
> +/* temporary while the tree is not built */
> +static unsigned int the_versionflags;

Hrm.  Using a global during parsing is pretty dangerous - it makes
assumptions about the order in which bison will execute semantic
actions that may not always be correct.

It'll probably work in practice, so I *might* be convinced it's
adequate for a first cut.  I'm a bit reluctant though, since I suspect
once merged it will become entrenched.

The correct way to handle this, IMO, is not to ever attempt to apply
overlays during the parse.  Basically, we'd change the overall
structure so that the output from the parser is not a single tree, but
a list of overlays / fragments.  Then, once the parse is complete, so
versioninfo (which could now become a member of bootinfo) is well
defined, we either apply the fragments in place (base tree) or encode
them into the overlay structure (plugin mode).

See https://github.com/dgibson/dtc/tree/overlay for some incomplete
work I did in this direction.

In addition to not making unsafe assumptions about parser operation, I
think this will also allow for better error reporting.  It also moves
more code into plain-old-C instead of potentially hard to follow bison
code fragments.

>  %}
>  
>  %union {
> @@ -52,9 +56,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 +77,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 +109,36 @@ 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));
> +			the_versionflags |= VF_DT_V1;
> +			$$ = the_versionflags;
>  		}
>  	;
>  
>  v1tag:
>  	  DT_V1 ';'
> +	| DT_V1
>  	| DT_V1 ';' v1tag
> +
> +plugindecl:
> +	DT_PLUGIN ';'
> +		{
> +			the_versionflags |= VF_PLUGIN;
> +			$$ = VF_PLUGIN;
> +		}
> +	| /* empty */
> +		{
> +			$$ = 0;
> +		}
>  	;
>  
>  memreserves:
> @@ -161,10 +189,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 (the_versionflags & VF_PLUGIN)
> +					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 +211,11 @@ devicetree:
>  
>  			$$ = $1;
>  		}
> +	| /* empty */
> +		{
> +			/* build empty node */
> +			$$ = name_node(build_node(NULL, NULL), "");
> +		}
>  	;
>  
>  nodedef:
> diff --git a/dtc.c b/dtc.c
> index 9dcf640..a654267 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 new_magic;			/* use new 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'},
> +	{"new-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\tUse new blog magic value",
>  	"\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':
> +			new_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);
> +	}
> +
>  	if (sort)
>  		sort_tree(bi);
>  
> @@ -318,12 +346,15 @@ int main(int argc, char *argv[])
>  			    outname, strerror(errno));
>  	}
>  
> +	if (new_magic && (bi->versionflags & VF_PLUGIN))
> +		out_magic = FDT_MAGIC_DTBO;
> +
>  	if (streq(outform, "dts")) {
>  		dt_to_source(outf, bi);
>  	} else if (streq(outform, "dtb")) {
> -		dt_to_blob(outf, bi, outversion);
> +		dt_to_blob(outf, bi, out_magic, outversion);
>  	} else if (streq(outform, "asm")) {
> -		dt_to_asm(outf, bi, outversion);
> +		dt_to_asm(outf, bi, out_magic, outversion);
>  	} else if (streq(outform, "null")) {
>  		/* do nothing */
>  	} else {
> diff --git a/dtc.h b/dtc.h
> index 32009bc..889b8f8 100644
> --- a/dtc.h
> +++ b/dtc.h
> @@ -55,6 +55,9 @@ extern int minsize;		/* Minimum blob size */
>  extern int padsize;		/* Additional padding to blob */
>  extern int alignsize;		/* Additional padding to blob accroding to the alignsize */
>  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 */
> +extern int new_magic;		/* use new FDT magic values for objects */
>  
>  #define PHANDLE_LEGACY	0x1
>  #define PHANDLE_EPAPR	0x2
> @@ -195,6 +198,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);
> @@ -202,6 +206,8 @@ void delete_property(struct property *prop);
>  void add_child(struct node *parent, struct node *child);
>  void delete_node_by_name(struct node *parent, char *name);
>  void delete_node(struct node *node);
> +void append_to_property(struct node *node,
> +			char *name, const void *data, int len);
>  
>  const char *get_unitname(struct node *node);
>  struct property *get_property(struct node *node, const char *propname);
> @@ -237,14 +243,22 @@ 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/ */
> +#define VF_PLUGIN	0x0002	/* /plugin/ */
> +
> +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 */
>  
> @@ -253,8 +267,8 @@ void process_checks(bool force, struct boot_info *bi);
>  
>  /* Flattened trees */
>  
> -void dt_to_blob(FILE *f, struct boot_info *bi, int version);
> -void dt_to_asm(FILE *f, struct boot_info *bi, int version);
> +void dt_to_blob(FILE *f, struct boot_info *bi, fdt32_t magic, int version);
> +void dt_to_asm(FILE *f, struct boot_info *bi, fdt32_t magic, int version);
>  
>  struct boot_info *dt_from_blob(const char *fname);
>  
> diff --git a/fdtdump.c b/fdtdump.c
> index a9a2484..dd63ac2 100644
> --- a/fdtdump.c
> +++ b/fdtdump.c
> @@ -201,7 +201,7 @@ int main(int argc, char *argv[])
>  			p = memchr(p, smagic[0], endp - p - FDT_MAGIC_SIZE);
>  			if (!p)
>  				break;
> -			if (fdt_magic(p) == FDT_MAGIC) {
> +			if (fdt_magic(p) == FDT_MAGIC || fdt_magic(p) == FDT_MAGIC_DTBO) {
>  				/* try and validate the main struct */
>  				off_t this_len = endp - p;
>  				fdt32_t max_version = 17;
> diff --git a/flattree.c b/flattree.c
> index a9d9520..57d76cf 100644
> --- a/flattree.c
> +++ b/flattree.c
> @@ -335,6 +335,7 @@ static struct data flatten_reserve_list(struct reserve_info *reservelist,
>  }
>  
>  static void make_fdt_header(struct fdt_header *fdt,
> +			    fdt32_t magic,
>  			    struct version_info *vi,
>  			    int reservesize, int dtsize, int strsize,
>  			    int boot_cpuid_phys)
> @@ -345,7 +346,7 @@ static void make_fdt_header(struct fdt_header *fdt,
>  
>  	memset(fdt, 0xff, sizeof(*fdt));
>  
> -	fdt->magic = cpu_to_fdt32(FDT_MAGIC);
> +	fdt->magic = cpu_to_fdt32(magic);
>  	fdt->version = cpu_to_fdt32(vi->version);
>  	fdt->last_comp_version = cpu_to_fdt32(vi->last_comp_version);
>  
> @@ -366,7 +367,7 @@ static void make_fdt_header(struct fdt_header *fdt,
>  		fdt->size_dt_struct = cpu_to_fdt32(dtsize);
>  }
>  
> -void dt_to_blob(FILE *f, struct boot_info *bi, int version)
> +void dt_to_blob(FILE *f, struct boot_info *bi, fdt32_t magic, int version)
>  {
>  	struct version_info *vi = NULL;
>  	int i;
> @@ -390,7 +391,7 @@ void dt_to_blob(FILE *f, struct boot_info *bi, int version)
>  	reservebuf = flatten_reserve_list(bi->reservelist, vi);
>  
>  	/* Make header */
> -	make_fdt_header(&fdt, vi, reservebuf.len, dtbuf.len, strbuf.len,
> +	make_fdt_header(&fdt, magic, vi, reservebuf.len, dtbuf.len, strbuf.len,
>  			bi->boot_cpuid_phys);
>  
>  	/*
> @@ -467,7 +468,7 @@ static void dump_stringtable_asm(FILE *f, struct data strbuf)
>  	}
>  }
>  
> -void dt_to_asm(FILE *f, struct boot_info *bi, int version)
> +void dt_to_asm(FILE *f, struct boot_info *bi, fdt32_t magic, int version)
>  {
>  	struct version_info *vi = NULL;
>  	int i;
> @@ -830,6 +831,7 @@ struct boot_info *dt_from_blob(const char *fname)
>  	struct node *tree;
>  	uint32_t val;
>  	int flags = 0;
> +	unsigned int versionflags = VF_DT_V1;
>  
>  	f = srcfile_relative_open(fname, NULL);
>  
> @@ -845,9 +847,12 @@ struct boot_info *dt_from_blob(const char *fname)
>  	}
>  
>  	magic = fdt32_to_cpu(magic);
> -	if (magic != FDT_MAGIC)
> +	if (magic != FDT_MAGIC && magic != FDT_MAGIC_DTBO)
>  		die("Blob has incorrect magic number\n");
>  
> +	if (magic == FDT_MAGIC_DTBO)
> +		versionflags |= VF_PLUGIN;
> +
>  	rc = fread(&totalsize, sizeof(totalsize), 1, f);
>  	if (ferror(f))
>  		die("Error reading DT blob size: %s\n", strerror(errno));
> @@ -942,5 +947,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(versionflags, 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/libfdt/fdt.c b/libfdt/fdt.c
> index 22286a1..28d422c 100644
> --- a/libfdt/fdt.c
> +++ b/libfdt/fdt.c
> @@ -57,7 +57,7 @@
>  
>  int fdt_check_header(const void *fdt)
>  {
> -	if (fdt_magic(fdt) == FDT_MAGIC) {
> +	if (fdt_magic(fdt) == FDT_MAGIC || fdt_magic(fdt) == FDT_MAGIC_DTBO) {
>  		/* Complete tree */
>  		if (fdt_version(fdt) < FDT_FIRST_SUPPORTED_VERSION)
>  			return -FDT_ERR_BADVERSION;
> diff --git a/libfdt/fdt.h b/libfdt/fdt.h
> index 526aedb..493cd55 100644
> --- a/libfdt/fdt.h
> +++ b/libfdt/fdt.h
> @@ -55,7 +55,7 @@
>  #ifndef __ASSEMBLY__
>  
>  struct fdt_header {
> -	fdt32_t magic;			 /* magic word FDT_MAGIC */
> +	fdt32_t magic;			 /* magic word FDT_MAGIC[|_DTBO] */
>  	fdt32_t totalsize;		 /* total size of DT block */
>  	fdt32_t off_dt_struct;		 /* offset to structure */
>  	fdt32_t off_dt_strings;		 /* offset to strings */
> @@ -93,6 +93,7 @@ struct fdt_property {
>  #endif /* !__ASSEMBLY */
>  
>  #define FDT_MAGIC	0xd00dfeed	/* 4: version, 4: total size */
> +#define FDT_MAGIC_DTBO	0xd00dfdb0	/* DTBO magic */
>  #define FDT_TAGSIZE	sizeof(fdt32_t)
>  
>  #define FDT_BEGIN_NODE	0x1		/* Start node: full name */
> diff --git a/livetree.c b/livetree.c
> index 3dc7559..1a2f4b1 100644
> --- a/livetree.c
> +++ b/livetree.c
> @@ -216,6 +216,31 @@ 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));

You shouldn't use a bare malloc() for a node.  Use build_node() instead.

> +	struct property *p;
> +	struct data d = empty_data;
> +	char *name;
> +
> +	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);
> +
> +	xasprintf(&name, "fragment@%u",
> +			next_orphan_fragment++);
> +	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);
> @@ -296,6 +321,23 @@ void delete_node(struct node *node)
>  	delete_labels(&node->labels);
>  }
>  
> +void append_to_property(struct node *node,
> +				    char *name, const void *data, int len)
> +{
> +	struct data d;
> +	struct property *p;
> +
> +	p = get_property(node, name);
> +	if (p) {
> +		d = data_append_data(p->val, data, len);
> +		p->val = d;
> +	} else {
> +		d = data_append_data(empty_data, data, len);
> +		p = build_property(name, d);
> +		add_property(node, p);
> +	}
> +}
> +
>  struct reserve_info *build_reserve_entry(uint64_t address, uint64_t size)
>  {
>  	struct reserve_info *new = xmalloc(sizeof(*new));
> @@ -335,12 +377,14 @@ 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;
> @@ -709,3 +753,182 @@ 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,
> +					 struct node *an, bool allocph)
> +{
> +	struct node *c;
> +	struct property *p;
> +	struct label *l;
> +
> +	/* if if there are labels */
> +	if (node->labels) {
> +		/* 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,
> +					an->name);
> +				continue;
> +			}
> +
> +			/* insert it */
> +			p = build_property(l->label,
> +				data_copy_escape_string(node->fullpath,
> +						strlen(node->fullpath)));

Um.. what?  The node path should absolutely not be an escape string.
It shouldn't contain escapes to begin with, and if it does, we
absolutely shouldn't be turning them into special characters here.

> +			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, an, allocph);
> +}
> +
> +void generate_label_tree(struct node *dt, char *gen_node_name, bool allocph)
> +{
> +	struct node *an;
> +
> +	for_each_child(dt, an)
> +		if (streq(gen_node_name, an->name))
> +			break;
> +
> +	if (!an)
> +		an = build_and_name_child_node(dt, gen_node_name);
> +	if (!an)
> +		die("Could not build label node /%s\n", gen_node_name);
> +
> +	generate_label_tree_internal(dt, dt, an, allocph);
> +}
> +
> +static char *fixups_name = "__fixups__";
> +static char *local_fixups_name = "__local_fixups__";

I'd actually prefer #defines for these, and all-caps names, so it's
more obvious when they're used that these are global constants.

> +
> +static void add_fixup_entry(struct node *dt, struct node *node,
> +		struct property *prop, struct marker *m)
> +{
> +	struct node *fn;	/* fixup node */
> +	char *entry;
> +
> +	/* m->ref can only be a REF_PHANDLE, but check anyway */
> +	assert(m->type == REF_PHANDLE);
> +
> +	/* fn is the node we're putting entries in */
> +	fn = get_subnode(dt, fixups_name);
> +	assert(fn != NULL);
> +
> +	/* there shouldn't be any ':' in the arguments */
> +	if (strchr(node->fullpath, ':') || strchr(prop->name, ':'))
> +		die("arguments should not contain ':'\n");
> +
> +	xasprintf(&entry, "%s:%s:%u",
> +			node->fullpath, prop->name, m->offset);
> +	append_to_property(fn, m->ref, entry, strlen(entry) + 1);
> +}
> +
> +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 */
> +	uint32_t value_32;
> +	char *s, *e, *comp;
> +	int len;
> +
> +	/* fn is the node we're putting entries in */
> +	lfn = get_subnode(dt, local_fixups_name);
> +	assert(lfn != NULL);
> +
> +	/* 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) {
> +		/* 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);
> +
> +	value_32 = cpu_to_fdt32(m->offset);
> +	append_to_property(wn, prop->name, &value_32, sizeof(value_32));
> +}
> +
> +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)
> +{
> +	struct node *an;
> +
> +	for_each_child(dt, an)
> +		if (streq(fixups_name, an->name))
> +			break;
> +
> +	if (!an)
> +		build_and_name_child_node(dt, fixups_name);
> +
> +	for_each_child(dt, an)
> +		if (streq(local_fixups_name, an->name))
> +			break;
> +
> +	if (!an)
> +		build_and_name_child_node(dt, local_fixups_name);
> +
> +	generate_fixups_tree_internal(dt, dt);
> +}
> diff --git a/tests/mangle-layout.c b/tests/mangle-layout.c
> index a76e51e..d29ebc6 100644
> --- a/tests/mangle-layout.c
> +++ b/tests/mangle-layout.c
> @@ -42,7 +42,8 @@ static void expand_buf(struct bufstate *buf, int newsize)
>  	buf->size = newsize;
>  }
>  
> -static void new_header(struct bufstate *buf, int version, const void *fdt)
> +static void new_header(struct bufstate *buf, fdt32_t magic, int version,
> +		       const void *fdt)
>  {
>  	int hdrsize;
>  
> @@ -56,7 +57,7 @@ static void new_header(struct bufstate *buf, int version, const void *fdt)
>  	expand_buf(buf, hdrsize);
>  	memset(buf->buf, 0, hdrsize);
>  
> -	fdt_set_magic(buf->buf, FDT_MAGIC);
> +	fdt_set_magic(buf->buf, magic);
>  	fdt_set_version(buf->buf, version);
>  	fdt_set_last_comp_version(buf->buf, 16);
>  	fdt_set_boot_cpuid_phys(buf->buf, fdt_boot_cpuid_phys(fdt));
> @@ -145,7 +146,7 @@ int main(int argc, char *argv[])
>  	if (fdt_version(fdt) < 17)
>  		CONFIG("Input tree must be v17");
>  
> -	new_header(&buf, version, fdt);
> +	new_header(&buf, FDT_MAGIC, version, fdt);
>  
>  	while (*blockorder) {
>  		add_block(&buf, version, *blockorder, fdt);
> diff --git a/treesource.c b/treesource.c
> index a55d1d1..75e920d 100644
> --- a/treesource.c
> +++ b/treesource.c
> @@ -267,7 +267,12 @@ void dt_to_source(FILE *f, struct boot_info *bi)
>  {
>  	struct reserve_info *re;
>  
> -	fprintf(f, "/dts-v1/;\n\n");
> +	fprintf(f, "/dts-v1/");
> +
> +	if (bi->versionflags & VF_PLUGIN)
> +		fprintf(f, " /plugin/");
> +
> +	fprintf(f, ";\n\n");

I'm not sure this really makes sense.  The /plugin/ tag triggers the
fixup construction and encoding of overlay fragments.  But in an
incoming dtb, that processing has already been done once, doing it
again would not make sense.  So I think the output should not have the
/plugin/ tag, even if the input did.

Unless, of course, you parsed the input into an explicit list of
overlays, then output it here again as a list of overlays, not the
encoded fragments.  i.e. if this was the original tree:

	/dts-v1/ /plugin/;

	&somwhere {
		name = "value";
	};

then legitimately the output of -I dts -O dts could be either:

	/dts-v1/ /plugin/;

	&somwhere {
		name = "value";
	};

OR

	/dts-v1/;

	/ {
		fragment@0 {
			target = <0xffffffff>;
			__overlay__{
				name = "value";
			};
		};
		__fixups__ {
			somewhere = "/fragment@0:target:0";
		};
	};

But it doesn't make sense to combine the two: the second structure
with the "/plugin/" tag.

Another advantage of moving the overlay application out of the parser
is you can sanely produce either output, both of which could be useful
in the right circumstances.  You can potentially even produce the
first output (or something close) from dtb input, with correct parsing
of the overlay structure on dtb input.

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

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

[Pantelis Antoniou]

Hi David,

> On Nov 25, 2016, at 06:11 , David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> 
> On Thu, Nov 24, 2016 at 02:31:32PM +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 disabled. This is in order to give time for DT blob
>> methods to be updated.
>> 
>> 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             |  49 +++++++++--
>> 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 +-
>> treesource.c             |   7 +-
>> 14 files changed, 380 insertions(+), 31 deletions(-)
>> 
>> diff --git a/Documentation/manual.txt b/Documentation/manual.txt
>> index 398de32..65fbf09 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 new FDT magic number. By default blobs with the
>> +	standard FDT magic number are generated.
> 
> First, this description is incomplete since -M *only* affects the
> magic number for /plugin/ input, not in other cases.  Second, the
> default is the wrong way around.  If we make old-style the default,
> then new-style will never be used, which defeats the purpose.
> 

Then we’ll break user-space that has this assumption (i.e. that the magic is the same).
I can certainly do it the other way around.

>> +
>>     -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 609975a..bc03d42 100644
>> --- a/checks.c
>> +++ b/checks.c
>> @@ -486,8 +486,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..4afc592 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,9 @@ extern void yyerror(char const *s);
>> 
>> extern struct boot_info *the_boot_info;
>> extern bool treesource_error;
>> +
>> +/* temporary while the tree is not built */
>> +static unsigned int the_versionflags;
> 
> Hrm.  Using a global during parsing is pretty dangerous - it makes
> assumptions about the order in which bison will execute semantic
> actions that may not always be correct.
> 
> It'll probably work in practice, so I *might* be convinced it's
> adequate for a first cut.  I'm a bit reluctant though, since I suspect
> once merged it will become entrenched.
> 

We use bison, globals are the way of life. It’s not going to be used
anywhere else, it’s static in the parser file.

We could allocate the boot_info earlier (when the v1tag is detected) but
that would be quite a big change for something as trivial. 

> The correct way to handle this, IMO, is not to ever attempt to apply
> overlays during the parse.  Basically, we'd change the overall
> structure so that the output from the parser is not a single tree, but
> a list of overlays / fragments.  Then, once the parse is complete, so
> versioninfo (which could now become a member of bootinfo) is well
> defined, we either apply the fragments in place (base tree) or encode
> them into the overlay structure (plugin mode).
> 
> See https://github.com/dgibson/dtc/tree/overlay for some incomplete
> work I did in this direction.
> 

This tree is pretty stale; last commit was back in march.

I thing there’s a wrong assumption here. The purpose of this patch is not
to apply overlays during compile time, is to generate a blob that can be
applied at runtime by another piece of software.

> In addition to not making unsafe assumptions about parser operation, I
> think this will also allow for better error reporting.  It also moves
> more code into plain-old-C instead of potentially hard to follow bison
> code fragments.
> 

We don’t try to apply overlays during parse, and especially in the parse code.
The global is used only for the syntactic sugar method of turning a &ref { };
node into an overlay.

>> %}
>> 
>> %union {
>> @@ -52,9 +56,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 +77,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 +109,36 @@ 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));
>> +			the_versionflags |= VF_DT_V1;
>> +			$$ = the_versionflags;
>> 		}
>> 	;
>> 
>> v1tag:
>> 	  DT_V1 ';'
>> +	| DT_V1
>> 	| DT_V1 ';' v1tag
>> +
>> +plugindecl:
>> +	DT_PLUGIN ';'
>> +		{
>> +			the_versionflags |= VF_PLUGIN;
>> +			$$ = VF_PLUGIN;
>> +		}
>> +	| /* empty */
>> +		{
>> +			$$ = 0;
>> +		}
>> 	;
>> 
>> memreserves:
>> @@ -161,10 +189,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 (the_versionflags & VF_PLUGIN)
>> +					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 +211,11 @@ devicetree:
>> 
>> 			$$ = $1;
>> 		}
>> +	| /* empty */
>> +		{
>> +			/* build empty node */
>> +			$$ = name_node(build_node(NULL, NULL), "");
>> +		}
>> 	;
>> 
>> nodedef:
>> diff --git a/dtc.c b/dtc.c
>> index 9dcf640..a654267 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 new_magic;			/* use new 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'},
>> +	{"new-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\tUse new blog magic value",
>> 	"\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':
>> +			new_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);
>> +	}
>> +
>> 	if (sort)
>> 		sort_tree(bi);
>> 
>> @@ -318,12 +346,15 @@ int main(int argc, char *argv[])
>> 			    outname, strerror(errno));
>> 	}
>> 
>> +	if (new_magic && (bi->versionflags & VF_PLUGIN))
>> +		out_magic = FDT_MAGIC_DTBO;
>> +
>> 	if (streq(outform, "dts")) {
>> 		dt_to_source(outf, bi);
>> 	} else if (streq(outform, "dtb")) {
>> -		dt_to_blob(outf, bi, outversion);
>> +		dt_to_blob(outf, bi, out_magic, outversion);
>> 	} else if (streq(outform, "asm")) {
>> -		dt_to_asm(outf, bi, outversion);
>> +		dt_to_asm(outf, bi, out_magic, outversion);
>> 	} else if (streq(outform, "null")) {
>> 		/* do nothing */
>> 	} else {
>> diff --git a/dtc.h b/dtc.h
>> index 32009bc..889b8f8 100644
>> --- a/dtc.h
>> +++ b/dtc.h
>> @@ -55,6 +55,9 @@ extern int minsize;		/* Minimum blob size */
>> extern int padsize;		/* Additional padding to blob */
>> extern int alignsize;		/* Additional padding to blob accroding to the alignsize */
>> 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 */
>> +extern int new_magic;		/* use new FDT magic values for objects */
>> 
>> #define PHANDLE_LEGACY	0x1
>> #define PHANDLE_EPAPR	0x2
>> @@ -195,6 +198,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);
>> @@ -202,6 +206,8 @@ void delete_property(struct property *prop);
>> void add_child(struct node *parent, struct node *child);
>> void delete_node_by_name(struct node *parent, char *name);
>> void delete_node(struct node *node);
>> +void append_to_property(struct node *node,
>> +			char *name, const void *data, int len);
>> 
>> const char *get_unitname(struct node *node);
>> struct property *get_property(struct node *node, const char *propname);
>> @@ -237,14 +243,22 @@ 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/ */
>> +#define VF_PLUGIN	0x0002	/* /plugin/ */
>> +
>> +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 */
>> 
>> @@ -253,8 +267,8 @@ void process_checks(bool force, struct boot_info *bi);
>> 
>> /* Flattened trees */
>> 
>> -void dt_to_blob(FILE *f, struct boot_info *bi, int version);
>> -void dt_to_asm(FILE *f, struct boot_info *bi, int version);
>> +void dt_to_blob(FILE *f, struct boot_info *bi, fdt32_t magic, int version);
>> +void dt_to_asm(FILE *f, struct boot_info *bi, fdt32_t magic, int version);
>> 
>> struct boot_info *dt_from_blob(const char *fname);
>> 
>> diff --git a/fdtdump.c b/fdtdump.c
>> index a9a2484..dd63ac2 100644
>> --- a/fdtdump.c
>> +++ b/fdtdump.c
>> @@ -201,7 +201,7 @@ int main(int argc, char *argv[])
>> 			p = memchr(p, smagic[0], endp - p - FDT_MAGIC_SIZE);
>> 			if (!p)
>> 				break;
>> -			if (fdt_magic(p) == FDT_MAGIC) {
>> +			if (fdt_magic(p) == FDT_MAGIC || fdt_magic(p) == FDT_MAGIC_DTBO) {
>> 				/* try and validate the main struct */
>> 				off_t this_len = endp - p;
>> 				fdt32_t max_version = 17;
>> diff --git a/flattree.c b/flattree.c
>> index a9d9520..57d76cf 100644
>> --- a/flattree.c
>> +++ b/flattree.c
>> @@ -335,6 +335,7 @@ static struct data flatten_reserve_list(struct reserve_info *reservelist,
>> }
>> 
>> static void make_fdt_header(struct fdt_header *fdt,
>> +			    fdt32_t magic,
>> 			    struct version_info *vi,
>> 			    int reservesize, int dtsize, int strsize,
>> 			    int boot_cpuid_phys)
>> @@ -345,7 +346,7 @@ static void make_fdt_header(struct fdt_header *fdt,
>> 
>> 	memset(fdt, 0xff, sizeof(*fdt));
>> 
>> -	fdt->magic = cpu_to_fdt32(FDT_MAGIC);
>> +	fdt->magic = cpu_to_fdt32(magic);
>> 	fdt->version = cpu_to_fdt32(vi->version);
>> 	fdt->last_comp_version = cpu_to_fdt32(vi->last_comp_version);
>> 
>> @@ -366,7 +367,7 @@ static void make_fdt_header(struct fdt_header *fdt,
>> 		fdt->size_dt_struct = cpu_to_fdt32(dtsize);
>> }
>> 
>> -void dt_to_blob(FILE *f, struct boot_info *bi, int version)
>> +void dt_to_blob(FILE *f, struct boot_info *bi, fdt32_t magic, int version)
>> {
>> 	struct version_info *vi = NULL;
>> 	int i;
>> @@ -390,7 +391,7 @@ void dt_to_blob(FILE *f, struct boot_info *bi, int version)
>> 	reservebuf = flatten_reserve_list(bi->reservelist, vi);
>> 
>> 	/* Make header */
>> -	make_fdt_header(&fdt, vi, reservebuf.len, dtbuf.len, strbuf.len,
>> +	make_fdt_header(&fdt, magic, vi, reservebuf.len, dtbuf.len, strbuf.len,
>> 			bi->boot_cpuid_phys);
>> 
>> 	/*
>> @@ -467,7 +468,7 @@ static void dump_stringtable_asm(FILE *f, struct data strbuf)
>> 	}
>> }
>> 
>> -void dt_to_asm(FILE *f, struct boot_info *bi, int version)
>> +void dt_to_asm(FILE *f, struct boot_info *bi, fdt32_t magic, int version)
>> {
>> 	struct version_info *vi = NULL;
>> 	int i;
>> @@ -830,6 +831,7 @@ struct boot_info *dt_from_blob(const char *fname)
>> 	struct node *tree;
>> 	uint32_t val;
>> 	int flags = 0;
>> +	unsigned int versionflags = VF_DT_V1;
>> 
>> 	f = srcfile_relative_open(fname, NULL);
>> 
>> @@ -845,9 +847,12 @@ struct boot_info *dt_from_blob(const char *fname)
>> 	}
>> 
>> 	magic = fdt32_to_cpu(magic);
>> -	if (magic != FDT_MAGIC)
>> +	if (magic != FDT_MAGIC && magic != FDT_MAGIC_DTBO)
>> 		die("Blob has incorrect magic number\n");
>> 
>> +	if (magic == FDT_MAGIC_DTBO)
>> +		versionflags |= VF_PLUGIN;
>> +
>> 	rc = fread(&totalsize, sizeof(totalsize), 1, f);
>> 	if (ferror(f))
>> 		die("Error reading DT blob size: %s\n", strerror(errno));
>> @@ -942,5 +947,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(versionflags, 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/libfdt/fdt.c b/libfdt/fdt.c
>> index 22286a1..28d422c 100644
>> --- a/libfdt/fdt.c
>> +++ b/libfdt/fdt.c
>> @@ -57,7 +57,7 @@
>> 
>> int fdt_check_header(const void *fdt)
>> {
>> -	if (fdt_magic(fdt) == FDT_MAGIC) {
>> +	if (fdt_magic(fdt) == FDT_MAGIC || fdt_magic(fdt) == FDT_MAGIC_DTBO) {
>> 		/* Complete tree */
>> 		if (fdt_version(fdt) < FDT_FIRST_SUPPORTED_VERSION)
>> 			return -FDT_ERR_BADVERSION;
>> diff --git a/libfdt/fdt.h b/libfdt/fdt.h
>> index 526aedb..493cd55 100644
>> --- a/libfdt/fdt.h
>> +++ b/libfdt/fdt.h
>> @@ -55,7 +55,7 @@
>> #ifndef __ASSEMBLY__
>> 
>> struct fdt_header {
>> -	fdt32_t magic;			 /* magic word FDT_MAGIC */
>> +	fdt32_t magic;			 /* magic word FDT_MAGIC[|_DTBO] */
>> 	fdt32_t totalsize;		 /* total size of DT block */
>> 	fdt32_t off_dt_struct;		 /* offset to structure */
>> 	fdt32_t off_dt_strings;		 /* offset to strings */
>> @@ -93,6 +93,7 @@ struct fdt_property {
>> #endif /* !__ASSEMBLY */
>> 
>> #define FDT_MAGIC	0xd00dfeed	/* 4: version, 4: total size */
>> +#define FDT_MAGIC_DTBO	0xd00dfdb0	/* DTBO magic */
>> #define FDT_TAGSIZE	sizeof(fdt32_t)
>> 
>> #define FDT_BEGIN_NODE	0x1		/* Start node: full name */
>> diff --git a/livetree.c b/livetree.c
>> index 3dc7559..1a2f4b1 100644
>> --- a/livetree.c
>> +++ b/livetree.c
>> @@ -216,6 +216,31 @@ 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));
> 
> You shouldn't use a bare malloc() for a node.  Use build_node() instead.
> 

OK.

>> +	struct property *p;
>> +	struct data d = empty_data;
>> +	char *name;
>> +
>> +	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);
>> +
>> +	xasprintf(&name, "fragment@%u",
>> +			next_orphan_fragment++);
>> +	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);
>> @@ -296,6 +321,23 @@ void delete_node(struct node *node)
>> 	delete_labels(&node->labels);
>> }
>> 
>> +void append_to_property(struct node *node,
>> +				    char *name, const void *data, int len)
>> +{
>> +	struct data d;
>> +	struct property *p;
>> +
>> +	p = get_property(node, name);
>> +	if (p) {
>> +		d = data_append_data(p->val, data, len);
>> +		p->val = d;
>> +	} else {
>> +		d = data_append_data(empty_data, data, len);
>> +		p = build_property(name, d);
>> +		add_property(node, p);
>> +	}
>> +}
>> +
>> struct reserve_info *build_reserve_entry(uint64_t address, uint64_t size)
>> {
>> 	struct reserve_info *new = xmalloc(sizeof(*new));
>> @@ -335,12 +377,14 @@ 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;
>> @@ -709,3 +753,182 @@ 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,
>> +					 struct node *an, bool allocph)
>> +{
>> +	struct node *c;
>> +	struct property *p;
>> +	struct label *l;
>> +
>> +	/* if if there are labels */
>> +	if (node->labels) {
>> +		/* 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,
>> +					an->name);
>> +				continue;
>> +			}
>> +
>> +			/* insert it */
>> +			p = build_property(l->label,
>> +				data_copy_escape_string(node->fullpath,
>> +						strlen(node->fullpath)));
> 
> Um.. what?  The node path should absolutely not be an escape string.
> It shouldn't contain escapes to begin with, and if it does, we
> absolutely shouldn't be turning them into special characters here.
> 

OK, trivial enough to do. I tried to play it safe here but this is an overkill.

>> +			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, an, allocph);
>> +}
>> +
>> +void generate_label_tree(struct node *dt, char *gen_node_name, bool allocph)
>> +{
>> +	struct node *an;
>> +
>> +	for_each_child(dt, an)
>> +		if (streq(gen_node_name, an->name))
>> +			break;
>> +
>> +	if (!an)
>> +		an = build_and_name_child_node(dt, gen_node_name);
>> +	if (!an)
>> +		die("Could not build label node /%s\n", gen_node_name);
>> +
>> +	generate_label_tree_internal(dt, dt, an, allocph);
>> +}
>> +
>> +static char *fixups_name = "__fixups__";
>> +static char *local_fixups_name = "__local_fixups__";
> 
> I'd actually prefer #defines for these, and all-caps names, so it's
> more obvious when they're used that these are global constants.
> 

OK.

>> +
>> +static void add_fixup_entry(struct node *dt, struct node *node,
>> +		struct property *prop, struct marker *m)
>> +{
>> +	struct node *fn;	/* fixup node */
>> +	char *entry;
>> +
>> +	/* m->ref can only be a REF_PHANDLE, but check anyway */
>> +	assert(m->type == REF_PHANDLE);
>> +
>> +	/* fn is the node we're putting entries in */
>> +	fn = get_subnode(dt, fixups_name);
>> +	assert(fn != NULL);
>> +
>> +	/* there shouldn't be any ':' in the arguments */
>> +	if (strchr(node->fullpath, ':') || strchr(prop->name, ':'))
>> +		die("arguments should not contain ':'\n");
>> +
>> +	xasprintf(&entry, "%s:%s:%u",
>> +			node->fullpath, prop->name, m->offset);
>> +	append_to_property(fn, m->ref, entry, strlen(entry) + 1);
>> +}
>> +
>> +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 */
>> +	uint32_t value_32;
>> +	char *s, *e, *comp;
>> +	int len;
>> +
>> +	/* fn is the node we're putting entries in */
>> +	lfn = get_subnode(dt, local_fixups_name);
>> +	assert(lfn != NULL);
>> +
>> +	/* 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) {
>> +		/* 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);
>> +
>> +	value_32 = cpu_to_fdt32(m->offset);
>> +	append_to_property(wn, prop->name, &value_32, sizeof(value_32));
>> +}
>> +
>> +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)
>> +{
>> +	struct node *an;
>> +
>> +	for_each_child(dt, an)
>> +		if (streq(fixups_name, an->name))
>> +			break;
>> +
>> +	if (!an)
>> +		build_and_name_child_node(dt, fixups_name);
>> +
>> +	for_each_child(dt, an)
>> +		if (streq(local_fixups_name, an->name))
>> +			break;
>> +
>> +	if (!an)
>> +		build_and_name_child_node(dt, local_fixups_name);
>> +
>> +	generate_fixups_tree_internal(dt, dt);
>> +}
>> diff --git a/tests/mangle-layout.c b/tests/mangle-layout.c
>> index a76e51e..d29ebc6 100644
>> --- a/tests/mangle-layout.c
>> +++ b/tests/mangle-layout.c
>> @@ -42,7 +42,8 @@ static void expand_buf(struct bufstate *buf, int newsize)
>> 	buf->size = newsize;
>> }
>> 
>> -static void new_header(struct bufstate *buf, int version, const void *fdt)
>> +static void new_header(struct bufstate *buf, fdt32_t magic, int version,
>> +		       const void *fdt)
>> {
>> 	int hdrsize;
>> 
>> @@ -56,7 +57,7 @@ static void new_header(struct bufstate *buf, int version, const void *fdt)
>> 	expand_buf(buf, hdrsize);
>> 	memset(buf->buf, 0, hdrsize);
>> 
>> -	fdt_set_magic(buf->buf, FDT_MAGIC);
>> +	fdt_set_magic(buf->buf, magic);
>> 	fdt_set_version(buf->buf, version);
>> 	fdt_set_last_comp_version(buf->buf, 16);
>> 	fdt_set_boot_cpuid_phys(buf->buf, fdt_boot_cpuid_phys(fdt));
>> @@ -145,7 +146,7 @@ int main(int argc, char *argv[])
>> 	if (fdt_version(fdt) < 17)
>> 		CONFIG("Input tree must be v17");
>> 
>> -	new_header(&buf, version, fdt);
>> +	new_header(&buf, FDT_MAGIC, version, fdt);
>> 
>> 	while (*blockorder) {
>> 		add_block(&buf, version, *blockorder, fdt);
>> diff --git a/treesource.c b/treesource.c
>> index a55d1d1..75e920d 100644
>> --- a/treesource.c
>> +++ b/treesource.c
>> @@ -267,7 +267,12 @@ void dt_to_source(FILE *f, struct boot_info *bi)
>> {
>> 	struct reserve_info *re;
>> 
>> -	fprintf(f, "/dts-v1/;\n\n");
>> +	fprintf(f, "/dts-v1/");
>> +
>> +	if (bi->versionflags & VF_PLUGIN)
>> +		fprintf(f, " /plugin/");
>> +
>> +	fprintf(f, ";\n\n");
> 
> I'm not sure this really makes sense.  The /plugin/ tag triggers the
> fixup construction and encoding of overlay fragments.  But in an
> incoming dtb, that processing has already been done once, doing it
> again would not make sense.  So I think the output should not have the
> /plugin/ tag, even if the input did.
> 
> Unless, of course, you parsed the input into an explicit list of
> overlays, then output it here again as a list of overlays, not the
> encoded fragments.  i.e. if this was the original tree:
> 
> 	/dts-v1/ /plugin/;
> 
> 	&somwhere {
> 		name = "value";
> 	};
> 
> then legitimately the output of -I dts -O dts could be either:
> 
> 	/dts-v1/ /plugin/;
> 
> 	&somwhere {
> 		name = "value";
> 	};
> 
> OR
> 
> 	/dts-v1/;
> 
> 	/ {
> 		fragment@0 {
> 			target = <0xffffffff>;
> 			__overlay__{
> 				name = "value";
> 			};
> 		};
> 		__fixups__ {
> 			somewhere = "/fragment@0:target:0";
> 		};
> 	};
> 
> But it doesn't make sense to combine the two: the second structure
> with the "/plugin/" tag.
> 

> Another advantage of moving the overlay application out of the parser
> is you can sanely produce either output, both of which could be useful
> in the right circumstances.  You can potentially even produce the
> first output (or something close) from dtb input, with correct parsing
> of the overlay structure on dtb input.
> 

Yes, this makes sense. For now I’ll remove the plugin tag, but if the blob
was compiled with -@ you could conceivably reverse back to a form that contains
labels and phandle references correctly.

But this is quite complicated to undertake in this patch series. 

To re-iterate though there is no overlay application here :)

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

Regards

— Pantelis

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[David Gibson]

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

On Fri, Nov 25, 2016 at 12:55:25PM +0200, Pantelis Antoniou wrote:
> Hi David,
> 
> > On Nov 25, 2016, at 06:11 , David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > 
> > On Thu, Nov 24, 2016 at 02:31:32PM +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 disabled. This is in order to give time for DT blob
> >> methods to be updated.
> >> 
> >> 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             |  49 +++++++++--
> >> 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 +-
> >> treesource.c             |   7 +-
> >> 14 files changed, 380 insertions(+), 31 deletions(-)
> >> 
> >> diff --git a/Documentation/manual.txt b/Documentation/manual.txt
> >> index 398de32..65fbf09 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 new FDT magic number. By default blobs with the
> >> +	standard FDT magic number are generated.
> > 
> > First, this description is incomplete since -M *only* affects the
> > magic number for /plugin/ input, not in other cases.  Second, the
> > default is the wrong way around.  If we make old-style the default,
> > then new-style will never be used, which defeats the purpose.
> 
> Then we’ll break user-space that has this assumption (i.e. that the magic is the same).
> I can certainly do it the other way around.

Which userspace in particular?

> >> +
> >>     -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 609975a..bc03d42 100644
> >> --- a/checks.c
> >> +++ b/checks.c
> >> @@ -486,8 +486,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..4afc592 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,9 @@ extern void yyerror(char const *s);
> >> 
> >> extern struct boot_info *the_boot_info;
> >> extern bool treesource_error;
> >> +
> >> +/* temporary while the tree is not built */
> >> +static unsigned int the_versionflags;
> > 
> > Hrm.  Using a global during parsing is pretty dangerous - it makes
> > assumptions about the order in which bison will execute semantic
> > actions that may not always be correct.
> > 
> > It'll probably work in practice, so I *might* be convinced it's
> > adequate for a first cut.  I'm a bit reluctant though, since I suspect
> > once merged it will become entrenched.
> > 
> 
> We use bison, globals are the way of life. It’s not going to be used
> anywhere else, it’s static in the parser file.

Using globals to communicate the final result is inevitable (well, not
without using the whole different re-entrant interface stuff).  That
just assumes that the start symbol's semantic action is executed
before the parser competes, which is guaranteed.

This is a different matter - using a global to communicate between
different parts of the parser.  More specifically different parts of
the parser that are in different arms of the parse tree.  That makes
assumptions about the relative order of semantic actions which are
*not* guaranteed.  In theory the parser could generate the entire
parse tree and semantic action dependency graph, then execute them in
an arbitrary order (well, it would have to be a topologically sorted
order, but that won't help).

> We could allocate the boot_info earlier (when the v1tag is detected) but
> that would be quite a big change for something as trivial. 

That wouldn't help.  You still wouldn't have a guarantee on the order
between setting the version flags and using the version flags.

> > The correct way to handle this, IMO, is not to ever attempt to apply
> > overlays during the parse.  Basically, we'd change the overall
> > structure so that the output from the parser is not a single tree, but
> > a list of overlays / fragments.  Then, once the parse is complete, so
> > versioninfo (which could now become a member of bootinfo) is well
> > defined, we either apply the fragments in place (base tree) or encode
> > them into the overlay structure (plugin mode).
> > 
> > See https://github.com/dgibson/dtc/tree/overlay for some incomplete
> > work I did in this direction.
> > 
> 
> This tree is pretty stale; last commit was back in march.

Yes, it was a while since I worked on it.  It rebased clean, though.

> I thing there’s a wrong assumption here. The purpose of this patch is not
> to apply overlays during compile time, is to generate a blob that can be
> applied at runtime by another piece of software.

No, I realise what you're doing.  But the input is in the form of a
batch of overlays, regardless.  You then have two modes of operation:
for base trees you resolve those overlays during the compile, for
plugins you assemble those overlays into a blob which can be applied
later.

Because it wasn't designed with runtime overlays in mind, the current
code handles the resolution of those overlays during the parse.  Your
patch extends that by rather than resolving them, just putting them
together with metadata into the dtbo.

I'm saying that resolution or assembly should be moved out of the
parser.  Instead, the parser output would be an (ordered) list of
overlay fragments.  In the main program the two modes of operation
become explicit: for base trees we resolve the overlays into a single
tree, for plugins we collate the pieces into the dtbo format.

> > In addition to not making unsafe assumptions about parser operation, I
> > think this will also allow for better error reporting.  It also moves
> > more code into plain-old-C instead of potentially hard to follow bison
> > code fragments.
> > 
> 
> We don’t try to apply overlays during parse, and especially in the parse code.

The existing code does this, and you don't change it.  Furthermore you
absolutely do do the assembly of the fragments within the parser -
that's exactly what add_orphan_node() called directly from semantic
actions does.  To verify this, all you need to do is look at the
parser output: the boot_info structure has just a single tree, not a
list of overlays.

> The global is used only for the syntactic sugar method of turning a &ref { };
> node into an overlay.

I'm saying that treating that as mere syntactic sugar is a fundamental
error in design.  We could get away with it when the &ref {} stuff was
always resolved immediately.  Now that that resolution can be delayed
until later, it gets worse.  We should move that resolution outside of
the parser.

> 
> >> %}
> >> 
> >> %union {
> >> @@ -52,9 +56,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 +77,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 +109,36 @@ 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));
> >> +			the_versionflags |= VF_DT_V1;
> >> +			$$ = the_versionflags;
> >> 		}
> >> 	;
> >> 
> >> v1tag:
> >> 	  DT_V1 ';'
> >> +	| DT_V1
> >> 	| DT_V1 ';' v1tag
> >> +
> >> +plugindecl:
> >> +	DT_PLUGIN ';'
> >> +		{
> >> +			the_versionflags |= VF_PLUGIN;
> >> +			$$ = VF_PLUGIN;
> >> +		}
> >> +	| /* empty */
> >> +		{
> >> +			$$ = 0;
> >> +		}
> >> 	;
> >> 
> >> memreserves:
> >> @@ -161,10 +189,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 (the_versionflags & VF_PLUGIN)
> >> +					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 +211,11 @@ devicetree:
> >> 
> >> 			$$ = $1;
> >> 		}
> >> +	| /* empty */
> >> +		{
> >> +			/* build empty node */
> >> +			$$ = name_node(build_node(NULL, NULL), "");
> >> +		}
> >> 	;
> >> 
> >> nodedef:
> >> diff --git a/dtc.c b/dtc.c
> >> index 9dcf640..a654267 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 new_magic;			/* use new 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'},
> >> +	{"new-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\tUse new blog magic value",
> >> 	"\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':
> >> +			new_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);
> >> +	}
> >> +
> >> 	if (sort)
> >> 		sort_tree(bi);
> >> 
> >> @@ -318,12 +346,15 @@ int main(int argc, char *argv[])
> >> 			    outname, strerror(errno));
> >> 	}
> >> 
> >> +	if (new_magic && (bi->versionflags & VF_PLUGIN))
> >> +		out_magic = FDT_MAGIC_DTBO;
> >> +
> >> 	if (streq(outform, "dts")) {
> >> 		dt_to_source(outf, bi);
> >> 	} else if (streq(outform, "dtb")) {
> >> -		dt_to_blob(outf, bi, outversion);
> >> +		dt_to_blob(outf, bi, out_magic, outversion);
> >> 	} else if (streq(outform, "asm")) {
> >> -		dt_to_asm(outf, bi, outversion);
> >> +		dt_to_asm(outf, bi, out_magic, outversion);
> >> 	} else if (streq(outform, "null")) {
> >> 		/* do nothing */
> >> 	} else {
> >> diff --git a/dtc.h b/dtc.h
> >> index 32009bc..889b8f8 100644
> >> --- a/dtc.h
> >> +++ b/dtc.h
> >> @@ -55,6 +55,9 @@ extern int minsize;		/* Minimum blob size */
> >> extern int padsize;		/* Additional padding to blob */
> >> extern int alignsize;		/* Additional padding to blob accroding to the alignsize */
> >> 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 */
> >> +extern int new_magic;		/* use new FDT magic values for objects */
> >> 
> >> #define PHANDLE_LEGACY	0x1
> >> #define PHANDLE_EPAPR	0x2
> >> @@ -195,6 +198,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);
> >> @@ -202,6 +206,8 @@ void delete_property(struct property *prop);
> >> void add_child(struct node *parent, struct node *child);
> >> void delete_node_by_name(struct node *parent, char *name);
> >> void delete_node(struct node *node);
> >> +void append_to_property(struct node *node,
> >> +			char *name, const void *data, int len);
> >> 
> >> const char *get_unitname(struct node *node);
> >> struct property *get_property(struct node *node, const char *propname);
> >> @@ -237,14 +243,22 @@ 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/ */
> >> +#define VF_PLUGIN	0x0002	/* /plugin/ */
> >> +
> >> +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 */
> >> 
> >> @@ -253,8 +267,8 @@ void process_checks(bool force, struct boot_info *bi);
> >> 
> >> /* Flattened trees */
> >> 
> >> -void dt_to_blob(FILE *f, struct boot_info *bi, int version);
> >> -void dt_to_asm(FILE *f, struct boot_info *bi, int version);
> >> +void dt_to_blob(FILE *f, struct boot_info *bi, fdt32_t magic, int version);
> >> +void dt_to_asm(FILE *f, struct boot_info *bi, fdt32_t magic, int version);
> >> 
> >> struct boot_info *dt_from_blob(const char *fname);
> >> 
> >> diff --git a/fdtdump.c b/fdtdump.c
> >> index a9a2484..dd63ac2 100644
> >> --- a/fdtdump.c
> >> +++ b/fdtdump.c
> >> @@ -201,7 +201,7 @@ int main(int argc, char *argv[])
> >> 			p = memchr(p, smagic[0], endp - p - FDT_MAGIC_SIZE);
> >> 			if (!p)
> >> 				break;
> >> -			if (fdt_magic(p) == FDT_MAGIC) {
> >> +			if (fdt_magic(p) == FDT_MAGIC || fdt_magic(p) == FDT_MAGIC_DTBO) {
> >> 				/* try and validate the main struct */
> >> 				off_t this_len = endp - p;
> >> 				fdt32_t max_version = 17;
> >> diff --git a/flattree.c b/flattree.c
> >> index a9d9520..57d76cf 100644
> >> --- a/flattree.c
> >> +++ b/flattree.c
> >> @@ -335,6 +335,7 @@ static struct data flatten_reserve_list(struct reserve_info *reservelist,
> >> }
> >> 
> >> static void make_fdt_header(struct fdt_header *fdt,
> >> +			    fdt32_t magic,
> >> 			    struct version_info *vi,
> >> 			    int reservesize, int dtsize, int strsize,
> >> 			    int boot_cpuid_phys)
> >> @@ -345,7 +346,7 @@ static void make_fdt_header(struct fdt_header *fdt,
> >> 
> >> 	memset(fdt, 0xff, sizeof(*fdt));
> >> 
> >> -	fdt->magic = cpu_to_fdt32(FDT_MAGIC);
> >> +	fdt->magic = cpu_to_fdt32(magic);
> >> 	fdt->version = cpu_to_fdt32(vi->version);
> >> 	fdt->last_comp_version = cpu_to_fdt32(vi->last_comp_version);
> >> 
> >> @@ -366,7 +367,7 @@ static void make_fdt_header(struct fdt_header *fdt,
> >> 		fdt->size_dt_struct = cpu_to_fdt32(dtsize);
> >> }
> >> 
> >> -void dt_to_blob(FILE *f, struct boot_info *bi, int version)
> >> +void dt_to_blob(FILE *f, struct boot_info *bi, fdt32_t magic, int version)
> >> {
> >> 	struct version_info *vi = NULL;
> >> 	int i;
> >> @@ -390,7 +391,7 @@ void dt_to_blob(FILE *f, struct boot_info *bi, int version)
> >> 	reservebuf = flatten_reserve_list(bi->reservelist, vi);
> >> 
> >> 	/* Make header */
> >> -	make_fdt_header(&fdt, vi, reservebuf.len, dtbuf.len, strbuf.len,
> >> +	make_fdt_header(&fdt, magic, vi, reservebuf.len, dtbuf.len, strbuf.len,
> >> 			bi->boot_cpuid_phys);
> >> 
> >> 	/*
> >> @@ -467,7 +468,7 @@ static void dump_stringtable_asm(FILE *f, struct data strbuf)
> >> 	}
> >> }
> >> 
> >> -void dt_to_asm(FILE *f, struct boot_info *bi, int version)
> >> +void dt_to_asm(FILE *f, struct boot_info *bi, fdt32_t magic, int version)
> >> {
> >> 	struct version_info *vi = NULL;
> >> 	int i;
> >> @@ -830,6 +831,7 @@ struct boot_info *dt_from_blob(const char *fname)
> >> 	struct node *tree;
> >> 	uint32_t val;
> >> 	int flags = 0;
> >> +	unsigned int versionflags = VF_DT_V1;
> >> 
> >> 	f = srcfile_relative_open(fname, NULL);
> >> 
> >> @@ -845,9 +847,12 @@ struct boot_info *dt_from_blob(const char *fname)
> >> 	}
> >> 
> >> 	magic = fdt32_to_cpu(magic);
> >> -	if (magic != FDT_MAGIC)
> >> +	if (magic != FDT_MAGIC && magic != FDT_MAGIC_DTBO)
> >> 		die("Blob has incorrect magic number\n");
> >> 
> >> +	if (magic == FDT_MAGIC_DTBO)
> >> +		versionflags |= VF_PLUGIN;
> >> +
> >> 	rc = fread(&totalsize, sizeof(totalsize), 1, f);
> >> 	if (ferror(f))
> >> 		die("Error reading DT blob size: %s\n", strerror(errno));
> >> @@ -942,5 +947,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(versionflags, 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/libfdt/fdt.c b/libfdt/fdt.c
> >> index 22286a1..28d422c 100644
> >> --- a/libfdt/fdt.c
> >> +++ b/libfdt/fdt.c
> >> @@ -57,7 +57,7 @@
> >> 
> >> int fdt_check_header(const void *fdt)
> >> {
> >> -	if (fdt_magic(fdt) == FDT_MAGIC) {
> >> +	if (fdt_magic(fdt) == FDT_MAGIC || fdt_magic(fdt) == FDT_MAGIC_DTBO) {
> >> 		/* Complete tree */
> >> 		if (fdt_version(fdt) < FDT_FIRST_SUPPORTED_VERSION)
> >> 			return -FDT_ERR_BADVERSION;
> >> diff --git a/libfdt/fdt.h b/libfdt/fdt.h
> >> index 526aedb..493cd55 100644
> >> --- a/libfdt/fdt.h
> >> +++ b/libfdt/fdt.h
> >> @@ -55,7 +55,7 @@
> >> #ifndef __ASSEMBLY__
> >> 
> >> struct fdt_header {
> >> -	fdt32_t magic;			 /* magic word FDT_MAGIC */
> >> +	fdt32_t magic;			 /* magic word FDT_MAGIC[|_DTBO] */
> >> 	fdt32_t totalsize;		 /* total size of DT block */
> >> 	fdt32_t off_dt_struct;		 /* offset to structure */
> >> 	fdt32_t off_dt_strings;		 /* offset to strings */
> >> @@ -93,6 +93,7 @@ struct fdt_property {
> >> #endif /* !__ASSEMBLY */
> >> 
> >> #define FDT_MAGIC	0xd00dfeed	/* 4: version, 4: total size */
> >> +#define FDT_MAGIC_DTBO	0xd00dfdb0	/* DTBO magic */
> >> #define FDT_TAGSIZE	sizeof(fdt32_t)
> >> 
> >> #define FDT_BEGIN_NODE	0x1		/* Start node: full name */
> >> diff --git a/livetree.c b/livetree.c
> >> index 3dc7559..1a2f4b1 100644
> >> --- a/livetree.c
> >> +++ b/livetree.c
> >> @@ -216,6 +216,31 @@ 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));
> > 
> > You shouldn't use a bare malloc() for a node.  Use build_node() instead.
> > 
> 
> OK.
> 
> >> +	struct property *p;
> >> +	struct data d = empty_data;
> >> +	char *name;
> >> +
> >> +	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);
> >> +
> >> +	xasprintf(&name, "fragment@%u",
> >> +			next_orphan_fragment++);
> >> +	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);
> >> @@ -296,6 +321,23 @@ void delete_node(struct node *node)
> >> 	delete_labels(&node->labels);
> >> }
> >> 
> >> +void append_to_property(struct node *node,
> >> +				    char *name, const void *data, int len)
> >> +{
> >> +	struct data d;
> >> +	struct property *p;
> >> +
> >> +	p = get_property(node, name);
> >> +	if (p) {
> >> +		d = data_append_data(p->val, data, len);
> >> +		p->val = d;
> >> +	} else {
> >> +		d = data_append_data(empty_data, data, len);
> >> +		p = build_property(name, d);
> >> +		add_property(node, p);
> >> +	}
> >> +}
> >> +
> >> struct reserve_info *build_reserve_entry(uint64_t address, uint64_t size)
> >> {
> >> 	struct reserve_info *new = xmalloc(sizeof(*new));
> >> @@ -335,12 +377,14 @@ 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;
> >> @@ -709,3 +753,182 @@ 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,
> >> +					 struct node *an, bool allocph)
> >> +{
> >> +	struct node *c;
> >> +	struct property *p;
> >> +	struct label *l;
> >> +
> >> +	/* if if there are labels */
> >> +	if (node->labels) {
> >> +		/* 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,
> >> +					an->name);
> >> +				continue;
> >> +			}
> >> +
> >> +			/* insert it */
> >> +			p = build_property(l->label,
> >> +				data_copy_escape_string(node->fullpath,
> >> +						strlen(node->fullpath)));
> > 
> > Um.. what?  The node path should absolutely not be an escape string.
> > It shouldn't contain escapes to begin with, and if it does, we
> > absolutely shouldn't be turning them into special characters here.
> > 
> 
> OK, trivial enough to do. I tried to play it safe here but this is an overkill.

It's not overkill, it's Just Plain Wrong.  Resolving escapes is not
idempotent.  If you somehow had a node path with a backslash in it -
well, that would be bad to begin with - but trying to resolve that
backslash as an escape would be unambiguously worse.

> >> +			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, an, allocph);
> >> +}
> >> +
> >> +void generate_label_tree(struct node *dt, char *gen_node_name, bool allocph)
> >> +{
> >> +	struct node *an;
> >> +
> >> +	for_each_child(dt, an)
> >> +		if (streq(gen_node_name, an->name))
> >> +			break;
> >> +
> >> +	if (!an)
> >> +		an = build_and_name_child_node(dt, gen_node_name);
> >> +	if (!an)
> >> +		die("Could not build label node /%s\n", gen_node_name);
> >> +
> >> +	generate_label_tree_internal(dt, dt, an, allocph);
> >> +}
> >> +
> >> +static char *fixups_name = "__fixups__";
> >> +static char *local_fixups_name = "__local_fixups__";
> > 
> > I'd actually prefer #defines for these, and all-caps names, so it's
> > more obvious when they're used that these are global constants.
> > 
> 
> OK.
> 
> >> +
> >> +static void add_fixup_entry(struct node *dt, struct node *node,
> >> +		struct property *prop, struct marker *m)
> >> +{
> >> +	struct node *fn;	/* fixup node */
> >> +	char *entry;
> >> +
> >> +	/* m->ref can only be a REF_PHANDLE, but check anyway */
> >> +	assert(m->type == REF_PHANDLE);
> >> +
> >> +	/* fn is the node we're putting entries in */
> >> +	fn = get_subnode(dt, fixups_name);
> >> +	assert(fn != NULL);
> >> +
> >> +	/* there shouldn't be any ':' in the arguments */
> >> +	if (strchr(node->fullpath, ':') || strchr(prop->name, ':'))
> >> +		die("arguments should not contain ':'\n");
> >> +
> >> +	xasprintf(&entry, "%s:%s:%u",
> >> +			node->fullpath, prop->name, m->offset);
> >> +	append_to_property(fn, m->ref, entry, strlen(entry) + 1);
> >> +}
> >> +
> >> +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 */
> >> +	uint32_t value_32;
> >> +	char *s, *e, *comp;
> >> +	int len;
> >> +
> >> +	/* fn is the node we're putting entries in */
> >> +	lfn = get_subnode(dt, local_fixups_name);
> >> +	assert(lfn != NULL);
> >> +
> >> +	/* 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) {
> >> +		/* 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);
> >> +
> >> +	value_32 = cpu_to_fdt32(m->offset);
> >> +	append_to_property(wn, prop->name, &value_32, sizeof(value_32));
> >> +}
> >> +
> >> +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)
> >> +{
> >> +	struct node *an;
> >> +
> >> +	for_each_child(dt, an)
> >> +		if (streq(fixups_name, an->name))
> >> +			break;
> >> +
> >> +	if (!an)
> >> +		build_and_name_child_node(dt, fixups_name);
> >> +
> >> +	for_each_child(dt, an)
> >> +		if (streq(local_fixups_name, an->name))
> >> +			break;
> >> +
> >> +	if (!an)
> >> +		build_and_name_child_node(dt, local_fixups_name);
> >> +
> >> +	generate_fixups_tree_internal(dt, dt);
> >> +}
> >> diff --git a/tests/mangle-layout.c b/tests/mangle-layout.c
> >> index a76e51e..d29ebc6 100644
> >> --- a/tests/mangle-layout.c
> >> +++ b/tests/mangle-layout.c
> >> @@ -42,7 +42,8 @@ static void expand_buf(struct bufstate *buf, int newsize)
> >> 	buf->size = newsize;
> >> }
> >> 
> >> -static void new_header(struct bufstate *buf, int version, const void *fdt)
> >> +static void new_header(struct bufstate *buf, fdt32_t magic, int version,
> >> +		       const void *fdt)
> >> {
> >> 	int hdrsize;
> >> 
> >> @@ -56,7 +57,7 @@ static void new_header(struct bufstate *buf, int version, const void *fdt)
> >> 	expand_buf(buf, hdrsize);
> >> 	memset(buf->buf, 0, hdrsize);
> >> 
> >> -	fdt_set_magic(buf->buf, FDT_MAGIC);
> >> +	fdt_set_magic(buf->buf, magic);
> >> 	fdt_set_version(buf->buf, version);
> >> 	fdt_set_last_comp_version(buf->buf, 16);
> >> 	fdt_set_boot_cpuid_phys(buf->buf, fdt_boot_cpuid_phys(fdt));
> >> @@ -145,7 +146,7 @@ int main(int argc, char *argv[])
> >> 	if (fdt_version(fdt) < 17)
> >> 		CONFIG("Input tree must be v17");
> >> 
> >> -	new_header(&buf, version, fdt);
> >> +	new_header(&buf, FDT_MAGIC, version, fdt);
> >> 
> >> 	while (*blockorder) {
> >> 		add_block(&buf, version, *blockorder, fdt);
> >> diff --git a/treesource.c b/treesource.c
> >> index a55d1d1..75e920d 100644
> >> --- a/treesource.c
> >> +++ b/treesource.c
> >> @@ -267,7 +267,12 @@ void dt_to_source(FILE *f, struct boot_info *bi)
> >> {
> >> 	struct reserve_info *re;
> >> 
> >> -	fprintf(f, "/dts-v1/;\n\n");
> >> +	fprintf(f, "/dts-v1/");
> >> +
> >> +	if (bi->versionflags & VF_PLUGIN)
> >> +		fprintf(f, " /plugin/");
> >> +
> >> +	fprintf(f, ";\n\n");
> > 
> > I'm not sure this really makes sense.  The /plugin/ tag triggers the
> > fixup construction and encoding of overlay fragments.  But in an
> > incoming dtb, that processing has already been done once, doing it
> > again would not make sense.  So I think the output should not have the
> > /plugin/ tag, even if the input did.
> > 
> > Unless, of course, you parsed the input into an explicit list of
> > overlays, then output it here again as a list of overlays, not the
> > encoded fragments.  i.e. if this was the original tree:
> > 
> > 	/dts-v1/ /plugin/;
> > 
> > 	&somwhere {
> > 		name = "value";
> > 	};
> > 
> > then legitimately the output of -I dts -O dts could be either:
> > 
> > 	/dts-v1/ /plugin/;
> > 
> > 	&somwhere {
> > 		name = "value";
> > 	};
> > 
> > OR
> > 
> > 	/dts-v1/;
> > 
> > 	/ {
> > 		fragment@0 {
> > 			target = <0xffffffff>;
> > 			__overlay__{
> > 				name = "value";
> > 			};
> > 		};
> > 		__fixups__ {
> > 			somewhere = "/fragment@0:target:0";
> > 		};
> > 	};
> > 
> > But it doesn't make sense to combine the two: the second structure
> > with the "/plugin/" tag.
> > 
> 
> > Another advantage of moving the overlay application out of the parser
> > is you can sanely produce either output, both of which could be useful
> > in the right circumstances.  You can potentially even produce the
> > first output (or something close) from dtb input, with correct parsing
> > of the overlay structure on dtb input.
> > 
> 
> Yes, this makes sense. For now I’ll remove the plugin tag, but if the blob
> was compiled with -@ you could conceivably reverse back to a form that contains
> labels and phandle references correctly.
> 
> But this is quite complicated to undertake in this patch series. 
> 
> To re-iterate though there is no overlay application here :)

Well, I think we've both been a bit sloppy with terminology - does
"overlay" refer to a dtbo file, or to one of the &ref { ... }
fragments in the source, or to both.

But whatever the right term is for the &ref { .. } fragments in the
source they absolutely *are* applied during compile for the base tree
case.  And while they're not resolved fully, they are encoded into
part of a larger tree structure for the plugin case.

The confusion will be reduced if we make these
overlays/fragments/whatever first class objects in the "live tree"
phase of the compile, and move the resolution and/or assembly of them
a stage that's separate from the parse.  In addition, it opens the way
for decompiling a dtbo more naturally, though as you say that's a
moderate amount of extra work.

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

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

[Pantelis Antoniou]

Hi David,

> On Nov 25, 2016, at 13:26 , David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> 
> On Fri, Nov 25, 2016 at 12:55:25PM +0200, Pantelis Antoniou wrote:
>> Hi David,
>> 
>>> On Nov 25, 2016, at 06:11 , David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>>> 
>>> On Thu, Nov 24, 2016 at 02:31:32PM +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 disabled. This is in order to give time for DT blob
>>>> methods to be updated.
>>>> 
>>>> 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             |  49 +++++++++--
>>>> 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 +-
>>>> treesource.c             |   7 +-
>>>> 14 files changed, 380 insertions(+), 31 deletions(-)
>>>> 
>>>> diff --git a/Documentation/manual.txt b/Documentation/manual.txt
>>>> index 398de32..65fbf09 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 new FDT magic number. By default blobs with the
>>>> +	standard FDT magic number are generated.
>>> 
>>> First, this description is incomplete since -M *only* affects the
>>> magic number for /plugin/ input, not in other cases.  Second, the
>>> default is the wrong way around.  If we make old-style the default,
>>> then new-style will never be used, which defeats the purpose.
>> 
>> Then we’ll break user-space that has this assumption (i.e. that the magic is the same).
>> I can certainly do it the other way around.
> 
> Which userspace in particular?
> 

Kernel unflatteners in various OSes/bootloaders? All those would have to be updated since
they don’t grok the new magic number.

>>>> +
>>>>    -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 609975a..bc03d42 100644
>>>> --- a/checks.c
>>>> +++ b/checks.c
>>>> @@ -486,8 +486,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..4afc592 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,9 @@ extern void yyerror(char const *s);
>>>> 
>>>> extern struct boot_info *the_boot_info;
>>>> extern bool treesource_error;
>>>> +
>>>> +/* temporary while the tree is not built */
>>>> +static unsigned int the_versionflags;
>>> 
>>> Hrm.  Using a global during parsing is pretty dangerous - it makes
>>> assumptions about the order in which bison will execute semantic
>>> actions that may not always be correct.
>>> 
>>> It'll probably work in practice, so I *might* be convinced it's
>>> adequate for a first cut.  I'm a bit reluctant though, since I suspect
>>> once merged it will become entrenched.
>>> 
>> 
>> We use bison, globals are the way of life. It’s not going to be used
>> anywhere else, it’s static in the parser file.
> 
> Using globals to communicate the final result is inevitable (well, not
> without using the whole different re-entrant interface stuff).  That
> just assumes that the start symbol's semantic action is executed
> before the parser competes, which is guaranteed.
> 
> This is a different matter - using a global to communicate between
> different parts of the parser.  More specifically different parts of
> the parser that are in different arms of the parse tree.  That makes
> assumptions about the relative order of semantic actions which are
> *not* guaranteed.  In theory the parser could generate the entire
> parse tree and semantic action dependency graph, then execute them in
> an arbitrary order (well, it would have to be a topologically sorted
> order, but that won't help).
> 

I’ve given it a little bit more thought and it’s easily fixed by using
inherited attributes which is safe to do and avoid the global all together.

>> We could allocate the boot_info earlier (when the v1tag is detected) but
>> that would be quite a big change for something as trivial. 
> 
> That wouldn't help.  You still wouldn't have a guarantee on the order
> between setting the version flags and using the version flags.
> 
>>> The correct way to handle this, IMO, is not to ever attempt to apply
>>> overlays during the parse.  Basically, we'd change the overall
>>> structure so that the output from the parser is not a single tree, but
>>> a list of overlays / fragments.  Then, once the parse is complete, so
>>> versioninfo (which could now become a member of bootinfo) is well
>>> defined, we either apply the fragments in place (base tree) or encode
>>> them into the overlay structure (plugin mode).
>>> 
>>> See https://github.com/dgibson/dtc/tree/overlay for some incomplete
>>> work I did in this direction.
>>> 
>> 
>> This tree is pretty stale; last commit was back in march.
> 
> Yes, it was a while since I worked on it.  It rebased clean, though.
> 
>> I thing there’s a wrong assumption here. The purpose of this patch is not
>> to apply overlays during compile time, is to generate a blob that can be
>> applied at runtime by another piece of software.
> 
> No, I realise what you're doing.  But the input is in the form of a
> batch of overlays, regardless.  You then have two modes of operation:
> for base trees you resolve those overlays during the compile, for
> plugins you assemble those overlays into a blob which can be applied
> later.
> 
> Because it wasn't designed with runtime overlays in mind, the current
> code handles the resolution of those overlays during the parse.  Your
> patch extends that by rather than resolving them, just putting them
> together with metadata into the dtbo.
> 
> I'm saying that resolution or assembly should be moved out of the
> parser.  Instead, the parser output would be an (ordered) list of
> overlay fragments.  In the main program the two modes of operation
> become explicit: for base trees we resolve the overlays into a single
> tree, for plugins we collate the pieces into the dtbo format.
> 

The case for building an overlay is only for the syntactic sugar version.

This is not the common use case at all. I could easily remove it and then
there would be no overlays built at all in the parser.

In fact the extra fixup nodes are generated now after the parse stage,
after the check stage and before the sort stage.

Perhaps it should be separated out so that we don’t get sidetracked?

>>> In addition to not making unsafe assumptions about parser operation, I
>>> think this will also allow for better error reporting.  It also moves
>>> more code into plain-old-C instead of potentially hard to follow bison
>>> code fragments.
>>> 
>> 
>> We don’t try to apply overlays during parse, and especially in the parse code.
> 
> The existing code does this, and you don't change it.  Furthermore you
> absolutely do do the assembly of the fragments within the parser -
> that's exactly what add_orphan_node() called directly from semantic
> actions does.  To verify this, all you need to do is look at the
> parser output: the boot_info structure has just a single tree, not a
> list of overlays.
> 

Only for the un-common case.

>> The global is used only for the syntactic sugar method of turning a &ref { };
>> node into an overlay.
> 
> I'm saying that treating that as mere syntactic sugar is a fundamental
> error in design.  We could get away with it when the &ref {} stuff was
> always resolved immediately.  Now that that resolution can be delayed
> until later, it gets worse.  We should move that resolution outside of
> the parser.
> 

The &ref { } stuff is sidetracking us. This is not the core issue.
In fact out in the community there are not a lot of cases where it is used.

>> 
>>>> %}
>>>> 
>>>> %union {
>>>> @@ -52,9 +56,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 +77,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 +109,36 @@ 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));
>>>> +			the_versionflags |= VF_DT_V1;
>>>> +			$$ = the_versionflags;
>>>> 		}
>>>> 	;
>>>> 
>>>> v1tag:
>>>> 	  DT_V1 ';'
>>>> +	| DT_V1
>>>> 	| DT_V1 ';' v1tag
>>>> +
>>>> +plugindecl:
>>>> +	DT_PLUGIN ';'
>>>> +		{
>>>> +			the_versionflags |= VF_PLUGIN;
>>>> +			$$ = VF_PLUGIN;
>>>> +		}
>>>> +	| /* empty */
>>>> +		{
>>>> +			$$ = 0;
>>>> +		}
>>>> 	;
>>>> 
>>>> memreserves:
>>>> @@ -161,10 +189,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 (the_versionflags & VF_PLUGIN)
>>>> +					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 +211,11 @@ devicetree:
>>>> 
>>>> 			$$ = $1;
>>>> 		}
>>>> +	| /* empty */
>>>> +		{
>>>> +			/* build empty node */
>>>> +			$$ = name_node(build_node(NULL, NULL), "");
>>>> +		}
>>>> 	;
>>>> 
>>>> nodedef:
>>>> diff --git a/dtc.c b/dtc.c
>>>> index 9dcf640..a654267 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 new_magic;			/* use new 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'},
>>>> +	{"new-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\tUse new blog magic value",
>>>> 	"\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':
>>>> +			new_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);
>>>> +	}
>>>> +
>>>> 	if (sort)
>>>> 		sort_tree(bi);
>>>> 
>>>> @@ -318,12 +346,15 @@ int main(int argc, char *argv[])
>>>> 			    outname, strerror(errno));
>>>> 	}
>>>> 
>>>> +	if (new_magic && (bi->versionflags & VF_PLUGIN))
>>>> +		out_magic = FDT_MAGIC_DTBO;
>>>> +
>>>> 	if (streq(outform, "dts")) {
>>>> 		dt_to_source(outf, bi);
>>>> 	} else if (streq(outform, "dtb")) {
>>>> -		dt_to_blob(outf, bi, outversion);
>>>> +		dt_to_blob(outf, bi, out_magic, outversion);
>>>> 	} else if (streq(outform, "asm")) {
>>>> -		dt_to_asm(outf, bi, outversion);
>>>> +		dt_to_asm(outf, bi, out_magic, outversion);
>>>> 	} else if (streq(outform, "null")) {
>>>> 		/* do nothing */
>>>> 	} else {
>>>> diff --git a/dtc.h b/dtc.h
>>>> index 32009bc..889b8f8 100644
>>>> --- a/dtc.h
>>>> +++ b/dtc.h
>>>> @@ -55,6 +55,9 @@ extern int minsize;		/* Minimum blob size */
>>>> extern int padsize;		/* Additional padding to blob */
>>>> extern int alignsize;		/* Additional padding to blob accroding to the alignsize */
>>>> 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 */
>>>> +extern int new_magic;		/* use new FDT magic values for objects */
>>>> 
>>>> #define PHANDLE_LEGACY	0x1
>>>> #define PHANDLE_EPAPR	0x2
>>>> @@ -195,6 +198,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);
>>>> @@ -202,6 +206,8 @@ void delete_property(struct property *prop);
>>>> void add_child(struct node *parent, struct node *child);
>>>> void delete_node_by_name(struct node *parent, char *name);
>>>> void delete_node(struct node *node);
>>>> +void append_to_property(struct node *node,
>>>> +			char *name, const void *data, int len);
>>>> 
>>>> const char *get_unitname(struct node *node);
>>>> struct property *get_property(struct node *node, const char *propname);
>>>> @@ -237,14 +243,22 @@ 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/ */
>>>> +#define VF_PLUGIN	0x0002	/* /plugin/ */
>>>> +
>>>> +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 */
>>>> 
>>>> @@ -253,8 +267,8 @@ void process_checks(bool force, struct boot_info *bi);
>>>> 
>>>> /* Flattened trees */
>>>> 
>>>> -void dt_to_blob(FILE *f, struct boot_info *bi, int version);
>>>> -void dt_to_asm(FILE *f, struct boot_info *bi, int version);
>>>> +void dt_to_blob(FILE *f, struct boot_info *bi, fdt32_t magic, int version);
>>>> +void dt_to_asm(FILE *f, struct boot_info *bi, fdt32_t magic, int version);
>>>> 
>>>> struct boot_info *dt_from_blob(const char *fname);
>>>> 
>>>> diff --git a/fdtdump.c b/fdtdump.c
>>>> index a9a2484..dd63ac2 100644
>>>> --- a/fdtdump.c
>>>> +++ b/fdtdump.c
>>>> @@ -201,7 +201,7 @@ int main(int argc, char *argv[])
>>>> 			p = memchr(p, smagic[0], endp - p - FDT_MAGIC_SIZE);
>>>> 			if (!p)
>>>> 				break;
>>>> -			if (fdt_magic(p) == FDT_MAGIC) {
>>>> +			if (fdt_magic(p) == FDT_MAGIC || fdt_magic(p) == FDT_MAGIC_DTBO) {
>>>> 				/* try and validate the main struct */
>>>> 				off_t this_len = endp - p;
>>>> 				fdt32_t max_version = 17;
>>>> diff --git a/flattree.c b/flattree.c
>>>> index a9d9520..57d76cf 100644
>>>> --- a/flattree.c
>>>> +++ b/flattree.c
>>>> @@ -335,6 +335,7 @@ static struct data flatten_reserve_list(struct reserve_info *reservelist,
>>>> }
>>>> 
>>>> static void make_fdt_header(struct fdt_header *fdt,
>>>> +			    fdt32_t magic,
>>>> 			    struct version_info *vi,
>>>> 			    int reservesize, int dtsize, int strsize,
>>>> 			    int boot_cpuid_phys)
>>>> @@ -345,7 +346,7 @@ static void make_fdt_header(struct fdt_header *fdt,
>>>> 
>>>> 	memset(fdt, 0xff, sizeof(*fdt));
>>>> 
>>>> -	fdt->magic = cpu_to_fdt32(FDT_MAGIC);
>>>> +	fdt->magic = cpu_to_fdt32(magic);
>>>> 	fdt->version = cpu_to_fdt32(vi->version);
>>>> 	fdt->last_comp_version = cpu_to_fdt32(vi->last_comp_version);
>>>> 
>>>> @@ -366,7 +367,7 @@ static void make_fdt_header(struct fdt_header *fdt,
>>>> 		fdt->size_dt_struct = cpu_to_fdt32(dtsize);
>>>> }
>>>> 
>>>> -void dt_to_blob(FILE *f, struct boot_info *bi, int version)
>>>> +void dt_to_blob(FILE *f, struct boot_info *bi, fdt32_t magic, int version)
>>>> {
>>>> 	struct version_info *vi = NULL;
>>>> 	int i;
>>>> @@ -390,7 +391,7 @@ void dt_to_blob(FILE *f, struct boot_info *bi, int version)
>>>> 	reservebuf = flatten_reserve_list(bi->reservelist, vi);
>>>> 
>>>> 	/* Make header */
>>>> -	make_fdt_header(&fdt, vi, reservebuf.len, dtbuf.len, strbuf.len,
>>>> +	make_fdt_header(&fdt, magic, vi, reservebuf.len, dtbuf.len, strbuf.len,
>>>> 			bi->boot_cpuid_phys);
>>>> 
>>>> 	/*
>>>> @@ -467,7 +468,7 @@ static void dump_stringtable_asm(FILE *f, struct data strbuf)
>>>> 	}
>>>> }
>>>> 
>>>> -void dt_to_asm(FILE *f, struct boot_info *bi, int version)
>>>> +void dt_to_asm(FILE *f, struct boot_info *bi, fdt32_t magic, int version)
>>>> {
>>>> 	struct version_info *vi = NULL;
>>>> 	int i;
>>>> @@ -830,6 +831,7 @@ struct boot_info *dt_from_blob(const char *fname)
>>>> 	struct node *tree;
>>>> 	uint32_t val;
>>>> 	int flags = 0;
>>>> +	unsigned int versionflags = VF_DT_V1;
>>>> 
>>>> 	f = srcfile_relative_open(fname, NULL);
>>>> 
>>>> @@ -845,9 +847,12 @@ struct boot_info *dt_from_blob(const char *fname)
>>>> 	}
>>>> 
>>>> 	magic = fdt32_to_cpu(magic);
>>>> -	if (magic != FDT_MAGIC)
>>>> +	if (magic != FDT_MAGIC && magic != FDT_MAGIC_DTBO)
>>>> 		die("Blob has incorrect magic number\n");
>>>> 
>>>> +	if (magic == FDT_MAGIC_DTBO)
>>>> +		versionflags |= VF_PLUGIN;
>>>> +
>>>> 	rc = fread(&totalsize, sizeof(totalsize), 1, f);
>>>> 	if (ferror(f))
>>>> 		die("Error reading DT blob size: %s\n", strerror(errno));
>>>> @@ -942,5 +947,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(versionflags, 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/libfdt/fdt.c b/libfdt/fdt.c
>>>> index 22286a1..28d422c 100644
>>>> --- a/libfdt/fdt.c
>>>> +++ b/libfdt/fdt.c
>>>> @@ -57,7 +57,7 @@
>>>> 
>>>> int fdt_check_header(const void *fdt)
>>>> {
>>>> -	if (fdt_magic(fdt) == FDT_MAGIC) {
>>>> +	if (fdt_magic(fdt) == FDT_MAGIC || fdt_magic(fdt) == FDT_MAGIC_DTBO) {
>>>> 		/* Complete tree */
>>>> 		if (fdt_version(fdt) < FDT_FIRST_SUPPORTED_VERSION)
>>>> 			return -FDT_ERR_BADVERSION;
>>>> diff --git a/libfdt/fdt.h b/libfdt/fdt.h
>>>> index 526aedb..493cd55 100644
>>>> --- a/libfdt/fdt.h
>>>> +++ b/libfdt/fdt.h
>>>> @@ -55,7 +55,7 @@
>>>> #ifndef __ASSEMBLY__
>>>> 
>>>> struct fdt_header {
>>>> -	fdt32_t magic;			 /* magic word FDT_MAGIC */
>>>> +	fdt32_t magic;			 /* magic word FDT_MAGIC[|_DTBO] */
>>>> 	fdt32_t totalsize;		 /* total size of DT block */
>>>> 	fdt32_t off_dt_struct;		 /* offset to structure */
>>>> 	fdt32_t off_dt_strings;		 /* offset to strings */
>>>> @@ -93,6 +93,7 @@ struct fdt_property {
>>>> #endif /* !__ASSEMBLY */
>>>> 
>>>> #define FDT_MAGIC	0xd00dfeed	/* 4: version, 4: total size */
>>>> +#define FDT_MAGIC_DTBO	0xd00dfdb0	/* DTBO magic */
>>>> #define FDT_TAGSIZE	sizeof(fdt32_t)
>>>> 
>>>> #define FDT_BEGIN_NODE	0x1		/* Start node: full name */
>>>> diff --git a/livetree.c b/livetree.c
>>>> index 3dc7559..1a2f4b1 100644
>>>> --- a/livetree.c
>>>> +++ b/livetree.c
>>>> @@ -216,6 +216,31 @@ 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));
>>> 
>>> You shouldn't use a bare malloc() for a node.  Use build_node() instead.
>>> 
>> 
>> OK.
>> 
>>>> +	struct property *p;
>>>> +	struct data d = empty_data;
>>>> +	char *name;
>>>> +
>>>> +	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);
>>>> +
>>>> +	xasprintf(&name, "fragment@%u",
>>>> +			next_orphan_fragment++);
>>>> +	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);
>>>> @@ -296,6 +321,23 @@ void delete_node(struct node *node)
>>>> 	delete_labels(&node->labels);
>>>> }
>>>> 
>>>> +void append_to_property(struct node *node,
>>>> +				    char *name, const void *data, int len)
>>>> +{
>>>> +	struct data d;
>>>> +	struct property *p;
>>>> +
>>>> +	p = get_property(node, name);
>>>> +	if (p) {
>>>> +		d = data_append_data(p->val, data, len);
>>>> +		p->val = d;
>>>> +	} else {
>>>> +		d = data_append_data(empty_data, data, len);
>>>> +		p = build_property(name, d);
>>>> +		add_property(node, p);
>>>> +	}
>>>> +}
>>>> +
>>>> struct reserve_info *build_reserve_entry(uint64_t address, uint64_t size)
>>>> {
>>>> 	struct reserve_info *new = xmalloc(sizeof(*new));
>>>> @@ -335,12 +377,14 @@ 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;
>>>> @@ -709,3 +753,182 @@ 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,
>>>> +					 struct node *an, bool allocph)
>>>> +{
>>>> +	struct node *c;
>>>> +	struct property *p;
>>>> +	struct label *l;
>>>> +
>>>> +	/* if if there are labels */
>>>> +	if (node->labels) {
>>>> +		/* 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,
>>>> +					an->name);
>>>> +				continue;
>>>> +			}
>>>> +
>>>> +			/* insert it */
>>>> +			p = build_property(l->label,
>>>> +				data_copy_escape_string(node->fullpath,
>>>> +						strlen(node->fullpath)));
>>> 
>>> Um.. what?  The node path should absolutely not be an escape string.
>>> It shouldn't contain escapes to begin with, and if it does, we
>>> absolutely shouldn't be turning them into special characters here.
>>> 
>> 
>> OK, trivial enough to do. I tried to play it safe here but this is an overkill.
> 
> It's not overkill, it's Just Plain Wrong.  Resolving escapes is not
> idempotent.  If you somehow had a node path with a backslash in it -
> well, that would be bad to begin with - but trying to resolve that
> backslash as an escape would be unambiguously worse.
> 
>>>> +			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, an, allocph);
>>>> +}
>>>> +
>>>> +void generate_label_tree(struct node *dt, char *gen_node_name, bool allocph)
>>>> +{
>>>> +	struct node *an;
>>>> +
>>>> +	for_each_child(dt, an)
>>>> +		if (streq(gen_node_name, an->name))
>>>> +			break;
>>>> +
>>>> +	if (!an)
>>>> +		an = build_and_name_child_node(dt, gen_node_name);
>>>> +	if (!an)
>>>> +		die("Could not build label node /%s\n", gen_node_name);
>>>> +
>>>> +	generate_label_tree_internal(dt, dt, an, allocph);
>>>> +}
>>>> +
>>>> +static char *fixups_name = "__fixups__";
>>>> +static char *local_fixups_name = "__local_fixups__";
>>> 
>>> I'd actually prefer #defines for these, and all-caps names, so it's
>>> more obvious when they're used that these are global constants.
>>> 
>> 
>> OK.
>> 
>>>> +
>>>> +static void add_fixup_entry(struct node *dt, struct node *node,
>>>> +		struct property *prop, struct marker *m)
>>>> +{
>>>> +	struct node *fn;	/* fixup node */
>>>> +	char *entry;
>>>> +
>>>> +	/* m->ref can only be a REF_PHANDLE, but check anyway */
>>>> +	assert(m->type == REF_PHANDLE);
>>>> +
>>>> +	/* fn is the node we're putting entries in */
>>>> +	fn = get_subnode(dt, fixups_name);
>>>> +	assert(fn != NULL);
>>>> +
>>>> +	/* there shouldn't be any ':' in the arguments */
>>>> +	if (strchr(node->fullpath, ':') || strchr(prop->name, ':'))
>>>> +		die("arguments should not contain ':'\n");
>>>> +
>>>> +	xasprintf(&entry, "%s:%s:%u",
>>>> +			node->fullpath, prop->name, m->offset);
>>>> +	append_to_property(fn, m->ref, entry, strlen(entry) + 1);
>>>> +}
>>>> +
>>>> +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 */
>>>> +	uint32_t value_32;
>>>> +	char *s, *e, *comp;
>>>> +	int len;
>>>> +
>>>> +	/* fn is the node we're putting entries in */
>>>> +	lfn = get_subnode(dt, local_fixups_name);
>>>> +	assert(lfn != NULL);
>>>> +
>>>> +	/* 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) {
>>>> +		/* 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);
>>>> +
>>>> +	value_32 = cpu_to_fdt32(m->offset);
>>>> +	append_to_property(wn, prop->name, &value_32, sizeof(value_32));
>>>> +}
>>>> +
>>>> +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)
>>>> +{
>>>> +	struct node *an;
>>>> +
>>>> +	for_each_child(dt, an)
>>>> +		if (streq(fixups_name, an->name))
>>>> +			break;
>>>> +
>>>> +	if (!an)
>>>> +		build_and_name_child_node(dt, fixups_name);
>>>> +
>>>> +	for_each_child(dt, an)
>>>> +		if (streq(local_fixups_name, an->name))
>>>> +			break;
>>>> +
>>>> +	if (!an)
>>>> +		build_and_name_child_node(dt, local_fixups_name);
>>>> +
>>>> +	generate_fixups_tree_internal(dt, dt);
>>>> +}
>>>> diff --git a/tests/mangle-layout.c b/tests/mangle-layout.c
>>>> index a76e51e..d29ebc6 100644
>>>> --- a/tests/mangle-layout.c
>>>> +++ b/tests/mangle-layout.c
>>>> @@ -42,7 +42,8 @@ static void expand_buf(struct bufstate *buf, int newsize)
>>>> 	buf->size = newsize;
>>>> }
>>>> 
>>>> -static void new_header(struct bufstate *buf, int version, const void *fdt)
>>>> +static void new_header(struct bufstate *buf, fdt32_t magic, int version,
>>>> +		       const void *fdt)
>>>> {
>>>> 	int hdrsize;
>>>> 
>>>> @@ -56,7 +57,7 @@ static void new_header(struct bufstate *buf, int version, const void *fdt)
>>>> 	expand_buf(buf, hdrsize);
>>>> 	memset(buf->buf, 0, hdrsize);
>>>> 
>>>> -	fdt_set_magic(buf->buf, FDT_MAGIC);
>>>> +	fdt_set_magic(buf->buf, magic);
>>>> 	fdt_set_version(buf->buf, version);
>>>> 	fdt_set_last_comp_version(buf->buf, 16);
>>>> 	fdt_set_boot_cpuid_phys(buf->buf, fdt_boot_cpuid_phys(fdt));
>>>> @@ -145,7 +146,7 @@ int main(int argc, char *argv[])
>>>> 	if (fdt_version(fdt) < 17)
>>>> 		CONFIG("Input tree must be v17");
>>>> 
>>>> -	new_header(&buf, version, fdt);
>>>> +	new_header(&buf, FDT_MAGIC, version, fdt);
>>>> 
>>>> 	while (*blockorder) {
>>>> 		add_block(&buf, version, *blockorder, fdt);
>>>> diff --git a/treesource.c b/treesource.c
>>>> index a55d1d1..75e920d 100644
>>>> --- a/treesource.c
>>>> +++ b/treesource.c
>>>> @@ -267,7 +267,12 @@ void dt_to_source(FILE *f, struct boot_info *bi)
>>>> {
>>>> 	struct reserve_info *re;
>>>> 
>>>> -	fprintf(f, "/dts-v1/;\n\n");
>>>> +	fprintf(f, "/dts-v1/");
>>>> +
>>>> +	if (bi->versionflags & VF_PLUGIN)
>>>> +		fprintf(f, " /plugin/");
>>>> +
>>>> +	fprintf(f, ";\n\n");
>>> 
>>> I'm not sure this really makes sense.  The /plugin/ tag triggers the
>>> fixup construction and encoding of overlay fragments.  But in an
>>> incoming dtb, that processing has already been done once, doing it
>>> again would not make sense.  So I think the output should not have the
>>> /plugin/ tag, even if the input did.
>>> 
>>> Unless, of course, you parsed the input into an explicit list of
>>> overlays, then output it here again as a list of overlays, not the
>>> encoded fragments.  i.e. if this was the original tree:
>>> 
>>> 	/dts-v1/ /plugin/;
>>> 
>>> 	&somwhere {
>>> 		name = "value";
>>> 	};
>>> 
>>> then legitimately the output of -I dts -O dts could be either:
>>> 
>>> 	/dts-v1/ /plugin/;
>>> 
>>> 	&somwhere {
>>> 		name = "value";
>>> 	};
>>> 
>>> OR
>>> 
>>> 	/dts-v1/;
>>> 
>>> 	/ {
>>> 		fragment@0 {
>>> 			target = <0xffffffff>;
>>> 			__overlay__{
>>> 				name = "value";
>>> 			};
>>> 		};
>>> 		__fixups__ {
>>> 			somewhere = "/fragment@0:target:0";
>>> 		};
>>> 	};
>>> 
>>> But it doesn't make sense to combine the two: the second structure
>>> with the "/plugin/" tag.
>>> 
>> 
>>> Another advantage of moving the overlay application out of the parser
>>> is you can sanely produce either output, both of which could be useful
>>> in the right circumstances.  You can potentially even produce the
>>> first output (or something close) from dtb input, with correct parsing
>>> of the overlay structure on dtb input.
>>> 
>> 
>> Yes, this makes sense. For now I’ll remove the plugin tag, but if the blob
>> was compiled with -@ you could conceivably reverse back to a form that contains
>> labels and phandle references correctly.
>> 
>> But this is quite complicated to undertake in this patch series. 
>> 
>> To re-iterate though there is no overlay application here :)
> 
> Well, I think we've both been a bit sloppy with terminology - does
> "overlay" refer to a dtbo file, or to one of the &ref { ... }
> fragments in the source, or to both.
> 
> But whatever the right term is for the &ref { .. } fragments in the
> source they absolutely *are* applied during compile for the base tree
> case.  And while they're not resolved fully, they are encoded into
> part of a larger tree structure for the plugin case.
> 
> The confusion will be reduced if we make these
> overlays/fragments/whatever first class objects in the "live tree"
> phase of the compile, and move the resolution and/or assembly of them
> a stage that's separate from the parse.  In addition, it opens the way
> for decompiling a dtbo more naturally, though as you say that's a
> moderate amount of extra work.
> 

How about we get the non &ref { } case sorted out and we can talk about
the syntactic sugar version done? 

v10 has been sent out.

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

Regards

— Pantelis

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

[David Gibson]

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

On Fri, Nov 25, 2016 at 02:44:39PM +0200, Pantelis Antoniou wrote:
> Hi David,
> 
> > On Nov 25, 2016, at 13:26 , David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > 
> > On Fri, Nov 25, 2016 at 12:55:25PM +0200, Pantelis Antoniou wrote:
> >> Hi David,
> >> 
> >>> On Nov 25, 2016, at 06:11 , David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> >>> 
> >>> On Thu, Nov 24, 2016 at 02:31:32PM +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 disabled. This is in order to give time for DT blob
> >>>> methods to be updated.
> >>>> 
> >>>> 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             |  49 +++++++++--
> >>>> 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 +-
> >>>> treesource.c             |   7 +-
> >>>> 14 files changed, 380 insertions(+), 31 deletions(-)
> >>>> 
> >>>> diff --git a/Documentation/manual.txt b/Documentation/manual.txt
> >>>> index 398de32..65fbf09 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 new FDT magic number. By default blobs with the
> >>>> +	standard FDT magic number are generated.
> >>> 
> >>> First, this description is incomplete since -M *only* affects the
> >>> magic number for /plugin/ input, not in other cases.  Second, the
> >>> default is the wrong way around.  If we make old-style the default,
> >>> then new-style will never be used, which defeats the purpose.
> >> 
> >> Then we’ll break user-space that has this assumption (i.e. that the magic is the same).
> >> I can certainly do it the other way around.
> > 
> > Which userspace in particular?
> > 
> 
> Kernel unflatteners in various OSes/bootloaders? All those would have to be updated since
> they don’t grok the new magic number.

Those will certainly need dtbs with the old magic number as input, but
I don't see how that affects the dtc default.  Using the option
explicitly if you're targetting a bootloader that hasn't been updated
seems reasonable.

> >>>> +
> >>>>    -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 609975a..bc03d42 100644
> >>>> --- a/checks.c
> >>>> +++ b/checks.c
> >>>> @@ -486,8 +486,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..4afc592 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,9 @@ extern void yyerror(char const *s);
> >>>> 
> >>>> extern struct boot_info *the_boot_info;
> >>>> extern bool treesource_error;
> >>>> +
> >>>> +/* temporary while the tree is not built */
> >>>> +static unsigned int the_versionflags;
> >>> 
> >>> Hrm.  Using a global during parsing is pretty dangerous - it makes
> >>> assumptions about the order in which bison will execute semantic
> >>> actions that may not always be correct.
> >>> 
> >>> It'll probably work in practice, so I *might* be convinced it's
> >>> adequate for a first cut.  I'm a bit reluctant though, since I suspect
> >>> once merged it will become entrenched.
> >>> 
> >> 
> >> We use bison, globals are the way of life. It’s not going to be used
> >> anywhere else, it’s static in the parser file.
> > 
> > Using globals to communicate the final result is inevitable (well, not
> > without using the whole different re-entrant interface stuff).  That
> > just assumes that the start symbol's semantic action is executed
> > before the parser competes, which is guaranteed.
> > 
> > This is a different matter - using a global to communicate between
> > different parts of the parser.  More specifically different parts of
> > the parser that are in different arms of the parse tree.  That makes
> > assumptions about the relative order of semantic actions which are
> > *not* guaranteed.  In theory the parser could generate the entire
> > parse tree and semantic action dependency graph, then execute them in
> > an arbitrary order (well, it would have to be a topologically sorted
> > order, but that won't help).
> > 
> 
> I’ve given it a little bit more thought and it’s easily fixed by using
> inherited attributes which is safe to do and avoid the global all together.

Hmm.. we'll see.

> >> We could allocate the boot_info earlier (when the v1tag is detected) but
> >> that would be quite a big change for something as trivial. 
> > 
> > That wouldn't help.  You still wouldn't have a guarantee on the order
> > between setting the version flags and using the version flags.
> > 
> >>> The correct way to handle this, IMO, is not to ever attempt to apply
> >>> overlays during the parse.  Basically, we'd change the overall
> >>> structure so that the output from the parser is not a single tree, but
> >>> a list of overlays / fragments.  Then, once the parse is complete, so
> >>> versioninfo (which could now become a member of bootinfo) is well
> >>> defined, we either apply the fragments in place (base tree) or encode
> >>> them into the overlay structure (plugin mode).
> >>> 
> >>> See https://github.com/dgibson/dtc/tree/overlay for some incomplete
> >>> work I did in this direction.
> >>> 
> >> 
> >> This tree is pretty stale; last commit was back in march.
> > 
> > Yes, it was a while since I worked on it.  It rebased clean, though.
> > 
> >> I thing there’s a wrong assumption here. The purpose of this patch is not
> >> to apply overlays during compile time, is to generate a blob that can be
> >> applied at runtime by another piece of software.
> > 
> > No, I realise what you're doing.  But the input is in the form of a
> > batch of overlays, regardless.  You then have two modes of operation:
> > for base trees you resolve those overlays during the compile, for
> > plugins you assemble those overlays into a blob which can be applied
> > later.
> > 
> > Because it wasn't designed with runtime overlays in mind, the current
> > code handles the resolution of those overlays during the parse.  Your
> > patch extends that by rather than resolving them, just putting them
> > together with metadata into the dtbo.
> > 
> > I'm saying that resolution or assembly should be moved out of the
> > parser.  Instead, the parser output would be an (ordered) list of
> > overlay fragments.  In the main program the two modes of operation
> > become explicit: for base trees we resolve the overlays into a single
> > tree, for plugins we collate the pieces into the dtbo format.
> 
> The case for building an overlay is only for the syntactic sugar version.

I don't understand what you're saying here.

> This is not the common use case at all. I could easily remove it and then
> there would be no overlays built at all in the parser.

Remove what exactly?  You mean the case with &ref { } when you're not
building a dtbo?  I'm not sure that is so uncommon - dts include files
are basically useless without it.

> In fact the extra fixup nodes are generated now after the parse stage,
> after the check stage and before the sort stage.

Yes.. I'm not talking about the fixup nodes, I'm talking about the
assembly of the tree fragments.

> Perhaps it should be separated out so that we don’t get sidetracked?

What exactly should be separated out from what?

> >>> In addition to not making unsafe assumptions about parser operation, I
> >>> think this will also allow for better error reporting.  It also moves
> >>> more code into plain-old-C instead of potentially hard to follow bison
> >>> code fragments.
> >>> 
> >> 
> >> We don’t try to apply overlays during parse, and especially in the parse code.
> > 
> > The existing code does this, and you don't change it.  Furthermore you
> > absolutely do do the assembly of the fragments within the parser -
> > that's exactly what add_orphan_node() called directly from semantic
> > actions does.  To verify this, all you need to do is look at the
> > parser output: the boot_info structure has just a single tree, not a
> > list of overlays.
> > 
> 
> Only for the un-common case.
> 
> >> The global is used only for the syntactic sugar method of turning a &ref { };
> >> node into an overlay.
> > 
> > I'm saying that treating that as mere syntactic sugar is a fundamental
> > error in design.  We could get away with it when the &ref {} stuff was
> > always resolved immediately.  Now that that resolution can be delayed
> > until later, it gets worse.  We should move that resolution outside of
> > the parser.
> > 
> 
> The &ref { } stuff is sidetracking us. This is not the core issue.

You haven't convinced me of that.

> In fact out in the community there are not a lot of cases where it
> is used.

I thought it was the standard way to construct a dtbo.

But in any case, regardless of how common it is, it is currently
introducing an order dependency between different parts of the parse
tree that hasn't been adequately handled (in v9, I'll reasses when I
review the v10 patches).

> >>>> diff --git a/treesource.c b/treesource.c
> >>>> index a55d1d1..75e920d 100644
> >>>> --- a/treesource.c
> >>>> +++ b/treesource.c
> >>>> @@ -267,7 +267,12 @@ void dt_to_source(FILE *f, struct boot_info *bi)
> >>>> {
> >>>> 	struct reserve_info *re;
> >>>> 
> >>>> -	fprintf(f, "/dts-v1/;\n\n");
> >>>> +	fprintf(f, "/dts-v1/");
> >>>> +
> >>>> +	if (bi->versionflags & VF_PLUGIN)
> >>>> +		fprintf(f, " /plugin/");
> >>>> +
> >>>> +	fprintf(f, ";\n\n");
> >>> 
> >>> I'm not sure this really makes sense.  The /plugin/ tag triggers the
> >>> fixup construction and encoding of overlay fragments.  But in an
> >>> incoming dtb, that processing has already been done once, doing it
> >>> again would not make sense.  So I think the output should not have the
> >>> /plugin/ tag, even if the input did.
> >>> 
> >>> Unless, of course, you parsed the input into an explicit list of
> >>> overlays, then output it here again as a list of overlays, not the
> >>> encoded fragments.  i.e. if this was the original tree:
> >>> 
> >>> 	/dts-v1/ /plugin/;
> >>> 
> >>> 	&somwhere {
> >>> 		name = "value";
> >>> 	};
> >>> 
> >>> then legitimately the output of -I dts -O dts could be either:
> >>> 
> >>> 	/dts-v1/ /plugin/;
> >>> 
> >>> 	&somwhere {
> >>> 		name = "value";
> >>> 	};
> >>> 
> >>> OR
> >>> 
> >>> 	/dts-v1/;
> >>> 
> >>> 	/ {
> >>> 		fragment@0 {
> >>> 			target = <0xffffffff>;
> >>> 			__overlay__{
> >>> 				name = "value";
> >>> 			};
> >>> 		};
> >>> 		__fixups__ {
> >>> 			somewhere = "/fragment@0:target:0";
> >>> 		};
> >>> 	};
> >>> 
> >>> But it doesn't make sense to combine the two: the second structure
> >>> with the "/plugin/" tag.
> >>> 
> >> 
> >>> Another advantage of moving the overlay application out of the parser
> >>> is you can sanely produce either output, both of which could be useful
> >>> in the right circumstances.  You can potentially even produce the
> >>> first output (or something close) from dtb input, with correct parsing
> >>> of the overlay structure on dtb input.
> >>> 
> >> 
> >> Yes, this makes sense. For now I’ll remove the plugin tag, but if the blob
> >> was compiled with -@ you could conceivably reverse back to a form that contains
> >> labels and phandle references correctly.
> >> 
> >> But this is quite complicated to undertake in this patch series. 
> >> 
> >> To re-iterate though there is no overlay application here :)
> > 
> > Well, I think we've both been a bit sloppy with terminology - does
> > "overlay" refer to a dtbo file, or to one of the &ref { ... }
> > fragments in the source, or to both.
> > 
> > But whatever the right term is for the &ref { .. } fragments in the
> > source they absolutely *are* applied during compile for the base tree
> > case.  And while they're not resolved fully, they are encoded into
> > part of a larger tree structure for the plugin case.
> > 
> > The confusion will be reduced if we make these
> > overlays/fragments/whatever first class objects in the "live tree"
> > phase of the compile, and move the resolution and/or assembly of them
> > a stage that's separate from the parse.  In addition, it opens the way
> > for decompiling a dtbo more naturally, though as you say that's a
> > moderate amount of extra work.
> 
> How about we get the non &ref { } case sorted out and we can talk about
> the syntactic sugar version done?

Uh.. sure.  I'm not really clear on what you mean by that, but I guess
I'll look and see.

> 
> v10 has been sent out.
> 
> 
> 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 --]

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

[Pantelis Antoniou]

Hi David,

> On Nov 28, 2016, at 04:32 , David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> 
> On Fri, Nov 25, 2016 at 02:44:39PM +0200, Pantelis Antoniou wrote:
>> Hi David,
>> 
>>> On Nov 25, 2016, at 13:26 , David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>>> 
>>> On Fri, Nov 25, 2016 at 12:55:25PM +0200, Pantelis Antoniou wrote:
>>>> Hi David,
>>>> 
>>>>> On Nov 25, 2016, at 06:11 , David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>>>>> 
>>>>> On Thu, Nov 24, 2016 at 02:31:32PM +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 disabled. This is in order to give time for DT blob
>>>>>> methods to be updated.
>>>>>> 
>>>>>> 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             |  49 +++++++++--
>>>>>> 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 +-
>>>>>> treesource.c             |   7 +-
>>>>>> 14 files changed, 380 insertions(+), 31 deletions(-)
>>>>>> 
>>>>>> diff --git a/Documentation/manual.txt b/Documentation/manual.txt
>>>>>> index 398de32..65fbf09 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 new FDT magic number. By default blobs with the
>>>>>> +	standard FDT magic number are generated.
>>>>> 
>>>>> First, this description is incomplete since -M *only* affects the
>>>>> magic number for /plugin/ input, not in other cases.  Second, the
>>>>> default is the wrong way around.  If we make old-style the default,
>>>>> then new-style will never be used, which defeats the purpose.
>>>> 
>>>> Then we’ll break user-space that has this assumption (i.e. that the magic is the same).
>>>> I can certainly do it the other way around.
>>> 
>>> Which userspace in particular?
>>> 
>> 
>> Kernel unflatteners in various OSes/bootloaders? All those would have to be updated since
>> they don’t grok the new magic number.
> 
> Those will certainly need dtbs with the old magic number as input, but
> I don't see how that affects the dtc default.  Using the option
> explicitly if you're targetting a bootloader that hasn't been updated
> seems reasonable.
> 

OK.

>>>>>> +
>>>>>>   -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 609975a..bc03d42 100644
>>>>>> --- a/checks.c
>>>>>> +++ b/checks.c
>>>>>> @@ -486,8 +486,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..4afc592 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,9 @@ extern void yyerror(char const *s);
>>>>>> 
>>>>>> extern struct boot_info *the_boot_info;
>>>>>> extern bool treesource_error;
>>>>>> +
>>>>>> +/* temporary while the tree is not built */
>>>>>> +static unsigned int the_versionflags;
>>>>> 
>>>>> Hrm.  Using a global during parsing is pretty dangerous - it makes
>>>>> assumptions about the order in which bison will execute semantic
>>>>> actions that may not always be correct.
>>>>> 
>>>>> It'll probably work in practice, so I *might* be convinced it's
>>>>> adequate for a first cut.  I'm a bit reluctant though, since I suspect
>>>>> once merged it will become entrenched.
>>>>> 
>>>> 
>>>> We use bison, globals are the way of life. It’s not going to be used
>>>> anywhere else, it’s static in the parser file.
>>> 
>>> Using globals to communicate the final result is inevitable (well, not
>>> without using the whole different re-entrant interface stuff).  That
>>> just assumes that the start symbol's semantic action is executed
>>> before the parser competes, which is guaranteed.
>>> 
>>> This is a different matter - using a global to communicate between
>>> different parts of the parser.  More specifically different parts of
>>> the parser that are in different arms of the parse tree.  That makes
>>> assumptions about the relative order of semantic actions which are
>>> *not* guaranteed.  In theory the parser could generate the entire
>>> parse tree and semantic action dependency graph, then execute them in
>>> an arbitrary order (well, it would have to be a topologically sorted
>>> order, but that won't help).
>>> 
>> 
>> I’ve given it a little bit more thought and it’s easily fixed by using
>> inherited attributes which is safe to do and avoid the global all together.
> 
> Hmm.. we'll see.
> 
>>>> We could allocate the boot_info earlier (when the v1tag is detected) but
>>>> that would be quite a big change for something as trivial. 
>>> 
>>> That wouldn't help.  You still wouldn't have a guarantee on the order
>>> between setting the version flags and using the version flags.
>>> 
>>>>> The correct way to handle this, IMO, is not to ever attempt to apply
>>>>> overlays during the parse.  Basically, we'd change the overall
>>>>> structure so that the output from the parser is not a single tree, but
>>>>> a list of overlays / fragments.  Then, once the parse is complete, so
>>>>> versioninfo (which could now become a member of bootinfo) is well
>>>>> defined, we either apply the fragments in place (base tree) or encode
>>>>> them into the overlay structure (plugin mode).
>>>>> 
>>>>> See https://github.com/dgibson/dtc/tree/overlay for some incomplete
>>>>> work I did in this direction.
>>>>> 
>>>> 
>>>> This tree is pretty stale; last commit was back in march.
>>> 
>>> Yes, it was a while since I worked on it.  It rebased clean, though.
>>> 
>>>> I thing there’s a wrong assumption here. The purpose of this patch is not
>>>> to apply overlays during compile time, is to generate a blob that can be
>>>> applied at runtime by another piece of software.
>>> 
>>> No, I realise what you're doing.  But the input is in the form of a
>>> batch of overlays, regardless.  You then have two modes of operation:
>>> for base trees you resolve those overlays during the compile, for
>>> plugins you assemble those overlays into a blob which can be applied
>>> later.
>>> 
>>> Because it wasn't designed with runtime overlays in mind, the current
>>> code handles the resolution of those overlays during the parse.  Your
>>> patch extends that by rather than resolving them, just putting them
>>> together with metadata into the dtbo.
>>> 
>>> I'm saying that resolution or assembly should be moved out of the
>>> parser.  Instead, the parser output would be an (ordered) list of
>>> overlay fragments.  In the main program the two modes of operation
>>> become explicit: for base trees we resolve the overlays into a single
>>> tree, for plugins we collate the pieces into the dtbo format.
>> 
>> The case for building an overlay is only for the syntactic sugar version.
> 
> I don't understand what you're saying here.
> 
>> This is not the common use case at all. I could easily remove it and then
>> there would be no overlays built at all in the parser.
> 
> Remove what exactly?  You mean the case with &ref { } when you're not
> building a dtbo?  I'm not sure that is so uncommon - dts include files
> are basically useless without it.
> 

Those cases are not meant to generate an overlay. We are not targeting
this right now.
 
>> In fact the extra fixup nodes are generated now after the parse stage,
>> after the check stage and before the sort stage.
> 
> Yes.. I'm not talking about the fixup nodes, I'm talking about the
> assembly of the tree fragments.
> 
>> Perhaps it should be separated out so that we don’t get sidetracked?
> 
> What exactly should be separated out from what?
> 

Well, a new patch is going to be shortly posted and will make this clearer.

>>>>> In addition to not making unsafe assumptions about parser operation, I
>>>>> think this will also allow for better error reporting.  It also moves
>>>>> more code into plain-old-C instead of potentially hard to follow bison
>>>>> code fragments.
>>>>> 
>>>> 
>>>> We don’t try to apply overlays during parse, and especially in the parse code.
>>> 
>>> The existing code does this, and you don't change it.  Furthermore you
>>> absolutely do do the assembly of the fragments within the parser -
>>> that's exactly what add_orphan_node() called directly from semantic
>>> actions does.  To verify this, all you need to do is look at the
>>> parser output: the boot_info structure has just a single tree, not a
>>> list of overlays.
>>> 
>> 
>> Only for the un-common case.
>> 
>>>> The global is used only for the syntactic sugar method of turning a &ref { };
>>>> node into an overlay.
>>> 
>>> I'm saying that treating that as mere syntactic sugar is a fundamental
>>> error in design.  We could get away with it when the &ref {} stuff was
>>> always resolved immediately.  Now that that resolution can be delayed
>>> until later, it gets worse.  We should move that resolution outside of
>>> the parser.
>>> 
>> 
>> The &ref { } stuff is sidetracking us. This is not the core issue.
> 
> You haven't convinced me of that.
> 
>> In fact out in the community there are not a lot of cases where it
>> is used.
> 
> I thought it was the standard way to construct a dtbo.
> 

No, not by a long shot. All bb.org, rpi & chip overlays use the vanilla
method.


> But in any case, regardless of how common it is, it is currently
> introducing an order dependency between different parts of the parse
> tree that hasn't been adequately handled (in v9, I'll reasses when I
> review the v10 patches).
> 
>>>>>> diff --git a/treesource.c b/treesource.c
>>>>>> index a55d1d1..75e920d 100644
>>>>>> --- a/treesource.c
>>>>>> +++ b/treesource.c
>>>>>> @@ -267,7 +267,12 @@ void dt_to_source(FILE *f, struct boot_info *bi)
>>>>>> {
>>>>>> 	struct reserve_info *re;
>>>>>> 
>>>>>> -	fprintf(f, "/dts-v1/;\n\n");
>>>>>> +	fprintf(f, "/dts-v1/");
>>>>>> +
>>>>>> +	if (bi->versionflags & VF_PLUGIN)
>>>>>> +		fprintf(f, " /plugin/");
>>>>>> +
>>>>>> +	fprintf(f, ";\n\n");
>>>>> 
>>>>> I'm not sure this really makes sense.  The /plugin/ tag triggers the
>>>>> fixup construction and encoding of overlay fragments.  But in an
>>>>> incoming dtb, that processing has already been done once, doing it
>>>>> again would not make sense.  So I think the output should not have the
>>>>> /plugin/ tag, even if the input did.
>>>>> 
>>>>> Unless, of course, you parsed the input into an explicit list of
>>>>> overlays, then output it here again as a list of overlays, not the
>>>>> encoded fragments.  i.e. if this was the original tree:
>>>>> 
>>>>> 	/dts-v1/ /plugin/;
>>>>> 
>>>>> 	&somwhere {
>>>>> 		name = "value";
>>>>> 	};
>>>>> 
>>>>> then legitimately the output of -I dts -O dts could be either:
>>>>> 
>>>>> 	/dts-v1/ /plugin/;
>>>>> 
>>>>> 	&somwhere {
>>>>> 		name = "value";
>>>>> 	};
>>>>> 
>>>>> OR
>>>>> 
>>>>> 	/dts-v1/;
>>>>> 
>>>>> 	/ {
>>>>> 		fragment@0 {
>>>>> 			target = <0xffffffff>;
>>>>> 			__overlay__{
>>>>> 				name = "value";
>>>>> 			};
>>>>> 		};
>>>>> 		__fixups__ {
>>>>> 			somewhere = "/fragment@0:target:0";
>>>>> 		};
>>>>> 	};
>>>>> 
>>>>> But it doesn't make sense to combine the two: the second structure
>>>>> with the "/plugin/" tag.
>>>>> 
>>>> 
>>>>> Another advantage of moving the overlay application out of the parser
>>>>> is you can sanely produce either output, both of which could be useful
>>>>> in the right circumstances.  You can potentially even produce the
>>>>> first output (or something close) from dtb input, with correct parsing
>>>>> of the overlay structure on dtb input.
>>>>> 
>>>> 
>>>> Yes, this makes sense. For now I’ll remove the plugin tag, but if the blob
>>>> was compiled with -@ you could conceivably reverse back to a form that contains
>>>> labels and phandle references correctly.
>>>> 
>>>> But this is quite complicated to undertake in this patch series. 
>>>> 
>>>> To re-iterate though there is no overlay application here :)
>>> 
>>> Well, I think we've both been a bit sloppy with terminology - does
>>> "overlay" refer to a dtbo file, or to one of the &ref { ... }
>>> fragments in the source, or to both.
>>> 
>>> But whatever the right term is for the &ref { .. } fragments in the
>>> source they absolutely *are* applied during compile for the base tree
>>> case.  And while they're not resolved fully, they are encoded into
>>> part of a larger tree structure for the plugin case.
>>> 
>>> The confusion will be reduced if we make these
>>> overlays/fragments/whatever first class objects in the "live tree"
>>> phase of the compile, and move the resolution and/or assembly of them
>>> a stage that's separate from the parse.  In addition, it opens the way
>>> for decompiling a dtbo more naturally, though as you say that's a
>>> moderate amount of extra work.
>> 
>> How about we get the non &ref { } case sorted out and we can talk about
>> the syntactic sugar version done?
> 
> Uh.. sure.  I'm not really clear on what you mean by that, but I guess
> I'll look and see.
> 
>> 
>> v10 has been sent out.
>> 
>> 
>> 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

Regards

— Pantelis