Skip to contentSkip to conversation

x86, efi: Do not reserve boot services regions within reserved areas

PATCH Superseded Maarten Lankhorst 2 participants 4 messages Jun 14, 2011

Subsystems (3)
x86 x86 MM EFI

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Maarten Lankhorst

author
#0 lore raw

PATCH 1/1 x86, efi: Do not reserve boot services regions within reserved areas

Commit 916f676f8dc started reserving boot service code since some systems require you to keep that code around until SetVirtualAddressMap is called.

However, in some cases those areas will overlap with reserved regions. The proper medium-term fix is to fix the bootloader to prevent the conflicts from occuring by moving the kernel to a better position, but the kernel should check for this possibility, and only reserve regions which can be reserved.

Signed-off-by Maarten Lankhorst <[email protected]>

---

arch/x86/include/asm/memblock.h +1 -1
index 19ae14b..0cd3800 100644
@@ -4,7 +4,6 @@
 #define ARCH_DISCARD_MEMBLOCK
 
 u64 memblock_x86_find_in_range_size(u64 start, u64 *sizep, u64 align);
-void memblock_x86_to_bootmem(u64 start, u64 end);
 
 void memblock_x86_reserve_range(u64 start, u64 end, char *name);
 void memblock_x86_free_range(u64 start, u64 end);

@@ -19,5 +18,6 @@ u64 memblock_x86_hole_size(u64 start, u64 end);
 u64 memblock_x86_find_in_range_node(int nid, u64 start, u64 end, u64 size, u64 align);
 u64 memblock_x86_free_memory_in_range(u64 addr, u64 limit);
 u64 memblock_x86_memory_in_range(u64 addr, u64 limit);
+bool memblock_x86_check_reserved_size(u64 *addrp, u64 *sizep, u64 align);
 
 #endif
arch/x86/mm/memblock.c +2 -2
index aa11693..992da5e 100644
@@ -8,7 +8,7 @@
 #include <linux/range.h>
 
 /* Check for already reserved areas */
-static bool __init check_with_memblock_reserved_size(u64 *addrp, u64 *sizep, u64 align)
+bool __init memblock_x86_check_reserved_size(u64 *addrp, u64 *sizep, u64 align)
 {
 	struct memblock_region *r;
 	u64 addr = *addrp, last;

@@ -59,7 +59,7 @@ u64 __init memblock_x86_find_in_range_size(u64 start, u64 *sizep, u64 align)
 		if (addr >= ei_last)
 			continue;
 		*sizep = ei_last - addr;
-		while (check_with_memblock_reserved_size(&addr, sizep, align))
+		while (memblock_x86_check_reserved_size(&addr, sizep, align))
 			;
 
 		if (*sizep)
arch/x86/platform/efi/efi.c +31 -5
index 0d3a4fa..fae8448 100644
@@ -310,14 +310,32 @@ void __init efi_reserve_boot_services(void)
 
 	for (p = memmap.map; p < memmap.map_end; p += memmap.desc_size) {
 		efi_memory_desc_t *md = p;
-		unsigned long long start = md->phys_addr;
-		unsigned long long size = md->num_pages << EFI_PAGE_SHIFT;
+		u64 start = md->phys_addr;
+		u64 size = md->num_pages << EFI_PAGE_SHIFT;
 
 		if (md->type != EFI_BOOT_SERVICES_CODE &&
 		    md->type != EFI_BOOT_SERVICES_DATA)
 			continue;
-
-		memblock_x86_reserve_range(start, start + size, "EFI Boot");
+		/* Only reserve where possible:
+		 * - Not within any already allocated areas
+		 * - Not over any memory area (really needed, if above?)
+		 * - Not within any part of the kernel
+		 * - Not the bios reserved area (0x0-0x10000, depending on config
+		*/
+		if ((start+size >= virt_to_phys(_text)
+				&& start <= virt_to_phys(_end)) ||
+			!e820_all_mapped(start, start+size, E820_RAM) ||
+			memblock_x86_check_reserved_size(&start, &size,
+							1<<EFI_PAGE_SHIFT)) {
+			/* Could not reserve, skip it */
+			md->num_pages = 0;
+			printk(KERN_INFO PFX "Could not reserve boot area "
+				"[0x%llx-0x%llx)\n", start, start+size);
+		} else {
+			memblock_x86_reserve_range(start, start+size, "EFI Boot");
+			printk(KERN_INFO PFX "Reserved boot area "
+				"[0x%llx-0x%llx)\n", start, start+size);
+		}
 	}
 }
 

@@ -334,6 +352,12 @@ static void __init efi_free_boot_services(void)
 		    md->type != EFI_BOOT_SERVICES_DATA)
 			continue;
 
+		/* Could not reserve boot area */
+		if (!size)
+			continue;
+
+		printk(KERN_INFO PFX "Freeing boot area "
+			"[0x%llx-0x%llx)\n", start, start+size);
 		free_bootmem_late(start, size);
 	}
 }

@@ -473,7 +497,9 @@ void __init efi_init(void)
 
 	if (memmap.desc_size != sizeof(efi_memory_desc_t))
 		printk(KERN_WARNING
-		  "Kernel-defined memdesc doesn't match the one from EFI!\n");
+		  "Kernel-defined memdesc (%lu) "
+			"doesn't match the one from EFI(%lu)!\n",
+			sizeof(efi_memory_desc_t), memmap.desc_size);
 
 	if (add_efi_memmap)
 		do_add_efi_memmap();
#1 lore raw

arch/x86/platform/efi/efi.c static void __init efi_free_boot_services(void) · line 360

Quoted 6 lines · Maarten Lankhorst
arch/x86/platform/efi/efi.c +6 -0
+		/* Could not reserve boot area */
+		if (!size)
+			continue;
+
+		printk(KERN_INFO PFX "Freeing boot area "
+			"[0x%llx-0x%llx)\n", start, start+size);

Probably don't need the printk - we could be dumping a few hundred of those.

arch/x86/platform/efi/efi.c void __init efi_init(void) · line 502

Quoted 4 lines
arch/x86/platform/efi/efi.c +3 -1
-		  "Kernel-defined memdesc doesn't match the one from EFI!\n");
+		  "Kernel-defined memdesc (%lu) "
+			"doesn't match the one from EFI(%lu)!\n",
+			sizeof(efi_memory_desc_t), memmap.desc_size);

And drop this hunk. It's entirely valid for an implementation to do this, we should just delete the message entirely.

Signature · 1 line
Matthew Garrett | [email protected]
author
#2 lore raw

Op 14-06-11 17:21, Matthew Garrett schreef:

Quoted 10 lines · Maarten Lankhorst

arch/x86/platform/efi/efi.c static void __init efi_free_boot_services(void) · line 360

Quoted 7 lines
arch/x86/platform/efi/efi.c +6 -0
+		/* Could not reserve boot area */
+		if (!size)
+			continue;
+
+		printk(KERN_INFO PFX "Freeing boot area "
+			"[0x%llx-0x%llx)\n", start, start+size);

Probably don't need the printk - we could be dumping a few hundred of those.

Ok, I believe you are right about the succesful ones, since memblock=debug will let you see those anyhow.

Should I change the unsuccesful ones to use memblock_dbg instead of printk? In that case you can still debug it if needed. Same question for free path, change printk to memblock_dbg?

Quoted 6 lines

arch/x86/platform/efi/efi.c void __init efi_init(void) · line 502

Quoted 4 lines
arch/x86/platform/efi/efi.c +3 -1
-		  "Kernel-defined memdesc doesn't match the one from EFI!\n");
+		  "Kernel-defined memdesc (%lu) "
+			"doesn't match the one from EFI(%lu)!\n",
+			sizeof(efi_memory_desc_t), memmap.desc_size);

And drop this hunk. It's entirely valid for an implementation to do this, we should just delete the message entirely.

Ok. Just wasn't sure what to make of it, since that message kept popping up here.

~Maarten

#3 lore raw
Quoted 11 lines · Maarten Lankhorst

Op 14-06-11 17:21, Matthew Garrett schreef:

Quoted 10 lines · Maarten Lankhorst

arch/x86/platform/efi/efi.c static void __init efi_free_boot_services(void) · line 360

Quoted 7 lines
arch/x86/platform/efi/efi.c +6 -0
+		/* Could not reserve boot area */
+		if (!size)
+			continue;
+
+		printk(KERN_INFO PFX "Freeing boot area "
+			"[0x%llx-0x%llx)\n", start, start+size);

Probably don't need the printk - we could be dumping a few hundred of those.

Quoted 6 lines

Ok, I believe you are right about the succesful ones, since memblock=debug will let you see those anyhow.

Should I change the unsuccesful ones to use memblock_dbg instead of printk? In that case you can still debug it if needed. Same question for free path, change printk to memblock_dbg?

I'd prefer just not printing anything here. We're engaging in a bug workaround without any guarantee that the bug affects a machine, so increasing the message load may result in people thinking something's wrong even when everything's fine. Limiting it to memblock_dbg seems sane.

Signature · 1 line
Matthew Garrett | [email protected]