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