binman: Support updating section contents

Implement this feature since it is useful for updating FITs within an
image.

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

10 files changed, 287 insertions, 15 deletions

diff --git a/tools/binman/binman.rst b/tools/binman/binman.rst
index 86a4b95e57..e65fbff783 100644
--- a/tools/binman/binman.rst
+++ b/tools/binman/binman.rst
@@ -1347,6 +1347,22 @@ You can also replace just a selection of entries::
 
     $ binman replace -i image.bin "*u-boot*" -I indir
 
+It is possible to replace whole sections as well, but in that case any
+information about entries within the section may become outdated. This is
+because Binman cannot know whether things have moved around or resized within
+the section, once you have updated its data.
+
+Technical note: With 'allow-repack', Binman writes information about the
+original offset and size properties of each entry, if any were specified, in
+the 'orig-offset' and 'orig-size' properties. This allows Binman to distinguish
+between an entry which ended up being packed at an offset (or assigned a size)
+and an entry which had a particular offset / size requested in the Binman
+configuration. Where are particular offset / size was requested, this is treated
+as set in stone, so Binman will ensure it doesn't change. Without this feature,
+repacking an entry might cause it to disobey the original constraints provided
+when it was created.
+
+ Repacking an image involves
 
 .. _`BinmanLogging`:
 
