[PATCH 00/10] binman: Various minor fixes and improvement

[PATCH 00/10] binman: Various minor fixes and improvement

[Simon Glass]

This series includes a number of minor fixes and improvements identified
while trying out Chrome OS verified boot on it.

These were not apparent without this real-world testing, but all are
deficiencies in one way or another.


Simon Glass (10):
  dtoc: binman: Drop Python 2 code
  patman: Correct lz4 compression parameters
  binman: Update the TODO list
  binman: Show the size when writing entries
  binman: Fix a few file comments
  binman: Support finding symbols in sub-sections
  binman: Support reading an image with entry args
  binman: Allow vblock to include devicetree blobs
  binman: Support alignment of files
  binman: Allow for skip_at_start when reading entries

 tools/binman/README                       |   3 +-
 tools/binman/control.py                   |   3 +-
 tools/binman/elf.py                       |   3 +-
 tools/binman/elf_test.py                  |   4 +-
 tools/binman/entry.py                     |   3 +-
 tools/binman/etype/blob.py                |   4 +
 tools/binman/etype/files.py               |   4 +
 tools/binman/etype/section.py             |  66 +++++++++++--
 tools/binman/etype/u_boot_spl_bss_pad.py  |   1 -
 tools/binman/etype/u_boot_spl_nodtb.py    |   2 +-
 tools/binman/etype/vblock.py              |  15 ++-
 tools/binman/fmap_util.py                 |   4 +-
 tools/binman/ftest.py                     | 110 +++++++++++++++++++++-
 tools/binman/image.py                     |  62 ++++++++++--
 tools/binman/state.py                     |  10 ++
 tools/binman/test/084_files.dts           |   2 +-
 tools/binman/test/187_symbols_sub.dts     |  22 +++++
 tools/binman/test/188_image_entryarg.dts  |  21 +++++
 tools/binman/test/189_vblock_content.dts  |  31 ++++++
 tools/binman/test/190_files_align.dts     |  12 +++
 tools/binman/test/191_read_image_skip.dts |  23 +++++
 tools/dtoc/fdt.py                         |  15 ++-
 tools/dtoc/test_fdt.py                    |   6 ++
 tools/patman/tools.py                     |   3 +-
 24 files changed, 393 insertions(+), 36 deletions(-)
 create mode 100644 tools/binman/test/187_symbols_sub.dts
 create mode 100644 tools/binman/test/188_image_entryarg.dts
 create mode 100644 tools/binman/test/189_vblock_content.dts
 create mode 100644 tools/binman/test/190_files_align.dts
 create mode 100644 tools/binman/test/191_read_image_skip.dts

-- 
2.29.2.729.g45daf8777d-goog

[PATCH 01/10] dtoc: binman: Drop Python 2 code

[Simon Glass]

Drop a few more Python 2 relics that are no-longer needed.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 tools/binman/fmap_util.py | 4 ++--
 tools/dtoc/fdt.py         | 3 +--
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/tools/binman/fmap_util.py b/tools/binman/fmap_util.py
index b03fc28fbb4..8277619768c 100644
--- a/tools/binman/fmap_util.py
+++ b/tools/binman/fmap_util.py
@@ -53,8 +53,8 @@ FmapArea = collections.namedtuple('FmapArea', FMAP_AREA_NAMES)
 
 
 def NameToFmap(name):
-    if type(name) == bytes and sys.version_info[0] >= 3:
-        name = name.decode('utf-8')  # pragma: no cover (for Python 2)
+    if type(name) == bytes:
+        name = name.decode('utf-8')
     return name.replace('\0', '').replace('-', '_').upper()
 
 def ConvertName(field_names, fields):
