[U-Boot-Users] [PATCH] 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 afleming@freescale.com --- common/cmd_bootm.c | 24 +++++++++++++++--------- 1 files changed, 15 insertions(+), 9 deletions(-)
diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c index 2436581..580a9f0 100644 --- a/common/cmd_bootm.c +++ b/common/cmd_bootm.c @@ -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);
Dear Andy,
in message 11870460962397-git-send-email-afleming@freescale.com 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:
flag the blob for relocation whenever its address is above BOOTMAPSZ
If the blob is being relocated, relocate it before kbd, not initrd
Signed-off-by: Andy Fleming afleming@freescale.com
NAK.
I'm afraid I have to reject this patch.
diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c index 2436581..580a9f0 100644 --- a/common/cmd_bootm.c +++ b/common/cmd_bootm.c @@ -21,6 +21,7 @@
- MA 02111-1307 USA
*/
+#define DEBUG
First, please don't enable DEBUG like this in common files.
+#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;elseof_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
Wolfgang Denk wrote:
Dear Andy,
in message 11870460962397-git-send-email-afleming@freescale.com 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:
flag the blob for relocation whenever its address is above BOOTMAPSZ
If the blob is being relocated, relocate it before kbd, not initrd
Signed-off-by: Andy Fleming afleming@freescale.com
NAK.
I'm afraid I have to reject this patch.
diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c index 2436581..580a9f0 100644 --- a/common/cmd_bootm.c +++ b/common/cmd_bootm.c @@ -21,6 +21,7 @@
- MA 02111-1307 USA
*/
+#define DEBUG
First, please don't enable DEBUG like this in common files.
Agree with this comment.
+#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]
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?
Best regards,
Wolfgang Denk
Best regards, gvb
In message 46C11C06.8060106@gmail.com you wrote:
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?
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
diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c index 2436581..580a9f0 100644 --- a/common/cmd_bootm.c +++ b/common/cmd_bootm.c @@ -21,6 +21,7 @@
- MA 02111-1307 USA
*/
+#define DEBUG
Oops. That was a stupid oversight.
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;elseof_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.
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
In message 2acbd3e40708140000g1acdf9e3mf3d7f4f8c2788d1@mail.gmail.com you wrote:
+#define DEBUG
Oops. That was a stupid oversight.
OK. Please resubmit without this, then.
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.
Agreed. So plese resubmit without the DEBUG thingy, and we'll discuss and fix the other thing next time. OK?
Best regards,
Wolfgang Denk
participants (4)
-
Andy Fleming -
Andy Fleming -
Jerry Van Baren -
Wolfgang Denk