linux-next: reservetop fix disables mem=

linux-next: reservetop fix disables mem=

[Hugh Dickins]

I find the "mem=" boot parameter disabled in today's linux-next:
reverting the tip commit below fixes that.

Hugh

From: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
Date: Thu, 20 Aug 2009 12:23:11 +0000 (+0800)
Subject: x86: Fix system crash when loading with "reservetop" parameter
X-Git-Url: http://git.kernel.org/?p=linux%2Fkernel%2Fgit%2Fmingo%2Flinux-2.6-x86.git;a=commitdiff_plain;h=8126dec32738421afa362114337331337b4be17f

x86: Fix system crash when loading with "reservetop" parameter

The system will die if the kernel is booted with "reservetop"
parameter, in present code, parse "reservetop" parameter after
early_ioremap_init(), and some function still use
early_ioremap() after it.

The problem is, "reservetop" parameter can modify
'FIXADDR_TOP', then the virtual address got by early_ioremap()
is base on old 'FIXADDR_TOP', but the page mapping is base on
new 'FIXADDR_TOP', it will occur page fault, and the IDT is not
prepare yet, so, the system is dead.

So, put parse_early_param() in the front of
early_ioremap_init() in this patch.

Signed-off-by: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
Cc: yinghai@kernel.org
Cc: Andrew Morton <akpm@linux-foundation.org>
LKML-Reference: <4A8D402F.4080805@cn.fujitsu.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---

diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 63f32d2..02643cc 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -711,6 +711,11 @@ void __init setup_arch(char **cmdline_p)
 	printk(KERN_INFO "Command line: %s\n", boot_command_line);
 #endif
 
+	strlcpy(command_line, boot_command_line, COMMAND_LINE_SIZE);
+	*cmdline_p = command_line;
+
+	parse_early_param();
+
 	/* VMI may relocate the fixmap; do this before touching ioremap area */
 	vmi_init();
 
@@ -793,11 +798,6 @@ void __init setup_arch(char **cmdline_p)
 #endif
 #endif
 
-	strlcpy(command_line, boot_command_line, COMMAND_LINE_SIZE);
-	*cmdline_p = command_line;
-
-	parse_early_param();
-
 #ifdef CONFIG_X86_64
 	check_efer();
 #endif

Re: linux-next: reservetop fix disables mem=

[Yinghai Lu]

Hugh Dickins wrote:
> I find the "mem=" boot parameter disabled in today's linux-next:
> reverting the tip commit below fixes that.
> 
> Hugh
> 
> From: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
> Date: Thu, 20 Aug 2009 12:23:11 +0000 (+0800)
> Subject: x86: Fix system crash when loading with "reservetop" parameter
> X-Git-Url: http://git.kernel.org/?p=linux%2Fkernel%2Fgit%2Fmingo%2Flinux-2.6-x86.git;a=commitdiff_plain;h=8126dec32738421afa362114337331337b4be17f
> 
> x86: Fix system crash when loading with "reservetop" parameter
> 
> The system will die if the kernel is booted with "reservetop"
> parameter, in present code, parse "reservetop" parameter after
> early_ioremap_init(), and some function still use
> early_ioremap() after it.
> 
> The problem is, "reservetop" parameter can modify
> 'FIXADDR_TOP', then the virtual address got by early_ioremap()
> is base on old 'FIXADDR_TOP', but the page mapping is base on
> new 'FIXADDR_TOP', it will occur page fault, and the IDT is not
> prepare yet, so, the system is dead.
> 
> So, put parse_early_param() in the front of
> early_ioremap_init() in this patch.
> 
> Signed-off-by: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
> Cc: yinghai@kernel.org
> Cc: Andrew Morton <akpm@linux-foundation.org>
> LKML-Reference: <4A8D402F.4080805@cn.fujitsu.com>
> Signed-off-by: Ingo Molnar <mingo@elte.hu>
> ---
> 
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index 63f32d2..02643cc 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -711,6 +711,11 @@ void __init setup_arch(char **cmdline_p)
>  	printk(KERN_INFO "Command line: %s\n", boot_command_line);
>  #endif
>  
> +	strlcpy(command_line, boot_command_line, COMMAND_LINE_SIZE);
> +	*cmdline_p = command_line;
> +
> +	parse_early_param();
> +
>  	/* VMI may relocate the fixmap; do this before touching ioremap area */
>  	vmi_init();
>  
> @@ -793,11 +798,6 @@ void __init setup_arch(char **cmdline_p)
>  #endif
>  #endif
>  
> -	strlcpy(command_line, boot_command_line, COMMAND_LINE_SIZE);
> -	*cmdline_p = command_line;
> -
> -	parse_early_param();
> -
>  #ifdef CONFIG_X86_64
>  	check_efer();
>  #endif

yes, that patch will break other built-in command too.

need drop that patch.

also the problem was caused by vmi patch, and that commit should be reverted.

commit ae8d04e2ecbb233926860e9ce145eac19c7835dc
Author: Zachary Amsden <zach@vmware.com>
Date:   Sat Dec 13 12:36:58 2008 -0800

    x86 Fix VMI crash on boot in 2.6.28-rc8
    
    VMI initialiation can relocate the fixmap, causing early_ioremap to
    malfunction if it is initialized before the relocation.  To fix this,
    VMI activation is split into two phases; the detection, which must
    happen before setting up ioremap, and the activation, which must happen
    after parsing early boot parameters.
    
    This fixes a crash on boot when VMI is enabled under VMware.
    
    Signed-off-by: Zachary Amsden <zach@vmware.com>
    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 9d5674f..bdec76e 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -794,6 +794,9 @@ void __init setup_arch(char **cmdline_p)
        printk(KERN_INFO "Command line: %s\n", boot_command_line);
 #endif
 
+       /* VMI may relocate the fixmap; do this before touching ioremap area */
+       vmi_init();
+
        early_cpu_init();
        early_ioremap_init();
 
@@ -880,13 +883,8 @@ void __init setup_arch(char **cmdline_p)
        check_efer();
 #endif
 
-#if defined(CONFIG_VMI) && defined(CONFIG_X86_32)
-       /*
-        * Must be before kernel pagetables are setup
-        * or fixmap area is touched.
-        */
-       vmi_init();
-#endif
+       /* Must be before kernel pagetables are setup */
+       vmi_activate();
 
        /* after early param, so could get panic from serial */
        reserve_early_setup_data();


and according to
http://lkml.org/lkml/2008/12/10/388
http://lkml.org/lkml/2008/12/10/456

Zachary should split reserve_top_address() to two functions... before sending that patch to Linus


YH

Re: linux-next: reservetop fix disables mem=

[Ingo Molnar]

* Yinghai Lu <yinghai@kernel.org> wrote:

