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
Regards,
Graeme
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
