board_r - fixup functions table after relocation

PATCH New Alexey Brodkin 6 participants 12 messages Jan 15, 2014 – Jan 17, 2014

Share & tools

Alexey Brodkin

author

2014-01-15 11:19 UTC #0 lore raw

PATCH 1/1 board_r - fixup functions table after relocation

"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 <[email protected]>

Cc Tom Rini <[email protected]>

Cc Simon Glass <[email protected]>

Cc Masahiro Yamada <[email protected]>

Cc Doug Anderson <[email protected]>

---
 common/board_r.c | 5 +++++
 1 file changed, 5 insertions(+)

common/board_r.c +5 -0

index 86ca1cb..8f45943 100644
@@ -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();

Signature · 1 line

1.8.4.2

thomas.langer at lantiq.com

2014-01-15 11:27 UTC #1 lore raw

PATCH 1/1 board_r - fixup functions table after relocation

Hello Alexey,

Alexey Brodkin wrote on?2014-01-15:

Quoted 27 lines

"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 <[email protected]>
Cc Tom Rini <[email protected]>
Cc Simon Glass <[email protected]>
Cc Masahiro Yamada <[email protected]>
Cc Doug Anderson <[email protected]>
---
 common/board_r.c | 5 +++++
 1 file changed, 5 insertions(+)
common/board_r.c +0 -0
index 86ca1cb..8f45943 100644
@@ -903,9 +903,14 @@ init_fnc_t init_sequence_r[] = {
 

common/board_r.c init_fnc_t init_sequence_r[] = { · line 913

Quoted 10 lines

common/board_r.c +5 -0

 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"

common/board_r.c init_fnc_t init_sequence_r[] = { · line 916

Quoted 3 lines

common/board_r.c +0 -0

 	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.
---

Alexey Brodkin

author

2014-01-15 12:58 UTC #2 lore raw

PATCH 1/1 board_r - fixup functions table after relocation

Quoted 5 lines · thomas.langer at lantiq.com

I think this is only required/possible for architectures which define CONFIG_NEEDS_MANUAL_RELOC, others don't have "gd->reloc_off"
Quoted 2 lines
common/board_r.c +0 -0
 	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

thomas.langer at lantiq.com

2014-01-15 13:50 UTC #3 lore raw

PATCH 1/1 board_r - fixup functions table after relocation

Alexey Brodkin wrote on?2014-01-15:

Quoted 7 lines · thomas.langer at lantiq.com

Quoted 5 lines
I think this is only required/possible for architectures which define CONFIG_NEEDS_MANUAL_RELOC, others don't have "gd->reloc_off"
Quoted 2 lines
common/board_r.c +0 -0
 	if (initcall_run_list(init_sequence_r))
 		hang();

Quoted 9 lines

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.

Quoted 2 lines

Regards,
Alexey

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

Simon Glass

2014-01-15 16:56 UTC #4 lore raw

PATCH 1/1 board_r - fixup functions table after relocation

In reply to Alexey Brodkin

Hi Alexey,

Quoted 28 lines · 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 <[email protected]>
Cc Tom Rini <[email protected]>
Cc Simon Glass <[email protected]>
Cc Masahiro Yamada <[email protected]>
Cc Doug Anderson <[email protected]>
---
 common/board_r.c | 5 +++++
 1 file changed, 5 insertions(+)
common/board_r.c +0 -0
index 86ca1cb..8f45943 100644
@@ -903,9 +903,14 @@ init_fnc_t init_sequence_r[] = {
 
 void board_init_r(gd_t *new_gd, ulong dest_addr)

common/board_r.c init_fnc_t init_sequence_r[] = { · line 914

Quoted 10 lines

common/board_r.c +5 -0

 {
+       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))

Quoted 6 lines

common/board_r.c +0 -1

                hang();
 
--

1.8.4.2

Regards,
Simon

Alexey Brodkin

author

2014-01-16 17:51 UTC #5 lore raw

PATCH 1/1 board_r - fixup functions table after relocation

Quoted 2 lines · Simon Glass

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

Albert ARIBAUD

2014-01-15 21:43 UTC #6 lore raw

PATCH 1/1 board_r - fixup functions table after relocation

In reply to Alexey Brodkin

Hi Alexey,

Quoted 4 lines · 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.

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?

Quoted 25 lines

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 <[email protected]>
Cc Tom Rini <[email protected]>
Cc Simon Glass <[email protected]>
Cc Masahiro Yamada <[email protected]>
Cc Doug Anderson <[email protected]>
---
 common/board_r.c | 5 +++++
 1 file changed, 5 insertions(+)
common/board_r.c +1 -0
index 86ca1cb..8f45943 100644
@@ -903,9 +903,14 @@ init_fnc_t init_sequence_r[] = {
 
 void board_init_r(gd_t *new_gd, ulong dest_addr)
 {
+	int i;

common/board_r.c init_fnc_t init_sequence_r[] = { · line 916

Quoted 10 lines

common/board_r.c +4 -0

 #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,

Signature · 1 line

Albert.

Alexey Brodkin

author

2014-01-16 17:48 UTC #7 lore raw

PATCH 1/1 board_r - fixup functions table after relocation

Quoted 10 lines · Albert ARIBAUD

Hi Alexey,
Quoted 4 lines · 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.

Quoted 4 lines

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.

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

Albert ARIBAUD

2014-01-16 20:27 UTC #8 lore raw

PATCH 1/1 board_r - fixup functions table after relocation

Hi Alexey,

Quoted 16 lines · Alexey Brodkin

Quoted 10 lines · Albert ARIBAUD
Hi Alexey,
Quoted 4 lines · 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.
Quoted 4 lines
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?

Quoted 4 lines

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.

Quoted 1 line

I use GOT relocation and see following outputs.

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

Quoted 3 lines

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?

Quoted 1 line

-Alexey

Amicalement,

Signature · 1 line

Albert.

Alexey Brodkin

author

2014-01-16 20:40 UTC #9 lore raw

PATCH 1/1 board_r - fixup functions table after relocation

Quoted 8 lines · Albert ARIBAUD

GOT relocation does not relocate references within data, contrary to ELF.
Quoted 3 lines
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.

Quoted 2 lines

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

Alexey Brodkin

2014-01-17 00:06 UTC #10 lore raw

PATCH 1/1 board_r - fixup functions table after relocation

Quoted 1 line · Alexey Brodkin

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

Tom Rini

2014-01-17 12:54 UTC #11 lore raw

PATCH 1/1 board_r - fixup functions table after relocation

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

Quoted 10 lines · Alexey Brodkin

Quoted 1 line · Alexey Brodkin
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-----