binman: tegra: Adjust symbol calculation depending on end-at-4gb
authorSimon Glass <sjg@chromium.org>
Thu, 7 Nov 2019 00:22:44 +0000 (17:22 -0700)
committerTom Rini <trini@konsulko.com>
Mon, 11 Nov 2019 19:20:35 +0000 (14:20 -0500)
A recent change adjusted the symbol calculation to work on x86 but broke
it for Tegra. In fact this is because they have different needs.

On x86 devices the code is linked to a ROM address and the end-at-4gb
property is used for the image. In this case there is no need to add the
base address of the image, since the base address is already built into
the offset and image-pos properties.

On other devices we must add the base address since the offsets start at
zero.

In addition the base address is currently added to the 'offset' and 'size'
values. It should in fact only be added to 'image-pos', since 'offset' is
relative to its parent and 'size' is not actually an address. This code
should have been adjusted when support for 'image-pos' and 'size' was
added, but it was not.

To correct these problems:
- move the code that handles adding the base address to section.py, which
  can check the end-at-4gb property and which property
  (offset/size/image-pos) is being read
- add the base address only when needed (only for image-pos and not if the
  image uses end-at-4gb)
- add a note to the documentation
- add a separate test to cover x86 behaviour

Fixes: 15c981cc (binman: Correct symbol calculation with non-zero image base)

Signed-off-by: Simon Glass <sjg@chromium.org>
Tested-by: Stephen Warren <swarren@nvidia.com>
tools/binman/README
tools/binman/elf.py
tools/binman/elf_test.py
tools/binman/etype/section.py
tools/binman/ftest.py
tools/binman/image_test.py
tools/binman/test/155_symbols_tpl_x86.dts [new file with mode: 0644]
tools/binman/test/Makefile
tools/binman/test/u_boot_binman_syms_x86.c [new symlink]
tools/binman/test/u_boot_binman_syms_x86.lds [new file with mode: 0644]

index c96a5642267367b5e1e70ef082b9f70539275f5d..20a80944e29fb9b92f93cc8fe7f010dafb1980f4 100644 (file)
@@ -766,20 +766,38 @@ when SPL is finished.
 Binman allows you to declare symbols in the SPL image which are filled in
 with their correct values during the build. For example:
 
-    binman_sym_declare(ulong, u_boot_any, offset);
+    binman_sym_declare(ulong, u_boot_any, image_pos);
 
-declares a ulong value which will be assigned to the offset of any U-Boot
+declares a ulong value which will be assigned to the image-pos of any U-Boot
 image (u-boot.bin, u-boot.img, u-boot-nodtb.bin) that is present in the image.
 You can access this value with something like:
 
-    ulong u_boot_offset = binman_sym(ulong, u_boot_any, offset);
+    ulong u_boot_offset = binman_sym(ulong, u_boot_any, image_pos);
 
-Thus u_boot_offset will be set to the offset of U-Boot in memory, assuming that
-the whole image has been loaded, or is available in flash. You can then jump to
-that address to start U-Boot.
+Thus u_boot_offset will be set to the image-pos of U-Boot in memory, assuming
+that the whole image has been loaded, or is available in flash. You can then
+jump to that address to start U-Boot.
 
-At present this feature is only supported in SPL. In principle it is possible
-to fill in such symbols in U-Boot proper, as well.
+At present this feature is only supported in SPL and TPL. In principle it is
+possible to fill in such symbols in U-Boot proper, as well, but a future C
+library is planned for this instead, to read from the device tree.
+
+As well as image-pos, it is possible to read the size of an entry and its
+offset (which is the start position of the entry within its parent).
+
+A small technical note: Binman automatically adds the base address of the image
+(i.e. __image_copy_start) to the value of the image-pos symbol, so that when the
+image is loaded to its linked address, the value will be correct and actually
+point into the image.
+
+For example, say SPL is at the start of the image and linked to start at address
+80108000. If U-Boot's image-pos is 0x8000 then binman will write an image-pos
+for U-Boot of 80110000 into the SPL binary, since it assumes the image is loaded
+to 80108000, with SPL at 80108000 and U-Boot at 80110000.
+
+For x86 devices (with the end-at-4gb property) this base address is not added
+since it is assumed that images are XIP and the offsets already include the
+address.
 
 
 Access to binman entry offsets at run time (fdt)
