Merge patch series "Fix handling of optional blobs in binman"

Yannic Moog <[email protected]> says:

This series solves a contradiction regarding ext blobs packaged in
binman. When they are marked as optional, by default they are faked, two
messages are emitted. One says the image is not functional the other
says the image is still functional. Both concern the same binman
entry/blob.

Binman is set up to have fake external blobs in case they are missing.
This is regardless on whether they are optional or not.
The implementation does not allow different types of entries to override
the faking decision; at least there wouldn't be much sense in doing so.

Here is an example build output of a phycore-imx8mp:

  BINMAN  .binman_stamp
Image 'image' is missing optional external blobs but is still functional: tee-os

/binman/section/fit/images/tee/tee-os (tee.bin):
   See the documentation for your board. You may need to build Open Portable
   Trusted Execution Environment (OP-TEE) and build with TEE=/path/to/tee.bin

Image 'image' has faked optional external blobs and is still functional: tee.bin

  OFCHK   .config

The output stays to inform/warn the user, but in this case the tee-os
entry will not be present in the final image.

Link: https://lore.kernel.org/r/[email protected]

10 files changed, 102 insertions, 42 deletions

diff --git a/tools/binman/binman.rst b/tools/binman/binman.rst
index 84b1331df5c..392e507d449 100644
--- a/tools/binman/binman.rst
+++ b/tools/binman/binman.rst
@@ -1143,6 +1143,13 @@ Optional entries
 
 Some entries need to exist only if certain conditions are met. For example, an
 entry may want to appear in the image only if a file has a particular format.
+Also, the ``optional`` property may be used to mark entries as optional::
+
+    tee-os {
+        filename = "tee.bin";
+        optional;
+    };
+
 Obviously the entry must exist in the image description for it to be processed
 at all, so a way needs to be found to have the entry remove itself.
 
diff --git a/tools/binman/control.py b/tools/binman/control.py
index e5bd7889806..3e0108d9635 100644
--- a/tools/binman/control.py
+++ b/tools/binman/control.py
@@ -645,14 +645,27 @@ def CheckForProblems(image):
         _ShowHelpForMissingBlobs(tout.ERROR, missing_list)
 
     faked_list = []
+    faked_optional_list = []
+    faked_required_list = []
     image.CheckFakedBlobs(faked_list)
-    if faked_list:
+    for e in faked_list:
+        if e.optional:
+            faked_optional_list.append(e)
+        else:
+            faked_required_list.append(e)
+    if faked_required_list:
         tout.warning(
             "Image '%s' has faked external blobs and is non-functional: %s\n" %
             (image.name, ' '.join([os.path.basename(e.GetDefaultFilename())
-                                   for e in faked_list])))
+                                   for e in faked_required_list])))
 
     optional_list = []
+    # For optional blobs, we should inform the user when the blob is not present. This will come as
+    # a warning since it may not be immediately apparent that something is missing otherwise.
+    # E.g. user thinks they supplied a blob, but there is no info of the contrary if they made an
+    # error.
+    # Faked optional blobs are not relevant for final images (as they are dropped anyway) so we
+    # will omit the message with default verbosity.
     image.CheckOptional(optional_list)
     if optional_list:
         tout.warning(
@@ -660,6 +673,12 @@ def CheckForProblems(image):
             (image.name, ' '.join([e.name for e in optional_list])))
         _ShowHelpForMissingBlobs(tout.WARNING, optional_list)
 
+    if faked_optional_list:
+        tout.info(
+            "Image '%s' has faked optional external blobs but is still functional: %s\n" %
+            (image.name, ' '.join([os.path.basename(e.GetDefaultFilename())
+                                   for e in faked_optional_list])))
+
     missing_bintool_list = []
     image.check_missing_bintools(missing_bintool_list)
     if missing_bintool_list:
@@ -667,7 +686,7 @@ def CheckForProblems(image):
             "Image '%s' has missing bintools and is non-functional: %s\n" %
             (image.name, ' '.join([os.path.basename(bintool.name)
                                    for bintool in missing_bintool_list])))
