Re: linux-next: parsing mem=700M broken

[Rusty Russell <rusty@rustcorp.com.au>]

On Wednesday 24 December 2008 01:03:27 Ingo Molnar wrote:
> 
> * Yinghai Lu <yinghai@kernel.org> wrote:
> 
> > On Mon, Dec 22, 2008 at 7:06 PM, Hugh Dickins <hugh@veritas.com> wrote:
> > > Hi Rusty,
> > >
> > > I find that bootparam "mem=700M" isn't working in linux-next or mmotm,
> > > and have bisected it down to your patch below; but now I'm off to bed
> > > without working out just what goes wrong (I'll bet it's the "=").

Thanks for the report and analysis.

> Rusty, will you fix or revert it?

mem= is actually easy, memmap= is harder.  This is tested and pushed.

This commit is actually orthogonal to the other changes, but makes most
sense I think in the bootparam tree.

commit b69cab1078c69a568afdd2eb8ee6f8559b769e2c
Author: Rusty Russell <rusty@rustcorp.com.au>
Date:   Wed Dec 24 18:04:19 2008 +1030

    Fix mem= and memmap= parsing: now it happens *before* e820 setup.
    
    Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index 7aafeb5..cab24a6 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -37,6 +37,8 @@
  * copied. It doesn't get modified afterwards. It's registered for the
  * /sys/firmware/memmap interface.
  *
+ * user_e820, exactmap and memlimit are set on cmdline by mem= and memmap=.
+ *
  * That memory map is not modified and is used as base for kexec. The kexec'd
  * kernel should get the same memory map as the firmware provides. Then the
  * user can e.g. boot the original kernel with mem=1G while still booting the
@@ -44,6 +46,9 @@
  */
 struct e820map e820;
 struct e820map e820_saved;
+static struct e820map user_e820 __initdata;
+static bool exactmap __initdata;
+static u64 memlimit __initdata;
 
 /* For PCI or other memory-mapped resources */
 unsigned long pci_mem_start = 0xaeedbabe;
@@ -107,22 +112,28 @@ int __init e820_all_mapped(u64 start, u64 end, unsigned type)
 	return 0;
 }
 
-/*
- * Add a memory region to the kernel e820 map.
- */
-void __init e820_add_region(u64 start, u64 size, int type)
+static void __init __e820_add_region(struct e820map *e820,
+				     u64 start, u64 size, int type)
 {
-	int x = e820.nr_map;
+	int x = e820->nr_map;
 
-	if (x == ARRAY_SIZE(e820.map)) {
+	if (x == ARRAY_SIZE(e820->map)) {
 		printk(KERN_ERR "Ooops! Too many entries in the memory map!\n");
 		return;
 	}
 
-	e820.map[x].addr = start;
-	e820.map[x].size = size;
-	e820.map[x].type = type;
-	e820.nr_map++;
+	e820->map[x].addr = start;
+	e820->map[x].size = size;
+	e820->map[x].type = type;
+	e820->nr_map++;
+}
+
+/*
+ * Add a memory region to the kernel e820 map.
+ */
+void __init e820_add_region(u64 start, u64 size, int type)
+{
+	__e820_add_region(&e820, start, size, type);
 }
 
 void __init e820_print_map(char *who)
@@ -1173,13 +1184,9 @@ static void early_panic(char *msg)
 	panic(msg);
 }
 
-static int userdef __initdata;
-
 /* "mem=nopentium" disables the 4MB page tables. */
 static int __init parse_memopt(char *p)
 {
-	u64 mem_size;
-
 	if (!p)
 		return -EINVAL;
 
@@ -1190,10 +1197,7 @@ static int __init parse_memopt(char *p)
 	}
 #endif
 
-	userdef = 1;
-	mem_size = memparse(p, &p);
-	e820_remove_range(mem_size, ULLONG_MAX - mem_size, E820_RAM, 1);
-
+	memlimit = memparse(p, &p);
 	return 0;
 }
 early_param("mem", parse_memopt);
@@ -1207,16 +1211,7 @@ static int __init parse_memmap_opt(char *p)
 		return -EINVAL;
 
 	if (!strncmp(p, "exactmap", 8)) {
-#ifdef CONFIG_CRASH_DUMP
-		/*
-		 * If we are doing a crash dump, we still need to know
-		 * the real mem size before original memory map is
-		 * reset.
-		 */
-		saved_max_pfn = e820_end_of_ram_pfn();
-#endif
-		e820.nr_map = 0;
-		userdef = 1;
+		exactmap = true;
 		return 0;
 	}
 
@@ -1225,18 +1220,18 @@ static int __init parse_memmap_opt(char *p)
 	if (p == oldp)
 		return -EINVAL;
 
-	userdef = 1;
 	if (*p == '@') {
 		start_at = memparse(p+1, &p);
-		e820_add_region(start_at, mem_size, E820_RAM);
+		__e820_add_region(&user_e820, start_at, mem_size, E820_RAM);
 	} else if (*p == '#') {
 		start_at = memparse(p+1, &p);
-		e820_add_region(start_at, mem_size, E820_ACPI);
+		__e820_add_region(&user_e820, start_at, mem_size, E820_ACPI);
 	} else if (*p == '$') {
 		start_at = memparse(p+1, &p);
-		e820_add_region(start_at, mem_size, E820_RESERVED);
+		__e820_add_region(&user_e820, start_at, mem_size,
+				  E820_RESERVED);
 	} else
-		e820_remove_range(mem_size, ULLONG_MAX - mem_size, E820_RAM, 1);
+		memlimit = mem_size;
 
 	return *p == '\0' ? 0 : -EINVAL;
 }
@@ -1244,6 +1239,33 @@ early_param("memmap", parse_memmap_opt);
 
 void __init finish_e820_parsing(void)
 {
+	bool userdef;
+	int i;
+
+	if (memlimit) {
+		e820_remove_range(memlimit, ULLONG_MAX - memlimit, E820_RAM, 1);
+		userdef = true;
+	}
+
+	if (exactmap) {
+#ifdef CONFIG_CRASH_DUMP
+		/*
+		 * If we are doing a crash dump, we still need to know
+		 * the real mem size before original memory map is
+		 * reset.
+		 */
+		saved_max_pfn = e820_end_of_ram_pfn();
+#endif
+		e820.nr_map = 0;
+		userdef = true;
+	}
+
+	for (i = 0; i < user_e820.nr_map; i++) {
+		e820_add_region(user_e820.map[i].addr, user_e820.map[i].size,
+				user_e820.map[i].type);
+		userdef = true;
+	}
+
 	if (userdef) {
 		int nr = e820.nr_map;