> Hugh Dickins wrote:
> > I find the "mem=" boot parameter disabled in today's linux-next:
> > reverting the tip commit below fixes that.
> > 
> > Hugh
> > 
> > From: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
> > Date: Thu, 20 Aug 2009 12:23:11 +0000 (+0800)
> > Subject: x86: Fix system crash when loading with "reservetop" parameter
> > X-Git-Url: http://git.kernel.org/?p=linux%2Fkernel%2Fgit%2Fmingo%2Flinux-2.6-x86.git;a=commitdiff_plain;h=8126dec32738421afa362114337331337b4be17f
> > 
> > x86: Fix system crash when loading with "reservetop" parameter
> > 
> > The system will die if the kernel is booted with "reservetop"
> > parameter, in present code, parse "reservetop" parameter after
> > early_ioremap_init(), and some function still use
> > early_ioremap() after it.
> > 
> > The problem is, "reservetop" parameter can modify
> > 'FIXADDR_TOP', then the virtual address got by early_ioremap()
> > is base on old 'FIXADDR_TOP', but the page mapping is base on
> > new 'FIXADDR_TOP', it will occur page fault, and the IDT is not
> > prepare yet, so, the system is dead.
> > 
> > So, put parse_early_param() in the front of
> > early_ioremap_init() in this patch.
> > 
> > Signed-off-by: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
> > Cc: yinghai@kernel.org
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > LKML-Reference: <4A8D402F.4080805@cn.fujitsu.com>
> > Signed-off-by: Ingo Molnar <mingo@elte.hu>
> > ---
> > 
> > diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> > index 63f32d2..02643cc 100644
> > --- a/arch/x86/kernel/setup.c
> > +++ b/arch/x86/kernel/setup.c
> > @@ -711,6 +711,11 @@ void __init setup_arch(char **cmdline_p)
> >  	printk(KERN_INFO "Command line: %s\n", boot_command_line);
> >  #endif
> >  
> > +	strlcpy(command_line, boot_command_line, COMMAND_LINE_SIZE);
> > +	*cmdline_p = command_line;
> > +
> > +	parse_early_param();
> > +
> >  	/* VMI may relocate the fixmap; do this before touching ioremap area */
> >  	vmi_init();
> >  
> > @@ -793,11 +798,6 @@ void __init setup_arch(char **cmdline_p)
> >  #endif
> >  #endif
> >  
> > -	strlcpy(command_line, boot_command_line, COMMAND_LINE_SIZE);
> > -	*cmdline_p = command_line;
> > -
> > -	parse_early_param();
> > -
> >  #ifdef CONFIG_X86_64
> >  	check_efer();
> >  #endif
> 
> yes, that patch will break other built-in command too.
> 
> need drop that patch.

done. Was nervous about the patch already:

  http://lkml.org/lkml/2009/8/21/127

> also the problem was caused by vmi patch, and that commit should 
> be reverted.
>
> commit ae8d04e2ecbb233926860e9ce145eac19c7835dc
> Author: Zachary Amsden <zach@vmware.com>
> Date:   Sat Dec 13 12:36:58 2008 -0800
> 
>     x86 Fix VMI crash on boot in 2.6.28-rc8
>     
>     VMI initialiation can relocate the fixmap, causing early_ioremap to
>     malfunction if it is initialized before the relocation.  To fix this,
>     VMI activation is split into two phases; the detection, which must
>     happen before setting up ioremap, and the activation, which must happen
>     after parsing early boot parameters.
>     
>     This fixes a crash on boot when VMI is enabled under VMware.
>     
>     Signed-off-by: Zachary Amsden <zach@vmware.com>
>     Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> 
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index 9d5674f..bdec76e 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -794,6 +794,9 @@ void __init setup_arch(char **cmdline_p)
>         printk(KERN_INFO "Command line: %s\n", boot_command_line);
>  #endif
>  
> +       /* VMI may relocate the fixmap; do this before touching ioremap area */
> +       vmi_init();
> +
>         early_cpu_init();
>         early_ioremap_init();
>  
> @@ -880,13 +883,8 @@ void __init setup_arch(char **cmdline_p)
>         check_efer();
>  #endif
>  
> -#if defined(CONFIG_VMI) && defined(CONFIG_X86_32)
> -       /*
> -        * Must be before kernel pagetables are setup
> -        * or fixmap area is touched.
> -        */
> -       vmi_init();
> -#endif
> +       /* Must be before kernel pagetables are setup */
> +       vmi_activate();
>  
>         /* after early param, so could get panic from serial */
>         reserve_early_setup_data();
> 
> 
> and according to
> http://lkml.org/lkml/2008/12/10/388
> http://lkml.org/lkml/2008/12/10/456
> 
> Zachary should split reserve_top_address() to two functions... 
> before sending that patch to Linus

mind looking at this yourself if interested? Zachary has not been 
active for quite some time.

	Ingo

Re: linux-next: reservetop fix disables mem=

[Xiao Guangrong]

Yinghai Lu wrote:

> 
> yes, that patch will break other built-in command too.
> 
> need drop that patch.
> 

Sorry for trouble you all by my stupid patch.

The main problem is that can't parse 'reservetop' and 'mem'/'memmap'
parameters at the same time, because we should parse 'mem'/'memmap'
after e820 map detected, and it might use early_memremap() during
e820 map detecting, so we should call early_ioremap_init() before it,
'reservetop' parameter can change 'FIXADDR_TOP', so we parse
'reservetop' first,

like below sequences:
  ......
  ___parse 'reservetop'___  ->
  early_ioremap_init()	  ->
  setup_memory_map(); /* might call early_memremap()*/  ->
  parse_setup_data(); /* might call early_memremap()*/  ->
  e820_reserve_setup_data();	->
  ___parse 'mem'/'memmap' and other parameters___
  ......

(the same as 'early_ioremap_debug' parameter)

So, I'm afraid that we need separate parse 'reservetop'/
'early_ioremap_debug' parameters early

Thanks,
Xiao

> also the problem was caused by vmi patch, and that commit should be reverted.
> 
> commit ae8d04e2ecbb233926860e9ce145eac19c7835dc
> Author: Zachary Amsden <zach@vmware.com>
> Date:   Sat Dec 13 12:36:58 2008 -0800
> 
>     x86 Fix VMI crash on boot in 2.6.28-rc8
>     
>     VMI initialiation can relocate the fixmap, causing early_ioremap to
>     malfunction if it is initialized before the relocation.  To fix this,
>     VMI activation is split into two phases; the detection, which must
>     happen before setting up ioremap, and the activation, which must happen
>     after parsing early boot parameters.
>     
>     This fixes a crash on boot when VMI is enabled under VMware.
>     
>     Signed-off-by: Zachary Amsden <zach@vmware.com>
>     Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> 
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index 9d5674f..bdec76e 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -794,6 +794,9 @@ void __init setup_arch(char **cmdline_p)
>         printk(KERN_INFO "Command line: %s\n", boot_command_line);
>  #endif
>  
> +       /* VMI may relocate the fixmap; do this before touching ioremap area */
> +       vmi_init();
> +
>         early_cpu_init();
>         early_ioremap_init();
>  
> @@ -880,13 +883,8 @@ void __init setup_arch(char **cmdline_p)
>         check_efer();
>  #endif
>  
> -#if defined(CONFIG_VMI) && defined(CONFIG_X86_32)
> -       /*
> -        * Must be before kernel pagetables are setup
> -        * or fixmap area is touched.
> -        */
> -       vmi_init();
> -#endif
> +       /* Must be before kernel pagetables are setup */
> +       vmi_activate();
>  
>         /* after early param, so could get panic from serial */
>         reserve_early_setup_data();
> 
> 
> and according to
> http://lkml.org/lkml/2008/12/10/388
> http://lkml.org/lkml/2008/12/10/456
> 
> Zachary should split reserve_top_address() to two functions... before sending that patch to Linus
> 
> 
> YH
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
> 

