[U-Boot] [PATCH v1] arm: remove unneeded symbol offsets and _TEXT_BASE

[Benoît Thébaudeau]

Hi Albert,

On Sunday, October 13, 2013 9:10:28 AM, Albert ARIBAUD wrote:
> Remove the last uses of symbol offsets in ARM U-Boot.
> Remove some needless uses of _TEXT_BASE.
> Remove all _TEXT_BASE definitions.
> 
> Signed-off-by: Albert ARIBAUD <albert.u.boot at aribaud.net>
> ---
> _TEXT_BASE was only used by ARM to allow resolution of
> symbol offsets, themselves only needed due to absolute
> relocations.
> 
> In some places, _TEXT_BASE was locally defined only
> to provide a literal for CONFIG_SYS_TEXT_BASE when the
> latter could have been used directly.
> 
> Sometimes even, _TEXT_BASE was defined but unused.
> 
> Since all relocations in ARM are relative, offsets,
> _TEXT_BASE and CONFIG_SYS_SYM_OFFSETS can be completely
> removed, and their uses can be replaced with adequate
> use of compiler-generated symbols from sections.c file.

[...]

> diff --git a/arch/arm/cpu/arm1136/start.S b/arch/arm/cpu/arm1136/start.S
> index bd1e067..d15124b 100644
> --- a/arch/arm/cpu/arm1136/start.S
> +++ b/arch/arm/cpu/arm1136/start.S

[...]

> @@ -295,7 +269,6 @@ cpu_init_crit:
>  #ifdef CONFIG_SPL_BUILD
>  	.align	5
>  do_hang:
> -	ldr	sp, _TEXT_BASE			/* use 32 words about stack */
>  	bl	hang				/* hang and never return */
>  #else	/* !CONFIG_SPL_BUILD */
>  	.align	5

Is this change (and the same change in the other start.S files) safe?

lib/hang.c/hang() may need a valid stack pointer because the functions that it
calls may use the stack.

When the CPU lands in do_hang, it's because some exception occurred, which may
follow a situation having corrupted sp. If sp is corrupted, the CPU won't be
able to push the exception context onto the stack, but it might still be able to
run the exception vector.

Setting sp to *_TEXT_BASE was not great, but at least this provided a few valid
words of RAM for the stack.

[...]

> diff --git a/arch/arm/lib/board.c b/arch/arm/lib/board.c
> index 34f50b0..f8ac573 100644
> --- a/arch/arm/lib/board.c
> +++ b/arch/arm/lib/board.c
> @@ -105,8 +105,8 @@ static int display_banner(void)
>  {
>  	printf("\n\n%s\n\n", version_string);
>  	debug("U-Boot code: %08lX -> %08lX  BSS: -> %08lX\n",
> -	       _TEXT_BASE,
> -	       _bss_start_ofs + _TEXT_BASE, _bss_end_ofs + _TEXT_BASE);
> +	       (ulong)&_start,
> +	       (ulong)&__bss_start, (ulong)&__bss_end);
>  #ifdef CONFIG_MODEM_SUPPORT
>  	debug("Modem Support enabled\n");
>  #endif

This hunk and all the other hunks using _TEXT_BASE in the same way will
introduce different resulting values than the original code for targets having
different build-time and run-time addresses.

This is not too much of an issue for the debug() call here, but this may be more
damaging for things like gd->reloc_off below.

