binman: Honour the skip-at-start property more faithfully

A discussion on the mailing list about dealing with block offsets and
binman symbols made me think that something is wrong with how Binman
deals with the skip-at-start property.

The feature was originally designed to handle x86 ROMs, which are mapped
at the top of the address space. That seemed too specific, whereas
skipping some space at the start seemed more generally useful.

It has proved useful. For example, rockchip images start at block 64,
so a skip-at-start of 0x8000 deals with this.

But it doesn't actually work correctly, since the image_pos value does
not give the actual position on the media.

Fix this and update the documentation, moving it into the 'section'
section.

Signed-off-by: Simon Glass <[email protected]>

Author: sjg20

tools/binman/binman.rst

@@ -823,24 +823,6 @@ multiple-images:
             };
         };
 
-end-at-4gb:
-    For x86 machines the ROM offsets start just before 4GB and extend
-    up so that the image finished at the 4GB boundary. This boolean
-    option can be enabled to support this. The image size must be
-    provided so that binman knows when the image should start. For an
-    8MB ROM, the offset of the first entry would be 0xfff80000 with
-    this option, instead of 0 without this option.
-
-skip-at-start:
-    This property specifies the entry offset of the first entry.
-
-    For PowerPC mpc85xx based CPU, CONFIG_TEXT_BASE is the entry
-    offset of the first entry. It can be 0xeff40000 or 0xfff40000 for
-    nor flash boot, 0x201000 for sd boot etc.
-
-    'end-at-4gb' property is not applicable where CONFIG_TEXT_BASE +
-    Image size != 4gb.
-
 align-default:
     Specifies the default alignment for entries in this section if they do
     not specify an alignment. Note that this only applies to top-level entries
@@ -957,6 +939,35 @@ filename:
     section in different image, since there is currently no way to share data
     between images other than through files.
 
+end-at-4gb:
+    For x86 machines the ROM offsets start just before 4GB and extend
+    up so that the image finished at the 4GB boundary. This boolean
+    option can be enabled to support this. The image size must be
+    provided so that binman knows when the image should start. For an
+    8MB ROM, the offset of the first entry would be 0xfff80000 with
+    this option, instead of 0 without this option.
+
+skip-at-start:
+    This property specifies the entry offset of the first entry in the section.
+    It is useful when the Binman image is written to a particular offset in the
+    media. It allows the offset of the first entry to be the media offset, even
+    though it is at the start of the image. It effectively creates a hole at the
+    start of the image, an implied, empty area.
+
+    For example, if the image is written to offset 4K on the media, set
+    skip-at-start to 0x1000. At runtime, the Binman image will assume that it
+    has be written at offset 4K and all symbols and offsets will take account of
+    that. The image-pos values will also be adjusted. The effect is similar to
+    adding an empty 4K region at the start, except that Binman does not actually
+    output it.
+
+    For PowerPC mpc85xx based CPU, CONFIG_TEXT_BASE is the entry
+    offset of the first entry. It can be 0xeff40000 or 0xfff40000 for
+    nor flash boot, 0x201000 for sd boot etc.
+
+    'end-at-4gb' property is not applicable where CONFIG_TEXT_BASE +
+    Image size != 4gb.
+
 Image Properties
 ----------------
 

tools/binman/entry.py

@@ -392,9 +392,8 @@ def SetCalculatedProperties(self):
         """Set the value of device-tree properties calculated by binman"""
         state.SetInt(self._node, 'offset', self.offset)
         state.SetInt(self._node, 'size', self.size)
-        base = self.section.GetRootSkipAtStart() if self.section else 0
         if self.image_pos is not None:
-            state.SetInt(self._node, 'image-pos', self.image_pos - base)
+            state.SetInt(self._node, 'image-pos', self.image_pos)
         if self.GetImage().allow_repack:
             if self.orig_offset is not None:
                 state.SetInt(self._node, 'orig-offset', self.orig_offset, True)

tools/binman/etype/fmap.py

@@ -65,7 +65,7 @@ def _AddEntries(areas, entry):
                 if entry.image_pos is None:
                     pos = 0
                 else:
-                    pos = entry.image_pos - entry.GetRootSkipAtStart()
+                    pos = entry.image_pos
 
                 # Drop @ symbols in name
                 name = entry.name.replace('@', '')
@@ -75,8 +75,6 @@ def _AddEntries(areas, entry):
                     _AddEntries(areas, subentry)
             else:
                 pos = entry.image_pos
-                if pos is not None:
-                    pos -= entry.section.GetRootSkipAtStart()
                 areas.append(fmap_util.FmapArea(pos or 0, entry.size or 0,
                                                 entry.name, flags))
 