Re: linux-next: reservetop fix disables mem=

[Yinghai Lu]

Ingo Molnar wrote:
>> and according to
>> http://lkml.org/lkml/2008/12/10/388
>> http://lkml.org/lkml/2008/12/10/456
>>
>> Zachary should split reserve_top_address() to two functions... 
>> before sending that patch to Linus
> 
> mind looking at this yourself if interested? Zachary has not been 
> active for quite some time.
> 


other ways is to revive the Rusty Russell's patchset that move paramater early...

YH

[RFC][PATCH] x86: introduce parse_early_param_alone() to parse param early (Was Re: linux-next: reservetop fix disables mem= )

[Xiao Guangrong]

Hi Ingo,

I find the broken patch is still in -tip tree:

Commit-ID:  8126dec32738421afa362114337331337b4be17f
Gitweb:     http://git.kernel.org/tip/8126dec32738421afa362114337331337b4be17f
Author:     Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
AuthorDate: Thu, 20 Aug 2009 20:23:11 +0800
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Fri, 21 Aug 2009 16:40:30 +0200

x86: Fix system crash when loading with "reservetop" parameter

So, this patch is base on it.
---------------------------------------------
Subject: [RFC][PATCH] x86: introduce parse_early_param_alone() to parse param before parse_early_param()

Sometimes, we should parse specific boot parameter alone before
parse_early_param(), such as "reservetop" and "early_ioremap_debug",
like below:

> The main problem is that can't parse 'reservetop' and 'mem'/'memmap'
> parameters at the same time, because we should parse 'mem'/'memmap'
> after e820 map detected, and it might use early_memremap() during
> e820 map detecting, so we should call early_ioremap_init() before it,
> 'reservetop' parameter can change 'FIXADDR_TOP', so we parse
> 'reservetop' first,
> 
> like below sequences:
>   ......
>   ___parse 'reservetop'___  ->
>   early_ioremap_init()	  ->
>   setup_memory_map(); /* might call early_memremap()*/  ->
>   parse_setup_data(); /* might call early_memremap()*/  ->
>   e820_reserve_setup_data();	->
>   ___parse 'mem'/'memmap' and other parameters___
>   ......
> 
> (the same as 'early_ioremap_debug' parameter)
> 
> So, I'm afraid that we need separate parse 'reservetop'/
> 'early_ioremap_debug' parameters early
> 

This patch introduce a new api named parse_early_param_alone() to
do this.

Signed-off-by: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
---
 arch/x86/kernel/setup.c |   33 ++++++++++++++++++------------
 include/linux/init.h    |    1 +
 init/main.c             |   50 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 71 insertions(+), 13 deletions(-)

diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 83ea092..f241c12 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -696,10 +696,28 @@ void __init setup_arch(char **cmdline_p)
 	printk(KERN_INFO "Command line: %s\n", boot_command_line);
 #endif
 
+#ifdef CONFIG_CMDLINE_BOOL
+#ifdef CONFIG_CMDLINE_OVERRIDE
+	strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
+#else
+	if (builtin_cmdline[0]) {
+		/* append boot loader cmdline to builtin */
+		strlcat(builtin_cmdline, " ", COMMAND_LINE_SIZE);
+		strlcat(builtin_cmdline, boot_command_line, COMMAND_LINE_SIZE);
+		strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
+	}
+#endif
+#endif
+
 	strlcpy(command_line, boot_command_line, COMMAND_LINE_SIZE);
 	*cmdline_p = command_line;
 
-	parse_early_param();
+	/*
+	 * We should parse "reservetop" and "early_ioremap_debug" before
+	 * early_ioremap_init().
+	 */
+	parse_early_param_alone("reservetop");
+	parse_early_param_alone("early_ioremap_debug");
 
 	/* VMI may relocate the fixmap; do this before touching ioremap area */
 	vmi_init();
@@ -770,18 +788,7 @@ void __init setup_arch(char **cmdline_p)
 	bss_resource.start = virt_to_phys(&__bss_start);
 	bss_resource.end = virt_to_phys(&__bss_stop)-1;
 
-#ifdef CONFIG_CMDLINE_BOOL
-#ifdef CONFIG_CMDLINE_OVERRIDE
-	strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
-#else
-	if (builtin_cmdline[0]) {
-		/* append boot loader cmdline to builtin */
-		strlcat(builtin_cmdline, " ", COMMAND_LINE_SIZE);
-		strlcat(builtin_cmdline, boot_command_line, COMMAND_LINE_SIZE);
-		strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
-	}
-#endif
-#endif
+	parse_early_param();
 
 #ifdef CONFIG_X86_64
 	check_efer();
