[PATCH v3 09/19] binman: Convert mkimage to Entry_section
Jonas Karlman
jonas at kwiboo.se
Mon Jul 10 19:35:05 CEST 2023
Hi Simon,
On 2023-07-10 04:41, Simon Glass wrote:
> From: Marek Vasut <marex at denx.de>
>
> This is needed to handle mkimage with inner section located itself in a
> section.
>
> Signed-off-by: Marek Vasut <marex at denx.de>
> Use BuildSectionData() instead of ObtainContents(), add tests and a few
> other minor fixes:
> Signed-off-by: Simon Glass <sjg at chromium.org>
> ---
>
> Changes in v3:
> - Fix up some tests which now need SPL and TPL
> - Avoid calling ObtainContents() when not needed
>
> Changes in v2:
> - Drop now-unnecessary methods in mkimage etype
Looks like this change never made it into the patch?
The following functions are still in this file and can now be removed:
- SetAllowMissing
- SetAllowFakeBlob
- CheckMissing
- CheckFakedBlobs
>
> tools/binman/entry.py | 6 +-
> tools/binman/etype/mkimage.py | 71 ++++++++++++++---------
> tools/binman/ftest.py | 69 +++++++++++++++++++++-
> tools/binman/test/283_mkimage_special.dts | 24 ++++++++
> 4 files changed, 135 insertions(+), 35 deletions(-)
> create mode 100644 tools/binman/test/283_mkimage_special.dts
>
> diff --git a/tools/binman/entry.py b/tools/binman/entry.py
> index f20f32213a9b..0d4cb94f7002 100644
> --- a/tools/binman/entry.py
> +++ b/tools/binman/entry.py
> @@ -1314,10 +1314,8 @@ features to produce new behaviours.
> """
> data = b''
> for entry in entries:
> - # First get the input data and put it in a file. If not available,
> - # try later.
> - if not entry.ObtainContents(fake_size=fake_size):
> - return None, None, None
> + # First get the input data and put it in a file
> + entry.ObtainContents(fake_size=fake_size)
> data += entry.GetData()
> uniq = self.GetUniqueName()
> fname = tools.get_output_filename(f'{prefix}.{uniq}')
> diff --git a/tools/binman/etype/mkimage.py b/tools/binman/etype/mkimage.py
> index cb3e10672ad7..493761da59f5 100644
> --- a/tools/binman/etype/mkimage.py
> +++ b/tools/binman/etype/mkimage.py
> @@ -8,10 +8,11 @@
> from collections import OrderedDict
>
> from binman.entry import Entry
> +from binman.etype.section import Entry_section
> from dtoc import fdt_util
> from u_boot_pylib import tools
>
> -class Entry_mkimage(Entry):
> +class Entry_mkimage(Entry_section):
> """Binary produced by mkimage
>
> Properties / Entry arguments:
> @@ -121,10 +122,8 @@ class Entry_mkimage(Entry):
> """
> def __init__(self, section, etype, node):
> super().__init__(section, etype, node)
> - self._mkimage_entries = OrderedDict()
> self._imagename = None
> - self._filename = fdt_util.GetString(self._node, 'filename')
> - self.align_default = None
> + self._multiple_data_files = False
>
> def ReadNode(self):
> super().ReadNode()
> @@ -135,41 +134,55 @@ class Entry_mkimage(Entry):
> 'data-to-imagename')
> if self._data_to_imagename and self._node.FindNode('imagename'):
> self.Raise('Cannot use both imagename node and data-to-imagename')
> - self.ReadEntries()
>
> def ReadEntries(self):
> """Read the subnodes to find out what should go in this image"""
> for node in self._node.subnodes:
> - entry = Entry.Create(self, node)
> + if self.IsSpecialSubnode(node):
> + continue
> + entry = Entry.Create(self, node,
> + expanded=self.GetImage().use_expanded,
> + missing_etype=self.GetImage().missing_etype)
> entry.ReadNode()
> + entry.SetPrefix(self._name_prefix)
> if entry.name == 'imagename':
> self._imagename = entry
> else:
> - self._mkimage_entries[entry.name] = entry
> + self._entries[entry.name] = entry
>
> - def ObtainContents(self):
> + def BuildSectionData(self, required):
> + """Build mkimage entry contents
> +
> + Runs mkimage to build the entry contents
> +
> + Args:
> + required (bool): True if the data must be present, False if it is OK
> + to return None
> +
> + Returns:
> + bytes: Contents of the section
> + """
> # Use a non-zero size for any fake files to keep mkimage happy
> # Note that testMkimageImagename() relies on this 'mkimage' parameter
> fake_size = 1024
> if self._multiple_data_files:
> fnames = []
> uniq = self.GetUniqueName()
> - for entry in self._mkimage_entries.values():
> - if not entry.ObtainContents(fake_size=fake_size):
> - return False
> - if entry._pathname:
> - fnames.append(entry._pathname)
> + for entry in self._entries.values():
> + # Put the contents in a temporary file
> + ename = f'mkimage-in-{uniq}-{entry.name}'
> + fname = tools.get_output_filename(ename)
> + data = entry.GetData(required)
> + tools.write_file(fname, data)
> + fnames.append(fname)
> input_fname = ":".join(fnames)
> + data = b''
> else:
> data, input_fname, uniq = self.collect_contents_to_file(
> - self._mkimage_entries.values(), 'mkimage', fake_size)
> - if data is None:
> - return False
> + self._entries.values(), 'mkimage', fake_size)
> if self._imagename:
> image_data, imagename_fname, _ = self.collect_contents_to_file(
> [self._imagename], 'mkimage-n', 1024)
> - if image_data is None:
> - return False
> outfile = self._filename if self._filename else 'mkimage-out.%s' % uniq
> output_fname = tools.get_output_filename(outfile)
>
> @@ -177,8 +190,7 @@ class Entry_mkimage(Entry):
> self.CheckMissing(missing_list)
> self.missing = bool(missing_list)
> if self.missing:
> - self.SetContents(b'')
> - return self.allow_missing
> + return b''
>
> args = ['-d', input_fname]
> if self._data_to_imagename:
> @@ -187,17 +199,15 @@ class Entry_mkimage(Entry):
> args += ['-n', imagename_fname]
> args += self._args + [output_fname]
> if self.mkimage.run_cmd(*args) is not None:
> - self.SetContents(tools.read_file(output_fname))
> + return tools.read_file(output_fname)
> else:
> # Bintool is missing; just use the input data as the output
> self.record_missing_bintool(self.mkimage)
> - self.SetContents(data)
> -
> - return True
> + return data
>
> def GetEntries(self):
> # Make a copy so we don't change the original
> - entries = OrderedDict(self._mkimage_entries)
> + entries = OrderedDict(self._entries)
> if self._imagename:
> entries['imagename'] = self._imagename
> return entries
> @@ -209,7 +219,7 @@ class Entry_mkimage(Entry):
> allow_missing: True if allowed, False if not allowed
> """
> self.allow_missing = allow_missing
> - for entry in self._mkimage_entries.values():
> + for entry in self._entries.values():
> entry.SetAllowMissing(allow_missing)
> if self._imagename:
> self._imagename.SetAllowMissing(allow_missing)
> @@ -220,7 +230,7 @@ class Entry_mkimage(Entry):
> Args:
> allow_fake: True if allowed, False if not allowed
> """
> - for entry in self._mkimage_entries.values():
> + for entry in self._entries.values():
> entry.SetAllowFakeBlob(allow_fake)
> if self._imagename:
> self._imagename.SetAllowFakeBlob(allow_fake)
> @@ -234,7 +244,7 @@ class Entry_mkimage(Entry):
> Args:
> missing_list: List of Entry objects to be added to
> """
> - for entry in self._mkimage_entries.values():
> + for entry in self._entries.values():
> entry.CheckMissing(missing_list)
> if self._imagename:
> self._imagename.CheckMissing(missing_list)
> @@ -247,7 +257,7 @@ class Entry_mkimage(Entry):
> Args:
> faked_blobs_list: List of Entry objects to be added to
> """
> - for entry in self._mkimage_entries.values():
> + for entry in self._entries.values():
> entry.CheckFakedBlobs(faked_blobs_list)
> if self._imagename:
> self._imagename.CheckFakedBlobs(faked_blobs_list)
> @@ -255,3 +265,6 @@ class Entry_mkimage(Entry):
> def AddBintools(self, btools):
> super().AddBintools(btools)
> self.mkimage = self.AddBintool(btools, 'mkimage')
> +
> + def CheckEntries(self):
> + pass
> diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
> index dabb3f689fdb..60e69443c400 100644
> --- a/tools/binman/ftest.py
> +++ b/tools/binman/ftest.py
> @@ -1103,6 +1103,7 @@ class TestFunctional(unittest.TestCase):
>
> def testPackZeroOffset(self):
> """Test that an entry at offset 0 is not given a new offset"""
> + self._SetupSplElf()
> with self.assertRaises(ValueError) as e:
> self._DoTestFile('025_pack_zero_size.dts')
> self.assertIn("Node '/binman/u-boot-spl': Offset 0x0 (0) overlaps "
> @@ -1116,6 +1117,7 @@ class TestFunctional(unittest.TestCase):
>
> def testPackX86RomNoSize(self):
> """Test that the end-at-4gb property requires a size property"""
> + self._SetupSplElf()
> with self.assertRaises(ValueError) as e:
> self._DoTestFile('027_pack_4gb_no_size.dts')
> self.assertIn("Image '/binman': Section size must be provided when "
> @@ -1124,6 +1126,7 @@ class TestFunctional(unittest.TestCase):
> def test4gbAndSkipAtStartTogether(self):
> """Test that the end-at-4gb and skip-at-size property can't be used
> together"""
> + self._SetupSplElf()
> with self.assertRaises(ValueError) as e:
> self._DoTestFile('098_4gb_and_skip_at_start_together.dts')
> self.assertIn("Image '/binman': Provide either 'end-at-4gb' or "
> @@ -1131,6 +1134,7 @@ class TestFunctional(unittest.TestCase):
>
> def testPackX86RomOutside(self):
> """Test that the end-at-4gb property checks for offset boundaries"""
> + self._SetupSplElf()
> with self.assertRaises(ValueError) as e:
> self._DoTestFile('028_pack_4gb_outside.dts')
> self.assertIn("Node '/binman/u-boot': Offset 0x0 (0) size 0x4 (4) "
> @@ -1423,6 +1427,7 @@ class TestFunctional(unittest.TestCase):
>
> def testPackUbootSplMicrocode(self):
> """Test that x86 microcode can be handled correctly in SPL"""
> + self._SetupSplElf()
> self._PackUbootSplMicrocode('049_x86_ucode_spl.dts')
>
> def testPackUbootSplMicrocodeReorder(self):
> @@ -1442,6 +1447,7 @@ class TestFunctional(unittest.TestCase):
>
> def testSplDtb(self):
> """Test that an image with spl/u-boot-spl.dtb can be created"""
> + self._SetupSplElf()
> data = self._DoReadFile('051_u_boot_spl_dtb.dts')
> self.assertEqual(U_BOOT_SPL_DTB_DATA, data[:len(U_BOOT_SPL_DTB_DATA)])
>
> @@ -1962,6 +1968,8 @@ class TestFunctional(unittest.TestCase):
>
> def testUpdateFdtAll(self):
> """Test that all device trees are updated with offset/size info"""
> + self._SetupSplElf()
> + self._SetupTplElf()
> data = self._DoReadFileRealDtb('082_fdt_update_all.dts')
>
> base_expected = {
> @@ -3284,6 +3292,8 @@ class TestFunctional(unittest.TestCase):
>
> def testUpdateFdtAllRepack(self):
> """Test that all device trees are updated with offset/size info"""
> + self._SetupSplElf()
> + self._SetupTplElf()
> data = self._DoReadFileRealDtb('134_fdt_update_all_repack.dts')
> SECTION_SIZE = 0x300
> DTB_SIZE = 602
> @@ -3737,6 +3747,7 @@ class TestFunctional(unittest.TestCase):
>
> def testMkimage(self):
> """Test using mkimage to build an image"""
> + self._SetupSplElf()
> data = self._DoReadFile('156_mkimage.dts')
>
> # Just check that the data appears in the file somewhere
> @@ -3744,6 +3755,7 @@ class TestFunctional(unittest.TestCase):
>
> def testMkimageMissing(self):
> """Test that binman still produces an image if mkimage is missing"""
> + self._SetupSplElf()
> with test_util.capture_sys_output() as (_, stderr):
> self._DoTestFile('156_mkimage.dts',
> force_missing_bintools='mkimage')
> @@ -3856,6 +3868,7 @@ class TestFunctional(unittest.TestCase):
>
> def testSimpleFit(self):
> """Test an image with a FIT inside"""
> + self._SetupSplElf()
> data = self._DoReadFile('161_fit.dts')
> self.assertEqual(U_BOOT_DATA, data[:len(U_BOOT_DATA)])
> self.assertEqual(U_BOOT_NODTB_DATA, data[-len(U_BOOT_NODTB_DATA):])
> @@ -5375,6 +5388,7 @@ fdt fdtmap Extract the devicetree blob from the fdtmap
>
> def testFitSubentryHashSubnode(self):
> """Test an image with a FIT inside"""
> + self._SetupSplElf()
> data, _, _, out_dtb_name = self._DoReadFileDtb(
> '221_fit_subentry_hash.dts', use_real_dtb=True, update_dtb=True)
>
> @@ -5893,6 +5907,7 @@ fdt fdtmap Extract the devicetree blob from the fdtmap
>
> def testMkimageImagename(self):
> """Test using mkimage with -n holding the data too"""
> + self._SetupSplElf()
> data = self._DoReadFile('242_mkimage_name.dts')
>
> # Check that the data appears in the file somewhere
> @@ -5910,6 +5925,7 @@ fdt fdtmap Extract the devicetree blob from the fdtmap
>
> def testMkimageImage(self):
> """Test using mkimage with -n holding the data too"""
> + self._SetupSplElf()
> data = self._DoReadFile('243_mkimage_image.dts')
>
> # Check that the data appears in the file somewhere
> @@ -5930,6 +5946,7 @@ fdt fdtmap Extract the devicetree blob from the fdtmap
>
> def testMkimageImageNoContent(self):
> """Test using mkimage with -n and no data"""
> + self._SetupSplElf()
> with self.assertRaises(ValueError) as exc:
> self._DoReadFile('244_mkimage_image_no_content.dts')
> self.assertIn('Could not complete processing of contents',
> @@ -5937,6 +5954,7 @@ fdt fdtmap Extract the devicetree blob from the fdtmap
>
> def testMkimageImageBad(self):
> """Test using mkimage with imagename node and data-to-imagename"""
> + self._SetupSplElf()
> with self.assertRaises(ValueError) as exc:
> self._DoReadFile('245_mkimage_image_bad.dts')
> self.assertIn('Cannot use both imagename node and data-to-imagename',
> @@ -5952,6 +5970,7 @@ fdt fdtmap Extract the devicetree blob from the fdtmap
>
> def testMkimageCollection(self):
> """Test using a collection referring to an entry in a mkimage entry"""
> + self._SetupSplElf()
> data = self._DoReadFile('247_mkimage_coll.dts')
> expect = U_BOOT_SPL_DATA + U_BOOT_DATA
> self.assertEqual(expect, data[:len(expect)])
> @@ -6037,6 +6056,8 @@ fdt fdtmap Extract the devicetree blob from the fdtmap
>
> def testMkimageMultipleDataFiles(self):
> """Test passing multiple files to mkimage in a mkimage entry"""
> + self._SetupSplElf()
> + self._SetupTplElf()
> data = self._DoReadFile('252_mkimage_mult_data.dts')
> # Size of files are packed in their 4B big-endian format
> expect = struct.pack('>I', len(U_BOOT_TPL_DATA))
> @@ -6051,8 +6072,42 @@ fdt fdtmap Extract the devicetree blob from the fdtmap
> expect += U_BOOT_SPL_DATA
> self.assertEqual(expect, data[-len(expect):])
>
> + def testMkimageMultipleExpanded(self):
> + """Test passing multiple files to mkimage in a mkimage entry"""
> + self._SetupSplElf()
> + self._SetupTplElf()
> + entry_args = {
> + 'spl-bss-pad': 'y',
> + 'spl-dtb': 'y',
> + }
> + data = self._DoReadFileDtb('252_mkimage_mult_data.dts',
> + use_expanded=True, entry_args=entry_args)[0]
> + pad_len = 10
> + tpl_expect = U_BOOT_TPL_DATA
> + spl_expect = U_BOOT_SPL_NODTB_DATA + tools.get_bytes(0, pad_len)
> + spl_expect += U_BOOT_SPL_DTB_DATA
> +
> + content = data[0x40:]
> + lens = struct.unpack('>III', content[:12])
> +
> + # Size of files are packed in their 4B big-endian format
> + # Size info is always followed by a 4B zero value.
> + self.assertEqual(len(tpl_expect), lens[0])
> + self.assertEqual(len(spl_expect), lens[1])
> + self.assertEqual(0, lens[2])
> +
> + rest = content[12:]
> + self.assertEqual(tpl_expect, rest[:len(tpl_expect)])
> +
> + rest = rest[len(tpl_expect):]
> + align_pad = len(tpl_expect) % 4
> + self.assertEqual(tools.get_bytes(0, align_pad), rest[:align_pad])
> + rest = rest[align_pad:]
> + self.assertEqual(spl_expect, rest)
> +
> def testMkimageMultipleNoContent(self):
> """Test passing multiple data files to mkimage with one data file having no content"""
> + self._SetupSplElf()
> with self.assertRaises(ValueError) as exc:
> self._DoReadFile('253_mkimage_mult_no_content.dts')
> self.assertIn('Could not complete processing of contents',
> @@ -6060,6 +6115,7 @@ fdt fdtmap Extract the devicetree blob from the fdtmap
>
> def testMkimageFilename(self):
> """Test using mkimage to build a binary with a filename"""
> + self._SetupSplElf()
> retcode = self._DoTestFile('254_mkimage_filename.dts')
> self.assertEqual(0, retcode)
> fname = tools.get_output_filename('mkimage-test.bin')
> @@ -6534,6 +6590,7 @@ fdt fdtmap Extract the devicetree blob from the fdtmap
>
> def testReplaceFitSibling(self):
> """Test an image with a FIT inside where we replace its sibling"""
> + self._SetupSplElf()
> fname = TestFunctional._MakeInputFile('once', b'available once')
> self._DoReadFileRealDtb('277_replace_fit_sibling.dts')
> os.remove(fname)
> @@ -6608,7 +6665,7 @@ fdt fdtmap Extract the devicetree blob from the fdtmap
> Private key
> DTB
> """
> -
> + self._SetupSplElf()
> data = self._DoReadFileRealDtb(dts)
> updated_fname = tools.get_output_filename('image-updated.bin')
> tools.write_file(updated_fname, data)
> @@ -6683,6 +6740,7 @@ fdt fdtmap Extract the devicetree blob from the fdtmap
>
> def testSymbolNoWrite(self):
> """Test disabling of symbol writing"""
> + self._SetupSplElf()
> self.checkSymbols('282_symbols_disable.dts', U_BOOT_SPL_DATA, 0x1c,
> no_write_symbols=True)
>
> @@ -6696,6 +6754,13 @@ fdt fdtmap Extract the devicetree blob from the fdtmap
> entry_args=entry_args, use_expanded=True,
> no_write_symbols=True)
>
> + def testMkimageSpecial(self):
> + """Test mkimage ignores special hash-1 node"""
> + data = self._DoReadFile('283_mkimage_special.dts')
> +
> + # Just check that the data appears in the file somewhere
> + self.assertIn(U_BOOT_DATA, data)
> +
>
> -if __name__ == "__main__":
> +if __name__ == "_s_main__":
This looks like an unintended change in this patch?
Regards,
Jonas
> unittest.main()
> diff --git a/tools/binman/test/283_mkimage_special.dts b/tools/binman/test/283_mkimage_special.dts
> new file mode 100644
> index 000000000000..c234093e6ece
> --- /dev/null
> +++ b/tools/binman/test/283_mkimage_special.dts
> @@ -0,0 +1,24 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +/dts-v1/;
> +
> +/ {
> + #address-cells = <1>;
> + #size-cells = <1>;
> +
> + binman {
> + mkimage {
> + args = "-T script";
> +
> + u-boot {
> + };
> +
> + hash {
> + };
> +
> + imagename {
> + type = "u-boot";
> + };
> + };
> + };
> +};
More information about the U-Boot
mailing list