tools/binman/etype/section.py

@@ -801,7 +801,7 @@ def _SetEntryOffsetSize(self, name, offset, size):
         if not entry:
             self._Raise("Unable to set offset/size for unknown entry '%s'" %
                         name)
-        entry.SetOffsetSize(self._skip_at_start + offset if offset is not None
+        entry.SetOffsetSize(offset + self._skip_at_start if offset is not None
                             else None, size)
 
     def GetEntryOffsets(self):

tools/binman/ftest.py

@@ -2297,16 +2297,17 @@ def testFmapX86(self):
         fhdr, fentries = fmap_util.DecodeFmap(data[32:])
 
         self.assertEqual(0x100, fhdr.image_size)
+        base = (1 << 32) - 0x100
 
-        self.assertEqual(0, fentries[0].offset)
+        self.assertEqual(base, fentries[0].offset)
         self.assertEqual(4, fentries[0].size)
         self.assertEqual(b'U_BOOT', fentries[0].name)
 
-        self.assertEqual(4, fentries[1].offset)
+        self.assertEqual(base + 4, fentries[1].offset)
         self.assertEqual(3, fentries[1].size)
         self.assertEqual(b'INTEL_MRC', fentries[1].name)
 
-        self.assertEqual(32, fentries[2].offset)
+        self.assertEqual(base + 32, fentries[2].offset)
         self.assertEqual(fmap_util.FMAP_HEADER_LEN +
                          fmap_util.FMAP_AREA_LEN * 3, fentries[2].size)
         self.assertEqual(b'FMAP', fentries[2].name)
@@ -2319,27 +2320,28 @@ def testFmapX86Section(self):
         fhdr, fentries = fmap_util.DecodeFmap(data[36:])
 
         self.assertEqual(0x180, fhdr.image_size)
+        base = (1 << 32) - 0x180
         expect_size = fmap_util.FMAP_HEADER_LEN + fmap_util.FMAP_AREA_LEN * 4
         fiter = iter(fentries)
 
         fentry = next(fiter)
         self.assertEqual(b'U_BOOT', fentry.name)
-        self.assertEqual(0, fentry.offset)
+        self.assertEqual(base, fentry.offset)
         self.assertEqual(4, fentry.size)
 
         fentry = next(fiter)
         self.assertEqual(b'SECTION', fentry.name)
-        self.assertEqual(4, fentry.offset)
+        self.assertEqual(base + 4, fentry.offset)
         self.assertEqual(0x20 + expect_size, fentry.size)
 
         fentry = next(fiter)
         self.assertEqual(b'INTEL_MRC', fentry.name)
-        self.assertEqual(4, fentry.offset)
+        self.assertEqual(base + 4, fentry.offset)
         self.assertEqual(3, fentry.size)
 
         fentry = next(fiter)
         self.assertEqual(b'FMAP', fentry.name)
-        self.assertEqual(36, fentry.offset)
+        self.assertEqual(base + 36, fentry.offset)
         self.assertEqual(expect_size, fentry.size)
 
     def testElf(self):
@@ -3535,8 +3537,8 @@ def testDescriptorOffset(self):
         image = control.images['image']
         entries = image.GetEntries()
         desc = entries['intel-descriptor']
-        self.assertEqual(0xff800000, desc.offset);
-        self.assertEqual(0xff800000, desc.image_pos);
+        self.assertEqual(0xff800000, desc.offset)
+        self.assertEqual(0xff800000, desc.image_pos)
 
     def testReplaceCbfs(self):
         """Test replacing a single file in CBFS without changing the size"""
@@ -3778,8 +3780,8 @@ def testPackIntelFit(self):
 
         image = control.images['image']
         entries = image.GetEntries()
-        expected_ptr = entries['intel-fit'].image_pos - (1 << 32)
-        self.assertEqual(expected_ptr, ptr)
+        expected_ptr = entries['intel-fit'].image_pos #- (1 << 32)
+        self.assertEqual(expected_ptr, ptr + (1 << 32))
 
     def testPackIntelFitMissing(self):
         """Test detection of a FIT pointer with not FIT region"""
@@ -4773,7 +4775,7 @@ def testReadImageSkip(self):
         entry = image.GetEntries()['fdtmap']
         self.assertEqual(orig_entry.offset, entry.offset)
         self.assertEqual(orig_entry.size, entry.size)
-        self.assertEqual(16, entry.image_pos)
+        self.assertEqual((1 << 32) - 0x400 + 16, entry.image_pos)
 
         u_boot = image.GetEntries()['section'].GetEntries()['u-boot']