-    return any([missing_list, faked_list, missing_bintool_list])
+    return any([missing_list, faked_required_list, missing_bintool_list])
 
 def ProcessImage(image, update_fdt, write_map, get_contents=True,
                  allow_resize=True, allow_missing=False,
@@ -697,7 +716,6 @@ def ProcessImage(image, update_fdt, write_map, get_contents=True,
         image.SetAllowMissing(allow_missing)
         image.SetAllowFakeBlob(allow_fake_blobs)
         image.GetEntryContents()
-        image.drop_absent()
     image.GetEntryOffsets()
 
     # We need to pack the entries to figure out where everything
@@ -736,12 +754,12 @@ def ProcessImage(image, update_fdt, write_map, get_contents=True,
         image.Raise('Entries changed size after packing (tried %s passes)' %
                     passes)
 
+    has_problems = CheckForProblems(image)
+
     image.BuildImage()
     if write_map:
         image.WriteMap()
 
-    has_problems = CheckForProblems(image)
-
     image.WriteAlternates()
 
     return has_problems
diff --git a/tools/binman/entry.py b/tools/binman/entry.py
index bdc60e47fca..ce7ef28e94b 100644
--- a/tools/binman/entry.py
+++ b/tools/binman/entry.py
@@ -88,6 +88,7 @@ class Entry(object):
             updated with a hash of the entry contents
         comp_bintool: Bintools used for compress and decompress data
         fake_fname: Fake filename, if one was created, else None
+        faked (bool): True if the entry is absent and faked
         required_props (dict of str): Properties which must be present. This can
             be added to by subclasses
         elf_fname (str): Filename of the ELF file, if this entry holds an ELF
@@ -759,7 +760,7 @@ class Entry(object):
                           self.image_pos)
 
     # pylint: disable=assignment-from-none
-    def GetEntries(self):
+    def GetEntries(self) -> None:
         """Return a list of entries contained by this entry
 
         Returns:
@@ -1120,7 +1121,7 @@ features to produce new behaviours.
         if self.missing and not self.optional:
             missing_list.append(self)
 
-    def check_fake_fname(self, fname, size=0):
+    def check_fake_fname(self, fname: str, size: int = 0) -> str:
         """If the file is missing and the entry allows fake blobs, fake it
 
         Sets self.faked to True if faked
@@ -1130,9 +1131,7 @@ features to produce new behaviours.
             size (int): Size of fake file to create
 
         Returns:
-            tuple:
-                fname (str): Filename of faked file
-                bool: True if the blob was faked, False if not
+            fname (str): Filename of faked file
         """
         if self.allow_fake and not pathlib.Path(fname).is_file():
             if not self.fake_fname:
@@ -1142,8 +1141,8 @@ features to produce new behaviours.
                 tout.info(f"Entry '{self._node.path}': Faked blob '{outfname}'")
                 self.fake_fname = outfname
             self.faked = True
-            return self.fake_fname, True
-        return fname, False
+            return self.fake_fname
+        return fname
 
     def CheckFakedBlobs(self, faked_blobs_list):
         """Check if any entries in this section have faked external blobs
@@ -1352,6 +1351,10 @@ features to produce new behaviours.
             os.mkdir(cls.fake_dir)
         tout.notice(f"Fake-blob dir is '{cls.fake_dir}'")
 
+    def drop_absent_optional(self) -> None:
+        """Entries don't have any entries, do nothing"""
+        pass
+
     def ensure_props(self):
         """Raise an exception if properties are missing
 
diff --git a/tools/binman/etype/blob.py b/tools/binman/etype/blob.py
index 041e1122953..acd9ae34074 100644
--- a/tools/binman/etype/blob.py
+++ b/tools/binman/etype/blob.py
@@ -42,7 +42,7 @@ class Entry_blob(Entry):
             if fdt_util.GetBool(self._node, 'write-symbols'):
                 self.auto_write_symbols = True
 
-    def ObtainContents(self, fake_size=0):
+    def ObtainContents(self, fake_size: int = 0) -> bool:
         self._filename = self.GetDefaultFilename()
         self._pathname = tools.get_input_filename(self._filename,
             self.external and (self.optional or self.section.GetAllowMissing()))
@@ -50,10 +50,11 @@ class Entry_blob(Entry):
         if not self._pathname:
             if not fake_size and self.assume_size:
                 fake_size = self.assume_size
-            self._pathname, faked = self.check_fake_fname(self._filename,
-                                                          fake_size)
+            self._pathname = self.check_fake_fname(self._filename, fake_size)
             self.missing = True
-            if not faked:
+            if self.optional:
+                self.mark_absent("missing but optional")
+            if not self.faked:
                 content_size = 0
                 if self.assume_size: # Ensure we get test coverage on next line
                     content_size = self.assume_size
diff --git a/tools/binman/etype/blob_ext_list.py b/tools/binman/etype/blob_ext_list.py
index 1bfcf6733a7..a8b5a24c3a1 100644
--- a/tools/binman/etype/blob_ext_list.py
+++ b/tools/binman/etype/blob_ext_list.py
@@ -33,11 +33,11 @@ class Entry_blob_ext_list(Entry_blob):
         self._filenames = fdt_util.GetStringList(self._node, 'filenames')
         self._pathnames = []
 
-    def ObtainContents(self):
+    def ObtainContents(self) -> bool:
         missing = False
         pathnames = []
         for fname in self._filenames:
-            fname, _ = self.check_fake_fname(fname)
+            fname = self.check_fake_fname(fname)
             pathname = tools.get_input_filename(
                 fname, self.external and self.section.GetAllowMissing())
             # Allow the file to be missing
diff --git a/tools/binman/etype/cbfs.py b/tools/binman/etype/cbfs.py
index 124fa1e4ffc..5879f377231 100644
--- a/tools/binman/etype/cbfs.py
+++ b/tools/binman/etype/cbfs.py
@@ -276,7 +276,8 @@ class Entry_cbfs(Entry):
         for entry in self._entries.values():
             entry.ListEntries(entries, indent + 1)
 
-    def GetEntries(self):
+    def GetEntries(self) -> dict[str, Entry]:
+        """Returns the entries (tree children) of this section"""
         return self._entries
 
     def ReadData(self, decomp=True, alt_format=None):
diff --git a/tools/binman/etype/mkimage.py b/tools/binman/etype/mkimage.py
index 6ae5d0c8a4f..75e59c3d3a3 100644
--- a/tools/binman/etype/mkimage.py
+++ b/tools/binman/etype/mkimage.py
@@ -205,7 +205,7 @@ class Entry_mkimage(Entry_section):
             self.record_missing_bintool(self.mkimage)
             return data
 
-    def GetEntries(self):
+    def GetEntries(self) -> dict[str, Entry]:
         # Make a copy so we don't change the original
         entries = OrderedDict(self._entries)
         if self._imagename:
diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py
index 1d50bb47753..03c4f7c6ec7 100644
--- a/tools/binman/etype/section.py
+++ b/tools/binman/etype/section.py
@@ -537,7 +537,7 @@ class Entry_section(Entry):
         for entry in self._entries.values():
             entry.WriteMap(fd, indent + 1)
 
-    def GetEntries(self):
+    def GetEntries(self) -> dict[str, Entry]:
         return self._entries
 
     def GetContentsByPhandle(self, phandle, source_entry, required):
@@ -772,9 +772,17 @@ class Entry_section(Entry):
                        todo)
         return True
 
-    def drop_absent(self):
-        """Drop entries which are absent"""
-        self._entries = {n: e for n, e in self._entries.items() if not e.absent}
+    def drop_absent_optional(self) -> None:
+        """Drop entries which are absent.
+        Call for all nodes in the tree. Leaf nodes will do nothing per
+        definition. Sections however have _entries and should drop all children
+        which are absent.
+        """
+        self._entries = {n: e for n, e in self._entries.items() if not (e.absent and e.optional)}
+        # Drop nodes first before traversing children to avoid superfluous calls
+        # to children of absent nodes.
+        for e in self.GetEntries().values():
+            e.drop_absent_optional()
 
     def _SetEntryOffsetSize(self, name, offset, size):
         """Set the offset and size of an entry
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index 4cf7dfc8216..bedfcb53c48 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -84,6 +84,7 @@ FILES_DATA            = (b"sorry I'm late\nOh, don't bother apologising, I'm " +
                          b"sorry you're alive\n")
 COMPRESS_DATA         = b'compress xxxxxxxxxxxxxxxxxxxxxx data'
 COMPRESS_DATA_BIG     = COMPRESS_DATA * 2
+MISSING_DATA          = b'missing'
 REFCODE_DATA          = b'refcode'
 FSP_M_DATA            = b'fsp_m'
 FSP_S_DATA            = b'fsp_s'
@@ -250,7 +251,7 @@ class TestFunctional(unittest.TestCase):
         # ATF and OP_TEE
         TestFunctional._MakeInputFile('bl31.elf',
             tools.read_file(cls.ElfTestFile('elf_sections')))
-        TestFunctional._MakeInputFile('tee.elf',
+        TestFunctional.tee_elf_path = TestFunctional._MakeInputFile('tee.elf',
             tools.read_file(cls.ElfTestFile('elf_sections')))
 
         # Newer OP_TEE file in v1 binary format
@@ -514,9 +515,9 @@ class TestFunctional(unittest.TestCase):
         return dtb.GetContents()
 
     def _DoReadFileDtb(self, fname, use_real_dtb=False, use_expanded=False,
-                       verbosity=None, map=False, update_dtb=False,
-                       entry_args=None, reset_dtbs=True, extra_indirs=None,
-                       threads=None):
+                       verbosity=None, allow_fake_blobs=True, map=False,
+                       update_dtb=False, entry_args=None, reset_dtbs=True,
+                       extra_indirs=None, threads=None):
         """Run binman and return the resulting image
 
         This runs binman with a given test file and then reads the resulting
@@ -534,6 +535,7 @@ class TestFunctional(unittest.TestCase):
             use_expanded: True to use expanded entries where available, e.g.
                 'u-boot-expanded' instead of 'u-boot'
             verbosity: Verbosity level to use (0-3, None=don't set it)
+            allow_fake_blobs: whether binman should fake missing ext blobs
             map: True to output map files for the images
             update_dtb: Update the offset and size of each entry in the device
                 tree before packing it into the image
@@ -571,7 +573,7 @@ class TestFunctional(unittest.TestCase):
             retcode = self._DoTestFile(fname, map=map, update_dtb=update_dtb,
                     entry_args=entry_args, use_real_dtb=use_real_dtb,
                     use_expanded=use_expanded, verbosity=verbosity,
-                    extra_indirs=extra_indirs,
+                    allow_fake_blobs=allow_fake_blobs, extra_indirs=extra_indirs,
                     threads=threads)
             self.assertEqual(0, retcode)
             out_dtb_fname = tools.get_output_filename('u-boot.dtb.out')
@@ -5210,13 +5212,15 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
 
     def testExtblobList(self):
         """Test an image with an external blob list"""
-        data = self._DoReadFile('215_blob_ext_list.dts')
-        self.assertEqual(REFCODE_DATA + FSP_M_DATA, data)
+        data = self._DoReadFileDtb('215_blob_ext_list.dts',
+                                   allow_fake_blobs=False)
+        self.assertEqual(REFCODE_DATA + FSP_M_DATA, data[0])
 
     def testExtblobListMissing(self):
         """Test an image with a missing external blob"""
         with self.assertRaises(ValueError) as e:
-            self._DoReadFile('216_blob_ext_list_missing.dts')
+            self._DoReadFileDtb('216_blob_ext_list_missing.dts',
+                                allow_fake_blobs=False)
         self.assertIn("Filename 'missing-file' not found in input path",
                       str(e.exception))
 
@@ -5224,7 +5228,7 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
         """Test an image with an missing external blob that is allowed"""
         with terminal.capture() as (stdout, stderr):
             self._DoTestFile('216_blob_ext_list_missing.dts',
-                             allow_missing=True)
+                             allow_missing=True, allow_fake_blobs=False)
         err = stderr.getvalue()
         self.assertRegex(err, "Image 'image'.*missing.*: blob-ext")
 
@@ -5766,10 +5770,10 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
 
 
     def testFitSplitElfMissing(self):
-        """Test an split-elf FIT with a missing ELF file"""
+        """Test an split-elf FIT with a missing ELF file. Don't fake the file."""
         if not elf.ELF_TOOLS:
             self.skipTest('Python elftools not available')
-        out, err = self.checkFitSplitElf(allow_missing=True)
+        out, err = self.checkFitSplitElf(allow_missing=True, allow_fake_blobs=False)
         self.assertRegex(
             err,
             "Image '.*' is missing external blobs and is non-functional: .*")
@@ -6458,16 +6462,18 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
     def testAbsent(self):
         """Check handling of absent entries"""
         data = self._DoReadFile('262_absent.dts')
-        self.assertEqual(U_BOOT_DATA + U_BOOT_IMG_DATA, data)
+        self.assertEqual(U_BOOT_DATA + b'aa' + U_BOOT_IMG_DATA, data)
 
-    def testPackTeeOsOptional(self):
-        """Test that an image with an optional TEE binary can be created"""
+    def testPackTeeOsElf(self):
+        """Test that an image with a TEE elf binary can be created"""
         entry_args = {
             'tee-os-path': 'tee.elf',
         }
+        tee_path = self.tee_elf_path
         data = self._DoReadFileDtb('263_tee_os_opt.dts',
                                    entry_args=entry_args)[0]
-        self.assertEqual(U_BOOT_DATA + U_BOOT_IMG_DATA, data)
+        self.assertEqual(U_BOOT_DATA + tools.read_file(tee_path) +
+                         U_BOOT_IMG_DATA, data)
 
     def checkFitTee(self, dts, tee_fname):
         """Check that a tee-os entry works and returns data
@@ -6512,6 +6518,9 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
         self.assertRegex(
             err,
             "Image '.*' is missing optional external blobs but is still functional: tee-os")
+        self.assertNotRegex(
+            err,
+            "Image '.*' has faked external blobs and is non-functional: tee-os")
 
     def testFitTeeOsOptionalFitBad(self):
         """Test an image with a FIT with an optional OP-TEE binary"""
@@ -6537,7 +6546,15 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
             "Node '/binman/fit/images/@tee-SEQ/tee-os': Invalid OP-TEE file: size mismatch (expected 0x4, have 0xe)",
             str(exc.exception))
 
-    def testExtblobOptional(self):
+    def testExtblobMissingOptional(self):
+        """Test an image with an external blob that is optional"""
+        with terminal.capture() as (stdout, stderr):
+            data = self._DoReadFileDtb('266_blob_ext_opt.dts',
+                                       allow_fake_blobs=False)[0]
+        self.assertEqual(REFCODE_DATA, data)
+        self.assertNotIn(MISSING_DATA, data)
+
+    def testExtblobFakedOptional(self):
         """Test an image with an external blob that is optional"""
         with terminal.capture() as (stdout, stderr):
             data = self._DoReadFile('266_blob_ext_opt.dts')
@@ -6546,6 +6563,9 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
         self.assertRegex(
             err,
             "Image '.*' is missing optional external blobs but is still functional: missing")
+        self.assertNotRegex(
+            err,
+            "Image '.*' has faked external blobs and is non-functional: missing")
 
     def testSectionInner(self):
         """Test an inner section with a size"""
@@ -6726,7 +6746,7 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
 
         node = dtb.GetNode('/configurations/conf-missing-tee-1')
         self.assertEqual('atf-1', node.props['firmware'].value)
-        self.assertEqual(['u-boot', 'atf-2'],
+        self.assertEqual(['u-boot', 'tee', 'atf-2'],
                          fdt_util.GetStringList(node, 'loadables'))
 
     def testTooldir(self):
diff --git a/tools/binman/image.py b/tools/binman/image.py
index 24ce0af7c72..698cfa4148e 100644
--- a/tools/binman/image.py
+++ b/tools/binman/image.py
@@ -183,6 +183,8 @@ class Image(section.Entry_section):
         fname = tools.get_output_filename(self._filename)
         tout.info("Writing image to '%s'" % fname)
         with open(fname, 'wb') as fd:
+            # For final image, don't write absent blobs to file
+            self.drop_absent_optional()
             data = self.GetPaddedData()
             fd.write(data)
         tout.info("Wrote %#x bytes" % len(data))