[PATCH] x86: seperate reserve_early and reserve_early_overlap_check

[PATCH] x86: seperate reserve_early and reserve_early_overlap_check

[Yinghai Lu]

when the area is from find_e820_area(), it could be overlapped with others.

so just add it directly. the new reserve_early()

and rename old reserve_early() to reserve_early_overlap_check()

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

---
 arch/x86/include/asm/e820.h  |    3 ++-
 arch/x86/kernel/e820.c       |   26 +++++++++++++++++++++++++-
 arch/x86/kernel/efi.c        |    4 ++--
 arch/x86/kernel/head32.c     |    6 ++++--
 arch/x86/kernel/head64.c     |    6 ++++--
 arch/x86/kernel/mpparse.c    |    6 ++++--
 arch/x86/kernel/setup.c      |    6 ++++--
 arch/x86/kernel/trampoline.c |    7 ++++---
 arch/x86/xen/mmu.c           |    4 ++--
 arch/x86/xen/setup.c         |    2 +-
 10 files changed, 52 insertions(+), 18 deletions(-)

Index: linux-2.6/arch/x86/include/asm/e820.h
===================================================================
--- linux-2.6.orig/arch/x86/include/asm/e820.h
+++ linux-2.6/arch/x86/include/asm/e820.h
@@ -111,7 +111,8 @@ extern unsigned long end_user_pfn;
 
 extern u64 find_e820_area(u64 start, u64 end, u64 size, u64 align);
 extern u64 find_e820_area_size(u64 start, u64 *sizep, u64 align);
-extern void reserve_early(u64 start, u64 end, char *name);
+void reserve_early(u64 start, u64 end, char *name);
+void reserve_early_overlap_check(u64 start, u64 end, char *name);
 extern void reserve_early_overlap_ok(u64 start, u64 end, char *name);
 extern void free_early(u64 start, u64 end);
 extern void early_res_to_bootmem(u64 start, u64 end);
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
@@ -835,6 +835,30 @@ static void __init drop_overlaps_that_ar
 	}
 }
 
