Fix initrd/dtb interaction

PATCH Superseded Andy Fleming 4 participants 6 messages Aug 13, 2007 – Aug 14, 2007

Share & tools

Andy Fleming

author

2007-08-13 23:01 UTC #0 lore raw

PATCH 1/1 Fix initrd/dtb interaction

The original code would wrongly relocate the blob to be right before the initrd if it existed. The blob *must* be within CFG_BOOTMAPSZ, if it is defined. So we make two changes:

1) flag the blob for relocation whenever its address is above BOOTMAPSZ

2) If the blob is being relocated, relocate it before kbd, not initrd

Signed-off-by Andy Fleming <[email protected]>

---
 common/cmd_bootm.c |   24 +++++++++++++++---------
 1 files changed, 15 insertions(+), 9 deletions(-)

common/cmd_bootm.c +15 -9

index 2436581..580a9f0 100644
@@ -21,6 +21,7 @@
  * MA 02111-1307 USA
  */
 
+#define DEBUG
 /*
  * Boot support
  */

@@ -925,6 +926,15 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag,
 	unlock_ram_in_cache();
 #endif
 
+#ifdef CFG_BOOTMAPSZ
+	/*
+	 * The blob must be within CFG_BOOTMAPSZ,
+	 * so we flag it to be copied if it is
+	 */
+	if (of_flat_tree >= (char *)CFG_BOOTMAPSZ)
+		of_data = of_flat_tree;
+#endif
+
 #if defined(CONFIG_OF_LIBFDT)
 	/* move of_flat_tree if needed */
 	if (of_data) {

@@ -932,11 +942,9 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag,
 		ulong of_start, of_len;
 
 		of_len = be32_to_cpu(fdt_totalsize(of_data));
-		/* position on a 4K boundary before the initrd/kbd */
-		if (initrd_start)
-			of_start = initrd_start - of_len;
-		else
-			of_start  = (ulong)kbd - of_len;
+
+		/* position on a 4K boundary before the kbd */
+		of_start  = (ulong)kbd - of_len;
 		of_start &= ~(4096 - 1);	/* align on page */
 		debug ("## device tree at 0x%08lX ... 0x%08lX (len=%ld=0x%lX)\n",
 			of_data, of_data + of_len - 1, of_len, of_len);

@@ -975,11 +983,9 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag,
 	if (of_data) {
 		ulong of_start, of_len;
 		of_len = ((struct boot_param_header *)of_data)->totalsize;
+
 		/* provide extra 8k pad */
-		if (initrd_start)
-			of_start = initrd_start - of_len - 8192;
-		else
-			of_start  = (ulong)kbd - of_len - 8192;
+		of_start  = (ulong)kbd - of_len - 8192;
 		of_start &= ~(4096 - 1);	/* align on page */
 		debug ("## device tree at 0x%08lX ... 0x%08lX (len=%ld=0x%lX)\n",
 			of_data, of_data + of_len - 1, of_len, of_len);

Signature · 1 line

1.5.0.2.230.gfbe3d-dirty

Wolfgang Denk

NAK

2007-08-14 00:19 UTC #1 lore raw

PATCH 1/1 Fix initrd/dtb interaction

Dear Andy,

Quoted 9 lines · in message <[email protected]> you wrote:

The original code would wrongly relocate the blob to be right before the initrd if it existed. The blob *must* be within CFG_BOOTMAPSZ, if it is defined. So we make two changes:
1) flag the blob for relocation whenever its address is above BOOTMAPSZ
2) If the blob is being relocated, relocate it before kbd, not initrd
Signed-off-by Andy Fleming <[email protected]>

NAK.

I'm afraid I have to reject this patch.

common/cmd_bootm.c line 24

Quoted 9 lines

common/cmd_bootm.c +1 -0

index 2436581..580a9f0 100644
@@ -21,6 +21,7 @@
  * MA 02111-1307 USA
  */
 
+#define DEBUG

First, please don't enable DEBUG like this in common files.

Quoted 14 lines

common/cmd_bootm.c +9 -0

