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.
> >>>> +
> >>>> -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:
signature.asc
Description: PGP signature
