linux-next: parsing mem=700M broken

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 "=").

Hugh

commit 5c886584a758edba7e25ad9df974cf15a4a1f59d

Author: Rusty Russell <[email protected]>
Date:   Wed Dec 3 13:34:34 2008 +1030

Call early_param earlier.

We delete all the arch calls: we call it from start_kernel earlier now. We also no longer take a temporary copy, but parse in place.

Note: IA64 needs to parse "machvec=" before other commandline options but machvec_init_from_cmdline() needs efi_init() and io_port_init(), so they are all moved into arch_get_boot_command_line().

Quoted 12 lines · Hugh Dickins

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 "=").

Hugh

commit 5c886584a758edba7e25ad9df974cf15a4a1f59d

Author: Rusty Russell <[email protected]>
Date:   Wed Dec 3 13:34:34 2008 +1030

Quoted 9 lines

Call early_param earlier.

We delete all the arch calls: we call it from start_kernel earlier now. We also no longer take a temporary copy, but parse in place.

Note: IA64 needs to parse "machvec=" before other commandline options but machvec_init_from_cmdline() needs efi_init() and io_port_init(), so they are all moved into arch_get_boot_command_line().

yeah, you are right.

static int __init parse_memopt(char *p)

{
        u64 mem_size;

        if (!p)
                return -EINVAL;

#ifdef CONFIG_X86_32
        if (!strcmp(p, "nopentium")) {
                setup_clear_cpu_cap(X86_FEATURE_PSE);
                return 0;
        }

#endif

        userdef = 1;
        mem_size = memparse(p, &p);
        e820_remove_range(mem_size, ULLONG_MAX - mem_size, E820_RAM, 1);

        return 0;
}

early_param("mem", parse_memopt);

in x86 we use that param analyser much later ( after we check the e820 table..)

other cpu flag will be overwrite too..

anyway, this patch is not go through x86 tip..., some strange!

Quoted 6 lines · Yinghai Lu

Quoted 5 lines · Hugh Dickins

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 "=").

Rusty, will you fix or revert it?

Quoted 8 lines · On Wednesday 24 December 2008 01:03:27 Ingo Molnar wrote:

Quoted 6 lines · Yinghai Lu

Quoted 5 lines · Hugh Dickins

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.

Quoted 1 line

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

arch/x86/kernel/e820.c +48 -33

index 7aafeb5..cab24a6 100644
@@ -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;
+	}

Quoted 19 lines · Rusty Russell

Quoted 8 lines · On Wednesday 24 December 2008 01:03:27 Ingo Molnar wrote:

Quoted 6 lines · Yinghai Lu

Quoted 5 lines · Hugh Dickins

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.

Quoted 1 line

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.

Quoted 8 lines

commit b69cab1078c69a568afdd2eb8ee6f8559b769e2c

Author: Rusty Russell <[email protected]>
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 <[email protected]>

for cpu caps on boot cpu setup still need

could be in tip your tree.

---

[PATCH] x86: clean up setup_clear/force_cpu_cap handling

Impact: fix and cleanup

setup_force_cpu_cap() only have one user xen
but it should not reuse cleared_cpu_cpus. it will have problem for smp.

need to have cpu_cpus_set array too.
also need to setup handling before all cpus cap AND

Signed-off-by Yinghai Lu <[email protected]>

---
---
could be put in next:

[PATCH] x86: update boot_cpu x86 cap according in early_identify_cpu

Impact: fix

early_param is moved more earlier. So need to update boot_cpu_data x86 cap in early_identify_cpu() to make sure that is right.

Signed-off-by Yinghai Lu <[email protected]>

---
 arch/x86/kernel/cpu/common.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

Index: linux-2.6/arch/x86/kernel/cpu/common.c
===================================================================

arch/x86/kernel/cpu/common.c +4 -0

