Re: [PATCH v8 06/13] binman: Support new op-tee binary format

[Quentin Schulz <quentin.schulz@theobroma-systems.com>]

Hi Simon,

On 12/22/22 00:07, Simon Glass wrote:
> OP-TEE has a format with a binary header that can be used instead of the
> ELF file. With newer versions of OP-TEE this may be required on some
> platforms.
> 
> Add support for this in binman. First, add a method to obtain the ELF
> sections from an entry, then use that in the FIT support. We then end up
> with the ability to support both types of OP-TEE files, depending on which
> one is passed in with the entry argument (TEE=xxx in the U-Boot build).
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
> (no changes since v7)
> 
> Changes in v7:
> - Correct missing test coverage
> 
> Changes in v6:
> - Update op-tee to support new v1 binary header
> 
>   tools/binman/entries.rst                     | 35 ++++++++-
>   tools/binman/entry.py                        | 13 +++
>   tools/binman/etype/fit.py                    | 69 +++++++++-------
>   tools/binman/etype/section.py                |  9 +++
>   tools/binman/etype/tee_os.py                 | 68 +++++++++++++++-
>   tools/binman/ftest.py                        | 83 ++++++++++++++++++++
>   tools/binman/test/263_tee_os_opt.dts         | 22 ++++++
>   tools/binman/test/264_tee_os_opt_fit.dts     | 33 ++++++++
>   tools/binman/test/265_tee_os_opt_fit_bad.dts | 40 ++++++++++
>   9 files changed, 340 insertions(+), 32 deletions(-)
>   create mode 100644 tools/binman/test/263_tee_os_opt.dts
>   create mode 100644 tools/binman/test/264_tee_os_opt_fit.dts
>   create mode 100644 tools/binman/test/265_tee_os_opt_fit_bad.dts
> 
> diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst
> index b2ce7960d3b..a3e4493a44f 100644
> --- a/tools/binman/entries.rst
> +++ b/tools/binman/entries.rst
> @@ -1508,12 +1508,45 @@ Entry: tee-os: Entry containing an OP-TEE Trusted OS (TEE) blob
>   
>   Properties / Entry arguments:
>       - tee-os-path: Filename of file to read into entry. This is typically
> -        called tee-pager.bin
> +        called tee.bin or tee.elf
>   
>   This entry holds the run-time firmware, typically started by U-Boot SPL.
>   See the U-Boot README for your architecture or board for how to use it. See
>   https://urldefense.com/v3/__https://github.com/OP-TEE/optee_os__;!!OOPJP91ZZw!kx6SLv4sPusmg1TyYMw-Ho5G9jxeVMf9HOYx_4yq3ZNl_TpoGJoUyICZMgEiv0Zd6l_Bl8f5OAFYyJxm8wDUevhARIs$  for more information about OP-TEE.
>   
> +Note that if the file is in ELF format, it must go in a FIT. In that case,
> +this entry will mark itself as absent, providing the data only through the
> +read_elf_segments() method.
> +
> +Marking this entry as absent means that it if is used in the wrong context
> +it can be automatically dropped. Thus it is possible to add anb OP-TEE entry

s/anb/an/

> +like this::
> +
> +    binman {
> +        tee-os {
> +        };
> +    };
> +
> +and pass either an ELF or plain binary in with -a tee-os-path <filename>
> +and have binman do the right thing:
> +
> +   - include the entry if tee.bin is provided and it doesn't have the v1
> +     header
> +   - drop it otherwise
> +

Is there an actual usecase for this? (sorry if this was mentioned in the 
earlier versions of the patch) Are we expecting to be able to append the 
content of tee-os to some raw binary instead of putting OP-TEE OS in a 
u-boot.itb image?

> +When used within a FIT, we can do::
> +
> +    binman {
> +        fit {
> +            tee-os {
> +            };
> +        };
> +    };
> +
> +which will split the ELF into separate nodes for each segment, if an ELF
> +file is provide (see Flat Image Tree / FIT), or produce a single node if

s/provide/provided/

You can use a reference here since we have a _etype_fit target for "Flat 
Image Tree / FIT".

> +the binary v1 format is provided.
> +
>   

I'm a bit worried this is OP-TEE OS specific? We could also point to the 
documentation here: 
https://optee.readthedocs.io/en/latest/architecture/core.html#partitioning-of-the-binary?

