From mboxrd@z Thu Jan 1 00:00:00 1970 From: Graeme Russ Date: Sun, 08 Jan 2012 20:04:45 +1100 Subject: [U-Boot] [PATCH v2 15/17] x86: Move relocation code out of board.c In-Reply-To: References: <1325707195-3218-1-git-send-email-graeme.russ@gmail.com> <1325707195-3218-15-git-send-email-graeme.russ@gmail.com> Message-ID: <4F095C2D.7000109@gmail.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Simon, On 08/01/12 09:15, Simon Glass wrote: > Hi Graeme, > > On Wed, Jan 4, 2012 at 11:59 AM, Graeme Russ wrote: >> Signed-off-by: Graeme Russ >> --- >> 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