Binman tries to expand some entries into parts that make it up, e.g. 'u-boot' into a 'u-boot-expanded' section that contains 'u-boot-nodtb' and 'u-boot-dtb'. Entries with child entries must call ExpandEntries() on them to build a correct image, as it's possible that unexpanded child entries have no data of their own. The FIT entry type doesn't currently do this, which means putting a "u-boot" entry inside it doesn't work as expected.

Implement ExpandEntries() for FIT and add a copy of a simple FIT image test that checks subentry expansion in FIT entries.

Signed-off-by: Alper Nebi Yasak alpernebiyasak@gmail.com ---

tools/binman/etype/fit.py | 5 +++++ tools/binman/ftest.py | 45 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+)

diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py index 6ad4a686df55..38bc2a2d784e 100644 --- a/tools/binman/etype/fit.py +++ b/tools/binman/etype/fit.py @@ -237,6 +237,11 @@ def _AddNode(base_node, depth, node): self._fdt = Fdt.FromData(fdt.as_bytearray()) self._fdt.Scan()

+ def ExpandEntries(self): + super().ExpandEntries() + for section in self._fit_sections.values(): + section.ExpandEntries() + def ObtainContents(self): """Obtain the contents of the FIT

diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 5400f76c676a..b0f548057980 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -61,6 +61,9 @@ U_BOOT_NODTB_DATA = b'nodtb with microcode pointer somewhere in here' U_BOOT_SPL_NODTB_DATA = b'splnodtb with microcode pointer somewhere in here' U_BOOT_TPL_NODTB_DATA = b'tplnodtb with microcode pointer somewhere in here' +U_BOOT_EXP_DATA = U_BOOT_NODTB_DATA + U_BOOT_DTB_DATA +U_BOOT_SPL_EXP_DATA = U_BOOT_SPL_NODTB_DATA + U_BOOT_SPL_DTB_DATA +U_BOOT_TPL_EXP_DATA = U_BOOT_TPL_NODTB_DATA + U_BOOT_TPL_DTB_DATA FSP_DATA = b'fsp' CMC_DATA = b'cmc' VBT_DATA = b'vbt' @@ -3755,6 +3758,48 @@ def testSimpleFit(self): self.assertEqual(len(U_BOOT_DATA), int(data_sizes[0].split()[0])) self.assertEqual(len(U_BOOT_SPL_DTB_DATA), int(data_sizes[1].split()[0]))

+ def testSimpleFitExpandsSubentries(self): + """Test that FIT images expand their subentries""" + data = self._DoReadFileDtb('161_fit.dts', use_expanded=True)[0] + self.assertEqual(U_BOOT_EXP_DATA, data[:len(U_BOOT_EXP_DATA)]) + self.assertEqual(U_BOOT_NODTB_DATA, data[-len(U_BOOT_NODTB_DATA):]) + fit_data = data[len(U_BOOT_EXP_DATA):-len(U_BOOT_NODTB_DATA)] + + # The data should be inside the FIT + dtb = fdt.Fdt.FromData(fit_data) + dtb.Scan() + fnode = dtb.GetNode('/images/kernel') + self.assertIn('data', fnode.props) + + fname = os.path.join(self._indir, 'fit_data.fit') + tools.WriteFile(fname, fit_data) + out = tools.Run('dumpimage', '-l', fname) + + # Check a few features to make sure the plumbing works. We don't need + # to test the operation of mkimage or dumpimage here. First convert the + # output into a dict where the keys are the fields printed by dumpimage + # and the values are a list of values for each field + lines = out.splitlines() + + # Converts "Compression: gzip compressed" into two groups: + # 'Compression' and 'gzip compressed' + re_line = re.compile(r'^ *([^:]*)(?:: *(.*))?$') + vals = collections.defaultdict(list) + for line in lines: + mat = re_line.match(line) + vals[mat.group(1)].append(mat.group(2)) + + self.assertEquals('FIT description: test-desc', lines[0]) + self.assertIn('Created:', lines[1]) + self.assertIn('Image 0 (kernel)', vals) + self.assertIn('Hash value', vals) + data_sizes = vals.get('Data Size') + self.assertIsNotNone(data_sizes) + self.assertEqual(2, len(data_sizes)) + # Format is "50 Bytes = 0.00 KiB = 0.00 MiB" so take the first word + self.assertEqual(len(U_BOOT_EXP_DATA), int(data_sizes[0].split()[0])) + self.assertEqual(len(U_BOOT_SPL_DTB_DATA), int(data_sizes[1].split()[0])) + def testFitExternal(self): """Test an image with an FIT with external images""" data = self._DoReadFile('162_fit_external.dts')

Binman keeps track of binary tools each entry wants to use. The implementation of this for the FIT entry only adds "mkimage", but not the tools that would be used by its subentries.

Register the binary tools that FIT subentries will use in addition to the one FIT itself uses, and check their existence by copying the appropriate method from Section entry type. Also add tests that check if these subentries can use and warn about binary tools.

Signed-off-by: Alper Nebi Yasak alpernebiyasak@gmail.com ---

tools/binman/etype/fit.py | 14 +++++++ tools/binman/ftest.py | 25 ++++++++++++ .../binman/test/220_fit_subentry_bintool.dts | 39 +++++++++++++++++++ 3 files changed, 78 insertions(+) create mode 100644 tools/binman/test/220_fit_subentry_bintool.dts

diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py index 38bc2a2d784e..9dffcd5adbd6 100644 --- a/tools/binman/etype/fit.py +++ b/tools/binman/etype/fit.py @@ -311,4 +311,18 @@ def SetAllowMissing(self, allow_missing): section.SetAllowMissing(allow_missing)

def AddBintools(self, tools): + for section in self._fit_sections.values(): + section.AddBintools(tools) self.mkimage = self.AddBintool(tools, 'mkimage') + + def check_missing_bintools(self, missing_list): + """Check if any entries in this section have missing bintools + + If there are missing bintools, these are added to the list + + Args: + missing_list: List of Bintool objects to be added to + """ + super().check_missing_bintools(missing_list) + for entry in self._fit_sections.values(): + entry.check_missing_bintools(missing_list) diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index b0f548057980..ce0dcf7c3e02 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -5163,6 +5163,31 @@ def testListWithGenNode(self): finally: shutil.rmtree(tmpdir)

+ def testFitSubentryUsesBintool(self): + """Test that binman FIT subentries can use bintools""" + command.test_result = self._HandleGbbCommand + entry_args = { + 'keydir': 'devkeys', + 'bmpblk': 'bmpblk.bin', + } + data, _, _, _ = self._DoReadFileDtb('220_fit_subentry_bintool.dts', + entry_args=entry_args) + + expected = (GBB_DATA + GBB_DATA + tools.GetBytes(0, 8) + + tools.GetBytes(0, 0x2180 - 16)) + self.assertIn(expected, data) + + def testFitSubentryMissingBintool(self): + """Test that binman reports missing bintools for FIT subentries""" + entry_args = { + 'keydir': 'devkeys', + } + with test_util.capture_sys_output() as (_, stderr): + self._DoTestFile('220_fit_subentry_bintool.dts', + force_missing_bintools='futility', entry_args=entry_args) + err = stderr.getvalue() + self.assertRegex(err, + "Image 'main-section'.*missing bintools.*: futility")

if __name__ == "__main__": unittest.main() diff --git a/tools/binman/test/220_fit_subentry_bintool.dts b/tools/binman/test/220_fit_subentry_bintool.dts new file mode 100644 index 000000000000..6e29d41eeb33 --- /dev/null +++ b/tools/binman/test/220_fit_subentry_bintool.dts @@ -0,0 +1,39 @@ +// SPDX-License-Identifier: GPL-2.0+ + +/dts-v1/; + +/ { + #address-cells = <1>; + #size-cells = <1>; + + binman { + fit { + description = "test-desc"; + #address-cells = <1>; + + images { + test { + description = "Something using a bintool"; + type = "kernel"; + arch = "arm"; + os = "linux"; + compression = "gzip"; + load = <00000000>; + entry = <00000000>; + + gbb { + size = <0x2180>; + }; + }; + }; + + configurations { + default = "conf-1"; + conf-1 { + description = "Boot bintool output"; + kernel = "kernel"; + }; + }; + }; + }; +};

Binman can check for missing binary tools and prints warnings if anything required for an image is missing. The implementation of this for the Section entry only checks the subentries, presumably because Section does not use any binary tools itself. However, this means the check is also skipped for subclasses of Section which might need binary tools.

Make sure missing binary tools are checked for subclasses of the Section entry type as well, by calling the parent class' implementation in the relevant Section method.

Signed-off-by: Alper Nebi Yasak alpernebiyasak@gmail.com --- This starts being tested by testFitMissing in the next patch when FIT becomes a Section subclass.

tools/binman/etype/section.py | 1 + 1 file changed, 1 insertion(+)

diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py index bb375e9063df..213a510a8d5b 100644 --- a/tools/binman/etype/section.py +++ b/tools/binman/etype/section.py @@ -840,6 +840,7 @@ def check_missing_bintools(self, missing_list): Args: missing_list: List of Bintool objects to be added to """ + super().check_missing_bintools(missing_list) for entry in self._entries.values(): entry.check_missing_bintools(missing_list)

