[U-Boot] [PATCH] board_r - fixup functions table after relocation

[U-Boot] [PATCH] board_r - fixup functions table after relocation

[Alexey Brodkin]

"init_sequence_r" is just an array that consists of compile-time
adresses of init functions. Since this is basically an array of integers
(pointers to "void" to be more precise) it won't be modified during
relocation - it will be just copied to new location as it is.

As a consequence on execution after relocation "initcall_run_list" will
be jumping to pre-relocation addresses. As long as we don't overwrite
pre-relocation memory area init calls are executed correctly. But still
it is dangerous because after relocation we don't expect initially used
memory to stay untouched.

Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>

Cc: Tom Rini <trini@ti.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Masahiro Yamada <yamada.m@jp.panasonic.com>
Cc: Doug Anderson <dianders@chromium.org>
---
 common/board_r.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/common/board_r.c b/common/board_r.c
index 86ca1cb..8f45943 100644
--- a/common/board_r.c
+++ b/common/board_r.c
@@ -903,9 +903,14 @@ init_fnc_t init_sequence_r[] = {
 
 void board_init_r(gd_t *new_gd, ulong dest_addr)
 {
+	int i;
 #ifndef CONFIG_X86
 	gd = new_gd;
 #endif
+	/* Fixup table after relocation */
+	for (i = 0; i < sizeof(init_sequence_r)/sizeof(void *); i++)
+		init_sequence_r[i] += gd->reloc_off;
+
 	if (initcall_run_list(init_sequence_r))
 		hang();
 
-- 
1.8.4.2

[U-Boot] [PATCH] board_r - fixup functions table after relocation

[thomas.langer at lantiq.com]

Hello Alexey,

Alexey Brodkin wrote on?2014-01-15:
> "init_sequence_r" is just an array that consists of compile-time
> adresses of init functions. Since this is basically an array of integers
> (pointers to "void" to be more precise) it won't be modified during
> relocation - it will be just copied to new location as it is.
> 
> As a consequence on execution after relocation "initcall_run_list" will
> be jumping to pre-relocation addresses. As long as we don't overwrite
> pre-relocation memory area init calls are executed correctly. But still
> it is dangerous because after relocation we don't expect initially used
> memory to stay untouched.
> 
> Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
> 
> Cc: Tom Rini <trini@ti.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Masahiro Yamada <yamada.m@jp.panasonic.com>
> Cc: Doug Anderson <dianders@chromium.org>
> ---
>  common/board_r.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/common/board_r.c b/common/board_r.c
> index 86ca1cb..8f45943 100644
> --- a/common/board_r.c
> +++ b/common/board_r.c
> @@ -903,9 +903,14 @@ init_fnc_t init_sequence_r[] = {
> 
>  void board_init_r(gd_t *new_gd, ulong dest_addr)
>  {
> +	int i;
>  #ifndef CONFIG_X86
>  	gd = new_gd;
>  #endif
> +	/* Fixup table after relocation */
> +	for (i = 0; i < sizeof(init_sequence_r)/sizeof(void *); i++)
> +		init_sequence_r[i] += gd->reloc_off;
> +

I think this is only required/possible for architectures which define
CONFIG_NEEDS_MANUAL_RELOC, others don't have "gd->reloc_off"

>  	if (initcall_run_list(init_sequence_r))
>  		hang();
> 

Best Regards,
Thomas
---
There are two hard things in computer science: cache invalidation, naming things, and off-by-one errors.
---

[U-Boot] [PATCH] board_r - fixup functions table after relocation

[Alexey Brodkin]

On Wed, 2014-01-15 at 11:27 +0000, thomas.langer at lantiq.com wrote:
> I think this is only required/possible for architectures which define
> CONFIG_NEEDS_MANUAL_RELOC, others don't have "gd->reloc_off"
> 
> >  	if (initcall_run_list(init_sequence_r))
> >  		hang();

Hi Thomas,

I think it's a generic item for all boards that use "common/board_r.c"
and "common/board_f.c". I.e. have CONFIG_BOARD_EARLY_INIT_F and
CONFIG_BOARD_EARLY_INIT_R defined.

I see that in "common/board_f.c" it is used for example in
"setup_reloc()" and this function is executed regardless platform.

Regards,
Alexey 

[U-Boot] [PATCH] board_r - fixup functions table after relocation

[thomas.langer at lantiq.com]

Alexey Brodkin wrote on?2014-01-15:
> On Wed, 2014-01-15 at 11:27 +0000, thomas.langer at lantiq.com wrote:
>> I think this is only required/possible for architectures which define
>> CONFIG_NEEDS_MANUAL_RELOC, others don't have "gd->reloc_off"
>> 
>>>  	if (initcall_run_list(init_sequence_r))
>>>  		hang();
> 
> Hi Thomas,
> 
> I think it's a generic item for all boards that use "common/board_r.c"
> and "common/board_f.c". I.e. have CONFIG_BOARD_EARLY_INIT_F and
> CONFIG_BOARD_EARLY_INIT_R defined.
> 
> I see that in "common/board_f.c" it is used for example in
> "setup_reloc()" and this function is executed regardless platform.
> 
Hello Alexey,

it seems that CONFIG_SYS_GENERIC_BOARD is not used by any architecture
without CONFIG_NEEDS_MANUAL_RELOC, so your patch is okay.

Sorry for the noise.

> Regards,
> Alexey

Best Regards,
Thomas
---
There are two hard things in computer science: cache invalidation, naming things, and off-by-one errors.
---

[U-Boot] [PATCH] board_r - fixup functions table after relocation

[Simon Glass]

Hi Alexey,

On 15 January 2014 04:19, Alexey Brodkin <Alexey.Brodkin@synopsys.com>wrote:

> "init_sequence_r" is just an array that consists of compile-time
> adresses of init functions. Since this is basically an array of integers
> (pointers to "void" to be more precise) it won't be modified during
> relocation - it will be just copied to new location as it is.
>
> As a consequence on execution after relocation "initcall_run_list" will
> be jumping to pre-relocation addresses. As long as we don't overwrite
> pre-relocation memory area init calls are executed correctly. But still
> it is dangerous because after relocation we don't expect initially used
> memory to stay untouched.
>
> Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
>
> Cc: Tom Rini <trini@ti.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Masahiro Yamada <yamada.m@jp.panasonic.com>
> Cc: Doug Anderson <dianders@chromium.org>
> ---
>  common/board_r.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/common/board_r.c b/common/board_r.c
> index 86ca1cb..8f45943 100644
> --- a/common/board_r.c
> +++ b/common/board_r.c
> @@ -903,9 +903,14 @@ init_fnc_t init_sequence_r[] = {
>
>  void board_init_r(gd_t *new_gd, ulong dest_addr)
>  {
> +       int i;
>  #ifndef CONFIG_X86
>         gd = new_gd;
>  #endif
> +       /* Fixup table after relocation */
> +       for (i = 0; i < sizeof(init_sequence_r)/sizeof(void *); i++)
> +               init_sequence_r[i] += gd->reloc_off;
> +
>

ARRAY_SIZE() might be better.

I have not checked to make sure that the array contents remains
un-relocated. Did you see this?


        if (initcall_run_list(init_sequence_r))
>                 hang();
>
> --
> 1.8.4.2
>
>
Regards,
Simon

[U-Boot] [PATCH] board_r - fixup functions table after relocation

[Alexey Brodkin]

On Wed, 2014-01-15 at 09:56 -0700, Simon Glass wrote:


> I have not checked to make sure that the array contents remains
> un-relocated. Did you see this?


Hi Simon,

yes, I do see it.

Please refer to my outputs below:

1. Without manual "init_sequence_r" modification:
================
U-Boot 2014.01-rc1-00207-ga065b75-dirty (Jan 16 2014 - 21:30:12)
initcall: 81000890
U-Boot code: 81000000 -> 8103FE00  BSS: -> 81044278
initcall: 810008f4
DRAM:  initcall: 81000a84
Monitor len: 00044278
Ram size: 10000000
Ram top: 90000000
initcall: 81000b24
initcall: 81000b48
initcall: 81000b5c
Reserving 272k for U-Boot at: 8ffbb000
initcall: 81000bd0
Reserving 2048k for malloc() at: 8fdbae00
initcall: 81000c24
Reserving 64 Bytes for Board Info at: 8fdbadc0
initcall: 81000c9c
initcall: 81000cb0
Reserving 132 Bytes for Global Data at: 8fdbad3c
initcall: 81000d18
initcall: 81000de8
initcall: 81000e78
initcall: 8100092c
256 MiB
initcall: 81000e5c
initcall: 81000e1c
New Stack Pointer is: 8fdbad20
initcall: 81000e94
initcall: 81000ed4
Relocation Offset is: 0efbb000
Relocating to 8ffbb000, new gd at 8fdbad3c, sp at 8fdbad20
initcall: 81000f60
initcall: 81001074
initcall: 81001088
initcall: 810010f4
initcall: 81001120
initcall: 810011b0
Now running in RAM - U-Boot at: 8ffbb000
initcall: 81000760
================

2. With manual "init_sequence_r" modification
================
U-Boot 2014.01-rc1-00207-ga065b75-dirty (Jan 16 2014 - 21:26:42)
initcall: 81000890
U-Boot code: 81000000 -> 8103FE00  BSS: -> 81044278
initcall: 810008f4
DRAM:  initcall: 81000a84
Monitor len: 00044278
Ram size: 10000000
Ram top: 90000000
initcall: 81000b24
initcall: 81000b48
initcall: 81000b5c
Reserving 272k for U-Boot at: 8ffbb000
initcall: 81000bd0
Reserving 2048k for malloc() at: 8fdbae00
initcall: 81000c24
Reserving 64 Bytes for Board Info at: 8fdbadc0
initcall: 81000c9c
initcall: 81000cb0
Reserving 132 Bytes for Global Data at: 8fdbad3c
initcall: 81000d18
initcall: 81000de8
initcall: 81000e78
initcall: 8100092c
256 MiB
initcall: 81000e5c
initcall: 81000e1c
New Stack Pointer is: 8fdbad20
initcall: 81000e94
initcall: 81000ed4
Relocation Offset is: 0efbb000
Relocating to 8ffbb000, new gd at 8fdbad3c, sp at 8fdbad20
initcall: 81000f60
initcall: 8ffbc074
initcall: 8ffbc088
initcall: 8ffbc0f4
initcall: 8ffbc120
initcall: 8ffbc1b0
Now running in RAM - U-Boot at: 8ffbb000
initcall: 8ffbb760
================

Note how in second case mapped initcalls executed.

-Alexey

[U-Boot] [PATCH] board_r - fixup functions table after relocation

[Albert ARIBAUD]

Hi Alexey,

On Wed, 15 Jan 2014 15:19:56 +0400, Alexey Brodkin
<Alexey.Brodkin@synopsys.com> wrote:

> "init_sequence_r" is just an array that consists of compile-time
> adresses of init functions. Since this is basically an array of integers
> (pointers to "void" to be more precise) it won't be modified during
> relocation - it will be just copied to new location as it is.

IIRC, in ARM we switched from GOT to ELF relocation precisely so that
data would be relocated as well as code, and I think it actually is,
otherwise we'd have a lot of complains. Therefore I fail to understand
the statements above. Can someone tell me what I'm getting wrong?
 
> As a consequence on execution after relocation "initcall_run_list" will
> be jumping to pre-relocation addresses. As long as we don't overwrite
> pre-relocation memory area init calls are executed correctly. But still
> it is dangerous because after relocation we don't expect initially used
> memory to stay untouched.
> 
> Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
> 
> Cc: Tom Rini <trini@ti.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Masahiro Yamada <yamada.m@jp.panasonic.com>
> Cc: Doug Anderson <dianders@chromium.org>
> ---
>  common/board_r.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/common/board_r.c b/common/board_r.c
> index 86ca1cb..8f45943 100644
> --- a/common/board_r.c
> +++ b/common/board_r.c
> @@ -903,9 +903,14 @@ init_fnc_t init_sequence_r[] = {
>  
>  void board_init_r(gd_t *new_gd, ulong dest_addr)
>  {
> +	int i;
>  #ifndef CONFIG_X86
>  	gd = new_gd;
>  #endif
> +	/* Fixup table after relocation */
> +	for (i = 0; i < sizeof(init_sequence_r)/sizeof(void *); i++)
> +		init_sequence_r[i] += gd->reloc_off;
> +
>  	if (initcall_run_list(init_sequence_r))
>  		hang();
>  


Amicalement,
-- 
Albert.

[U-Boot] [PATCH] board_r - fixup functions table after relocation

[Alexey Brodkin]

On Wed, 2014-01-15 at 22:43 +0100, Albert ARIBAUD wrote:
> Hi Alexey,
> 
> On Wed, 15 Jan 2014 15:19:56 +0400, Alexey Brodkin
> <Alexey.Brodkin@synopsys.com> wrote:
> 
> > "init_sequence_r" is just an array that consists of compile-time
> > adresses of init functions. Since this is basically an array of integers
> > (pointers to "void" to be more precise) it won't be modified during
> > relocation - it will be just copied to new location as it is.
> 
> IIRC, in ARM we switched from GOT to ELF relocation precisely so that
> data would be relocated as well as code, and I think it actually is,
> otherwise we'd have a lot of complains. Therefore I fail to understand
> the statements above. Can someone tell me what I'm getting wrong?

Unfortunately I don't have any supported in U-Boot ARM board handy and
run U-boot on another architecture at all (Synopsys DesignWare ARC) so
I'm not sure if on ARM functions from "init_sequence_r" list are
executed from "RAM" (i.e. from location where they were relocated).

I use GOT relocation and see following outputs.

1. Without manual "init_sequence_r" modification:
================
U-Boot 2014.01-rc1-00207-ga065b75-dirty (Jan 16 2014 - 21:30:12)
initcall: 81000890
U-Boot code: 81000000 -> 8103FE00  BSS: -> 81044278
initcall: 810008f4
DRAM:  initcall: 81000a84
Monitor len: 00044278
Ram size: 10000000
Ram top: 90000000
initcall: 81000b24
initcall: 81000b48
initcall: 81000b5c
Reserving 272k for U-Boot at: 8ffbb000
initcall: 81000bd0
Reserving 2048k for malloc() at: 8fdbae00
initcall: 81000c24
Reserving 64 Bytes for Board Info at: 8fdbadc0
initcall: 81000c9c
initcall: 81000cb0
Reserving 132 Bytes for Global Data at: 8fdbad3c
initcall: 81000d18
initcall: 81000de8
initcall: 81000e78
initcall: 8100092c
256 MiB
initcall: 81000e5c
initcall: 81000e1c
New Stack Pointer is: 8fdbad20
initcall: 81000e94
initcall: 81000ed4
Relocation Offset is: 0efbb000
Relocating to 8ffbb000, new gd at 8fdbad3c, sp at 8fdbad20
initcall: 81000f60
initcall: 81001074
initcall: 81001088
initcall: 810010f4
initcall: 81001120
initcall: 810011b0
Now running in RAM - U-Boot at: 8ffbb000
initcall: 81000760
================

2. With manual "init_sequence_r" modification
================
U-Boot 2014.01-rc1-00207-ga065b75-dirty (Jan 16 2014 - 21:26:42)
initcall: 81000890
U-Boot code: 81000000 -> 8103FE00  BSS: -> 81044278
initcall: 810008f4
DRAM:  initcall: 81000a84
Monitor len: 00044278
Ram size: 10000000
Ram top: 90000000
initcall: 81000b24
initcall: 81000b48
initcall: 81000b5c
Reserving 272k for U-Boot at: 8ffbb000
initcall: 81000bd0
Reserving 2048k for malloc() at: 8fdbae00
initcall: 81000c24
Reserving 64 Bytes for Board Info at: 8fdbadc0
initcall: 81000c9c
initcall: 81000cb0
Reserving 132 Bytes for Global Data at: 8fdbad3c
initcall: 81000d18
initcall: 81000de8
initcall: 81000e78
initcall: 8100092c
256 MiB
initcall: 81000e5c
initcall: 81000e1c
New Stack Pointer is: 8fdbad20
initcall: 81000e94
initcall: 81000ed4
Relocation Offset is: 0efbb000
Relocating to 8ffbb000, new gd at 8fdbad3c, sp at 8fdbad20
initcall: 81000f60
initcall: 8ffbc074
initcall: 8ffbc088
initcall: 8ffbc0f4
initcall: 8ffbc120
initcall: 8ffbc1b0
Now running in RAM - U-Boot at: 8ffbb000
initcall: 8ffbb760
================

Note how in second case mapped initcalls executed.

If anybody may try to build U-Boot with global "DEBUG" defined and paste
his log in this thread it would be helpful.

Maybe it's just my faulty implementation of relocation but it might be
that nobody ever noticed this because I think only initcalls are
affected.

-Alexey

[U-Boot] [PATCH] board_r - fixup functions table after relocation

[Albert ARIBAUD]

Hi Alexey,

On Thu, 16 Jan 2014 17:48:45 +0000, Alexey Brodkin
<Alexey.Brodkin@synopsys.com> wrote:

> On Wed, 2014-01-15 at 22:43 +0100, Albert ARIBAUD wrote:
> > Hi Alexey,
> > 
> > On Wed, 15 Jan 2014 15:19:56 +0400, Alexey Brodkin
> > <Alexey.Brodkin@synopsys.com> wrote:
> > 
> > > "init_sequence_r" is just an array that consists of compile-time
> > > adresses of init functions. Since this is basically an array of integers
> > > (pointers to "void" to be more precise) it won't be modified during
> > > relocation - it will be just copied to new location as it is.
> > 
> > IIRC, in ARM we switched from GOT to ELF relocation precisely so that
> > data would be relocated as well as code, and I think it actually is,
> > otherwise we'd have a lot of complains. Therefore I fail to understand
> > the statements above. Can someone tell me what I'm getting wrong?
> 
> Unfortunately I don't have any supported in U-Boot ARM board handy and
> run U-boot on another architecture at all (Synopsys DesignWare ARC) so
> I'm not sure if on ARM functions from "init_sequence_r" list are
> executed from "RAM" (i.e. from location where they were relocated).

Yes, they are.

> I use GOT relocation and see following outputs.

GOT relocation does not relocate references within data, contrary
to ELF.

> Maybe it's just my faulty implementation of relocation but it might be
> that nobody ever noticed this because I think only initcalls are
> affected.

Well, initcalls are quite essential, I guess.

When you say "my faulty implementation of relocation"... do you mean
some implementation different from what is currently in mainline?

> -Alexey

Amicalement,
-- 
Albert.

[U-Boot] [PATCH] board_r - fixup functions table after relocation

[Alexey Brodkin]

On Thu, 2014-01-16 at 21:27 +0100, Albert ARIBAUD wrote:
> GOT relocation does not relocate references within data, contrary
> to ELF.
> 
> > Maybe it's just my faulty implementation of relocation but it might be
> > that nobody ever noticed this because I think only initcalls are
> > affected.
> 
> Well, initcalls are quite essential, I guess.
Agree, but IMHO it may not hurt if they are executed from initial
location until this initial memory was overwritten which I don't
expect to happen till the end of init sequence.

So the only way to learn that init calls were executed from wrong
location was examination of boot log in DEBUG mode.

> When you say "my faulty implementation of relocation"... do you mean
> some implementation different from what is currently in mainline?

Well as I'm preparing for submission port for new (for mainline U-Boot)
architecture and relocation code as I see is implemented in
arch-specific assembly code so my implementation of relocation code is
indeed differs from currently available ones in U-Boot.

But I copied my implementation from PIC-based relocation of NDS32 so in
general I now think it has to work (as good as PIC-relocation culd work
in current U-Boot). 

-Alexey

[U-Boot] [PATCH] board_r - fixup functions table after relocation

[Alexey Brodkin]

On Fri, 2014-01-17 at 00:40 +0400, Alexey Brodkin wrote:
> On Thu, 2014-01-16 at 21:27 +0100, Albert ARIBAUD wrote:

Well, I've just realized that very similar thing is done in
"fixup_cmdtable".

So I believe with addition of CONFIG_NEEDS_MANUAL_RELOC wrapper my
change may make some sense.

Any comments/thoughts?

-Alexey

[U-Boot] [PATCH] board_r - fixup functions table after relocation

[Tom Rini]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 01/16/2014 07:06 PM, Alexey Brodkin wrote:
> On Fri, 2014-01-17 at 00:40 +0400, Alexey Brodkin wrote:
>> On Thu, 2014-01-16 at 21:27 +0100, Albert ARIBAUD wrote:
> 
> Well, I've just realized that very similar thing is done in 
> "fixup_cmdtable".
> 
> So I believe with addition of CONFIG_NEEDS_MANUAL_RELOC wrapper my 
> change may make some sense.
> 
> Any comments/thoughts?

Yes, with a wrapper on CONFIG_NEEDS_MANUAL_RELOC this patch sounds
right.  Thanks!

- -- 
Tom
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJS2Sf1AAoJENk4IS6UOR1WYRMP/1SYow+7k5RITcITfPcd7eOR
2dkduJhxGh02vz50/9oWsN2jBIOJq8CddtT+N77+PVCbJf96Tgm4bVju/TcmMwwr
9xFBr7cZmHtVkgB2bb3iZN/KkMwRF9d26QMDr3hiPKYVCN7appHDe3TCwWuQopbp
BjcKhOF+IWDb7qzzsRdgghaWz5hFqnNFMBBtszhdOLGkdEF3be7MwqPqcOsUN59p
J6BnI8C7q/A/uQC22kHIhaofQaYel+mEgMOqzdqrUopM1NzFZ7eX36kyQYoZn5Hl
DVVmeskj9OqOrGPMMU9DrJqWdD8gQ35cgScrkIhy0x0gtN2DX0YF49CDBu95MEHX
5oMDB/eIARNtrL5gwAiNGKug7TXvnqwIYpXedH1vMXLkafAyIiOD6aDik+DB+CTl
Jd1OfzrtzhMxy/vLkyxNReh6CTJj0zsAmjeksiaYl+WFJ1HNdmoThwZU2s6KG00n
pYaeQUgDaysHG1x2HPcx88zGkPlHLpRP5jy75PSJG9dsIBpVL29O9oj1sqKqoGl1
7sdSbfA7YFBSAlNjO5j4zsKDl5WxUx0kgWnPRhLJ+oU1SloFN6i5x9vn8cVXz8Bq
3u8pPsb9iyVDfUgXHzkNy7VwLiWToMtSSGhuuzttzFEe/2IkyG+vgnAFf/J4qHQj
J0UdIk4Ha52fkLM5Q4uu
=CWdX
-----END PGP SIGNATURE-----