diff --git a/tools/dtoc/fdt.py b/tools/dtoc/fdt.py
index 4a78c737252..965106a3b28 100644
--- a/tools/dtoc/fdt.py
+++ b/tools/dtoc/fdt.py
@@ -460,8 +460,7 @@ class Node:
             prop_name: Name of property to add
             val: String value of property
         """
-        if sys.version_info[0] >= 3:  # pragma: no cover
-            val = bytes(val, 'utf-8')
+        val = bytes(val, 'utf-8')
         self.AddData(prop_name, val + b'\0')
 
     def AddSubnode(self, name):
-- 
2.29.2.729.g45daf8777d-goog

[PATCH 02/10] patman: Correct lz4 compression parameters

[Simon Glass]

At present on large files, lz4 uses a larger block size (e.g. 256KB) than
the 64KB supported by the U-Boot decompression implementation. Also it is
optimised for maximum compression speed, producing larger output than we
would like.

Update the parameters to correct these problems.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 tools/patman/tools.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/patman/tools.py b/tools/patman/tools.py
index d8e01a3e60e..10997e43868 100644
--- a/tools/patman/tools.py
+++ b/tools/patman/tools.py
@@ -476,7 +476,8 @@ def Compress(indata, algo, with_header=True):
     fname = GetOutputFilename('%s.comp.tmp' % algo)
     WriteFile(fname, indata)
     if algo == 'lz4':
-        data = Run('lz4', '--no-frame-crc', '-c', fname, binary=True)
+        data = Run('lz4', '--no-frame-crc', '-B4', '-5', '-c', fname,
+                   binary=True)
     # cbfstool uses a very old version of lzma
     elif algo == 'lzma':
         outfname = GetOutputFilename('%s.comp.otmp' % algo)
-- 
2.29.2.729.g45daf8777d-goog

[PATCH 03/10] binman: Update the TODO list

[Simon Glass]

Two of the items have been completed and I thought of another one. Update
the list.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 tools/binman/README | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/binman/README b/tools/binman/README
index de1eedfc3f7..a00c9026163 100644
--- a/tools/binman/README
+++ b/tools/binman/README
@@ -1050,10 +1050,9 @@ Some ideas:
 - Allow easy building of images by specifying just the board name
 - Support building an image for a board (-b) more completely, with a
   configurable build directory
-- Support adding FITs to an image
-- Support for ARM Trusted Firmware (ATF)
 - Detect invalid properties in nodes
 - Sort the fdtmap by offset
+- Output temporary files to a different directory
 
 --
 Simon Glass <sjg@chromium.org>
-- 
2.29.2.729.g45daf8777d-goog

[PATCH 04/10] binman: Show the size when writing entries

[Simon Glass]

Update the log output to show the size, since this is useful information.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 tools/binman/control.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/binman/control.py b/tools/binman/control.py
index 072417f3644..1952b2abf48 100644
--- a/tools/binman/control.py
+++ b/tools/binman/control.py
@@ -244,7 +244,8 @@ def ExtractEntries(image_fname, output_fname, outdir, entry_paths,
             if not os.path.exists(fname):
                 os.makedirs(fname)
             fname = os.path.join(fname, 'root')
-        tout.Notice("Write entry '%s' to '%s'" % (entry.GetPath(), fname))
+        tout.Notice("Write entry '%s' size %x to '%s'" %
+                    (entry.GetPath(), len(data), fname))
         tools.WriteFile(fname, data)
     return einfos
 
-- 
2.29.2.729.g45daf8777d-goog

[PATCH 05/10] binman: Fix a few file comments

[Simon Glass]

Two files have the wrong comment at the top of them. Fix this.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 tools/binman/etype/u_boot_spl_bss_pad.py | 1 -
 tools/binman/etype/u_boot_spl_nodtb.py   | 2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/binman/etype/u_boot_spl_bss_pad.py b/tools/binman/etype/u_boot_spl_bss_pad.py
index 596b2bed97e..df15cd24ce7 100644
--- a/tools/binman/etype/u_boot_spl_bss_pad.py
+++ b/tools/binman/etype/u_boot_spl_bss_pad.py
@@ -9,7 +9,6 @@
 
 from binman import elf
 from binman.entry import Entry
-from patman import command
 from binman.etype.blob import Entry_blob
 from patman import tools
 
diff --git a/tools/binman/etype/u_boot_spl_nodtb.py b/tools/binman/etype/u_boot_spl_nodtb.py
index 6f4529396d8..c154cfde57b 100644
--- a/tools/binman/etype/u_boot_spl_nodtb.py
+++ b/tools/binman/etype/u_boot_spl_nodtb.py
@@ -2,7 +2,7 @@
 # Copyright (c) 2016 Google, Inc
 # Written by Simon Glass <sjg@chromium.org>
 #
-# Entry-type module for 'u-boot-nodtb.bin'
+# Entry-type module for 'u-boot-spl-nodtb.bin'
 #
 
 from binman.entry import Entry
-- 
2.29.2.729.g45daf8777d-goog

[PATCH 06/10] binman: Support finding symbols in sub-sections

[Simon Glass]

At present binman only supports resolving symbols in the same section as
the binary that uses it. This is quite limited because we often need to
group entries into different sections.

Enhance the algorithm to search the entire image for symbols.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 tools/binman/elf.py                   |  3 +-
 tools/binman/elf_test.py              |  4 ++-
 tools/binman/etype/section.py         | 41 +++++++++++++++++++++---
 tools/binman/ftest.py                 | 15 +++++++++
 tools/binman/image.py                 | 45 +++++++++++++++++++++++++++
 tools/binman/test/187_symbols_sub.dts | 22 +++++++++++++
 6 files changed, 123 insertions(+), 7 deletions(-)
 create mode 100644 tools/binman/test/187_symbols_sub.dts

diff --git a/tools/binman/elf.py b/tools/binman/elf.py
index 249074a334a..03b49d7163c 100644
--- a/tools/binman/elf.py
+++ b/tools/binman/elf.py
@@ -132,7 +132,8 @@ def LookupAndWriteSymbols(elf_fname, entry, section):
                                  (msg, sym.size))
 
             # Look up the symbol in our entry tables.
-            value = section.LookupSymbol(name, sym.weak, msg, base.address)
+            value = section.GetImage().LookupImageSymbol(name, sym.weak, msg,
+                                                         base.address)
             if value is None:
                 value = -1
                 pack_string = pack_string.lower()
diff --git a/tools/binman/elf_test.py b/tools/binman/elf_test.py
index e3d218a89e9..7a128018d9f 100644
--- a/tools/binman/elf_test.py
+++ b/tools/binman/elf_test.py
@@ -45,10 +45,12 @@ class FakeSection:
     def GetPath(self):
         return 'section_path'
 
-    def LookupSymbol(self, name, weak, msg, base_addr):
+    def LookupImageSymbol(self, name, weak, msg, base_addr):
         """Fake implementation which returns the same value for all symbols"""
         return self.sym_value
 
+    def GetImage(self):
+        return self
 
 def BuildElfTestFiles(target_dir):
     """Build ELF files used for testing in binman
diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py
index 3dd5f58c4c2..9c6334c4504 100644
--- a/tools/binman/etype/section.py
+++ b/tools/binman/etype/section.py
@@ -385,7 +385,7 @@ class Entry_section(Entry):
                 return entry.GetData()
         source_entry.Raise("Cannot find entry for node '%s'" % node.name)
 