> @@ -277,13 +277,13 @@ void board_init_f(ulong bootflag)
>  
>  	memset((void *)gd, 0, sizeof(gd_t));
>  
> -	gd->mon_len = _bss_end_ofs;
> +	gd->mon_len = (ulong)&__bss_end - (ulong)_start;
>  #ifdef CONFIG_OF_EMBED
>  	/* Get a pointer to the FDT */
>  	gd->fdt_blob = _binary_dt_dtb_start;
>  #elif defined CONFIG_OF_SEPARATE
>  	/* FDT is at end of image */
> -	gd->fdt_blob = (void *)(_end_ofs + _TEXT_BASE);
> +	gd->fdt_blob = &_end;
>  #endif
>  	/* Allow the early environment to override the fdt address */
>  	gd->fdt_blob = (void *)getenv_ulong("fdtcontroladdr", 16,
> @@ -451,7 +451,7 @@ void board_init_f(ulong bootflag)
>  
>  	gd->relocaddr = addr;
>  	gd->start_addr_sp = addr_sp;
> -	gd->reloc_off = addr - _TEXT_BASE;
> +	gd->reloc_off = addr - (ulong)&_start;
>  	debug("relocation Offset is: %08lx\n", gd->reloc_off);
>  	if (new_fdt) {
>  		memcpy(new_fdt, gd->fdt_blob, fdt_size);
> @@ -516,7 +516,7 @@ void board_init_r(gd_t *id, ulong dest_addr)
>  	gd->flags |= GD_FLG_RELOC;	/* tell others: relocation done */
>  	bootstage_mark_name(BOOTSTAGE_ID_START_UBOOT_R, "board_init_r");
>  
> -	monitor_flash_len = _end_ofs;
> +	monitor_flash_len = (ulong)&__rel_dyn_end - (ulong)_start;
>  
>  	/* Enable caches */
>  	enable_caches();

[...]

> diff --git a/common/board_f.c b/common/board_f.c
> index 0ada1af..decd7f2 100644
> --- a/common/board_f.c
> +++ b/common/board_f.c
> @@ -149,13 +149,9 @@ static int display_text_info(void)
>  #ifndef CONFIG_SANDBOX
>  	ulong bss_start, bss_end;
>  
> -#ifdef CONFIG_SYS_SYM_OFFSETS
> -	bss_start = _bss_start_ofs + _TEXT_BASE;
> -	bss_end = _bss_end_ofs + _TEXT_BASE;
> -#else
>  	bss_start = (ulong)&__bss_start;
>  	bss_end = (ulong)&__bss_end;
> -#endif
> +
>  	debug("U-Boot code: %08X -> %08lX  BSS: -> %08lX\n",
>  	      CONFIG_SYS_TEXT_BASE, bss_start, bss_end);
>  #endif

Same comment as above regarding the use of _TEXT_BASE, except that here we want
to use the runtime addresses, so the new code is more correct than the original
one, and this change is hence fine.

> @@ -275,8 +271,8 @@ static int zero_global_data(void)
>  
>  static int setup_mon_len(void)
>  {
> -#ifdef CONFIG_SYS_SYM_OFFSETS
> -	gd->mon_len = _bss_end_ofs;
> +#ifdef __ARM__
> +	gd->mon_len = (ulong)&__bss_end - (ulong)_start;
>  #elif defined(CONFIG_SANDBOX)
>  	gd->mon_len = (ulong)&_end - (ulong)_init;
>  #else
> @@ -358,11 +354,11 @@ static int setup_fdt(void)
>  	gd->fdt_blob = _binary_dt_dtb_start;
>  #elif defined CONFIG_OF_SEPARATE
>  	/* FDT is at end of image */
> -# ifdef CONFIG_SYS_SYM_OFFSETS
> -	gd->fdt_blob = (void *)(_end_ofs + CONFIG_SYS_TEXT_BASE);
> -# else
> +#ifdef CONFIG_ARM
> +	gd->fdt_blob = (ulong *)__rel_dyn_end;
> +#else
>  	gd->fdt_blob = (ulong *)&_end;
> -# endif
> +#endif
>  #elif defined(CONFIG_OF_HOSTFILE)
>  	if (read_fdt_from_file()) {
>  		puts("Failed to read control FDT\n");

There are 2 unrelated changes for gd->fdt_blob here for ARM:
 - _end_ofs + CONFIG_SYS_TEXT_BASE -> _end,
 - _end -> __rel_dyn_end.

So maybe this should be split using another patch.

> diff --git a/common/board_r.c b/common/board_r.c
> index 86ca1cb..ce84753 100644
> --- a/common/board_r.c
> +++ b/common/board_r.c
> @@ -128,8 +128,8 @@ __weak int fixup_cpu(void)
>  
>  static int initr_reloc_global_data(void)
>  {
> -#ifdef CONFIG_SYS_SYM_OFFSETS
> -	monitor_flash_len = _end_ofs;
> +#ifdef __ARM__
> +	monitor_flash_len = __rel_dyn_end - __image_copy_start;
>  #elif !defined(CONFIG_SANDBOX)
>  	monitor_flash_len = (ulong)&__init_end - gd->relocaddr;
>  #endif

Ditto.

[...]

Best regards,
Benoît