image: Don't relocate ramdisk to highmem

PATCH New Thierry Reding 4 participants 10 messages Jul 10, 2013 – Jul 11, 2013

lore

Share & tools

lore

Thierry Reding

author

2013-07-10 23:47 UTC #0 lore raw

PATCH 1/1 image: Don't relocate ramdisk to highmem

From: Thierry Reding <[email protected]>

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 <[email protected]>

---
 common/image.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

common/image.c +2 -2

index e91c89e..bc79b43 100644
@@ -1090,8 +1090,8 @@ int boot_ramdisk_high(struct lmb *lmb, ulong rd_data, ulong rd_len,
 		if (initrd_high == ~0)
 			initrd_copy_to_ram = 0;
 	} else {
-		/* 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();
 	}

Signature · 1 line

1.8.1.5

Wolfgang Denk

2013-07-11 09:46 UTC #1 lore raw

PATCH 1/1 image: Don't relocate ramdisk to highmem

Dear Thierry Reding,

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

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?

common/image.c int boot_ramdisk_high(struct lmb *lmb, ulong rd_data, ulong rd_len, · line 1094

Quoted 4 lines

common/image.c +2 -2

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

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
The trouble with our times is that the future is not what it used  to
be.                                                     - Paul Valery

Tom Rini

2013-07-11 12:39 UTC #2 lore raw

PATCH 1/1 image: Don't relocate ramdisk to highmem

Quoted 9 lines · Wolfgang Denk

Dear Thierry Reding,
Quoted 2 lines · In message <[email protected]> you…
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).

Quoted 9 lines

common/image.c int boot_ramdisk_high(struct lmb *lmb, ulong rd_data, ulong rd_len, · line 1094
Quoted 4 lines
common/image.c +2 -2
-		/* 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.

Quoted 2 lines

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

Agreed.

Quoted 15 lines

Best regards,

Wolfgang Denk

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

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130711/442ea53c/attachment.pgp>

Thierry Reding

author

2013-07-11 15:00 UTC #3 lore raw

PATCH 1/1 image: Don't relocate ramdisk to highmem

Quoted 7 lines · Tom Rini

Quoted 6 lines · Wolfgang Denk
Dear Thierry Reding,
Quoted 2 lines · In message <[email protected]> you…
The Linux kernel cannot unpack a ramdisk that's stored in high memory.

Quoted 8 lines

Quoted 3 lines
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.

Quoted 8 lines

Quoted 8 lines
common/image.c int boot_ramdisk_high(struct lmb *lmb, ulong rd_data, ulong rd_len, · line 1094
Quoted 4 lines
common/image.c +2 -2
-		/* 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.

Quoted 5 lines

Quoted 1 line
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.

Quoted 2 lines

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.

Quoted 8 lines

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

Quoted 4 lines

Quoted 2 lines
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130711/a83f334c/attachment.pgp>

Wolfgang Denk

2013-07-11 18:21 UTC #4 lore raw

PATCH 1/1 image: Don't relocate ramdisk to highmem

Dear Thierry Reding,

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

Quoted 5 lines
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?

Quoted 2 lines

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.

Quoted 4 lines

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.

Quoted 9 lines

Quoted 4 lines
Quoted 2 lines
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

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
The only time the world beats a path to your door is when you are  in
the bathroom.

Tom Rini

2013-07-11 18:29 UTC #5 lore raw

PATCH 1/1 image: Don't relocate ramdisk to highmem

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

Quoted 1 line · Wolfgang Denk

Dear Thierry Reding,

[snip]

Quoted 5 lines

Quoted 5 lines
Quoted 4 lines
Quoted 2 lines
Also, when changing the behaviour, you should also update the comments.
Agreed.

Quoted 7 lines

Quoted 4 lines
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-----

Stephen Warren

2013-07-11 19:06 UTC #6 lore raw

PATCH 1/1 image: Don't relocate ramdisk to highmem

In reply to Wolfgang Denk

Quoted 15 lines · Wolfgang Denk

Dear Thierry Reding,
Quoted 11 lines · In message <[email protected]> you wrote:
Quoted 5 lines
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.

Quoted 4 lines

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

Quoted 7 lines

#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).

Wolfgang Denk

2013-07-11 20:55 UTC #7 lore raw

PATCH 1/1 image: Don't relocate ramdisk to highmem

Dear Stephen Warren,

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

Quoted 5 lines
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

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
As in certain cults it is possible to kill a process if you know  its
true name.                      -- Ken Thompson and Dennis M. Ritchie

Thierry Reding

author

2013-07-11 19:08 UTC #8 lore raw

PATCH 1/1 image: Don't relocate ramdisk to highmem

In reply to Wolfgang Denk

Quoted 15 lines · Wolfgang Denk

Dear Thierry Reding,
Quoted 11 lines · In message <[email protected]> you wrote:
Quoted 5 lines
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.

Quoted 7 lines

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.

Quoted 4 lines

Quoted 2 lines
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?

Quoted 5 lines

Quoted 5 lines
Quoted 4 lines
Quoted 2 lines
Also, when changing the behaviour, you should also update the comments.
Agreed.

Quoted 7 lines

Quoted 4 lines
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?

Wolfgang Denk

2013-07-11 20:57 UTC #9 lore raw

PATCH 1/1 image: Don't relocate ramdisk to highmem

Dear Thierry Reding,

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

Quoted 8 lines
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?

Quoted 4 lines

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.

Quoted 3 lines

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.

Quoted 9 lines

Quoted 2 lines
...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;

Quoted 6 lines

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

Signature · 9 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
'What shall we do?' said Twoflower.  'Panic?'  said  Rincewind  hope-
fully. He always held that panic was the best means of survival; back
in  the  olden days, his theory went, people faced with hungry sabre-
toothed tigers could be divided very simply in those who panicked and
those who stood there saying 'What a magnificent  brute!'  or  'Here,
pussy.'                      - Terry Pratchett, _The Light Fantastic_