Hi David,
> On Nov 28, 2016, at 04:32 , David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> 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@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.
>
> 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
--
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