+void __init reserve_early(u64 start, u64 end, char *name)
+{
+	int i;
+	struct early_res *r;
+
+	if (start >= end)
+		return;
+
+	for (i = 0; i < MAX_EARLY_RES; i++) {
+		r = &early_res[i];
+		if (!r->end) {
+			r->start = start;
+			r->end = end;
+			if (name)
+				strncpy(r->name, name, sizeof(r->name) - 1);
+			r->overlap_ok = 0;
+
+			return;
+		}
+	}
+
+	panic("Too many early reservations");
+}
+
 static void __init __reserve_early(u64 start, u64 end, char *name,
 						int overlap_ok)
 {
@@ -891,7 +915,7 @@ void __init reserve_early_overlap_ok(u64
  * range without risk of panic'ing on an overlapping overlap_ok
  * early reservation.
  */
-void __init reserve_early(u64 start, u64 end, char *name)
+void __init reserve_early_overlap_check(u64 start, u64 end, char *name)
 {
 	if (start >= end)
 		return;
Index: linux-2.6/arch/x86/kernel/efi.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/efi.c
+++ linux-2.6/arch/x86/kernel/efi.c
@@ -290,8 +290,8 @@ void __init efi_reserve_early(void)
 		boot_params.efi_info.efi_memdesc_size;
 	memmap.desc_version = boot_params.efi_info.efi_memdesc_version;
 	memmap.desc_size = boot_params.efi_info.efi_memdesc_size;
-	reserve_early(pmap, pmap + memmap.nr_map * memmap.desc_size,
-		      "EFI memmap");
+	reserve_early_overlap_check(pmap,
+			 pmap + memmap.nr_map * memmap.desc_size, "EFI memmap");
 }
 
 #if EFI_DEBUG
Index: linux-2.6/arch/x86/kernel/head32.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/head32.c
+++ linux-2.6/arch/x86/kernel/head32.c
@@ -31,7 +31,8 @@ void __init i386_start_kernel(void)
 {
 	reserve_trampoline_memory();
 
-	reserve_early(__pa_symbol(&_text), __pa_symbol(&__bss_stop), "TEXT DATA BSS");
+	reserve_early_overlap_check(__pa_symbol(&_text),
+				 __pa_symbol(&__bss_stop), "TEXT DATA BSS");
 
 #ifdef CONFIG_BLK_DEV_INITRD
 	/* Reserve INITRD */
@@ -39,7 +40,8 @@ void __init i386_start_kernel(void)
 		u64 ramdisk_image = boot_params.hdr.ramdisk_image;
 		u64 ramdisk_size  = boot_params.hdr.ramdisk_size;
 		u64 ramdisk_end   = ramdisk_image + ramdisk_size;
-		reserve_early(ramdisk_image, ramdisk_end, "RAMDISK");
+		reserve_early_overlap_check(ramdisk_image, ramdisk_end,
+						 "RAMDISK");
 	}
 #endif
 
Index: linux-2.6/arch/x86/kernel/head64.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/head64.c
+++ linux-2.6/arch/x86/kernel/head64.c
@@ -100,7 +100,8 @@ void __init x86_64_start_reservations(ch
 
 	reserve_trampoline_memory();
 
-	reserve_early(__pa_symbol(&_text), __pa_symbol(&__bss_stop), "TEXT DATA BSS");
+	reserve_early_overlap_check(__pa_symbol(&_text),
+				 __pa_symbol(&__bss_stop), "TEXT DATA BSS");
 
 #ifdef CONFIG_BLK_DEV_INITRD
 	/* Reserve INITRD */
@@ -108,7 +109,8 @@ void __init x86_64_start_reservations(ch
 		unsigned long ramdisk_image = boot_params.hdr.ramdisk_image;
 		unsigned long ramdisk_size  = boot_params.hdr.ramdisk_size;
 		unsigned long ramdisk_end   = ramdisk_image + ramdisk_size;
-		reserve_early(ramdisk_image, ramdisk_end, "RAMDISK");
+		reserve_early_overlap_check(ramdisk_image, ramdisk_end,
+						 "RAMDISK");
 	}
 #endif
 
Index: linux-2.6/arch/x86/kernel/mpparse.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/mpparse.c
+++ linux-2.6/arch/x86/kernel/mpparse.c
@@ -671,7 +671,8 @@ static void __init smp_reserve_memory(st
 {
 	unsigned long size = get_mpc_size(mpf->physptr);
 
-	reserve_early(mpf->physptr, mpf->physptr+size, "MP-table mpc");
+	reserve_early_overlap_check(mpf->physptr, mpf->physptr+size,
+					 "MP-table mpc");
 }
 
 static int __init smp_scan_config(unsigned long base, unsigned long length)
@@ -700,7 +701,8 @@ static int __init smp_scan_config(unsign
 			       mpf, (u64)virt_to_phys(mpf));
 
 			mem = virt_to_phys(mpf);
-			reserve_early(mem, mem + sizeof(*mpf), "MP-table mpf");
+			reserve_early_overlap_check(mem, mem + sizeof(*mpf),
+							 "MP-table mpf");
 			if (mpf->physptr)
 				smp_reserve_memory(mpf);
 
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
@@ -299,7 +299,8 @@ static inline void init_gbpages(void)
 static void __init reserve_brk(void)
 {
 	if (_brk_end > _brk_start)
-		reserve_early(__pa(_brk_start), __pa(_brk_end), "BRK");
+		reserve_early_overlap_check(__pa(_brk_start), __pa(_brk_end),
+						 "BRK");
 
 	/* Mark brk area as locked down and no longer taking any
 	   new allocations */
@@ -476,7 +477,8 @@ static void __init reserve_early_setup_d
 	while (pa_data) {
 		data = early_memremap(pa_data, sizeof(*data));
 		sprintf(buf, "setup data %x", data->type);
-		reserve_early(pa_data, pa_data+sizeof(*data)+data->len, buf);
+		reserve_early_overlap_check(pa_data,
+					 pa_data+sizeof(*data)+data->len, buf);
 		pa_data = data->next;
 		early_iounmap(data, sizeof(*data));
 	}
Index: linux-2.6/arch/x86/kernel/trampoline.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/trampoline.c
+++ linux-2.6/arch/x86/kernel/trampoline.c
@@ -22,11 +22,12 @@ void __init reserve_trampoline_memory(vo
 	 * FIXME: Don't need the extra page at 4K, but need to fix
 	 * trampoline before removing it. (see the GDT stuff)
 	 */
-	reserve_early(PAGE_SIZE, PAGE_SIZE + PAGE_SIZE, "EX TRAMPOLINE");
+	reserve_early_overlap_check(PAGE_SIZE, PAGE_SIZE + PAGE_SIZE,
+					 "EX TRAMPOLINE");
 #endif
 	/* Has to be in very low memory so we can execute real-mode AP code. */
-	reserve_early(TRAMPOLINE_BASE, TRAMPOLINE_BASE + TRAMPOLINE_SIZE,
-			"TRAMPOLINE");
+	reserve_early_overlap_check(TRAMPOLINE_BASE,
+			 TRAMPOLINE_BASE + TRAMPOLINE_SIZE, "TRAMPOLINE");
 }
 
 /*
Index: linux-2.6/arch/x86/xen/mmu.c
===================================================================
--- linux-2.6.orig/arch/x86/xen/mmu.c
+++ linux-2.6/arch/x86/xen/mmu.c
@@ -1751,7 +1751,7 @@ __init pgd_t *xen_setup_kernel_pagetable
 	__xen_write_cr3(true, __pa(pgd));
 	xen_mc_issue(PARAVIRT_LAZY_CPU);
 
-	reserve_early(__pa(xen_start_info->pt_base),
+	reserve_early_overlap_check(__pa(xen_start_info->pt_base),
 		      __pa(xen_start_info->pt_base +
 			   xen_start_info->nr_pt_frames * PAGE_SIZE),
 		      "XEN PAGETABLES");
@@ -1789,7 +1789,7 @@ __init pgd_t *xen_setup_kernel_pagetable
 
 	pin_pagetable_pfn(MMUEXT_PIN_L3_TABLE, PFN_DOWN(__pa(swapper_pg_dir)));
 
-	reserve_early(__pa(xen_start_info->pt_base),
+	reserve_early_overlap_check(__pa(xen_start_info->pt_base),
 		      __pa(xen_start_info->pt_base +
 			   xen_start_info->nr_pt_frames * PAGE_SIZE),
 		      "XEN PAGETABLES");
Index: linux-2.6/arch/x86/xen/setup.c
===================================================================
--- linux-2.6.orig/arch/x86/xen/setup.c
+++ linux-2.6/arch/x86/xen/setup.c
@@ -61,7 +61,7 @@ char * __init xen_memory_setup(void)
 	 *  - xen_start_info
 	 * See comment above "struct start_info" in <xen/interface/xen.h>
 	 */
-	reserve_early(__pa(xen_start_info->mfn_list),
+	reserve_early_overlap_check(__pa(xen_start_info->mfn_list),
 		      __pa(xen_start_info->pt_base),
 			"XEN START INFO");
 

Re: [PATCH] x86: seperate reserve_early and reserve_early_overlap_check

[H. Peter Anvin]

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

On 11/25/2009 12:58 AM, Yinghai Lu wrote:
> 
> when the area is from find_e820_area(), it could be overlapped with others.
> 
> so just add it directly. the new reserve_early()
> 
> and rename old reserve_early() to reserve_early_overlap_check()
> 
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>

Hi Yinghai,

I had this patch in my queue but it looks like I had overlooked it as it
arrived during the U.S. holiday; I apologize profusely!

I'm concerned about several things with this patch, which doesn't mean
it isn't fulfilling a genuine need:

1. Renaming reserve_early() to reserve_early_overlap_check() is most
likely going to get overlooked, and people will use the "new"
reserve_early() thinking that they got the old one.

2. This creates overlapping ranges in the reservation array itself.

What it looks to me is what we need is actually a
reserve_early_clobber() which does what the current reserve_early() does
except that it ignores the overlap_ok flag on existing reservations.

I have attached an untested patch to do that.  Note that I don't have
any callers for reserve_early_clobber(), since one effect of changing
the semantics of an existing function in the way you did is that the
patch contains the call sites that *didn't* need modification rather
than the one that *did* need modification.  The call sites that want the
new semantics need to be modified.  As such, it's possible that the
comment I added is completely wrong, I really need some further
information on this.

[Note: the function __reserve_early() hasn't actually changed; I just
moved it ahead of drop_overlaps().]

Sorry again for the delay.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.


[-- Attachment #2: diff --]
[-- Type: text/plain, Size: 4924 bytes --]

diff --git a/arch/x86/include/asm/e820.h b/arch/x86/include/asm/e820.h
index 761249e..b45bf41 100644
--- a/arch/x86/include/asm/e820.h
+++ b/arch/x86/include/asm/e820.h
@@ -113,6 +113,7 @@ extern u64 find_e820_area(u64 start, u64 end, u64 size, u64 align);
 extern u64 find_e820_area_size(u64 start, u64 *sizep, u64 align);
 extern void reserve_early(u64 start, u64 end, char *name);
 extern void reserve_early_overlap_ok(u64 start, u64 end, char *name);
+extern void reserve_early_clobber(u64 start, u64 end, char *name);
 extern void free_early(u64 start, u64 end);
 extern void early_res_to_bootmem(u64 start, u64 end);
 extern u64 early_reserve_e820(u64 startt, u64 sizet, u64 align);
diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index d17d482..afe5023 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -768,9 +768,31 @@ static void __init drop_range(int i)
 	early_res[j - 1].end = 0;
 }
 
+static void __init __reserve_early(u64 start, u64 end, char *name,
+				   int overlap_ok)
+{
+	int i;
+	struct early_res *r;
+
+	i = find_overlapped_early(start, end);
+	if (i >= MAX_EARLY_RES)
+		panic("Too many early reservations");
+	r = &early_res[i];
+	if (r->end)
+		panic("Overlapping early reservations "
+		      "%llx-%llx %s to %llx-%llx %s\n",
+		      start, end - 1, name?name:"", r->start,
+		      r->end - 1, r->name);
+	r->start = start;
+	r->end = end;
+	r->overlap_ok = overlap_ok;
+	if (name)
+		strncpy(r->name, name, sizeof(r->name) - 1);
+}
+
 /*
  * Split any existing ranges that:
- *  1) are marked 'overlap_ok', and
+ *  1) are marked 'overlap_ok' (unless 'force' is set), and
  *  2) overlap with the stated range [start, end)
  * into whatever portion (if any) of the existing range is entirely
  * below or entirely above the stated range.  Drop the portion
@@ -778,13 +800,14 @@ static void __init drop_range(int i)
  * which will allow the caller of this routine to then add that
  * stated range without conflicting with any existing range.
  */
-static void __init drop_overlaps_that_are_ok(u64 start, u64 end)
+static void __init drop_overlaps(u64 start, u64 end, int force)
 {
 	int i;
 	struct early_res *r;
 	u64 lower_start, lower_end;
 	u64 upper_start, upper_end;
 	char name[16];
+	int overlap_ok;
 
 	for (i = 0; i < MAX_EARLY_RES && early_res[i].end; i++) {
 		r = &early_res[i];
@@ -798,7 +821,7 @@ static void __init drop_overlaps_that_are_ok(u64 start, u64 end)
 		 * panic "Overlapping early reservations"
 		 * when it hits this overlap.
 		 */
-		if (!r->overlap_ok)
+		if (!force && !r->overlap_ok)
 			return;
 
 		/*
@@ -811,6 +834,7 @@ static void __init drop_overlaps_that_are_ok(u64 start, u64 end)
 		/* 1. Note any non-overlapping (lower or upper) ranges. */
 		strncpy(name, r->name, sizeof(name) - 1);
 
+		overlap_ok = r->overlap_ok;
 		lower_start = lower_end = 0;
 		upper_start = upper_end = 0;
 		if (r->start < start) {
@@ -829,34 +853,14 @@ static void __init drop_overlaps_that_are_ok(u64 start, u64 end)
 
 		/* 3. Add back in any non-overlapping ranges. */
 		if (lower_end)
-			reserve_early_overlap_ok(lower_start, lower_end, name);
+			__reserve_early(lower_start, lower_end,
+					name, overlap_ok);
 		if (upper_end)
-			reserve_early_overlap_ok(upper_start, upper_end, name);
+			__reserve_early(upper_start, upper_end,
+					name, overlap_ok);
 	}
 }
 
-static void __init __reserve_early(u64 start, u64 end, char *name,
-						int overlap_ok)
-{
-	int i;
-	struct early_res *r;
-
-	i = find_overlapped_early(start, end);
-	if (i >= MAX_EARLY_RES)
-		panic("Too many early reservations");
-	r = &early_res[i];
-	if (r->end)
-		panic("Overlapping early reservations "
-		      "%llx-%llx %s to %llx-%llx %s\n",
-		      start, end - 1, name?name:"", r->start,
-		      r->end - 1, r->name);
-	r->start = start;
-	r->end = end;
-	r->overlap_ok = overlap_ok;
-	if (name)
-		strncpy(r->name, name, sizeof(r->name) - 1);
-}
-
 /*
  * A few early reservtations come here.
  *
@@ -879,7 +883,7 @@ static void __init __reserve_early(u64 start, u64 end, char *name,
  */
 void __init reserve_early_overlap_ok(u64 start, u64 end, char *name)
 {
-	drop_overlaps_that_are_ok(start, end);
+	drop_overlaps(start, end, 0);
 	__reserve_early(start, end, name, 1);
 }
 
@@ -896,7 +900,22 @@ void __init reserve_early(u64 start, u64 end, char *name)
 	if (start >= end)
 		return;
 
-	drop_overlaps_that_are_ok(start, end);
+	drop_overlaps(start, end, 0);
+	__reserve_early(start, end, name, 0);
+}
+
+/*
+ * Early reservations during the scan of the e820 array itself
+ * can come in here; this is used when the information available
+ * to us may be internally inconsistent.  This force-drops any
+ * previous range that conflicts with this one.
+ */
+void __init reserve_early_clobber(u64 start, u64 end, char *name)
+{
+	if (start >= end)
+		return;
+
+	drop_overlaps(start, end, 1);
 	__reserve_early(start, end, name, 0);
 }
 

Re: [PATCH] x86: seperate reserve_early and reserve_early_overlap_check

[Yinghai Lu]

H. Peter Anvin wrote:
> On 11/25/2009 12:58 AM, Yinghai Lu wrote:
>> when the area is from find_e820_area(), it could be overlapped with others.
>>
>> so just add it directly. the new reserve_early()
>>
>> and rename old reserve_early() to reserve_early_overlap_check()
>>
>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> 
> Hi Yinghai,
> 
> I had this patch in my queue but it looks like I had overlooked it as it
> arrived during the U.S. holiday; I apologize profusely!
> 
> I'm concerned about several things with this patch, which doesn't mean
> it isn't fulfilling a genuine need:
> 
> 1. Renaming reserve_early() to reserve_early_overlap_check() is most
> likely going to get overlooked, and people will use the "new"
> reserve_early() thinking that they got the old one.
kill reserve_early()
and use
reserve_early_overlap_check()
reserve_early_overlap_not_check()
?
> 
> 2. This creates overlapping ranges in the reservation array itself.

why?

if the range is retrieved from find_e820_area(). that range should be safe.
we could add the early_res directly.

> 
> What it looks to me is what we need is actually a
> reserve_early_clobber() which does what the current reserve_early() does
> except that it ignores the overlap_ok flag on existing reservations.
> 
> I have attached an untested patch to do that.  Note that I don't have
> any callers for reserve_early_clobber(), since one effect of changing
> the semantics of an existing function in the way you did is that the
> patch contains the call sites that *didn't* need modification rather
> than the one that *did* need modification.  The call sites that want the
> new semantics need to be modified.  As such, it's possible that the
> comment I added is completely wrong, I really need some further
> information on this.
> 
> [Note: the function __reserve_early() hasn't actually changed; I just
> moved it ahead of drop_overlaps().]
> 

your patch seems to solve the multiple overlap ok problem.

these overlap check stuff is added by SGI guys to bandit their bios problem. maybe we can kill them at some point.

YH

Re: [PATCH] x86: seperate reserve_early and reserve_early_overlap_check

[H. Peter Anvin]

On 12/02/2009 06:31 PM, Yinghai Lu wrote:
>> 1. Renaming reserve_early() to reserve_early_overlap_check() is most
>> likely going to get overlooked, and people will use the "new"
>> reserve_early() thinking that they got the old one.
> kill reserve_early()
> and use
> reserve_early_overlap_check()
> reserve_early_overlap_not_check()

I would think that we should just leave the current reserve_early() in
place... it is what we want most of the time.

I guess I'd like to know specifically when one does *not* want an
overlap check, which is really the issue here.

>> 2. This creates overlapping ranges in the reservation array itself.
> 
> why?
> 
> if the range is retrieved from find_e820_area(). that range should be safe.
> we could add the early_res directly.

If there is no overlap, the current reserve_early() will work just fine.
 If there is overlap, then your code would create overlapping
reservations in the array, since it doesn't seem to check.

Either the new interface is unnecessary, or it is bad... I don't see any
other alternatives.  Unless the only point is to try to shave off a few
microseconds, but if so, it seems rather pointless to seek to the end of
the array first...

> 
> your patch seems to solve the multiple overlap ok problem.
> 
> these overlap check stuff is added by SGI guys to bandit their bios problem. maybe we can kill them at some point.
> 

It's probably a good idea to have them... errors happen.

One thing I would like to see is to change the underlying data structure
to instead of having (start, end, attributes) for each reservation, have
(start, attributes) for all address space.  Such a list would by
definition always be sorted, and overlaps would be trivial to detect.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.