>   
>   .. _etype_text:
> diff --git a/tools/binman/entry.py b/tools/binman/entry.py
> index 637aece3705..de51d295891 100644
> --- a/tools/binman/entry.py
> +++ b/tools/binman/entry.py
> @@ -1290,3 +1290,16 @@ features to produce new behaviours.
>       def mark_absent(self, msg):
>           tout.info("Entry '%s' marked absent: %s" % (self._node.path, msg))
>           self.absent = True
> +
> +    def read_elf_segments(self):
> +        """Read segments from an entry that can generate an ELF file
> +
> +        Returns:
> +            tuple:
> +                list of segments, each:
> +                    int: Segment number (0 = first)
> +                    int: Start address of segment in memory
> +                    bytes: Contents of segment
> +                int: entry address of ELF file
> +        """
> +        return None

Does it really make sense to have this function available to all Entry 
objects?

> diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
> index 8ad4f3a8a83..21c769a1cbe 100644
> --- a/tools/binman/etype/fit.py
> +++ b/tools/binman/etype/fit.py
> @@ -540,41 +540,34 @@ class Entry_fit(Entry_section):
>                       else:
>                           self.Raise("Generator node requires 'fit,fdt-list' property")
>   
> -        def _gen_split_elf(base_node, node, elf_data, missing):
> +        def _gen_split_elf(base_node, node, segments, entry_addr):
>               """Add nodes for the ELF file, one per group of contiguous segments
>   
>               Args:
>                   base_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)
> -                missing (bool): True if any of the data is missing
>   
> +                segments, entry_addr

Please document.

> +
> +                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_loadable_segments(elf_data)
> -                except ValueError as exc:
> -                    self._raise_subnode(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 seq == 0:
> -                                    fsw.property_u32('entry', entry)
> -                            elif pname == 'fit,data':
> -                                fsw.property('data', bytes(data))
> -                            elif pname != 'fit,operation':
> -                                self._raise_subnode(
> -                                    node, f"Unknown directive '{pname}'")
> +            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 seq == 0:
> +                                fsw.property_u32('entry', entry_addr)
> +                        elif pname == 'fit,data':
> +                            fsw.property('data', bytes(data))
> +                        elif pname != 'fit,operation':
> +                            self._raise_subnode(
> +                                node, f"Unknown directive '{pname}'")
>   
>           def _gen_node(base_node, node, depth, in_images, entry):
>               """Generate nodes from a template
> @@ -598,6 +591,8 @@ class Entry_fit(Entry_section):
>                   depth (int): Current node depth (0 is the base 'fit' node)
>                   in_images (bool): True if this is inside the 'images' node, so
>                       that 'data' properties should be generated
> +                entry (entry_Section): Entry for the second containing the

s/second/section/ ?