index 0c1a5b44b669a39e46c8cf1f575ac5b89ad0daea..de1ce73f2ae9bdd86856bbe10411a103196f186e 100644 (file)
@@ -134,7 +134,7 @@ def LookupAndWriteSymbols(elf_fname, entry, section):
                                  (msg, sym.size))
 
             # Look up the symbol in our entry tables.
-            value = section.LookupSymbol(name, sym.weak, msg)
+            value = section.LookupSymbol(name, sym.weak, msg, base.address)
             if value is None:
                 value = -1
                 pack_string = pack_string.lower()
index c0c11cb3403195fd0bb7b4a17a411960fc90f2c9..ac26fd51e41975c6ef1ddee488f00fbd022fb762 100644 (file)
@@ -45,7 +45,7 @@ class FakeSection:
     def GetPath(self):
         return 'section_path'
 
-    def LookupSymbol(self, name, weak, msg):
+    def LookupSymbol(self, name, weak, msg, base_addr):
         """Fake implementation which returns the same value for all symbols"""
         return self.sym_value
 
index ab0c42cee045eef1ec02878b7ced769d50e20a45..89b7bf67fa6fab589d55500c1dfcf32e3982403b 100644 (file)
@@ -290,13 +290,16 @@ class Entry_section(Entry):
                 return entry.GetData()
         source_entry.Raise("Cannot find entry for node '%s'" % node.name)
 
