Hi David,
> On Nov 25, 2016, at 13:26 , David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxx>
>>>> Signed-off-by: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
>>>> Signed-off-by: Jan Luebbe <jlu@xxxxxxxxxxxxxx>
>>>> ---
>>>> 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
