Hi Alper,
On Tue, 15 Feb 2022 at 04:53, Alper Nebi Yasak <alpernebiya...@gmail.com> wrote:
>
> On 08/02/2022 21:50, Simon Glass wrote:
> > Some boards need to load an ELF file using the 'loadables' property, but
> > the file has segments at different memory addresses. This means that it
> > cannot be supplied as a flat binary.
> >
> > Allow generating a separate node in the FIT for each segment in the ELF,
> > with a different load address for each.
> >
> > Signed-off-by: Simon Glass <s...@chromium.org>
> > ---
> >
> > tools/binman/entries.rst | 146 +++++++++++
> > tools/binman/etype/fit.py | 259 ++++++++++++++++++-
> > tools/binman/ftest.py | 116 +++++++++
> > tools/binman/test/221_fit_split_elf.dts | 67 +++++
> > tools/binman/test/222_fit_bad_dir.dts | 9 +
> > tools/binman/test/223_fit_bad_dir_config.dts | 9 +
> > 6 files changed, 594 insertions(+), 12 deletions(-)
> > create mode 100644 tools/binman/test/221_fit_split_elf.dts
> > create mode 100644 tools/binman/test/222_fit_bad_dir.dts
> > create mode 100644 tools/binman/test/223_fit_bad_dir_config.dts
> >
> > diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst
> > index d483169712..079fed1a9c 100644
> > --- a/tools/binman/entries.rst
> > +++ b/tools/binman/entries.rst
> > @@ -612,6 +612,9 @@ gen-fdt-nodes
> > Generate FDT nodes as above. This is the default if there is no
> > `fit,operation` property.
> >
> > +split-elf
> > + Split an ELF file into a separate node for each segment.
> > +
> > Generating nodes from an FDT list (gen-fdt-nodes)
> > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > @@ -655,6 +658,149 @@ for each of your two files.
> > Note that if no devicetree files are provided (with '-a of-list' as above)
> > then no nodes will be generated.
> >
> > +Generating nodes from an ELF file (split-elf)
> > +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > +
> > +This uses the node as a template to generate multiple nodes. The following
> > +special properties are available:
> > +
> > +split-elf
> > + Split an ELF file into a separate node for each segment. This uses the
> > + node as a template to generate multiple nodes. The following special
> > + properties are available:
> > +
> > + fit,load
> > + Generates a `load = <...>` property with the load address of the
> > + segmnet
> > +
> > + fit,entry
> > + Generates a `entry = <...>` property with the entry address of the
> > + ELF. This is only produced for the first entry
> > +
> > + fit,data
> > + Generates a `data = <...>` property with the contents of the
> > segment
>
> I think all these should be done by default. I don't see the point of
> not setting the properties, or setting them manually to values that will
> be the same for multiple nodes.
My intent is to make things discoverable and obvious, so that magic
processing is explicit.
>
> > +
> > + fit,loadables
> > + Generates a `loadable = <...>` property with a list of the
> > generated
> > + nodes (including all nodes if this operation is used multiple
> > times)
>
> I think this could also be the default behaviour like the above, and
> even for configs which have a "loadables" property we could append to it
> by default.
>
> Either way, if the "loadables" property exists this needs to append to
> it instead of entirely replacing it. (Needs to append to "u-boot", see
> comment at example generated config below.)
Yes, makes sense.
>
> > +
> > +
> > +Here is an example showing ATF, TEE and a device tree all combined::
>
> This looks like a better place to write about the template node design
> so I'll try here instead of the previous patch. I've always thought they
> were ugly but I couldn't think of a good enough design when they were
> introduced. Maybe I have now.
>
> > +
> > + fit {
> > + description = "test-desc";
> > + #address-cells = <1>;
> > + fit,fdt-list = "of-list";
> > +
> > + images {
> > + u-boot {
> > + description = "U-Boot (64-bit)";
> > + type = "standalone";
> > + os = "U-Boot";
> > + arch = "arm64";
> > + compression = "none";
> > + load = <CONFIG_SYS_TEXT_BASE>;
> > + u-boot-nodtb {
> > + };
> > + };
> > + @fdt-SEQ {
> > + description = "fdt-NAME.dtb";
> > + type = "flat_dt";
> > + compression = "none";
> > + };
>
> Instead of putting these into the images subnode, we could have
> images-level subnodes for the operations. For example, instead of the above:
>
> images@gen-fdt-nodes {
> fdt-list = "of-list";
>
> fdt {
> type = "flat_dt";
> compression = "none";
> };
> };
What does that mean, though? I presume it creates a second images {}
node, which is fine as dtc will merge them. But what about ordering?
I certainly prefer this in terms of elegance. I'm just not convinced
that people will understand it as well.
>
> We can remove the -SEQ if we always append a sequence number, and we can
> set "description" to "NAME.dtb" when it's missing, or do the replacement
> when it's given. We can go further and use "%s", "%(name)s" or "{name}"
> instead of "NAME" for python-ish formatting and likewise for the
> sequence number.
Yes, but again this adds more magic. For the Python formatting, we
still need to restrict what is put in there - e.g. we cannot just eval
an arbitrary varaible.
>
> > + @atf-SEQ {
> > + fit,operation = "split-elf";
> > + description = "ARM Trusted Firmware";
> > + type = "firmware";
> > + arch = "arm64";
> > + os = "arm-trusted-firmware";
> > + compression = "none";
> > + fit,load;
> > + fit,entry;
> > + fit,data;
> > +
> > + atf-bl31 {
> > + };
> > + };
> > +
> > + @tee-SEQ {
> > + fit,operation = "split-elf";
> > + description = "TEE";
> > + type = "tee";
> > + arch = "arm64";
> > + os = "tee";
> > + compression = "none";
> > + fit,load;
> > + fit,entry;
> > + fit,data;
> > +
> > + op-tee {
> > + };
> > + };
> > + };
>
> Similarly, instead of the above two:
>
> images@split-elf {
> atf {
> description = "ARM Trusted Firmware";
> type = "firmware";
> arch = "arm64";
> os = "arm-trusted-firmware";
> compression = "none";
>
> atf-bl31 {
> };
> };
>
> tee {
> description = "TEE";
> type = "tee";
> arch = "arm64";
> os = "tee";
> compression = "none";
>
> op-tee {
> };
> };
> };
>
> The load, entry, data properties could be set automatically without
> explicit mention.
>
> > +
> > + configurations {
> > + default = "@config-DEFAULT-SEQ";
> > + @config-SEQ {
> > + description = "conf-NAME.dtb";
> > + fdt = "fdt-SEQ";
> > + firmware = "u-boot";
> > + fit,loadables;
> > + };
> > + };
>
> And instead of the above:
>
> configurations@gen-fdt-nodes {
> fdt-list = "of-list";
>
> config {
> fdt = "fdt";
> firmware = "u-boot";
> };
> };
>
> Again, we can automatically add a sequence number to "config" node names
> and "fdt" property value, and set/replace a description property.
>
> We can collect all loadables from the images@split-elf nodes and create
> or append to the "loadables" properties. Or we could specify "atf" and
> "fdt" in the loadables and they could be expanded with sequence numbers.
>
> And if the "default" property of "configurations" node (or even the
> whole node) is missing, we can make configurations@gen-fdt-nodes set it
> to the default-dt.
>
>
> (Remains to be seen if I'll be annoyed enough to implement this myself.)
We should talk about this some more though. I'm a bit worried it will
get complaints about too much magic. I am trying to make it obvious
that nodes get generated in certain places.
>
> > + };
> > +
> > +If ATF-BL31 is available, this generates a node for each segment in the
> > +ELF file, for example::
> > +
> > + images {
> > + atf-1 {
> > + data = <...contents of first segment...>;
> > + data-offset = <0x00000000>;
> > + entry = <0x00040000>;
> > + load = <0x00040000>;
> > + compression = "none";
> > + os = "arm-trusted-firmware";
> > + arch = "arm64";
> > + type = "firmware";
> > + description = "ARM Trusted Firmware";
> > + };
> > + atf-2 {
> > + data = <...contents of second segment...>;
> > + load = <0xff3b0000>;
> > + compression = "none";
> > + os = "arm-trusted-firmware";
> > + arch = "arm64";
> > + type = "firmware";
> > + description = "ARM Trusted Firmware";
> > + };
> > + };
> > +
> > +The same applies for OP-TEE if that is available.
> > +
> > +If each binary is not available, the relevant template node (@atf-SEQ or
> > +@tee-SEQ) is removed from the output.
>
> I'm running into the fake-file thing with op-tee and getting a "Magic
> number does not match" error on consecutive builds as the leftover fake
> op-tee isn't an ELF...
Yes, I added a TODO for that. I think the take files should go in a
binman-fake subdir, removed on startup.
>
> > +
> > +This also generates a `config-xxx` node for each device tree in `of-list`.
> > +Note that the U-Boot build system uses `-a of-list=$(CONFIG_OF_LIST)`
> > +so you can use `CONFIG_OF_LIST` to define that list. In this example it is
> > +set up for `firefly-rk3399` with a single device tree and the default set
> > +with `-a default-dt=$(CONFIG_DEFAULT_DEVICE_TREE)`, so the resulting output
> > +is::
> > +
> > + configurations {
> > + default = "config-1";
> > + config-1 {
> > + loadables = "atf-1", "atf-2", "atf-3", "tee-1", "tee-2";
> > + description = "rk3399-firefly.dtb";
> > + fdt = "fdt-1";
> > + firmware = "u-boot";
> > + };
> > + };
>
> These loadables/firmware values didn't work on my chromebook_kevin. The
> Rockchip generator gets me:
>
> firmware = "atf-1";
> loadables = "u-boot", "atf-2", "atf-3";
>
> instead of the ones above (I didn't pass an op-tee file), and these work
> when I hardcode them in the binman definition.
I believe this is fixed now.
[..]
> > - def _scan_node(subnode, depth, in_images):
> > + def _scan_split_elf(subnode, rel_path):
> > + #data, input_fname, uniq = self.collect_contents_to_file(
> > + entry = Entry.Create(self.section, subnode, etype='section')
> > + entry.ReadNode()
> > + self._fit_sections[rel_path] = entry
>
> I did rebase this on top of dm-pull-8feb22 and my other two patches, and
> this series started failing on CheckEntries() complaining about the
> template node's entry's content not fitting inside the FIT section. I
> didn't really track it down but I guess it's due to adding it to
> self._entries here or somewhere else.
Yes I ended up creating a separate list of entries. See what you think.
[..]
> > + def _add_split_elf(self, orig_node, node, data, missing):
> > + """Add nodes for the ELF file, one per group of contiguous segments
> > +
> > + The existing placeholder node is replaced
> > +
> > + Args:
> > + orig_node (Node): Template node from the binman definition
> > + node (Node): Node to replace (in the FIT being built)
> > + data (bytes): ELF-format data to process (may be empty)
> > + """
> > + # If any pieces are missing, skip this. The missing entries will
> > show
> > + # an error
> > + if not missing:
> > + try:
> > + segments, entry = elf.read_segments(data)
> > + except ValueError as exc:
> > + self.Raise(f'Failed to read ELF file for {orig_node.path}:
> > {str(exc)}')
> > + parent = node.parent
> > + for (seq, start, data) in segments:
> > + node_name = orig_node.name[1:].replace('SEQ', str(seq + 1))
> > + subnode = parent.AddSubnode(node_name) #, before=node)
> > + self._loadables.append(subnode)
> > + for pname, prop in orig_node.props.items():
> > + if not pname.startswith('fit,'):
> > + subnode.AddData(pname, prop.bytes)
> > + elif pname == 'fit,load':
> > + subnode.AddInt('load', start)
> > + elif pname == 'fit,entry':
> > + if not seq:
> > + subnode.AddInt('entry', entry)
> > + elif pname == 'fit,data':
> > + subnode.AddData('data', data)
> > + elif pname != 'fit,operation':
> > + self.Raise(
> > + f"Unknown directive in '{subnode.path}':
> > '{pname}'")
> > +
> > + # Delete the template node as it has served its purpose
> > + node.Delete()
>
> Also, with the fit-as-section patches I get an error in SetImagePos()
> due to a template subentry in self._entries getting None as its node,
> thought it's related to the deletion here but didn't confirm.
Yes it cannot be deleted twice. Fixed in latest series.
>
> > +
> > def _BuildInput(self, fdt):
> > """Finish the FIT by adding the 'data' properties to it
> >
> > @@ -393,13 +605,36 @@ class Entry_fit(Entry):
> > New fdt contents (bytes)
> > """
> > for path, section in self._fit_sections.items():
> > - node = fdt.GetNode(path)
> > # Entry_section.ObtainContents() either returns True or
> > # raises an exception.
> > section.ObtainContents()
> > section.Pack(0)
> > data = section.GetData()
> > - node.AddData('data', data)
> > + missing_list = []
> > + section.CheckMissing(missing_list)
> > +
> > + node = fdt.GetNode(path)
> > + oper = self._get_operation(section._node)
> > + if oper == OP_GEN_FDT_NODES:
> > + node.AddData('data', data)
> > + elif oper == OP_SPLIT_ELF:
> > + self._add_split_elf(section._node, node, data,
> > + bool(missing_list))
> > +
> > + # Set up the 'firmware' and 'loadables' properties in all
> > + # 'configurations' nodes, but only if we are generating FDTs. Note
> > that
> > + # self._config_node is set in _scan_gen_fdt_nodes()
> > + node = fdt.GetNode('/configurations')
> > + if self._config_node:
> > + for subnode in node.subnodes:
> > + for pname, prop in self._config_node.props.items():
> > + if pname == 'fit,loadables':
> > + subnode.AddStringList(
> > + 'loadables',
> > + [node.name for node in self._loadables])
>
> As mentioned above, if "loadables" exists this should append to it.
I agree with that, but have not done it yet.
[..]
> > + def testFitSplitElfBadElf(self):
> > + """Test a FIT split-elf operation with an invalid ELF file"""
> > + TestFunctional._MakeInputFile('bad.elf', tools.GetBytes(100, 100))
> > + entry_args = {
> > + 'of-list': 'test-fdt1 test-fdt2',
> > + 'default-dt': 'test-fdt2',
> > + 'atf-bl31-path': 'bad.elf',
>
> This test fails for me with the fit-as-section patches but adding
> 'op-tee-path' here like the ones above makes it succeed. Though, you
> might have meant to test a missing op-tee here in addition to the bad.elf.
>
> Could use an independent test that checks a missing op-tee doesn't
> result in an error, as it should just omit the nodes in the FIT.
Should be fixed with new series.
[..]
Thanks for all the comments and ideas. I think this all need a little
more thought...
Regards,
Simon
