[U-Boot] [PATCH v1] arm: remove unneeded symbol offsets and _TEXT_BASE
Albert ARIBAUD
albert.u.boot at aribaud.net
Sun Oct 13 19:16:33 CEST 2013
Hi Benoît,
On Sun, 13 Oct 2013 17:00:25 +0200 (CEST), Benoît Thébaudeau
<benoit.thebaudeau at advansee.com> wrote:
> 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.
Yes, there is a call to hang() which might or might not imply saving on
the stack depending on code generation, and sp might be incorrect
depending on the exception raised. I'll reintroduce the sp setting but
use the runtime value of _start as stack top. As you say, not great but
hey.
> [...]
>
> > 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.
Indeed build-time and run-time values might be different. Normally,
U-Boot starts by being loaded at its build-time address and run from
there (or directly run there if from NOR). At that point, its build- and
run-time addresses coincide. then we reach relocation. Then U-Boot
runs from top of RAM, with a run-time address different from its
build-time one, bu with all base-address-dependent locations fixed by
relocation, so again, run-time and (relocated) build-time values are
equal.
IOW, this debug() line would use true build-time values if invoked
before relocation, and actual run-time values after relocation, because
the &symbol constructs would have relocation entries and thus be fixed
during relocation.
This does not preclude corner-case situations where some in-relocation
code requires knowing both the pre- and post-relocation addresses of
a symbol; usually it's a matter of looking at the "in-RAM" U-Boot image
vs the "in-FLASH" U-Boot image, e.g. when relocating, we copy
U-Bootfrom "source" to "destination" and then fix relocation using
the "source" relocation table, because there is no "destination"
relocation table. Normally these corner-case situations only arise near
the relocation code itself.
However, if e.g. some fields of this debug() call should be the
"initial" addresses while the debug() is executed at "final" location,
just point them to me.
> > @@ -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.
I don't think so. The switch from _end to __rel_dyn_end is done
precisely to avoid using a symbol for which an offset is needed (_end,
because linker-defined, can only have absolute relocations) and use
instead a compiler-defined, relocatable, symbol (__rel_dyn_end, compiled
from sections.c, always takes relative relocation) which does not need
an offset. This switch thus belongs in this 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.
>
> [...]
Thanks for your review!
> Best regards,
> Benoît
Amicalement,
--
Albert.
More information about the U-Boot
mailing list