-    def LookupSymbol(self, sym_name, optional, msg, base_addr):
+    def LookupSymbol(self, sym_name, optional, msg, base_addr, entries=None):
         """Look up a symbol in an ELF file
 
         Looks up a symbol in an ELF file. Only entry types which come from an
@@ -428,18 +428,20 @@ class Entry_section(Entry):
                              (msg, sym_name))
         entry_name, prop_name = m.groups()
         entry_name = entry_name.replace('_', '-')
-        entry = self._entries.get(entry_name)
+        if not entries:
+            entries = self._entries
+        entry = entries.get(entry_name)
         if not entry:
             if entry_name.endswith('-any'):
                 root = entry_name[:-4]
-                for name in self._entries:
+                for name in entries:
                     if name.startswith(root):
                         rest = name[len(root):]
                         if rest in ['', '-img', '-nodtb']:
-                            entry = self._entries[name]
+                            entry = entries[name]
         if not entry:
             err = ("%s: Entry '%s' not found in list (%s)" %
-                   (msg, entry_name, ','.join(self._entries.keys())))
+                   (msg, entry_name, ','.join(entries.keys())))
             if optional:
                 print('Warning: %s' % err, file=sys.stderr)
                 return None
@@ -648,3 +650,32 @@ class Entry_section(Entry):
         """
         for entry in self._entries.values():
             entry.CheckMissing(missing_list)
+
+    def _CollectEntries(self, entries, entries_by_name, add_entry):
+        """Collect all the entries in an section
+
+        This builds up a dict of entries in this section and all subsections.
+        Entries are indexed by path and by name.
+
+        Since all paths are unique, entries will not have any conflicts. However
+        entries_by_name make have conflicts if two entries have the same name
+        (e.g. with different parent sections). In this case, an entry at a
+        higher level in the hierarchy will win over a lower-level entry.
+
+        Args:
+            entries: dict to put entries:
+                key: entry path
+                value: Entry object
+            entries_by_name: dict to put entries
+                key: entry name
+                value: Entry object
+            add_entry: Entry to add
+        """
+        entries[add_entry.GetPath()] = add_entry
+        to_add = add_entry.GetEntries()
+        if to_add:
+            for entry in to_add.values():
+                entries[entry.GetPath()] = entry
+            for entry in to_add.values():
+                self._CollectEntries(entries, entries_by_name, entry)
+        entries_by_name[add_entry.name] = add_entry
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index e753a342c8f..5016060ea68 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -4139,6 +4139,21 @@ class TestFunctional(unittest.TestCase):
             }
         self.assertEqual(expected, props)
 
+    def testSymbolsSubsection(self):
+        """Test binman can assign symbols from a subsection"""
+        elf_fname = self.ElfTestFile('u_boot_binman_syms')
+        syms = elf.GetSymbols(elf_fname, ['binman', 'image'])
+        addr = elf.GetSymbolAddress(elf_fname, '__image_copy_start')
+        self.assertEqual(syms['_binman_u_boot_spl_prop_offset'].address, addr)
+
+        self._SetupSplElf('u_boot_binman_syms')
+        data = self._DoReadFile('187_symbols_sub.dts')
+        sym_values = struct.pack('<LQLL', 0x00, 0x1c, 0x28, 0x04)
+        expected = (sym_values + U_BOOT_SPL_DATA[20:] +
+                    tools.GetBytes(0xff, 1) + U_BOOT_DATA + sym_values +
+                    U_BOOT_SPL_DATA[20:])
+        self.assertEqual(expected, data)
+
 
 if __name__ == "__main__":
     unittest.main()
diff --git a/tools/binman/image.py b/tools/binman/image.py
index d65ab887b80..3622efc6862 100644
--- a/tools/binman/image.py
+++ b/tools/binman/image.py
@@ -324,3 +324,48 @@ class Image(section.Entry_section):
             _DoLine(lines, _EntryToStrings(entry))
             selected_entries.append(entry)
         return selected_entries, lines, widths
+
+    def LookupImageSymbol(self, sym_name, optional, msg, base_addr):
+        """Look up a symbol in an ELF file
+
+        Looks up a symbol in an ELF file. Only entry types which come from an
+        ELF image can be used by this function.
+
+        This searches through this image including all of its subsections.
+
+        At present the only entry properties supported are:
+            offset
+            image_pos - 'base_addr' is added if this is not an end-at-4gb image
+            size
+
+        Args:
+            sym_name: Symbol name in the ELF file to look up in the format
+                _binman_<entry>_prop_<property> where <entry> is the name of
+                the entry and <property> is the property to find (e.g.
+                _binman_u_boot_prop_offset). As a special case, you can append
+                _any to <entry> to have it search for any matching entry. E.g.
+                _binman_u_boot_any_prop_offset will match entries called u-boot,
+                u-boot-img and u-boot-nodtb)
+            optional: True if the symbol is optional. If False this function
+                will raise if the symbol is not found
+            msg: Message to display if an error occurs
+            base_addr: Base address of image. This is added to the returned
+                image_pos in most cases so that the returned position indicates
+                where the targeted entry/binary has actually been loaded. But
+                if end-at-4gb is used, this is not done, since the binary is
+                already assumed to be linked to the ROM position and using
+                execute-in-place (XIP).
+
+        Returns:
+            Value that should be assigned to that symbol, or None if it was
+                optional and not found
+
+        Raises:
+            ValueError if the symbol is invalid or not found, or references a
+                property which is not supported
+        """
+        entries = OrderedDict()
+        entries_by_name = {}
+        self._CollectEntries(entries, entries_by_name, self)
+        return self.LookupSymbol(sym_name, optional, msg, base_addr,
+                                 entries_by_name)
diff --git a/tools/binman/test/187_symbols_sub.dts b/tools/binman/test/187_symbols_sub.dts
new file mode 100644
index 00000000000..54511a73711
--- /dev/null
+++ b/tools/binman/test/187_symbols_sub.dts
@@ -0,0 +1,22 @@
+/dts-v1/;
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	binman {
+		section {
+			pad-byte = <0xff>;
+			u-boot-spl {
+			};
+
+			u-boot {
+				offset = <24>;
+			};
+		};
+
+		u-boot-spl2 {
+			type = "u-boot-spl";
+		};
+	};
+};
-- 
2.29.2.729.g45daf8777d-goog

