[U-Boot] [PATCH] futile c relocation attempt

[Albert ARIBAUD]

Le 05/10/2010 19:41, Reinhard Meyer a écrit :
> Dear Albert ARIBAUD,
>>> include/configs/top9000_9xe.h | 1 +
> That's my board. Not in mainstream yet. Just sets the define there.
>
>>> +#ifdef CONFIG_USE_C_RELOCATION
>>> + /* TODO: check for identical source and destination */
>>> + /* TODO: check for overlapping */
>>> + /* copy image, including initialized data */
>>> + debug ("memcpy(%08lx,%08lx,%ld)\n",
>>> + addr, _TEXT_BASE, _bss_start_ofs);
>>> + memcpy ((void *)addr, (void *)_TEXT_BASE, _bss_start_ofs);
>>> + /* now fix the code */
>>> + debug ("_dynsym_start_ofs=%08lx _rel_dyn_start_ofs=%08lx
>>> _rel_dyn_end_ofs=%08lx\n",
>>> + _dynsym_start_ofs, _rel_dyn_start_ofs, _rel_dyn_end_ofs);
>>> + for (rel_dyn_ptr = (Elf32_Rel *)(_TEXT_BASE + _rel_dyn_start_ofs);
>>> + rel_dyn_ptr< (Elf32_Rel *)(_TEXT_BASE + _rel_dyn_end_ofs);
>>> + rel_dyn_ptr++) {
>>> + ulong *patchaddr = (ulong *) (rel_dyn_ptr->r_offset + gd->reloc_off);
>>> + debug ("patch %08lx : %08lx\n",
>>> + (ulong)patchaddr, (ulong)rel_dyn_ptr->r_info);
>>> + switch (ELF32_R_TYPE(rel_dyn_ptr->r_info)) {
>>> + case 23: /* rel fixup */
>>> + *patchaddr += gd->reloc_off;
>>> + break;
>>> + case 2: /* abs fixup */
>>> + {
>>> + Elf32_Sym *sym = (Elf32_Sym *)(_TEXT_BASE + _dynsym_start_ofs);
>>> + sym += ELF32_R_SYM(rel_dyn_ptr->r_info);
>>> + *patchaddr = gd->reloc_off + sym->st_value;
>>> + }
>>> + break;
>>> + default: /* unhandled fixup */
>>> + break;
>>> + }
>>> + }
>>> + /* clear BSS */
>>> +# ifndef CONFIG_PRELOADER
>>> + debug ("clearing BSS %08lx..%08lx\n",
>>> + addr + _bss_start_ofs, addr + _bss_end_ofs);
>>> + for (p = (ulong *)(addr + _bss_start_ofs);
>>> + p< (ulong *)(addr + _bss_end_ofs);
>>> + *p++ = 0)
>>> + ;
>>> +# endif
>>> +#endif
>>
>> All of _bss_start_ofs, _rel_dyn_start_ofs, _rel_dyn_end_ofs,
>> _dynsym_start_ofs and _bss_end_ofs are going to be modified by the
>> relocation fixup loop. The compiler *might* preload them into
>> registers... or not; which could cause issues with ..._end_ofs ones,
>> because it could cause the loops to overflow/underflow.
>
> They are not going to be modified, they are in the unrelocated location!

Oops. Correct.

>>> + debug ("calling relocate_code\n");
>>> relocate_code (addr_sp, id, addr);
>>
>> Have you not just relocated and fixed up in the C code? If so,
>> relocate_code (which would then become a misnomer) should have its own
>> relocate and fixup code removed (your patch does not), and only perform
>> the C environment (stack, etc) switch.
>
> Incorrect. I removed the relocation (#ifndef) in start.S leaving only
> setting of the SP and calling of board_init_r active. True for the
> misnomer...
> It was supposed to be a quick hack, beauty comes later ;)

Ok.

>> Side note: if board_init_f handles relocation and fixup and
>> relocate_code just does stack switch between board_init_f and
>> board_init_r, then I'd rather board_init_f *return* to start.S rather
>> than *call* it if that's possible. That would make the control flow less
>> circuitous: begin in start.S, set a minimal stack, call board_init_f,
>> returns to start.S, switch to final stack, calls (branches to)
>> board_init_r.
>
> I agree here. I would have board_init_f return the desired SP value,
> for example, or let it return the gd pointer - something along that line.
>
> Or one completely skips returning back to asm and sets the sp with an
> asm() statement in "C", obtains a function pointer to board_init_r, adjusts
> that and calls that function.

I'd rather not leave asm statements in board.c as it is supposed to 
converge between archs IIUC.

> Amicalement,

:)

> Reinhard

Amicalement,
-- 
Albert.