Re: + x86-fix-handling-of-the-reservetop-boot-option.patch added to -mm tree

Re: + x86-fix-handling-of-the-reservetop-boot-option.patch added to -mm tree

[Yinghai]

On 04/07/2010 03:00 PM, akpm@linux-foundation.org wrote:
> The patch titled
>      x86: fix handling of the 'reservetop' boot option
> has been added to the -mm tree.  Its filename is
>      x86-fix-handling-of-the-reservetop-boot-option.patch
> 
> Before you just go and hit "reply", please:
>    a) Consider who else should be cc'ed
>    b) Prefer to cc a suitable mailing list as well
>    c) Ideally: find the original patch on the mailing list and do a
>       reply-to-all to that, adding suitable additional cc's
> 
> *** Remember to use Documentation/SubmitChecklist when testing your code ***
> 
> See http://userweb.kernel.org/~akpm/stuff/added-to-mm.txt to find
> out what to do about this
> 
> The current -mm tree may be found at http://userweb.kernel.org/~akpm/mmotm/
> 
> ------------------------------------------------------
> Subject: x86: fix handling of the 'reservetop' boot option
> From: Liang Li <liang.li@windriver.com>
> 
> When specifying the 'reservetop=0xbadc0de' kernel parameter, the kernel
> will stop booting due to a early_ioremap bug that relate to commit
> 8827247ff ("x86: don't define __this_fixmap_does_not_exist()").
> 
> The root cause of boot failure problem is the value of 'slot_virt[i]' was
> initialized in setup_arch->early_ioremap_init.  But later in setup_arch,
> the function 'parse_early_param' will modify 'FIXADDR_TOP' when
> 'reservetop=0xbadc0de' being specified.
> 
> The simplest fix might be use __fix_to_virt(idx0) to get updated value
> of 'FIXADDR_TOP' in '__early_ioremap' instead of reference old value
> from slot_virt[slot] directly.
> 
> Signed-off-by: Liang Li <liang.li@windriver.com>
> Cc: Wang Chen <wangchen@cn.fujitsu.com>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Yinghai Lu <yinghai@kernel.org>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>  arch/x86/mm/ioremap.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff -puN arch/x86/mm/ioremap.c~x86-fix-handling-of-the-reservetop-boot-option arch/x86/mm/ioremap.c
> --- a/arch/x86/mm/ioremap.c~x86-fix-handling-of-the-reservetop-boot-option
> +++ a/arch/x86/mm/ioremap.c
> @@ -537,9 +537,9 @@ __early_ioremap(resource_size_t phys_add
>  		--nrpages;
>  	}
>  	if (early_ioremap_debug)
> -		printk(KERN_CONT "%08lx + %08lx\n", offset, slot_virt[slot]);
> +		printk(KERN_CONT "%08lx + %08lx\n", offset, __fix_to_virt(idx0));
>  
> -	prev_map[slot] = (void __iomem *)(offset + slot_virt[slot]);
> +	prev_map[slot] = (void __iomem *)(offset + __fix_to_virt(idx0));
>  	return prev_map[slot];
>  }
>  
> _

not that simple. but it looks like correct direction.

please consider:
when early_parsing reserve_top, double check if there is left over in prev_map[], and 
reinitialize slot_virt[] and clear old PMD and  setup new PMD if needed.

Thanks

Yinghai

Re: + x86-fix-handling-of-the-reservetop-boot-option.patch added to -mm tree

[Liang Li]

On Wed, Apr 07, 2010 at 04:52:03PM -0700, Yinghai wrote:
> On 04/07/2010 03:00 PM, akpm@linux-foundation.org wrote:
> > The patch titled
> >      x86: fix handling of the 'reservetop' boot option
> > has been added to the -mm tree.  Its filename is
> >      x86-fix-handling-of-the-reservetop-boot-option.patch
> > 
> > Before you just go and hit "reply", please:
> >    a) Consider who else should be cc'ed
> >    b) Prefer to cc a suitable mailing list as well
> >    c) Ideally: find the original patch on the mailing list and do a
> >       reply-to-all to that, adding suitable additional cc's
> > 
> > *** Remember to use Documentation/SubmitChecklist when testing your code ***
> > 
> > See http://userweb.kernel.org/~akpm/stuff/added-to-mm.txt to find
> > out what to do about this
> > 
> > The current -mm tree may be found at http://userweb.kernel.org/~akpm/mmotm/
> > 
> > ------------------------------------------------------
> > Subject: x86: fix handling of the 'reservetop' boot option
> > From: Liang Li <liang.li@windriver.com>
> > 
> > When specifying the 'reservetop=0xbadc0de' kernel parameter, the kernel
> > will stop booting due to a early_ioremap bug that relate to commit
> > 8827247ff ("x86: don't define __this_fixmap_does_not_exist()").
> > 
> > The root cause of boot failure problem is the value of 'slot_virt[i]' was
> > initialized in setup_arch->early_ioremap_init.  But later in setup_arch,
> > the function 'parse_early_param' will modify 'FIXADDR_TOP' when
> > 'reservetop=0xbadc0de' being specified.
> > 
> > The simplest fix might be use __fix_to_virt(idx0) to get updated value
> > of 'FIXADDR_TOP' in '__early_ioremap' instead of reference old value
> > from slot_virt[slot] directly.
> > 
> > Signed-off-by: Liang Li <liang.li@windriver.com>
> > Cc: Wang Chen <wangchen@cn.fujitsu.com>
> > Cc: Ingo Molnar <mingo@elte.hu>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: "H. Peter Anvin" <hpa@zytor.com>
> > Cc: Yinghai Lu <yinghai@kernel.org>
> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> > ---
> > 
> >  arch/x86/mm/ioremap.c |    4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff -puN arch/x86/mm/ioremap.c~x86-fix-handling-of-the-reservetop-boot-option arch/x86/mm/ioremap.c
> > --- a/arch/x86/mm/ioremap.c~x86-fix-handling-of-the-reservetop-boot-option
> > +++ a/arch/x86/mm/ioremap.c
> > @@ -537,9 +537,9 @@ __early_ioremap(resource_size_t phys_add
> >  		--nrpages;
> >  	}
> >  	if (early_ioremap_debug)
> > -		printk(KERN_CONT "%08lx + %08lx\n", offset, slot_virt[slot]);
> > +		printk(KERN_CONT "%08lx + %08lx\n", offset, __fix_to_virt(idx0));
> >  
> > -	prev_map[slot] = (void __iomem *)(offset + slot_virt[slot]);
> > +	prev_map[slot] = (void __iomem *)(offset + __fix_to_virt(idx0));
> >  	return prev_map[slot];
> >  }
> >  
> > _
> 
> not that simple. but it looks like correct direction.
> 
> please consider:
> when early_parsing reserve_top, double check if there is left over in prev_map[], and 
> reinitialize slot_virt[] and clear old PMD and  setup new PMD if needed.

Hi Yinghai,

Thanks for your reply, its better to have eyes on then being ignored. :)

Your suggestions were considered before the patch to public, let me try
to explain:

#1 check/adjust prev_map[]?
In my tests, seems early_ioremap is untouched between early_ioremap_init
and parse_early_param so I did not check prev_map. Even its get touched,
I think we could do nothing to this mapping, since prev_map[i] just
record virt addr for clients of early_ioremap. We can check and adjust
prev_map but clients of early_ioremap won't realize the fact so nothing
being fixed or broken.

#2 reinitialize slot_virt and update PMD
I actually tried this approach, call early_ioremap_init again after
parse_early_param will do that work, it also works but I am not sure
that is the better solution or too heavy for solve the problem? So I
tend to say 'simplest' solution in git commit log.

Thanks and best regards,
		-Liang Li

Re: + x86-fix-handling-of-the-reservetop-boot-option.patch added to -mm tree

[Yinghai]

