[U-Boot] image: Don't relocate ramdisk to highmem

From: Thierry Reding <treding@nvidia.com>

The Linux kernel cannot unpack a ramdisk that's stored in high memory.
Unless the initrd_high environment variable is explicitly set, abide by
that restriction using the getenv_bootm_low() and getenv_bootm_mapsize()
helpers.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 common/image.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Dear Thierry Reding,

In message <1373500071-6476-1-git-send-email-thierry.reding@gmail.com> you wrote:
> 
> The Linux kernel cannot unpack a ramdisk that's stored in high memory.

Can you please be a bit more explicit?  Which exact architecture are
you talking about?  Can you show a specific example of an error case
for such a system?

> -		/* not set, no restrictions to load high */
> -		initrd_high = ~0;
> +		/* make sure to put ramdisk in low memory */
> +		initrd_high = getenv_bootm_low() + getenv_bootm_mapsize();

I don't like to have such a patch (which affects ALL systems) added
unless we understand exactly why we should do that. 

Why do you not simply set initrd_high as needed?

Also, when changing the behaviour, you should also update the
comments.

Best regards,

Wolfgang Denk

On Thu, Jul 11, 2013 at 11:46:19AM +0200, Wolfgang Denk wrote:
> Dear Thierry Reding,
> 
> In message <1373500071-6476-1-git-send-email-thierry.reding@gmail.com> you wrote:
> > 
> > The Linux kernel cannot unpack a ramdisk that's stored in high memory.
> 
> Can you please be a bit more explicit?  Which exact architecture are
> you talking about?  Can you show a specific example of an error case
> for such a system?

I'm pretty sure it's all architectures, and this is a problem for device
trees as well.  The tricks done to deal with highmem mean it's not
suitable for certain tasks, if I recall things right (it's been a
while).

> > -		/* not set, no restrictions to load high */
> > -		initrd_high = ~0;
> > +		/* make sure to put ramdisk in low memory */
> > +		initrd_high = getenv_bootm_low() + getenv_bootm_mapsize();
> 
> I don't like to have such a patch (which affects ALL systems) added
> unless we understand exactly why we should do that. 
> 
> Why do you not simply set initrd_high as needed?

The problem is that setting initrd_high (and fdt_high) becomes one of
those things you have to on every new port, and you start to go "why do
I have to do this again?".  In fact, a related problem is that folks set
initrd_high/fdt_high to 0xffffffff to just stop relocation (which may or
may not be a time saver too) after hitting the sequence of:
- Loaded ramdisk somewhere "good" in memory
- Tried booting the kernel, ramdisk not found
- Spend non-zero time cursing and debugging why the ramdisk wasn't seen
- Realize that U-Boot shoves it up to the top of memory and the kernel
  doesn't like that.
- Curse U-Boot for moving things around on you.

> Also, when changing the behaviour, you should also update the
> comments.

Agreed.

> 
> 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@denx.de
> The trouble with our times is that the future is not what it used  to
> be.                                                     - Paul Valery
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

On Thu, Jul 11, 2013 at 08:39:28AM -0400, Tom Rini wrote:
> On Thu, Jul 11, 2013 at 11:46:19AM +0200, Wolfgang Denk wrote:
> > Dear Thierry Reding,
> > 
> > In message <1373500071-6476-1-git-send-email-thierry.reding@gmail.com> you wrote:
> > > 
> > > The Linux kernel cannot unpack a ramdisk that's stored in high memory.
> > 
> > Can you please be a bit more explicit?  Which exact architecture are
> > you talking about?  Can you show a specific example of an error case
> > for such a system?
> 
> I'm pretty sure it's all architectures, and this is a problem for device
> trees as well.  The tricks done to deal with highmem mean it's not
> suitable for certain tasks, if I recall things right (it's been a
> while).

Yes, that's my understanding as well. The same changes were done for
fdt_high a few months back and ramdisks aren't any different in this
respect. I was a bit surprised that this hadn't been fixed yet, but
maybe people just aren't using ramdisks anymore these days, or they
worked around it by setting initrd_high explicitly.

> > > -		/* not set, no restrictions to load high */
> > > -		initrd_high = ~0;
> > > +		/* make sure to put ramdisk in low memory */
> > > +		initrd_high = getenv_bootm_low() + getenv_bootm_mapsize();
> > 
> > I don't like to have such a patch (which affects ALL systems) added
> > unless we understand exactly why we should do that. 
> > 
> > Why do you not simply set initrd_high as needed?
> 
> The problem is that setting initrd_high (and fdt_high) becomes one of
> those things you have to on every new port, and you start to go "why do
> I have to do this again?".

Relocating to the top of memory is probably the wrong thing to do for a
lot of boards these days. I know I've been using the old code without a
problem on a number of boards, but more recent hardware tends to come
equipped with 1 GiB or more RAM, which should always trigger this error.

> In fact, a related problem is that folks set initrd_high/fdt_high to
> 0xffffffff to just stop relocation

Yeah, I wondered whether maybe not relocating the ramdisk (and the FDT)
in the first place might even be a better default fallback. It makes the
boot process slightly more brittle because it can go equally wrong if
people load files to weird locations. It seems, though, that most boards
use sensible default locations that are somewhere below the first 256
MiB of RAM.

> (which may or may not be a time saver too) after hitting the sequence
> of:
> - Loaded ramdisk somewhere "good" in memory
> - Tried booting the kernel, ramdisk not found
> - Spend non-zero time cursing and debugging why the ramdisk wasn't seen
> - Realize that U-Boot shoves it up to the top of memory and the kernel
>   doesn't like that.
> - Curse U-Boot for moving things around on you.

That's *exactly* what happened to me, only that instead of just not
finding the ramdisk it would page fault and oops. The first thing I did
was indeed to just set initrd_high to 0xffffffff but then decided to try
and properly fix it in an attempt to save others the trouble.

> > Also, when changing the behaviour, you should also update the
> > comments.
> 
> Agreed.

I'm not sure which comments you are referring to. I updated the one
immediately above the changed code and the one above the function
doesn't contain anything relating to the default behaviour in case
initrd_high is unset.

Thierry

Dear Thierry Reding,

In message <20130711150014.GA2198@dhcp-172-17-186-34.nvidia.com> you wrote:
> 
> > I'm pretty sure it's all architectures, and this is a problem for device
> > trees as well.  The tricks done to deal with highmem mean it's not
> > suitable for certain tasks, if I recall things right (it's been a
> > while).
> 
> Yes, that's my understanding as well. The same changes were done for
> fdt_high a few months back and ramdisks aren't any different in this
> respect. I was a bit surprised that this hadn't been fixed yet, but
> maybe people just aren't using ramdisks anymore these days, or they
> worked around it by setting initrd_high explicitly.

This depends a lot on a number of things.  For example, you should be
able to use a ramdisk in NOR flash directly, i. e. without loading it
to RAM first - especially if it;s a comprtessed ramdis image, and the
Kernel will copy/uncompress it anyway.  Depending on your memory map
the address range of the NOR flash may be way outside (and above) that
of the system RAM.  Are you sure your changes will not break any such
usage?

> Yeah, I wondered whether maybe not relocating the ramdisk (and the FDT)
> in the first place might even be a better default fallback. It makes the

We want to avoid any copy of the ramdisk at all, if possible.

> That's *exactly* what happened to me, only that instead of just not
> finding the ramdisk it would page fault and oops. The first thing I did
> was indeed to just set initrd_high to 0xffffffff but then decided to try
> and properly fix it in an attempt to save others the trouble.

What is a fix for you may be a breakage elsewhere.

> > > Also, when changing the behaviour, you should also update the
> > > comments.
> >
> > Agreed.
> 
> I'm not sure which comments you are referring to. I updated the one
> immediately above the changed code and the one above the function
> doesn't contain anything relating to the default behaviour in case
> initrd_high is unset.

...because default behaviour was do do nothing. Now you do something,
so this should be documented.

Best regards,

Wolfgang Denk

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 07/11/2013 02:21 PM, Wolfgang Denk wrote:
> Dear Thierry Reding,
[snip]
>>>> Also, when changing the behaviour, you should also update
>>>> the comments.
>>> 
>>> Agreed.
>> 
>> I'm not sure which comments you are referring to. I updated the
>> one immediately above the changed code and the one above the
>> function doesn't contain anything relating to the default
>> behaviour in case initrd_high is unset.
> 
> ...because default behaviour was do do nothing. Now you do
> something, so this should be documented.