+#ifdef CFG_BOOTMAPSZ
+	/*
+	 * The blob must be within CFG_BOOTMAPSZ,
+	 * so we flag it to be copied if it is
+	 */
+	if (of_flat_tree >= (char *)CFG_BOOTMAPSZ)
+		of_data = of_flat_tree;
+#endif
+
 #if defined(CONFIG_OF_LIBFDT)
 	/* move of_flat_tree if needed */
 	if (of_data) {

@@ -932,11 +942,9 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag,
 		ulong of_start, of_len;

common/cmd_bootm.c do_bootm_linux (cmd_tbl_t *cmdtp, int flag, · line 947

Quoted 10 lines

common/cmd_bootm.c +3 -5

 
 		of_len = be32_to_cpu(fdt_totalsize(of_data));
-		/* position on a 4K boundary before the initrd/kbd */
-		if (initrd_start)
-			of_start = initrd_start - of_len;
-		else
-			of_start  = (ulong)kbd - of_len;
+
+		/* position on a 4K boundary before the kbd */
+		of_start  = (ulong)kbd - of_len;

Second, I asked you before to implement this similar like the initrd location can be controlled using the "initrd_high" environment variable (see my message Tue, 07 Aug 2007 21:21:19 +0200). AFAICT you never replied to this.

Best regards,

Wolfgang Denk

Signature · 4 lines

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
A fail-safe circuit will destroy others.                 -- Klipstein

Jerry Van Baren

2007-08-14 03:05 UTC #2 lore raw

PATCH 1/1 Fix initrd/dtb interaction

Quoted 27 lines · Wolfgang Denk

Dear Andy,
Quoted 9 lines · in message <[email protected]> you wrote:
The original code would wrongly relocate the blob to be right before the initrd if it existed. The blob *must* be within CFG_BOOTMAPSZ, if it is defined. So we make two changes:
1) flag the blob for relocation whenever its address is above BOOTMAPSZ
2) If the blob is being relocated, relocate it before kbd, not initrd
Signed-off-by Andy Fleming <[email protected]>
NAK.
I'm afraid I have to reject this patch.
Quoted 8 lines
common/cmd_bootm.c +0 -0
index 2436581..580a9f0 100644
@@ -21,6 +21,7 @@
  * MA 02111-1307 USA
  */
 

Quoted 3 lines

common/cmd_bootm.c line 24
Quoted 1 line
common/cmd_bootm.c +1 -0
+#define DEBUG
First, please don't enable DEBUG like this in common files.

Agree with this comment.

Quoted 9 lines

common/cmd_bootm.c +9 -0

+#ifdef CFG_BOOTMAPSZ
+	/*
+	 * The blob must be within CFG_BOOTMAPSZ,
+	 * so we flag it to be copied if it is
+	 */
+	if (of_flat_tree >= (char *)CFG_BOOTMAPSZ)
+		of_data = of_flat_tree;
+#endif
+

[snip]

Quoted 5 lines

Second, I asked you before to implement this similar like the initrd location can be controlled using the "initrd_high" environment variable (see my message Tue, 07 Aug 2007 21:21:19 +0200). AFAICT you never replied to this.

I believe Andy got this from me. I keyed off his statement that "The blob must be within CFG_BOOTMAPSZ..." so I suggested he compare of_flat_tree to CFG_BOOTMAPSZ, since that was the criteria that he stated.

<http://comments.gmane.org/gmane.comp.boot-loaders.u-boot/30616>

I'm ignorant of the particulars, but I assumed from the above referenced discussion/description that CFG_BOOTMAPSZ is a better criteria than initrd_high. True? False?

Quoted 3 lines

Best regards,
Wolfgang Denk

Best regards,
gvb

Wolfgang Denk

2007-08-14 07:06 UTC #3 lore raw

PATCH 1/1 Fix initrd/dtb interaction

Quoted 10 lines · In message <[email protected]> you wrote:

Quoted 5 lines
Second, I asked you before to implement this similar like the initrd location can be controlled using the "initrd_high" environment variable (see my message Tue, 07 Aug 2007 21:21:19 +0200). AFAICT you never replied to this.
I believe Andy got this from me. I keyed off his statement that "The blob must be within CFG_BOOTMAPSZ..." so I suggested he compare of_flat_tree to CFG_BOOTMAPSZ, since that was the criteria that he stated.

Quoted 5 lines

