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.bur...@imgtec.com> > > Cc: Daniel Schwierzeck <daniel.schwierz...@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) $@ .$@.relocs > > what'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
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot