Bug#815125: Boot failure with Debian linux 4.4.2 package
On Wed, 09 Mar, at 10:56:07PM, Matt Fleming wrote:
> On Wed, 09 Mar, at 11:01:18PM, Alexis Murzeau wrote:
> >
> > Indeed I get the "Could not reserve range" message, and with a kernel
> > v4.3 the physical address 0x1 contains the value 1.
> > And this patch works and make a unmodified + this patch 4.4 debian
> > kernel boots, nice well found :)
>
> Great, thanks for testing.
>
> > However, now a bad page state is reported in dmesg (which doesn't seem
> > to affect the kernel to me as a user but might hide something buggy) :
> > [ 0.030096] BUG: Bad page state in process swapper/0 pfn:00000
> > [ 0.030100] page:ffffea0000000000 count:0 mapcount:1 mapping:
> > (null) index:0x0
> > [ 0.030102] flags: 0x0()
> >
> > The efi_free_boot_services function seems to expect size == 0 to not
> > free non reserved memory according to commit 7d68dc3.
> > Not sure if this bad page state is related to this patch though, but I
> > don't get this with the 4.3 kernel.
>
> Yeah, it's definitely related to my quick and dirty patch. I'll have a
> think about how to fix it properly tomorrow morning.
Alexis, could you, and anybody else that hit this bug, please try out
the attached patch? If it works for you I'll pull it into the EFI tree
ASAP - the merge window is approaching fast.
commit 7738188068b9
Author: Matt Fleming <matt@codeblueprint.co.uk>
Date: Wed Mar 9 14:34:24 2016 +0000
x86/efi: Allow reserving E820_RESERVED regions
Some machines place EFI regions in the zero page (physical address
0x00000000) and we need to ensure that they're mapped into the EFI
page tables now that the kernel doesn't do it for us with
trim_bios_range().
commit 7d68dc3f1003 ("x86, efi: Do not reserve boot services regions
within reserved areas").
Reported-by: Alexis Murzeau <amurzeau@gmail.com>
Link: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=815125
Cc: Maarten Lankhorst <maarten.lankhorst@canonical.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Ben Hutchings <ben@decadent.org.uk>
Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
index 2326bf51978f..ae850932dd16 100644
--- a/arch/x86/platform/efi/quirks.c
+++ b/arch/x86/platform/efi/quirks.c
@@ -164,6 +164,25 @@ efi_status_t efi_query_variable_store(u32 attributes, unsigned long size,
EXPORT_SYMBOL_GPL(efi_query_variable_store);
/*
+ * This function must be used to ensure we do not free already reserved
+ * regions, since that means they're owned by somebody else. Only
+ * reserve (and then free) regions:
+ *
+ * - Not within any part of the kernel
+ * - Not the bios reserved area (E820_RESERVED)
+ */
+static bool can_free_region(u64 start, u64 size)
+{
+ if (start + size > __pa_symbol(_text) && start <= __pa_symbol(_end))
+ return false;
+
+ if (!e820_all_mapped(start, start+size, E820_RAM))
+ return false;
+
+ return true;
+}
+
+/*
* The UEFI specification makes it clear that the operating system is free to do
* whatever it wants with boot services code after ExitBootServices() has been
* called. Ignoring this recommendation a significant bunch of EFI implementations
@@ -180,26 +199,50 @@ void __init efi_reserve_boot_services(void)
efi_memory_desc_t *md = p;
u64 start = md->phys_addr;
u64 size = md->num_pages << EFI_PAGE_SHIFT;
+ bool already_reserved;
if (md->type != EFI_BOOT_SERVICES_CODE &&
md->type != EFI_BOOT_SERVICES_DATA)
continue;
- /* 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
- */
- if ((start + size > __pa_symbol(_text)
- && start <= __pa_symbol(_end)) ||
- !e820_all_mapped(start, start+size, E820_RAM) ||
- memblock_is_region_reserved(start, size)) {
- /* Could not reserve, skip it */
- md->num_pages = 0;
- memblock_dbg("Could not reserve boot range [0x%010llx-0x%010llx]\n",
- start, start+size-1);
- } else
+
+ already_reserved = memblock_is_region_reserved(start, size);
+
+ /*
+ * Because the following memblock_reserve() is paired
+ * with free_bootmem_late() for this region in
+ * efi_free_boot_services(), we must be extremely
+ * careful not to reserve, and subsequently free,
+ * critical regions of memory (like the kernel image) or
+ * those regions that somebody else has already
+ * reserved.
+ *
+ * A good example of a critical region is the zero page
+ * (first 4Kb of memory), which may contain boot
+ * services code/data but is marked E820_RESERVED by
+ * trim_bios_range().
+ */
+ if (!already_reserved) {
memblock_reserve(start, size);
+
+ /*
+ * If we are the first to reserve the region, no
+ * one else cares about it. We own it and can
+ * free it later.
+ */
+ if (can_free_region(start, size))
+ continue;
+ }
+
+ /*
+ * We don't own the region.
+ *
+ * Setting this bit for a boot services region really
+ * doesn't make sense as far as the firmware is
+ * concerned, but it does provide us with a way to tag
+ * those regions that must not be paired with
+ * free_bootmem_late().
+ */
+ md->attribute |= EFI_MEMORY_RUNTIME;
}
}
@@ -216,8 +259,8 @@ void __init efi_free_boot_services(void)
md->type != EFI_BOOT_SERVICES_DATA)
continue;
- /* Could not reserve boot area */
- if (!size)
+ /* Do not free, someone else owns it */
+ if (md->attribute & EFI_MEMORY_RUNTIME)
continue;
free_bootmem_late(start, size);
Reply to: