[PATCH 3/3] binman: Build FIT image subentries with the section etype

[Simon Glass]

Hi Alper,

On Tue, 25 Aug 2020 at 12:01, Alper Nebi Yasak <alpernebiyasak at gmail.com> wrote:
>
> When reading subentries of each image, the FIT entry type directly
> concatenates their contents without padding them according to their
> offset, size, align, align-size, align-end, pad-before, pad-after
> properties.
>
> This patch makes sure these properties are respected by offloading this
> image-data building to the section etype, where each subnode of the
> "images" node is processed as a section. Alignments and offsets are
> respective to the beginning of each image. For example, the following
> fragment can end up having "u-boot-spl" start at 0x88 within the final
> FIT binary, while "u-boot" would then end up starting at e.g. 0x20088.
>
>         fit {
>                 description = "example";
>
>                 images {
>                         kernel-1 {
>                                 description = "U-Boot with SPL";
>                                 type = "kernel";
>                                 arch = "arm64";
>                                 os = "linux";
>                                 compression = "none";
>
>                                 u-boot-spl {
>                                 };
>                                 u-boot {
>                                         align = <0x10000>;
>                                 };
>                         };
>                 };
>         }
>
> Signed-off-by: Alper Nebi Yasak <alpernebiyasak at gmail.com>
> ---
>
>  tools/binman/etype/fit.py                     | 22 +++----
>  tools/binman/ftest.py                         | 24 ++++++++
>  .../test/166_fit_image_subentry_alignment.dts | 57 +++++++++++++++++++
>  3 files changed, 93 insertions(+), 10 deletions(-)
>  create mode 100644 tools/binman/test/166_fit_image_subentry_alignment.dts

This is a nice enhancement.

A few nits below.

>
> diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
> index 75712f4409..f136a2c254 100644
> --- a/tools/binman/etype/fit.py
> +++ b/tools/binman/etype/fit.py
> @@ -62,7 +62,7 @@ class Entry_fit(Entry):
>          """
>          super().__init__(section, etype, node)
>          self._fit = None
> -        self._fit_content = defaultdict(list)
> +        self._fit_images = {}

Can you add a Properties comment above for this?

>          self._fit_props = {}
>
>      def ReadNode(self):
> @@ -91,15 +91,18 @@ class Entry_fit(Entry):
>
>              rel_path = node.path[len(base_node.path):]
>              has_images = depth == 2 and rel_path.startswith('/images/')
> +            if has_images:
> +                entry = Entry.Create(self.section, node, etype='section')
> +                entry.ReadNode()
> +                self._fit_images[rel_path] = entry
> +
>              for subnode in node.subnodes:
>                  if has_images and not (subnode.name.startswith('hash') or
>                                         subnode.name.startswith('signature')):
>                      # This is a content node. We collect all of these together
>                      # and put them in the 'data' property. They do not appear
>                      # in the FIT.

This comment should move along with the code you moved. Perhaps here
you can mention that it is not a content node.

> -                    entry = Entry.Create(self.section, subnode)
> -                    entry.ReadNode()
> -                    self._fit_content[rel_path].append(entry)
> +                    pass
>                  else:
>                      with fsw.add_node(subnode.name):
>                          _AddNode(base_node, depth + 1, subnode)
> @@ -150,13 +153,12 @@ class Entry_fit(Entry):
>          Returns:
>              New fdt contents (bytes)
>          """
> -        for path, entries in self._fit_content.items():
> +        for path, image in self._fit_images.items():

I think section is a better name than image. In binman, 'image' means
the final output file, containing a collection of binaries. The FIT
itself therefore ends up in an image, so calling the components of the
FIT 'images' is confusing.

>              node = fdt.GetNode(path)
> -            data = b''
> -            for entry in entries:
> -                if not entry.ObtainContents():
> -                    return False
> -                data += entry.GetData()
> +            if not image.ObtainContents():
> +                return False
> +            image.Pack(0)
> +            data = image.GetData()
>              node.AddData('data', data)
>
>          fdt.Sync(auto_resize=True)

[..]

Please also do check code coverage: binman test -T

Regards,
Simon