Hi Graeme,
On Sun, Jan 8, 2012 at 1:04 AM, Graeme Russ <graeme.r...@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.r...@gmail.com> wrote:
>>> Signed-off-by: Graeme Russ <graeme.r...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
