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

[Albert ARIBAUD]

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 at 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.