Implement this feature since it is useful for updating FITs within an image.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: - Update to support more replacement cases
tools/binman/binman.rst | 16 ++ tools/binman/control.py | 2 + tools/binman/entry.py | 22 +++ tools/binman/etype/atf_fip.py | 2 +- tools/binman/etype/cbfs.py | 2 +- tools/binman/etype/fit.py | 5 + tools/binman/etype/section.py | 30 +++- tools/binman/ftest.py | 137 +++++++++++++++++- tools/binman/test/277_replace_fit_sibling.dts | 61 ++++++++ .../binman/test/278_replace_section_deep.dts | 25 ++++ 10 files changed, 287 insertions(+), 15 deletions(-) create mode 100644 tools/binman/test/277_replace_fit_sibling.dts create mode 100644 tools/binman/test/278_replace_section_deep.dts
diff --git a/tools/binman/binman.rst b/tools/binman/binman.rst index 55ef317a691..717780a39ef 100644 --- a/tools/binman/binman.rst +++ b/tools/binman/binman.rst @@ -1326,6 +1326,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 f4e3898b34c..803ed5f01d5 100644 --- a/tools/binman/control.py +++ b/tools/binman/control.py @@ -402,6 +402,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 5eacc5fa6c4..07209673f22 100644 --- a/tools/binman/entry.py +++ b/tools/binman/entry.py @@ -100,6 +100,10 @@ class Entry(object): appear in the map optional (bool): True if this entry contains an optional external blob overlap (bool): True if this entry overlaps with others + 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
@@ -148,6 +152,7 @@ class Entry(object): self.overlap = False self.elf_base_sym = None self.offset_from_elf = None + self.build_done = False
@staticmethod def FindEntryClass(etype, expanded): @@ -1006,6 +1011,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 @@ -1027,6 +1033,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): @@ -1349,3 +1363,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 6ecd95b71f9..c44ee4aa53d 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 832f8d038f0..575aa624f6c 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 be0cdcc900f..76687492f8a 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 57b91ff726c..6565348e36e 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 b4b5019f90c..e732e07c781 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): @@ -6394,6 +6442,85 @@ fdt fdtmap Extract the devicetree blob from the fdtmap self.assertEqual(['u-boot', 'atf-2'], fdt_util.GetStringList(node, 'loadables'))
+ 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 00000000000..fc941a80816 --- /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 00000000000..fba2d7dcf28 --- /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 { + }; + }; +};