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.

Index: common/cmd_elf.c
===================================================================
@@ -65,6 +65,8 @@
 	 */
 	if (dcache_status ())
 		dcache_disable ();
+	if (icache_status ())
+		icache_disable ();
 
 	/* pass cmdline to the kernel. */
 	cmdline = make_command_line();

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

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
"Though a program be but three lines long,
someday it will have to be maintained."
- The Tao of Programming

Hi,

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:

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

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.

There is more than just Linux...

I disagree.

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
Clothes make the man. Naked people have little  or  no  influence  on
society.                                                 - Mark Twain

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.

Yes. But the same reasons could apply to the other systems too. From my understanding we are "on the safe side" with caches disabled.

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 at 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();
        }

...

Using "weak", right.

Ummm...

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

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
There are some things worth dying for.
	-- Kirk, "Errand of Mercy", stardate 3201.7

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

it's called pseudo code ... of course code that has "..." in it wont build :p -mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 827 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20070813/7d25b842/attachment.pgp

Dear Mike,

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

Yes, a few #ifdef's can be eliminated that way.

The only "..." I see was in the printf() format string, where it doesn't hurt compilation.

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
Swap read error.  You lose your mind.

the word on the street was weaks were banned due to old busted compilers ... but if this is not the case :)

ok, we'll look at doing that, thanks !
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 827 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20070813/4d5b4c35/attachment.pgp

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 827 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20080128/a3885df1/attachment.pgp

???

That should never happen. What is your toolchain?

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
As usual, this being a 1.3.x release, I haven't  even  compiled  this
kernel yet. So if it works, you should be doubly impressed.
      - Linus Torvalds in <[email protected]>

read the binutils mailing list. this is the expected behavior of ld.

http://sourceware.org/ml/binutils/2008-01/msg00301.html
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 827 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20080129/961d4384/attachment.pgp

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

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 827 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20080201/909b988b/attachment.pgp

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.

---

index 2eb7453..5689e2c 100644
@@ -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;
 

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.

---

index e68f16f..82a5710 100644
@@ -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);

index 47fbab4..22191e6 100644
@@ -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);
+}

index 0a0d2e3..cdaf753 100644
@@ -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.

---

index e68f16f..82a5710 100644
@@ -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);

index 47fbab4..36ab6a8 100644
@@ -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);
+}

index 0a0d2e3..3a05e83 100644
@@ -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);
+}

This doesn't fit current code any more. Please rebase and resubmit.

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
Even if you aren't in doubt, consider the mental welfare of the  per-
son who has to maintain the code after you, and who will probably put
parens in the wrong place.          - Larry Wall in the perl man page