[U-Boot-Users] [Patch] Disable icache before call the first line of kernel in do_bootelf().
On some architecture, it is not save to call the first line of kernel without disable the instruction cache. For do_bootm() path, architecture specific library code do_bootm_linux() can adress this issue, but do_bootelf() is common code. This patch disables icache before call kernel.
Signed-off-by: Sonic Zhang sonic.zhang@gmail.com
Index: common/cmd_elf.c =================================================================== --- common/cmd_elf.c (revision 871) +++ common/cmd_elf.c (working copy) @@ -65,6 +65,8 @@ */ if (dcache_status ()) dcache_disable (); + if (icache_status ()) + icache_disable ();
/* pass cmdline to the kernel. */ cmdline = make_command_line();
In message 1186552257.15798.6.camel@sevens.analog.com you wrote:
On some architecture, it is not save to call the first line of kernel without disable the instruction cache. For do_bootm() path, architecture specific library code do_bootm_linux() can adress this issue, but do_bootelf() is common code. This patch disables icache before call kernel.
Signed-off-by: Sonic Zhang sonic.zhang@gmail.com
Did you test the impact of this change on all other architectures but the one you are talking about (without ever mentioning it)?
I don't think we want to do this in general. Please make this code conditional.
Best regards,
Wolfgang Denk
Hi,
On some architecture, it is not save to call the first line of kernel without disable the instruction cache. For do_bootm() path, architecture specific library code do_bootm_linux() can adress this issue, but do_bootelf() is common code. This patch disables icache before call kernel. Signed-off-by: Sonic Zhang sonic.zhang@gmail.com
Did you test the impact of this change on all other architectures but the one you are talking about (without ever mentioning it)?
I don't think we want to do this in general. Please make this code conditional.
I met with this problem on PowerQuicc II Pro in bootm command. Disabling icache before call kernel solved my problem. I suppose the same problem can be in bootelf command.
Best regards, Michal Simek
On Wed 8 Aug 2007 02:50, Wolfgang Denk pondered:
In message 1186552257.15798.6.camel@sevens.analog.com you wrote:
On some architecture, it is not save to call the first line of kernel without disable the instruction cache. For do_bootm() path,
architecture
specific library code do_bootm_linux() can adress this issue, but do_bootelf() is common code. This patch disables icache before call
kernel.
Signed-off-by: Sonic Zhang sonic.zhang@gmail.com
Did you test the impact of this change on all other architectures but the one you are talking about (without ever mentioning it)?
I don't think we want to do this in general. Please make this code conditional.
I think it is reasonable/prudent to turn off cache in all situations, on all architectures.
As an application developer, I don't want an early TLB miss on any architecture to cause a vector back to U-Boot's cache handler (which may not be there anymore). I would expect to have to write my own, and set it up myself if I want to run with cache on.
Instruction and data caches in U-Boot are a great thing, since it decreases the time for a kernel or file system to be loaded, decompressed, but once U-Boot looses control (the jump to the unknown application), it shouldn't expect a return, or the hardware not to be changed by the app.
go, bootm, boote, etc - should flush & turn off cache before the jump to the application code (should also turn off what limited interrupts are enabled) to be safe.
It could be a fair comment to tell people to add "dcache off; icache off; boote" to their scripts, but this slows down boot, since you want the cache on while you are doing the elf relocations. What you might want is a "relocate_elf; dcache off; icache off; go $(elf_entry)" which seems worse than just forcing it off in all situations. ?
-Robin
In message 200708081147.09111.rgetz@blackfin.uclinux.org you wrote:
I think it is reasonable/prudent to turn off cache in all situations, on all architectures.
No, it is not. Some architectures / operating systems may require cache(s) being on. Some others may not care, except for boot speed which in turn is something some of the customers do care about.
Finally, I don't want to fix a prblem where non exists.
If we know for sure that certain systems require caches disabeled, then let's provide them this environment; but please leave all the rest unchanged.
Instruction and data caches in U-Boot are a great thing, since it decreases the time for a kernel or file system to be loaded, decompressed, but once U-Boot looses control (the jump to the unknown application), it shouldn't expect a return, or the hardware not to be changed by the app.
There is more than just Linux...
go, bootm, boote, etc - should flush & turn off cache before the jump to the application code (should also turn off what limited interrupts are enabled) to be safe.
I disagree.
Best regards,
Wolfgang Denk
On Wednesday 08 August 2007, Wolfgang Denk wrote:
In message 200708081147.09111.rgetz@blackfin.uclinux.org you wrote:
I think it is reasonable/prudent to turn off cache in all situations, on all architectures.
No, it is not. Some architectures / operating systems may require cache(s) being on. Some others may not care, except for boot speed which in turn is something some of the customers do care about.
Finally, I don't want to fix a prblem where non exists.
I'm not totally sure here, but I remember seeing a problem with booting VxWorks which could be related to having the icache still on. Or was it QNX/Neutrino? Not sure anymore, too long ago. From my memory, all those other OS'es I ever worked with, are much "happier" with interrupts and caches disabled upon startup. It could be that we just forgot to disable the icache in the initial port of the bootelf command.
Perhaps some users of those other OS'es (VxWorks, QNX...) could speak up and suggest which implementation is recommended.
If we know for sure that certain systems require caches disabeled, then let's provide them this environment; but please leave all the rest unchanged.
Instruction and data caches in U-Boot are a great thing, since it decreases the time for a kernel or file system to be loaded, decompressed, but once U-Boot looses control (the jump to the unknown application), it shouldn't expect a return, or the hardware not to be changed by the app.
There is more than just Linux...
Yes. But the same reasons could apply to the other systems too. From my understanding we are "on the safe side" with caches disabled.
go, bootm, boote, etc - should flush & turn off cache before the jump to the application code (should also turn off what limited interrupts are enabled) to be safe.
I disagree.
I tend to agree with Robin here.
Best regards, Stefan
===================================================================== DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de =====================================================================
Hi Wolfgang Denk,
Thank you for your comment. Yes, this may not be necessary for all arch. How about add a hook function for each arch?
I find the icache is also disabled before call a binary in common code do_go(). See following snip. Should these lines be conditional?
Thanks
Sonic
int do_go (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[]) { ulong addr, rc; int rcode = 0;
if (argc < 2) { printf ("Usage:\n%s\n", cmdtp->usage); return 1; }
addr = simple_strtoul(argv[1], NULL, 16); printf ("## Starting application at 0x%08lX ...\n", addr);
if (icache_status()){ icache_disable(); } if (dcache_status()) { dcache_disable(); }
...
In message 1186558396.17519.11.camel@sevens.analog.com you wrote:
Thank you for your comment. Yes, this may not be necessary for all arch. How about add a hook function for each arch?
Using "weak", right.
I find the icache is also disabled before call a binary in common code do_go(). See following snip. Should these lines be conditional?
Ummm...
int do_go (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[]) { ulong addr, rc; int rcode = 0;
if (argc < 2) { printf ("Usage:\n%s\n", cmdtp->usage); return 1; } addr = simple_strtoul(argv[1], NULL, 16); printf ("## Starting application at 0x%08lX ...\n", addr); if (icache_status()){ icache_disable(); } if (dcache_status()) { dcache_disable(); }
This is not in the official tree, and it will not make it there. It will probably not even build for many boards.
Best regards,
Wolfgang Denk
On Wednesday 08 August 2007, Wolfgang Denk wrote:
In message 1186558396.17519.11.camel@sevens.analog.com you wrote:
Thank you for your comment. Yes, this may not be necessary for all arch. How about add a hook function for each arch?
Using "weak", right.
so using weak hooks is OK now ? i think it'd be good to migrate all of the ugly boote/bootm/etc... cruft to external weaks and let arches define their own
int do_go (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[]) { ulong addr, rc; int rcode = 0;
if (argc < 2) { printf ("Usage:\n%s\n", cmdtp->usage); return 1; } addr = simple_strtoul(argv[1], NULL, 16); printf ("## Starting application at 0x%08lX ...\n", addr); if (icache_status()){ icache_disable(); } if (dcache_status()) { dcache_disable(); }This is not in the official tree, and it will not make it there. It will probably not even build for many boards.
it's called pseudo code ... of course code that has "..." in it wont build :p -mike
Dear Mike,
in message 200708131052.44472.vapier@gentoo.org you wrote:
so using weak hooks is OK now ? i think it'd be good to migrate all of the
It always has been OK - just nobody bothered to use it. [And I didn't even know about it when I started working on PPCBoot.]
ugly boote/bootm/etc... cruft to external weaks and let arches define their own
Yes, a few #ifdef's can be eliminated that way.
int do_go (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[]) { ulong addr, rc; int rcode = 0;
if (argc < 2) { printf ("Usage:\n%s\n", cmdtp->usage); return 1; } addr = simple_strtoul(argv[1], NULL, 16); printf ("## Starting application at 0x%08lX ...\n", addr); if (icache_status()){ icache_disable(); } if (dcache_status()) { dcache_disable(); }This is not in the official tree, and it will not make it there. It will probably not even build for many boards.
it's called pseudo code ... of course code that has "..." in it wont build > :p
The only "..." I see was in the printf() format string, where it doesn't hurt compilation.
Best regards,
Wolfgang Denk
On Monday 13 August 2007, Wolfgang Denk wrote:
in message 200708131052.44472.vapier@gentoo.org you wrote:
so using weak hooks is OK now ? i think it'd be good to migrate all of the
It always has been OK - just nobody bothered to use it. [And I didn't even know about it when I started working on PPCBoot.]
the word on the street was weaks were banned due to old busted compilers ... but if this is not the case :)
ugly boote/bootm/etc... cruft to external weaks and let arches define their own
Yes, a few #ifdef's can be eliminated that way.
ok, we'll look at doing that, thanks ! -mike
On Monday 13 August 2007, Wolfgang Denk wrote:
in message 200708131052.44472.vapier@gentoo.org you wrote:
so using weak hooks is OK now ? i think it'd be good to migrate all of the
It always has been OK - just nobody bothered to use it. [And I didn't even know about it when I started working on PPCBoot.]
ugly boote/bootm/etc... cruft to external weaks and let arches define their own
Yes, a few #ifdef's can be eliminated that way.
unfortunately, using weak symbols and overriding elsewhere doesnt look like it's possible currently due to the way ld searches archives. for example, if i do something like: common/cmd_elf.c: __attribute__((weak)) do_bootelf_setup() { ... current code ... } do_bootelf() { ... do_bootelf_setup(); ...
and then i want to override this with a Blackfin version: lib_blackfin/bootelf_setup.c: do_bootelf_setup() { ... }
but since the linking process looks like: ld ... --start-group ... \ ... lib_blackfin/libblackfin.a ... \ ... common/libcommon.a ... \ --end-group ... ld will pick the weak symbol provided by libcommon.a even though a strong symbol is also available in libblackfin.a :(
so our only realistic options are: - create another static archive in common/ for weak symbols and specify it just before libcommon.a - live with #ifdef's, but minimize the crappy situation by splitting out just the relevant code so the main bootelf function stays clean -mike
In message 200801282114.14393.vapier@gentoo.org you wrote:
unfortunately, using weak symbols and overriding elsewhere doesnt look like it's possible currently due to the way ld searches archives. for example, if
???
ld will pick the weak symbol provided by libcommon.a even though a strong symbol is also available in libblackfin.a :(
That should never happen. What is your toolchain?
Best regards,
Wolfgang Denk
On Tuesday 29 January 2008, Wolfgang Denk wrote:
In message 200801282114.14393.vapier@gentoo.org you wrote:
unfortunately, using weak symbols and overriding elsewhere doesnt look like it's possible currently due to the way ld searches archives. for example, if
???
ld will pick the weak symbol provided by libcommon.a even though a strong symbol is also available in libblackfin.a :(
That should never happen. What is your toolchain?
read the binutils mailing list. this is the expected behavior of ld.
http://sourceware.org/ml/binutils/2008-01/msg00301.html -mike
On Tue, Jan 29, 2008 at 07:12:10PM -0500, Mike Frysinger wrote:
On Tuesday 29 January 2008, Wolfgang Denk wrote:
In message 200801282114.14393.vapier@gentoo.org you wrote:
unfortunately, using weak symbols and overriding elsewhere doesnt look like it's possible currently due to the way ld searches archives. for example, if
???
ld will pick the weak symbol provided by libcommon.a even though a strong symbol is also available in libblackfin.a :(
That should never happen. What is your toolchain?
read the binutils mailing list. this is the expected behavior of ld.
Overriding weak with strong symbols works just fine, however in your case ld has no reason to even look at your override, since it already has a (weak) definition for do_bootelf_setup().
Thus the solution for your problem is to give ld a reason to pull in your do_bootelf_setup() definition. There are two ways:
- explicitly list the .o file on the linker command line, i.e. add it to $(OBJS) or $(PLATFORM_LIBS) instead of libblackfin.a
- put the do_bootelf_setup() definition in a .o file along with some other code you know will be pulled in, e.g. add it to lib_blackfin/cache.c instead of putting it in its own file
HTH, Johannes
On Wednesday 30 January 2008, Johannes Stezenbach wrote:
On Tue, Jan 29, 2008 at 07:12:10PM -0500, Mike Frysinger wrote:
On Tuesday 29 January 2008, Wolfgang Denk wrote:
In message 200801282114.14393.vapier@gentoo.org you wrote:
unfortunately, using weak symbols and overriding elsewhere doesnt look like it's possible currently due to the way ld searches archives. for example, if
???
ld will pick the weak symbol provided by libcommon.a even though a strong symbol is also available in libblackfin.a :(
That should never happen. What is your toolchain?
read the binutils mailing list. this is the expected behavior of ld.
Overriding weak with strong symbols works just fine, however in your case ld has no reason to even look at your override, since it already has a (weak) definition for do_bootelf_setup().
Thus the solution for your problem is to give ld a reason to pull in your do_bootelf_setup() definition. There are two ways:
explicitly list the .o file on the linker command line, i.e. add it to $(OBJS) or $(PLATFORM_LIBS) instead of libblackfin.a
put the do_bootelf_setup() definition in a .o file along with some other code you know will be pulled in, e.g. add it to lib_blackfin/cache.c instead of putting it in its own file
thanks ... i had created a dedicated .c file for it of only strong symbols which would override the weaks, but as you say, with no other compelling reason to look at the object, ld wasnt considering it -mike
This splits the dcache logic out of do_bootelf into a dedicated weak function called do_bootelf_exec. This way ports can control the behavior before executing an ELF image however they like.
Signed-off-by: Mike Frysinger vapier@gentoo.org --- diff --git a/common/cmd_elf.c b/common/cmd_elf.c index 2eb7453..5689e2c 100644 --- a/common/cmd_elf.c +++ b/common/cmd_elf.c @@ -30,6 +30,31 @@ DECLARE_GLOBAL_DATA_PTR; int valid_elf_image (unsigned long addr); unsigned long load_elf_image (unsigned long addr);
+__attribute__((weak)) +unsigned long do_bootelf_exec (ulong (*entry)(int, char *[]), int argc, char *argv[]) +{ + unsigned long ret; + + /* + * QNX images require the data cache is disabled. + * Data cache is already flushed, so just turn it off. + */ + int dcache = dcache_status (); + if (dcache) + dcache_disable (); + + /* + * pass address parameter as argv[0] (aka command name), + * and all remaining args + */ + ret = entry (argc, argv); + + if (dcache) + dcache_enable (); + + return ret; +} + /* ====================================================================== * Interpreter command to boot an arbitrary ELF image from memory. * ====================================================================== */ @@ -53,18 +78,7 @@ int do_bootelf (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
printf ("## Starting application at 0x%08lx ...\n", addr);
- /* - * QNX images require the data cache is disabled. - * Data cache is already flushed, so just turn it off. - */ - if (dcache_status ()) - dcache_disable (); - - /* - * pass address parameter as argv[0] (aka command name), - * and all remaining args - */ - rc = ((ulong (*)(int, char *[])) addr) (--argc, &argv[1]); + rc = do_bootelf_exec ((void *)addr, argc - 1, argv + 1); if (rc != 0) rcode = 1;
In message 200802011045.00200.vapier@gentoo.org you wrote:
This splits the dcache logic out of do_bootelf into a dedicated weak function called do_bootelf_exec. This way ports can control the behavior before executing an ELF image however they like.
Signed-off-by: Mike Frysinger vapier@gentoo.org
diff --git a/common/cmd_elf.c b/common/cmd_elf.c
This doesn't fit current code any moe. Please rebase and resubmit.
Best regards,
Wolfgang Denk
This splits the arch-specific logic out of do_go() and into a dedicated weak function called do_go_exec() that lives in cpu directories. This will need review from i386/nios people to make sure i didnt break them.
Signed-off-by: Mike Frysinger vapier@gentoo.org --- diff --git a/common/cmd_boot.c b/common/cmd_boot.c index e68f16f..82a5710 100644 --- a/common/cmd_boot.c +++ b/common/cmd_boot.c @@ -28,9 +28,11 @@ #include <command.h> #include <net.h>
-#if defined(CONFIG_I386) -DECLARE_GLOBAL_DATA_PTR; -#endif +__attribute__((weak)) +unsigned long do_go_exec (ulong (*entry)(int, char *[]), int argc, char *argv[]) +{ + return entry (argc, argv); +}
int do_go (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[]) { @@ -50,21 +52,7 @@ int do_go (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[]) * pass address parameter as argv[0] (aka command name), * and all remaining args */ -#if defined(CONFIG_I386) - /* - * x86 does not use a dedicated register to pass the pointer - * to the global_data - */ - argv[0] = (char *)gd; -#endif -#if !defined(CONFIG_NIOS) - rc = ((ulong (*)(int, char *[]))addr) (--argc, &argv[1]); -#else - /* - * Nios function pointers are address >> 1 - */ - rc = ((ulong (*)(int, char *[]))(addr>>1)) (--argc, &argv[1]); -#endif + rc = do_go_exec ((void *)addr, argv - 1, argv + 1); if (rc != 0) rcode = 1;
printf ("## Application terminated, rc = 0x%lX\n", rc); diff --git a/lib_i386/board.c b/lib_i386/board.c index 47fbab4..22191e6 100644 --- a/lib_i386/board.c +++ b/lib_i386/board.c @@ -421,3 +421,11 @@ void hang (void) puts ("### ERROR ### Please RESET the board ###\n"); for (;;); } + +unsigned long do_go_exec (ulong (*entry)(int, char *[]), int argc, char *argv[]) +{ + /* + * Nios function pointers are address >> 1 + */ + return (entry >> 1) (argc, argv); +} diff --git a/lib_nios/board.c b/lib_nios/board.c index 0a0d2e3..cdaf753 100644 --- a/lib_nios/board.c +++ b/lib_nios/board.c @@ -190,3 +190,13 @@ void hang (void) puts("### ERROR ### Please reset board ###\n"); for (;;); } + +unsigned long do_go_exec (ulong (*entry)(int, char *[]), int argc, char *argv[]) +{ + /* + * x86 does not use a dedicated register to pass the pointer + * to the global_data + */ + argv[-1] = (char *)gd; + return entry (argc, argv); +}
blah, and without fail, i swapped nios/i386 --- This splits the arch-specific logic out of do_go() and into a dedicated weak function called do_go_exec() that lives in cpu directories. This will need review from i386/nios people to make sure i didnt break them.
Signed-off-by: Mike Frysinger vapier@gentoo.org --- diff --git a/common/cmd_boot.c b/common/cmd_boot.c index e68f16f..82a5710 100644 --- a/common/cmd_boot.c +++ b/common/cmd_boot.c @@ -28,9 +28,11 @@ #include <command.h> #include <net.h>
-#if defined(CONFIG_I386) -DECLARE_GLOBAL_DATA_PTR; -#endif +__attribute__((weak)) +unsigned long do_go_exec (ulong (*entry)(int, char *[]), int argc, char *argv[]) +{ + return entry (argc, argv); +}
int do_go (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[]) { @@ -50,21 +52,7 @@ int do_go (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[]) * pass address parameter as argv[0] (aka command name), * and all remaining args */ -#if defined(CONFIG_I386) - /* - * x86 does not use a dedicated register to pass the pointer - * to the global_data - */ - argv[0] = (char *)gd; -#endif -#if !defined(CONFIG_NIOS) - rc = ((ulong (*)(int, char *[]))addr) (--argc, &argv[1]); -#else - /* - * Nios function pointers are address >> 1 - */ - rc = ((ulong (*)(int, char *[]))(addr>>1)) (--argc, &argv[1]); -#endif + rc = do_go_exec ((void *)addr, argv - 1, argv + 1); if (rc != 0) rcode = 1;
printf ("## Application terminated, rc = 0x%lX\n", rc); diff --git a/lib_i386/board.c b/lib_i386/board.c index 47fbab4..36ab6a8 100644 --- a/lib_i386/board.c +++ b/lib_i386/board.c @@ -421,3 +421,13 @@ void hang (void) puts ("### ERROR ### Please RESET the board ###\n"); for (;;); } + +unsigned long do_go_exec (ulong (*entry)(int, char *[]), int argc, char *argv[]) +{ + /* + * x86 does not use a dedicated register to pass the pointer + * to the global_data + */ + argv[-1] = (char *)gd; + return entry (argc, argv); +} diff --git a/lib_nios/board.c b/lib_nios/board.c index 0a0d2e3..3a05e83 100644 --- a/lib_nios/board.c +++ b/lib_nios/board.c @@ -190,3 +190,11 @@ void hang (void) puts("### ERROR ### Please reset board ###\n"); for (;;); } + +unsigned long do_go_exec (ulong (*entry)(int, char *[]), int argc, char *argv[]) +{ + /* + * Nios function pointers are address >> 1 + */ + return (entry >> 1) (argc, argv); +}
In message 200802011136.55390.vapier@gentoo.org you wrote:
blah, and without fail, i swapped nios/i386
This splits the arch-specific logic out of do_go() and into a dedicated weak function called do_go_exec() that lives in cpu directories. This will need review from i386/nios people to make sure i didnt break them.
Signed-off-by: Mike Frysinger vapier@gentoo.org
diff --git a/common/cmd_boot.c b/common/cmd_boot.c
This doesn't fit current code any more. Please rebase and resubmit.
Please make sure to use only ONE "---" line, and place comments that are supposed NOT to go into the commit message BELOW that line; you got this swapped here.
Best regards,
Wolfgang Denk
In message 200802011058.34891.vapier@gentoo.org you wrote:
This splits the arch-specific logic out of do_go() and into a dedicated weak function called do_go_exec() that lives in cpu directories. This will need review from i386/nios people to make sure i didnt break them.
Signed-off-by: Mike Frysinger vapier@gentoo.org
This doesn't fit current code any more. Please rebase and resubmit.
Best regards,
Wolfgang Denk
participants (7)
-
Johannes Stezenbach -
Michal Simek -
Mike Frysinger -
Robin Getz -
Sonic Zhang -
Stefan Roese -
Wolfgang Denk