On Mon, Nov 28, 2016 at 02:10:35PM +0200, Pantelis Antoniou wrote:
>
> > On Nov 28, 2016, at 06:12 , David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> wrote:
> >
> > On Fri, Nov 25, 2016 at 02:32:10PM +0200, Pantelis Antoniou wrote:
> >> This patch enable the generation of symbols & local fixup information
> >> for trees compiled with the -@ (--symbols) option.
> >>
> >> Using this patch labels in the tree and their users emit information
> >> in __symbols__ and __local_fixups__ nodes.
> >>
> >> The __fixups__ node make possible the dynamic resolution of phandle
> >> references which are present in the plugin tree but lie in the
> >> tree that are applying the overlay against.
> >>
> >> While there is a new magic number for dynamic device tree/overlays blobs
> >> it is by default enabled. Remember to use -M to generate compatible
> >> blobs.
> >>
> >> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@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 | 50 +++++++++--
> >> dtc.c | 39 +++++++-
> >> dtc.h | 20 ++++-
> >> fdtdump.c | 2 +-
> >> flattree.c | 17 ++--
> >> fstree.c | 2 +-
> >> libfdt/fdt.c | 2 +-
> >> libfdt/fdt.h | 3 +-
> >> livetree.c | 225 ++++++++++++++++++++++++++++++++++++++++++++++-
> >> tests/mangle-layout.c | 7 +-
> >> 13 files changed, 375 insertions(+), 30 deletions(-)
> >>
> >> diff --git a/Documentation/manual.txt b/Documentation/manual.txt
> >> index 398de32..094893b 100644
> >> --- a/Documentation/manual.txt
> >> +++ b/Documentation/manual.txt
> >> @@ -119,6 +119,24 @@ Options:
> >> Make space for <number> reserve map entries
> >> Relevant for dtb and asm output only.
> >>
> >> + -@
> >> + Generates a __symbols__ node at the root node of the resulting blob
> >> + for any node labels used, and for any local references using phandles
> >> + it also generates a __local_fixups__ node that tracks them.
> >> +
> >> + When using the /plugin/ tag all unresolved label references to
> >> + be tracked in the __fixups__ node, making dynamic resolution possible.
> >> +
> >> + -A
> >> + Generate automatically aliases for all node labels. This is similar to
> >> + the -@ option (the __symbols__ node contain identical information) but
> >> + the semantics are slightly different since no phandles are automatically
> >> + generated for labeled nodes.
> >> +
> >> + -M
> >> + Generate blobs with the old FDT magic number for device tree objects.
> >> + By default blobs use the DTBO FDT magic number instead.
> >> +
> >> -S <bytes>
> >> Ensure the blob at least <bytes> long, adding additional
> >> space if needed.
> >> @@ -146,13 +164,18 @@ Additionally, dtc performs various sanity checks on the tree.
> >> Here is a very rough overview of the layout of a DTS source file:
> >>
> >>
> >> - sourcefile: list_of_memreserve devicetree
> >> + sourcefile: versioninfo plugindecl list_of_memreserve devicetree
> >>
> >> memreserve: label 'memreserve' ADDR ADDR ';'
> >> | label 'memreserve' ADDR '-' ADDR ';'
> >>
> >> devicetree: '/' nodedef
> >>
> >> + versioninfo: '/' 'dts-v1' '/' ';'
> >> +
> >> + plugindecl: '/' 'plugin' '/' ';'
> >> + | /* empty */
> >> +
> >> nodedef: '{' list_of_property list_of_subnode '}' ';'
> >>
> >> property: label PROPNAME '=' propdata ';'
> >> diff --git a/checks.c b/checks.c
> >> index 2bd27a4..4292f4b 100644
> >> --- a/checks.c
> >> +++ b/checks.c
> >> @@ -487,8 +487,12 @@ static void fixup_phandle_references(struct check *c, struct boot_info *bi,
> >>
> >> refnode = get_node_by_ref(dt, m->ref);
> >> if (! refnode) {
> >> - FAIL(c, "Reference to non-existent node or label \"%s\"\n",
> >> - m->ref);
> >> + if (!(bi->versionflags & VF_PLUGIN))
> >> + FAIL(c, "Reference to non-existent node or "
> >> + "label \"%s\"\n", m->ref);
> >> + else /* mark the entry as unresolved */
> >> + *((cell_t *)(prop->val.val + m->offset)) =
> >> + cpu_to_fdt32(0xffffffff);
> >> continue;
> >> }
> >>
> >> diff --git a/dtc-lexer.l b/dtc-lexer.l
> >> index 790fbf6..40bbc87 100644
> >> --- a/dtc-lexer.l
> >> +++ b/dtc-lexer.l
> >> @@ -121,6 +121,11 @@ static void lexical_error(const char *fmt, ...);
> >> return DT_V1;
> >> }
> >>
> >> +<*>"/plugin/" {
> >> + DPRINT("Keyword: /plugin/\n");
> >> + return DT_PLUGIN;
> >> + }
> >> +
> >> <*>"/memreserve/" {
> >> DPRINT("Keyword: /memreserve/\n");
> >> BEGIN_DEFAULT();
> >> diff --git a/dtc-parser.y b/dtc-parser.y
> >> index 14aaf2e..1a1f660 100644
> >> --- a/dtc-parser.y
> >> +++ b/dtc-parser.y
> >> @@ -19,6 +19,7 @@
> >> */
> >> %{
> >> #include <stdio.h>
> >> +#include <inttypes.h>
> >>
> >> #include "dtc.h"
> >> #include "srcpos.h"
> >> @@ -33,6 +34,7 @@ extern void yyerror(char const *s);
> >>
> >> extern struct boot_info *the_boot_info;
> >> extern bool treesource_error;
> >> +
> >
> > Extraneous whitespace change here
> >
>
> OK.
>
> >> %}
> >>
> >> %union {
> >> @@ -52,9 +54,11 @@ extern bool treesource_error;
> >> struct node *nodelist;
> >> struct reserve_info *re;
> >> uint64_t integer;
> >> + unsigned int flags;
> >> }
> >>
> >> %token DT_V1
> >> +%token DT_PLUGIN
> >> %token DT_MEMRESERVE
> >> %token DT_LSHIFT DT_RSHIFT DT_LE DT_GE DT_EQ DT_NE DT_AND DT_OR
> >> %token DT_BITS
> >> @@ -71,6 +75,8 @@ extern bool treesource_error;
> >>
> >> %type <data> propdata
> >> %type <data> propdataprefix
> >> +%type <flags> versioninfo
> >> +%type <flags> plugindecl
> >> %type <re> memreserve
> >> %type <re> memreserves
> >> %type <array> arrayprefix
> >> @@ -101,16 +107,34 @@ extern bool treesource_error;
> >> %%
> >>
> >> sourcefile:
> >> - v1tag memreserves devicetree
> >> + versioninfo plugindecl memreserves devicetree
> >> + {
> >> + the_boot_info = build_boot_info($1 | $2, $3, $4,
> >> + guess_boot_cpuid($4));
> >> + }
> >> + ;
> >> +
> >> +versioninfo:
> >> + v1tag
> >> {
> >> - the_boot_info = build_boot_info($2, $3,
> >> - guess_boot_cpuid($3));
> >> + $$ = VF_DT_V1;
> >> }
> >> ;
> >>
> >> v1tag:
> >> DT_V1 ';'
> >> + | DT_V1
> >> | DT_V1 ';' v1tag
> >> +
> >> +plugindecl:
> >> + DT_PLUGIN ';'
> >> + {
> >> + $$ = VF_PLUGIN;
> >> + }
> >> + | /* empty */
> >> + {
> >> + $$ = 0;
> >> + }
> >> ;
> >>
> >> memreserves:
> >> @@ -161,10 +185,19 @@ devicetree:
> >> {
> >> struct node *target = get_node_by_ref($1, $2);
> >>
> >> - if (target)
> >> + if (target) {
> >> merge_nodes(target, $3);
> >> - else
> >> - ERROR(&@2, "Label or path %s not found", $2);
> >> + } else {
> >> + /*
> >> + * We rely on the rule being always:
> >> + * versioninfo plugindecl memreserves devicetree
> >> + * so $-1 is what we want (plugindecl)
> >> + */
> >> + if ($<flags>-1 & VF_PLUGIN)
> >
> > o_O... ok. I've never seen negative value references before. Can you
> > provide a link to some documentation saying this is actually supported
> > usage in bison? I wasn't able to find it when I looked.
> >
>
> There is a section about inherited attributes in the flex & bison book by O’Reily.
>
> https://books.google.gr/books?id=3Sr1V5J9_qMC&lpg=PP1&dq=flex%20bison&hl=el&pg=PP1#v=onepage&q=flex%20bison&f=false
>
> There’s a direct link to the 2nd Edition of lex & yacc:
>
> https://books.google.gr/books?id=fMPxfWfe67EC&lpg=PA183&ots=RcRSji2NAT&dq=yacc%20inherited%20attributes&hl=el&pg=PA183#v=onepage&q=yacc%20inherited%20attributes&f=false
Thanks for the link. I still think moving the fragment assembly out
of the parser will be a better idea long term, but this does address
the main concern I had, so it will do for now.
> >> + add_orphan_node($1, $3, $2);
> >> + else
> >> + ERROR(&@2, "Label or path %s not found", $2);
> >> + }
> >> $$ = $1;
> >> }
> >> | devicetree DT_DEL_NODE DT_REF ';'
> >> @@ -179,6 +212,11 @@ devicetree:
> >>
> >> $$ = $1;
> >> }
> >> + | /* empty */
> >> + {
> >> + /* build empty node */
> >> + $$ = name_node(build_node(NULL, NULL), "");
> >> + }
> >> ;
> >>
> >> nodedef:
> >> diff --git a/dtc.c b/dtc.c
> >> index 9dcf640..06e91bc 100644
> >> --- a/dtc.c
> >> +++ b/dtc.c
> >> @@ -32,6 +32,9 @@ int minsize; /* Minimum blob size */
> >> int padsize; /* Additional padding to blob */
> >> int alignsize; /* Additional padding to blob accroding to the alignsize */
> >> int phandle_format = PHANDLE_BOTH; /* Use linux,phandle or phandle properties */
> >> +int symbol_fixup_support; /* enable symbols & fixup support */
> >> +int auto_label_aliases; /* auto generate labels -> aliases */
> >> +int no_dtbo_magic; /* use old FDT magic values for objects */
> >>
> >> static int is_power_of_2(int x)
> >> {
> >> @@ -59,7 +62,7 @@ static void fill_fullpaths(struct node *tree, const char *prefix)
> >> #define FDT_VERSION(version) _FDT_VERSION(version)
> >> #define _FDT_VERSION(version) #version
> >> static const char usage_synopsis[] = "dtc [options] <input file>";
> >> -static const char usage_short_opts[] = "qI:O:o:V:d:R:S:p:a:fb:i:H:sW:E:hv";
> >> +static const char usage_short_opts[] = "qI:O:o:V:d:R:S:p:a:fb:i:H:sW:E:@AMhv";
> >> static struct option const usage_long_opts[] = {
> >> {"quiet", no_argument, NULL, 'q'},
> >> {"in-format", a_argument, NULL, 'I'},
> >> @@ -78,6 +81,9 @@ static struct option const usage_long_opts[] = {
> >> {"phandle", a_argument, NULL, 'H'},
> >> {"warning", a_argument, NULL, 'W'},
> >> {"error", a_argument, NULL, 'E'},
> >> + {"symbols", no_argument, NULL, '@'},
> >> + {"auto-alias", no_argument, NULL, 'A'},
> >> + {"no-dtbo-magic", no_argument, NULL, 'M'},
> >> {"help", no_argument, NULL, 'h'},
> >> {"version", no_argument, NULL, 'v'},
> >> {NULL, no_argument, NULL, 0x0},
> >> @@ -109,6 +115,9 @@ static const char * const usage_opts_help[] = {
> >> "\t\tboth - Both \"linux,phandle\" and \"phandle\" properties",
> >> "\n\tEnable/disable warnings (prefix with \"no-\")",
> >> "\n\tEnable/disable errors (prefix with \"no-\")",
> >> + "\n\tEnable symbols/fixup support",
> >> + "\n\tEnable auto-alias of labels",
> >> + "\n\tDo not use DTBO magic value for plugin objects",
> >> "\n\tPrint this help and exit",
> >> "\n\tPrint version and exit",
> >> NULL,
> >> @@ -153,7 +162,7 @@ static const char *guess_input_format(const char *fname, const char *fallback)
> >> fclose(f);
> >>
> >> magic = fdt32_to_cpu(magic);
> >> - if (magic == FDT_MAGIC)
> >> + if (magic == FDT_MAGIC || magic == FDT_MAGIC_DTBO)
> >> return "dtb";
> >>
> >> return guess_type_by_name(fname, fallback);
> >> @@ -172,6 +181,7 @@ int main(int argc, char *argv[])
> >> FILE *outf = NULL;
> >> int outversion = DEFAULT_FDT_VERSION;
> >> long long cmdline_boot_cpuid = -1;
> >> + fdt32_t out_magic = FDT_MAGIC;
> >>
> >> quiet = 0;
> >> reservenum = 0;
> >> @@ -249,6 +259,16 @@ int main(int argc, char *argv[])
> >> parse_checks_option(false, true, optarg);
> >> break;
> >>
> >> + case '@':
> >> + symbol_fixup_support = 1;
> >> + break;
> >> + case 'A':
> >> + auto_label_aliases = 1;
> >> + break;
> >> + case 'M':
> >> + no_dtbo_magic = 1;
> >> + break;
> >> +
> >> case 'h':
> >> usage(NULL);
> >> default:
> >> @@ -306,6 +326,14 @@ int main(int argc, char *argv[])
> >> fill_fullpaths(bi->dt, "");
> >> process_checks(force, bi);
> >>
> >> + if (auto_label_aliases)
> >> + generate_label_tree(bi->dt, "aliases", false);
> >> +
> >> + if (symbol_fixup_support) {
> >> + generate_label_tree(bi->dt, "__symbols__", true);
> >> + generate_fixups_tree(bi->dt);
> >
> > Hang on.. this doesn't seem right. I thought -@ controlled the
> > __symbols__ side (i.e. the part upon which we overlay) rather than the
> > fixups side (the part which overlays). A dtbo could certainly have
> > both, of course, but for base trees, wouldn't you have symbols without
> > fixups? And should it be illegal to try to build a /plugin/ without
> > -@?
>
> It does control both for now. For base trees having the fixup nodes
> will allow us to do probe order dependency tracking in the future.
Erm.. how?
> For plugins we need the __symbols__ node to support stacked overlays, i.e.
> overlays referring label that were introduced by a previous overlay.
Yes, I realise that an overlay may well want __symbols__ as well. But
they still seem conceptually different. I think -@ should control
__symbols__ whereas /plugin/ should control __fixups__.
> For plugins there is no requirement for now to actually contain references to
> be resolved. It can easily be enforced though.
Sure, but I don't see the relevance of that here. You could just omit
the __fixups__ node if there's nothing to go into them.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
Attachment:
signature.asc
Description: PGP signature
