[U-Boot] [PATCH v2 4/4] arm: factorize relocate_code routine

Albert ARIBAUD
2013-05-15T07:31:37Z

Hi Beno?t,

(I had indeed missed remarks in your reply; apologies for this)

On Tue, 14 May 2013 18:01:50 +0200 (CEST), Beno?t Th?baudeau
<benoit.thebaudeau@advansee.com> wrote:

> Hi Albert,

> > +/*
> > + * These are defined in the board-specific linker script.
> > + * Subtracting _start from them lets the linker put their
> > + * relative position in the executable instead of leaving
> > + * them null.
> > + */
>
> This comment is obsolete. It should either be removed or updated.

Correct.

> > +
> > +/*
> > + * void relocate_code(addr_moni)
> > + *
> > + * This function relocates the monitor code.
> > + */
> > +ENTRY(relocate_code)
> > + mov r6, r0 /* save addr of destination */
> > +
> > + ldr r0, =_start
>
> If you are avoiding the "ldr =" construct below, why do you use it
> here? You could "ldr r0, _TEXT_BASE".

_start is defined in a compilation unit, not in the linker file.
References to _start such as the one above are therefore correct before
as well as after relocation.

> > + subs r9, r6, r0 /* r9 <- relocation offset */
> > + beq relocate_done /* skip relocation */
> > + mov r1, r6 /* r1 <- scratch for copy loop */
> > + ldr r3, _image_copy_end_ofs
> > + add r2, r0, r3 /* r2 <- source end address */
>
> Adding _start to a relocate_code-relative _image_copy_end_ofs?!

You're correct. For some reason I did not complete the rewrite here. :(

I'd made the offsets relative to relocate_code in order to avoid
the linker issues with subtracting symbols not defined in the current
compilation unit.

Then I should add =relocate_code to r3, not =_start, and also -- as r9
is not the right offset here -- compute r7 as the delta between the
link-time =_start and the run-time relocate_code (r7 becomes useless
once R10, r2 and r3 are fixed).

I'd run-tested tested each commit but apparently this bug did not
cause any immediately visible issue. This time I'll step-test it.

> > + /*
> > + * fix .rel.dyn relocations
> > + */
> > + ldr r10, _dynsym_start_ofs /* r10 <- sym table ofs */
> > + add r10, r10, r9 /* r10 <- sym table in FLASH */
> > + ldr r2, _rel_dyn_start_ofs /* r2 <- rel dyn start ofs */
> > + add r2, r2, r9 /* r2 <- rel dyn start in FLASH */
> > + ldr r3, _rel_dyn_end_ofs /* r3 <- rel dyn end ofs */
> > + add r3, r3, r9 /* r3 <- rel dyn end in FLASH */
>
> This is mixing relocate_code-relative offsets with the relocation offset!

Correct, that's where r7 kicks in to replace r9.

Again, apologies for missing this.

> Best regards,
> Beno?t

Amicalement,
--
Albert.