@@ -552,6 +552,16 @@ static void __init early_identify_cpu(st
       if (this_cpu->c_early_init)
               this_cpu->c_early_init(c);
 
+       /*
+        * Clear/Set all flags overriden by options, need do it
+        * because early_param is done before early_cpu_init now.
+        * We can get boot_cpu_data.x86_capability right.

for cpu caps on boot cpu setup still need

Quoted 1 line · On Wednesday 24 December 2008 18:39:04 Yinghai Lu wrote:

for cpu caps on boot cpu setup still need

OK, how about these? I cleaned up description a little, and put it in own function.

Subject: [PATCH] x86: clean up setup_clear/force_cpu_cap handling
From: Yinghai Lu <[email protected]>

Impact: fix and cleanup

setup_force_cpu_cap() only has one user (xen) but it should not reuse cleared_cpu_cpus. It will have problems for smp.

Need to have cpu_cpus_set array too, and need to combine it before we AND into boot_cpu_data.x86_capability.

Signed-off-by Yinghai Lu <[email protected]>

Signed-off-by Rusty Russell <[email protected]>

---
 arch/x86/include/asm/cpufeature.h |    4 ++--
 arch/x86/include/asm/processor.h  |    3 ++-
 arch/x86/kernel/cpu/common.c      |   17 ++++++++++++-----
 3 files changed, 16 insertions(+), 8 deletions(-)

arch/x86/include/asm/cpufeature.h +2 -2

@@ -188,11 +188,11 @@ extern const char * const x86_power_flag
 #define clear_cpu_cap(c, bit)  clear_bit(bit, (unsigned long *)((c)->x86_capability))
 #define setup_clear_cpu_cap(bit) do { \
       clear_cpu_cap(&boot_cpu_data, bit);     \
-       set_bit(bit, (unsigned long *)cleared_cpu_caps); \
+       set_bit(bit, (unsigned long *)cpu_caps_cleared); \
 } while (0)
 #define setup_force_cpu_cap(bit) do { \
       set_cpu_cap(&boot_cpu_data, bit);       \
-       clear_bit(bit, (unsigned long *)cleared_cpu_caps);      \
+       set_bit(bit, (unsigned long *)cpu_caps_set);    \
 } while (0)
 
 #define cpu_has_fpu            boot_cpu_has(X86_FEATURE_FPU)

arch/x86/include/asm/processor.h +2 -1

@@ -130,7 +130,8 @@ extern struct cpuinfo_x86   new_cpu_data;
 extern struct cpuinfo_x86      new_cpu_data;
 
 extern struct tss_struct       doublefault_tss;
-extern __u32                   cleared_cpu_caps[NCAPINTS];
+extern __u32                   cpu_caps_cleared[NCAPINTS];
+extern __u32                   cpu_caps_set[NCAPINTS];
 
 #ifdef CONFIG_SMP
 DECLARE_PER_CPU(struct cpuinfo_x86, cpu_info);

arch/x86/kernel/cpu/common.c +15 -5

@@ -219,7 +219,8 @@ static char __cpuinit *table_lookup_mode
       return NULL;            /* Not found */
 }
 
-__u32 cleared_cpu_caps[NCAPINTS] __cpuinitdata;
+__u32 cpu_caps_cleared[NCAPINTS] __cpuinitdata;
+__u32 cpu_caps_set[NCAPINTS] __cpuinitdata;
 
 /* Current gdt points %fs at the "master" per-cpu area: after this,
 * it's on the real one. */

@@ -512,6 +513,16 @@ static void __cpuinit identify_cpu_witho
 #endif
 }
 
+static void __cpuinit override_capabilities(u32 x86_capability[])
+{
+       unsigned int i;
+
+       for (i = 0; i < NCAPINTS; i++) {
+               x86_capability[i] &= ~cpu_caps_cleared[i];
+               x86_capability[i] |= cpu_caps_set[i];
+       }
+}
+
 /*
 * Do minimum CPU detection early.
 * Fields really needed: vendor, cpuid_level, family, model, mask,

@@ -703,6 +714,9 @@ static void __cpuinit identify_cpu(struc
       detect_ht(c);
 #endif
 
+       /* Command-line override before we AND into smp all cpus cap. */
+       override_capabilities(c->x86_capability);
+
       /*
        * On SMP, boot_cpu_data holds the common feature set between
        * all CPUs; so make sure that we indicate which features are

@@ -714,10 +728,6 @@ static void __cpuinit identify_cpu(struc
               for (i = 0; i < NCAPINTS; i++)
                       boot_cpu_data.x86_capability[i] &= c->x86_capability[i];
       }
-
-       /* Clear all flags overriden by options */
-       for (i = 0; i < NCAPINTS; i++)
-               c->x86_capability[i] &= ~cleared_cpu_caps[i];
 
 #ifdef CONFIG_X86_MCE
       /* Init Machine Check Exception if available. */

Subject: x86: override_capabilities in early_identify_cpu
From: Rusty Russell <[email protected]>

Impact: future-proof

This does nothing at the moment, but once early_param() are parsed before setup_arch, it will ensure we respect commandline overrides on the boot cpu's capabilities.

Signed-off-by Rusty Russell <[email protected]>

Cc Yinghai Lu <[email protected]>

