[RFC PATCH] x86: let 'reservetop' functioning right
Liang Li
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.
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 <lian...@windriver.com>
---
Hi,
I am not sure if kernel command line option 'reservetop' will block boot
is the normal situation or not. Could someone tell me if we should fix
it or just leave it as is or someone is doing something to replace
'reservetop' with some other stuff?
arch/x86/mm/ioremap.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index 5eb1ba7..ea82ef0 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -537,9 +537,9 @@ __early_ioremap(resource_size_t phys_addr, unsigned long size, pgprot_t prot)
--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];
}
--
1.6.6
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Liang Li
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.
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.
Changelog since v0:
-v1: 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.
-v2: place fixup_early_ioremap hence call early_ioremap_init in
reserve_top_address to re-initialize slot_virt and corresponding PMD
when parse_reservetop
-v3: move fixup_early_ioremap out of reserve_top_address to make sure
other clients of reserve_top_address like xen/lguest won't broken
Signed-off-by: Liang Li <lian...@windriver.com>
Cc: Wang Chen <wang...@cn.fujitsu.com>
Cc: Ingo Molnar <mi...@elte.hu>
Cc: Thomas Gleixner <tg...@linutronix.de>
Cc: "H. Peter Anvin" <h...@zytor.com>
Cc: Yinghai Lu <yin...@kernel.org>
Cc: Andrew Morton <ak...@linux-foundation.org>
Acked-by: Jeremy Fitzhardinge <jeremy.fi...@citrix.com>
Tested-by: Konrad Rzeszutek Wilk <konra...@oracle.com>
---
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);
Jeremy Fitzhardinge
> 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.
>
> 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.
>
While I guess this patch works OK, I have to say that I'm worried by the
need for it at all; it seems to be papering over a more serious
problem. reserve_top_address() is supposed to be called very early,
before anything has used or referenced FIXADDR_TOP. If we're seeing
problems with FIXADDR_TOP changing after it has been used, then it means
that reserve_top_address() is being called too late. Fixing that would
be the real fix.
J
--
Liang Li
> On 04/08/2010 05:43 PM, Liang Li wrote:
> > 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.
> >
> > 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.
> >
>
> While I guess this patch works OK, I have to say that I'm worried by the
> need for it at all; it seems to be papering over a more serious
> problem. reserve_top_address() is supposed to be called very early,
> before anything has used or referenced FIXADDR_TOP. If we're seeing
> problems with FIXADDR_TOP changing after it has been used, then it means
> that reserve_top_address() is being called too late. Fixing that would
> be the real fix.
The ideal thing is FIXADDR_TOP should not be touched after
early_ioremap_init. The late call to reserve_top_address is from
parse_reservetop, aka when reservetop=0xabcd0000 being passed as kernel
commandline parameter. In setup_arch, the call sequence is:
setup_arch
-> early_ioremap_init
-> parse_early_param
-> parse_reservetop
->reserve_top_address
See, how could we solve the confliction better?
Best regards,
-Liang Li
Jeremy Fitzhardinge
Well, the first question is "do we need the reservetop= kernel
parameter"? Zach added it, I think, so that VMI could be loaded
dynamically as a module. Given that VMI is deprecated anyway, I wonder
if we can just drop support for modular VMI and remove the reservetop=
kernel parameter. Or are there other uses for it?
J
Liang Li
Agree. We can remove the 'reservetop=' kernel parameter then the
problem dispear. :) But we don't have to.....
Regards,
-Liang Li