[U-Boot] Image.c get_table_entry_name

[U-Boot] Image.c get_table_entry_name

[Swarthout Edward L-SWARTHOU]

The string pointers in uimage_{arch,os,type,comp}[] are not being
relocated and still point to flash.
If flash is erased (all f's), this causes bootm to trap on a bad
pointer.

This is for powerpc (mpc8572ds).  How are they suppose to be relocated?

-EdS

=> boot
boot
## Booting kernel from Legacy Image at 01000000 ...
   Image Name:   Linux-2.6.26-f8572-8572ds-00610-
   Image Type:   Bad trap at PC: 3ffa7ae8, SR: 29200, vector=d00
NIP: 3FFA7AE8 XER: 20000000 LR: 3FFA74F8 REGS: 3fe9d8b0 TRAP: 0d00 DAR:
F0000000
MSR: 00029200 EE: 1 PR: 0 FP: 0 ME: 1 IR/DR: 00

GPR00: 000000FF 3FE9D9A0 3FE9DF70 F0000000 00000000 3FE9D9E0 EFFB8358
EFFB8300 
GPR08: 000000A0 EFFB849C 3FFD7FD5 00000002 00000001 10030BD0 3FFEAC00
00000000 
GPR16: 3FFC28D0 3FFD782C 00000000 00000000 00000000 00000000 00000000
3FFD6168 
GPR24: 3FE9D9A8 3FE9D9EC 00000000 FFFFFFFF EFFB849C 3FE9D9EC 3FFEADCC
3FE9D9E0 

[U-Boot] Image.c get_table_entry_name

[Peter Tyser]

Hi Ed,

> The string pointers in uimage_{arch,os,type,comp}[] are not being
> relocated and still point to flash.
> If flash is erased (all f's), this causes bootm to trap on a bad
> pointer.
> 
> This is for powerpc (mpc8572ds).  How are they suppose to be relocated?
> 
> -EdS
> 
> => boot
> boot
> ## Booting kernel from Legacy Image at 01000000 ...
>    Image Name:   Linux-2.6.26-f8572-8572ds-00610-
>    Image Type:   Bad trap at PC: 3ffa7ae8, SR: 29200, vector=d00
> NIP: 3FFA7AE8 XER: 20000000 LR: 3FFA74F8 REGS: 3fe9d8b0 TRAP: 0d00 DAR:
> F0000000
> MSR: 00029200 EE: 1 PR: 0 FP: 0 ME: 1 IR/DR: 00
> 
> GPR00: 000000FF 3FE9D9A0 3FE9DF70 F0000000 00000000 3FE9D9E0 EFFB8358
> EFFB8300 
> GPR08: 000000A0 EFFB849C 3FFD7FD5 00000002 00000001 10030BD0 3FFEAC00
> 00000000 
> GPR16: 3FFC28D0 3FFD782C 00000000 00000000 00000000 00000000 00000000
> 3FFD6168 
> GPR24: 3FE9D9A8 3FE9D9EC 00000000 FFFFFFFF EFFB849C 3FE9D9EC 3FFEADCC
> 3FE9D9E0 

Re-applying Grant's change that was backed out here:
http://article.gmane.org/gmane.comp.boot-loaders.u-boot/33297 solved the
same issue (with other commands) for me when using gcc 4.2.2 for an
8572-based board.  eg:

- #define CONFIG_RELOC_FIXUP_WORKS in your board header
- remove *(.fixup) from your board's linker script
- add -mrelocatable to your compile flags

Best,
Peter

[U-Boot] [PATCH RFC] mpc8572ds relocatable

[Ed Swarthout]

Fixes boot crash from bad string pointers in get_table_entry_name
when flash is erased or differs from current u-boot image.

Signed-off-by: Ed Swarthout <Ed.Swarthout@freescale.com>
---

Fix was pointed out by Peter Tyser in Image.c get_table_entry_name thread.

This redoes Grant Likey's relocation change, but leaves control to each board.
This also fixes the "mii dump" command when flash is erased.
Tested with gcc 4.2

 board/freescale/mpc8572ds/config.mk  |    1 +
 board/freescale/mpc8572ds/u-boot.lds |    2 +-
 2 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/board/freescale/mpc8572ds/config.mk b/board/freescale/mpc8572ds/config.mk
index 5b32186..8b3651b 100644
--- a/board/freescale/mpc8572ds/config.mk
+++ b/board/freescale/mpc8572ds/config.mk
@@ -30,3 +30,4 @@ endif
 PLATFORM_CPPFLAGS += -DCONFIG_E500=1
 PLATFORM_CPPFLAGS += -DCONFIG_MPC85xx=1
 PLATFORM_CPPFLAGS += -DCONFIG_MPC8572=1
+PLATFORM_CPPFLAGS += -mrelocatable -DCONFIG_RELOC_FIXUP_WORKS
diff --git a/board/freescale/mpc8572ds/u-boot.lds b/board/freescale/mpc8572ds/u-boot.lds
index a05ece5..79fb41f 100644
--- a/board/freescale/mpc8572ds/u-boot.lds
+++ b/board/freescale/mpc8572ds/u-boot.lds
@@ -58,7 +58,7 @@ SECTIONS
   .text      :
   {
     *(.text)
-    *(.fixup)
+/*  *(.fixup)*/
     *(.got1)
    } :text
     _etext = .;
-- 
1.5.6.5

[U-Boot] [PATCH RFC] mpc8572ds relocatable

[JerryVanBaren]

Ed Swarthout wrote:
> Fixes boot crash from bad string pointers in get_table_entry_name
> when flash is erased or differs from current u-boot image.
> 
> Signed-off-by: Ed Swarthout <Ed.Swarthout@freescale.com>
> ---
> 
> Fix was pointed out by Peter Tyser in Image.c get_table_entry_name thread.
> 
> This redoes Grant Likey's relocation change, but leaves control to each board.
> This also fixes the "mii dump" command when flash is erased.
> Tested with gcc 4.2
> 
>  board/freescale/mpc8572ds/config.mk  |    1 +
>  board/freescale/mpc8572ds/u-boot.lds |    2 +-
>  2 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/board/freescale/mpc8572ds/config.mk b/board/freescale/mpc8572ds/config.mk
> index 5b32186..8b3651b 100644
> --- a/board/freescale/mpc8572ds/config.mk
> +++ b/board/freescale/mpc8572ds/config.mk
> @@ -30,3 +30,4 @@ endif
>  PLATFORM_CPPFLAGS += -DCONFIG_E500=1
>  PLATFORM_CPPFLAGS += -DCONFIG_MPC85xx=1
>  PLATFORM_CPPFLAGS += -DCONFIG_MPC8572=1
> +PLATFORM_CPPFLAGS += -mrelocatable -DCONFIG_RELOC_FIXUP_WORKS
> diff --git a/board/freescale/mpc8572ds/u-boot.lds b/board/freescale/mpc8572ds/u-boot.lds
> index a05ece5..79fb41f 100644
> --- a/board/freescale/mpc8572ds/u-boot.lds
> +++ b/board/freescale/mpc8572ds/u-boot.lds
> @@ -58,7 +58,7 @@ SECTIONS
>    .text      :
>    {
>      *(.text)
> -    *(.fixup)
> +/*  *(.fixup)*/

Please don't (just) comment this out.

Preferably your commit message would have enough information to answer 
the question of why .fixup was removed and no comment in the linker 
control file would be necessary.

In some cases it is worth adding a comment to the code as to why .fixup 
was removed so that someone that doesn't know the background doesn't add 
it back in.  Something like this hand generated pseudo patch:

+/*
+ * Note: The *(.fixup) section is unnecessary because the
+ * CONFIG_RELOC_FIXUP_WORKS method is being used to do the relocation.
+ */

    .text      :
    {
      *(.text)
-    *(.fixup)
      *(.got1)
     } :text


Thanks,
gvb

[U-Boot] [PATCH RFC] mpc8572ds relocatable

[Wolfgang Denk]

Dear Ed Swarthout,

In message <1220862412-16162-1-git-send-email-Ed.Swarthout@freescale.com> you wrote:
> Fixes boot crash from bad string pointers in get_table_entry_name
> when flash is erased or differs from current u-boot image.
> 
> Signed-off-by: Ed Swarthout <Ed.Swarthout@freescale.com>
> ---
> 
> Fix was pointed out by Peter Tyser in Image.c get_table_entry_name thread.
> 
> This redoes Grant Likey's relocation change, but leaves control to each board.
> This also fixes the "mii dump" command when flash is erased.
> Tested with gcc 4.2

NAK.


Sorry, this doesn't work. We cannot have one board compile this way,
and another one another way - one working with that  tool  chain  and
the other with another tool chain only.

Theproblem you're addressing is a bug, and not only  on  this  single
board, but everywhere. So it must be fixed everywhere.

Either we  add  manual  relocation  to  these  pointers  like  we  do
elsewhere (that would be my recommendation for a quick workaround aka
bug  fix for this release), or we should find a clean way to get real
relocation working (that would be much better, but probably it is too
late for this release).

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The optimum committee has no members.
                                                   - Norman Augustine

[U-Boot] [PATCH RFC] mpc8572ds relocatable

[Graeme Russ]

Wolfgang,

On Mon, Oct 13, 2008 at 9:53 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Ed Swarthout,
>
> In message <1220862412-16162-1-git-send-email-Ed.Swarthout@freescale.com> you wrote:
>> Fixes boot crash from bad string pointers in get_table_entry_name
>> when flash is erased or differs from current u-boot image.
>>

This is exactly the problem I am looking at for my board so I can do
flash upgrades of U-Boot from U-Boot

> Either we  add  manual  relocation  to  these  pointers  like  we  do
> elsewhere (that would be my recommendation for a quick workaround aka
> bug  fix for this release), or we should find a clean way to get real
> relocation working (that would be much better, but probably it is too
> late for this release).

I am looking into this this using gcc -fpie / ld -pie. This is used
by hardened system for address space randomization. It appears to be
very well supported by recent versions of gcc and binutils on all
platforms so appears to me to be the way to go.

The number of new sections generated in a PIE image is more than I
expected. The following sections have differences when TEXT_BASE is
changed when compiled with -fpie and linked with -pie:

.dynsym
.dynamic
.data.rel
.data.rel.local
.data.rel.ro.local
.got.plt
.got
.rel.got
.rel.text
.rel.u_boot_cmd
.rel.data.rel
.rel.data.rel.ro.local
.rel.data.rel.local

Unfortunately, not all of these appear to be record-based sections,
and may even cross-reference each other. I was looking at throwing
this in the 'too hard' basket for now, keeping -fpie / -pie and just
setting TEXT_BASE to a fixed address in RAM and look at implementing
the relocator later. However, it seems to me that implementing full,
clean relocation just keeps on popping up as a real issue.

Wolfgang - If you like, I could bump this up my priority list and get
PIE support added to lib_i386 and from there it can be ported to other
arches. Unfortunately my work to date has been for a custom i386 board
so whatever I come up with may not tie 100% cleanly into mainline. I
have been maintaining a detailed step-by-step commit path, so it
should not be that bad, but expect ~20 commits from mainline to PIE.

>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
> The optimum committee has no members.
>                                                   - Norman Augustine
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>

[U-Boot] [PATCH RFC] mpc8572ds relocatable

[Joakim Tjernlund]

On Mon, 2008-10-13 at 00:53 +0200, Wolfgang Denk wrote:
> Dear Ed Swarthout,
> 
> In message <1220862412-16162-1-git-send-email-Ed.Swarthout@freescale.com> you wrote:
> > Fixes boot crash from bad string pointers in get_table_entry_name
> > when flash is erased or differs from current u-boot image.
> > 
> > Signed-off-by: Ed Swarthout <Ed.Swarthout@freescale.com>
> > ---
> > 
> > Fix was pointed out by Peter Tyser in Image.c get_table_entry_name thread.
> > 
> > This redoes Grant Likey's relocation change, but leaves control to each board.
> > This also fixes the "mii dump" command when flash is erased.
> > Tested with gcc 4.2

Just wanted to point out that the problems from Grants relocation change
might come from something I found a while back, quoting from previous
mail:

For fun I had a look into eabi.asm code at 
 http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/config/rs6000/eabi.asm?rev=1.13&content-type=text/x-cvsweb-markup
and I noticed one difference:
The __eabi_uconvert() function skips NULL ptrs.

Perhaps this is the missing piece needed in start.S for PPC?

__eabi_convert pasted below for convenience.

    Jocke

FUNC_START(__eabi_convert)
        cmplw   1,3,4                                /* any pointers to convert? */
        subf    5,3,4                                /* calculate number of words to convert */
        bclr    4,4                                /* return if no pointers */

        srawi   5,5,2
        addi        3,3,-4                                /* start-4 for use with lwzu */
        mtctr   5

.Lcvt:
        lwzu        6,4(3)                                /* pointer to convert */
        cmpwi        0,6,0
        beq-        .Lcvt2                                /* if pointer is null, don't convert */

        add     6,6,12                                /* convert pointer */
        stw     6,0(3)
.Lcvt2:
        bdnz+   .Lcvt
        blr

[U-Boot] [PATCH RFC] mpc8572ds relocatable

[Swarthout Edward L]

From: Joakim Tjernlund [mailto:joakim.tjernlund at transmode.se] 
> 
> The __eabi_uconvert() function skips NULL ptrs.
> 
> Perhaps this is the missing piece needed in start.S for PPC?

I'm not seeing any zeros in the .got2 or .fixup entries 
in the .reloc section on 85xx when compiling with -mrelocatable.

So I don't think that would help.

-Ed

[U-Boot] [PATCH RFC] mpc8572ds relocatable

[Joakim Tjernlund]

On Mon, 2008-10-13 at 20:59 -0700, Swarthout Edward L wrote:
> From: Joakim Tjernlund [mailto:joakim.tjernlund at transmode.se] 
> > 
> > The __eabi_uconvert() function skips NULL ptrs.
> > 
> > Perhaps this is the missing piece needed in start.S for PPC?
> 
> I'm not seeing any zeros in the .got2 or .fixup entries 
> in the .reloc section on 85xx when compiling with -mrelocatable.
> 
> So I don't think that would help.

Neither do I but how do you know that others don't? It
can depend on your u-boot config and/or gcc version.
It is a bug anyway, it makes sense not to mess with NULL ptrs

 Jocke

[U-Boot] [PATCH RFC] mpc8572ds relocatable

[Swarthout Edward L]

From: Wolfgang Denk [mailto:wd at denx.de] 
> 
> NAK.
> 
> Sorry, this doesn't work. We cannot have one board compile 
> this way, and another one another way - one working with that 
> tool  chain  and the other with another tool chain only.

Is there a list of the toolchains that -mrelocatable must 
be tested against for each board?

> The problem you're addressing is a bug, and not only  on  this 
> single board, but everywhere. So it must be fixed everywhere.
> 
> Either we  add  manual  relocation  to  these  pointers  like 
> we  do elsewhere (that would be my recommendation for a 
> quick workaround aka bug  fix for this release), or we should 
> find a clean way to get real relocation working (that would 
> be much better, but probably it is too late for this release).

I know of pointers in structures in cmd_mii.c and image.c that 
need relocation, but how many more are there?  And where should 
the manual relocation code reside?

I think the effort should be applied to making sure the toolchain
can work with relocatable instead.

In my testing of this patch, I found a crash with the Radeon
driver because the x86emu code forces entries into got2 -
which get double relocated when using -mrelocatedable.

See:
[PATCH 1/2] Make mpc8572ds, mpc8544ds, mpc8536ds relocatable
[PATCH 2/2] Leave x86emu op code tables in default section

Here:
http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/48102

For a new version of this patch.

-Ed