Re: [PATCH] x86: seperate reserve_early and reserve_early_overlap_check

["H. Peter Anvin" <hpa@zytor.com>]

[-- 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);
 }