-    def LookupSymbol(self, sym_name, optional, msg):
+    def LookupSymbol(self, sym_name, optional, msg, base_addr):
         """Look up a symbol in an ELF file
 
         Looks up a symbol in an ELF file. Only entry types which come from an
         ELF image can be used by this function.
 
-        At present the only entry property supported is offset.
+        At present the only entry properties supported are:
+            offset
+            image_pos - 'base_addr' is added if this is not an end-at-4gb image
+            size
 
         Args:
             sym_name: Symbol name in the ELF file to look up in the format
@@ -309,6 +312,12 @@ class Entry_section(Entry):
             optional: True if the symbol is optional. If False this function
                 will raise if the symbol is not found
             msg: Message to display if an error occurs
+            base_addr: Base address of image. This is added to the returned
+                image_pos in most cases so that the returned position indicates
+                where the targetted entry/binary has actually been loaded. But
+                if end-at-4gb is used, this is not done, since the binary is
+                already assumed to be linked to the ROM position and using
+                execute-in-place (XIP).
 
         Returns:
             Value that should be assigned to that symbol, or None if it was
@@ -343,7 +352,10 @@ class Entry_section(Entry):
         if prop_name == 'offset':
             return entry.offset
         elif prop_name == 'image_pos':
-            return entry.image_pos
+            value = entry.image_pos
+            if not self.GetImage()._end_4gb:
+                value += base_addr
+            return value
         if prop_name == 'size':
             return entry.size
         else:
index 80df0e3ca9cc5a3a8b7db5effbb2825e02b3f4de..872b8554440d6a3799a4d0652b1a1e570109addb 100644 (file)
@@ -1242,7 +1242,7 @@ class TestFunctional(unittest.TestCase):
 
         self._SetupSplElf('u_boot_binman_syms')
         data = self._DoReadFile('053_symbols.dts')
-        sym_values = struct.pack('<LQLL', 0, 28, 24, 4)
+        sym_values = struct.pack('<LQLL', 0x00, 0x1c, 0x28, 0x04)
         expected = (sym_values + U_BOOT_SPL_DATA[20:] +
                     tools.GetBytes(0xff, 1) + U_BOOT_DATA + sym_values +
                     U_BOOT_SPL_DATA[20:])
@@ -3304,12 +3304,9 @@ class TestFunctional(unittest.TestCase):
         self.assertIn("'intel-fit-ptr' section must have an 'intel-fit' sibling",
                       str(e.exception))
 
-    def testSymbolsTplSection(self):
-        """Test binman can assign symbols embedded in U-Boot TPL in a section"""
-        self._SetupSplElf('u_boot_binman_syms')
-        self._SetupTplElf('u_boot_binman_syms')
-        data = self._DoReadFile('149_symbols_tpl.dts')
-        sym_values = struct.pack('<LQLL', 4, 0x1c, 0x34, 4)
+    def _CheckSymbolsTplSection(self, dts, expected_vals):
+        data = self._DoReadFile(dts)
+        sym_values = struct.pack('<LQLL', *expected_vals)
         upto1 = 4 + len(U_BOOT_SPL_DATA)
         expected1 = tools.GetBytes(0xff, 4) + sym_values + U_BOOT_SPL_DATA[20:]
         self.assertEqual(expected1, data[:upto1])
@@ -3323,7 +3320,22 @@ class TestFunctional(unittest.TestCase):
         self.assertEqual(expected3, data[upto2:upto3])
 
         expected4 = sym_values + U_BOOT_TPL_DATA[20:]
-        self.assertEqual(expected4, data[upto3:])
+        self.assertEqual(expected4, data[upto3:upto3 + len(U_BOOT_TPL_DATA)])
+
+    def testSymbolsTplSection(self):
+        """Test binman can assign symbols embedded in U-Boot TPL in a section"""
+        self._SetupSplElf('u_boot_binman_syms')
+        self._SetupTplElf('u_boot_binman_syms')
+        self._CheckSymbolsTplSection('149_symbols_tpl.dts',
+                                     [0x04, 0x1c, 0x10 + 0x34, 0x04])
+
+    def testSymbolsTplSectionX86(self):
+        """Test binman can assign symbols in a section with end-at-4gb"""
+        self._SetupSplElf('u_boot_binman_syms_x86')
+        self._SetupTplElf('u_boot_binman_syms_x86')
+        self._CheckSymbolsTplSection('155_symbols_tpl_x86.dts',
+                                     [0xffffff04, 0xffffff1c, 0xffffff34,
+                                      0x04])
 
     def testPackX86RomIfwiSectiom(self):
         """Test that a section can be placed in an IFWI region"""
index 4004f789b7c3964bac5bc6e229d49b0597bc16ca..10f85d1081fec9c0fe3b1cde987d69b3cccacd2d 100644 (file)
@@ -13,7 +13,7 @@ class TestImage(unittest.TestCase):
     def testInvalidFormat(self):
         image = Image('name', 'node', test=True)
         with self.assertRaises(ValueError) as e:
-            image.LookupSymbol('_binman_something_prop_', False, 'msg')
+            image.LookupSymbol('_binman_something_prop_', False, 'msg', 0)
         self.assertIn(
             "msg: Symbol '_binman_something_prop_' has invalid format",
             str(e.exception))
@@ -22,7 +22,7 @@ class TestImage(unittest.TestCase):
         image = Image('name', 'node', test=True)
         image._entries = {}
         with self.assertRaises(ValueError) as e:
-            image.LookupSymbol('_binman_type_prop_pname', False, 'msg')
+            image.LookupSymbol('_binman_type_prop_pname', False, 'msg', 0)
         self.assertIn("msg: Entry 'type' not found in list ()",
                       str(e.exception))
 
@@ -30,7 +30,7 @@ class TestImage(unittest.TestCase):
         image = Image('name', 'node', test=True)
         image._entries = {}
         with capture_sys_output() as (stdout, stderr):
-            val = image.LookupSymbol('_binman_type_prop_pname', True, 'msg')
+            val = image.LookupSymbol('_binman_type_prop_pname', True, 'msg', 0)
         self.assertEqual(val, None)
         self.assertEqual("Warning: msg: Entry 'type' not found in list ()\n",
                          stderr.getvalue())
@@ -40,5 +40,5 @@ class TestImage(unittest.TestCase):
         image = Image('name', 'node', test=True)
         image._entries = {'u-boot': 1}
         with self.assertRaises(ValueError) as e:
-            image.LookupSymbol('_binman_u_boot_prop_bad', False, 'msg')
+            image.LookupSymbol('_binman_u_boot_prop_bad', False, 'msg', 0)
         self.assertIn("msg: No such property 'bad", str(e.exception))
diff --git a/tools/binman/test/155_symbols_tpl_x86.dts b/tools/binman/test/155_symbols_tpl_x86.dts
new file mode 100644 (file)
index 0000000..72ca447
--- /dev/null
@@ -0,0 +1,31 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/dts-v1/;
+
+/ {
+       #address-cells = <1>;
+       #size-cells = <1>;
+
+       binman {
+               pad-byte = <0xff>;
+               end-at-4gb;
+               size = <0x100>;
+               u-boot-spl {
+                       offset = <0xffffff04>;
+               };
+
+               u-boot-spl2 {
+                       offset = <0xffffff1c>;
+                       type = "u-boot-spl";
+               };
+
+               u-boot {
+                       offset = <0xffffff34>;
+               };
+
+               section {
+                       u-boot-tpl {
+                               type = "u-boot-tpl";
+                       };
+               };
+       };
+};
index bdbb009874d4a6a1016a4735328ce22ce4456442..e4fd97bb2e11b6361efa9ac42f892e7c6d898afc 100644 (file)
@@ -14,10 +14,11 @@ CFLAGS := -march=i386 -m32 -nostdlib -I $(SRC)../../../include \
 LDS_UCODE := -T $(SRC)u_boot_ucode_ptr.lds
 LDS_BINMAN := -T $(SRC)u_boot_binman_syms.lds
 LDS_BINMAN_BAD := -T $(SRC)u_boot_binman_syms_bad.lds
+LDS_BINMAN_X86 := -T $(SRC)u_boot_binman_syms_x86.lds
 
 TARGETS = u_boot_ucode_ptr u_boot_no_ucode_ptr bss_data \
        u_boot_binman_syms u_boot_binman_syms.bin u_boot_binman_syms_bad \
-       u_boot_binman_syms_size
+       u_boot_binman_syms_size u_boot_binman_syms_x86
 
 all: $(TARGETS)
 
@@ -36,6 +37,9 @@ u_boot_binman_syms.bin: u_boot_binman_syms
 u_boot_binman_syms: CFLAGS += $(LDS_BINMAN)
 u_boot_binman_syms: u_boot_binman_syms.c
 
+u_boot_binman_syms_x86: CFLAGS += $(LDS_BINMAN_X86)
+u_boot_binman_syms_x86: u_boot_binman_syms_x86.c
+
 u_boot_binman_syms_bad: CFLAGS += $(LDS_BINMAN_BAD)
 u_boot_binman_syms_bad: u_boot_binman_syms_bad.c
 
diff --git a/tools/binman/test/u_boot_binman_syms_x86.c b/tools/binman/test/u_boot_binman_syms_x86.c
new file mode 120000 (symlink)
index 0000000..939b2e9
--- /dev/null
@@ -0,0 +1 @@
+u_boot_binman_syms.c
\ No newline at end of file
diff --git a/tools/binman/test/u_boot_binman_syms_x86.lds b/tools/binman/test/u_boot_binman_syms_x86.lds
new file mode 100644 (file)
index 0000000..9daf86f
--- /dev/null
@@ -0,0 +1,30 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright (c) 2016 Google, Inc
+ */
+
+OUTPUT_FORMAT("elf32-i386", "elf32-i386", "elf32-i386")
+OUTPUT_ARCH(i386)
+ENTRY(_start)
+
+SECTIONS
+{
+       . = 0xffffff00;
+       _start = .;
+
+       . = ALIGN(4);
+       .text :
+       {
+               __image_copy_start = .;
+               *(.text*)
+       }
+
+       . = ALIGN(4);
+       .binman_sym_table : {
+               __binman_sym_start = .;
+               KEEP(*(SORT(.binman_sym*)));
+               __binman_sym_end = .;
+       }
+       .interp : { *(.interp*) }
+
+}