diff --git a/tools/binman/control.py b/tools/binman/control.py
index bd40f074c8..2f2b4893b7 100644
--- a/tools/binman/control.py
+++ b/tools/binman/control.py
@@ -403,6 +403,8 @@ def ReplaceEntries(image_fname, input_fname, indir, entry_paths,
     image_fname = os.path.abspath(image_fname)
     image = Image.FromFile(image_fname)
 
+    image.mark_build_done()
+
     # Replace an entry from a single file, as a special case
     if input_fname:
         if not entry_paths:
diff --git a/tools/binman/entry.py b/tools/binman/entry.py
index f732d40c37..b10a43333e 100644
--- a/tools/binman/entry.py
+++ b/tools/binman/entry.py
@@ -104,6 +104,10 @@ class Entry(object):
             firmware. This means that it will not be changed by the update.
             This is just a signal: enforcement of this is up to the updater.
             This flag does not automatically propagate down to child entries.
+        build_done (bool): Indicates that the entry data has been built and does
+            not need to be done again. This is only used with 'binman replace',
+            to stop sections from being rebuilt if their entries have not been
+            replaced
     """
     fake_dir = None
 
@@ -153,6 +157,7 @@ class Entry(object):
         self.elf_base_sym = None
         self.offset_from_elf = None
         self.preserve = False
+        self.build_done = False
 
     @staticmethod
     def FindEntryClass(etype, expanded):
@@ -1013,6 +1018,7 @@ features to produce new behaviours.
         else:
             self.contents_size = self.pre_reset_size
         ok = self.ProcessContentsUpdate(data)
+        self.build_done = False
         self.Detail('WriteData: size=%x, ok=%s' % (len(data), ok))
         section_ok = self.section.WriteChildData(self)
         return ok and section_ok
@@ -1034,6 +1040,14 @@ features to produce new behaviours.
             True if the section could be updated successfully, False if the
                 data is such that the section could not update
         """
+        self.build_done = False
+        entry = self.section
+
+        # Now we must rebuild all sections above this one
+        while entry and entry != entry.section:
+            self.build_done = False
+            entry = entry.section
+
         return True
 
     def GetSiblingOrder(self):
@@ -1356,3 +1370,11 @@ features to produce new behaviours.
         val = elf.GetSymbolOffset(entry.elf_fname, sym_name,
                                   entry.elf_base_sym)
         return val + offset
+
+    def mark_build_done(self):
+        """Mark an entry as already built"""
+        self.build_done = True
+        entries = self.GetEntries()
+        if entries:
+            for entry in entries.values():
+                entry.mark_build_done()
diff --git a/tools/binman/etype/atf_fip.py b/tools/binman/etype/atf_fip.py
index d5b862040b..73a3f85b9f 100644
--- a/tools/binman/etype/atf_fip.py
+++ b/tools/binman/etype/atf_fip.py
@@ -270,4 +270,4 @@ class Entry_atf_fip(Entry_section):
         # Recreate the data structure, leaving the data for this child alone,
         # so that child.data is used to pack into the FIP.
         self.ObtainContents(skip_entry=child)
-        return True
+        return super().WriteChildData(child)
diff --git a/tools/binman/etype/cbfs.py b/tools/binman/etype/cbfs.py
index 832f8d038f..575aa624f6 100644
--- a/tools/binman/etype/cbfs.py
+++ b/tools/binman/etype/cbfs.py
@@ -295,7 +295,7 @@ class Entry_cbfs(Entry):
         # Recreate the data structure, leaving the data for this child alone,
         # so that child.data is used to pack into the FIP.
         self.ObtainContents(skip_entry=child)
-        return True
+        return super().WriteChildData(child)
 
     def AddBintools(self, btools):
         super().AddBintools(btools)
diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
index 39aa792b10..03fe88e7a6 100644
--- a/tools/binman/etype/fit.py
+++ b/tools/binman/etype/fit.py
@@ -777,6 +777,8 @@ class Entry_fit(Entry_section):
         Args:
             image_pos (int): Position of this entry in the image
         """
+        if self.build_done:
+            return
         super().SetImagePos(image_pos)
 
         # If mkimage is missing we'll have empty data,
@@ -830,3 +832,6 @@ class Entry_fit(Entry_section):
         # missing
         for entry in self._priv_entries.values():
             entry.CheckMissing(missing_list)
+
+    def CheckEntries(self):
+        pass
diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py
index eb73367285..c36edd1350 100644
--- a/tools/binman/etype/section.py
+++ b/tools/binman/etype/section.py
@@ -397,10 +397,13 @@ class Entry_section(Entry):
             This excludes any padding. If the section is compressed, the
             compressed data is returned
         """
-        data = self.BuildSectionData(required)
-        if data is None:
-            return None
-        self.SetContents(data)
+        if not self.build_done:
+            data = self.BuildSectionData(required)
+            if data is None:
+                return None
+            self.SetContents(data)
+        else:
+            data = self.data
         if self._filename:
             tools.write_file(tools.get_output_filename(self._filename), data)
         return data
@@ -427,8 +430,11 @@ class Entry_section(Entry):
             self._SortEntries()
         self._extend_entries()
 
-        data = self.BuildSectionData(True)
-        self.SetContents(data)
+        if self.build_done:
+            self.size = None
+        else:
+            data = self.BuildSectionData(True)
+            self.SetContents(data)
 
         self.CheckSize()
 
@@ -810,6 +816,9 @@ class Entry_section(Entry):
     def LoadData(self, decomp=True):
         for entry in self._entries.values():
             entry.LoadData(decomp)
+        data = self.ReadData(decomp)
+        self.contents_size = len(data)
+        self.ProcessContentsUpdate(data)
         self.Detail('Loaded data')
 
     def GetImage(self):
@@ -866,10 +875,15 @@ class Entry_section(Entry):
         return data
 
     def WriteData(self, data, decomp=True):
-        self.Raise("Replacing sections is not implemented yet")
+        ok = super().WriteData(data, decomp)
+
+        # The section contents are now fixed and cannot be rebuilt from the
+        # containing entries.
+        self.mark_build_done()
+        return ok
 
     def WriteChildData(self, child):
-        return True
+        return super().WriteChildData(child)
 
     def SetAllowMissing(self, allow_missing):
         """Set whether a section allows missing external blobs
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index 934c8dd26f..7644596920 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -5819,13 +5819,61 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
         self.assertEqual(expected_fdtmap, fdtmap)
 
     def testReplaceSectionSimple(self):
-        """Test replacing a simple section with arbitrary data"""
+        """Test replacing a simple section with same-sized data"""
         new_data = b'w' * len(COMPRESS_DATA + U_BOOT_DATA)
-        with self.assertRaises(ValueError) as exc:
-            self._RunReplaceCmd('section', new_data,
-                                dts='241_replace_section_simple.dts')
+        data, expected_fdtmap, image = self._RunReplaceCmd('section',
+            new_data, dts='241_replace_section_simple.dts')
+        self.assertEqual(new_data, data)
+
+        entries = image.GetEntries()
+        self.assertIn('section', entries)
+        entry = entries['section']
+        self.assertEqual(len(new_data), entry.size)
+
+    def testReplaceSectionLarger(self):
+        """Test replacing a simple section with larger data"""
+        new_data = b'w' * (len(COMPRESS_DATA + U_BOOT_DATA) + 1)
+        data, expected_fdtmap, image = self._RunReplaceCmd('section',
+            new_data, dts='241_replace_section_simple.dts')
+        self.assertEqual(new_data, data)
+
+        entries = image.GetEntries()
+        self.assertIn('section', entries)
+        entry = entries['section']
+        self.assertEqual(len(new_data), entry.size)
+        fentry = entries['fdtmap']
+        self.assertEqual(entry.offset + entry.size, fentry.offset)
+
+    def testReplaceSectionSmaller(self):
+        """Test replacing a simple section with smaller data"""
+        new_data = b'w' * (len(COMPRESS_DATA + U_BOOT_DATA) - 1) + b'\0'
+        data, expected_fdtmap, image = self._RunReplaceCmd('section',
+            new_data, dts='241_replace_section_simple.dts')
+        self.assertEqual(new_data, data)
+
+        # The new size is the same as the old, just with a pad byte at the end
+        entries = image.GetEntries()
+        self.assertIn('section', entries)
+        entry = entries['section']
+        self.assertEqual(len(new_data), entry.size)
+
+    def testReplaceSectionSmallerAllow(self):
+        """Test failing to replace a simple section with smaller data"""
+        new_data = b'w' * (len(COMPRESS_DATA + U_BOOT_DATA) - 1)
+        try:
+            state.SetAllowEntryContraction(True)
+            with self.assertRaises(ValueError) as exc:
+                self._RunReplaceCmd('section', new_data,
+                                    dts='241_replace_section_simple.dts')
+        finally:
+            state.SetAllowEntryContraction(False)
+
+        # Since we have no information about the position of things within the
+        # section, we cannot adjust the position of /section-u-boot so it ends
+        # up outside the section
         self.assertIn(
-            "Node '/section': Replacing sections is not implemented yet",
+            "Node '/section/u-boot': Offset 0x24 (36) size 0x4 (4) is outside "
+            "the section '/section' starting at 0x0 (0) of size 0x27 (39)",
             str(exc.exception))
 
     def testMkimageImagename(self):
@@ -6412,6 +6460,85 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
                 'tool', '-l'))
         self.assertEqual(['mary', 'anna', 'fred'], tools.tool_search_paths)
 
+    def testReplaceSectionEntry(self):
+        """Test replacing an entry in a section"""
+        expect_data = b'w' * len(U_BOOT_DATA + COMPRESS_DATA)
+        entry_data, expected_fdtmap, image = self._RunReplaceCmd('section/blob',
+            expect_data, dts='241_replace_section_simple.dts')
+        self.assertEqual(expect_data, entry_data)
+
+        entries = image.GetEntries()
+        self.assertIn('section', entries)
+        section = entries['section']
+
+        sect_entries = section.GetEntries()
+        self.assertIn('blob', sect_entries)
+        entry = sect_entries['blob']
+        self.assertEqual(len(expect_data), entry.size)
+
+        fname = tools.get_output_filename('image-updated.bin')
+        data = tools.read_file(fname)
+
+        new_blob_data = data[entry.image_pos:entry.image_pos + len(expect_data)]
+        self.assertEqual(expect_data, new_blob_data)
+
+        self.assertEqual(U_BOOT_DATA,
+                         data[entry.image_pos + len(expect_data):]
+                         [:len(U_BOOT_DATA)])
+
+    def testReplaceSectionDeep(self):
+        """Test replacing an entry in two levels of sections"""
+        expect_data = b'w' * len(U_BOOT_DATA + COMPRESS_DATA)
+        entry_data, expected_fdtmap, image = self._RunReplaceCmd(
+            'section/section/blob', expect_data,
+            dts='278_replace_section_deep.dts')
+        self.assertEqual(expect_data, entry_data)
+
+        entries = image.GetEntries()
+        self.assertIn('section', entries)
+        section = entries['section']
+
+        subentries = section.GetEntries()
+        self.assertIn('section', subentries)
+        section = subentries['section']
+
+        sect_entries = section.GetEntries()
+        self.assertIn('blob', sect_entries)
+        entry = sect_entries['blob']
+        self.assertEqual(len(expect_data), entry.size)
+
+        fname = tools.get_output_filename('image-updated.bin')
+        data = tools.read_file(fname)
+
+        new_blob_data = data[entry.image_pos:entry.image_pos + len(expect_data)]
+        self.assertEqual(expect_data, new_blob_data)
+
+        self.assertEqual(U_BOOT_DATA,
+                         data[entry.image_pos + len(expect_data):]
+                         [:len(U_BOOT_DATA)])
+
+    def testReplaceFitSibling(self):
+        """Test an image with a FIT inside where we replace its sibling"""
+        fname = TestFunctional._MakeInputFile('once', b'available once')
+        self._DoReadFileRealDtb('277_replace_fit_sibling.dts')
+        os.remove(fname)
+
+        try:
+            tmpdir, updated_fname = self._SetupImageInTmpdir()
+
+            fname = os.path.join(tmpdir, 'update-blob')
+            expected = b'w' * (len(COMPRESS_DATA + U_BOOT_DATA) + 1)
+            tools.write_file(fname, expected)
+
+            self._DoBinman('replace', '-i', updated_fname, 'blob', '-f', fname)
+            data = tools.read_file(updated_fname)
+            start = len(U_BOOT_DTB_DATA)
+            self.assertEqual(expected, data[start:start + len(expected)])
+            map_fname = os.path.join(tmpdir, 'image-updated.map')
+            self.assertFalse(os.path.exists(map_fname))
+        finally:
+            shutil.rmtree(tmpdir)
+
 
 if __name__ == "__main__":
     unittest.main()
diff --git a/tools/binman/test/277_replace_fit_sibling.dts b/tools/binman/test/277_replace_fit_sibling.dts
new file mode 100644
index 0000000000..fc941a8081
--- /dev/null
+++ b/tools/binman/test/277_replace_fit_sibling.dts
@@ -0,0 +1,61 @@
+// SPDX-License-Identifier: GPL-2.0+
+/dts-v1/;
+
+/ {
+	binman {
+		allow-repack;
+
+		u-boot {
+		};
+
+		blob {
+			filename = "compress";
+		};
+
+		fit {
+			description = "test-desc";
+			#address-cells = <1>;
+
+			images {
+				kernel {
+					description = "Vanilla Linux kernel";
+					type = "kernel";
+					arch = "ppc";
+					os = "linux";
+					compression = "gzip";
+					load = <00000000>;
+					entry = <00000000>;
+					hash-1 {
+						algo = "crc32";
+					};
+					blob-ext {
+						filename = "once";
+					};
+				};
+				fdt-1 {
+					description = "Flattened Device Tree blob";
+					type = "flat_dt";
+					arch = "ppc";
+					compression = "none";
+					hash-1 {
+						algo = "crc32";
+					};
+					u-boot-spl-dtb {
+					};
+				};
+			};
+
+			configurations {
+				default = "conf-1";
+				conf-1 {
+					description = "Boot Linux kernel with FDT blob";
+					kernel = "kernel";
+					fdt = "fdt-1";
+				};
+			};
+		};
+
+		fdtmap {
+		};
+	};
+};
diff --git a/tools/binman/test/278_replace_section_deep.dts b/tools/binman/test/278_replace_section_deep.dts
new file mode 100644
index 0000000000..fba2d7dcf2
--- /dev/null
+++ b/tools/binman/test/278_replace_section_deep.dts
@@ -0,0 +1,25 @@
+// SPDX-License-Identifier: GPL-2.0+
+/dts-v1/;
+
+/ {
+	binman {
+		allow-repack;
+
+		u-boot-dtb {
+		};
+
+		section {
+			section {
+				blob {
+					filename = "compress";
+				};
+			};
+
+			u-boot {
+			};
+		};
+
+		fdtmap {
+		};
+	};
+};