arch/x86/kernel/cpu/common.c +2 -1

@@ -559,6 +559,8 @@ static void __init early_identify_cpu(st
 
       if (this_cpu->c_early_init)
               this_cpu->c_early_init(c);
+
+       override_capabilities(c->x86_capability);
 
       validate_pat_support(c);
 
--

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

arch/x86/kernel/e820.c +18 -0

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

Hello Rusty,

arch/x86/kernel/e820.c early_param("memmap", parse_memmap_opt); · line 1242

Quoted 3 lines · Rusty Russell

void __init finish_e820_parsing(void)
{

arch/x86/kernel/e820.c +1 -0

+	bool userdef;

maybe init with userdef = false;

Quoted 6 lines · On Wednesday 24 December 2008 01:03:27 Ingo Molnar wrote:

Quoted 5 lines · Yinghai Lu

Quoted 4 lines · Hugh Dickins

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.

Quoted 1 line

Rusty, will you fix or revert it?

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

Quoted 1 line · Rusty Russell

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

Thanks, that gets my "mem=700M" respected on x86_32 and x86_64. But (of course: it's a patch to arch/x86) doesn't help at all on ppc64; and I presume other architectures also remain broken...

Quoted 3 lines · On Thursday 25 December 2008 01:14:35 Hugh Dickins wrote:

But (of course: it's a patch to arch/x86) doesn't help at all on ppc64; and I presume other architectures also remain broken...

Confirmed ppc64. But it's broken in a *different* way.

Whereas most archs called parse_early_param from setup_arch, powerpc does it before start_kernel. So instead of moving parsing earlier, my patch actually moved it later for powerpc.

Too late, for mem=.

Rather than try to untangle the powerpc boot process (most archs would do most of this in setup_arch), I think I have to expose the parser again so they can call it:

commit 25bf48b74b9fb23b347d00656b604f9e55c72183

Author: Rusty Russell <[email protected]>
Date:   Sat Dec 27 23:40:37 2008 +1030

Fix powerpc (tested on ppc64) command line handling. Powerpc used to call parse_early_param() really early; the change made it too late. Put it back.

Quoted 16 lines · Rusty Russell

Quoted 3 lines · On Thursday 25 December 2008 01:14:35 Hugh Dickins wrote:

But (of course: it's a patch to arch/x86) doesn't help at all on ppc64; and I presume other architectures also remain broken...

Confirmed ppc64. But it's broken in a *different* way.

Whereas most archs called parse_early_param from setup_arch, powerpc does it before start_kernel. So instead of moving parsing earlier, my patch actually moved it later for powerpc.

Too late, for mem=.

Rather than try to untangle the powerpc boot process (most archs would do most of this in setup_arch), I think I have to expose the parser again so they can call it:

Quoted 7 lines

commit 25bf48b74b9fb23b347d00656b604f9e55c72183

Author: Rusty Russell <[email protected]>
Date:   Sat Dec 27 23:40:37 2008 +1030

Fix powerpc (tested on ppc64) command line handling. Powerpc used to call parse_early_param() really early; the change made it too late. Put it back.

I'll make no pretence of reviewing any of this, but this has indeed got mem= back working for me on a 2.6.28-rc9-mm1 - thanks!

I have to wonder if all this comes too late in the cycle for 2.6.29:
changing the early param handling of all the arches is difficult, and apparently hasn't been much tested in the short time that it's been out there in linux-next. But of course, not for me to decide.

Quoted 4 lines · Hugh Dickins

I have to wonder if all this comes too late in the cycle for 2.6.29:
changing the early param handling of all the arches is difficult, and apparently hasn't been much tested in the short time that it's been out there in linux-next. But of course, not for me to decide.

should be ok, just need to go over all early_param()

(Not much change since last time, just consolidated fixes aired here, esp.
x86 early_param fixes thanks to Hugh and Yinghai.)

The following changes since commit 4a6908a3a050aacc9c3a2f36b276b46c0629ad91:
Linus Torvalds (1):

        Linux 2.6.28                                                            

are available in the git repository at:

git://git.kernel.org/pub/scm/linux/kernel/git/rusty/linux-2.6-boot-params.git master

 arch/x86/include/asm/cpufeature.h          |    4 +-
 arch/x86/include/asm/processor.h           |    3 +-
 arch/x86/kernel/cpu/common.c               |   22 +++++--
 arch/x86/kernel/e820.c                     |   88 +++++++++++++++++----------
 arch/x86/kernel/setup.c                    |   41 ++++++-------
 arch/x86/mm/init_32.c                      |   11 ++-