binman: Allow entries to expand after packing
authorSimon Glass <sjg@chromium.org>
Mon, 8 Jul 2019 20:25:37 +0000 (14:25 -0600)
committerSimon Glass <sjg@chromium.org>
Wed, 24 Jul 2019 19:54:08 +0000 (12:54 -0700)
Add support for detecting entries that change size after they have already
been packed, and re-running packing when it happens.

This removes the limitation that entry size cannot change after
PackEntries() is called.

Signed-off-by: Simon Glass <sjg@chromium.org>
12 files changed:
tools/binman/README
tools/binman/bsection.py
tools/binman/control.py
tools/binman/entry.py
tools/binman/etype/_testing.py
tools/binman/etype/section.py
tools/binman/etype/u_boot_with_ucode_ptr.py
tools/binman/ftest.py
tools/binman/image.py
tools/binman/test/121_entry_expand.dts [new file with mode: 0644]
tools/binman/test/122_entry_expand_twice.dts [new file with mode: 0644]
tools/binman/test/123_entry_expand_section.dts [new file with mode: 0644]

index abbf809b8235286a7351d6015c00f39802a6f9d7..9860633792fc67cdcdf40ce3055cb43f2426eb31 100644 (file)
@@ -566,7 +566,8 @@ tree. This sets the correct 'offset' and 'size' vaues, for example.
 The default implementatoin does nothing. This can be overriden to adjust the
 contents of an entry in some way. For example, it would be possible to create
 an entry containing a hash of the contents of some other entries. At this
-stage the offset and size of entries should not be adjusted.
+stage the offset and size of entries should not be adjusted unless absolutely
+necessary, since it requires a repack (going back to PackEntries()).
 
 10. WriteSymbols() - write the value of symbols into the U-Boot SPL binary.
 See 'Access to binman entry offsets at run time' below for a description of
index f49a6e93bc796c059eee6c9a1947c7cd2ad55218..9047e55a34afdcbfe1f8d0fa7a9230ed78f18e33 100644 (file)
@@ -45,6 +45,8 @@ class Section(object):
         _name_prefix: Prefix to add to the name of all entries within this
             section
         _entries: OrderedDict() of entries