The binman FIT entry type shares some code with the Section entry type. This shared code is bound to grow, since FIT entries are conceptually a variation of Section entries.

Make FIT entry type a subclass of Section entry type, simplifying it a bit and providing us the features that Section implements. Also fix the subentry alignment test which now attempts to write symbols to a nonexistent SPL ELF test file by creating it first.

Signed-off-by: Alper Nebi Yasak alpernebiyasak@gmail.com ---

tools/binman/etype/fit.py | 70 +++++++++++---------------------------- tools/binman/ftest.py | 1 + 2 files changed, 20 insertions(+), 51 deletions(-)

diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py index 9dffcd5adbd6..0f8c88a9720e 100644 --- a/tools/binman/etype/fit.py +++ b/tools/binman/etype/fit.py @@ -9,11 +9,12 @@ import libfdt

from binman.entry import Entry, EntryArg +from binman.etype.section import Entry_section from dtoc import fdt_util from dtoc.fdt import Fdt from patman import tools

-class Entry_fit(Entry): +class Entry_fit(Entry_section): """Flat Image Tree (FIT)

This calls mkimage to create a FIT (U-Boot Flat Image Tree) based on the @@ -112,15 +113,15 @@ def __init__(self, section, etype, node): """ Members: _fit: FIT file being built - _fit_sections: dict: + _entries: dict from Entry_section: key: relative path to entry Node (from the base of the FIT) value: Entry_section object comprising the contents of this node """ super().__init__(section, etype, node) self._fit = None - self._fit_sections = {} self._fit_props = {} + for pname, prop in self._node.props.items(): if pname.startswith('fit,'): self._fit_props[pname] = prop @@ -185,7 +186,7 @@ def _AddNode(base_node, depth, node): # 'data' property later. entry = Entry.Create(self.section, node, etype='section') entry.ReadNode() - self._fit_sections[rel_path] = entry + self._entries[rel_path] = entry

for subnode in node.subnodes: if has_images and not (subnode.name.startswith('hash') or @@ -237,18 +238,19 @@ def _AddNode(base_node, depth, node): self._fdt = Fdt.FromData(fdt.as_bytearray()) self._fdt.Scan()

- def ExpandEntries(self): - super().ExpandEntries() - for section in self._fit_sections.values(): - section.ExpandEntries() - - def ObtainContents(self): - """Obtain the contents of the FIT + def BuildSectionData(self, required): + """Build FIT entry contents

This adds the 'data' properties to the input ITB (Image-tree Binary) then runs mkimage to process it. + + Args: + required: True if the data must be present, False if it is OK to + return None + + Returns: + Contents of the section (bytes) """ - # self._BuildInput() either returns bytes or raises an exception. data = self._BuildInput(self._fdt) uniq = self.GetUniqueName() input_fname = tools.GetOutputFilename('%s.itb' % uniq) @@ -264,14 +266,12 @@ def ObtainContents(self): 'pad': fdt_util.fdt32_to_cpu(ext_offset.value) } if self.mkimage.run(reset_timestamp=True, output_fname=output_fname, - **args) is not None: - self.SetContents(tools.ReadFile(output_fname)) - else: + **args) is None: # Bintool is missing; just use empty data as the output self.record_missing_bintool(self.mkimage) - self.SetContents(tools.GetBytes(0, 1024)) + return tools.GetBytes(0, 1024)