> +                    contents of this node
>               """
>               oper = self._get_operation(base_node, node)
>               if oper == OP_GEN_FDT_NODES:
> @@ -609,10 +604,24 @@ class Entry_fit(Entry_section):
>                   missing_list = []
>                   entry.ObtainContents()
>                   entry.Pack(0)
> -                data = entry.GetData()
>                   entry.CheckMissing(missing_list)
>   
> -                _gen_split_elf(base_node, node, data, bool(missing_list))
> +                # If any pieces are missing, skip this. The missing entries will
> +                # show an error
> +                if not missing_list:
> +                    segs = entry.read_elf_segments()
> +                    if segs:
> +                        segments, entry_addr = segs
> +                    else:
> +                        elf_data = entry.GetData()
> +                        try:
> +                            segments, entry_addr = (
> +                                    elf.read_loadable_segments(elf_data))
> +                        except ValueError as exc:
> +                            self._raise_subnode(
> +                                node, f'Failed to read ELF file: {str(exc)}')
> +
> +                    _gen_split_elf(base_node, node, segments, entry_addr)
>   
>           def _add_node(base_node, depth, node):
>               """Add nodes to the output FIT
> diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py
> index dcb7a062047..57bfee0b286 100644
> --- a/tools/binman/etype/section.py
> +++ b/tools/binman/etype/section.py
> @@ -948,3 +948,12 @@ class Entry_section(Entry):
>           super().AddBintools(btools)
>           for entry in self._entries.values():
>               entry.AddBintools(btools)
> +
> +    def read_elf_segments(self):
> +        entries = self.GetEntries()
> +
> +        # If the section only has one entry, see if it can provide ELF segments
> +        if len(entries) == 1:
> +            for entry in entries.values():
> +                return entry.read_elf_segments()
> +        return None
> diff --git a/tools/binman/etype/tee_os.py b/tools/binman/etype/tee_os.py
> index 6ce4b672de4..687acd4689f 100644
> --- a/tools/binman/etype/tee_os.py
> +++ b/tools/binman/etype/tee_os.py
> @@ -4,19 +4,85 @@
>   # Entry-type module for OP-TEE Trusted OS firmware blob
>   #
>   
> +import struct
> +
>   from binman.etype.blob_named_by_arg import Entry_blob_named_by_arg
> +from binman import elf
>   
>   class Entry_tee_os(Entry_blob_named_by_arg):
>       """Entry containing an OP-TEE Trusted OS (TEE) blob
>   
>       Properties / Entry arguments:
>           - tee-os-path: Filename of file to read into entry. This is typically
> -            called tee-pager.bin
> +            called tee.bin or tee.elf
>   
>       This entry holds the run-time firmware, typically started by U-Boot SPL.
>       See the U-Boot README for your architecture or board for how to use it. See
>       https://urldefense.com/v3/__https://github.com/OP-TEE/optee_os__;!!OOPJP91ZZw!kx6SLv4sPusmg1TyYMw-Ho5G9jxeVMf9HOYx_4yq3ZNl_TpoGJoUyICZMgEiv0Zd6l_Bl8f5OAFYyJxm8wDUevhARIs$  for more information about OP-TEE.
> +
> +    Note that if the file is in ELF format, it must go in a FIT. In that case,
> +    this entry will mark itself as absent, providing the data only through the
> +    read_elf_segments() method.
> +
> +    Marking this entry as absent means that it if is used in the wrong context
> +    it can be automatically dropped. Thus it is possible to add anb OP-TEE entry
> +    like this::
> +
> +        binman {
> +            tee-os {
> +            };
> +        };
> +
> +    and pass either an ELF or plain binary in with -a tee-os-path <filename>
> +    and have binman do the right thing:
> +
> +       - include the entry if tee.bin is provided and it doesn't have the v1

"does NOT" ??

> +         header
> +       - drop it otherwise
> +
> +    When used within a FIT, we can do::
> +
> +        binman {
> +            fit {
> +                tee-os {
> +                };
> +            };
> +        };
> +
> +    which will split the ELF into separate nodes for each segment, if an ELF
> +    file is provide (see Flat Image Tree / FIT), or produce a single node if
> +    the binary v1 format is provided.
>       """
>       def __init__(self, section, etype, node):
>           super().__init__(section, etype, node, 'tee-os')
>           self.external = True
> +
> +    @staticmethod
> +    def is_optee_bin(data):

Here you are checking it's a binary with v1 header, so please explicit 
in the function name. (there's already a v2 header available FWIW).

> +        return len(data) >= 8 and data[0:5] == b'OPTE\x01'
> +
> +    def ObtainContents(self, fake_size=0):
> +        super().ObtainContents(fake_size)

Do not silence the return code of the parent class here? I know it's 
only returning True ATM but nothing guarantees it'll stay this way.

> +        if not self.missing:
> +            if elf.is_valid(self.data):
> +                self.mark_absent('uses Elf format which must be in a FIT')
> +            elif self.is_optee_bin(self.data):
> +                self.mark_absent('uses v1 format which must be in a FIT')

What should this support then if neither ELF not binary with v1 header 
are supported? I don't see support for binary with v2 header anywhere so 
I'm quite confused by what this piece of code is supposed to handle?

I'm also very much against displaying a warning if the user has TEE set 
in their environment and the file exists but it won't be used in the 
final image. If it's not compatible based on the binman DT node, error 
out. If it's the wrong file or it's missing, error out. Is there some 
scenario where displaying a warning (and/or removing the entry with 
mark_absent like you did here) make sense?

In any case, thanks Simon and Jerome for looking into this, it's a 
bigger task than I had anticipated but am looking forward to this 
Rockchip-specific behavior to be dropped from mainline :)

Cheers,
Quentin