[PATCH 07/10] binman: Support reading an image with entry args

[Simon Glass]

Normally when an entry is created, any entry arguments it has are required
to be provided, so it can actually generate its contents correctly.

However when an existing image is read, Entry objects are created for each
of the entries in the image. This happens as part of the process of
reading the image into binman.

In this case we don't need the entry arguments, since we do not intend to
regenerate the entries, or at least not unless requested. So there is no
sense in reporting an error for missing entry arguments.

Add a new property for the Image to handle this case. Update the error
reporting to be conditional on this property.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 tools/binman/entry.py                    |  3 +--
 tools/binman/etype/section.py            | 15 +++++++++++++++
 tools/binman/ftest.py                    | 19 +++++++++++++++++++
 tools/binman/image.py                    | 10 ++++++++--
 tools/binman/test/188_image_entryarg.dts | 21 +++++++++++++++++++++
 5 files changed, 64 insertions(+), 4 deletions(-)
 create mode 100644 tools/binman/test/188_image_entryarg.dts

diff --git a/tools/binman/entry.py b/tools/binman/entry.py
index 2be0d8e0532..d58a730f3d5 100644
--- a/tools/binman/entry.py
+++ b/tools/binman/entry.py
@@ -434,8 +434,7 @@ class Entry(object):
                 missing.append(prop.name)
             values.append(value)
         if missing:
-            self.Raise('Missing required properties/entry args: %s' %
-                       (', '.join(missing)))
+            self.GetImage().MissingArgs(self, missing)
         return values
 
     def GetPath(self):
diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py
index 9c6334c4504..28a888776af 100644
--- a/tools/binman/etype/section.py
+++ b/tools/binman/etype/section.py
@@ -679,3 +679,18 @@ class Entry_section(Entry):
             for entry in to_add.values():
                 self._CollectEntries(entries, entries_by_name, entry)
         entries_by_name[add_entry.name] = add_entry
+
+    def MissingArgs(self, entry, missing):
+        """Report a missing argument, if enabled
+
+        For entries which require arguments, this reports an error if some are
+        missing. If missing entries are being ignored (e.g. because we read the
+        entry from an image rather than creating it), this function does
+        nothing.
+
+        Args:
+            missing: List of missing properties / entry args, each a string
+        """
+        if not self._ignore_missing:
+            entry.Raise('Missing required properties/entry args: %s' %
+                       (', '.join(missing)))
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index 5016060ea68..8b928eb406d 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -4154,6 +4154,25 @@ class TestFunctional(unittest.TestCase):
                     U_BOOT_SPL_DATA[20:])
         self.assertEqual(expected, data)
 
+    def testReadImageEntryArg(self):
+        """Test reading an image that would need an entry arg to generate"""
+        entry_args = {
+            'cros-ec-rw-path': 'ecrw.bin',
+        }
+        data = self.data = self._DoReadFileDtb(
+            '188_image_entryarg.dts',use_real_dtb=True, update_dtb=True,
+            entry_args=entry_args)
+
+        image_fname = tools.GetOutputFilename('image.bin')
+        orig_image = control.images['image']
+
+        # This should not generate an error about the missing 'cros-ec-rw-path'
+        # since we are reading the image from a file. Compare with
+        # testEntryArgsRequired()
+        image = Image.FromFile(image_fname)
+        self.assertEqual(orig_image.GetEntries().keys(),
+                         image.GetEntries().keys())
+
 
 if __name__ == "__main__":
     unittest.main()
diff --git a/tools/binman/image.py b/tools/binman/image.py
index 3622efc6862..3c2fe5ea620 100644
--- a/tools/binman/image.py
+++ b/tools/binman/image.py
@@ -43,8 +43,13 @@ class Image(section.Entry_section):
         test: True if this is being called from a test of Images. This this case
             there is no device tree defining the structure of the section, so
             we create a section manually.
