[PATCH RFC] e820_remove_range fail?

[PATCH RFC] e820_remove_range fail?

[Rusty Russell]

Hi Yinghai,

   I just wrote this patch for lguest, but I think the real problem is
that e820_remove_range can't split a range.  Is this a problem for
real machines?

[    0.000000] BIOS-provided physical RAM map:
[    0.000000]  LGUEST: 0000000000000000 - 0000000004000000 (usable)
[    0.000000] Notice: NX (Execute Disable) protection missing in CPU or disabled in BIOS!
[    0.000000] DMI not present or invalid.
[    0.000000] last_pfn = 0x3fa0 max_arch_pfn = 0x100000
[    0.000000] init_memory_mapping: 0000000000000000-0000000003fa0000

Thanks,
Rusty.
===
lguest: handle trimming of our e820 map

Lguest doesn't need to reserve space for the BIOS, but in commit
1b5576e69a5f Yinghai added trim_bios_range which mangles our e820 map.
But e820_remove_range does not handle removing a range from the middle
of a region correctly, so we end up with a bad max_pfn value.

The result is we can't find our (lguest-specific) device table which
we expect above the top of RAM, and fail to find the root device.

Cc: Yinghai Lu <yinghai@kernel.org>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

diff --git a/arch/x86/lguest/boot.c b/arch/x86/lguest/boot.c
--- a/arch/x86/lguest/boot.c
+++ b/arch/x86/lguest/boot.c
@@ -1135,12 +1135,15 @@ static struct notifier_block paniced = {
 static __init char *lguest_memory_setup(void)
 {
 	/*
-	 *The Linux bootloader header contains an "e820" memory map: the
+	 * The Linux bootloader header contains an "e820" memory map: the
 	 * Launcher populated the first entry with our memory limit.
+	 *
+	 * We have to skip the traditional BIOS area here, otherwise the
+	 * kernel messes up our map trying to "fix" it in trim_bios_range.
 	 */
-	e820_add_region(boot_params.e820_map[0].addr,
-			  boot_params.e820_map[0].size,
-			  boot_params.e820_map[0].type);
+	e820_add_region(0, BIOS_BEGIN, boot_params.e820_map[0].type);
+	e820_add_region(BIOS_END, boot_params.e820_map[0].size - BIOS_END,
+			boot_params.e820_map[0].type);
 
 	/* This string is for the boot messages. */
 	return "LGUEST";

Re: [PATCH RFC] e820_remove_range fail?

[Yinghai Lu]

On 03/29/2010 07:08 PM, Rusty Russell wrote:
> Hi Yinghai,
> 
>    I just wrote this patch for lguest, but I think the real problem is
> that e820_remove_range can't split a range.  Is this a problem for
> real machines?
> 
> [    0.000000] BIOS-provided physical RAM map:
> [    0.000000]  LGUEST: 0000000000000000 - 0000000004000000 (usable)
> [    0.000000] Notice: NX (Execute Disable) protection missing in CPU or disabled in BIOS!
> [    0.000000] DMI not present or invalid.
> [    0.000000] last_pfn = 0x3fa0 max_arch_pfn = 0x100000
> [    0.000000] init_memory_mapping: 0000000000000000-0000000003fa0000
> 

you are right, looks like e820_remove_region() missed that case.

will send out one patch.

Thanks

Yinghai Lu

[PATCH] x86: Make e820_remove_range to handle all covered case

[Yinghai Lu]

Rusty found on lguest with trim_bios_range, max_pfn is not right anymore, and
looks e820_remove_range does not work right.

[    0.000000] BIOS-provided physical RAM map:
[    0.000000]  LGUEST: 0000000000000000 - 0000000004000000 (usable)
[    0.000000] Notice: NX (Execute Disable) protection missing in CPU or disabled in BIOS!
[    0.000000] DMI not present or invalid.
[    0.000000] last_pfn = 0x3fa0 max_arch_pfn = 0x100000
[    0.000000] init_memory_mapping: 0000000000000000-0000000003fa0000

root cause is: the e820_remove_range doesn't handle that all cover case.
e820_remove_range(BIOS_START, BIOS_END - BIOS_start, ,,)
get miss one left range.

Make that is corresponding to e820_update_range() to handle that case too.

Reported-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 arch/x86/kernel/e820.c |   24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 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
@@ -519,29 +519,45 @@ u64 __init e820_remove_range(u64 start,
 	printk(KERN_DEBUG "e820 remove range: %016Lx - %016Lx ",
 		       (unsigned long long) start,
 		       (unsigned long long) end);
-	e820_print_type(old_type);
+	if (checktype)
+		e820_print_type(old_type);
 	printk(KERN_CONT "\n");
 
 	for (i = 0; i < e820.nr_map; i++) {
 		struct e820entry *ei = &e820.map[i];
 		u64 final_start, final_end;
+		u64 ei_end;
 
 		if (checktype && ei->type != old_type)
 			continue;
+
+		ei_end = ei->addr + ei->size;
 		/* totally covered? */
-		if (ei->addr >= start &&
-		    (ei->addr + ei->size) <= (start + size)) {
+		if (ei->addr >= start && ei_end <= end) {
 			real_removed_size += ei->size;
 			memset(ei, 0, sizeof(struct e820entry));
 			continue;
 		}
+
+		/* new range is totally covered? */
+		if (ei->addr < start && ei_end > end) {
+			e820_add_region(end, ei_end - end, ei->type);
+			ei->size = start - ei->addr;
+			real_removed_size += size;
+			continue;
+		}
+
 		/* partially covered */
 		final_start = max(start, ei->addr);
-		final_end = min(start + size, ei->addr + ei->size);
+		final_end = min(end, ei_end);
 		if (final_start >= final_end)
 			continue;
 		real_removed_size += final_end - final_start;
 
+		/*
+		 * left range could be head or tail, so need to update
+		 * size at first.
+		 */
 		ei->size -= final_end - final_start;
 		if (ei->addr < final_start)
 			continue;

Re: [PATCH] x86: Make e820_remove_range to handle all covered case

[Rusty Russell]

On Tue, 30 Mar 2010 04:08:29 pm Yinghai Lu wrote:
> 
> Rusty found on lguest with trim_bios_range, max_pfn is not right anymore, and
> looks e820_remove_range does not work right.
> 
> [    0.000000] BIOS-provided physical RAM map:
> [    0.000000]  LGUEST: 0000000000000000 - 0000000004000000 (usable)
> [    0.000000] Notice: NX (Execute Disable) protection missing in CPU or disabled in BIOS!
> [    0.000000] DMI not present or invalid.
> [    0.000000] last_pfn = 0x3fa0 max_arch_pfn = 0x100000
> [    0.000000] init_memory_mapping: 0000000000000000-0000000003fa0000
> 
> root cause is: the e820_remove_range doesn't handle that all cover case.
> e820_remove_range(BIOS_START, BIOS_END - BIOS_start, ,,)
> get miss one left range.
> 
> Make that is corresponding to e820_update_range() to handle that case too.

Tested-by: Rusty Russell <rusty@rustcorp.com.au>

Please put in -urgent...

Thanks,
Rusty.

[tip:x86/urgent] x86: Make e820_remove_range to handle all covered case

[tip-bot for Yinghai Lu]

Commit-ID:  9f3a5f52aa63d3aa4c64a7245153549bb66bad8c
Gitweb:     http://git.kernel.org/tip/9f3a5f52aa63d3aa4c64a7245153549bb66bad8c
Author:     Yinghai Lu <yinghai@kernel.org>
AuthorDate: Mon, 29 Mar 2010 22:38:29 -0700
Committer:  H. Peter Anvin <hpa@zytor.com>
CommitDate: Wed, 31 Mar 2010 17:40:57 -0700

x86: Make e820_remove_range to handle all covered case

Rusty found on lguest with trim_bios_range, max_pfn is not right anymore, and
looks e820_remove_range does not work right.

[    0.000000] BIOS-provided physical RAM map:
[    0.000000]  LGUEST: 0000000000000000 - 0000000004000000 (usable)
[    0.000000] Notice: NX (Execute Disable) protection missing in CPU or disabled in BIOS!
[    0.000000] DMI not present or invalid.
[    0.000000] last_pfn = 0x3fa0 max_arch_pfn = 0x100000
[    0.000000] init_memory_mapping: 0000000000000000-0000000003fa0000

root cause is: the e820_remove_range doesn't handle the all covered
case.  e820_remove_range(BIOS_START, BIOS_END - BIOS_START, ...)
produces a bogus range as a result.

Make it match e820_update_range() by handling that case too.

Reported-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Tested-by: Rusty Russell <rusty@rustcorp.com.au>
LKML-Reference: <4BB18E55.6090903@kernel.org>
Signed-off-by: H. Peter Anvin <hpa@zytor.com>
---
 arch/x86/kernel/e820.c |   24 ++++++++++++++++++++----
 1 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index 740b440..7bca3c6 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -519,29 +519,45 @@ u64 __init e820_remove_range(u64 start, u64 size, unsigned old_type,
 	printk(KERN_DEBUG "e820 remove range: %016Lx - %016Lx ",
 		       (unsigned long long) start,
 		       (unsigned long long) end);
-	e820_print_type(old_type);
+	if (checktype)
+		e820_print_type(old_type);
 	printk(KERN_CONT "\n");
 
 	for (i = 0; i < e820.nr_map; i++) {
 		struct e820entry *ei = &e820.map[i];
 		u64 final_start, final_end;
+		u64 ei_end;
 
 		if (checktype && ei->type != old_type)
 			continue;
+
+		ei_end = ei->addr + ei->size;
 		/* totally covered? */
-		if (ei->addr >= start &&
-		    (ei->addr + ei->size) <= (start + size)) {
+		if (ei->addr >= start && ei_end <= end) {
 			real_removed_size += ei->size;
 			memset(ei, 0, sizeof(struct e820entry));
 			continue;
 		}
+
+		/* new range is totally covered? */
+		if (ei->addr < start && ei_end > end) {
+			e820_add_region(end, ei_end - end, ei->type);
+			ei->size = start - ei->addr;
+			real_removed_size += size;
+			continue;
+		}
+
 		/* partially covered */
 		final_start = max(start, ei->addr);
-		final_end = min(start + size, ei->addr + ei->size);
+		final_end = min(end, ei_end);
 		if (final_start >= final_end)
 			continue;
 		real_removed_size += final_end - final_start;
 
+		/*
+		 * left range could be head or tail, so need to update
+		 * size at first.
+		 */
 		ei->size -= final_end - final_start;
 		if (ei->addr < final_start)
 			continue;