[PATCH] arm64: kdump: retain reserved memory regions

Mon Jan 29 00:12:27 PST 2018

James,

On Fri, Jan 19, 2018 at 11:39:58AM +0000, James Morse wrote:
> Hi Akashi,
> 
> On 11/01/18 11:38, AKASHI Takahiro wrote:
> > On Wed, Jan 10, 2018 at 11:26:55AM +0000, James Morse wrote:
> >> On 10/01/18 10:09, AKASHI Takahiro wrote:
> >>> This is a fix against the issue that crash dump kernel may hang up
> >>> during booting, which can happen on any ACPI-based system with "ACPI
> >>> Reclaim Memory."
> 
> >>> (diagnosis)
> >>> * This fault is a data abort, alignment fault (ESR=0x96000021)
> >>>   during reading out ACPI table.
> >>> * Initial ACPI tables are normally stored in system ram and marked as
> >>>   "ACPI Reclaim memory" by the firmware.
> >>> * After the commit f56ab9a5b73c ("efi/arm: Don't mark ACPI reclaim
> >>>   memory as MEMBLOCK_NOMAP"), those regions' attribute were changed
> >>>   removing NOMAP bit and they are instead "memblock-reserved".
> >>> * When crash dump kernel boots up, it tries to accesses ACPI tables by
> >>>   ioremap'ing them (through acpi_os_ioremap()).
> >>> * Since those regions are not included in device tree's
> >>>   "usable-memory-range" and so not recognized as part of crash dump
> >>>   kernel's system ram, ioremap() will create a non-cacheable mapping here.
> >>
> >> Ugh, because acpi_os_ioremap() looks at the efi memory map through the prism of
> >> what we pulled into memblock, which is different during kdump.
> >>
> >> Is an alternative to teach acpi_os_ioremap() to ask
> >> efi_mem_attributes() directly for the attributes to use?
> >> (e.g. arch_apei_get_mem_attribute())
> > 
> > I didn't think of this approach.
> > Do you mean a change like the patch below?
> 
> Yes. Aha, you can pretty much re-use the helper directly.
> 
> It was just a suggestion, removing the extra abstraction that is causing the bug
> could be cleaner ...
> 
> > (I'm still debugging this code since the kernel fails to boot.)
> 
> ... but might be too fragile.
> 
> There are points during boot when the EFI memory map isn't mapped.

Right, this was a problem for my patch.
Attached is the revised and workable one.
Efi_memmap_init_late() may alternatively be called in acpi_early_init() or
even in acpi_os_ioremap(), but either way it looks a bit odd.

Thanks,
-Takahiro AKASHI

> I think that
> helper will return 'device memory' for everything when this happens.
> 
> 
> Thanks,
> 
> James
===8<===
>From c88f4c8106ba7a918c835b1cdf538b1d21019863 Mon Sep 17 00:00:00 2001
From: AKASHI Takahiro <takahiro.akashi at linaro.org>
Date: Mon, 29 Jan 2018 15:07:43 +0900
Subject: [PATCH] arm64: kdump: make acpi_os_ioremap() more generic

---
 arch/arm64/include/asm/acpi.h | 23 ++++++++++++++++-------
 arch/arm64/kernel/acpi.c      |  7 ++-----
 init/main.c                   |  4 ++++
 3 files changed, 22 insertions(+), 12 deletions(-)

diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index 32f465a80e4e..d53c95f4e1a9 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -12,10 +12,12 @@
 #ifndef _ASM_ACPI_H
 #define _ASM_ACPI_H
 
+#include <linux/efi.h>
 #include <linux/memblock.h>
 #include <linux/psci.h>
 
 #include <asm/cputype.h>
+#include <asm/io.h>
 #include <asm/smp_plat.h>
 #include <asm/tlbflush.h>
 
@@ -29,18 +31,22 @@
 
 /* Basic configuration for ACPI */
 #ifdef	CONFIG_ACPI
+pgprot_t __acpi_get_mem_attribute(phys_addr_t addr);
+
 /* ACPI table mapping after acpi_permanent_mmap is set */
 static inline void __iomem *acpi_os_ioremap(acpi_physical_address phys,
 					    acpi_size size)
 {
+	/* For normal memory we already have a cacheable mapping. */
+	if (memblock_is_map_memory(phys))
+		return (void __iomem *)__phys_to_virt(phys);
+
 	/*
-	 * EFI's reserve_regions() call adds memory with the WB attribute
-	 * to memblock via early_init_dt_add_memory_arch().
+	 * We should still honor the memory's attribute here because
+	 * crash dump kernel possibly excludes some ACPI (reclaim)
+	 * regions from memblock list.
 	 */
-	if (!memblock_is_memory(phys))
-		return ioremap(phys, size);
-
-	return ioremap_cache(phys, size);
+	return __ioremap(phys, size, __acpi_get_mem_attribute(phys));
 }
 #define acpi_os_ioremap acpi_os_ioremap
 
@@ -125,7 +131,10 @@ static inline const char *acpi_get_enable_method(int cpu)
  * for compatibility.
  */
 #define acpi_disable_cmcff 1
-pgprot_t arch_apei_get_mem_attribute(phys_addr_t addr);
+static inline pgprot_t arch_apei_get_mem_attribute(phys_addr_t addr)
+{
+	return __acpi_get_mem_attribute(addr);
+}
 #endif /* CONFIG_ACPI_APEI */
 
 #ifdef CONFIG_ACPI_NUMA
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index b3162715ed78..f94bdf7be439 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -31,10 +31,9 @@
 #include <asm/cpu_ops.h>
 #include <asm/smp_plat.h>
 
-#ifdef CONFIG_ACPI_APEI
+/* CONFIG_ACPI_APEI */
 # include <linux/efi.h>
 # include <asm/pgtable.h>
-#endif
 
 int acpi_noirq = 1;		/* skip ACPI IRQ initialization */
 int acpi_disabled = 1;
@@ -239,8 +238,7 @@ void __init acpi_boot_table_init(void)
 	}
 }
 
-#ifdef CONFIG_ACPI_APEI
-pgprot_t arch_apei_get_mem_attribute(phys_addr_t addr)
+pgprot_t __acpi_get_mem_attribute(phys_addr_t addr)
 {
 	/*
 	 * According to "Table 8 Map: EFI memory types to AArch64 memory
@@ -261,4 +259,3 @@ pgprot_t arch_apei_get_mem_attribute(phys_addr_t addr)
 		return __pgprot(PROT_NORMAL_NC);
 	return __pgprot(PROT_DEVICE_nGnRnE);
 }
-#endif
diff --git a/init/main.c b/init/main.c
index a8100b954839..a479ece2bae9 100644
--- a/init/main.c
+++ b/init/main.c
@@ -674,6 +674,10 @@ asmlinkage __visible void __init start_kernel(void)
 	debug_objects_mem_init();
 	setup_per_cpu_pageset();
 	numa_policy_init();
+#if defined(CONFIG_ARM64) && defined(CONFIG_EFI)
+	efi_memmap_init_late(efi.memmap.phys_map,
+			efi.memmap.nr_map * efi.memmap.desc_size);
+#endif
 	acpi_early_init();
 	if (late_time_init)
 		late_time_init();
-- 
2.15.1


