(dropping the two bounces from cc)
On Thu, 22 Dec 2022 at 13:18, Simon Glass <s...@chromium.org> wrote: > > Hi Jerome, > > On Thu, 22 Dec 2022 at 08:36, Jerome Forissier > <jerome.foriss...@linaro.org> wrote: > > > > > > > > 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). > > > > So, with: > > > > BL31=/path/to/bl31.elf TEE=/path/to/tee.bin make -C u-boot \ > > CROSS_COMPILE=aarch64-linux-gnu- CC=aarch64-linux-gnu-gcc \ > > HOSTCC=gcc BINMAN_DEBUG=1 BINMAN_VERBOSE=4 > > > > > > ...I get: > > > > Entry '/binman/simple-bin/fit/images/@tee-SEQ/tee-os' marked absent: uses > > v1 format which must be in a FIT > > > > > > More complete log at https://pastebin.com/UZzZeicQ > > Thanks. > > Is this file in the v1 binary format (with the custom header), or is > is a plain binary file? At present only the former is supported, as I > thought that it can only be the elf or the v1 binary format these > days? Actually can you please send me the tee.bin ? > > Regards, > Simon > > > > > > Thanks, > > -- > > Jerome > > > > > Signed-off-by: Simon Glass <s...@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://github.com/OP-TEE/optee_os 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 > > > + 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. > > > + > > > > > > > > > .. _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 > > > 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 > > > + > > > + 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 > > > + 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://github.com/OP-TEE/optee_os 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 > > > + 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): > > > + return len(data) >= 8 and data[0:5] == b'OPTE\x01' > > > + > > > + def ObtainContents(self, fake_size=0): > > > + super().ObtainContents(fake_size) > > > + 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') > > > + return True > > > + > > > + def read_elf_segments(self): > > > + data = self.GetData() > > > + if self.is_optee_bin(data): > > > + # OP-TEE v1 format (tee.bin) > > > + init_sz, start_hi, start_lo, _, paged_sz = ( > > > + struct.unpack_from('<5I', data, 0x8)) > > > + if paged_sz != 0: > > > + self.Raise("OP-TEE paged mode not supported") > > > + e_entry = (start_hi << 32) + start_lo > > > + p_addr = e_entry > > > + p_data = data[0x1c:] > > > + if len(p_data) != init_sz: > > > + self.Raise("Invalid OP-TEE file: size mismatch (expected > > > %#x, have %#x)" % > > > + (init_sz, len(p_data))) > > > + return [[0, p_addr, p_data]], e_entry > > > + return None > > > diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py > > > index f47a745f1e1..f893050e706 100644 > > > --- a/tools/binman/ftest.py > > > +++ b/tools/binman/ftest.py > > > @@ -112,6 +112,8 @@ REPACK_DTB_PROPS = ['orig-offset', 'orig-size'] > > > # Supported compression bintools > > > COMP_BINTOOLS = ['bzip2', 'gzip', 'lz4', 'lzma_alone', 'lzop', 'xz', > > > 'zstd'] > > > > > > +TEE_ADDR = 0x5678 > > > + > > > class TestFunctional(unittest.TestCase): > > > """Functional tests for binman > > > > > > @@ -219,6 +221,9 @@ class TestFunctional(unittest.TestCase): > > > TestFunctional._MakeInputFile('tee.elf', > > > tools.read_file(cls.ElfTestFile('elf_sections'))) > > > > > > + # Newer OP_TEE file in v1 binary format > > > + cls.make_tee_bin('tee.bin') > > > + > > > cls.comp_bintools = {} > > > for name in COMP_BINTOOLS: > > > cls.comp_bintools[name] = bintool.Bintool.create(name) > > > @@ -644,6 +649,14 @@ class TestFunctional(unittest.TestCase): > > > def ElfTestFile(cls, fname): > > > return os.path.join(cls._elf_testdir, fname) > > > > > > + @classmethod > > > + def make_tee_bin(cls, fname, paged_sz=0, extra_data=b''): > > > + init_sz, start_hi, start_lo, dummy = (len(U_BOOT_DATA), 0, > > > TEE_ADDR, 0) > > > + data = b'OPTE\x01xxx' + struct.pack('<5I', init_sz, start_hi, > > > start_lo, > > > + dummy, paged_sz) + > > > U_BOOT_DATA > > > + data += extra_data > > > + TestFunctional._MakeInputFile(fname, data) > > > + > > > def AssertInList(self, grep_list, target): > > > """Assert that at least one of a list of things is in a target > > > > > > @@ -6095,6 +6108,76 @@ fdt fdtmap Extract the > > > devicetree blob from the fdtmap > > > data = self._DoReadFile('262_absent.dts') > > > self.assertEqual(U_BOOT_DATA + U_BOOT_IMG_DATA, data) > > > > > > + def testPackTeeOsOptional(self): > > > + """Test that an image with an optional TEE binary can be > > > created""" > > > + entry_args = { > > > + 'tee-os-path': 'tee.elf', > > > + } > > > + data = self._DoReadFileDtb('263_tee_os_opt.dts', > > > + entry_args=entry_args)[0] > > > + self.assertEqual(U_BOOT_DATA + U_BOOT_IMG_DATA, data) > > > + > > > + def checkFitTee(self, dts, tee_fname): > > > + """Check that a tee-os entry works and returns data > > > + > > > + Args: > > > + dts (str): Device tree filename to use > > > + tee_fname (str): filename containing tee-os > > > + > > > + Returns: > > > + bytes: Image contents > > > + """ > > > + if not elf.ELF_TOOLS: > > > + self.skipTest('Python elftools not available') > > > + entry_args = { > > > + 'of-list': 'test-fdt1 test-fdt2', > > > + 'default-dt': 'test-fdt2', > > > + 'tee-os-path': tee_fname, > > > + } > > > + test_subdir = os.path.join(self._indir, TEST_FDT_SUBDIR) > > > + data = self._DoReadFileDtb(dts, entry_args=entry_args, > > > + extra_indirs=[test_subdir])[0] > > > + return data > > > + > > > + def testFitTeeOsOptionalFit(self): > > > + """Test an image with a FIT with an optional OP-TEE binary""" > > > + data = self.checkFitTee('264_tee_os_opt_fit.dts', 'tee.bin') > > > + > > > + # There should be only one node, holding the data set up in > > > SetUpClass() > > > + # for tee.bin > > > + dtb = fdt.Fdt.FromData(data) > > > + dtb.Scan() > > > + node = dtb.GetNode('/images/tee-1') > > > + self.assertEqual(TEE_ADDR, > > > + fdt_util.fdt32_to_cpu(node.props['load'].value)) > > > + self.assertEqual(TEE_ADDR, > > > + > > > fdt_util.fdt32_to_cpu(node.props['entry'].value)) > > > + self.assertEqual(U_BOOT_DATA, node.props['data'].bytes) > > > + > > > + def testFitTeeOsOptionalFitBad(self): > > > + """Test an image with a FIT with an optional OP-TEE binary""" > > > + with self.assertRaises(ValueError) as exc: > > > + self.checkFitTee('265_tee_os_opt_fit_bad.dts', 'tee.bin') > > > + self.assertIn( > > > + "Node '/binman/fit': subnode 'images/@tee-SEQ': Failed to > > > read ELF file: Magic number does not match", > > > + str(exc.exception)) > > > + > > > + def testFitTeeOsBad(self): > > > + """Test an OP-TEE binary with wrong formats""" > > > + self.make_tee_bin('tee.bad1', 123) > > > + with self.assertRaises(ValueError) as exc: > > > + self.checkFitTee('264_tee_os_opt_fit.dts', 'tee.bad1') > > > + self.assertIn( > > > + "Node '/binman/fit/images/@tee-SEQ/tee-os': OP-TEE paged > > > mode not supported", > > > + str(exc.exception)) > > > + > > > + self.make_tee_bin('tee.bad2', 0, b'extra data') > > > + with self.assertRaises(ValueError) as exc: > > > + self.checkFitTee('264_tee_os_opt_fit.dts', 'tee.bad2') > > > + self.assertIn( > > > + "Node '/binman/fit/images/@tee-SEQ/tee-os': Invalid OP-TEE > > > file: size mismatch (expected 0x4, have 0xe)", > > > + str(exc.exception)) > > > + > > > > > > if __name__ == "__main__": > > > unittest.main() > > > diff --git a/tools/binman/test/263_tee_os_opt.dts > > > b/tools/binman/test/263_tee_os_opt.dts > > > new file mode 100644 > > > index 00000000000..2e4ec24ac2c > > > --- /dev/null > > > +++ b/tools/binman/test/263_tee_os_opt.dts > > > @@ -0,0 +1,22 @@ > > > +// SPDX-License-Identifier: GPL-2.0+ > > > + > > > +/dts-v1/; > > > + > > > +/ { > > > + #address-cells = <1>; > > > + #size-cells = <1>; > > > + > > > + binman { > > > + u-boot { > > > + }; > > > + tee-os { > > > + /* > > > + * this results in nothing being added since only > > > the > > > + * .bin format is supported by this etype, unless > > > it is > > > + * part of a FIT > > > + */ > > > + }; > > > + u-boot-img { > > > + }; > > > + }; > > > +}; > > > diff --git a/tools/binman/test/264_tee_os_opt_fit.dts > > > b/tools/binman/test/264_tee_os_opt_fit.dts > > > new file mode 100644 > > > index 00000000000..ae44b433edf > > > --- /dev/null > > > +++ b/tools/binman/test/264_tee_os_opt_fit.dts > > > @@ -0,0 +1,33 @@ > > > +// SPDX-License-Identifier: GPL-2.0+ > > > + > > > +/dts-v1/; > > > + > > > +/ { > > > + #address-cells = <1>; > > > + #size-cells = <1>; > > > + > > > + binman { > > > + fit { > > > + description = "test-desc"; > > > + #address-cells = <1>; > > > + fit,fdt-list = "of-list"; > > > + > > > + images { > > > + @tee-SEQ { > > > + fit,operation = "split-elf"; > > > + description = "TEE"; > > > + type = "tee"; > > > + arch = "arm64"; > > > + os = "tee"; > > > + compression = "none"; > > > + fit,load; > > > + fit,entry; > > > + fit,data; > > > + > > > + tee-os { > > > + }; > > > + }; > > > + }; > > > + }; > > > + }; > > > +}; > > > diff --git a/tools/binman/test/265_tee_os_opt_fit_bad.dts > > > b/tools/binman/test/265_tee_os_opt_fit_bad.dts > > > new file mode 100644 > > > index 00000000000..7fa363cc199 > > > --- /dev/null > > > +++ b/tools/binman/test/265_tee_os_opt_fit_bad.dts > > > @@ -0,0 +1,40 @@ > > > +// SPDX-License-Identifier: GPL-2.0+ > > > + > > > +/dts-v1/; > > > + > > > +/ { > > > + #address-cells = <1>; > > > + #size-cells = <1>; > > > + > > > + binman { > > > + fit { > > > + description = "test-desc"; > > > + #address-cells = <1>; > > > + fit,fdt-list = "of-list"; > > > + > > > + images { > > > + @tee-SEQ { > > > + fit,operation = "split-elf"; > > > + description = "TEE"; > > > + type = "tee"; > > > + arch = "arm64"; > > > + os = "tee"; > > > + compression = "none"; > > > + fit,load; > > > + fit,entry; > > > + fit,data; > > > + > > > + tee-os { > > > + }; > > > + > > > + /* > > > + * mess up the ELF data by adding > > > + * another bit of data at the end > > > + */ > > > + u-boot { > > > + }; > > > + }; > > > + }; > > > + }; > > > + }; > > > +};
