From: Simon Glass Date: Fri, 3 Mar 2023 00:02:44 +0000 (-0700) Subject: binman: Support updating section contents X-Git-Url: http://git.dujemihanovic.xyz/img/static/gitweb.css?a=commitdiff_plain;h=7caa372a5e41cadd3903165fb26a4b1e0268edbc;p=u-boot.git binman: Support updating section contents Implement this feature since it is useful for updating FITs within an image. Signed-off-by: Simon Glass --- 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 { + }; + }; +};