Wait, the default behaviour is to relocate things to the top of memory,
at least in the cases I've played with recently.  This changes things to
have a limit on how high up it can be placed.  A change, yes.  But not a
change from nothing.

- -- 
Tom
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJR3vl6AAoJENk4IS6UOR1W/mIQAIXWASuOgGa9+OHPEjC1oTAs
Be2bO8aC5j3Hw3dw2QwVUekiIWPzHi+bz+B8WYdnZHlqBXZPerhi+JOEog6zwb4+
o5CrzY6w21RA0ZjDNGQyjKNA02a5ZkAwSfamLURJeIjsAJmt2exZQJZQsG+reMTo
jphJbPMRQJOoLH9QoYUQWlb+6NrsqppsrTHQlyL5E1/+r2Wz2KVAru8AembRSCOW
a5kanhiuPDNCn2WPxCHFQ0u9YtaVW8+W7APw+szsszqZ+fWeUDmtSU4gXGZ9kJjt
LGHbwQDnTkS4fGAAU9mwuKhp/y5EhlZpOjvD1KJxQpxxdKGxO1xH7A7xWwb4wzee
iVu+p6PNS6ssXP1ha+ZjwrvfN3hyWJL/JOmAVrnhr+3iq5yTrggXWMTeou0mxtzV
+v5HfuKLqXqMIMSI++MAEPdvg7GTx/SlXzFlRbWyKC4v7guYl70g9GuPE6flnnVJ
4OPd4YWN0N1CDmddkwbBP9gIzbJxOoVwcW0WaRL1HE4W7wkrDjJFOjTP/ieydKH3
7Nuv+X1pXdRJv38A2Z7KuYzm7ewQdAZfh1n4vivcET0DH86doHwCDfestLTPsdjR
mIQbrXjZyU2jq7GV/dEUTUI/aeAeB/9Bog2+gc/0ne5cSvAKD4cM0Kwqp6cvknCx
utrwMmEazs+bpAkW5+CR
=rguU
-----END PGP SIGNATURE-----

On 07/11/2013 12:21 PM, Wolfgang Denk wrote:
> Dear Thierry Reding,
> 
> In message <20130711150014.GA2198@dhcp-172-17-186-34.nvidia.com> you wrote:
>>
>>> I'm pretty sure it's all architectures, and this is a problem for device
>>> trees as well.  The tricks done to deal with highmem mean it's not
>>> suitable for certain tasks, if I recall things right (it's been a
>>> while).
>>
>> Yes, that's my understanding as well. The same changes were done for
>> fdt_high a few months back and ramdisks aren't any different in this
>> respect. I was a bit surprised that this hadn't been fixed yet, but
>> maybe people just aren't using ramdisks anymore these days, or they
>> worked around it by setting initrd_high explicitly.
> 
> This depends a lot on a number of things.  For example, you should be
> able to use a ramdisk in NOR flash directly, i. e. without loading it
> to RAM first - especially if it;s a comprtessed ramdis image, and the
> Kernel will copy/uncompress it anyway.

That would be nice, but I don't believe the kernel supports that (at
least not on ARM):

arch/arm/mm/init.c:arm_memblock_init():

> #ifdef CONFIG_BLK_DEV_INITRD
> 	if (phys_initrd_size &&
> 	    !memblock_is_region_memory(phys_initrd_start, phys_initrd_size)) {
> 		pr_err("INITRD: 0x%08llx+0x%08lx is not a memory region - disabling initrd\n",
> 		       (u64)phys_initrd_start, phys_initrd_size);
> 		phys_initrd_start = phys_initrd_size = 0;
> 	}

(where "memory" in this context is RAM specifically, IIUC).

On Thu, Jul 11, 2013 at 08:21:17PM +0200, Wolfgang Denk wrote:
> Dear Thierry Reding,
> 
> In message <20130711150014.GA2198@dhcp-172-17-186-34.nvidia.com> you wrote:
> > 
> > > I'm pretty sure it's all architectures, and this is a problem for device
> > > trees as well.  The tricks done to deal with highmem mean it's not
> > > suitable for certain tasks, if I recall things right (it's been a
> > > while).
> > 
> > Yes, that's my understanding as well. The same changes were done for
> > fdt_high a few months back and ramdisks aren't any different in this
> > respect. I was a bit surprised that this hadn't been fixed yet, but
> > maybe people just aren't using ramdisks anymore these days, or they
> > worked around it by setting initrd_high explicitly.
> 
> This depends a lot on a number of things.  For example, you should be
> able to use a ramdisk in NOR flash directly, i. e. without loading it
> to RAM first - especially if it;s a comprtessed ramdis image, and the
> Kernel will copy/uncompress it anyway.  Depending on your memory map
> the address range of the NOR flash may be way outside (and above) that
> of the system RAM.  Are you sure your changes will not break any such
> usage?

