| Message ID | 1389784796-5584-1-git-send-email-abrodkin@synopsys.com |
|---|---|
| State | Superseded |
| Headers | show |
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. ---
On Wed, 2014-01-15 at 11:27 +0000, thomas.langer@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
Alexey Brodkin wrote on 2014-01-15: > On Wed, 2014-01-15 at 11:27 +0000, thomas.langer@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. ---
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
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,
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
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
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,
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
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
-----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-----
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();
"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(+)