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





--

[ message continues ]

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

[ message continues ]

efi related code need them

dmi 


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

YH
--

[ message continues ]

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

--

[ message continues ]

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

--

[ message continues ]

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

[ message continues ]

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 ...
[ message continues ]

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

[ message continues ]

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 ...
[ message continues ]

Yes indeed.  That looks better.

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

Thanks,

--

[ message continues ]

good.

Liang please resubmit with clear version changelog.

like

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


--

[ message continues ]

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

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

[ message continues ]

Thanks all. Updated patch will be sent out soon.

Best regards,
--

[ message continues ]

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

[ message continues ]