+        _orig_offset: Original offset value read from node
+        _orig_size: Original size value read from node
     """
     def __init__(self, name, parent_section, node, image, test=False):
         global entry
@@ -76,6 +78,8 @@ class Section(object):
         """Read properties from the section node"""
         self._offset = fdt_util.GetInt(self._node, 'offset')
         self._size = fdt_util.GetInt(self._node, 'size')
+        self._orig_offset = self._offset
+        self._orig_size = self._size
         self._align_size = fdt_util.GetInt(self._node, 'align-size')
         if tools.NotPowerOfTwo(self._align_size):
             self._Raise("Alignment size %s must be a power of two" %
@@ -257,6 +261,13 @@ class Section(object):
             for name, info in offset_dict.items():
                 self._SetEntryOffsetSize(name, *info)
 
+    def ResetForPack(self):
+        """Reset offset/size fields so that packing can be done again"""
+        self._offset = self._orig_offset
+        self._size = self._orig_size
+        for entry in self._entries.values():
+            entry.ResetForPack()
+
     def PackEntries(self):
         """Pack all entries into the section"""
         offset = self._skip_at_start
@@ -325,6 +336,7 @@ class Section(object):
         for entry in self._entries.values():
             if not entry.ProcessContents():
                 sizes_ok = False
+                print("Entry '%s' size change" % self._node.path)
         return sizes_ok
 
     def WriteSymbols(self):
index 9022cf76e9966bddd462fc84c4b6ee99dbff71dc..35faf1150629e3c0bfb87969b60702937181990f 100644 (file)
@@ -170,21 +170,42 @@ def Binman(args):
                 # completed and written, but that does not seem important.
                 image.GetEntryContents()
                 image.GetEntryOffsets()
-                try:
-                    image.PackEntries()
-                    image.CheckSize()
-                    image.CheckEntries()
-                except Exception as e:
-                    if args.map:
-                        fname = image.WriteMap()
-                        print("Wrote map file '%s' to show errors"  % fname)
-                    raise
-                image.SetImagePos()
-                if args.update_fdt:
-                    image.SetCalculatedProperties()
-                    for dtb_item in state.GetFdts():
-                        dtb_item.Sync()
-                image.ProcessEntryContents()
+
+                # We need to pack the entries to figure out where everything
+                # should be placed. This sets the offset/size of each entry.
+                # However, after packing we call ProcessEntryContents() which
+                # may result in an entry changing size. In that case we need to
+                # do another pass. Since the device tree often contains the
+                # final offset/size information we try to make space for this in
+                # AddMissingProperties() above. However, if the device is
+                # compressed we cannot know this compressed size in advance,
+                # since changing an offset from 0x100 to 0x104 (for example) can
+                # alter the compressed size of the device tree. So we need a
+                # third pass for this.
+                passes = 3
+                for pack_pass in range(passes):
+                    try:
+                        image.PackEntries()
+                        image.CheckSize()
+                        image.CheckEntries()
+                    except Exception as e:
+                        if args.map:
+                            fname = image.WriteMap()
+                            print("Wrote map file '%s' to show errors"  % fname)
+                        raise
+                    image.SetImagePos()
+                    if args.update_fdt:
+                        image.SetCalculatedProperties()
+                        for dtb_item in state.GetFdts():
+                            dtb_item.Sync()
+                    sizes_ok = image.ProcessEntryContents()
+                    if sizes_ok:
+                        break
+                    image.ResetForPack()
+                if not sizes_ok:
+                    image.Raise('Entries expanded after packing (tried %s passes)' %
+                                passes)
+
                 image.WriteSymbols()
                 image.BuildImage()
                 if args.map:
index 7db1402b84fb2e24e94322d29b18bc253bbaad71..e38cb71c5966e6d8fe23163a8211246fdbcae3e6 100644 (file)
@@ -61,6 +61,8 @@ class Entry(object):
         pad_after: Number of pad bytes after the contents, 0 if none
         data: Contents of entry (string of bytes)
         compress: Compression algoithm used (e.g. 'lz4'), 'none' if none
+        orig_offset: Original offset value read from node
+        orig_size: Original size value read from node
     """
     def __init__(self, section, etype, node, read_node=True, name_prefix=''):
         self.section = section
@@ -153,6 +155,9 @@ class Entry(object):
             self.Raise("Please use 'offset' instead of 'pos'")
         self.offset = fdt_util.GetInt(self._node, 'offset')
         self.size = fdt_util.GetInt(self._node, 'size')
+        self.orig_offset = self.offset
+        self.orig_size = self.size
+
         self.align = fdt_util.GetInt(self._node, 'align')
         if tools.NotPowerOfTwo(self.align):
             raise ValueError("Node '%s': Alignment %s must be a power of two" %
@@ -255,9 +260,16 @@ class Entry(object):
             ValueError if the new data size is not the same as the old
         """
         size_ok = True
-        if len(data) != self.contents_size:
+        new_size = len(data)
+        if state.AllowEntryExpansion():
+            if new_size > self.contents_size:
+                print("Entry '%s' size change from %#x to %#x" % (
+                    self._node.path, self.contents_size, new_size))
+                # self.data will indicate the new size needed
+                size_ok = False
+        elif new_size != self.contents_size:
             self.Raise('Cannot update entry size from %d to %d' %
-                       (self.contents_size, len(data)))
+                       (self.contents_size, new_size))
         self.SetContents(data)
         return size_ok
 
@@ -271,6 +283,11 @@ class Entry(object):
         # No contents by default: subclasses can implement this
         return True
 
+    def ResetForPack(self):
+        """Reset offset/size fields so that packing can be done again"""
+        self.offset = self.orig_offset
+        self.size = self.orig_size
+
     def Pack(self, offset):
         """Figure out how to pack the entry into the section
 
index 2204362281c6b40117d9a105bde11d96404a0b59..ae24fe8105a63864414b9ab2a29bc89af87ef11a 100644 (file)
@@ -50,6 +50,8 @@ class Entry__testing(Entry):
                                                     'bad-update-contents')
         self.return_contents_once = fdt_util.GetBool(self._node,
                                                      'return-contents-once')
