From 180a3a9ed3e0ee80f4ed4d02d671a7b0fb28db6d Mon Sep 17 00:00:00 2001 From: Jimmy Brisson Date: Tue, 30 Jul 2024 11:46:03 -0500 Subject: [PATCH 1/2] fix(arm): remove duplicate jumptable entry Change-Id: I4cc4ef493318372ec0d0531ca3e98196e7065ab9 Signed-off-by: Jimmy Brisson --- plat/arm/board/fvp/jmptbl.i | 1 - 1 file changed, 1 deletion(-) diff --git a/plat/arm/board/fvp/jmptbl.i b/plat/arm/board/fvp/jmptbl.i index dc8032f31..077283e47 100644 --- a/plat/arm/board/fvp/jmptbl.i +++ b/plat/arm/board/fvp/jmptbl.i @@ -36,7 +36,6 @@ fdt fdt_get_alias_namelen fdt fdt_get_name fdt fdt_get_alias fdt fdt_node_offset_by_phandle -fdt fdt_subnode_offset fdt fdt_add_subnode mbedtls mbedtls_asn1_get_alg mbedtls mbedtls_asn1_get_alg_null From d95d56bd2bfc87951f35d2badde9db336c0a6489 Mon Sep 17 00:00:00 2001 From: Jimmy Brisson Date: Mon, 22 Jul 2024 12:55:43 -0500 Subject: [PATCH 2/2] fix(romlib): wrap indirectly included functions The problem that this resolves is a bit involved; the following must be met at the same time for some function : * to_be_wrapped must be specified as part of the romlib * to_be_wrapped must _not_ be referenced by any translation unit in TF-A * to_be_wrapped must be referenced by a translation unit in a dependent library, mbedtls for example. Under these circumstances, to_be_wrapped will not be wrapped, and will instead reference its original definition while simultaneously residing in romlib. This is a side effect of two issues with romlib prior to this patch: 1 to_be_wrapped is expected to wrap by duplicating its definition. This causes any condition that links against both the base and wrapper functions to be a link error (duplicate symbol definition). 2 to_be_wrapped is in its own translation unit This causes the wrappers to be used by TF-A in an as needed. The duplicate function definitions can be worked around using the linker's `--wrap` flag, which redirects all references to a symbol to resolve to `__wrap_` and the original symbol to be available as `__real_`. Most of the changes handle creating this arguments and passing them to the linker. Further, once you use the linker's wrap, you will encounter another issue: if TF-A does not use a function, its wrapper is not present. This causes link issues when a library and not TF-A uses the wrapper. Note that this issue would have been resolved previously by ignoring the wrapper and using the base definition. This further issue is worked around by concatenating the assembly for all of the wrappers into a single translation unit. It's possible to work around this issue in a few other ways, including reordering the libraries passed to the linker to place libwrapper.a last or grouping the libraries so that symbols from later libraries may be resolved with prior libraries. I chose the translation unit concatenation approach as it revealed that a jumptable has duplicate symbols within it. Change-Id: Ie57b5ae69bde2fc8705bdc7a93fae3ddb5341ed9 Signed-off-by: Jimmy Brisson --- docs/components/romlib-design.rst | 11 +++++++- lib/romlib/Makefile | 6 ++++- lib/romlib/romlib_generator.py | 41 ++++++++++++++++++++---------- lib/romlib/templates/wrapper.S | 5 ++-- lib/romlib/templates/wrapper_bti.S | 5 ++-- make_helpers/build_macros.mk | 8 ++++-- 6 files changed, 55 insertions(+), 21 deletions(-) diff --git a/docs/components/romlib-design.rst b/docs/components/romlib-design.rst index 62c173ac1..c0f3ed34b 100644 --- a/docs/components/romlib-design.rst +++ b/docs/components/romlib-design.rst @@ -71,6 +71,15 @@ image(s) is replaced with the wrapper function. The "library at ROM" contains a necessary init function that initialises the global variables defined by the functions inside "library at ROM". +Wrapper functions are specified at the link stage of compilation and cannot +interpose uppon functions within the same translation unit. For example, if +function ``fn_a`` calls ``fn_b`` within translation unit ``functions.c`` and +the romlib jump table includes an entry for ``fn_b``, ``fn_a`` will include +a reference to ``fn_b``'s original program text instead of the wrapper. Thus +the jumptable author must take care to include public entry points into +translation units to avoid paying the program text cost twice, once in the +original executable and once in romlib. + Script ~~~~~~ @@ -86,7 +95,7 @@ files for the "library at ROM" to work. It implements multiple functions: 3. ``romlib_generator.py genwrappers [args]`` - Generates a wrapper function for each entry in the index file except for the ones that contain the keyword - ``patch``. The generated wrapper file is called ``.s``. + ``patch``. The generated wrapper file is called ``wrappers.s``. 4. ``romlib_generator.py pre [args]`` - Preprocesses the index file which means it resolves all the include commands in the file recursively. It can also diff --git a/lib/romlib/Makefile b/lib/romlib/Makefile index 9ade331d0..8c8a11024 100644 --- a/lib/romlib/Makefile +++ b/lib/romlib/Makefile @@ -44,7 +44,7 @@ endif .PHONY: all clean distclean -all: $(BUILD_DIR)/romlib.bin $(LIB_DIR)/libwrappers.a +all: $(BUILD_DIR)/romlib.bin $(BUILD_DIR)/romlib.ldflags $(LIB_DIR)/libwrappers.a %.o: %.s $(s)echo " AS $@" @@ -88,6 +88,10 @@ $(BUILD_DIR)/jmptbl.s: $(BUILD_DIR)/jmptbl.i $(s)echo " TBL $@" $(q)$(ROMLIB_GEN) gentbl --output $@ --bti=$(ENABLE_BTI) $< +$(BUILD_DIR)/romlib.ldflags: ../../$(PLAT_DIR)/jmptbl.i + $(s)echo " LDFLAGS $@" + $(q)$(ROMLIB_GEN) link-flags $< > $@ + clean: $(q)rm -f $(BUILD_DIR)/* diff --git a/lib/romlib/romlib_generator.py b/lib/romlib/romlib_generator.py index 0682dd49e..8d2e88d54 100755 --- a/lib/romlib/romlib_generator.py +++ b/lib/romlib/romlib_generator.py @@ -182,6 +182,22 @@ class TableGenerator(RomlibApplication): template_name = "jmptbl_entry_" + item["type"] + bti + ".S" output_file.write(self.build_template(template_name, item, True)) +class LinkArgs(RomlibApplication): + """ Generates the link arguments to wrap functions. """ + + def __init__(self, prog): + RomlibApplication.__init__(self, prog) + self.args.add_argument("file", help="Input file") + + def main(self): + index_file_parser = IndexFileParser() + index_file_parser.parse(self.config.file) + + fns = [item["function_name"] for item in index_file_parser.items + if not item["patch"] and item["type"] != "reserved"] + + print(" ".join("-Wl,--wrap " + f for f in fns)) + class WrapperGenerator(RomlibApplication): """ Generates a wrapper function for each entry in the index file except for the ones that contain @@ -214,21 +230,19 @@ class WrapperGenerator(RomlibApplication): if item["type"] == "reserved" or item["patch"]: continue - asm = self.config.b + "/" + item["function_name"] + ".s" - if self.config.list: - # Only listing files - files.append(asm) - else: - with open(asm, "w") as asm_file: - # The jump instruction is 4 bytes but BTI requires and extra instruction so - # this makes it 8 bytes per entry. - function_offset = item_index * (8 if self.config.bti else 4) + if not self.config.list: + # The jump instruction is 4 bytes but BTI requires and extra instruction so + # this makes it 8 bytes per entry. + function_offset = item_index * (8 if self.config.bti else 4) - item["function_offset"] = function_offset - asm_file.write(self.build_template("wrapper" + bti + ".S", item)) + item["function_offset"] = function_offset + files.append(self.build_template("wrapper" + bti + ".S", item)) if self.config.list: - print(" ".join(files)) + print(self.config.b + "/wrappers.s") + else: + with open(self.config.b + "/wrappers.s", "w") as asm_file: + asm_file.write("\n".join(files)) class VariableGenerator(RomlibApplication): """ Generates the jump table global variable with the absolute address in ROM. """ @@ -258,7 +272,8 @@ class VariableGenerator(RomlibApplication): if __name__ == "__main__": APPS = {"genvar": VariableGenerator, "pre": IndexPreprocessor, - "gentbl": TableGenerator, "genwrappers": WrapperGenerator} + "gentbl": TableGenerator, "genwrappers": WrapperGenerator, + "link-flags": LinkArgs} if len(sys.argv) < 2 or sys.argv[1] not in APPS: print("usage: romlib_generator.py [%s] [args]" % "|".join(APPS.keys()), file=sys.stderr) diff --git a/lib/romlib/templates/wrapper.S b/lib/romlib/templates/wrapper.S index 734a68a2e..576474a63 100644 --- a/lib/romlib/templates/wrapper.S +++ b/lib/romlib/templates/wrapper.S @@ -3,8 +3,9 @@ * * SPDX-License-Identifier: BSD-3-Clause */ - .globl ${function_name} -${function_name}: + .section .text.__wrap_${function_name} + .globl __wrap_${function_name} +__wrap_${function_name}: ldr x17, =jmptbl mov x16, #${function_offset} ldr x17, [x17] diff --git a/lib/romlib/templates/wrapper_bti.S b/lib/romlib/templates/wrapper_bti.S index ba9b11ce5..0dc316cbe 100644 --- a/lib/romlib/templates/wrapper_bti.S +++ b/lib/romlib/templates/wrapper_bti.S @@ -3,8 +3,9 @@ * * SPDX-License-Identifier: BSD-3-Clause */ - .globl ${function_name} -${function_name}: + .section .text.__wrap_${function_name} + .globl __wrap_${function_name} +__wrap_${function_name}: bti jc ldr x17, =jmptbl mov x16, #${function_offset} diff --git a/make_helpers/build_macros.mk b/make_helpers/build_macros.mk index d27408c50..23dd59245 100644 --- a/make_helpers/build_macros.mk +++ b/make_helpers/build_macros.mk @@ -479,6 +479,10 @@ define linker_script_path $(patsubst %.S,$(BUILD_DIR)/%,$(1)) endef +ifeq ($(USE_ROMLIB),1) +WRAPPER_FLAGS := @${BUILD_PLAT}/romlib/romlib.ldflags +endif + # MAKE_BL macro defines the targets and options to build each BL image. # Arguments: # $(1) = BL stage @@ -548,11 +552,11 @@ ifeq ($($(ARCH)-ld-id),arm-link) --map --list="$(MAPFILE)" --scatter=${PLAT_DIR}/scat/${1}.scat \ $(LDPATHS) $(LIBWRAPPER) $(LDLIBS) $(BL_LIBS) $(OBJS) else ifeq ($($(ARCH)-ld-id),gnu-gcc) - $$(q)$($(ARCH)-ld) -o $$@ $$(TF_LDFLAGS) $$(LDFLAGS) $(BL_LDFLAGS) -Wl,-Map=$(MAPFILE) \ + $$(q)$($(ARCH)-ld) -o $$@ $$(TF_LDFLAGS) $$(LDFLAGS) $$(WRAPPER_FLAGS) $(BL_LDFLAGS) -Wl,-Map=$(MAPFILE) \ $(addprefix -Wl$(comma)--script$(comma),$(LINKER_SCRIPTS)) -Wl,--script,$(DEFAULT_LINKER_SCRIPT) \ $(OBJS) $(LDPATHS) $(LIBWRAPPER) $(LDLIBS) $(BL_LIBS) else - $$(q)$($(ARCH)-ld) -o $$@ $$(TF_LDFLAGS) $$(LDFLAGS) $(BL_LDFLAGS) -Map=$(MAPFILE) \ + $$(q)$($(ARCH)-ld) -o $$@ $$(TF_LDFLAGS) $$(LDFLAGS) $$(WRAPPER_FLAGS) $(BL_LDFLAGS) -Map=$(MAPFILE) \ $(addprefix -T ,$(LINKER_SCRIPTS)) --script $(DEFAULT_LINKER_SCRIPT) \ $(OBJS) $(LDPATHS) $(LIBWRAPPER) $(LDLIBS) $(BL_LIBS) endif