[U-Boot-Users] [PATCH] Fix initrd/dtb interaction

[U-Boot-Users] [PATCH] Fix initrd/dtb interaction

[Andy Fleming]

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);
-- 
1.5.0.2.230.gfbe3d-dirty

[U-Boot-Users] [PATCH] Fix initrd/dtb interaction

[Wolfgang Denk]

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:
> 
> 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>

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;
> -		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

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

[U-Boot-Users] [PATCH] Fix initrd/dtb interaction

[Jerry Van Baren]

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:
>>
>> 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>
> 
> 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

[U-Boot-Users] [PATCH] Fix initrd/dtb interaction

[Andy Fleming]

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


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

[U-Boot-Users] [PATCH] Fix initrd/dtb interaction

[Wolfgang Denk]

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

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

[U-Boot-Users] [PATCH] Fix initrd/dtb interaction

[Wolfgang Denk]

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

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