+        self.bad_update_contents_twice = fdt_util.GetBool(self._node,
+                                                    'bad-update-contents-twice')
 
         # Set to True when the entry is ready to process the FDT.
         self.process_fdt_ready = False
@@ -71,11 +73,12 @@ class Entry__testing(Entry):
         if self.force_bad_datatype:
             self.GetEntryArgsOrProps([EntryArg('test-bad-datatype-arg', bool)])
         self.return_contents = True
+        self.contents = b'a'
 
     def ObtainContents(self):
         if self.return_unknown_contents or not self.return_contents:
             return False
-        self.data = b'a'
+        self.data = self.contents
         self.contents_size = len(self.data)
         if self.return_contents_once:
             self.return_contents = False
@@ -90,7 +93,11 @@ class Entry__testing(Entry):
         if self.bad_update_contents:
             # Request to update the contents with something larger, to cause a
             # failure.
-            return self.ProcessContentsUpdate('aa')
+            if self.bad_update_contents_twice:
+                self.contents += b'a'
+            else:
+                self.contents = b'aa'
+            return self.ProcessContentsUpdate(self.contents)
         return True
 
     def ProcessFdt(self, fdt):
index 51eddcd995ae6a1b6df9b76449b350467a77b940..23bf22113d4d66eaa87858bea270d20a95aaa4ee 100644 (file)
@@ -64,6 +64,11 @@ class Entry_section(Entry):
         self._section.GetEntryOffsets()
         return {}
 
+    def ResetForPack(self):
+        """Reset offset/size fields so that packing can be done again"""
+        self._section.ResetForPack()
+        Entry.ResetForPack(self)
+
     def Pack(self, offset):
         """Pack all entries into the section"""
         self._section.PackEntries()
index 4104bf8bf1325b2b4bf74dc05b87d9abe2f10cdf..cb7dbc68dbb8c45d6cb7117be219ef74faf8badc 100644 (file)
@@ -49,7 +49,7 @@ class Entry_u_boot_with_ucode_ptr(Entry_blob):
     def ProcessContents(self):
         # If the image does not need microcode, there is nothing to do
         if not self.target_offset:
-            return
+            return True
 
         # Get the offset of the microcode
         ucode_entry = self.section.FindEntryType('u-boot-ucode')
index 1c917345f2a343f0ba057cc737ea2023ff1a7d2c..aae8dbc1b33d9c5f1ed0329241129c7292fea8ca 100644 (file)
@@ -1220,10 +1220,14 @@ class TestFunctional(unittest.TestCase):
 
     def testBadChangeSize(self):
         """Test that trying to change the size of an entry fails"""
-        with self.assertRaises(ValueError) as e:
-            self._DoReadFile('059_change_size.dts', True)
-        self.assertIn("Node '/binman/_testing': Cannot update entry size from "
-                      '1 to 2', str(e.exception))
+        try:
+            state.SetAllowEntryExpansion(False)
+            with self.assertRaises(ValueError) as e:
+                self._DoReadFile('059_change_size.dts', True)
+            self.assertIn("Node '/binman/_testing': Cannot update entry size from 1 to 2",
+                          str(e.exception))
+        finally:
+            state.SetAllowEntryExpansion(True)
 
     def testUpdateFdt(self):
         """Test that we can update the device tree with offset/size info"""
@@ -2117,6 +2121,27 @@ class TestFunctional(unittest.TestCase):
         self.assertIn("Invalid location 'None', expected 'start' or 'end'",
                       str(e.exception))
 