As far as I can tell, the only place where initrd_high is used is to
allocate an area from RAM that's used to relocate the ramdisk to. In
case where the ramdisk is directly in NOR and isn't supposed to be
copied anyway, this code should never be run.

> > Yeah, I wondered whether maybe not relocating the ramdisk (and the FDT)
> > in the first place might even be a better default fallback. It makes the
> 
> We want to avoid any copy of the ramdisk at all, if possible.

Well, that's not what the current code does. It currently always
relocates unless you happen to explicitly set initrd_high = 0xffffffff.

So maybe one other alternative would be to check whether the current
location of the ramdisk is within the low memory area and only relocate
otherwise. Does that match your expectation?

> > > > Also, when changing the behaviour, you should also update the
> > > > comments.
> > >
> > > Agreed.
> > 
> > I'm not sure which comments you are referring to. I updated the one
> > immediately above the changed code and the one above the function
> > doesn't contain anything relating to the default behaviour in case
> > initrd_high is unset.
> 
> ...because default behaviour was do do nothing. Now you do something,
> so this should be documented.

The default behaviour was to not restrict loading to high memory at all,
which is what the comment said:

	/* not set, no restrictions to load high */
	initrd_high = ~0;

The patch that I posted changes the above to this:

	/* make sure to put ramdisk in low memory */
	initrd_high = getenv_bootm_low() + getenv_bootm_mapsize();

Doesn't that accurately describe the change?

Thierry

Dear Stephen Warren,

In message <51DF024C.2080102@wwwdotorg.org> you wrote:
>
> > This depends a lot on a number of things.  For example, you should be
> > able to use a ramdisk in NOR flash directly, i. e. without loading it
> > to RAM first - especially if it;s a comprtessed ramdis image, and the
> > Kernel will copy/uncompress it anyway.
> 
> That would be nice, but I don't believe the kernel supports that (at
> least not on ARM):

You are right. RMK repeatedly rejected patches to allow this; but it
has always been available on other architectures, like PPC.

Best regards,

Wolfgang Denk

Dear Thierry Reding,

In message <20130711190849.GA17212@dhcp-172-17-186-34.nvidia.com> you wrote:
> 
> > This depends a lot on a number of things.  For example, you should be
> > able to use a ramdisk in NOR flash directly, i. e. without loading it
> > to RAM first - especially if it;s a comprtessed ramdis image, and the
> > Kernel will copy/uncompress it anyway.  Depending on your memory map
> > the address range of the NOR flash may be way outside (and above) that
> > of the system RAM.  Are you sure your changes will not break any such
> > usage?
> 
> As far as I can tell, the only place where initrd_high is used is to
> allocate an area from RAM that's used to relocate the ramdisk to. In
> case where the ramdisk is directly in NOR and isn't supposed to be
> copied anyway, this code should never be run.

Agreed.  I'm not sure this is what's happening currently, though.

> So maybe one other alternative would be to check whether the current
> location of the ramdisk is within the low memory area and only relocate
> otherwise. Does that match your expectation?

Appears to make sense, indeed.  But I have to admit that I'm not deep
enough into the details of this code at the moment to make a qualified
statement.

> > ...because default behaviour was do do nothing. Now you do something,
> > so this should be documented.
>
> The default behaviour was to not restrict loading to high memory at all,
> which is what the comment said:
>
> 	/* not set, no restrictions to load high */
> 	initrd_high = ~0;
>
> The patch that I posted changes the above to this:
>
> 	/* make sure to put ramdisk in low memory */
> 	initrd_high = getenv_bootm_low() + getenv_bootm_mapsize();
>
> Doesn't that accurately describe the change?

I think this shouldbe added to the function header (too).

Best regards,

Wolfgang Denk