+        ignore_missing: Ignore any missing entry arguments (i.e. don't raise an
+            exception). This should be used if the Image is being loaded from
+            a file rather than generated. In that case we obviously don't need
+            the entry arguments since the contents already exists.
     """
-    def __init__(self, name, node, copy_to_orig=True, test=False):
+    def __init__(self, name, node, copy_to_orig=True, test=False,
+                 ignore_missing=False):
         super().__init__(None, 'section', node, test=test)
         self.copy_to_orig = copy_to_orig
         self.name = 'main-section'
@@ -53,6 +58,7 @@ class Image(section.Entry_section):
         self.fdtmap_dtb = None
         self.fdtmap_data = None
         self.allow_repack = False
+        self._ignore_missing = ignore_missing
         if not test:
             self.ReadNode()
 
@@ -100,7 +106,7 @@ class Image(section.Entry_section):
 
         # Return an Image with the associated nodes
         root = dtb.GetRoot()
-        image = Image('image', root, copy_to_orig=False)
+        image = Image('image', root, copy_to_orig=False, ignore_missing=True)
 
         image.image_node = fdt_util.GetString(root, 'image-node', 'image')
         image.fdtmap_dtb = dtb
diff --git a/tools/binman/test/188_image_entryarg.dts b/tools/binman/test/188_image_entryarg.dts
new file mode 100644
index 00000000000..29d81491623
--- /dev/null
+++ b/tools/binman/test/188_image_entryarg.dts
@@ -0,0 +1,21 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+/dts-v1/;
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	binman {
+		size = <0xc00>;
+		u-boot {
+		};
+		cros-ec-rw {
+		};
+		fdtmap {
+		};
+		image-header {
+			location = "end";
+		};
+	};
+};
-- 
2.29.2.729.g45daf8777d-goog

[PATCH 08/10] binman: Allow vblock to include devicetree blobs

[Simon Glass]

At present if a devicetree blob is included in a vblock it does not deal
with updates. This is because the vblock is created once at the start and
does not have a method to update itself later, after all the entry
contents are finalised.

Fix this by adjusting how the vblock is created.

Also simplify Image.ProcessEntryContents() since it effectively duplicates
the code in Section.ProcessContents().

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 tools/binman/etype/blob.py               |  4 ++
 tools/binman/etype/vblock.py             | 15 +++++++-
 tools/binman/ftest.py                    | 49 +++++++++++++++++++++++-
 tools/binman/image.py                    |  7 +---
 tools/binman/test/189_vblock_content.dts | 31 +++++++++++++++
 5 files changed, 96 insertions(+), 10 deletions(-)
 create mode 100644 tools/binman/test/189_vblock_content.dts

diff --git a/tools/binman/etype/blob.py b/tools/binman/etype/blob.py
index 301ac55e3b2..81756c326d9 100644
--- a/tools/binman/etype/blob.py
+++ b/tools/binman/etype/blob.py
@@ -66,3 +66,7 @@ class Entry_blob(Entry):
 
     def GetDefaultFilename(self):
         return self._filename
+
+    def ProcessContents(self):
+        # The blob may have changed due to WriteSymbols()
+        return self.ProcessContentsUpdate(self.data)
diff --git a/tools/binman/etype/vblock.py b/tools/binman/etype/vblock.py
index f734fbaec49..eba5351dd52 100644
--- a/tools/binman/etype/vblock.py
+++ b/tools/binman/etype/vblock.py
@@ -49,7 +49,7 @@ class Entry_vblock(Entry):
             EntryArg('kernelkey', str),
             EntryArg('preamble-flags', int)])
 
-    def ObtainContents(self):
+    def GetVblock(self):
         # Join up the data files to be signed
         input_data = b''
         for entry_phandle in self.content:
@@ -76,5 +76,16 @@ class Entry_vblock(Entry):
         ]
         #out.Notice("Sign '%s' into %s" % (', '.join(self.value), self.label))
         stdout = tools.Run('futility', *args)
-        self.SetContents(tools.ReadFile(output_fname))
+        return tools.ReadFile(output_fname)
+
+    def ObtainContents(self):
+        data = self.GetVblock()
+        if data is False:
+            return False
+        self.SetContents(data)
         return True
+
+    def ProcessContents(self):
+        # The blob may have changed due to WriteSymbols()
+        data = self.GetVblock()
+        return self.ProcessContentsUpdate(data)
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index 8b928eb406d..7f7827b6a7d 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -1638,15 +1638,37 @@ class TestFunctional(unittest.TestCase):
                       str(e.exception))
 
     def _HandleVblockCommand(self, pipe_list):
-        """Fake calls to the futility utility"""
+        """Fake calls to the futility utility
+
+        The expected pipe is:
+
+           [('futility', 'vbutil_firmware', '--vblock',
+             'vblock.vblock', '--keyblock', 'devkeys/firmware.keyblock',
+             '--signprivate', 'devkeys/firmware_data_key.vbprivk',
+             '--version', '1', '--fv', 'input.vblock', '--kernelkey',
+             'devkeys/kernel_subkey.vbpubk', '--flags', '1')]
+
+        This writes to the output file (here, 'vblock.vblock'). If
+        self._hash_data is False, it writes VBLOCK_DATA, else it writes a hash
+        of the input data (here, 'input.vblock').
+        """
         if pipe_list[0][0] == 'futility':
             fname = pipe_list[0][3]
             with open(fname, 'wb') as fd:
-                fd.write(VBLOCK_DATA)
+                if self._hash_data:
+                    infile = pipe_list[0][11]
+                    m = hashlib.sha256()
+                    data = tools.ReadFile(infile)
+                    m.update(data)
+                    fd.write(m.digest())
+                else:
+                    fd.write(VBLOCK_DATA)
+
             return command.CommandResult()
 
     def testVblock(self):
         """Test for the Chromium OS Verified Boot Block"""
+        self._hash_data = False
         command.test_result = self._HandleVblockCommand
         entry_args = {
             'keydir': 'devkeys',
@@ -1677,6 +1699,29 @@ class TestFunctional(unittest.TestCase):
         self.assertIn("Node '/binman/vblock': Cannot find entry for node "
                       "'other'", str(e.exception))
 
+    def testVblockContent(self):
+        """Test that the vblock signs the right data"""
+        self._hash_data = True
+        command.test_result = self._HandleVblockCommand
+        entry_args = {
+            'keydir': 'devkeys',
+        }
+        data = self._DoReadFileDtb(
+            '189_vblock_content.dts', use_real_dtb=True, update_dtb=True,
+            entry_args=entry_args)[0]
+        hashlen = 32  # SHA256 hash is 32 bytes
+        self.assertEqual(U_BOOT_DATA, data[:len(U_BOOT_DATA)])
+        hashval = data[-hashlen:]
+        dtb = data[len(U_BOOT_DATA):-hashlen]
+
+        expected_data = U_BOOT_DATA + dtb
+
+        # The hashval should be a hash of the dtb
+        m = hashlib.sha256()
+        m.update(expected_data)
+        expected_hashval = m.digest()
+        self.assertEqual(expected_hashval, hashval)
+
     def testTpl(self):
         """Test that an image with TPL and its device tree can be created"""
         # ELF file with a '__bss_size' symbol
diff --git a/tools/binman/image.py b/tools/binman/image.py
index 3c2fe5ea620..e9494352413 100644
--- a/tools/binman/image.py
+++ b/tools/binman/image.py
@@ -136,12 +136,7 @@ class Image(section.Entry_section):
         Returns:
             True if the new data size is OK, False if expansion is needed
         """