On 04/07/2010 06:05 PM, Liang Li wrote:
> On Wed, Apr 07, 2010 at 04:52:03PM -0700, Yinghai wrote:
>> On 04/07/2010 03:00 PM, akpm@linux-foundation.org wrote:
>>> The patch titled
>>>      x86: fix handling of the 'reservetop' boot option
>>> has been added to the -mm tree.  Its filename is
>>>      x86-fix-handling-of-the-reservetop-boot-option.patch
>>>
>>> Before you just go and hit "reply", please:
>>>    a) Consider who else should be cc'ed
>>>    b) Prefer to cc a suitable mailing list as well
>>>    c) Ideally: find the original patch on the mailing list and do a
>>>       reply-to-all to that, adding suitable additional cc's
>>>
>>> *** Remember to use Documentation/SubmitChecklist when testing your code ***
>>>
>>> See http://userweb.kernel.org/~akpm/stuff/added-to-mm.txt to find
>>> out what to do about this
>>>
>>> The current -mm tree may be found at http://userweb.kernel.org/~akpm/mmotm/
>>>
>>> ------------------------------------------------------
>>> Subject: x86: fix handling of the 'reservetop' boot option
>>> From: Liang Li <liang.li@windriver.com>
>>>
>>> When specifying the 'reservetop=0xbadc0de' kernel parameter, the kernel
>>> will stop booting due to a early_ioremap bug that relate to commit
>>> 8827247ff ("x86: don't define __this_fixmap_does_not_exist()").
>>>
>>> The root cause of boot failure problem is the value of 'slot_virt[i]' was
>>> initialized in setup_arch->early_ioremap_init.  But later in setup_arch,
>>> the function 'parse_early_param' will modify 'FIXADDR_TOP' when
>>> 'reservetop=0xbadc0de' being specified.
>>>
>>> The simplest fix might be use __fix_to_virt(idx0) to get updated value
>>> of 'FIXADDR_TOP' in '__early_ioremap' instead of reference old value
>>> from slot_virt[slot] directly.
>>>
>>> Signed-off-by: Liang Li <liang.li@windriver.com>
>>> Cc: Wang Chen <wangchen@cn.fujitsu.com>
>>> Cc: Ingo Molnar <mingo@elte.hu>
>>> Cc: Thomas Gleixner <tglx@linutronix.de>
>>> Cc: "H. Peter Anvin" <hpa@zytor.com>
>>> Cc: Yinghai Lu <yinghai@kernel.org>
>>> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
>>> ---
>>>
>>>  arch/x86/mm/ioremap.c |    4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff -puN arch/x86/mm/ioremap.c~x86-fix-handling-of-the-reservetop-boot-option arch/x86/mm/ioremap.c
>>> --- a/arch/x86/mm/ioremap.c~x86-fix-handling-of-the-reservetop-boot-option
>>> +++ a/arch/x86/mm/ioremap.c
>>> @@ -537,9 +537,9 @@ __early_ioremap(resource_size_t phys_add
>>>  		--nrpages;
>>>  	}
>>>  	if (early_ioremap_debug)
>>> -		printk(KERN_CONT "%08lx + %08lx\n", offset, slot_virt[slot]);
>>> +		printk(KERN_CONT "%08lx + %08lx\n", offset, __fix_to_virt(idx0));
>>>  
>>> -	prev_map[slot] = (void __iomem *)(offset + slot_virt[slot]);
>>> +	prev_map[slot] = (void __iomem *)(offset + __fix_to_virt(idx0));
>>>  	return prev_map[slot];
>>>  }
>>>  
>>> _
>>
>> not that simple. but it looks like correct direction.
>>
>> please consider:
>> when early_parsing reserve_top, double check if there is left over in prev_map[], and 
>> reinitialize slot_virt[] and clear old PMD and  setup new PMD if needed.
> 
> Hi Yinghai,
> 
> Thanks for your reply, its better to have eyes on then being ignored. :)
> 
> Your suggestions were considered before the patch to public, let me try
> to explain:
> 
> #1 check/adjust prev_map[]?
> In my tests, seems early_ioremap is untouched between early_ioremap_init
> and parse_early_param so I did not check prev_map. Even its get touched,
> I think we could do nothing to this mapping, since prev_map[i] just
> record virt addr for clients of early_ioremap. We can check and adjust
> prev_map but clients of early_ioremap won't realize the fact so nothing
> being fixed or broken.

efi related code need them

dmi 

you need to add bug_on if there is still have left over, and need the caller to re map it again later.

> 
> #2 reinitialize slot_virt and update PMD
> I actually tried this approach, call early_ioremap_init again after
> parse_early_param will do that work, it also works but I am not sure
> that is the better solution or too heavy for solve the problem? So I
> tend to say 'simplest' solution in git commit log.

how about PMD? you don't need set PMD again.

YH

Re: + x86-fix-handling-of-the-reservetop-boot-option.patch added to -mm tree

[Liang Li]

On Wed, Apr 07, 2010 at 06:13:56PM -0700, Yinghai wrote:
> On 04/07/2010 06:05 PM, Liang Li wrote:
> > On Wed, Apr 07, 2010 at 04:52:03PM -0700, Yinghai wrote:
> >> On 04/07/2010 03:00 PM, akpm@linux-foundation.org wrote:
> >>> The patch titled
> >>>      x86: fix handling of the 'reservetop' boot option
> >>> has been added to the -mm tree.  Its filename is
> >>>      x86-fix-handling-of-the-reservetop-boot-option.patch
> >>>
> >>> Before you just go and hit "reply", please:
> >>>    a) Consider who else should be cc'ed
> >>>    b) Prefer to cc a suitable mailing list as well
> >>>    c) Ideally: find the original patch on the mailing list and do a
> >>>       reply-to-all to that, adding suitable additional cc's
> >>>
> >>> *** Remember to use Documentation/SubmitChecklist when testing your code ***
> >>>
> >>> See http://userweb.kernel.org/~akpm/stuff/added-to-mm.txt to find
> >>> out what to do about this
> >>>
> >>> The current -mm tree may be found at http://userweb.kernel.org/~akpm/mmotm/
> >>>
> >>> ------------------------------------------------------
> >>> Subject: x86: fix handling of the 'reservetop' boot option
> >>> From: Liang Li <liang.li@windriver.com>
> >>>
> >>> When specifying the 'reservetop=0xbadc0de' kernel parameter, the kernel
> >>> will stop booting due to a early_ioremap bug that relate to commit
> >>> 8827247ff ("x86: don't define __this_fixmap_does_not_exist()").
> >>>
> >>> The root cause of boot failure problem is the value of 'slot_virt[i]' was
> >>> initialized in setup_arch->early_ioremap_init.  But later in setup_arch,
> >>> the function 'parse_early_param' will modify 'FIXADDR_TOP' when
> >>> 'reservetop=0xbadc0de' being specified.
> >>>
> >>> The simplest fix might be use __fix_to_virt(idx0) to get updated value
> >>> of 'FIXADDR_TOP' in '__early_ioremap' instead of reference old value
> >>> from slot_virt[slot] directly.
> >>>
> >>> Signed-off-by: Liang Li <liang.li@windriver.com>
> >>> Cc: Wang Chen <wangchen@cn.fujitsu.com>
> >>> Cc: Ingo Molnar <mingo@elte.hu>
> >>> Cc: Thomas Gleixner <tglx@linutronix.de>
> >>> Cc: "H. Peter Anvin" <hpa@zytor.com>
> >>> Cc: Yinghai Lu <yinghai@kernel.org>
> >>> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> >>> ---
> >>>
> >>>  arch/x86/mm/ioremap.c |    4 ++--
> >>>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff -puN arch/x86/mm/ioremap.c~x86-fix-handling-of-the-reservetop-boot-option arch/x86/mm/ioremap.c
> >>> --- a/arch/x86/mm/ioremap.c~x86-fix-handling-of-the-reservetop-boot-option
> >>> +++ a/arch/x86/mm/ioremap.c
> >>> @@ -537,9 +537,9 @@ __early_ioremap(resource_size_t phys_add
> >>>  		--nrpages;
> >>>  	}
> >>>  	if (early_ioremap_debug)
> >>> -		printk(KERN_CONT "%08lx + %08lx\n", offset, slot_virt[slot]);
> >>> +		printk(KERN_CONT "%08lx + %08lx\n", offset, __fix_to_virt(idx0));
> >>>  
> >>> -	prev_map[slot] = (void __iomem *)(offset + slot_virt[slot]);
> >>> +	prev_map[slot] = (void __iomem *)(offset + __fix_to_virt(idx0));
> >>>  	return prev_map[slot];
> >>>  }
> >>>  
> >>> _
> >>
> >> not that simple. but it looks like correct direction.
> >>
> >> please consider:
> >> when early_parsing reserve_top, double check if there is left over in prev_map[], and 
> >> reinitialize slot_virt[] and clear old PMD and  setup new PMD if needed.
> > 
> > Hi Yinghai,
> > 
> > Thanks for your reply, its better to have eyes on then being ignored. :)
> > 
> > Your suggestions were considered before the patch to public, let me try
> > to explain:
> > 
> > #1 check/adjust prev_map[]?
> > In my tests, seems early_ioremap is untouched between early_ioremap_init
> > and parse_early_param so I did not check prev_map. Even its get touched,
> > I think we could do nothing to this mapping, since prev_map[i] just
> > record virt addr for clients of early_ioremap. We can check and adjust
> > prev_map but clients of early_ioremap won't realize the fact so nothing
> > being fixed or broken.
> 
> efi related code need them
> 
> dmi 
> 
> you need to add bug_on if there is still have left over, and need the caller to re map it again later.
> 
> > 
> > #2 reinitialize slot_virt and update PMD
> > I actually tried this approach, call early_ioremap_init again after
> > parse_early_param will do that work, it also works but I am not sure
> > that is the better solution or too heavy for solve the problem? So I
> > tend to say 'simplest' solution in git commit log.
> 
> how about PMD? you don't need set PMD again.
> 
> YH

Hi Yinghai,

Does this similar modification like this is more preferred?

diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index a1dcfa3..30a3e97 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -347,6 +347,7 @@ extern void __iomem *early_ioremap(resource_size_t phys_addr,
 extern void __iomem *early_memremap(resource_size_t phys_addr,
 				    unsigned long size);
 extern void early_iounmap(void __iomem *addr, unsigned long size);
+extern void fixup_early_ioremap(void);
 
 #define IO_SPACE_LIMIT 0xffff
 
diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index ea82ef0..fe06296 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -448,6 +448,23 @@ static inline void __init early_clear_fixmap(enum fixed_addresses idx)
 static void __iomem *prev_map[FIX_BTMAPS_SLOTS] __initdata;
 static unsigned long prev_size[FIX_BTMAPS_SLOTS] __initdata;
 
+void __init fixup_early_ioremap(void)
+{
+	int i;
+	for (i = 0; i < FIX_BTMAPS_SLOTS; i++) {
+		if (prev_map[i])
+			break;
+	}
+
+	if (i == FIX_BTMAPS_SLOTS)
+		WARN_ON(1);
+
+	for (i = 0; i < FIX_BTMAPS_SLOTS; i++)
+		slot_virt[i] = __fix_to_virt(FIX_BTMAP_BEGIN - NR_FIX_BTMAPS * i);
+
+	return;
+}
+
 static int __init check_early_ioremap_leak(void)
 {
 	int count = 0;
diff --git a/arch/x86/mm/pgtable.c b/arch/x86/mm/pgtable.c
index 5c4ee42..ea4d54c 100644
--- a/arch/x86/mm/pgtable.c
+++ b/arch/x86/mm/pgtable.c
@@ -4,6 +4,7 @@
 #include <asm/pgtable.h>
 #include <asm/tlb.h>
 #include <asm/fixmap.h>
+#include <asm/io.h>
 
 #define PGALLOC_GFP GFP_KERNEL | __GFP_NOTRACK | __GFP_REPEAT | __GFP_ZERO
 
@@ -351,6 +352,7 @@ void __init reserve_top_address(unsigned long reserve)
 	printk(KERN_INFO "Reserving virtual address space above 0x%08x\n",
 	       (int)-reserve);
 	__FIXADDR_TOP = -reserve - PAGE_SIZE;
+	fixup_early_ioremap();
 #endif
 }
 

Thanks,
	-Liang Li

Re: + x86-fix-handling-of-the-reservetop-boot-option.patch added to -mm tree

[Liang Li]

On Thu, Apr 08, 2010 at 09:53:53AM +0800, Liang Li wrote:
> On Wed, Apr 07, 2010 at 06:13:56PM -0700, Yinghai wrote:
> > On 04/07/2010 06:05 PM, Liang Li wrote:
> > > On Wed, Apr 07, 2010 at 04:52:03PM -0700, Yinghai wrote:
> > >> On 04/07/2010 03:00 PM, akpm@linux-foundation.org wrote:
> > >>> The patch titled
> > >>>      x86: fix handling of the 'reservetop' boot option
> > >>> has been added to the -mm tree.  Its filename is
> > >>>      x86-fix-handling-of-the-reservetop-boot-option.patch
> > >>>
> > >>> Before you just go and hit "reply", please:
> > >>>    a) Consider who else should be cc'ed
> > >>>    b) Prefer to cc a suitable mailing list as well
> > >>>    c) Ideally: find the original patch on the mailing list and do a
> > >>>       reply-to-all to that, adding suitable additional cc's
> > >>>
> > >>> *** Remember to use Documentation/SubmitChecklist when testing your code ***
> > >>>
> > >>> See http://userweb.kernel.org/~akpm/stuff/added-to-mm.txt to find
> > >>> out what to do about this
> > >>>
> > >>> The current -mm tree may be found at http://userweb.kernel.org/~akpm/mmotm/
> > >>>
> > >>> ------------------------------------------------------
> > >>> Subject: x86: fix handling of the 'reservetop' boot option
> > >>> From: Liang Li <liang.li@windriver.com>
> > >>>
> > >>> When specifying the 'reservetop=0xbadc0de' kernel parameter, the kernel
> > >>> will stop booting due to a early_ioremap bug that relate to commit
> > >>> 8827247ff ("x86: don't define __this_fixmap_does_not_exist()").
> > >>>
> > >>> The root cause of boot failure problem is the value of 'slot_virt[i]' was
> > >>> initialized in setup_arch->early_ioremap_init.  But later in setup_arch,
> > >>> the function 'parse_early_param' will modify 'FIXADDR_TOP' when
> > >>> 'reservetop=0xbadc0de' being specified.
> > >>>
> > >>> The simplest fix might be use __fix_to_virt(idx0) to get updated value
> > >>> of 'FIXADDR_TOP' in '__early_ioremap' instead of reference old value
> > >>> from slot_virt[slot] directly.
> > >>>
> > >>> Signed-off-by: Liang Li <liang.li@windriver.com>
> > >>> Cc: Wang Chen <wangchen@cn.fujitsu.com>
> > >>> Cc: Ingo Molnar <mingo@elte.hu>
> > >>> Cc: Thomas Gleixner <tglx@linutronix.de>
> > >>> Cc: "H. Peter Anvin" <hpa@zytor.com>
> > >>> Cc: Yinghai Lu <yinghai@kernel.org>
> > >>> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> > >>> ---
> > >>>
> > >>>  arch/x86/mm/ioremap.c |    4 ++--
> > >>>  1 file changed, 2 insertions(+), 2 deletions(-)
> > >>>
> > >>> diff -puN arch/x86/mm/ioremap.c~x86-fix-handling-of-the-reservetop-boot-option arch/x86/mm/ioremap.c
> > >>> --- a/arch/x86/mm/ioremap.c~x86-fix-handling-of-the-reservetop-boot-option
> > >>> +++ a/arch/x86/mm/ioremap.c
> > >>> @@ -537,9 +537,9 @@ __early_ioremap(resource_size_t phys_add
> > >>>  		--nrpages;
> > >>>  	}
> > >>>  	if (early_ioremap_debug)
> > >>> -		printk(KERN_CONT "%08lx + %08lx\n", offset, slot_virt[slot]);
> > >>> +		printk(KERN_CONT "%08lx + %08lx\n", offset, __fix_to_virt(idx0));
> > >>>  
> > >>> -	prev_map[slot] = (void __iomem *)(offset + slot_virt[slot]);
> > >>> +	prev_map[slot] = (void __iomem *)(offset + __fix_to_virt(idx0));
> > >>>  	return prev_map[slot];
> > >>>  }
> > >>>  
> > >>> _
> > >>
> > >> not that simple. but it looks like correct direction.
> > >>
> > >> please consider:
> > >> when early_parsing reserve_top, double check if there is left over in prev_map[], and 
> > >> reinitialize slot_virt[] and clear old PMD and  setup new PMD if needed.
> > > 
> > > Hi Yinghai,
> > > 
> > > Thanks for your reply, its better to have eyes on then being ignored. :)
> > > 
> > > Your suggestions were considered before the patch to public, let me try
> > > to explain:
> > > 
> > > #1 check/adjust prev_map[]?
> > > In my tests, seems early_ioremap is untouched between early_ioremap_init
> > > and parse_early_param so I did not check prev_map. Even its get touched,
> > > I think we could do nothing to this mapping, since prev_map[i] just
> > > record virt addr for clients of early_ioremap. We can check and adjust
> > > prev_map but clients of early_ioremap won't realize the fact so nothing
> > > being fixed or broken.
> > 
> > efi related code need them
> > 
> > dmi 
> > 
> > you need to add bug_on if there is still have left over, and need the caller to re map it again later.
> > 
> > > 
> > > #2 reinitialize slot_virt and update PMD
> > > I actually tried this approach, call early_ioremap_init again after
> > > parse_early_param will do that work, it also works but I am not sure
> > > that is the better solution or too heavy for solve the problem? So I
> > > tend to say 'simplest' solution in git commit log.
> > 
> > how about PMD? you don't need set PMD again.
> > 
> > YH
> 
> Hi Yinghai,
> 
> Does this similar modification like this is more preferred?
> 
> diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
> index a1dcfa3..30a3e97 100644
> --- a/arch/x86/include/asm/io.h
> +++ b/arch/x86/include/asm/io.h
> @@ -347,6 +347,7 @@ extern void __iomem *early_ioremap(resource_size_t phys_addr,
>  extern void __iomem *early_memremap(resource_size_t phys_addr,
>  				    unsigned long size);
>  extern void early_iounmap(void __iomem *addr, unsigned long size);
> +extern void fixup_early_ioremap(void);
>  
>  #define IO_SPACE_LIMIT 0xffff
>  
> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
> index ea82ef0..fe06296 100644
> --- a/arch/x86/mm/ioremap.c
> +++ b/arch/x86/mm/ioremap.c
> @@ -448,6 +448,23 @@ static inline void __init early_clear_fixmap(enum fixed_addresses idx)
>  static void __iomem *prev_map[FIX_BTMAPS_SLOTS] __initdata;
>  static unsigned long prev_size[FIX_BTMAPS_SLOTS] __initdata;
>  
> +void __init fixup_early_ioremap(void)
> +{
> +	int i;
> +	for (i = 0; i < FIX_BTMAPS_SLOTS; i++) {
> +		if (prev_map[i])
> +			break;
> +	}
> +
> +	if (i == FIX_BTMAPS_SLOTS)
> +		WARN_ON(1);

Should be:
	if (i < FIX_BTMAPS_SLOTS)
		WARN_ON(1);

Re: + x86-fix-handling-of-the-reservetop-boot-option.patch added to -mm tree

[Yinghai]

On 04/07/2010 06:53 PM, Liang Li wrote:
> Does this similar modification like this is more preferred?
> 
> diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
> index a1dcfa3..30a3e97 100644
> --- a/arch/x86/include/asm/io.h
> +++ b/arch/x86/include/asm/io.h
> @@ -347,6 +347,7 @@ extern void __iomem *early_ioremap(resource_size_t phys_addr,
>  extern void __iomem *early_memremap(resource_size_t phys_addr,
>  				    unsigned long size);
>  extern void early_iounmap(void __iomem *addr, unsigned long size);
> +extern void fixup_early_ioremap(void);
>  
>  #define IO_SPACE_LIMIT 0xffff
>  
> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
> index ea82ef0..fe06296 100644
> --- a/arch/x86/mm/ioremap.c
> +++ b/arch/x86/mm/ioremap.c
> @@ -448,6 +448,23 @@ static inline void __init early_clear_fixmap(enum fixed_addresses idx)
>  static void __iomem *prev_map[FIX_BTMAPS_SLOTS] __initdata;
>  static unsigned long prev_size[FIX_BTMAPS_SLOTS] __initdata;
>  
> +void __init fixup_early_ioremap(void)
> +{
> +	int i;
> +	for (i = 0; i < FIX_BTMAPS_SLOTS; i++) {
> +		if (prev_map[i])
> +			break;
> +	}
> +
> +	if (i == FIX_BTMAPS_SLOTS)
<
> +		WARN_ON(1);
BUG_ON()
> +
> +	for (i = 0; i < FIX_BTMAPS_SLOTS; i++)
> +		slot_virt[i] = __fix_to_virt(FIX_BTMAP_BEGIN - NR_FIX_BTMAPS * i);
> +
need to clear the old PMD, and set new PMD.

so you can clear old PMD and call early_ioremap_init() in fixup_early_ioremap()


Thanks

Yinghai

Re: + x86-fix-handling-of-the-reservetop-boot-option.patch added to -mm tree

[Liang Li]

On Wed, Apr 07, 2010 at 09:30:39PM -0700, Yinghai wrote:
> On 04/07/2010 06:53 PM, Liang Li wrote:
> > Does this similar modification like this is more preferred?
> > 
> > diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
> > index a1dcfa3..30a3e97 100644
> > --- a/arch/x86/include/asm/io.h
> > +++ b/arch/x86/include/asm/io.h
> > @@ -347,6 +347,7 @@ extern void __iomem *early_ioremap(resource_size_t phys_addr,
> >  extern void __iomem *early_memremap(resource_size_t phys_addr,
> >  				    unsigned long size);
> >  extern void early_iounmap(void __iomem *addr, unsigned long size);
> > +extern void fixup_early_ioremap(void);
> >  
> >  #define IO_SPACE_LIMIT 0xffff
> >  
> > diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
> > index ea82ef0..fe06296 100644
> > --- a/arch/x86/mm/ioremap.c
> > +++ b/arch/x86/mm/ioremap.c
> > @@ -448,6 +448,23 @@ static inline void __init early_clear_fixmap(enum fixed_addresses idx)
> >  static void __iomem *prev_map[FIX_BTMAPS_SLOTS] __initdata;
> >  static unsigned long prev_size[FIX_BTMAPS_SLOTS] __initdata;
> >  
> > +void __init fixup_early_ioremap(void)
> > +{
> > +	int i;
> > +	for (i = 0; i < FIX_BTMAPS_SLOTS; i++) {
> > +		if (prev_map[i])
> > +			break;
> > +	}
> > +
> > +	if (i == FIX_BTMAPS_SLOTS)
> <
> > +		WARN_ON(1);
> BUG_ON()
> > +
> > +	for (i = 0; i < FIX_BTMAPS_SLOTS; i++)
> > +		slot_virt[i] = __fix_to_virt(FIX_BTMAP_BEGIN - NR_FIX_BTMAPS * i);
> > +
> need to clear the old PMD, and set new PMD.
> 
> so you can clear old PMD and call early_ioremap_init() in fixup_early_ioremap()

Call early_ioremap_init will do the update PMD work. So the preferred
patch would be:
---------------
>From 61fe7a116cbbf6eef98a49b88ed5861ed9ebd32d Mon Sep 17 00:00:00 2001
From: Liang Li <liang.li@windriver.com>
Date: Mon, 22 Mar 2010 18:38:14 +0800
Subject: [PATCH] x86: let 'reservetop' functioning right

When specify 'reservetop=0xbadc0de' kernel parameter, the kernel will
stop booting due to a early_ioremap bug that relate to commit 8827247ff.

The root cause of boot failure problem is the value of 'slot_virt[i]'
was initialized in setup_arch->early_ioremap_init. But later in
setup_arch, the function 'parse_early_param' will modify 'FIXADDR_TOP'
when 'reservetop=0xbadc0de' being specified.

When reservetop being handled then FIXADDR_TOP get adjusted, Hence check
prev_map then re-initialize slot_virt and PMD based on new FIXADDR_TOP.

Signed-off-by: Liang Li <liang.li@windriver.com>
Cc: Wang Chen <wangchen@cn.fujitsu.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Yinghai Lu <yinghai@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 arch/x86/include/asm/io.h |    1 +
 arch/x86/mm/ioremap.c     |   15 +++++++++++++++
 arch/x86/mm/pgtable.c     |    2 ++
 3 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index a1dcfa3..30a3e97 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -347,6 +347,7 @@ extern void __iomem *early_ioremap(resource_size_t phys_addr,
 extern void __iomem *early_memremap(resource_size_t phys_addr,
 				    unsigned long size);
 extern void early_iounmap(void __iomem *addr, unsigned long size);
+extern void fixup_early_ioremap(void);
 
 #define IO_SPACE_LIMIT 0xffff
 
diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index 5eb1ba7..e4ab706 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -448,6 +448,21 @@ static inline void __init early_clear_fixmap(enum fixed_addresses idx)
 static void __iomem *prev_map[FIX_BTMAPS_SLOTS] __initdata;
 static unsigned long prev_size[FIX_BTMAPS_SLOTS] __initdata;
 
+void __init fixup_early_ioremap(void)
+{
+	int i;
+	for (i = 0; i < FIX_BTMAPS_SLOTS; i++) {
+		if (prev_map[i])
+			break;
+	}
+
+	if (i < FIX_BTMAPS_SLOTS)
+		BUG_ON(1);
+
+	early_ioremap_init();
+	return;
+}
+
 static int __init check_early_ioremap_leak(void)
 {
 	int count = 0;
diff --git a/arch/x86/mm/pgtable.c b/arch/x86/mm/pgtable.c
index 5c4ee42..ea4d54c 100644
--- a/arch/x86/mm/pgtable.c
+++ b/arch/x86/mm/pgtable.c
@@ -4,6 +4,7 @@
 #include <asm/pgtable.h>
 #include <asm/tlb.h>
 #include <asm/fixmap.h>
+#include <asm/io.h>
 
 #define PGALLOC_GFP GFP_KERNEL | __GFP_NOTRACK | __GFP_REPEAT | __GFP_ZERO
 
@@ -351,6 +352,7 @@ void __init reserve_top_address(unsigned long reserve)
 	printk(KERN_INFO "Reserving virtual address space above 0x%08x\n",
 	       (int)-reserve);
 	__FIXADDR_TOP = -reserve - PAGE_SIZE;
+	fixup_early_ioremap();
 #endif
 }
 
-----------
Acceptable?

Thanks and regards,
	   -Liang Li

Re: + x86-fix-handling-of-the-reservetop-boot-option.patch added to -mm tree

[Yinghai]

On 04/07/2010 09:59 PM, Liang Li wrote:
> On Wed, Apr 07, 2010 at 09:30:39PM -0700, Yinghai wrote:
>> On 04/07/2010 06:53 PM, Liang Li wrote:
>>> Does this similar modification like this is more preferred?
>>>
>>> diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
>>> index a1dcfa3..30a3e97 100644
>>> --- a/arch/x86/include/asm/io.h
>>> +++ b/arch/x86/include/asm/io.h
>>> @@ -347,6 +347,7 @@ extern void __iomem *early_ioremap(resource_size_t phys_addr,
>>>  extern void __iomem *early_memremap(resource_size_t phys_addr,
>>>  				    unsigned long size);
>>>  extern void early_iounmap(void __iomem *addr, unsigned long size);
>>> +extern void fixup_early_ioremap(void);
>>>  
>>>  #define IO_SPACE_LIMIT 0xffff
>>>  
>>> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
>>> index ea82ef0..fe06296 100644
>>> --- a/arch/x86/mm/ioremap.c
>>> +++ b/arch/x86/mm/ioremap.c
>>> @@ -448,6 +448,23 @@ static inline void __init early_clear_fixmap(enum fixed_addresses idx)
>>>  static void __iomem *prev_map[FIX_BTMAPS_SLOTS] __initdata;
>>>  static unsigned long prev_size[FIX_BTMAPS_SLOTS] __initdata;
>>>  
>>> +void __init fixup_early_ioremap(void)
>>> +{
>>> +	int i;
>>> +	for (i = 0; i < FIX_BTMAPS_SLOTS; i++) {
>>> +		if (prev_map[i])
>>> +			break;
>>> +	}
>>> +
>>> +	if (i == FIX_BTMAPS_SLOTS)
>> <
>>> +		WARN_ON(1);
>> BUG_ON()
>>> +
>>> +	for (i = 0; i < FIX_BTMAPS_SLOTS; i++)
>>> +		slot_virt[i] = __fix_to_virt(FIX_BTMAP_BEGIN - NR_FIX_BTMAPS * i);
>>> +
>> need to clear the old PMD, and set new PMD.
>>
>> so you can clear old PMD and call early_ioremap_init() in fixup_early_ioremap()
> 
> Call early_ioremap_init will do the update PMD work. So the preferred
> patch would be:
> ---------------
> From 61fe7a116cbbf6eef98a49b88ed5861ed9ebd32d Mon Sep 17 00:00:00 2001
> From: Liang Li <liang.li@windriver.com>
> Date: Mon, 22 Mar 2010 18:38:14 +0800
> Subject: [PATCH] x86: let 'reservetop' functioning right
> 
> When specify 'reservetop=0xbadc0de' kernel parameter, the kernel will
> stop booting due to a early_ioremap bug that relate to commit 8827247ff.
> 
> The root cause of boot failure problem is the value of 'slot_virt[i]'
> was initialized in setup_arch->early_ioremap_init. But later in
> setup_arch, the function 'parse_early_param' will modify 'FIXADDR_TOP'
> when 'reservetop=0xbadc0de' being specified.
> 
> When reservetop being handled then FIXADDR_TOP get adjusted, Hence check
> prev_map then re-initialize slot_virt and PMD based on new FIXADDR_TOP.
> 
> Signed-off-by: Liang Li <liang.li@windriver.com>
> Cc: Wang Chen <wangchen@cn.fujitsu.com>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Yinghai Lu <yinghai@kernel.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> ---
>  arch/x86/include/asm/io.h |    1 +
>  arch/x86/mm/ioremap.c     |   15 +++++++++++++++
>  arch/x86/mm/pgtable.c     |    2 ++
>  3 files changed, 18 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
> index a1dcfa3..30a3e97 100644
> --- a/arch/x86/include/asm/io.h
> +++ b/arch/x86/include/asm/io.h
> @@ -347,6 +347,7 @@ extern void __iomem *early_ioremap(resource_size_t phys_addr,
>  extern void __iomem *early_memremap(resource_size_t phys_addr,
>  				    unsigned long size);
>  extern void early_iounmap(void __iomem *addr, unsigned long size);
> +extern void fixup_early_ioremap(void);
>  
>  #define IO_SPACE_LIMIT 0xffff
>  
> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
> index 5eb1ba7..e4ab706 100644
> --- a/arch/x86/mm/ioremap.c
> +++ b/arch/x86/mm/ioremap.c
> @@ -448,6 +448,21 @@ static inline void __init early_clear_fixmap(enum fixed_addresses idx)
>  static void __iomem *prev_map[FIX_BTMAPS_SLOTS] __initdata;
>  static unsigned long prev_size[FIX_BTMAPS_SLOTS] __initdata;
>  
> +void __init fixup_early_ioremap(void)
> +{
> +	int i;
> +	for (i = 0; i < FIX_BTMAPS_SLOTS; i++) {
> +		if (prev_map[i])
> +			break;
> +	}
> +
> +	if (i < FIX_BTMAPS_SLOTS)
> +		BUG_ON(1);
> +
> +	early_ioremap_init();
> +	return;
> +}
> +
>  static int __init check_early_ioremap_leak(void)
>  {
>  	int count = 0;
> diff --git a/arch/x86/mm/pgtable.c b/arch/x86/mm/pgtable.c
> index 5c4ee42..ea4d54c 100644
> --- a/arch/x86/mm/pgtable.c
> +++ b/arch/x86/mm/pgtable.c
> @@ -4,6 +4,7 @@
>  #include <asm/pgtable.h>
>  #include <asm/tlb.h>
>  #include <asm/fixmap.h>
> +#include <asm/io.h>
>  
>  #define PGALLOC_GFP GFP_KERNEL | __GFP_NOTRACK | __GFP_REPEAT | __GFP_ZERO
>  
> @@ -351,6 +352,7 @@ void __init reserve_top_address(unsigned long reserve)
>  	printk(KERN_INFO "Reserving virtual address space above 0x%08x\n",
>  	       (int)-reserve);
>  	__FIXADDR_TOP = -reserve - PAGE_SIZE;
> +	fixup_early_ioremap();
>  #endif
>  }
>  
> -----------
> Acceptable?
> 
good to me.

may need to ask xen/lguest/vmi related to check that too.

arch/x86/kernel/vmi_32.c:       reserve_top_address(-vmi_rom->virtual_top);
arch/x86/lguest/boot.c: reserve_top_address(lguest_data.reserve_mem);
arch/x86/mm/pgtable_32.c:       reserve_top_address(address);
arch/x86/xen/mmu.c:     reserve_top_address(-top);


YH

Re: + x86-fix-handling-of-the-reservetop-boot-option.patch added to -mm tree

[Liang Li]

On Wed, Apr 07, 2010 at 11:57:22PM -0700, Yinghai wrote:
> On 04/07/2010 09:59 PM, Liang Li wrote:
> > On Wed, Apr 07, 2010 at 09:30:39PM -0700, Yinghai wrote:
> >> On 04/07/2010 06:53 PM, Liang Li wrote:
> >>> Does this similar modification like this is more preferred?
> >>>
> >>> diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
> >>> index a1dcfa3..30a3e97 100644
> >>> --- a/arch/x86/include/asm/io.h
> >>> +++ b/arch/x86/include/asm/io.h
> >>> @@ -347,6 +347,7 @@ extern void __iomem *early_ioremap(resource_size_t phys_addr,
> >>>  extern void __iomem *early_memremap(resource_size_t phys_addr,
> >>>  				    unsigned long size);
> >>>  extern void early_iounmap(void __iomem *addr, unsigned long size);
> >>> +extern void fixup_early_ioremap(void);
> >>>  
> >>>  #define IO_SPACE_LIMIT 0xffff
> >>>  
> >>> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
> >>> index ea82ef0..fe06296 100644
> >>> --- a/arch/x86/mm/ioremap.c
> >>> +++ b/arch/x86/mm/ioremap.c
> >>> @@ -448,6 +448,23 @@ static inline void __init early_clear_fixmap(enum fixed_addresses idx)
> >>>  static void __iomem *prev_map[FIX_BTMAPS_SLOTS] __initdata;
> >>>  static unsigned long prev_size[FIX_BTMAPS_SLOTS] __initdata;
> >>>  
> >>> +void __init fixup_early_ioremap(void)
> >>> +{
> >>> +	int i;
> >>> +	for (i = 0; i < FIX_BTMAPS_SLOTS; i++) {
> >>> +		if (prev_map[i])
> >>> +			break;
> >>> +	}
> >>> +
> >>> +	if (i == FIX_BTMAPS_SLOTS)
> >> <
> >>> +		WARN_ON(1);
> >> BUG_ON()
> >>> +
> >>> +	for (i = 0; i < FIX_BTMAPS_SLOTS; i++)
> >>> +		slot_virt[i] = __fix_to_virt(FIX_BTMAP_BEGIN - NR_FIX_BTMAPS * i);
> >>> +
> >> need to clear the old PMD, and set new PMD.
> >>
> >> so you can clear old PMD and call early_ioremap_init() in fixup_early_ioremap()
> > 
> > Call early_ioremap_init will do the update PMD work. So the preferred
> > patch would be:
> > ---------------
> > From 61fe7a116cbbf6eef98a49b88ed5861ed9ebd32d Mon Sep 17 00:00:00 2001
> > From: Liang Li <liang.li@windriver.com>
> > Date: Mon, 22 Mar 2010 18:38:14 +0800
> > Subject: [PATCH] x86: let 'reservetop' functioning right
> > 
> > When specify 'reservetop=0xbadc0de' kernel parameter, the kernel will
> > stop booting due to a early_ioremap bug that relate to commit 8827247ff.
> > 
> > The root cause of boot failure problem is the value of 'slot_virt[i]'
> > was initialized in setup_arch->early_ioremap_init. But later in
> > setup_arch, the function 'parse_early_param' will modify 'FIXADDR_TOP'
> > when 'reservetop=0xbadc0de' being specified.
> > 
> > When reservetop being handled then FIXADDR_TOP get adjusted, Hence check
> > prev_map then re-initialize slot_virt and PMD based on new FIXADDR_TOP.
> > 
> > Signed-off-by: Liang Li <liang.li@windriver.com>
> > Cc: Wang Chen <wangchen@cn.fujitsu.com>
> > Cc: Ingo Molnar <mingo@elte.hu>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: "H. Peter Anvin" <hpa@zytor.com>
> > Cc: Yinghai Lu <yinghai@kernel.org>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > ---
> >  arch/x86/include/asm/io.h |    1 +
> >  arch/x86/mm/ioremap.c     |   15 +++++++++++++++
> >  arch/x86/mm/pgtable.c     |    2 ++
> >  3 files changed, 18 insertions(+), 0 deletions(-)
> > 
> > diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
> > index a1dcfa3..30a3e97 100644
> > --- a/arch/x86/include/asm/io.h
> > +++ b/arch/x86/include/asm/io.h
> > @@ -347,6 +347,7 @@ extern void __iomem *early_ioremap(resource_size_t phys_addr,
> >  extern void __iomem *early_memremap(resource_size_t phys_addr,
> >  				    unsigned long size);
> >  extern void early_iounmap(void __iomem *addr, unsigned long size);
> > +extern void fixup_early_ioremap(void);
> >  
> >  #define IO_SPACE_LIMIT 0xffff
> >  
> > diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
> > index 5eb1ba7..e4ab706 100644
> > --- a/arch/x86/mm/ioremap.c
> > +++ b/arch/x86/mm/ioremap.c
> > @@ -448,6 +448,21 @@ static inline void __init early_clear_fixmap(enum fixed_addresses idx)
> >  static void __iomem *prev_map[FIX_BTMAPS_SLOTS] __initdata;
> >  static unsigned long prev_size[FIX_BTMAPS_SLOTS] __initdata;
> >  
> > +void __init fixup_early_ioremap(void)
> > +{
> > +	int i;
> > +	for (i = 0; i < FIX_BTMAPS_SLOTS; i++) {
> > +		if (prev_map[i])
> > +			break;
> > +	}
> > +
> > +	if (i < FIX_BTMAPS_SLOTS)
> > +		BUG_ON(1);
> > +
> > +	early_ioremap_init();
> > +	return;
> > +}
> > +
> >  static int __init check_early_ioremap_leak(void)
> >  {
> >  	int count = 0;
> > diff --git a/arch/x86/mm/pgtable.c b/arch/x86/mm/pgtable.c
> > index 5c4ee42..ea4d54c 100644
> > --- a/arch/x86/mm/pgtable.c
> > +++ b/arch/x86/mm/pgtable.c
> > @@ -4,6 +4,7 @@
> >  #include <asm/pgtable.h>
> >  #include <asm/tlb.h>
> >  #include <asm/fixmap.h>
> > +#include <asm/io.h>
> >  
> >  #define PGALLOC_GFP GFP_KERNEL | __GFP_NOTRACK | __GFP_REPEAT | __GFP_ZERO
> >  
> > @@ -351,6 +352,7 @@ void __init reserve_top_address(unsigned long reserve)
> >  	printk(KERN_INFO "Reserving virtual address space above 0x%08x\n",
> >  	       (int)-reserve);
> >  	__FIXADDR_TOP = -reserve - PAGE_SIZE;
> > +	fixup_early_ioremap();
> >  #endif
> >  }
> >  
> > -----------
> > Acceptable?
> > 
> good to me.
> 
> may need to ask xen/lguest/vmi related to check that too.
> 
> arch/x86/kernel/vmi_32.c:       reserve_top_address(-vmi_rom->virtual_top);
> arch/x86/lguest/boot.c: reserve_top_address(lguest_data.reserve_mem);
> arch/x86/mm/pgtable_32.c:       reserve_top_address(address);
> arch/x86/xen/mmu.c:     reserve_top_address(-top);

When linux as vmi/xen/lguest guest OS, kernel call reserve_top_address
before start_kernel. It is far before the start_kernel hence far before
setup_arch->early_ioremap_init. So it is unsafe to place
fixup_early_ioremap inside reserve_top_address. So I think the patch
should be:

>From 7cefa9a80c4434f2941a7072d39b1f1ffc08a40f Mon Sep 17 00:00:00 2001
From: Liang Li <liang.li@windriver.com>
Date: Mon, 22 Mar 2010 18:38:14 +0800
Subject: [PATCH] x86: let 'reservetop' functioning right

When specify 'reservetop=0xbadc0de' kernel parameter, the kernel will
stop booting due to a early_ioremap bug that relate to commit 8827247ff.

The root cause of boot failure problem is the value of 'slot_virt[i]'
was initialized in setup_arch->early_ioremap_init. But later in
setup_arch, the function 'parse_early_param' will modify 'FIXADDR_TOP'
when 'reservetop=0xbadc0de' being specified.

When reservetop being handled then FIXADDR_TOP get adjusted, Hence check
prev_map then re-initialize slot_virt and PMD based on new FIXADDR_TOP.

Signed-off-by: Liang Li <liang.li@windriver.com>
Cc: Wang Chen <wangchen@cn.fujitsu.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Yinghai Lu <yinghai@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 arch/x86/include/asm/io.h |    1 +
 arch/x86/mm/ioremap.c     |   15 +++++++++++++++
 arch/x86/mm/pgtable_32.c  |    1 +
 3 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index a1dcfa3..30a3e97 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -347,6 +347,7 @@ extern void __iomem *early_ioremap(resource_size_t phys_addr,
 extern void __iomem *early_memremap(resource_size_t phys_addr,
 				    unsigned long size);
 extern void early_iounmap(void __iomem *addr, unsigned long size);
+extern void fixup_early_ioremap(void);
 
 #define IO_SPACE_LIMIT 0xffff
 
diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index 5eb1ba7..e4ab706 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -448,6 +448,21 @@ static inline void __init early_clear_fixmap(enum fixed_addresses idx)
 static void __iomem *prev_map[FIX_BTMAPS_SLOTS] __initdata;
 static unsigned long prev_size[FIX_BTMAPS_SLOTS] __initdata;
 
+void __init fixup_early_ioremap(void)
+{
+	int i;
+	for (i = 0; i < FIX_BTMAPS_SLOTS; i++) {
+		if (prev_map[i])
+			break;
+	}
+
+	if (i < FIX_BTMAPS_SLOTS)
+		BUG_ON(1);
+
+	early_ioremap_init();
+	return;
+}
+
 static int __init check_early_ioremap_leak(void)
 {
 	int count = 0;
diff --git a/arch/x86/mm/pgtable_32.c b/arch/x86/mm/pgtable_32.c
index 1a8faf0..26eadaa 100644
--- a/arch/x86/mm/pgtable_32.c
+++ b/arch/x86/mm/pgtable_32.c
@@ -128,6 +128,7 @@ static int __init parse_reservetop(char *arg)
 
 	address = memparse(arg, &arg);
 	reserve_top_address(address);
+	fixup_early_ioremap();
 	return 0;
 }
 early_param("reservetop", parse_reservetop);
-- 
1.6.6

Thanks and best regards,
	   -Liang Li

> 
> 
> YH

Re: + x86-fix-handling-of-the-reservetop-boot-option.patch added to -mm tree

[Jeremy Fitzhardinge]

On 04/08/2010 01:58 AM, Liang Li wrote:
> On Wed, Apr 07, 2010 at 11:57:22PM -0700, Yinghai wrote:
>   
>> On 04/07/2010 09:59 PM, Liang Li wrote:
>>     
>>> On Wed, Apr 07, 2010 at 09:30:39PM -0700, Yinghai wrote:
>>>       
>>>> On 04/07/2010 06:53 PM, Liang Li wrote:
>>>>         
>>>>> Does this similar modification like this is more preferred?
>>>>>
>>>>> diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
>>>>> index a1dcfa3..30a3e97 100644
>>>>> --- a/arch/x86/include/asm/io.h
>>>>> +++ b/arch/x86/include/asm/io.h
>>>>> @@ -347,6 +347,7 @@ extern void __iomem *early_ioremap(resource_size_t phys_addr,
>>>>>  extern void __iomem *early_memremap(resource_size_t phys_addr,
>>>>>  				    unsigned long size);
>>>>>  extern void early_iounmap(void __iomem *addr, unsigned long size);
>>>>> +extern void fixup_early_ioremap(void);
>>>>>  
>>>>>  #define IO_SPACE_LIMIT 0xffff
>>>>>  
>>>>> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
>>>>> index ea82ef0..fe06296 100644
>>>>> --- a/arch/x86/mm/ioremap.c
>>>>> +++ b/arch/x86/mm/ioremap.c
>>>>> @@ -448,6 +448,23 @@ static inline void __init early_clear_fixmap(enum fixed_addresses idx)
>>>>>  static void __iomem *prev_map[FIX_BTMAPS_SLOTS] __initdata;
>>>>>  static unsigned long prev_size[FIX_BTMAPS_SLOTS] __initdata;
>>>>>  
>>>>> +void __init fixup_early_ioremap(void)
>>>>> +{
>>>>> +	int i;
>>>>> +	for (i = 0; i < FIX_BTMAPS_SLOTS; i++) {
>>>>> +		if (prev_map[i])
>>>>> +			break;
>>>>> +	}
>>>>> +
>>>>> +	if (i == FIX_BTMAPS_SLOTS)
>>>>>           
>>>> <
>>>>         
>>>>> +		WARN_ON(1);
>>>>>           
>>>> BUG_ON()
>>>>         
>>>>> +
>>>>> +	for (i = 0; i < FIX_BTMAPS_SLOTS; i++)
>>>>> +		slot_virt[i] = __fix_to_virt(FIX_BTMAP_BEGIN - NR_FIX_BTMAPS * i);
>>>>> +
>>>>>           
>>>> need to clear the old PMD, and set new PMD.
>>>>
>>>> so you can clear old PMD and call early_ioremap_init() in fixup_early_ioremap()
>>>>         
>>> Call early_ioremap_init will do the update PMD work. So the preferred
>>> patch would be:
>>> ---------------
>>> From 61fe7a116cbbf6eef98a49b88ed5861ed9ebd32d Mon Sep 17 00:00:00 2001
>>> From: Liang Li <liang.li@windriver.com>
>>> Date: Mon, 22 Mar 2010 18:38:14 +0800
>>> Subject: [PATCH] x86: let 'reservetop' functioning right
>>>
>>> When specify 'reservetop=0xbadc0de' kernel parameter, the kernel will
>>> stop booting due to a early_ioremap bug that relate to commit 8827247ff.
>>>
>>> The root cause of boot failure problem is the value of 'slot_virt[i]'
>>> was initialized in setup_arch->early_ioremap_init. But later in
>>> setup_arch, the function 'parse_early_param' will modify 'FIXADDR_TOP'
>>> when 'reservetop=0xbadc0de' being specified.
>>>
>>> When reservetop being handled then FIXADDR_TOP get adjusted, Hence check
>>> prev_map then re-initialize slot_virt and PMD based on new FIXADDR_TOP.
>>>
>>> Signed-off-by: Liang Li <liang.li@windriver.com>
>>> Cc: Wang Chen <wangchen@cn.fujitsu.com>
>>> Cc: Ingo Molnar <mingo@elte.hu>
>>> Cc: Thomas Gleixner <tglx@linutronix.de>
>>> Cc: "H. Peter Anvin" <hpa@zytor.com>
>>> Cc: Yinghai Lu <yinghai@kernel.org>
>>> Cc: Andrew Morton <akpm@linux-foundation.org>
>>> ---
>>>  arch/x86/include/asm/io.h |    1 +
>>>  arch/x86/mm/ioremap.c     |   15 +++++++++++++++
>>>  arch/x86/mm/pgtable.c     |    2 ++
>>>  3 files changed, 18 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
>>> index a1dcfa3..30a3e97 100644
>>> --- a/arch/x86/include/asm/io.h
>>> +++ b/arch/x86/include/asm/io.h
>>> @@ -347,6 +347,7 @@ extern void __iomem *early_ioremap(resource_size_t phys_addr,
>>>  extern void __iomem *early_memremap(resource_size_t phys_addr,
>>>  				    unsigned long size);
>>>  extern void early_iounmap(void __iomem *addr, unsigned long size);
>>> +extern void fixup_early_ioremap(void);
>>>  
>>>  #define IO_SPACE_LIMIT 0xffff
>>>  
>>> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
>>> index 5eb1ba7..e4ab706 100644
>>> --- a/arch/x86/mm/ioremap.c
>>> +++ b/arch/x86/mm/ioremap.c
>>> @@ -448,6 +448,21 @@ static inline void __init early_clear_fixmap(enum fixed_addresses idx)
>>>  static void __iomem *prev_map[FIX_BTMAPS_SLOTS] __initdata;
>>>  static unsigned long prev_size[FIX_BTMAPS_SLOTS] __initdata;
>>>  
>>> +void __init fixup_early_ioremap(void)
>>> +{
>>> +	int i;
>>> +	for (i = 0; i < FIX_BTMAPS_SLOTS; i++) {
>>> +		if (prev_map[i])
>>> +			break;
>>> +	}
>>> +
>>> +	if (i < FIX_BTMAPS_SLOTS)
>>> +		BUG_ON(1);
>>> +
>>> +	early_ioremap_init();
>>> +	return;
>>> +}
>>> +
>>>  static int __init check_early_ioremap_leak(void)
>>>  {
>>>  	int count = 0;
>>> diff --git a/arch/x86/mm/pgtable.c b/arch/x86/mm/pgtable.c
>>> index 5c4ee42..ea4d54c 100644
>>> --- a/arch/x86/mm/pgtable.c
>>> +++ b/arch/x86/mm/pgtable.c
>>> @@ -4,6 +4,7 @@
>>>  #include <asm/pgtable.h>
>>>  #include <asm/tlb.h>
>>>  #include <asm/fixmap.h>
>>> +#include <asm/io.h>
>>>  
>>>  #define PGALLOC_GFP GFP_KERNEL | __GFP_NOTRACK | __GFP_REPEAT | __GFP_ZERO
>>>  
>>> @@ -351,6 +352,7 @@ void __init reserve_top_address(unsigned long reserve)
>>>  	printk(KERN_INFO "Reserving virtual address space above 0x%08x\n",
>>>  	       (int)-reserve);
>>>  	__FIXADDR_TOP = -reserve - PAGE_SIZE;
>>> +	fixup_early_ioremap();
>>>  #endif
>>>  }
>>>  
>>> -----------
>>> Acceptable?
>>>
>>>       
>> good to me.
>>
>> may need to ask xen/lguest/vmi related to check that too.
>>
>> arch/x86/kernel/vmi_32.c:       reserve_top_address(-vmi_rom->virtual_top);
>> arch/x86/lguest/boot.c: reserve_top_address(lguest_data.reserve_mem);
>> arch/x86/mm/pgtable_32.c:       reserve_top_address(address);
>> arch/x86/xen/mmu.c:     reserve_top_address(-top);
>>     
> When linux as vmi/xen/lguest guest OS, kernel call reserve_top_address
> before start_kernel. It is far before the start_kernel hence far before
> setup_arch->early_ioremap_init. So it is unsafe to place
> fixup_early_ioremap inside reserve_top_address. So I think the patch
> should be:
>   

Yes indeed.  That looks better.

Acked-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>

Thanks,
    J

> From 7cefa9a80c4434f2941a7072d39b1f1ffc08a40f Mon Sep 17 00:00:00 2001
> From: Liang Li <liang.li@windriver.com>
> Date: Mon, 22 Mar 2010 18:38:14 +0800
> Subject: [PATCH] x86: let 'reservetop' functioning right
>
> When specify 'reservetop=0xbadc0de' kernel parameter, the kernel will
> stop booting due to a early_ioremap bug that relate to commit 8827247ff.
>
> The root cause of boot failure problem is the value of 'slot_virt[i]'
> was initialized in setup_arch->early_ioremap_init. But later in
> setup_arch, the function 'parse_early_param' will modify 'FIXADDR_TOP'
> when 'reservetop=0xbadc0de' being specified.
>
> When reservetop being handled then FIXADDR_TOP get adjusted, Hence check
> prev_map then re-initialize slot_virt and PMD based on new FIXADDR_TOP.
>
> Signed-off-by: Liang Li <liang.li@windriver.com>
> Cc: Wang Chen <wangchen@cn.fujitsu.com>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Yinghai Lu <yinghai@kernel.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> ---
>  arch/x86/include/asm/io.h |    1 +
>  arch/x86/mm/ioremap.c     |   15 +++++++++++++++
>  arch/x86/mm/pgtable_32.c  |    1 +
>  3 files changed, 17 insertions(+), 0 deletions(-)
>
> diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
> index a1dcfa3..30a3e97 100644
> --- a/arch/x86/include/asm/io.h
> +++ b/arch/x86/include/asm/io.h
> @@ -347,6 +347,7 @@ extern void __iomem *early_ioremap(resource_size_t phys_addr,
>  extern void __iomem *early_memremap(resource_size_t phys_addr,
>  				    unsigned long size);
>  extern void early_iounmap(void __iomem *addr, unsigned long size);
> +extern void fixup_early_ioremap(void);
>  
>  #define IO_SPACE_LIMIT 0xffff
>  
> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
> index 5eb1ba7..e4ab706 100644
> --- a/arch/x86/mm/ioremap.c
> +++ b/arch/x86/mm/ioremap.c
> @@ -448,6 +448,21 @@ static inline void __init early_clear_fixmap(enum fixed_addresses idx)
>  static void __iomem *prev_map[FIX_BTMAPS_SLOTS] __initdata;
>  static unsigned long prev_size[FIX_BTMAPS_SLOTS] __initdata;
>  
> +void __init fixup_early_ioremap(void)
> +{
> +	int i;
> +	for (i = 0; i < FIX_BTMAPS_SLOTS; i++) {
> +		if (prev_map[i])
> +			break;
> +	}
> +
> +	if (i < FIX_BTMAPS_SLOTS)
> +		BUG_ON(1);
> +
> +	early_ioremap_init();
> +	return;
> +}
> +
>  static int __init check_early_ioremap_leak(void)
>  {
>  	int count = 0;
> diff --git a/arch/x86/mm/pgtable_32.c b/arch/x86/mm/pgtable_32.c
> index 1a8faf0..26eadaa 100644
> --- a/arch/x86/mm/pgtable_32.c
> +++ b/arch/x86/mm/pgtable_32.c
> @@ -128,6 +128,7 @@ static int __init parse_reservetop(char *arg)
>  
>  	address = memparse(arg, &arg);
>  	reserve_top_address(address);
> +	fixup_early_ioremap();
>  	return 0;
>  }
>  early_param("reservetop", parse_reservetop);
>   

Re: + x86-fix-handling-of-the-reservetop-boot-option.patch added to -mm tree

[Yinghai]

On 04/08/2010 10:12 AM, Jeremy Fitzhardinge wrote:
> On 04/08/2010 01:58 AM, Liang Li wrote:
>>>     
>> When linux as vmi/xen/lguest guest OS, kernel call reserve_top_address
>> before start_kernel. It is far before the start_kernel hence far before
>> setup_arch->early_ioremap_init. So it is unsafe to place
>> fixup_early_ioremap inside reserve_top_address. So I think the patch
>> should be:
>>   
> 
> Yes indeed.  That looks better.
> 
> Acked-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
> 

good.

Liang please resubmit with clear version changelog.

like

-v2: ...
-v3: move fixup_early_ioremap out of reserve_top_address..

Yinghai

> Thanks,
>     J
> 
>> From 7cefa9a80c4434f2941a7072d39b1f1ffc08a40f Mon Sep 17 00:00:00 2001
>> From: Liang Li <liang.li@windriver.com>
>> Date: Mon, 22 Mar 2010 18:38:14 +0800
>> Subject: [PATCH] x86: let 'reservetop' functioning right
>>
>> When specify 'reservetop=0xbadc0de' kernel parameter, the kernel will
>> stop booting due to a early_ioremap bug that relate to commit 8827247ff.
>>
>> The root cause of boot failure problem is the value of 'slot_virt[i]'
>> was initialized in setup_arch->early_ioremap_init. But later in
>> setup_arch, the function 'parse_early_param' will modify 'FIXADDR_TOP'
>> when 'reservetop=0xbadc0de' being specified.
>>
>> When reservetop being handled then FIXADDR_TOP get adjusted, Hence check
>> prev_map then re-initialize slot_virt and PMD based on new FIXADDR_TOP.
>>
>> Signed-off-by: Liang Li <liang.li@windriver.com>
>> Cc: Wang Chen <wangchen@cn.fujitsu.com>
>> Cc: Ingo Molnar <mingo@elte.hu>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: "H. Peter Anvin" <hpa@zytor.com>
>> Cc: Yinghai Lu <yinghai@kernel.org>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> ---
>>  arch/x86/include/asm/io.h |    1 +
>>  arch/x86/mm/ioremap.c     |   15 +++++++++++++++
>>  arch/x86/mm/pgtable_32.c  |    1 +
>>  3 files changed, 17 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
>> index a1dcfa3..30a3e97 100644
>> --- a/arch/x86/include/asm/io.h
>> +++ b/arch/x86/include/asm/io.h
>> @@ -347,6 +347,7 @@ extern void __iomem *early_ioremap(resource_size_t phys_addr,
>>  extern void __iomem *early_memremap(resource_size_t phys_addr,
>>  				    unsigned long size);
>>  extern void early_iounmap(void __iomem *addr, unsigned long size);
>> +extern void fixup_early_ioremap(void);
>>  
>>  #define IO_SPACE_LIMIT 0xffff
>>  
>> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
>> index 5eb1ba7..e4ab706 100644
>> --- a/arch/x86/mm/ioremap.c
>> +++ b/arch/x86/mm/ioremap.c
>> @@ -448,6 +448,21 @@ static inline void __init early_clear_fixmap(enum fixed_addresses idx)
>>  static void __iomem *prev_map[FIX_BTMAPS_SLOTS] __initdata;
>>  static unsigned long prev_size[FIX_BTMAPS_SLOTS] __initdata;
>>  
>> +void __init fixup_early_ioremap(void)
>> +{
>> +	int i;
>> +	for (i = 0; i < FIX_BTMAPS_SLOTS; i++) {
>> +		if (prev_map[i])
>> +			break;
>> +	}
>> +
>> +	if (i < FIX_BTMAPS_SLOTS)
>> +		BUG_ON(1);
>> +
>> +	early_ioremap_init();
>> +	return;
>> +}
>> +
>>  static int __init check_early_ioremap_leak(void)
>>  {
>>  	int count = 0;
>> diff --git a/arch/x86/mm/pgtable_32.c b/arch/x86/mm/pgtable_32.c
>> index 1a8faf0..26eadaa 100644
>> --- a/arch/x86/mm/pgtable_32.c
>> +++ b/arch/x86/mm/pgtable_32.c
>> @@ -128,6 +128,7 @@ static int __init parse_reservetop(char *arg)
>>  
>>  	address = memparse(arg, &arg);
>>  	reserve_top_address(address);
>> +	fixup_early_ioremap();
>>  	return 0;
>>  }
>>  early_param("reservetop", parse_reservetop);
>>   
> 

Re: [LKML] Re: + x86-fix-handling-of-the-reservetop-boot-option.patch added to -mm tree

[Konrad Rzeszutek Wilk]

On Thu, Apr 08, 2010 at 11:03:22AM -0700, Yinghai wrote:
> On 04/08/2010 10:12 AM, Jeremy Fitzhardinge wrote:
> > On 04/08/2010 01:58 AM, Liang Li wrote:
> >>>     
> >> When linux as vmi/xen/lguest guest OS, kernel call reserve_top_address
> >> before start_kernel. It is far before the start_kernel hence far before
> >> setup_arch->early_ioremap_init. So it is unsafe to place
> >> fixup_early_ioremap inside reserve_top_address. So I think the patch
> >> should be:
> >>   
> > 
> > Yes indeed.  That looks better.
> > 
> > Acked-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>

I've also tested it today as PV Xen guest:

Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Re: [LKML] Re: + x86-fix-handling-of-the-reservetop-boot-option.patch added to -mm tree

[Liang Li]

On Thu, Apr 08, 2010 at 02:38:06PM -0400, Konrad Rzeszutek Wilk wrote:
> On Thu, Apr 08, 2010 at 11:03:22AM -0700, Yinghai wrote:
> > On 04/08/2010 10:12 AM, Jeremy Fitzhardinge wrote:
> > > On 04/08/2010 01:58 AM, Liang Li wrote:
> > >>>     
> > >> When linux as vmi/xen/lguest guest OS, kernel call reserve_top_address
> > >> before start_kernel. It is far before the start_kernel hence far before
> > >> setup_arch->early_ioremap_init. So it is unsafe to place
> > >> fixup_early_ioremap inside reserve_top_address. So I think the patch
> > >> should be:
> > >>   
> > > 
> > > Yes indeed.  That looks better.
> > > 
> > > Acked-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
> 
> I've also tested it today as PV Xen guest:
> 
> Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Thanks all. Updated patch will be sent out soon.

Best regards,
	 	-Liang Li

> --
> 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: + x86-fix-handling-of-the-reservetop-boot-option.patch added to -mm tree

[Jeremy Fitzhardinge]

On 04/07/2010 11:57 PM, Yinghai wrote:
> On 04/07/2010 09:59 PM, Liang Li wrote:
>   
>> On Wed, Apr 07, 2010 at 09:30:39PM -0700, Yinghai wrote:
>>     
>>> On 04/07/2010 06:53 PM, Liang Li wrote:
>>>       
>>>> Does this similar modification like this is more preferred?
>>>>
>>>> diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
>>>> index a1dcfa3..30a3e97 100644
>>>> --- a/arch/x86/include/asm/io.h
>>>> +++ b/arch/x86/include/asm/io.h
>>>> @@ -347,6 +347,7 @@ extern void __iomem *early_ioremap(resource_size_t phys_addr,
>>>>  extern void __iomem *early_memremap(resource_size_t phys_addr,
>>>>  				    unsigned long size);
>>>>  extern void early_iounmap(void __iomem *addr, unsigned long size);
>>>> +extern void fixup_early_ioremap(void);
>>>>  
>>>>  #define IO_SPACE_LIMIT 0xffff
>>>>  
>>>> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
>>>> index ea82ef0..fe06296 100644
>>>> --- a/arch/x86/mm/ioremap.c
>>>> +++ b/arch/x86/mm/ioremap.c
>>>> @@ -448,6 +448,23 @@ static inline void __init early_clear_fixmap(enum fixed_addresses idx)
>>>>  static void __iomem *prev_map[FIX_BTMAPS_SLOTS] __initdata;
>>>>  static unsigned long prev_size[FIX_BTMAPS_SLOTS] __initdata;
>>>>  
>>>> +void __init fixup_early_ioremap(void)
>>>> +{
>>>> +	int i;
>>>> +	for (i = 0; i < FIX_BTMAPS_SLOTS; i++) {
>>>> +		if (prev_map[i])
>>>> +			break;
>>>> +	}
>>>> +
>>>> +	if (i == FIX_BTMAPS_SLOTS)
>>>>         
>>> <
>>>       
>>>> +		WARN_ON(1);
>>>>         
>>> BUG_ON()
>>>       
>>>> +
>>>> +	for (i = 0; i < FIX_BTMAPS_SLOTS; i++)
>>>> +		slot_virt[i] = __fix_to_virt(FIX_BTMAP_BEGIN - NR_FIX_BTMAPS * i);
>>>> +
>>>>         
>>> need to clear the old PMD, and set new PMD.
>>>
>>> so you can clear old PMD and call early_ioremap_init() in fixup_early_ioremap()
>>>       
>> Call early_ioremap_init will do the update PMD work. So the preferred
>> patch would be:
>> ---------------
>> From 61fe7a116cbbf6eef98a49b88ed5861ed9ebd32d Mon Sep 17 00:00:00 2001
>> From: Liang Li <liang.li@windriver.com>
>> Date: Mon, 22 Mar 2010 18:38:14 +0800
>> Subject: [PATCH] x86: let 'reservetop' functioning right
>>
>> When specify 'reservetop=0xbadc0de' kernel parameter, the kernel will
>> stop booting due to a early_ioremap bug that relate to commit 8827247ff.
>>
>> The root cause of boot failure problem is the value of 'slot_virt[i]'
>> was initialized in setup_arch->early_ioremap_init. But later in
>> setup_arch, the function 'parse_early_param' will modify 'FIXADDR_TOP'
>> when 'reservetop=0xbadc0de' being specified.
>>
>> When reservetop being handled then FIXADDR_TOP get adjusted, Hence check
>> prev_map then re-initialize slot_virt and PMD based on new FIXADDR_TOP.
>>
>> Signed-off-by: Liang Li <liang.li@windriver.com>
>> Cc: Wang Chen <wangchen@cn.fujitsu.com>
>> Cc: Ingo Molnar <mingo@elte.hu>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: "H. Peter Anvin" <hpa@zytor.com>
>> Cc: Yinghai Lu <yinghai@kernel.org>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> ---
>>  arch/x86/include/asm/io.h |    1 +
>>  arch/x86/mm/ioremap.c     |   15 +++++++++++++++
>>  arch/x86/mm/pgtable.c     |    2 ++
>>  3 files changed, 18 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
>> index a1dcfa3..30a3e97 100644
>> --- a/arch/x86/include/asm/io.h
>> +++ b/arch/x86/include/asm/io.h
>> @@ -347,6 +347,7 @@ extern void __iomem *early_ioremap(resource_size_t phys_addr,
>>  extern void __iomem *early_memremap(resource_size_t phys_addr,
>>  				    unsigned long size);
>>  extern void early_iounmap(void __iomem *addr, unsigned long size);
>> +extern void fixup_early_ioremap(void);
>>  
>>  #define IO_SPACE_LIMIT 0xffff
>>  
>> diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
>> index 5eb1ba7..e4ab706 100644
>> --- a/arch/x86/mm/ioremap.c
>> +++ b/arch/x86/mm/ioremap.c
>> @@ -448,6 +448,21 @@ static inline void __init early_clear_fixmap(enum fixed_addresses idx)
>>  static void __iomem *prev_map[FIX_BTMAPS_SLOTS] __initdata;
>>  static unsigned long prev_size[FIX_BTMAPS_SLOTS] __initdata;
>>  
>> +void __init fixup_early_ioremap(void)
>> +{
>> +	int i;
>> +	for (i = 0; i < FIX_BTMAPS_SLOTS; i++) {
>> +		if (prev_map[i])
>> +			break;
>> +	}
>> +
>> +	if (i < FIX_BTMAPS_SLOTS)
>> +		BUG_ON(1);
>> +
>> +	early_ioremap_init();
>> +	return;
>> +}
>> +
>>  static int __init check_early_ioremap_leak(void)
>>  {
>>  	int count = 0;
>> diff --git a/arch/x86/mm/pgtable.c b/arch/x86/mm/pgtable.c
>> index 5c4ee42..ea4d54c 100644
>> --- a/arch/x86/mm/pgtable.c
>> +++ b/arch/x86/mm/pgtable.c
>> @@ -4,6 +4,7 @@
>>  #include <asm/pgtable.h>
>>  #include <asm/tlb.h>
>>  #include <asm/fixmap.h>
>> +#include <asm/io.h>
>>  
>>  #define PGALLOC_GFP GFP_KERNEL | __GFP_NOTRACK | __GFP_REPEAT | __GFP_ZERO
>>  
>> @@ -351,6 +352,7 @@ void __init reserve_top_address(unsigned long reserve)
>>  	printk(KERN_INFO "Reserving virtual address space above 0x%08x\n",
>>  	       (int)-reserve);
>>  	__FIXADDR_TOP = -reserve - PAGE_SIZE;
>> +	fixup_early_ioremap();
>>  #endif
>>  }
>>  
>> -----------
>> Acceptable?
>>
>>     
> good to me.
>
> may need to ask xen/lguest/vmi related to check that too.
>
> arch/x86/kernel/vmi_32.c:       reserve_top_address(-vmi_rom->virtual_top);
> arch/x86/lguest/boot.c: reserve_top_address(lguest_data.reserve_mem);
> arch/x86/mm/pgtable_32.c:       reserve_top_address(address);
> arch/x86/xen/mmu.c:     reserve_top_address(-top);
>   

This looks troublesome for us.  We're calling reserve_top_address() very
early - before start_kernel - to make sure the address space for the
hypervisor has been reserved.  Calling fixup_early_ioremap() will
probably fail horribly.

Can you make it so that it only calls fixup_early_ioremap() if ioremap
init has already happened?

Also, do we actually need reservetop= any more.  It looks like Zach
added it for VMI, but VMI has been deprecated.  Are there any other use
cases?

Thanks,
     J