Hi Daniel,
On Friday, 16 June 2017 06:49:55 PDT Daniel Schwierzeck wrote:
Hi Paul,
Am 16.06.2017 um 02:05 schrieb Paul Burton:
U-Boot has up until now built with -fpic for the MIPS architecture, producing position independent code which uses indirection through a global offset table, making relocation fairly straightforward as it simply involves patching up GOT entries.
Using -fpic does however have some downsides. The biggest of these is that generated code is bloated in various ways. For example, function calls are indirected through the GOT & the t9 register:
8f998064 lw t9,-32668(gp) 0320f809 jalr t9
Without -fpic the call is simply:
0f803f01 jal be00fc04 <puts>
This is more compact & faster (due to the lack of the load & the dependency the jump has on its result). It is also easier to read & debug because the disassembly shows what function is being called, rather than just an offset from gp which would then have to be looked up in the ELF to discover the target function.
Another disadvantage of -fpic is that each function begins with a sequence to calculate the value of the gp register, for example:
3c1c0004 lui gp,0x4 279c3384 addiu gp,gp,13188 0399e021 addu gp,gp,t9
Without using -fpic this sequence no longer appears at the start of each function, reducing code size considerably.
This patch switches U-Boot from building with -fpic to building with -fno-pic, in order to gain the benefits described above. The cost of this is an extra step during the build process to extract relocation data from the ELF & write it into a new .rel section in a compact format, plus the added complexity of dealing with multiple types of relocation rather than the single type that applied to the GOT. The benefit is smaller, cleaner, more debuggable code. The relocate_code() function is reimplemented in C to handle the new relocation scheme, which also makes it easier to read & debug.
Taking maltael_defconfig as an example the size of u-boot.bin built using the Codescape MIPS 2016.05-06 toolchain (gcc 4.9.2, binutils 2.24.90) shrinks from 254KiB to 224KiB.
Signed-off-by: Paul Burton paul.burton@imgtec.com Cc: Daniel Schwierzeck daniel.schwierzeck@gmail.com Cc: u-boot@lists.denx.de
nice work, thanks. Nits below
Thanks for reviewing. Hope to get more submitted soon..!
arch/mips/Makefile.postlink | 23 +++ arch/mips/config.mk | 19 +- arch/mips/cpu/start.S | 130 ------------- arch/mips/cpu/u-boot.lds | 41 +--- arch/mips/include/asm/relocs.h | 24 +++ arch/mips/lib/Makefile | 1 + arch/mips/lib/reloc.c | 164 ++++++++++++++++ common/board_f.c | 2 +- tools/.gitignore | 1 + tools/Makefile | 2 + tools/mips-relocs.c | 426 +++++++++++++++++++++++++++++++++++++++++ 11 files changed, 656 insertions(+), 177 deletions(-) create mode 100644 arch/mips/Makefile.postlink create mode 100644 arch/mips/include/asm/relocs.h create mode 100644 arch/mips/lib/reloc.c create mode 100644 tools/mips-relocs.c
diff --git a/arch/mips/Makefile.postlink b/arch/mips/Makefile.postlink new file mode 100644 index 0000000000..d6fbc0d404 --- /dev/null +++ b/arch/mips/Makefile.postlink @@ -0,0 +1,23 @@ +# +# Copyright (c) 2017 Imagination Technologies Ltd. +# +# SPDX-License-Identifier: GPL-2.0+ +#
+PHONY := __archpost +__archpost:
+-include include/config/auto.conf +include scripts/Kbuild.include
+CMD_RELOCS = tools/mips-relocs +quiet_cmd_relocs = RELOCS $@
cmd_relocs = $(CMD_RELOCS) $@ .$@.relocswhat's the purpose of .$@.relocs? The mips-relocs tool only has one arguments and the kernel Makefile doesn't have this
Well spotted. At one point I experimented with having the mips-relocs tool output a separate file then inserting it into the ELF using objcopy, but that didn't work out so well. Will remove for v2.
+u-boot: FORCE
- @true
- $(call if_changed,relocs)
+.PHONY: FORCE
+FORCE: diff --git a/arch/mips/config.mk b/arch/mips/config.mk index 2c72c1553d..56d150171e 100644 --- a/arch/mips/config.mk +++ b/arch/mips/config.mk @@ -56,25 +56,16 @@ PLATFORM_ELFFLAGS += -B mips $(OBJCOPYFLAGS)
# LDFLAGS_vmlinux += -G 0 -static -n -nostdlib # MODFLAGS += -mlong-calls #
-# On the other hand, we want PIC in the U-Boot code to relocate it from ROM -# to RAM. $28 is always used as gp. -# -ifdef CONFIG_SPL_BUILD -PF_ABICALLS := -mno-abicalls -PF_PIC := -fno-pic -PF_PIE := -else -PF_ABICALLS := -mabicalls -PF_PIC := -fpic -PF_PIE := -pie -PF_OBJCOPY := -j .got -j .rel.dyn -j .padding +ifndef CONFIG_SPL_BUILD +PF_OBJCOPY := -j .got -j .rel -j .padding
PF_OBJCOPY += -j .dtb.init.rodata
+LDFLAGS_FINAL += --emit-relocs
endif
I think we could now drop the extra PF_OBJCOPY variable and directly assign the values to OBJCOPYFLAGS
Sure, will do in v2.
-PLATFORM_CPPFLAGS += -G 0 $(PF_ABICALLS) $(PF_PIC) +PLATFORM_CPPFLAGS += -G 0 -mno-abicalls -fno-pic
PLATFORM_CPPFLAGS += -msoft-float PLATFORM_LDFLAGS += -G 0 -static -n -nostdlib PLATFORM_RELFLAGS += -ffunction-sections -fdata-sections
-LDFLAGS_FINAL += --gc-sections $(PF_PIE) +LDFLAGS_FINAL += --gc-sections
OBJCOPYFLAGS += -j .text -j .rodata -j .data -j .u_boot_list OBJCOPYFLAGS += $(PF_OBJCOPY)
diff --git a/arch/mips/lib/reloc.c b/arch/mips/lib/reloc.c new file mode 100644 index 0000000000..b7ae56df5a --- /dev/null +++ b/arch/mips/lib/reloc.c @@ -0,0 +1,164 @@ +/*
- MIPS Relocation
- Copyright (c) 2017 Imagination Technologies Ltd.
- SPDX-License-Identifier: GPL-2.0+
- */
+#include <common.h> +#include <asm/relocs.h> +#include <asm/sections.h>
+/*
- __rel_start: Relocation data generated by the mips-relocs tool
- This data, found in the .rel section, is generated by the mips-relocs
tool & + * contains a record of all locations in the U-Boot binary that need to be + * fixed up during relocation.
- The data is a sequence of unsigned integers, which are of somewhat
arbitrary + * size. This is achieved by encoding integers as a sequence of bytes, each of + * which contains 7 bits of data with the most significant bit indicating + * whether any further bytes need to be read. The least significant bits of the + * integer are found in the first byte
- ie. it somewhat resembles little + * endian.
- Each pair of two integers represents a relocation that must be
applied. The + * first integer represents the type of relocation as a standard ELF relocation + * type (ie. R_MIPS_*). The second integer represents the offset at which to + * apply the relocation, relative to the previous relocation or for the first + * relocation the start of the relocated .text section.
- The end of the relocation data is indicated when type R_MIPS_NONE (0)
is + * read, at which point no further integers should be read. That is, the + * terminating R_MIPS_NONE reloc includes no offset.
- */
+extern uint8_t __rel_start[];
could you move this to asm/sections.h (or asm-generic/sections.h perhaps) to suppress a checkpatch.pl warning and to have linker script exports in one place?
OK, will do.
Thanks, Paul