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

[Simon Glass]

Hi Graeme,

On Sun, Jan 8, 2012 at 1:04 AM, Graeme Russ <graeme.russ at gmail.com> wrote:
> 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

Hoping we can one day support relocating to an address known at link time...

Regards,
Simon

>
> Regards,
>
> Graeme