[PATCH] Re: EFI memory map

[Emmanuel Dreyfus <manu%netbsd.org@localhost>]

On Thu, Jan 02, 2020 at 03:55:31PM +0000, Emmanuel Dreyfus wrote:
> And indeed, studying the crash in ddb shows it happens when
> accessing a physical address that is excluded by x86_fake_clusters() 
> but included by EFI memory map.

I think the problem lies in sys/arch/x86/x86/x86_machdep.c:init_x86_vm()
The function loops on the memory segments provided by EFI/BIOS and 
call x86_load_region() on the ones that can be used.

The function considers the case of a segment that spans the
addresses used by the kernel and splits the segments in that case,
but it does not consider the cases where the segment is included in
kernel, or if there is a leading or trailing part that overlaps the
kernel. 

The bug happens because a segment overlapping the kernel is used
by x86_load_region(). 

Attached patch adds code for that three extra cases, and it makes
the bug vanish. Is it a reasonable fix? Or did I miss something?

-- 
Emmanuel Dreyfus
manu%netbsd.org@localhost

Index: sys/arch/x86/x86/x86_machdep.c
===================================================================
RCS file: /cvsroot/src/sys/arch/x86/x86/x86_machdep.c,v
retrieving revision 1.133
diff -U4 -r1.133 x86_machdep.c
--- sys/arch/x86/x86/x86_machdep.c	3 Dec 2019 15:20:59 -0000	1.133
+++ sys/arch/x86/x86/x86_machdep.c	6 Jan 2020 07:54:13 -0000
@@ -667,9 +667,9 @@
 		size = bim->entry[x].size;
 		type = bim->entry[x].type;
 #ifdef DEBUG_MEMLOAD
 		printf("MEMMAP: 0x%016" PRIx64 "-0x%016" PRIx64
-		    ", size=0x%016" PRIx64 ", type=%d(%s)\n",
+		    "\n\tsize=0x%016" PRIx64 ", type=%d(%s)\n",
 		    addr, addr + size - 1, size, type,
 		    (type == BIM_Memory) ?  "Memory" :
 		    (type == BIM_Reserved) ?  "Reserved" :
 		    (type == BIM_ACPI) ? "ACPI" :
@@ -907,29 +907,98 @@
 		seg_end = cluster->start + cluster->size;
 		seg_start1 = 0;
 		seg_end1 = 0;
 
+#ifdef DEBUG_MEMLOAD
+		printf("segment %" PRIx64 " - %" PRIx64 "\n",
+		    seg_start, seg_end);
+#endif
+
 		/* Skip memory before our available starting point. */
-		if (seg_end <= lowmem_rsvd)
+		if (seg_end <= lowmem_rsvd) {
+#ifdef DEBUG_MEMLOAD
+			printf("discard segment below starting point "
+			    "%" PRIx64 " - %" PRIx64 "\n", seg_start, seg_end);
+#endif
 			continue;
+		}
 
 		if (seg_start <= lowmem_rsvd && lowmem_rsvd < seg_end) {
 			seg_start = lowmem_rsvd;
-			if (seg_start == seg_end)
+			if (seg_start == seg_end) {
+#ifdef DEBUG_MEMLOAD
+				printf("discard segment below starting point "
+				    "%" PRIx64 " - %" PRIx64 "\n",
+				    seg_start, seg_end);
+
+
+#endif
 				continue;
+			}
 		}
 
 		/*
 		 * If this segment contains the kernel, split it in two, around
 		 * the kernel.
+		 *  [seg_start                       seg_end]
+		 *             [pa_kstart  pa_kend]
 		 */
 		if (seg_start <= pa_kstart && pa_kend <= seg_end) {
+#ifdef DEBUG_MEMLOAD
+			printf("split kernel overlapping to "
+			    "%" PRIx64 " - %" PRIx64 " and "
+			    "%" PRIx64 " - %" PRIx64 "\n",
+			    seg_start, pa_kstart, pa_kend, seg_end);
+#endif
 			seg_start1 = pa_kend;
 			seg_end1 = seg_end;
 			seg_end = pa_kstart;
 			KASSERT(seg_end < seg_end1);
 		}
 
+		/*
+		 * Discard a segment inside the kernel
+		 *  [pa_kstart                       pa_kend]
+		 *             [seg_start  seg_end]
+		 */
+		if (pa_kstart < seg_start && seg_end < pa_kend) {
+#ifdef DEBUG_MEMLOAD
+			printf("discard complete kernel overlap "
+			    "%" PRIx64 " - %" PRIx64 "\n", seg_start, seg_end);
+#endif
+			continue;
+		}
+
+		/*
+		 * Discard leading hunk that overlaps the kernel
+		 *  [pa_kstart             pa_kend]
+		 *            [seg_start            seg_end]
+		 */
+		if (pa_kstart < seg_start &&
+		    seg_start < pa_kend &&
+		    pa_kend < seg_end) {
+#ifdef DEBUG_MEMLOAD
+			printf("discard leading kernel overlap "
+			    "%" PRIx64 " - %" PRIx64 "\n", seg_start, pa_kend);
+#endif
+			seg_start = pa_kend;
+		}
+
+		/*
+		 * Discard trailing hunk that overlaps the kernel
+		 *             [pa_kstart            pa_kend]
+		 *  [seg_start              seg_end]
+		 */
+		if (seg_start < pa_kstart &&
+		    pa_kstart < seg_end &&
+		    seg_end < pa_kend) {
+#ifdef DEBUG_MEMLOAD
+			printf("discard trailing kernel overlap "
+			    "%" PRIx64 " - %" PRIx64 "\n", pa_kstart, seg_end);
+#endif
+			seg_end = pa_kstart;
+		}
+		
 		/* First hunk */
 		if (seg_start != seg_end) {
 			x86_load_region(seg_start, seg_end);
 		}