diff --git a/include/linux/init.h b/include/linux/init.h
index b30f694..4e0082a 100644
--- a/include/linux/init.h
+++ b/include/linux/init.h
@@ -242,6 +242,7 @@ struct obs_kernel_param {
 	__setup_param(str, fn, fn, 1)
 
 /* Relies on boot_command_line being set */
+void __init parse_early_param_alone(char * param);
 void __init parse_early_param(void);
 void __init parse_early_options(char *cmdline);
 #endif /* __ASSEMBLY__ */
diff --git a/init/main.c b/init/main.c
index 0ec75ce..d7d155b 100644
--- a/init/main.c
+++ b/init/main.c
@@ -477,6 +477,11 @@ static int __init do_early_param(char *param, char *val)
 	struct obs_kernel_param *p;
 
 	for (p = __setup_start; p < __setup_end; p++) {
+
+		/* Skip it if it has been parsed in do_early_param_alone() */
+		if (unlikely(p->early == -1))
+			continue;
+
 		if ((p->early && strcmp(param, p->str) == 0) ||
 		    (strcmp(param, "console") == 0 &&
 		     strcmp(p->str, "earlycon") == 0)
@@ -490,6 +495,51 @@ static int __init do_early_param(char *param, char *val)
 	return 0;
 }
 
+static char *alone_param __initdata;
+
+static int __init do_early_param_alone(char *param, char *val)
+{
+	struct obs_kernel_param *p;
+
+	BUG_ON(!alone_param);
+
+	if (strcmp(param, alone_param))
+		return 0;
+
+	for (p = __setup_start; p < __setup_end; p++) {
+		if (p->early && strcmp(param, p->str) == 0) {
+
+			WARN(p->early != 1, "%s is not a early param or "
+			     "it has been parsed\n", p->str);
+
+			if (p->setup_func(val) != 0)
+				printk(KERN_WARNING
+				       "Malformed early option '%s'\n", param);
+			/*
+			 * Set p->early to -1 to avoid parse_early_param()
+			 * and obsolete_checksetup() parse it again.
+			 */
+			p->early = -1;
+		}
+	}
+	/* We accept everything at this stage. */
+	return 0;
+}
+
+/*
+ * Parse specific boot parameter alone before parse_early_param().
+ * Note: only early params can do this.
+ */
+void __init parse_early_param_alone(char *param)
+{
+       static __initdata char tmp_cmdline[COMMAND_LINE_SIZE];
+
+       alone_param = param;
+       strlcpy(tmp_cmdline, boot_command_line, COMMAND_LINE_SIZE);
+       parse_args("early param alone", tmp_cmdline, NULL, 0,
+		  do_early_param_alone);
+}
+
 void __init parse_early_options(char *cmdline)
 {
 	parse_args("early options", cmdline, NULL, 0, do_early_param);
-- 
1.6.1.2

Re: [RFC][PATCH] x86: introduce parse_early_param_alone() to parse param early (Was Re: linux-next: reservetop fix disables mem= )

[Américo Wang]

On Tue, Sep 01, 2009 at 03:55:14PM +0800, Xiao Guangrong wrote:
>Hi Ingo,
>
>I find the broken patch is still in -tip tree:
>
>Commit-ID:  8126dec32738421afa362114337331337b4be17f
>Gitweb:     http://git.kernel.org/tip/8126dec32738421afa362114337331337b4be17f
>Author:     Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
>AuthorDate: Thu, 20 Aug 2009 20:23:11 +0800
>Committer:  Ingo Molnar <mingo@elte.hu>
>CommitDate: Fri, 21 Aug 2009 16:40:30 +0200
>
>x86: Fix system crash when loading with "reservetop" parameter
>
>So, this patch is base on it.
>---------------------------------------------
>Subject: [RFC][PATCH] x86: introduce parse_early_param_alone() to parse param before parse_early_param()
>
>Sometimes, we should parse specific boot parameter alone before
>parse_early_param(), such as "reservetop" and "early_ioremap_debug",
>like below:
>
>> The main problem is that can't parse 'reservetop' and 'mem'/'memmap'
>> parameters at the same time, because we should parse 'mem'/'memmap'
>> after e820 map detected, and it might use early_memremap() during
>> e820 map detecting, so we should call early_ioremap_init() before it,
>> 'reservetop' parameter can change 'FIXADDR_TOP', so we parse
>> 'reservetop' first,
>> 
>> like below sequences:
>>   ......
>>   ___parse 'reservetop'___  ->
>>   early_ioremap_init()	  ->
>>   setup_memory_map(); /* might call early_memremap()*/  ->
>>   parse_setup_data(); /* might call early_memremap()*/  ->
>>   e820_reserve_setup_data();	->
>>   ___parse 'mem'/'memmap' and other parameters___
>>   ......
>> 
>> (the same as 'early_ioremap_debug' parameter)
>> 
>> So, I'm afraid that we need separate parse 'reservetop'/
>> 'early_ioremap_debug' parameters early
>> 
>
>This patch introduce a new api named parse_early_param_alone() to
>do this.
>
>Signed-off-by: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
>---
> arch/x86/kernel/setup.c |   33 ++++++++++++++++++------------
> include/linux/init.h    |    1 +
> init/main.c             |   50 +++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 71 insertions(+), 13 deletions(-)
>
>diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
>index 83ea092..f241c12 100644
>--- a/arch/x86/kernel/setup.c
>+++ b/arch/x86/kernel/setup.c
>@@ -696,10 +696,28 @@ void __init setup_arch(char **cmdline_p)
> 	printk(KERN_INFO "Command line: %s\n", boot_command_line);
> #endif
> 
>+#ifdef CONFIG_CMDLINE_BOOL
>+#ifdef CONFIG_CMDLINE_OVERRIDE
>+	strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
>+#else
>+	if (builtin_cmdline[0]) {
>+		/* append boot loader cmdline to builtin */
>+		strlcat(builtin_cmdline, " ", COMMAND_LINE_SIZE);
>+		strlcat(builtin_cmdline, boot_command_line, COMMAND_LINE_SIZE);
>+		strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
>+	}
>+#endif
>+#endif


This seems ugly.

CMDLINE_OVERRIDE depends on CMDLINE_BOOL, right? So the outer #ifdef
can be removed. :)


-- 
Live like a child, think like the god.
 

Re: [RFC][PATCH] x86: introduce parse_early_param_alone() to parse param early (Was Re: linux-next: reservetop fix disables mem= )

[Xiao Guangrong]

Américo Wang wrote:

>> +#ifdef CONFIG_CMDLINE_BOOL
>> +#ifdef CONFIG_CMDLINE_OVERRIDE
>> +	strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
>> +#else
>> +	if (builtin_cmdline[0]) {
>> +		/* append boot loader cmdline to builtin */
>> +		strlcat(builtin_cmdline, " ", COMMAND_LINE_SIZE);
>> +		strlcat(builtin_cmdline, boot_command_line, COMMAND_LINE_SIZE);
>> +		strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
>> +	}
>> +#endif
>> +#endif
> 
> 
> This seems ugly.
> 
> CMDLINE_OVERRIDE depends on CMDLINE_BOOL, right? So the outer #ifdef
> can be removed. :)
> 

Thanks for your review, but I think we can't do that.

Yeah, CMDLINE_OVERRIDE depends on CMDLINE_BOOL, but we don't know whether 
CONFIG_CMDLINE_BOOL is defined if CONFIG_CMDLINE_OVERRIDE is not defined,
like below:

#ifdef CONFIG_CMDLINE_OVERRIDE
	strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
#else
	/* There have two cases:
	 * 1: if CONFIG_CMDLINE_BOOL is defined, it's OK 
	 * 2: if CONFIG_CMDLINE_BOOL is not defined, builtin_cmdline
	 * is not defined, so, the compiler will complain with it
	 */
	if (builtin_cmdline[0]) {
		/* append boot loader cmdline to builtin */
		strlcat(builtin_cmdline, " ", COMMAND_LINE_SIZE);
		strlcat(builtin_cmdline, boot_command_line, COMMAND_LINE_SIZE);
		strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
	}
#endif

Thanks,
Xiao

> 

Re: linux-next: reservetop fix disables mem=

[Yinghai Lu]

[-- Attachment #1: Type: text/plain, Size: 5054 bytes --]

On Mon, Aug 24, 2009 at 11:27 AM, Ingo Molnar<mingo@elte.hu> wrote:
>
> * Yinghai Lu <yinghai@kernel.org> wrote:
>
>> Hugh Dickins wrote:
>> > I find the "mem=" boot parameter disabled in today's linux-next:
>> > reverting the tip commit below fixes that.
>> >
>> > Hugh
>> >
>> > From: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
>> > Date: Thu, 20 Aug 2009 12:23:11 +0000 (+0800)
>> > Subject: x86: Fix system crash when loading with "reservetop" parameter
>> > X-Git-Url: http://git.kernel.org/?p=linux%2Fkernel%2Fgit%2Fmingo%2Flinux-2.6-x86.git;a=commitdiff_plain;h=8126dec32738421afa362114337331337b4be17f
>> >
>> > x86: Fix system crash when loading with "reservetop" parameter
>> >
>> > The system will die if the kernel is booted with "reservetop"
>> > parameter, in present code, parse "reservetop" parameter after
>> > early_ioremap_init(), and some function still use
>> > early_ioremap() after it.
>> >
>> > The problem is, "reservetop" parameter can modify
>> > 'FIXADDR_TOP', then the virtual address got by early_ioremap()
>> > is base on old 'FIXADDR_TOP', but the page mapping is base on
>> > new 'FIXADDR_TOP', it will occur page fault, and the IDT is not
>> > prepare yet, so, the system is dead.
>> >
>> > So, put parse_early_param() in the front of
>> > early_ioremap_init() in this patch.
>> >
>> > Signed-off-by: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
>> > Cc: yinghai@kernel.org
>> > Cc: Andrew Morton <akpm@linux-foundation.org>
>> > LKML-Reference: <4A8D402F.4080805@cn.fujitsu.com>
>> > Signed-off-by: Ingo Molnar <mingo@elte.hu>
>> > ---
>> >
>> > diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
>> > index 63f32d2..02643cc 100644
>> > --- a/arch/x86/kernel/setup.c
>> > +++ b/arch/x86/kernel/setup.c
>> > @@ -711,6 +711,11 @@ void __init setup_arch(char **cmdline_p)
>> >     printk(KERN_INFO "Command line: %s\n", boot_command_line);
>> >  #endif
>> >
>> > +   strlcpy(command_line, boot_command_line, COMMAND_LINE_SIZE);
>> > +   *cmdline_p = command_line;
>> > +
>> > +   parse_early_param();
>> > +
>> >     /* VMI may relocate the fixmap; do this before touching ioremap area */
>> >     vmi_init();
>> >
>> > @@ -793,11 +798,6 @@ void __init setup_arch(char **cmdline_p)
>> >  #endif
>> >  #endif
>> >
>> > -   strlcpy(command_line, boot_command_line, COMMAND_LINE_SIZE);
>> > -   *cmdline_p = command_line;
>> > -
>> > -   parse_early_param();
>> > -
>> >  #ifdef CONFIG_X86_64
>> >     check_efer();
>> >  #endif
>>
>> yes, that patch will break other built-in command too.
>>
>> need drop that patch.
>
> done. Was nervous about the patch already:
>
>  http://lkml.org/lkml/2009/8/21/127
>
>> also the problem was caused by vmi patch, and that commit should
>> be reverted.
>>
>> commit ae8d04e2ecbb233926860e9ce145eac19c7835dc
>> Author: Zachary Amsden <zach@vmware.com>
>> Date:   Sat Dec 13 12:36:58 2008 -0800
>>
>>     x86 Fix VMI crash on boot in 2.6.28-rc8
>>
>>     VMI initialiation can relocate the fixmap, causing early_ioremap to
>>     malfunction if it is initialized before the relocation.  To fix this,
>>     VMI activation is split into two phases; the detection, which must
>>     happen before setting up ioremap, and the activation, which must happen
>>     after parsing early boot parameters.
>>
>>     This fixes a crash on boot when VMI is enabled under VMware.
>>
>>     Signed-off-by: Zachary Amsden <zach@vmware.com>
>>     Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
>>
>> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
>> index 9d5674f..bdec76e 100644
>> --- a/arch/x86/kernel/setup.c
>> +++ b/arch/x86/kernel/setup.c
>> @@ -794,6 +794,9 @@ void __init setup_arch(char **cmdline_p)
>>         printk(KERN_INFO "Command line: %s\n", boot_command_line);
>>  #endif
>>
>> +       /* VMI may relocate the fixmap; do this before touching ioremap area */
>> +       vmi_init();
>> +
>>         early_cpu_init();
>>         early_ioremap_init();
>>
>> @@ -880,13 +883,8 @@ void __init setup_arch(char **cmdline_p)
>>         check_efer();
>>  #endif
>>
>> -#if defined(CONFIG_VMI) && defined(CONFIG_X86_32)
>> -       /*
>> -        * Must be before kernel pagetables are setup
>> -        * or fixmap area is touched.
>> -        */
>> -       vmi_init();
>> -#endif
>> +       /* Must be before kernel pagetables are setup */
>> +       vmi_activate();
>>
>>         /* after early param, so could get panic from serial */
>>         reserve_early_setup_data();
>>
>>
>> and according to
>> http://lkml.org/lkml/2008/12/10/388
>> http://lkml.org/lkml/2008/12/10/456
>>
>> Zachary should split reserve_top_address() to two functions...
>> before sending that patch to Linus
>
> mind looking at this yourself if interested? Zachary has not been
> active for quite some time.
>

please check attached patch...

still need to make early_dbgp and early_console that would use early_ioremap?

YH

[-- Attachment #2: fix_early_param.patch --]
[-- Type: text/x-patch, Size: 6200 bytes --]

[PATCH] x86: fix early_param hanle moved more early

the patch the move early_param_parse early to make reservetop to work again,
it is reported to break mem=...

after close looking, it will break
1. some cpu feature  in early stage too, like cpu_has_x2apic
2. will break built-in-command line
3. will break other memmap= and mem=
4. early_dbgp and early_console that will use early_ioremap to access mmio (?)

so try to
1. move early_cpu_init early
2. move built-in-command copying early
3. add userdef_mem_size to remember mem=
.  add e820_user to remember memmap
   to use userdef_mem_size and e820_user in finalize_map
4: ?

Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 arch/x86/kernel/e820.c  |   52 ++++++++++++++++++++++++++++++++----------------
 arch/x86/kernel/setup.c |   39 ++++++++++++++++++------------------
 2 files changed, 55 insertions(+), 36 deletions(-)

Index: linux-2.6/arch/x86/kernel/e820.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/e820.c
+++ linux-2.6/arch/x86/kernel/e820.c
@@ -44,6 +44,7 @@
  */
 struct e820map e820;
 struct e820map e820_saved;
+static struct e820map e820_user __initdata;
 
 /* For PCI or other memory-mapped resources */
 unsigned long pci_mem_start = 0xaeedbabe;
@@ -1227,11 +1228,11 @@ static void early_panic(char *msg)
 }
 
 static int userdef __initdata;
+static	u64 userdef_mem_size __initdata;
 
 /* "mem=nopentium" disables the 4MB page tables. */
 static int __init parse_memopt(char *p)
 {
-	u64 mem_size;
 
 	if (!p)
 		return -EINVAL;
@@ -1244,13 +1245,13 @@ 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);
+	userdef_mem_size = memparse(p, &p);
 
 	return 0;
 }
 early_param("mem", parse_memopt);
 
+static int userdef_exactmap __initdata;
 static int __init parse_memmap_opt(char *p)
 {
 	char *oldp;
@@ -1260,16 +1261,9 @@ static int __init parse_memmap_opt(char
 		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;
+		e820_user.nr_map = 0;
 		userdef = 1;
+		userdef_exactmap = 1;
 		return 0;
 	}
 
@@ -1281,15 +1275,16 @@ static int __init parse_memmap_opt(char
 	userdef = 1;
 	if (*p == '@') {
 		start_at = memparse(p+1, &p);
-		e820_add_region(start_at, mem_size, E820_RAM);
+		__e820_add_region(&e820_user, 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(&e820_user, 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(&e820_user, start_at, mem_size,
+			 E820_RESERVED);
 	} else
-		e820_remove_range(mem_size, ULLONG_MAX - mem_size, E820_RAM, 1);
+		userdef_mem_size = mem_size;
 
 	return *p == '\0' ? 0 : -EINVAL;
 }
@@ -1298,7 +1293,30 @@ early_param("memmap", parse_memmap_opt);
 void __init finish_e820_parsing(void)
 {
 	if (userdef) {
-		u32 nr = e820.nr_map;
+		u32 nr;
+
+		if (userdef_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
+			/* good, just use e820_user instead */
+			memcpy(&e820, &e820_user, sizeof(struct e820map));
+		} else {
+			/* ok, need to append e820_user to e820 */
+			__append_e820_map(e820_user.map, e820_user.nr_map);
+		}
+
+		nr = e820.nr_map;
+
+		if (userdef_mem_size)
+			e820_remove_range(userdef_mem_size,
+					  ULLONG_MAX - userdef_mem_size,
+					  E820_RAM, 1);
 
 		if (sanitize_e820_map(e820.map, ARRAY_SIZE(e820.map), &nr) < 0)
 			early_panic("Invalid user supplied memory map");
Index: linux-2.6/arch/x86/kernel/setup.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/setup.c
+++ linux-2.6/arch/x86/kernel/setup.c
@@ -693,13 +693,28 @@ void __init setup_arch(char **cmdline_p)
 #ifdef CONFIG_X86_32
 	memcpy(&boot_cpu_data, &new_cpu_data, sizeof(new_cpu_data));
 	visws_early_detect();
+#endif
+
+#ifdef CONFIG_CMDLINE_BOOL
+#ifdef CONFIG_CMDLINE_OVERRIDE
+	strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
 #else
-	printk(KERN_INFO "Command line: %s\n", boot_command_line);
+	if (builtin_cmdline[0]) {
+		/* append boot loader cmdline to builtin */
+		strlcat(builtin_cmdline, " ", COMMAND_LINE_SIZE);
+		strlcat(builtin_cmdline, boot_command_line, COMMAND_LINE_SIZE);
+		strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
+	}
+#endif
 #endif
 
+	printk(KERN_INFO "Command line: %s\n", boot_command_line);
+
 	strlcpy(command_line, boot_command_line, COMMAND_LINE_SIZE);
 	*cmdline_p = command_line;
 
+	early_cpu_init();
+
 #ifdef CONFIG_X86_64
 	/*
 	 * Must call this twice: Once just to detect whether hardware doesn't
@@ -712,10 +727,13 @@ void __init setup_arch(char **cmdline_p)
 
 	parse_early_param();
 
+#ifdef CONFIG_X86_64
+	check_efer();
+#endif
+
 	/* VMI may relocate the fixmap; do this before touching ioremap area */
 	vmi_init();
 
-	early_cpu_init();
 	early_ioremap_init();
 
 	ROOT_DEV = old_decode_dev(boot_params.hdr.root_dev);
@@ -781,23 +799,6 @@ void __init setup_arch(char **cmdline_p)
 	bss_resource.start = virt_to_phys(&__bss_start);
 	bss_resource.end = virt_to_phys(&__bss_stop)-1;
 
-#ifdef CONFIG_CMDLINE_BOOL
-#ifdef CONFIG_CMDLINE_OVERRIDE
-	strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
-#else
-	if (builtin_cmdline[0]) {
-		/* append boot loader cmdline to builtin */
-		strlcat(builtin_cmdline, " ", COMMAND_LINE_SIZE);
-		strlcat(builtin_cmdline, boot_command_line, COMMAND_LINE_SIZE);
-		strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
-	}
-#endif
-#endif
-
-#ifdef CONFIG_X86_64
-	check_efer();
-#endif
-
 	/* Must be before kernel pagetables are setup */
 	vmi_activate();
 

Re: linux-next: reservetop fix disables mem=

[Hugh Dickins]

On Mon, 24 Aug 2009, Ingo Molnar wrote:
> * Yinghai Lu <yinghai@kernel.org> wrote:
> > Hugh Dickins wrote:
> > > I find the "mem=" boot parameter disabled in today's linux-next:
> > > reverting the tip commit below fixes that.
> > > 
> > > Hugh
> > > 
> > > From: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
> > > Date: Thu, 20 Aug 2009 12:23:11 +0000 (+0800)
> > > Subject: x86: Fix system crash when loading with "reservetop" parameter
> > > X-Git-Url: http://git.kernel.org/?p=linux%2Fkernel%2Fgit%2Fmingo%2Flinux-2.6-x86.git;a=commitdiff_plain;h=8126dec32738421afa362114337331337b4be17f
> > > 
> > > x86: Fix system crash when loading with "reservetop" parameter
> > > 
> > > The system will die if the kernel is booted with "reservetop"
> > > parameter, in present code, parse "reservetop" parameter after
> > > early_ioremap_init(), and some function still use
> > > early_ioremap() after it.
> > > 
> > > The problem is, "reservetop" parameter can modify
> > > 'FIXADDR_TOP', then the virtual address got by early_ioremap()
> > > is base on old 'FIXADDR_TOP', but the page mapping is base on
> > > new 'FIXADDR_TOP', it will occur page fault, and the IDT is not
> > > prepare yet, so, the system is dead.
> > > 
> > > So, put parse_early_param() in the front of
> > > early_ioremap_init() in this patch.
> > > 
> > > Signed-off-by: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
> > > Cc: yinghai@kernel.org
> > > Cc: Andrew Morton <akpm@linux-foundation.org>
> > > LKML-Reference: <4A8D402F.4080805@cn.fujitsu.com>
> > > Signed-off-by: Ingo Molnar <mingo@elte.hu>
> > > ---
> > > 
> > > diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> > > index 63f32d2..02643cc 100644
> > > --- a/arch/x86/kernel/setup.c
> > > +++ b/arch/x86/kernel/setup.c
> > > @@ -711,6 +711,11 @@ void __init setup_arch(char **cmdline_p)
> > >  	printk(KERN_INFO "Command line: %s\n", boot_command_line);
> > >  #endif
> > >  
> > > +	strlcpy(command_line, boot_command_line, COMMAND_LINE_SIZE);
> > > +	*cmdline_p = command_line;
> > > +
> > > +	parse_early_param();
> > > +
> > >  	/* VMI may relocate the fixmap; do this before touching ioremap area */
> > >  	vmi_init();
> > >  
> > > @@ -793,11 +798,6 @@ void __init setup_arch(char **cmdline_p)
> > >  #endif
> > >  #endif
> > >  
> > > -	strlcpy(command_line, boot_command_line, COMMAND_LINE_SIZE);
> > > -	*cmdline_p = command_line;
> > > -
> > > -	parse_early_param();
> > > -
> > >  #ifdef CONFIG_X86_64
> > >  	check_efer();
> > >  #endif
> > 
> > yes, that patch will break other built-in command too.
> > 
> > need drop that patch.
> 
> done. Was nervous about the patch already:

Somehow it seems to have undropped itself, so mem= stopped working
again in recent mmotm/linux-next; and it looks like today the
undropped patch has made its way to Linus - I've not built current
git to check, but I think you'll find mem= is now broken there.

Hugh

> 
>   http://lkml.org/lkml/2009/8/21/127
> 
> > also the problem was caused by vmi patch, and that commit should 
> > be reverted.
> >
> > commit ae8d04e2ecbb233926860e9ce145eac19c7835dc
> > Author: Zachary Amsden <zach@vmware.com>
> > Date:   Sat Dec 13 12:36:58 2008 -0800
> > 
> >     x86 Fix VMI crash on boot in 2.6.28-rc8
> >     
> >     VMI initialiation can relocate the fixmap, causing early_ioremap to
> >     malfunction if it is initialized before the relocation.  To fix this,
> >     VMI activation is split into two phases; the detection, which must
> >     happen before setting up ioremap, and the activation, which must happen
> >     after parsing early boot parameters.
> >     
> >     This fixes a crash on boot when VMI is enabled under VMware.
> >     
> >     Signed-off-by: Zachary Amsden <zach@vmware.com>
> >     Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> > 
> > diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> > index 9d5674f..bdec76e 100644
> > --- a/arch/x86/kernel/setup.c
> > +++ b/arch/x86/kernel/setup.c
> > @@ -794,6 +794,9 @@ void __init setup_arch(char **cmdline_p)
> >         printk(KERN_INFO "Command line: %s\n", boot_command_line);
> >  #endif
> >  
> > +       /* VMI may relocate the fixmap; do this before touching ioremap area */
> > +       vmi_init();
> > +
> >         early_cpu_init();
> >         early_ioremap_init();
> >  
> > @@ -880,13 +883,8 @@ void __init setup_arch(char **cmdline_p)
> >         check_efer();
> >  #endif
> >  
> > -#if defined(CONFIG_VMI) && defined(CONFIG_X86_32)
> > -       /*
> > -        * Must be before kernel pagetables are setup
> > -        * or fixmap area is touched.
> > -        */
> > -       vmi_init();
> > -#endif
> > +       /* Must be before kernel pagetables are setup */
> > +       vmi_activate();
> >  
> >         /* after early param, so could get panic from serial */
> >         reserve_early_setup_data();
> > 
> > 
> > and according to
> > http://lkml.org/lkml/2008/12/10/388
> > http://lkml.org/lkml/2008/12/10/456
> > 
> > Zachary should split reserve_top_address() to two functions... 
> > before sending that patch to Linus
> 
> mind looking at this yourself if interested? Zachary has not been 
> active for quite some time.
> 
> 	Ingo

Re: linux-next: reservetop fix disables mem=

[Ingo Molnar]

* Hugh Dickins <hugh.dickins@tiscali.co.uk> wrote:

> On Mon, 24 Aug 2009, Ingo Molnar wrote:
> > * Yinghai Lu <yinghai@kernel.org> wrote:
> > > Hugh Dickins wrote:
> > > > I find the "mem=" boot parameter disabled in today's linux-next:
> > > > reverting the tip commit below fixes that.
> > > > 
> > > > Hugh
> > > > 
> > > > From: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
> > > > Date: Thu, 20 Aug 2009 12:23:11 +0000 (+0800)
> > > > Subject: x86: Fix system crash when loading with "reservetop" parameter
> > > > X-Git-Url: http://git.kernel.org/?p=linux%2Fkernel%2Fgit%2Fmingo%2Flinux-2.6-x86.git;a=commitdiff_plain;h=8126dec32738421afa362114337331337b4be17f
> > > > 
> > > > x86: Fix system crash when loading with "reservetop" parameter
> > > > 
> > > > The system will die if the kernel is booted with "reservetop"
> > > > parameter, in present code, parse "reservetop" parameter after
> > > > early_ioremap_init(), and some function still use
> > > > early_ioremap() after it.
> > > > 
> > > > The problem is, "reservetop" parameter can modify
> > > > 'FIXADDR_TOP', then the virtual address got by early_ioremap()
> > > > is base on old 'FIXADDR_TOP', but the page mapping is base on
> > > > new 'FIXADDR_TOP', it will occur page fault, and the IDT is not
> > > > prepare yet, so, the system is dead.
> > > > 
> > > > So, put parse_early_param() in the front of
> > > > early_ioremap_init() in this patch.
> > > > 
> > > > Signed-off-by: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
> > > > Cc: yinghai@kernel.org
> > > > Cc: Andrew Morton <akpm@linux-foundation.org>
> > > > LKML-Reference: <4A8D402F.4080805@cn.fujitsu.com>
> > > > Signed-off-by: Ingo Molnar <mingo@elte.hu>
> > > > ---
> > > > 
> > > > diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> > > > index 63f32d2..02643cc 100644
> > > > --- a/arch/x86/kernel/setup.c
> > > > +++ b/arch/x86/kernel/setup.c
> > > > @@ -711,6 +711,11 @@ void __init setup_arch(char **cmdline_p)
> > > >  	printk(KERN_INFO "Command line: %s\n", boot_command_line);
> > > >  #endif
> > > >  
> > > > +	strlcpy(command_line, boot_command_line, COMMAND_LINE_SIZE);
> > > > +	*cmdline_p = command_line;
> > > > +
> > > > +	parse_early_param();
> > > > +
> > > >  	/* VMI may relocate the fixmap; do this before touching ioremap area */
> > > >  	vmi_init();
> > > >  
> > > > @@ -793,11 +798,6 @@ void __init setup_arch(char **cmdline_p)
> > > >  #endif
> > > >  #endif
> > > >  
> > > > -	strlcpy(command_line, boot_command_line, COMMAND_LINE_SIZE);
> > > > -	*cmdline_p = command_line;
> > > > -
> > > > -	parse_early_param();
> > > > -
> > > >  #ifdef CONFIG_X86_64
> > > >  	check_efer();
> > > >  #endif
> > > 
> > > yes, that patch will break other built-in command too.
> > > 
> > > need drop that patch.
> > 
> > done. Was nervous about the patch already:
> 
> Somehow it seems to have undropped itself, so mem= stopped working 
> again in recent mmotm/linux-next; and it looks like today the 
> undropped patch has made its way to Linus - I've not built current git 
> to check, but I think you'll find mem= is now broken there.

I thoroughly zapped it. Do you know about any commit ID where it snuck 
in?

	Ingo

Re: linux-next: reservetop fix disables mem=

[Yinghai Lu]

Ingo Molnar wrote:
> I thoroughly zapped it. Do you know about any commit ID where it snuck 
> in?

after close looking, it will break
1. some cpu feature  in early stage too, like cpu_has_x2apic
2. will break built-in-command line
3. will break other memmap= and mem=
4. early_dbgp and early_console that will use early_ioremap to access mmio (?)

YH

commit 8126dec32738421afa362114337331337b4be17f
Author: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
Date:   Thu Aug 20 20:23:11 2009 +0800

    x86: Fix system crash when loading with "reservetop" parameter
    
    The system will die if the kernel is booted with "reservetop"
    parameter, in present code, parse "reservetop" parameter after
    early_ioremap_init(), and some function still use
    early_ioremap() after it.
    
    The problem is, "reservetop" parameter can modify
    'FIXADDR_TOP', then the virtual address got by early_ioremap()
    is base on old 'FIXADDR_TOP', but the page mapping is base on
    new 'FIXADDR_TOP', it will occur page fault, and the IDT is not
    prepare yet, so, the system is dead.
    
    So, put parse_early_param() in the front of
    early_ioremap_init() in this patch.
    
    Signed-off-by: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
    Cc: yinghai@kernel.org
    Cc: Andrew Morton <akpm@linux-foundation.org>
    LKML-Reference: <4A8D402F.4080805@cn.fujitsu.com>
    Signed-off-by: Ingo Molnar <mingo@elte.hu>

diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 63f32d2..02643cc 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -711,6 +711,11 @@ void __init setup_arch(char **cmdline_p)
        printk(KERN_INFO "Command line: %s\n", boot_command_line);
 #endif
 
+       strlcpy(command_line, boot_command_line, COMMAND_LINE_SIZE);
+       *cmdline_p = command_line;
+
+       parse_early_param();
+
        /* VMI may relocate the fixmap; do this before touching ioremap area */
        vmi_init();
 
@@ -793,11 +798,6 @@ void __init setup_arch(char **cmdline_p)
 #endif
 #endif
 
-       strlcpy(command_line, boot_command_line, COMMAND_LINE_SIZE);
-       *cmdline_p = command_line;
-
-       parse_early_param();
-
 #ifdef CONFIG_X86_64
        check_efer();
 #endif

Re: linux-next: reservetop fix disables mem=

[Yinghai Lu]

Yinghai Lu wrote:
> Ingo Molnar wrote:
>> I thoroughly zapped it. Do you know about any commit ID where it snuck 
>> in?
> 
> after close looking, it will break
> 1. some cpu feature  in early stage too, like cpu_has_x2apic
> 2. will break built-in-command line
> 3. will break other memmap= and mem=
> 4. early_dbgp and early_console that will use early_ioremap to access mmio (?)
> 
> YH
> 
> commit 8126dec32738421afa362114337331337b4be17f
> Author: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
> Date:   Thu Aug 20 20:23:11 2009 +0800
> 
>     x86: Fix system crash when loading with "reservetop" parameter
>     
>     The system will die if the kernel is booted with "reservetop"
>     parameter, in present code, parse "reservetop" parameter after
>     early_ioremap_init(), and some function still use
>     early_ioremap() after it.
>     
>     The problem is, "reservetop" parameter can modify
>     'FIXADDR_TOP', then the virtual address got by early_ioremap()
>     is base on old 'FIXADDR_TOP', but the page mapping is base on
>     new 'FIXADDR_TOP', it will occur page fault, and the IDT is not
>     prepare yet, so, the system is dead.
>     
>     So, put parse_early_param() in the front of
>     early_ioremap_init() in this patch.
>     
>     Signed-off-by: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
>     Cc: yinghai@kernel.org
>     Cc: Andrew Morton <akpm@linux-foundation.org>
>     LKML-Reference: <4A8D402F.4080805@cn.fujitsu.com>
>     Signed-off-by: Ingo Molnar <mingo@elte.hu>
> 
after revet it, it seem mem=32g etc still broken, maybe some other commit about x86 platform support from Thomas broke it.

YH

---
 arch/x86/kernel/setup.c |   30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

Index: linux-2.6/arch/x86/kernel/setup.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/setup.c
+++ linux-2.6/arch/x86/kernel/setup.c
@@ -697,21 +697,6 @@ void __init setup_arch(char **cmdline_p)
 	printk(KERN_INFO "Command line: %s\n", boot_command_line);
 #endif
 
-	strlcpy(command_line, boot_command_line, COMMAND_LINE_SIZE);
-	*cmdline_p = command_line;
-
-#ifdef CONFIG_X86_64
-	/*
-	 * Must call this twice: Once just to detect whether hardware doesn't
-	 * support NX (so that the early EHCI debug console setup can safely
-	 * call set_fixmap(), and then again after parsing early parameters to
-	 * honor the respective command line option.
-	 */
-	check_efer();
-#endif
-
-	parse_early_param();
-
 	/* VMI may relocate the fixmap; do this before touching ioremap area */
 	vmi_init();
 
@@ -794,6 +779,21 @@ void __init setup_arch(char **cmdline_p)
 #endif
 #endif
 
+	strlcpy(command_line, boot_command_line, COMMAND_LINE_SIZE);
+	*cmdline_p = command_line;
+
+#ifdef CONFIG_X86_64
+	/*
+	 * Must call this twice: Once just to detect whether hardware doesn't
+	 * support NX (so that the early EHCI debug console setup can safely
+	 * call set_fixmap(), and then again after parsing early parameters to
+	 * honor the respective command line option.
+	 */
+	check_efer();
+#endif
+
+	parse_early_param();
+
 #ifdef CONFIG_X86_64
 	check_efer();
 #endif

Re: linux-next: reservetop fix disables mem=

[Ingo Molnar]

* Yinghai Lu <yinghai@kernel.org> wrote:

> Yinghai Lu wrote:
> > Ingo Molnar wrote:
> >> I thoroughly zapped it. Do you know about any commit ID where it snuck 
> >> in?
> > 
> > after close looking, it will break
> > 1. some cpu feature  in early stage too, like cpu_has_x2apic
> > 2. will break built-in-command line
> > 3. will break other memmap= and mem=
> > 4. early_dbgp and early_console that will use early_ioremap to access mmio (?)

thanks Yinghai - i have queued up your revert. Thanks Hugh for noticing 
it!

> after revet it, it seem mem=32g etc still broken, maybe some other 
> commit about x86 platform support from Thomas broke it.

Thomas?

	Ingo

[tip:x86/urgent] Revert 'x86: Fix system crash when loading with "reservetop" parameter'

[tip-bot for Yinghai Lu]

Commit-ID:  eda6da9286ad5b35b1eb70f6368958a8ee41a9dd
Gitweb:     http://git.kernel.org/tip/eda6da9286ad5b35b1eb70f6368958a8ee41a9dd
Author:     Yinghai Lu <yinghai@kernel.org>
AuthorDate: Sat, 19 Sep 2009 11:07:57 -0700
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Sat, 19 Sep 2009 20:31:33 +0200

Revert 'x86: Fix system crash when loading with "reservetop" parameter'

After close looking, commit 8126dec3 will break:

 1. some cpu feature  in early stage too, like cpu_has_x2apic
 2. will break built-in-command line
 3. will break other memmap= and mem=
 4. early_dbgp and early_console that will use early_ioremap to access mmio (?)

So revert it.

Reported-by: Hugh Dickins <hugh.dickins@tiscali.co.uk>,
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
Cc: Andrew Morton <akpm@linux-foundation.org>,
LKML-Reference: <4AB51DFD.2000904@kernel.org>
Signed-off-by: Ingo Molnar <mingo@elte.hu>


---
 arch/x86/kernel/setup.c |   30 +++++++++++++++---------------
 1 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 19f15c4..f5baa2a 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -712,21 +712,6 @@ void __init setup_arch(char **cmdline_p)
 	printk(KERN_INFO "Command line: %s\n", boot_command_line);
 #endif
 
-	strlcpy(command_line, boot_command_line, COMMAND_LINE_SIZE);
-	*cmdline_p = command_line;
-
-#ifdef CONFIG_X86_64
-	/*
-	 * Must call this twice: Once just to detect whether hardware doesn't
-	 * support NX (so that the early EHCI debug console setup can safely
-	 * call set_fixmap(), and then again after parsing early parameters to
-	 * honor the respective command line option.
-	 */
-	check_efer();
-#endif
-
-	parse_early_param();
-
 	/* VMI may relocate the fixmap; do this before touching ioremap area */
 	vmi_init();
 
@@ -809,6 +794,21 @@ void __init setup_arch(char **cmdline_p)
 #endif
 #endif
 
+	strlcpy(command_line, boot_command_line, COMMAND_LINE_SIZE);
+	*cmdline_p = command_line;
+
+#ifdef CONFIG_X86_64
+	/*
+	 * Must call this twice: Once just to detect whether hardware doesn't
+	 * support NX (so that the early EHCI debug console setup can safely
+	 * call set_fixmap(), and then again after parsing early parameters to
+	 * honor the respective command line option.
+	 */
+	check_efer();
+#endif
+
+	parse_early_param();
+
 #ifdef CONFIG_X86_64
 	check_efer();
 #endif