-        sizes_ok = True
-        for entry in self._entries.values():
-            if not entry.ProcessContents():
-                sizes_ok = False
-                tout.Debug("Entry '%s' size change" % self._node.path)
-        return sizes_ok
+        return super().ProcessContents()
 
     def WriteSymbols(self):
         """Write symbol values into binary files for access at run time"""
diff --git a/tools/binman/test/189_vblock_content.dts b/tools/binman/test/189_vblock_content.dts
new file mode 100644
index 00000000000..dcc74449c17
--- /dev/null
+++ b/tools/binman/test/189_vblock_content.dts
@@ -0,0 +1,31 @@
+// SPDX-License-Identifier: GPL-2.0+
+/dts-v1/;
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	binman {
+		u_boot: u-boot {
+		};
+
+		dtb: u-boot-dtb {
+		};
+
+		/*
+		 * Put the vblock after the dtb so that the dtb is updated
+		 * before the vblock reads its data. At present binman does not
+		 * understand dependencies between entries, but simply
+		 * iterates again when it thinks something needs to be
+		 * recalculated.
+		 */
+		vblock {
+			content = <&u_boot &dtb>;
+			keyblock = "firmware.keyblock";
+			signprivate = "firmware_data_key.vbprivk";
+			version = <1>;
+			kernelkey = "kernel_subkey.vbpubk";
+			preamble-flags = <1>;
+		};
+	};
+};
-- 
2.29.2.729.g45daf8777d-goog

[PATCH 09/10] binman: Support alignment of files

[Simon Glass]

When packing files it is sometimes useful to align the start of each file,
e.g. if the flash driver can only access 32-bit-aligned data. Provides a
new property to support this.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 tools/binman/etype/files.py           |  4 ++++
 tools/binman/ftest.py                 |  8 ++++++++
 tools/binman/state.py                 | 10 ++++++++++
 tools/binman/test/084_files.dts       |  2 +-
 tools/binman/test/190_files_align.dts | 12 ++++++++++++
 tools/dtoc/fdt.py                     | 12 ++++++++++++
 tools/dtoc/test_fdt.py                |  6 ++++++
 7 files changed, 53 insertions(+), 1 deletion(-)
 create mode 100644 tools/binman/test/190_files_align.dts

diff --git a/tools/binman/etype/files.py b/tools/binman/etype/files.py
index ce3832e3cdd..1feebd0510e 100644
--- a/tools/binman/etype/files.py
+++ b/tools/binman/etype/files.py
@@ -22,6 +22,7 @@ class Entry_files(Entry_section):
         - files-compress: Compression algorithm to use:
             none: No compression
             lz4: Use lz4 compression (via 'lz4' command-line utility)
+        - files-align: Align each file to the given alignment
 
     This entry reads a number of files and places each in a separate sub-entry
     within this entry. To access these you need to enable device-tree updates
@@ -38,6 +39,7 @@ class Entry_files(Entry_section):
             self.Raise("Missing 'pattern' property")
         self._files_compress = fdt_util.GetString(self._node, 'files-compress',
                                                   'none')
+        self._files_align = fdt_util.GetInt(self._node, 'files-align');
         self._require_matches = fdt_util.GetBool(self._node,
                                                 'require-matches')
 
@@ -55,6 +57,8 @@ class Entry_files(Entry_section):
             state.AddString(subnode, 'type', 'blob')
             state.AddString(subnode, 'filename', fname)
             state.AddString(subnode, 'compress', self._files_compress)
+            if self._files_align:
+                state.AddInt(subnode, 'align', self._files_align)
 
         # Read entries again, now that we have some
         self._ReadEntries()
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index 7f7827b6a7d..b31a7305a33 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -4218,6 +4218,14 @@ class TestFunctional(unittest.TestCase):
         self.assertEqual(orig_image.GetEntries().keys(),
                          image.GetEntries().keys())
 
+    def testFilesAlign(self):
+        """Test alignment with files"""
+        data = self._DoReadFile('190_files_align.dts')
+
+        # The first string is 15 bytes so will align to 16
+        expect = FILES_DATA[:15] + b'\0' + FILES_DATA[15:]
+        self.assertEqual(expect, data)
+
 
 if __name__ == "__main__":
     unittest.main()
diff --git a/tools/binman/state.py b/tools/binman/state.py
index 36bc5135354..bb3e36ea7af 100644
--- a/tools/binman/state.py
+++ b/tools/binman/state.py
@@ -314,6 +314,16 @@ def AddString(node, prop, value):
     for n in GetUpdateNodes(node):
         n.AddString(prop, value)
 