+    def testEntryExpand(self):
+        """Test expanding an entry after it is packed"""
+        data = self._DoReadFile('121_entry_expand.dts')
+        self.assertEqual(b'aa', data[:2])
+        self.assertEqual(U_BOOT_DATA, data[2:2 + len(U_BOOT_DATA)])
+        self.assertEqual(b'aa', data[-2:])
+
+    def testEntryExpandBad(self):
+        """Test expanding an entry after it is packed, twice"""
+        with self.assertRaises(ValueError) as e:
+            self._DoReadFile('122_entry_expand_twice.dts')
+        self.assertIn("Image '/binman': Entries expanded after packing",
+                      str(e.exception))
+
+    def testEntryExpandSection(self):
+        """Test expanding an entry within a section after it is packed"""
+        data = self._DoReadFile('123_entry_expand_section.dts')
+        self.assertEqual(b'aa', data[:2])
+        self.assertEqual(U_BOOT_DATA, data[2:2 + len(U_BOOT_DATA)])
+        self.assertEqual(b'aa', data[-2:])
+
 
 if __name__ == "__main__":
     unittest.main()
index c8bce394aa16f1ae29f0d2660c83b239ab216447..6339d020e766dedc48e14826a8e5ee560501fd6c 100644 (file)
@@ -55,6 +55,10 @@ class Image:
             self._filename = filename
         self._section = bsection.Section('main-section', None, self._node, self)
 
+    def Raise(self, msg):
+        """Convenience function to raise an error referencing an image"""
+        raise ValueError("Image '%s': %s" % (self._node.path, msg))
+
     def GetFdtSet(self):
         """Get the set of device tree files used by this image"""
         return self._section.GetFdtSet()
@@ -100,6 +104,10 @@ class Image:
         """
         self._section.GetEntryOffsets()
 
+    def ResetForPack(self):
+        """Reset offset/size fields so that packing can be done again"""
+        self._section.ResetForPack()
+
     def PackEntries(self):
         """Pack all entries into the image"""
         self._section.PackEntries()
diff --git a/tools/binman/test/121_entry_expand.dts b/tools/binman/test/121_entry_expand.dts
new file mode 100644 (file)
index 0000000..ebb7816
--- /dev/null
@@ -0,0 +1,20 @@
+/dts-v1/;
+
+/ {
+       #address-cells = <1>;
+       #size-cells = <1>;
+
+       binman {
+               _testing {
+                       bad-update-contents;
+               };
+
+               u-boot {
+               };
+
+               _testing2 {
+                       type = "_testing";
+                       bad-update-contents;
+               };
+       };
+};
diff --git a/tools/binman/test/122_entry_expand_twice.dts b/tools/binman/test/122_entry_expand_twice.dts
new file mode 100644 (file)
index 0000000..258cf85
--- /dev/null
@@ -0,0 +1,21 @@
+/dts-v1/;
+
+/ {
+       #address-cells = <1>;
+       #size-cells = <1>;
+
+       binman {
+               _testing {
+                       bad-update-contents;
+                       bad-update-contents-twice;
+               };
+
+               u-boot {
+               };
+
+               _testing2 {
+                       type = "_testing";
+                       bad-update-contents;
+               };
+       };
+};
diff --git a/tools/binman/test/123_entry_expand_section.dts b/tools/binman/test/123_entry_expand_section.dts
new file mode 100644 (file)
index 0000000..046f723
--- /dev/null
@@ -0,0 +1,22 @@
+/dts-v1/;
+
+/ {
+       #address-cells = <1>;
+       #size-cells = <1>;
+
+       binman {
+               _testing {
+                       bad-update-contents;
+               };
+
+               u-boot {
+               };
+
+               section {
+                       _testing2 {
+                               type = "_testing";
+                               bad-update-contents;
+                       };
+               };
+       };
+};