Some SPL functions directly use the binman 'u_boot_any' symbols to get U-Boot's binman image position. These symbols are declared by the SPL/TPL_BINMAN_SYMBOLS configs, but they are accessed by macros defined by just CONFIG_BINMAN. So when BINMAN is enabled and BINMAN_SYMBOLS is disabled, the code tries to use undeclared symbols and we get an error.

Therefore, any use of 'u_boot_any' symbols in the code is an implicit dependency on SPL/TPL_BINMAN_SYMBOLS. However, in the current uses they are meant to be the next phase's values, where that happens to be U-Boot. In the meantime, helper funcions spl_get_image_pos/size() were introduced to get these values.

Convert all uses of u_boot_any symbols to these functions, so we only access these symbols at one place. Make sure they will not use these symbols when the BINMAN_SYMBOLS configs are disabled, by returning early in those cases.

Signed-off-by: Alper Nebi Yasak alpernebiyasak@gmail.com ---

(no changes since v1)

common/spl/spl.c | 10 +++++++--- common/spl/spl_ram.c | 2 +- 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/common/spl/spl.c b/common/spl/spl.c index 2a69a7c9324d..5630dcdb5c1e 100644 --- a/common/spl/spl.c +++ b/common/spl/spl.c @@ -149,9 +149,11 @@ void spl_fixup_fdt(void *fdt_blob) #endif }

-#if CONFIG_IS_ENABLED(BINMAN_SYMBOLS) ulong spl_get_image_pos(void) { + if (!CONFIG_IS_ENABLED(BINMAN_SYMBOLS)) + return BINMAN_SYM_MISSING; + #ifdef CONFIG_VPL if (spl_next_phase() == PHASE_VPL) return binman_sym(ulong, u_boot_vpl, image_pos); @@ -163,6 +165,9 @@ ulong spl_get_image_pos(void)