<http://comments.gmane.org/gmane.comp.boot-loaders.u-boot/30616>
I'm ignorant of the particulars, but I assumed from the above referenced discussion/description that CFG_BOOTMAPSZ is a better criteria than initrd_high. True? False?

I don't think it's better. CFG_BOOTMAPSZ is a hard coded limit. initrd_high (or fdt_high or whatever you will call it) allows you to use the same value as default, but overwrite it on systems which don't need it or have additional restrictions, i. e. it giveds you added flexibility. This is IMHO a Good Thing (TM).

Best regards,

Wolfgang Denk

Signature · 6 lines

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
If a man had a child who'd gone  anti-social,  killed  perhaps,  he'd
still tend to protect that child.
	-- McCoy, "The Ultimate Computer", stardate 4731.3

Andy Fleming

2007-08-14 07:00 UTC #4 lore raw

PATCH 1/1 Fix initrd/dtb interaction

In reply to Wolfgang Denk

Quoted 9 lines

common/cmd_bootm.c line 24
Quoted 9 lines
common/cmd_bootm.c +1 -0
index 2436581..580a9f0 100644
@@ -21,6 +21,7 @@
  * MA 02111-1307 USA
  */
 
+#define DEBUG

Oops. That was a stupid oversight.

Quoted 10 lines

common/cmd_bootm.c do_bootm_linux (cmd_tbl_t *cmdtp, int flag, · line 947

Quoted 9 lines

common/cmd_bootm.c +3 -5

              of_len = be32_to_cpu(fdt_totalsize(of_data));
-             /* position on a 4K boundary before the initrd/kbd */
-             if (initrd_start)
-                     of_start = initrd_start - of_len;
-             else
-                     of_start  = (ulong)kbd - of_len;
+
+             /* position on a 4K boundary before the kbd */
+             of_start  = (ulong)kbd - of_len;

Quoted 4 lines

Second, I asked you before to implement this similar like the initrd location can be controlled using the "initrd_high" environment variable (see my message Tue, 07 Aug 2007 21:21:19 +0200). AFAICT you never replied to this.

Sorry, I ended up replying more to gvb's comment. The initrd can go wherever it wants. The address of the ramdisk is contained in the blob, and the kernel will map memory using the blob. But the blob has to be accessed before the kernel maps that memory.

So we can add an environment variable to override the 16M limit for u-boot, but if anyone sets it above 16M (on 85xx), it'll screw things up. I don't have a fundamental issue with such a variable being added, but I don't think I have time to figure it out this week, and I don't think it solves the problem (though it would allow users to solve it).

In other words, I think we need both. BOOTMAPSZ is clearly meant to serve as a ceiling for things like the blob (the bd_t, for instance, is always put in that range if it exists). The environment variable would give everyone more flexibility. Sadly, though, I only have time to fix the obvious problem (leaving DEBUG defined). I'd be happy to implement the environment variable for the *next* merge window, but I'd really hate to let this bug sit in u-boot for another release.

Andy

Wolfgang Denk

2007-08-14 07:50 UTC #5 lore raw

PATCH 1/1 Fix initrd/dtb interaction

Quoted 4 lines · In message <[email protected]> you…

Quoted 2 lines
Quoted 2 lines
+#define DEBUG
Oops. That was a stupid oversight.

OK. Please resubmit without this, then.

Quoted 7 lines

So we can add an environment variable to override the 16M limit for u-boot, but if anyone sets it above 16M (on 85xx), it'll screw things up. I don't have a fundamental issue with such a variable being added, but I don't think I have time to figure it out this week, and I don't think it solves the problem (though it would allow users to solve it).

Quoted 7 lines

In other words, I think we need both. BOOTMAPSZ is clearly meant to serve as a ceiling for things like the blob (the bd_t, for instance, is always put in that range if it exists). The environment variable would give everyone more flexibility. Sadly, though, I only have time to fix the obvious problem (leaving DEBUG defined). I'd be happy to implement the environment variable for the *next* merge window, but I'd really hate to let this bug sit in u-boot for another release.

Agreed. So plese resubmit without the DEBUG thingy, and we'll discuss and fix the other thing next time. OK?

Best regards,

Wolfgang Denk

Signature · 5 lines

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
"If a computer can't directly address all the RAM you can  use,  it's
just a toy."         - anonymous comp.sys.amiga posting, non-sequitir