On Tue, Nov 29, 2016 at 01:09:11PM +0200, Pantelis Antoniou wrote:
> Hi David,
>
> > On Nov 29, 2016, at 04:10 , David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> wrote:
> >
> > 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__.
> >
>
> It is easily done. Although using /plugin/ as an auto-magic option does both
> just fine.
Sorry, I don't follow what you're saying.
> >> For plugins there is no requirement for now to actually contain references to
> >> be resolved. It can easily be enforced though.
> >
> > Sure, but I don't see the relevance of that here. You could just omit
> > the __fixups__ node if there's nothing to go into them.
> >
>
> Hmm, yeah.
>
>
> Regards
>
> — Pantelis
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
Attachment:
signature.asc
Description: PGP signature
