[U-Boot] [PATCH v2 15/17] x86: Move relocation code out of board.c

[Graeme Russ]

Hi Simon,

On 08/01/12 09:15, Simon Glass wrote:
> Hi Graeme,
> 
> On Wed, Jan 4, 2012 at 11:59 AM, Graeme Russ <graeme.russ at gmail.com> wrote:
>> Signed-off-by: Graeme Russ <graeme.russ at gmail.com>
>> ---
>> Changes for v2:
>>  - None
>>
>>  arch/x86/lib/Makefile   |    1 +
>>  arch/x86/lib/board.c    |   69 +---------------------------
>>  arch/x86/lib/relocate.c |  115 +++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 118 insertions(+), 67 deletions(-)
>>  create mode 100644 arch/x86/lib/relocate.c
> 
> Sorry - these comments are for future reference, since all you are
> doing here is moving code. But I might as well send them here.

Yes, your comments are good for future reference when the relocation code
becomes common

[snip]

>> -               /* Check that the location of the relocation is in .text */
>> -               if (offset_ptr_rom >= (Elf32_Addr *)CONFIG_SYS_TEXT_BASE) {
> 
> perhaps:
> 
> if (re_src->r_offset >= CONFIG_SYS_TEXT_BASE) {

Yep

> 
>> -
>> -                       /* Switch to the in-RAM version */
>> -                       offset_ptr_ram = (Elf32_Addr *)((ulong)offset_ptr_rom +
>> -                                                       gd->reloc_off);
>> -
>> -                       /* Check that the target points into .text */
>> -                       if (*offset_ptr_ram >= CONFIG_SYS_TEXT_BASE &&
>> -                                       *offset_ptr_ram <
>> -                                       (CONFIG_SYS_TEXT_BASE + size)) {
>> -                               *offset_ptr_ram += gd->reloc_off;
> 
> Can the target not pointer into data? I think you are allowing this
> anyway with your test, but are you sure this comment is right?

You are correct, target is not necessarily .text

> 
>> -                       }
>> -               }
>> -       } while (re_src++ < re_end);
> 
> Should this while() condition go at the top? What if the table has no entries?

An absurdly unlikely scenario, but worth changing for strict correctness

Regards,

Graeme