Re: [PATCH v2 20/25] binman: Support splitting an ELF file into multiple nodes

[Alper Nebi Yasak <alpernebiyasak@gmail.com>]

On 24/02/2022 02:00, 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.
> 
> Also add checks that the fit,xxx directives are valid.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
> Changes in v2:
> - Rewrite this to use the new FIT entry-type implementation
> - Rename op-tee to tee-os
> 
>  tools/binman/entries.rst                     | 146 ++++++++++++
>  tools/binman/etype/fit.py                    | 230 ++++++++++++++++++-
>  tools/binman/ftest.py                        | 147 ++++++++++++
>  tools/binman/test/226_fit_split_elf.dts      |  67 ++++++
>  tools/binman/test/227_fit_bad_dir.dts        |   9 +
>  tools/binman/test/228_fit_bad_dir_config.dts |   9 +
>  6 files changed, 598 insertions(+), 10 deletions(-)
>  create mode 100644 tools/binman/test/226_fit_split_elf.dts
>  create mode 100644 tools/binman/test/227_fit_bad_dir.dts
>  create mode 100644 tools/binman/test/228_fit_bad_dir_config.dts
> 
> [...]
> 
> diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
> index 30b20a07a2..63d552ed19 100644> > [...]
> 
> @@ -154,6 +159,149 @@ class Entry_fit(Entry_section):
>  
>      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

segmnet -> segment, missed it the first time around.

> +
> +        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
> +
> +        fit,loadables
> +            Generates a `loadable = <...>` property with a list of the generated
> +            nodes (including all nodes if this operation is used multiple times)
> 
> [...]
>  
> @@ -363,11 +514,20 @@ class Entry_fit(Entry_section):
>                      fname = tools.get_input_filename(fdt_fname + '.dtb')
>                      with fsw.add_node(node_name):
>                          for pname, prop in node.props.items():
> -                            val = prop.bytes.replace(
> -                                b'NAME', tools.to_bytes(fdt_fname))
> -                            val = val.replace(
> -                                b'SEQ', tools.to_bytes(str(seq + 1)))
> -                            fsw.property(pname, val)
> +                            if pname == 'fit,loadables':
> +                                val = '\0'.join(self._loadables) + '\0'
> +                                fsw.property('loadables', val.encode('utf-8'))
> +                            elif pname == 'fit,operation':
> +                                pass
> +                            elif pname.startswith('fit,'):
> +                                self._raise(base_node, node,
> +                                            f"Unknown directive '{pname}'")
> +                            else:
> +                                val = prop.bytes.replace(
> +                                    b'NAME', tools.to_bytes(fdt_fname))
> +                                val = val.replace(
> +                                    b'SEQ', tools.to_bytes(str(seq + 1)))
> +                                fsw.property(pname, val)

I get two distinct "loadables" properties with this config fragment,
and fdtget and mkimage -l only considers the first one:

  @config-SEQ {
     description = "NAME.dtb";
     fdt = "fdt-SEQ";
     firmware = "atf-1";
     loadables = "u-boot";
     fit,loadables;
  };

  $ fdtget simple-bin.fit.fit /configurations/config-1 -p
  description
  fdt
  firmware
  loadables
  loadables

  $ fdtget simple-bin.fit.fit /configurations/config-1 loadables
  u-boot

  $ mkimage -l simple-bin.fit.fit
  FIT description: FIT image for U-Boot with bl31 (TF-A)
  Created:         Thu Feb 24 17:36:36 2022
   Image 0 (u-boot)
    Description:  U-Boot (64-bit)
    Created:      Thu Feb 24 17:36:36 2022
    Type:         Standalone Program
    Compression:  uncompressed
    Data Size:    727192 Bytes = 710.15 KiB = 0.69 MiB
    Architecture: AArch64
    Load Address: 0x00200000
    Entry Point:  unavailable
   Image 1 (atf-1)
    Description:  ARM Trusted Firmware
    Created:      Thu Feb 24 17:36:36 2022
    Type:         Firmware
    Compression:  uncompressed
    Data Size:    139342 Bytes = 136.08 KiB = 0.13 MiB
    Architecture: AArch64
    OS:           ARM Trusted Firmware
    Load Address: 0x00040000
   Image 2 (atf-2)
    Description:  ARM Trusted Firmware
    Created:      Thu Feb 24 17:36:36 2022
    Type:         Firmware
    Compression:  uncompressed
    Data Size:    8024 Bytes = 7.84 KiB = 0.01 MiB
    Architecture: AArch64
    OS:           ARM Trusted Firmware
    Load Address: 0xff3b0000
   Image 3 (atf-3)
    Description:  ARM Trusted Firmware
    Created:      Thu Feb 24 17:36:36 2022
    Type:         Firmware
    Compression:  uncompressed
    Data Size:    8192 Bytes = 8.00 KiB = 0.01 MiB
    Architecture: AArch64
    OS:           ARM Trusted Firmware
    Load Address: 0xff8c0000
   Image 4 (fdt-1)
    Description:  fdt-rk3399-gru-bob
    Created:      Thu Feb 24 17:36:36 2022
    Type:         Flat Device Tree
    Compression:  uncompressed
    Data Size:    69392 Bytes = 67.77 KiB = 0.07 MiB
    Architecture: Unknown Architecture
   Default Configuration: 'config-1'
   Configuration 0 (config-1)
    Description:  rk3399-gru-bob.dtb
    Kernel:       unavailable
    Firmware:     atf-1
    FDT:          fdt-1
    Loadables:    u-boot

I didn't test if my chromebook_kevin boots in this double-loadables
configuration, though.

>  
>                          # Add data for 'images' nodes (but not 'config')
>                          if depth == 1 and in_images:
> @@ -380,7 +540,43 @@ class Entry_fit(Entry_section):
>                      else:
>                          self.Raise("Generator node requires 'fit,fdt-list' property")
>  
> -        def _gen_node(base_node, node, depth, in_images):
> +        def _gen_split_elf(base_node, node, elf_data, missing):
> +            """Add nodes for the ELF file, one per group of contiguous segments
> +
> +            Args:
> +                node (Node): Template node from the binman definition

node -> base_node

> +                node (Node): Node to replace (in the FIT being built)
> +                data (bytes): ELF-format data to process (may be empty)
> +                missing (bool): True if any of the data is missing
> +
> +            """
> +            # If any pieces are missing, skip this. The missing entries will
> +            # show an error
> +            if not missing:
> +                try:
> +                    segments, entry = elf.read_loadable_segments(elf_data)
> +                except ValueError as exc:
> +                    self._raise(base_node, node,
> +                                f'Failed to read ELF file: {str(exc)}')
> +                for (seq, start, data) in segments:
> +                    node_name = node.name[1:].replace('SEQ', str(seq + 1))
> +                    with fsw.add_node(node_name):
> +                        loadables.append(node_name)
> +                        for pname, prop in node.props.items():
> +                            if not pname.startswith('fit,'):
> +                                fsw.property(pname, prop.bytes)
> +                            elif pname == 'fit,load':
> +                                fsw.property_u32('load', start)
> +                            elif pname == 'fit,entry':
> +                                if not seq:

I'd prefer seq == 0 as it's a number.

> +                                    fsw.property_u32('entry', entry)
> +                            elif pname == 'fit,data':
> +                                fsw.property('data', bytes(data))
> +                            elif pname != 'fit,operation':
> +                                self._raise(base_node, node,
> +                                            f"Unknown directive '{pname}'")
> +
> +        def _gen_node(base_node, node, depth, in_images, entry):
>              """Generate nodes from a template
>  
>              This creates one node for each member of self._fdts using the
> [...]