+def AddInt(node, prop, value):
+    """Add a new string property to affected device trees
+
+    Args:
+        prop_name: Name of property
+        val: Integer value of property
+    """
+    for n in GetUpdateNodes(node):
+        n.AddInt(prop, value)
+
 def SetInt(node, prop, value, for_repack=False):
     """Update an integer property in affected device trees with an integer value
 
diff --git a/tools/binman/test/084_files.dts b/tools/binman/test/084_files.dts
index 83ddb78f8e7..8f09afd24ea 100644
--- a/tools/binman/test/084_files.dts
+++ b/tools/binman/test/084_files.dts
@@ -5,7 +5,7 @@
 	binman {
 		files {
 			pattern = "files/*.dat";
-			compress = "none";
+			files-compress = "none";
 		};
 	};
 };
diff --git a/tools/binman/test/190_files_align.dts b/tools/binman/test/190_files_align.dts
new file mode 100644
index 00000000000..213ba966d35
--- /dev/null
+++ b/tools/binman/test/190_files_align.dts
@@ -0,0 +1,12 @@
+// SPDX-License-Identifier: GPL-2.0+
+/dts-v1/;
+
+/ {
+	binman {
+		files {
+			pattern = "files/*.dat";
+			files-compress = "none";
+			files-align = <4>;
+		};
+	};
+};
diff --git a/tools/dtoc/fdt.py b/tools/dtoc/fdt.py
index 965106a3b28..25ce5136ebf 100644
--- a/tools/dtoc/fdt.py
+++ b/tools/dtoc/fdt.py
@@ -463,6 +463,18 @@ class Node:
         val = bytes(val, 'utf-8')
         self.AddData(prop_name, val + b'\0')
 
+    def AddInt(self, prop_name, val):
+        """Add a new integer property to a node
+
+        The device tree is marked dirty so that the value will be written to
+        the blob on the next sync.
+
+        Args:
+            prop_name: Name of property to add
+            val: Integer value of property
+        """
+        self.AddData(prop_name, struct.pack('>I', val))
+
     def AddSubnode(self, name):
         """Add a new subnode to the node
 
diff --git a/tools/dtoc/test_fdt.py b/tools/dtoc/test_fdt.py
index dc6943f7337..e8fbbd5d10a 100755
--- a/tools/dtoc/test_fdt.py
+++ b/tools/dtoc/test_fdt.py
@@ -397,6 +397,12 @@ class TestProp(unittest.TestCase):
         data = self.fdt.getprop(self.node.Offset(), 'one')
         self.assertEqual(1, fdt32_to_cpu(data))
 
+        val = 1234
+        self.node.AddInt('integer', val)
+        self.dtb.Sync(auto_resize=True)
+        data = self.fdt.getprop(self.node.Offset(), 'integer')
+        self.assertEqual(val, fdt32_to_cpu(data))
+
         val = '123' + chr(0) + '456'
         self.node.AddString('string', val)
         self.dtb.Sync(auto_resize=True)
-- 
2.29.2.729.g45daf8777d-goog

[PATCH 10/10] binman: Allow for skip_at_start when reading entries

[Simon Glass]

The offset of an entry needs to be adjusted by its skip-at-start value.
This is currently missing when reading entry data. Fix it.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 tools/binman/etype/section.py             | 10 ++++++----
 tools/binman/ftest.py                     | 19 +++++++++++++++++++
 tools/binman/test/191_read_image_skip.dts | 23 +++++++++++++++++++++++
 3 files changed, 48 insertions(+), 4 deletions(-)
 create mode 100644 tools/binman/test/191_read_image_skip.dts

diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py
index 28a888776af..1ceadef13f3 100644
--- a/tools/binman/etype/section.py
+++ b/tools/binman/etype/section.py
@@ -605,10 +605,12 @@ class Entry_section(Entry):
     def ReadData(self, decomp=True):
         tout.Info("ReadData path='%s'" % self.GetPath())
         parent_data = self.section.ReadData(True)
-        tout.Info('%s: Reading data from offset %#x-%#x, size %#x' %
-                  (self.GetPath(), self.offset, self.offset + self.size,
-                   self.size))
-        data = parent_data[self.offset:self.offset + self.size]
+        offset = self.offset - self.section._skip_at_start
+        data = parent_data[offset:offset + self.size]
+        tout.Info(
+            '%s: Reading data from offset %#x-%#x (real %#x), size %#x, got %#x' %
+                  (self.GetPath(), self.offset, self.offset + self.size, offset,
+                   self.size, len(data)))
         return data
 
     def ReadChildData(self, child, decomp=True):
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index b31a7305a33..814e91d42e9 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -4226,6 +4226,25 @@ class TestFunctional(unittest.TestCase):
         expect = FILES_DATA[:15] + b'\0' + FILES_DATA[15:]
         self.assertEqual(expect, data)
 
+    def testReadImageSkip(self):
+        """Test reading an image and accessing its FDT map"""
+        data = self.data = self._DoReadFileRealDtb('191_read_image_skip.dts')
+        image_fname = tools.GetOutputFilename('image.bin')
+        orig_image = control.images['image']
+        image = Image.FromFile(image_fname)
+        self.assertEqual(orig_image.GetEntries().keys(),
+                         image.GetEntries().keys())
+
+        orig_entry = orig_image.GetEntries()['fdtmap']
+        entry = image.GetEntries()['fdtmap']
+        self.assertEqual(orig_entry.offset, entry.offset)
+        self.assertEqual(orig_entry.size, entry.size)
+        self.assertEqual(16, entry.image_pos)
+
+        u_boot = image.GetEntries()['section'].GetEntries()['u-boot']
+
+        self.assertEquals(U_BOOT_DATA, u_boot.ReadData())
+
 
 if __name__ == "__main__":
     unittest.main()
diff --git a/tools/binman/test/191_read_image_skip.dts b/tools/binman/test/191_read_image_skip.dts
new file mode 100644
index 00000000000..31df518fae6
--- /dev/null
+++ b/tools/binman/test/191_read_image_skip.dts
@@ -0,0 +1,23 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+/dts-v1/;
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	binman {
+		end-at-4gb;
+		size = <0x400>;
+		section {
+			size = <0x10>;
+			u-boot {
+			};
+		};
+		fdtmap {
+		};
+		image-header {
+			location = "end";
+		};
+	};
+};
-- 
2.29.2.729.g45daf8777d-goog