ulong spl_get_image_size(void) { + if (!CONFIG_IS_ENABLED(BINMAN_SYMBOLS)) + return BINMAN_SYM_MISSING; + #ifdef CONFIG_VPL if (spl_next_phase() == PHASE_VPL) return binman_sym(ulong, u_boot_vpl, size); @@ -171,7 +176,6 @@ ulong spl_get_image_size(void) binman_sym(ulong, u_boot_spl, size) : binman_sym(ulong, u_boot_any, size); } -#endif /* BINMAN_SYMBOLS */

ulong spl_get_image_text_base(void) { @@ -222,7 +226,7 @@ __weak struct image_header *spl_get_load_buffer(ssize_t offset, size_t size)

void spl_set_header_raw_uboot(struct spl_image_info *spl_image) { - ulong u_boot_pos = binman_sym(ulong, u_boot_any, image_pos); + ulong u_boot_pos = spl_get_image_pos();

spl_image->size = CONFIG_SYS_MONITOR_LEN;

diff --git a/common/spl/spl_ram.c b/common/spl/spl_ram.c index 829645925718..d64710878cf2 100644 --- a/common/spl/spl_ram.c +++ b/common/spl/spl_ram.c @@ -70,7 +70,7 @@ static int spl_ram_load_image(struct spl_image_info *spl_image, load.read = spl_ram_load_read; spl_load_simple_fit(spl_image, &load, 0, header); } else { - ulong u_boot_pos = binman_sym(ulong, u_boot_any, image_pos); + ulong u_boot_pos = spl_get_image_pos();

debug("Legacy image\n"); /*

TPL_BINMAN_SYMBOLS depends on SPL_FRAMEWORK. The code this enables is compiled by checking CONFIG_$(SPL_TPL_)FRAMEWORK, so it should depend on TPL_FRAMEWORK instead (which in turn depends on SPL_FRAMEWORK). This was most likely a typo due to copy-pasting the config's SPL version, fix it.

Signed-off-by: Alper Nebi Yasak alpernebiyasak@gmail.com ---

(no changes since v1)

common/spl/Kconfig.tpl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/spl/Kconfig.tpl b/common/spl/Kconfig.tpl index 9a0e719cf949..834cb6b6dd82 100644 --- a/common/spl/Kconfig.tpl +++ b/common/spl/Kconfig.tpl @@ -10,7 +10,7 @@ config TPL_SIZE_LIMIT

config TPL_BINMAN_SYMBOLS bool "Declare binman symbols in TPL" - depends on SPL_FRAMEWORK && BINMAN + depends on TPL_FRAMEWORK && BINMAN default y help This enables use of symbols in TPL which refer to U-Boot, enabling TPL

The binman extern symbol declarations in spl.h are missing the VPL symbols recently added to spl.c, add them like the others.

Signed-off-by: Alper Nebi Yasak alpernebiyasak@gmail.com ---

(no changes since v1)

include/spl.h | 2 ++ 1 file changed, 2 insertions(+)

diff --git a/include/spl.h b/include/spl.h index 83ac583e0b49..1778e0f53686 100644 --- a/include/spl.h +++ b/include/spl.h @@ -288,6 +288,8 @@ binman_sym_extern(ulong, u_boot_any, image_pos); binman_sym_extern(ulong, u_boot_any, size); binman_sym_extern(ulong, u_boot_spl, image_pos); binman_sym_extern(ulong, u_boot_spl, size); +binman_sym_extern(ulong, u_boot_vpl, image_pos); +binman_sym_extern(ulong, u_boot_vpl, size);

/** * spl_get_image_pos() - get the image position of the next phase

Enabling CONFIG_BINMAN makes binman run after a build to package any images specified in the device-tree. It also enables a mechanism for SPL/TPL to declare and use special linker symbols that refer to other entries in the same binman image. A similar feature that gets this info from the device-tree exists for U-Boot proper, but it is gated behind a CONFIG_BINMAN_FDT unlike the symbols.

Confusingly, CONFIG_SPL/TPL_BINMAN_SYMBOLS also exist. These configs don't actually enable/disable the symbols mechanism as one would expect, but declare some symbols for U-Boot using this mechanism.

Reuse the BINMAN_SYMBOLS configs to make them toggle the symbols mechanism, and declare symbols for the U-Boot phases in a dependent BINMAN_UBOOT_SYMBOLS config. Extend it to cover symbols of all phases. Update the config prompt and help message to make it clearer about this. Fix binman test binaries to work with CONFIG_IS_ENABLED(BINMAN_SYMBOLS).

Co-developed-by: Peng Fan peng.fan@nxp.com [Alper: New config for phase symbols, update Kconfigs, commit message] Signed-off-by: Alper Nebi Yasak alpernebiyasak@gmail.com --- See Peng's patch [1] included in these changes.

[1] binman_sym: guard with CONFIG_IS_ENABLED(BINMAN_SYMBOLS) https://lore.kernel.org/u-boot/20220603071715.15212-8-peng.fan@oss.nxp.com/

Changes in v2: - Split binman symbols support from enabling binman - Move U-Boot phase symbol declarations to BINMAN_UBOOT_SYMBOLS configs - Merge in Peng's patch for binman_sym.h changes

common/spl/Kconfig | 22 ++++++++++++++----- common/spl/Kconfig.tpl | 24 +++++++++++++++------ common/spl/spl.c | 9 ++++---- include/binman_sym.h | 6 +++--- tools/binman/test/Makefile | 2 +- tools/binman/test/generated/autoconf.h | 3 +++ tools/binman/test/u_boot_binman_syms.c | 2 +- tools/binman/test/u_boot_binman_syms_size.c | 2 +- 8 files changed, 49 insertions(+), 21 deletions(-) create mode 100644 tools/binman/test/generated/autoconf.h

diff --git a/common/spl/Kconfig b/common/spl/Kconfig index 2ad2351c6eb3..46d9be73bb1f 100644 --- a/common/spl/Kconfig +++ b/common/spl/Kconfig @@ -190,12 +190,24 @@ config SPL_BINMAN_SYMBOLS depends on SPL_FRAMEWORK && BINMAN default y help - This enables use of symbols in SPL which refer to U-Boot, enabling SPL - to obtain the location of U-Boot simply by calling spl_get_image_pos() - and spl_get_image_size(). + This enables use of symbols in SPL which refer to other entries in + the same binman image as the SPL. These can be declared with the + binman_sym_declare(type, entry, prop) macro and accessed by the + binman_sym(type, entry, prop) macro defined in binman_sym.h.

- For this to work, you must have a U-Boot image in the binman image, so - binman can update SPL with the location of it. + See tools/binman/binman.rst for a detailed explanation. + +config SPL_BINMAN_UBOOT_SYMBOLS + bool "Declare binman symbols for U-Boot phases in SPL" + depends on SPL_BINMAN_SYMBOLS + default y + help + This enables use of symbols in SPL which refer to U-Boot phases, + enabling SPL to obtain the location and size of its next phase simply + by calling spl_get_image_pos() and spl_get_image_size(). + + For this to work, you must have all U-Boot phases in the same binman + image, so binman can update SPL with the locations of everything.

source "common/spl/Kconfig.nxp"

diff --git a/common/spl/Kconfig.tpl b/common/spl/Kconfig.tpl index 834cb6b6dd82..8c59c767302f 100644 --- a/common/spl/Kconfig.tpl +++ b/common/spl/Kconfig.tpl @@ -9,16 +9,28 @@ config TPL_SIZE_LIMIT If this value is zero, it is ignored.

config TPL_BINMAN_SYMBOLS - bool "Declare binman symbols in TPL" + bool "Support binman symbols in TPL" depends on TPL_FRAMEWORK && BINMAN default y help - This enables use of symbols in TPL which refer to U-Boot, enabling TPL - to obtain the location of U-Boot simply by calling spl_get_image_pos() - and spl_get_image_size(). + This enables use of symbols in TPL which refer to other entries in + the same binman image as the TPL. These can be declared with the + binman_sym_declare(type, entry, prop) macro and accessed by the + binman_sym(type, entry, prop) macro defined in binman_sym.h.

- For this to work, you must have a U-Boot image in the binman image, so - binman can update TPL with the location of it. + See tools/binman/binman.rst for a detailed explanation. + +config TPL_BINMAN_UBOOT_SYMBOLS + bool "Declare binman symbols for U-Boot phases in TPL" + depends on TPL_BINMAN_SYMBOLS + default y + help + This enables use of symbols in TPL which refer to U-Boot phases, + enabling TPL to obtain the location and size of its next phase simply + by calling spl_get_image_pos() and spl_get_image_size(). + + For this to work, you must have all U-Boot phases in the same binman + image, so binman can update TPL with the locations of everything.

config TPL_FRAMEWORK bool "Support TPL based upon the common SPL framework" diff --git a/common/spl/spl.c b/common/spl/spl.c index 5630dcdb5c1e..89b875d0e017 100644 --- a/common/spl/spl.c +++ b/common/spl/spl.c @@ -51,11 +51,10 @@ DECLARE_GLOBAL_DATA_PTR;

u32 *boot_params_ptr = NULL;

-#if CONFIG_IS_ENABLED(BINMAN_SYMBOLS) +#if CONFIG_IS_ENABLED(BINMAN_UBOOT_SYMBOLS) /* See spl.h for information about this */ binman_sym_declare(ulong, u_boot_any, image_pos); binman_sym_declare(ulong, u_boot_any, size); -#endif

#ifdef CONFIG_TPL binman_sym_declare(ulong, u_boot_spl, image_pos); @@ -67,6 +66,8 @@ binman_sym_declare(ulong, u_boot_vpl, image_pos); binman_sym_declare(ulong, u_boot_vpl, size); #endif

+#endif /* BINMAN_UBOOT_SYMBOLS */ + /* Define board data structure */ static struct bd_info bdata __attribute__ ((section(".data")));

@@ -151,7 +152,7 @@ void spl_fixup_fdt(void *fdt_blob)

ulong spl_get_image_pos(void) { - if (!CONFIG_IS_ENABLED(BINMAN_SYMBOLS)) + if (!CONFIG_IS_ENABLED(BINMAN_UBOOT_SYMBOLS)) return BINMAN_SYM_MISSING;

#ifdef CONFIG_VPL @@ -165,7 +166,7 @@ ulong spl_get_image_pos(void)

ulong spl_get_image_size(void) { - if (!CONFIG_IS_ENABLED(BINMAN_SYMBOLS)) + if (!CONFIG_IS_ENABLED(BINMAN_UBOOT_SYMBOLS)) return BINMAN_SYM_MISSING;

#ifdef CONFIG_VPL diff --git a/include/binman_sym.h b/include/binman_sym.h index 72e6765fe520..8586ef8731b6 100644 --- a/include/binman_sym.h +++ b/include/binman_sym.h @@ -13,7 +13,7 @@

#define BINMAN_SYM_MISSING (-1UL)

-#ifdef CONFIG_BINMAN +#if CONFIG_IS_ENABLED(BINMAN_SYMBOLS)

/** * binman_symname() - Internal function to get a binman symbol name @@ -77,7 +77,7 @@ #define binman_sym(_type, _entry_name, _prop_name) \ (*(_type *)&binman_symname(_entry_name, _prop_name))

-#else /* !BINMAN */ +#else /* !CONFIG_IS_ENABLED(BINMAN_SYMBOLS) */

#define binman_sym_declare(_type, _entry_name, _prop_name)

@@ -87,6 +87,6 @@

#define binman_sym(_type, _entry_name, _prop_name) BINMAN_SYM_MISSING

-#endif /* BINMAN */ +#endif /* CONFIG_IS_ENABLED(BINMAN_SYMBOLS) */

#endif diff --git a/tools/binman/test/Makefile b/tools/binman/test/Makefile index 57057e2d588f..bea8567c9b45 100644 --- a/tools/binman/test/Makefile +++ b/tools/binman/test/Makefile @@ -21,7 +21,7 @@ CC = $(CROSS_COMPILE)gcc OBJCOPY = $(CROSS_COMPILE)objcopy

VPATH := $(SRC) -CFLAGS := -march=i386 -m32 -nostdlib -I $(SRC)../../../include \ +CFLAGS := -march=i386 -m32 -nostdlib -I $(SRC)../../../include -I $(SRC) \ -Wl,--no-dynamic-linker

LDS_UCODE := -T $(SRC)u_boot_ucode_ptr.lds diff --git a/tools/binman/test/generated/autoconf.h b/tools/binman/test/generated/autoconf.h new file mode 100644 index 000000000000..6a23039f4699 --- /dev/null +++ b/tools/binman/test/generated/autoconf.h @@ -0,0 +1,3 @@ +#define CONFIG_BINMAN 1 +#define CONFIG_SPL_BUILD 1 +#define CONFIG_SPL_BINMAN_SYMBOLS 1 diff --git a/tools/binman/test/u_boot_binman_syms.c b/tools/binman/test/u_boot_binman_syms.c index 37fc339ce84b..89fee5567e1d 100644 --- a/tools/binman/test/u_boot_binman_syms.c +++ b/tools/binman/test/u_boot_binman_syms.c @@ -5,7 +5,7 @@ * Simple program to create some binman symbols. This is used by binman tests. */

-#define CONFIG_BINMAN +#include <linux/kconfig.h> #include <binman_sym.h>

binman_sym_declare(unsigned long, u_boot_spl_any, offset); diff --git a/tools/binman/test/u_boot_binman_syms_size.c b/tools/binman/test/u_boot_binman_syms_size.c index 7224bc1863c7..c4a053f96f1a 100644 --- a/tools/binman/test/u_boot_binman_syms_size.c +++ b/tools/binman/test/u_boot_binman_syms_size.c @@ -5,7 +5,7 @@ * Simple program to create some binman symbols. This is used by binman tests. */

-#define CONFIG_BINMAN +#include <linux/kconfig.h> #include <binman_sym.h>

binman_sym_declare(char, u_boot_spl, pos);

The SPL code declares binman symbols for U-Boot phases depending on CONFIG_IS_ENABLED(BINMAN_UBOOT_SYMBOLS). This config exists for SPL and TPL, also add a version for VPL.

Signed-off-by: Alper Nebi Yasak alpernebiyasak@gmail.com ---

Changes in v2: - Update VPL configs for the new BINMAN_UBOOT_SYMBOLS

common/spl/Kconfig.vpl | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)

diff --git a/common/spl/Kconfig.vpl b/common/spl/Kconfig.vpl index ba1ea6075b94..daec0bb7bfff 100644 --- a/common/spl/Kconfig.vpl +++ b/common/spl/Kconfig.vpl @@ -198,4 +198,28 @@ config VPL_TEXT_BASE help The address in memory that VPL will be running from.

+config VPL_BINMAN_SYMBOLS + bool "Declare binman symbols in VPL" + depends on VPL_FRAMEWORK && BINMAN + default y + help + This enables use of symbols in VPL which refer to other entries in + the same binman image as the VPL. These can be declared with the + binman_sym_declare(type, entry, prop) macro and accessed by the + binman_sym(type, entry, prop) macro defined in binman_sym.h. + + See tools/binman/binman.rst for a detailed explanation. + +config VPL_BINMAN_UBOOT_SYMBOLS + bool "Declare binman symbols for U-Boot phases in VPL" + depends on VPL_BINMAN_SYMBOLS + default y + help + This enables use of symbols in VPL which refer to U-Boot phases, + enabling VPL to obtain the location and size of its next phase simply + by calling spl_get_image_pos() and spl_get_image_size(). + + For this to work, you must have all U-Boot phases in the same binman + image, so binman can update VPL with the locations of everything. + endmenu

Binman lets us declare symbols in SPL/TPL that refer to other entries in the same binman image as them. These symbols are filled in with the correct values while binman assembles the images, but this is done in-memory only. Symbols marked as optional can be filled with BINMAN_SYM_MISSING as an error value if their referred entry is missing.

However, the unmodified SPL/TPL binaries are still available on disk, and can be used by people. For these files, nothing ensures that the symbols are set to this error value, and they will be considered valid when they are not.

Empirically, all symbols show up as zero in a sandbox_vpl build when we run e.g. tpl/u-boot-tpl directly. On the other hand, zero is a perfectly fine value for a binman-written symbol, so we cannot say the symbols have wrong values based on that.

Declare a magic symbol that binman always fills in with a fixed value. Check this value as an indicator that symbols were filled in correctly. Return the error value for all symbols when this magic symbol has the wrong value.

For binman tests, we need to make room for the new symbol in the mocked SPL/TPL data by extending them by four bytes. This messes up some test image layouts. Fix the affected values, and check the magic symbol wherever it makes sense.

Signed-off-by: Alper Nebi Yasak alpernebiyasak@gmail.com ---

(no changes since v1)

common/spl/spl.c | 1 + include/binman_sym.h | 45 ++++++++++++++++++++- tools/binman/elf.py | 12 ++++-- tools/binman/elf_test.py | 12 +++--- tools/binman/ftest.py | 33 +++++++-------- tools/binman/test/021_image_pad.dts | 2 +- tools/binman/test/024_sorted.dts | 2 +- tools/binman/test/028_pack_4gb_outside.dts | 2 +- tools/binman/test/029_x86_rom.dts | 6 +-- tools/binman/test/053_symbols.dts | 2 +- tools/binman/test/149_symbols_tpl.dts | 4 +- tools/binman/test/155_symbols_tpl_x86.dts | 4 +- tools/binman/test/187_symbols_sub.dts | 2 +- tools/binman/test/u_boot_binman_syms.c | 4 ++ tools/binman/test/u_boot_binman_syms_size.c | 4 ++ 15 files changed, 97 insertions(+), 38 deletions(-)

diff --git a/common/spl/spl.c b/common/spl/spl.c index 89b875d0e017..6f76205f8d7a 100644 --- a/common/spl/spl.c +++ b/common/spl/spl.c @@ -40,6 +40,7 @@ #include <wdt.h>

DECLARE_GLOBAL_DATA_PTR; +DECLARE_BINMAN_MAGIC_SYM;

#ifndef CONFIG_SYS_UBOOT_START #define CONFIG_SYS_UBOOT_START CONFIG_SYS_TEXT_BASE diff --git a/include/binman_sym.h b/include/binman_sym.h index 8586ef8731b6..528d7e4e90ed 100644 --- a/include/binman_sym.h +++ b/include/binman_sym.h @@ -11,6 +11,8 @@ #ifndef __BINMAN_SYM_H #define __BINMAN_SYM_H

+/* BSYM in little endian, keep in sync with tools/binman/elf.py */ +#define BINMAN_SYM_MAGIC_VALUE (0x4d595342UL) #define BINMAN_SYM_MISSING (-1UL)

#if CONFIG_IS_ENABLED(BINMAN_SYMBOLS) @@ -62,6 +64,37 @@ __attribute__((aligned(4), weak, unused, \ section(".binman_sym")))

+/** + * _binman_sym_magic - Internal magic symbol for validity checks + * + * When building images, binman fills in this symbol with the magic + * value #defined above. This is used to check at runtime if the + * symbol values were filled in and are OK to use. + */ +extern ulong _binman_sym_magic; + +/** + * DECLARE_BINMAN_MAGIC_SYM - Declare the internal magic symbol + * + * This macro declares the _binman_sym_magic symbol so that it exists. + * Declaring it here would cause errors during linking due to multiple + * definitions of the symbol. + */ +#define DECLARE_BINMAN_MAGIC_SYM \ + ulong _binman_sym_magic \ + __attribute__((aligned(4), section(".binman_sym"))) + +/** + * BINMAN_SYMS_OK - Check if the symbol values are valid + * + * This macro checks if the magic symbol's value is filled properly, + * which indicates that other symbols are OK to use as well. + * + * Return: 1 if binman symbol values are usable, 0 if not + */ +#define BINMAN_SYMS_OK \ + (*(ulong *)&_binman_sym_magic == BINMAN_SYM_MAGIC_VALUE) + /** * binman_sym() - Access a previously declared symbol * @@ -72,10 +105,14 @@ * @_type: Type f the symbol (e.g. unsigned long) * @entry_name: Name of the entry to look for (e.g. 'u_boot_spl') * @_prop_name: Property value to get from that entry (e.g. 'pos') - * @returns value of that property (filled in by binman) + * + * Return: value of that property (filled in by binman), or + * BINMAN_SYM_MISSING if the value is unavailable */ #define binman_sym(_type, _entry_name, _prop_name) \ - (*(_type *)&binman_symname(_entry_name, _prop_name)) + (BINMAN_SYMS_OK ? \ + (*(_type *)&binman_symname(_entry_name, _prop_name)) : \ + BINMAN_SYM_MISSING)

#else /* !CONFIG_IS_ENABLED(BINMAN_SYMBOLS) */

@@ -85,6 +122,10 @@

#define binman_sym_extern(_type, _entry_name, _prop_name)

+#define DECLARE_BINMAN_MAGIC_SYM + +#define BINMAN_SYMS_OK (0) + #define binman_sym(_type, _entry_name, _prop_name) BINMAN_SYM_MISSING

#endif /* CONFIG_IS_ENABLED(BINMAN_SYMBOLS) */ diff --git a/tools/binman/elf.py b/tools/binman/elf.py index afa05e58fdd8..6d440ddf21df 100644 --- a/tools/binman/elf.py +++ b/tools/binman/elf.py @@ -25,6 +25,9 @@ except: # pragma: no cover ELF_TOOLS = False

+# BSYM in little endian, keep in sync with include/binman_sym.h +BINMAN_SYM_MAGIC_VALUE = 0x4d595342 + # Information about an EFL symbol: # section (str): Name of the section containing this symbol # address (int): Address of the symbol (its value) @@ -223,9 +226,12 @@ def LookupAndWriteSymbols(elf_fname, entry, section): raise ValueError('%s has size %d: only 4 and 8 are supported' % (msg, sym.size))

- # Look up the symbol in our entry tables. - value = section.GetImage().LookupImageSymbol(name, sym.weak, msg, - base.address) + if name == '_binman_sym_magic': + value = BINMAN_SYM_MAGIC_VALUE + else: + # Look up the symbol in our entry tables. + value = section.GetImage().LookupImageSymbol(name, sym.weak, + msg, base.address) if value is None: value = -1 pack_string = pack_string.lower() diff --git a/tools/binman/elf_test.py b/tools/binman/elf_test.py index 02bc10837492..5a51c64cfee3 100644 --- a/tools/binman/elf_test.py +++ b/tools/binman/elf_test.py @@ -127,7 +127,7 @@ def testOutsideFile(self): elf_fname = self.ElfTestFile('u_boot_binman_syms') with self.assertRaises(ValueError) as e: elf.LookupAndWriteSymbols(elf_fname, entry, section) - self.assertIn('entry_path has offset 4 (size 8) but the contents size ' + self.assertIn('entry_path has offset 8 (size 8) but the contents size ' 'is a', str(e.exception))

def testMissingImageStart(self): @@ -161,18 +161,20 @@ def testNoValue(self): This should produce -1 values for all thress symbols, taking up the first 16 bytes of the image. """ - entry = FakeEntry(24) + entry = FakeEntry(28) section = FakeSection(sym_value=None) elf_fname = self.ElfTestFile('u_boot_binman_syms') elf.LookupAndWriteSymbols(elf_fname, entry, section) - self.assertEqual(tools.get_bytes(255, 20) + tools.get_bytes(ord('a'), 4), - entry.data) + expected = (struct.pack('<L', elf.BINMAN_SYM_MAGIC_VALUE) + + tools.get_bytes(255, 20) + + tools.get_bytes(ord('a'), 4)) + self.assertEqual(expected, entry.data)

def testDebug(self): """Check that enabling debug in the elf module produced debug output""" try: tout.init(tout.DEBUG) - entry = FakeEntry(20) + entry = FakeEntry(24) section = FakeSection() elf_fname = self.ElfTestFile('u_boot_binman_syms') with test_util.capture_sys_output() as (stdout, stderr): diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index b5cf549703ad..fa1f421c0521 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -43,8 +43,8 @@ # Contents of test files, corresponding to different entry types U_BOOT_DATA = b'1234' U_BOOT_IMG_DATA = b'img' -U_BOOT_SPL_DATA = b'56780123456789abcdefghi' -U_BOOT_TPL_DATA = b'tpl9876543210fedcbazyw' +U_BOOT_SPL_DATA = b'56780123456789abcdefghijklm' +U_BOOT_TPL_DATA = b'tpl9876543210fedcbazywvuts' BLOB_DATA = b'89' ME_DATA = b'0abcd' VGA_DATA = b'vga' @@ -1406,8 +1406,9 @@ def checkSymbols(self, dts, base_data, u_boot_offset, entry_args=None, elf_fname = self.ElfTestFile('u_boot_binman_syms') syms = elf.GetSymbols(elf_fname, ['binman', 'image']) addr = elf.GetSymbolAddress(elf_fname, '__image_copy_start') + self.assertEqual(syms['_binman_sym_magic'].address, addr) self.assertEqual(syms['_binman_u_boot_spl_any_prop_offset'].address, - addr) + addr + 4)

self._SetupSplElf('u_boot_binman_syms') data = self._DoReadFileDtb(dts, entry_args=entry_args, @@ -1415,17 +1416,17 @@ def checkSymbols(self, dts, base_data, u_boot_offset, entry_args=None, # The image should contain the symbols from u_boot_binman_syms.c # Note that image_pos is adjusted by the base address of the image, # which is 0x10 in our test image - sym_values = struct.pack('<LQLL', 0x00, - u_boot_offset + len(U_BOOT_DATA), + sym_values = struct.pack('<LLQLL', elf.BINMAN_SYM_MAGIC_VALUE, + 0x00, u_boot_offset + len(U_BOOT_DATA), 0x10 + u_boot_offset, 0x04) - expected = (sym_values + base_data[20:] + + expected = (sym_values + base_data[24:] + tools.get_bytes(0xff, 1) + U_BOOT_DATA + sym_values + - base_data[20:]) + base_data[24:]) self.assertEqual(expected, data)

def testSymbols(self): """Test binman can assign symbols embedded in U-Boot""" - self.checkSymbols('053_symbols.dts', U_BOOT_SPL_DATA, 0x18) + self.checkSymbols('053_symbols.dts', U_BOOT_SPL_DATA, 0x1c)

def testSymbolsNoDtb(self): """Test binman can assign symbols embedded in U-Boot SPL""" @@ -3610,20 +3611,20 @@ def testPackIntelFitMissing(self):

def _CheckSymbolsTplSection(self, dts, expected_vals): data = self._DoReadFile(dts) - sym_values = struct.pack('<LQLL', *expected_vals) + sym_values = struct.pack('<LLQLL', elf.BINMAN_SYM_MAGIC_VALUE, *expected_vals) upto1 = 4 + len(U_BOOT_SPL_DATA) - expected1 = tools.get_bytes(0xff, 4) + sym_values + U_BOOT_SPL_DATA[20:] + expected1 = tools.get_bytes(0xff, 4) + sym_values + U_BOOT_SPL_DATA[24:] self.assertEqual(expected1, data[:upto1])

upto2 = upto1 + 1 + len(U_BOOT_SPL_DATA) - expected2 = tools.get_bytes(0xff, 1) + sym_values + U_BOOT_SPL_DATA[20:] + expected2 = tools.get_bytes(0xff, 1) + sym_values + U_BOOT_SPL_DATA[24:] self.assertEqual(expected2, data[upto1:upto2])

- upto3 = 0x34 + len(U_BOOT_DATA) + upto3 = 0x3c + len(U_BOOT_DATA) expected3 = tools.get_bytes(0xff, 1) + U_BOOT_DATA self.assertEqual(expected3, data[upto2:upto3])

- expected4 = sym_values + U_BOOT_TPL_DATA[20:] + expected4 = sym_values + U_BOOT_TPL_DATA[24:] self.assertEqual(expected4, data[upto3:upto3 + len(U_BOOT_TPL_DATA)])

def testSymbolsTplSection(self): @@ -3631,14 +3632,14 @@ def testSymbolsTplSection(self): self._SetupSplElf('u_boot_binman_syms') self._SetupTplElf('u_boot_binman_syms') self._CheckSymbolsTplSection('149_symbols_tpl.dts', - [0x04, 0x1c, 0x10 + 0x34, 0x04]) + [0x04, 0x20, 0x10 + 0x3c, 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, + [0xffffff04, 0xffffff20, 0xffffff3c, 0x04])

def testPackX86RomIfwiSectiom(self): @@ -4488,7 +4489,7 @@ def testCompressExtra(self):

def testSymbolsSubsection(self): """Test binman can assign symbols from a subsection""" - self.checkSymbols('187_symbols_sub.dts', U_BOOT_SPL_DATA, 0x18) + self.checkSymbols('187_symbols_sub.dts', U_BOOT_SPL_DATA, 0x1c)

def testReadImageEntryArg(self): """Test reading an image that would need an entry arg to generate""" diff --git a/tools/binman/test/021_image_pad.dts b/tools/binman/test/021_image_pad.dts index 1ff8dab296fc..c5abbbcdd6ab 100644 --- a/tools/binman/test/021_image_pad.dts +++ b/tools/binman/test/021_image_pad.dts @@ -10,7 +10,7 @@ u-boot-spl { };

u-boot { - offset = <24>; + offset = <28>; }; }; }; diff --git a/tools/binman/test/024_sorted.dts b/tools/binman/test/024_sorted.dts index b79d9adf6827..b54f9b14191d 100644 --- a/tools/binman/test/024_sorted.dts +++ b/tools/binman/test/024_sorted.dts @@ -7,7 +7,7 @@ / { binman { sort-by-offset; u-boot { - offset = <26>; + offset = <30>; };

u-boot-spl { diff --git a/tools/binman/test/028_pack_4gb_outside.dts b/tools/binman/test/028_pack_4gb_outside.dts index 11a1f6059e26..b6ad7fb56a53 100644 --- a/tools/binman/test/028_pack_4gb_outside.dts +++ b/tools/binman/test/028_pack_4gb_outside.dts @@ -13,7 +13,7 @@ u-boot { };

u-boot-spl { - offset = <0xffffffe7>; + offset = <0xffffffe3>; }; }; }; diff --git a/tools/binman/test/029_x86_rom.dts b/tools/binman/test/029_x86_rom.dts index 88aa007bbae1..ad8f9d6e1bdd 100644 --- a/tools/binman/test/029_x86_rom.dts +++ b/tools/binman/test/029_x86_rom.dts @@ -7,13 +7,13 @@ / { binman { sort-by-offset; end-at-4gb; - size = <32>; + size = <36>; u-boot { - offset = <0xffffffe0>; + offset = <0xffffffdc>; };

u-boot-spl { - offset = <0xffffffe7>; + offset = <0xffffffe3>; }; }; }; diff --git a/tools/binman/test/053_symbols.dts b/tools/binman/test/053_symbols.dts index 296580927648..b28f34a72faa 100644 --- a/tools/binman/test/053_symbols.dts +++ b/tools/binman/test/053_symbols.dts @@ -10,7 +10,7 @@ u-boot-spl { };

u-boot { - offset = <0x18>; + offset = <0x1c>; };

u-boot-spl2 { diff --git a/tools/binman/test/149_symbols_tpl.dts b/tools/binman/test/149_symbols_tpl.dts index 0a4ab3f1fabd..4e649c45978d 100644 --- a/tools/binman/test/149_symbols_tpl.dts +++ b/tools/binman/test/149_symbols_tpl.dts @@ -11,12 +11,12 @@ u-boot-spl { };

u-boot-spl2 { - offset = <0x1c>; + offset = <0x20>; type = "u-boot-spl"; };

u-boot { - offset = <0x34>; + offset = <0x3c>; };

section { diff --git a/tools/binman/test/155_symbols_tpl_x86.dts b/tools/binman/test/155_symbols_tpl_x86.dts index 9d7dc51b3d9b..e1ce33e67fbc 100644 --- a/tools/binman/test/155_symbols_tpl_x86.dts +++ b/tools/binman/test/155_symbols_tpl_x86.dts @@ -14,12 +14,12 @@ u-boot-spl { };

u-boot-spl2 { - offset = <0xffffff1c>; + offset = <0xffffff20>; type = "u-boot-spl"; };

u-boot { - offset = <0xffffff34>; + offset = <0xffffff3c>; };

section { diff --git a/tools/binman/test/187_symbols_sub.dts b/tools/binman/test/187_symbols_sub.dts index 54511a737112..3ab62d372152 100644 --- a/tools/binman/test/187_symbols_sub.dts +++ b/tools/binman/test/187_symbols_sub.dts @@ -11,7 +11,7 @@ u-boot-spl { };

u-boot { - offset = <24>; + offset = <28>; }; };

diff --git a/tools/binman/test/u_boot_binman_syms.c b/tools/binman/test/u_boot_binman_syms.c index 89fee5567e1d..ed761246aec7 100644 --- a/tools/binman/test/u_boot_binman_syms.c +++ b/tools/binman/test/u_boot_binman_syms.c @@ -5,9 +5,13 @@ * Simple program to create some binman symbols. This is used by binman tests. */

+typedef unsigned long ulong; + #include <linux/kconfig.h> #include <binman_sym.h>

+DECLARE_BINMAN_MAGIC_SYM; + binman_sym_declare(unsigned long, u_boot_spl_any, offset); binman_sym_declare(unsigned long long, u_boot_spl2, offset); binman_sym_declare(unsigned long, u_boot_any, image_pos); diff --git a/tools/binman/test/u_boot_binman_syms_size.c b/tools/binman/test/u_boot_binman_syms_size.c index c4a053f96f1a..fa41b3d9a332 100644 --- a/tools/binman/test/u_boot_binman_syms_size.c +++ b/tools/binman/test/u_boot_binman_syms_size.c @@ -5,7 +5,11 @@ * Simple program to create some binman symbols. This is used by binman tests. */

+typedef unsigned long ulong; + #include <linux/kconfig.h> #include <binman_sym.h>

+DECLARE_BINMAN_MAGIC_SYM; + binman_sym_declare(char, u_boot_spl, pos);

The i.MX8M boards use partially specified binman images which have an SPL entry without a U-Boot entry. This would normally cause an error due to the 'u_boot_any' binman symbols declared by BINMAN_UBOOT_SYMBOLS requiring a U-Boot-like entry in the same image as the SPL.

However, a problem in the ARMv8 __image_copy_start symbol definition effectively disables binman from attempting to write any symbols at all, so everything appears to work fine until runtime. A future patch fixes the issue in the linker scripts, which lets binman fill in the symbols, which would result in the build error described above.

Explicitly disable the 'u_boot_any' symbols for i.MX8M boards. They are already effectively unusable, and they are incompatible with the boards' current binman image descriptions.

Signed-off-by: Alper Nebi Yasak alpernebiyasak@gmail.com ---

Changes in v2: - Add new patch to disable u_boot_any symbols for i.MX8M boards

common/spl/Kconfig | 1 + common/spl/Kconfig.tpl | 1 + common/spl/Kconfig.vpl | 1 + 3 files changed, 3 insertions(+)

diff --git a/common/spl/Kconfig b/common/spl/Kconfig index 46d9be73bb1f..152569ee4350 100644 --- a/common/spl/Kconfig +++ b/common/spl/Kconfig @@ -200,6 +200,7 @@ config SPL_BINMAN_SYMBOLS config SPL_BINMAN_UBOOT_SYMBOLS bool "Declare binman symbols for U-Boot phases in SPL" depends on SPL_BINMAN_SYMBOLS + default n if ARCH_IMX8M default y help This enables use of symbols in SPL which refer to U-Boot phases, diff --git a/common/spl/Kconfig.tpl b/common/spl/Kconfig.tpl index 8c59c767302f..e314b793a2e9 100644 --- a/common/spl/Kconfig.tpl +++ b/common/spl/Kconfig.tpl @@ -23,6 +23,7 @@ config TPL_BINMAN_SYMBOLS config TPL_BINMAN_UBOOT_SYMBOLS bool "Declare binman symbols for U-Boot phases in TPL" depends on TPL_BINMAN_SYMBOLS + default n if ARCH_IMX8M default y help This enables use of symbols in TPL which refer to U-Boot phases, diff --git a/common/spl/Kconfig.vpl b/common/spl/Kconfig.vpl index daec0bb7bfff..ba4b2e4f99e9 100644 --- a/common/spl/Kconfig.vpl +++ b/common/spl/Kconfig.vpl @@ -213,6 +213,7 @@ config VPL_BINMAN_SYMBOLS config VPL_BINMAN_UBOOT_SYMBOLS bool "Declare binman symbols for U-Boot phases in VPL" depends on VPL_BINMAN_SYMBOLS + default n if ARCH_IMX8M default y help This enables use of symbols in VPL which refer to U-Boot phases,

From: Peng Fan peng.fan@nxp.com

In arch/arm/lib/sections.c there is below code: char __image_copy_start[0] __section(".__image_copy_start"); But actually 'objdump -t spl/u-boot-spl' not able to find out symbol '__image_copy_start' for binman update image-pos/size.

So update link file

Signed-off-by: Peng Fan peng.fan@nxp.com Reviewed-by: Tom Rini trini@konsulko.com Reviewed-by: Alper Nebi Yasak alpernebiyasak@gmail.com --- This is from Peng's i.MX8M binman symbols series [1], picked it onto this series because it made more sense as a binman symbols fix.

[1] armv8: u-boot-spl.lds: mark __image_copy_start as symbol https://lore.kernel.org/u-boot/20220603071715.15212-5-peng.fan@oss.nxp.com/

Changes in v2: - Pick Peng's __image_copy_start fix

arch/arm/cpu/armv8/u-boot-spl.lds | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/cpu/armv8/u-boot-spl.lds b/arch/arm/cpu/armv8/u-boot-spl.lds index 730eb93dbc3b..9b1e7d462870 100644 --- a/arch/arm/cpu/armv8/u-boot-spl.lds +++ b/arch/arm/cpu/armv8/u-boot-spl.lds @@ -23,7 +23,7 @@ SECTIONS { .text : { . = ALIGN(8); - *(.__image_copy_start) + __image_copy_start = .; CPUDIR/start.o (.text*) *(.text*) } >.sram