- return True + return tools.ReadFile(output_fname)

def _BuildInput(self, fdt): """Finish the FIT by adding the 'data' properties to it @@ -282,12 +282,8 @@ def _BuildInput(self, fdt): Returns: New fdt contents (bytes) """ - for path, section in self._fit_sections.items(): + for path, section in self._entries.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)

@@ -295,34 +291,6 @@ def _BuildInput(self, fdt): data = fdt.GetContents() return data

- def CheckMissing(self, missing_list): - """Check if any entries in this FIT have missing external blobs - - If there are missing blobs, the entries are added to the list - - Args: - missing_list: List of Entry objects to be added to - """ - for path, section in self._fit_sections.items(): - section.CheckMissing(missing_list) - - def SetAllowMissing(self, allow_missing): - for section in self._fit_sections.values(): - section.SetAllowMissing(allow_missing) - def AddBintools(self, tools): - for section in self._fit_sections.values(): - section.AddBintools(tools) + super().AddBintools(tools) self.mkimage = self.AddBintool(tools, 'mkimage') - - def check_missing_bintools(self, missing_list): - """Check if any entries in this section have missing bintools - - If there are missing bintools, these are added to the list - - Args: - missing_list: List of Bintool objects to be added to - """ - super().check_missing_bintools(missing_list) - for entry in self._fit_sections.values(): - entry.check_missing_bintools(missing_list) diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index ce0dcf7c3e02..785f64a0c98b 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -3880,6 +3880,7 @@ def testPadInSections(self):