[PATCH 10/10] binman: Allow for skip_at_start when reading entries

[Simon Glass]

The offset of an entry needs to be adjusted by its skip-at-start value.
This is currently missing when reading entry data. Fix it.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 tools/binman/etype/section.py             | 10 ++++++----
 tools/binman/ftest.py                     | 19 +++++++++++++++++++
 tools/binman/test/191_read_image_skip.dts | 23 +++++++++++++++++++++++
 3 files changed, 48 insertions(+), 4 deletions(-)
 create mode 100644 tools/binman/test/191_read_image_skip.dts

Applied to u-boot-dm, thanks!

[PATCH 09/10] binman: Support alignment of files

[Simon Glass]

When packing files it is sometimes useful to align the start of each file,
e.g. if the flash driver can only access 32-bit-aligned data. Provides a
new property to support this.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 tools/binman/etype/files.py           |  4 ++++
 tools/binman/ftest.py                 |  8 ++++++++
 tools/binman/state.py                 | 10 ++++++++++
 tools/binman/test/084_files.dts       |  2 +-
 tools/binman/test/190_files_align.dts | 12 ++++++++++++
 tools/dtoc/fdt.py                     | 12 ++++++++++++
 tools/dtoc/test_fdt.py                |  6 ++++++
 7 files changed, 53 insertions(+), 1 deletion(-)
 create mode 100644 tools/binman/test/190_files_align.dts

Applied to u-boot-dm, thanks!

[PATCH 08/10] binman: Allow vblock to include devicetree blobs

[Simon Glass]

At present if a devicetree blob is included in a vblock it does not deal
with updates. This is because the vblock is created once at the start and
does not have a method to update itself later, after all the entry
contents are finalised.

Fix this by adjusting how the vblock is created.

Also simplify Image.ProcessEntryContents() since it effectively duplicates
the code in Section.ProcessContents().

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 tools/binman/etype/blob.py               |  4 ++
 tools/binman/etype/vblock.py             | 15 +++++++-
 tools/binman/ftest.py                    | 49 +++++++++++++++++++++++-
 tools/binman/image.py                    |  7 +---
 tools/binman/test/189_vblock_content.dts | 31 +++++++++++++++
 5 files changed, 96 insertions(+), 10 deletions(-)
 create mode 100644 tools/binman/test/189_vblock_content.dts

Applied to u-boot-dm, thanks!

[PATCH 07/10] binman: Support reading an image with entry args

[Simon Glass]

Normally when an entry is created, any entry arguments it has are required
to be provided, so it can actually generate its contents correctly.

However when an existing image is read, Entry objects are created for each
of the entries in the image. This happens as part of the process of
reading the image into binman.

In this case we don't need the entry arguments, since we do not intend to
regenerate the entries, or at least not unless requested. So there is no
sense in reporting an error for missing entry arguments.

Add a new property for the Image to handle this case. Update the error
reporting to be conditional on this property.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 tools/binman/entry.py                    |  3 +--
 tools/binman/etype/section.py            | 15 +++++++++++++++
 tools/binman/ftest.py                    | 19 +++++++++++++++++++
 tools/binman/image.py                    | 10 ++++++++--
 tools/binman/test/188_image_entryarg.dts | 21 +++++++++++++++++++++
 5 files changed, 64 insertions(+), 4 deletions(-)
 create mode 100644 tools/binman/test/188_image_entryarg.dts

Applied to u-boot-dm, thanks!

[PATCH 05/10] binman: Fix a few file comments

[Simon Glass]

Two files have the wrong comment at the top of them. Fix this.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 tools/binman/etype/u_boot_spl_bss_pad.py | 1 -
 tools/binman/etype/u_boot_spl_nodtb.py   | 2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)

Applied to u-boot-dm, thanks!

[PATCH 06/10] binman: Support finding symbols in sub-sections

[Simon Glass]

At present binman only supports resolving symbols in the same section as
the binary that uses it. This is quite limited because we often need to
group entries into different sections.

Enhance the algorithm to search the entire image for symbols.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 tools/binman/elf.py                   |  3 +-
 tools/binman/elf_test.py              |  4 ++-
 tools/binman/etype/section.py         | 41 +++++++++++++++++++++---
 tools/binman/ftest.py                 | 15 +++++++++
 tools/binman/image.py                 | 45 +++++++++++++++++++++++++++
 tools/binman/test/187_symbols_sub.dts | 22 +++++++++++++
 6 files changed, 123 insertions(+), 7 deletions(-)
 create mode 100644 tools/binman/test/187_symbols_sub.dts

Applied to u-boot-dm, thanks!

[PATCH 04/10] binman: Show the size when writing entries

[Simon Glass]

Update the log output to show the size, since this is useful information.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 tools/binman/control.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Applied to u-boot-dm, thanks!

[PATCH 03/10] binman: Update the TODO list

[Simon Glass]

Two of the items have been completed and I thought of another one. Update
the list.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 tools/binman/README | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Applied to u-boot-dm, thanks!

[PATCH 02/10] patman: Correct lz4 compression parameters

[Simon Glass]

At present on large files, lz4 uses a larger block size (e.g. 256KB) than
the 64KB supported by the U-Boot decompression implementation. Also it is
optimised for maximum compression speed, producing larger output than we
would like.

Update the parameters to correct these problems.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 tools/patman/tools.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Applied to u-boot-dm, thanks!

[PATCH 01/10] dtoc: binman: Drop Python 2 code

[Simon Glass]

Drop a few more Python 2 relics that are no-longer needed.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 tools/binman/fmap_util.py | 4 ++--
 tools/dtoc/fdt.py         | 3 +--
 2 files changed, 3 insertions(+), 4 deletions(-)

Applied to u-boot-dm, thanks!