Re: [PATCH v3 09/19] binman: Convert mkimage to Entry_section

Hi Simon,

On 2023-07-10 04:41, Simon Glass wrote:

From: Marek Vasut marex@denx.de

This is needed to handle mkimage with inner section located itself in a section.

Signed-off-by: Marek Vasut marex@denx.de Use BuildSectionData() instead of ObtainContents(), add tests and a few other minor fixes: Signed-off-by: Simon Glass sjg@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";
  

• 	};
  

• };
  

• };

+};