def testFitImageSubentryAlignment(self): """Test relative alignability of FIT image subentries""" + self._SetupSplElf() entry_args = { 'test-id': TEXT_DATA, }

Binman keeps track of positions of each entry in the final image, but currently this data is wrong for things included in FIT entries, especially since a previous patch makes FIT a subclass of Section and inherit its implementation.

There are three ways to put data into a FIT image. It can be directly included as a "data" property, or it can be external to the FIT image represented by an offset-size pair of properties. This external offset is either "data-position" from the start of the FIT or "data-offset" from the end of the FIT, and the size is "data-size" for both. However, binman doesn't use the "data-offset" method while building FIT entries.

According to the Section docstring, its subclasses should calculate and set the correct offsets and sizes in SetImagePos() method. Do this for FIT subentries for the three ways mentioned above, and add tests for the two ways binman can pack them in.

Signed-off-by: Alper Nebi Yasak alpernebiyasak@gmail.com ---

tools/binman/etype/fit.py | 51 +++++++++++++++++ tools/binman/ftest.py | 112 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 163 insertions(+)

diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py index 0f8c88a9720e..a66ddce095e1 100644 --- a/tools/binman/etype/fit.py +++ b/tools/binman/etype/fit.py @@ -291,6 +291,57 @@ def _BuildInput(self, fdt): data = fdt.GetContents() return data

+ def SetImagePos(self, image_pos): + """Set the position in the image + + This sets each subentry's offsets, sizes and positions-in-image + according to where they ended up in the packed FIT file. + + Args: + image_pos: Position of this entry in the image + """ + super().SetImagePos(image_pos) + + try: + fdt = Fdt.FromData(self.GetData()) + fdt.Scan() + except libfdt.FdtException: + # If mkimage is missing we'll have empty data, + # which will cause a FDT_ERR_BADMAGIC error + return + + for path, section in self._entries.items(): + node = fdt.GetNode(path) + + data_prop = node.props.get("data") + data_pos = fdt_util.GetInt(node, "data-position") + data_offset = fdt_util.GetInt(node, "data-offset") + data_size = fdt_util.GetInt(node, "data-size") + + # Contents are inside the FIT + if data_prop is not None: + # GetOffset() returns offset of a fdt_property struct, + # which has 3 fdt32_t members before the actual data. + offset = data_prop.GetOffset() + 12 + size = len(data_prop.bytes) + + # External offset from the base of the FIT + elif data_pos is not None: + offset = data_pos + size = data_size + + # External offset from the end of the FIT, not used in binman + elif data_offset is not None: # pragma: no cover + offset = fdt.GetFdtObj().totalsize() + data_offset + size = data_size + + # This should never happen + else: # pragma: no cover + self.Raise("%s: missing data properties" % (path)) + + section.SetOffsetSize(offset, size) + section.SetImagePos(self.image_pos) + def AddBintools(self, tools): super().AddBintools(tools) self.mkimage = self.AddBintool(tools, 'mkimage') diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 785f64a0c98b..029a3f303a56 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -3800,6 +3800,62 @@ def testSimpleFitExpandsSubentries(self): self.assertEqual(len(U_BOOT_EXP_DATA), int(data_sizes[0].split()[0])) self.assertEqual(len(U_BOOT_SPL_DTB_DATA), int(data_sizes[1].split()[0]))

+ def testSimpleFitImagePos(self): + """Test that we have correct image-pos for FIT subentries""" + data, _, _, out_dtb_fname = self._DoReadFileDtb('161_fit.dts', + update_dtb=True) + dtb = fdt.Fdt(out_dtb_fname) + dtb.Scan() + props = self._GetPropTree(dtb, BASE_DTB_PROPS + REPACK_DTB_PROPS) + + self.assertEqual({ + 'image-pos': 0, + 'offset': 0, + 'size': 1890, + + 'u-boot:image-pos': 0, + 'u-boot:offset': 0, + 'u-boot:size': 4, + + 'fit:image-pos': 4, + 'fit:offset': 4, + 'fit:size': 1840, + + 'fit/images/kernel:image-pos': 160, + 'fit/images/kernel:offset': 156, + 'fit/images/kernel:size': 4, + + 'fit/images/kernel/u-boot:image-pos': 160, + 'fit/images/kernel/u-boot:offset': 0, + 'fit/images/kernel/u-boot:size': 4, + + 'fit/images/fdt-1:image-pos': 456, + 'fit/images/fdt-1:offset': 452, + 'fit/images/fdt-1:size': 6, + + 'fit/images/fdt-1/u-boot-spl-dtb:image-pos': 456, + 'fit/images/fdt-1/u-boot-spl-dtb:offset': 0, + 'fit/images/fdt-1/u-boot-spl-dtb:size': 6, + + 'u-boot-nodtb:image-pos': 1844, + 'u-boot-nodtb:offset': 1844, + 'u-boot-nodtb:size': 46, + }, props) + + # Actually check the data is where we think it is + for node, expected in [ + ("u-boot", U_BOOT_DATA), + ("fit/images/kernel", U_BOOT_DATA), + ("fit/images/kernel/u-boot", U_BOOT_DATA), + ("fit/images/fdt-1", U_BOOT_SPL_DTB_DATA), + ("fit/images/fdt-1/u-boot-spl-dtb", U_BOOT_SPL_DTB_DATA), + ("u-boot-nodtb", U_BOOT_NODTB_DATA), + ]: + image_pos = props[f"{node}:image-pos"] + size = props[f"{node}:size"] + self.assertEqual(len(expected), size) + self.assertEqual(expected, data[image_pos:image_pos+size]) + def testFitExternal(self): """Test an image with an FIT with external images""" data = self._DoReadFile('162_fit_external.dts') @@ -3828,6 +3884,62 @@ def testFitExternal(self): self.assertEqual(U_BOOT_DATA + b'aa', data[actual_pos:actual_pos + external_data_size])

+ def testFitExternalImagePos(self): + """Test that we have correct image-pos for external FIT subentries""" + data, _, _, out_dtb_fname = self._DoReadFileDtb('162_fit_external.dts', + update_dtb=True) + dtb = fdt.Fdt(out_dtb_fname) + dtb.Scan() + props = self._GetPropTree(dtb, BASE_DTB_PROPS + REPACK_DTB_PROPS) + + self.assertEqual({ + 'image-pos': 0, + 'offset': 0, + 'size': 1082, + + 'u-boot:image-pos': 0, + 'u-boot:offset': 0, + 'u-boot:size': 4, + + 'fit:size': 1032, + 'fit:offset': 4, + 'fit:image-pos': 4, + + 'fit/images/kernel:size': 4, + 'fit/images/kernel:offset': 1024, + 'fit/images/kernel:image-pos': 1028, + + 'fit/images/kernel/u-boot:size': 4, + 'fit/images/kernel/u-boot:offset': 0, + 'fit/images/kernel/u-boot:image-pos': 1028, + + 'fit/images/fdt-1:size': 2, + 'fit/images/fdt-1:offset': 1028, + 'fit/images/fdt-1:image-pos': 1032, + + 'fit/images/fdt-1/_testing:size': 2, + 'fit/images/fdt-1/_testing:offset': 0, + 'fit/images/fdt-1/_testing:image-pos': 1032, + + 'u-boot-nodtb:image-pos': 1036, + 'u-boot-nodtb:offset': 1036, + 'u-boot-nodtb:size': 46, + }, props) + + # Actually check the data is where we think it is + for node, expected in [ + ("u-boot", U_BOOT_DATA), + ("fit/images/kernel", U_BOOT_DATA), + ("fit/images/kernel/u-boot", U_BOOT_DATA), + ("fit/images/fdt-1", b'aa'), + ("fit/images/fdt-1/_testing", b'aa'), + ("u-boot-nodtb", U_BOOT_NODTB_DATA), + ]: + image_pos = props[f"{node}:image-pos"] + size = props[f"{node}:size"] + self.assertEqual(len(expected), size) + self.assertEqual(expected, data[image_pos:image_pos+size]) + def testFitMissing(self): """Test that binman still produces a FIT image if mkimage is missing""" with test_util.capture_sys_output() as (_, stderr):