arm64: stable UEFI mappings for kexec

For UEFI, we need to install the memory mappings used for Runtime Services in a dedicated set of page tables. Add create_pgd_mapping(), which allows us to allocate and install those page table entries early.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm64/include/asm/mmu.h     |  3 +++
 arch/arm64/include/asm/pgtable.h |  5 +++++
 arch/arm64/mm/mmu.c              | 44 +++++++++++++++++++++-------------------
 3 files changed, 31 insertions(+), 21 deletions(-)

arch/arm64/include/asm/mmu.h +3 -0

index c2f006c48bdb..5fd40c43be80 100644
@@ -33,5 +33,8 @@ extern void __iomem *early_io_map(phys_addr_t phys, unsigned long virt);
 extern void init_mem_pgprot(void);
 /* create an identity mapping for memory (or io if map_io is true) */
 extern void create_id_mapping(phys_addr_t addr, phys_addr_t size, int map_io);
+extern void create_pgd_mapping(struct mm_struct *mm, phys_addr_t phys,
+			       unsigned long virt, phys_addr_t size,
+			       pgprot_t prot);
 
 #endif

arch/arm64/include/asm/pgtable.h +5 -0

index 41a43bf26492..1abe4d08725b 100644
@@ -264,6 +264,11 @@ static inline pmd_t pte_pmd(pte_t pte)
 	return __pmd(pte_val(pte));
 }
 
+static inline pgprot_t mk_sect_prot(pgprot_t prot)
+{
+	return __pgprot(pgprot_val(prot) & ~PTE_TABLE_BIT);
+}
+
 /*
  * THP definitions.
  */

arch/arm64/mm/mmu.c +1 -1

index 83e6713143a3..b6dc2ce3991a 100644
@@ -157,20 +157,10 @@ static void __init alloc_init_pte(pmd_t *pmd, unsigned long addr,
 
 static void __init alloc_init_pmd(struct mm_struct *mm, pud_t *pud,
 				  unsigned long addr, unsigned long end,
-				  phys_addr_t phys, int map_io)
+				  phys_addr_t phys, pgprot_t prot)
 {
 	pmd_t *pmd;
 	unsigned long next;

arch/arm64/mm/mmu.c +12 -9

 	/*
 	 * Check for initial section mappings in the pgd/pud and remove them.

@@ -186,7 +176,8 @@ static void __init alloc_init_pmd(struct mm_struct *mm, pud_t *pud,
 		/* try section mapping first */
 		if (((addr | next | phys) & ~SECTION_MASK) == 0) {
 			pmd_t old_pmd =*pmd;
-			set_pmd(pmd, __pmd(phys | prot_sect));
+			set_pmd(pmd, __pmd(phys |
+					   pgprot_val(mk_sect_prot(prot))));
 			/*
 			 * Check for previous table entries created during
 			 * boot (__create_page_tables) and flush them.

@@ -195,7 +186,7 @@ static void __init alloc_init_pmd(struct mm_struct *mm, pud_t *pud,
 				flush_tlb_all();
 		} else {
 			alloc_init_pte(pmd, addr, next, __phys_to_pfn(phys),
-				       prot_pte);
+				       prot);
 		}
 		phys += next - addr;
 	} while (pmd++, addr = next, addr != end);

@@ -203,7 +194,7 @@ static void __init alloc_init_pmd(struct mm_struct *mm, pud_t *pud,
 
 static void __init alloc_init_pud(struct mm_struct *mm, pgd_t *pgd,
 				  unsigned long addr, unsigned long end,
-				  unsigned long phys, int map_io)
+				  unsigned long phys, pgprot_t prot)
 {
 	pud_t *pud;
 	unsigned long next;

@@ -221,10 +212,11 @@ static void __init alloc_init_pud(struct mm_struct *mm, pgd_t *pgd,
 		/*
 		 * For 4K granule only, attempt to put down a 1GB block
 		 */
-		if (!map_io && (PAGE_SHIFT == 12) &&
+		if ((PAGE_SHIFT == 12) &&
 		    ((addr | next | phys) & ~PUD_MASK) == 0) {
 			pud_t old_pud = *pud;
-			set_pud(pud, __pud(phys | PROT_SECT_NORMAL_EXEC));
+			set_pud(pud, __pud(phys |
+					   pgprot_val(mk_sect_prot(prot))));
 
 			/*
 			 * If we have an old value for a pud, it will

@@ -239,7 +231,7 @@ static void __init alloc_init_pud(struct mm_struct *mm, pgd_t *pgd,
 				flush_tlb_all();
 			}
 		} else {
-			alloc_init_pmd(mm, pud, addr, next, phys, map_io);
+			alloc_init_pmd(mm, pud, addr, next, phys, prot);
 		}
 		phys += next - addr;
 	} while (pud++, addr = next, addr != end);

@@ -251,7 +243,7 @@ static void __init alloc_init_pud(struct mm_struct *mm, pgd_t *pgd,
  */
 static void __init __create_mapping(struct mm_struct *mm, pgd_t *pgd,
 				    phys_addr_t phys, unsigned long virt,
-				    phys_addr_t size, int map_io)
+				    phys_addr_t size, pgprot_t prot)
 {
 	unsigned long addr, length, end, next;
 

@@ -261,7 +253,8 @@ static void __init __create_mapping(struct mm_struct *mm, pgd_t *pgd,
 	end = addr + length;
 	do {
 		next = pgd_addr_end(addr, end);
-		alloc_init_pud(mm, pgd, addr, next, phys, map_io);
+		alloc_init_pud(mm, pgd, addr, next, phys, prot);
+			       
 		phys += next - addr;
 	} while (pgd++, addr = next, addr != end);
 }

@@ -275,7 +268,7 @@ static void __init create_mapping(phys_addr_t phys, unsigned long virt,
 		return;
 	}
 	__create_mapping(&init_mm, pgd_offset_k(virt & PAGE_MASK), phys, virt,
-			 size, 0);
+			 size, PAGE_KERNEL_EXEC);
 }
 
 void __init create_id_mapping(phys_addr_t addr, phys_addr_t size, int map_io)

arch/arm64/mm/mmu.c +6 -0

+
+void __init create_pgd_mapping(struct mm_struct *mm, phys_addr_t phys,
+			       unsigned long virt, phys_addr_t size,
+			       pgprot_t prot)
+{
+	__create_mapping(mm, pgd_offset(mm, virt), phys, virt, size, prot);
 }
 
 static void __init map_mem(void)

Signature · 1 line

1.8.3.2

On Thu, Nov 06, 2014 at 03:13:18PM +0100, Ard Biesheuvel wrote:

Hi Ard,
Some comments below:

Quoted 53 lines

For UEFI, we need to install the memory mappings used for Runtime Services in a dedicated set of page tables. Add create_pgd_mapping(), which allows us to allocate and install those page table entries early.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm64/include/asm/mmu.h     |  3 +++
 arch/arm64/include/asm/pgtable.h |  5 +++++
 arch/arm64/mm/mmu.c              | 44 +++++++++++++++++++++-------------------
 3 files changed, 31 insertions(+), 21 deletions(-)

arch/arm64/include/asm/mmu.h +3 -0

index c2f006c48bdb..5fd40c43be80 100644
@@ -33,5 +33,8 @@ extern void __iomem *early_io_map(phys_addr_t phys, unsigned long virt);
 extern void init_mem_pgprot(void);
 /* create an identity mapping for memory (or io if map_io is true) */
 extern void create_id_mapping(phys_addr_t addr, phys_addr_t size, int map_io);
+extern void create_pgd_mapping(struct mm_struct *mm, phys_addr_t phys,
+                            unsigned long virt, phys_addr_t size,
+                            pgprot_t prot);
 
 #endif

arch/arm64/include/asm/pgtable.h +5 -0

index 41a43bf26492..1abe4d08725b 100644
@@ -264,6 +264,11 @@ static inline pmd_t pte_pmd(pte_t pte)
      return __pmd(pte_val(pte));
 }
 
+static inline pgprot_t mk_sect_prot(pgprot_t prot)
+{
+     return __pgprot(pgprot_val(prot) & ~PTE_TABLE_BIT);
+}
+
 /*
  * THP definitions.
  */

arch/arm64/mm/mmu.c +1 -1

index 83e6713143a3..b6dc2ce3991a 100644
@@ -157,20 +157,10 @@ static void __init alloc_init_pte(pmd_t *pmd, unsigned long addr,
 
 static void __init alloc_init_pmd(struct mm_struct *mm, pud_t *pud,
                                unsigned long addr, unsigned long end,
-                               phys_addr_t phys, int map_io)
+                               phys_addr_t phys, pgprot_t prot)
 {
      pmd_t *pmd;
      unsigned long next;

arch/arm64/mm/mmu.c static void __init alloc_init_pte(pmd_t *pmd, unsigned long addr, · line 173

Quoted 10 lines

arch/arm64/mm/mmu.c +0 -10

-     pmdval_t prot_sect;
-     pgprot_t prot_pte;
-
-     if (map_io) {
-             prot_sect = PROT_SECT_DEVICE_nGnRE;
-             prot_pte = __pgprot(PROT_DEVICE_nGnRE);
-     } else {
-             prot_sect = PROT_SECT_NORMAL_EXEC;
-             prot_pte = PAGE_KERNEL_EXEC;
-     }

Thanks :-)

Quoted 83 lines

arch/arm64/mm/mmu.c +12 -9

      /*
       * Check for initial section mappings in the pgd/pud and remove them.

@@ -186,7 +176,8 @@ static void __init alloc_init_pmd(struct mm_struct *mm, pud_t *pud,
              /* try section mapping first */
              if (((addr | next | phys) & ~SECTION_MASK) == 0) {
                      pmd_t old_pmd =*pmd;
-                     set_pmd(pmd, __pmd(phys | prot_sect));
+                     set_pmd(pmd, __pmd(phys |
+                                        pgprot_val(mk_sect_prot(prot))));
                      /*
                       * Check for previous table entries created during
                       * boot (__create_page_tables) and flush them.

@@ -195,7 +186,7 @@ static void __init alloc_init_pmd(struct mm_struct *mm, pud_t *pud,
                              flush_tlb_all();
              } else {
                      alloc_init_pte(pmd, addr, next, __phys_to_pfn(phys),
-                                    prot_pte);
+                                    prot);
              }
              phys += next - addr;
      } while (pmd++, addr = next, addr != end);

@@ -203,7 +194,7 @@ static void __init alloc_init_pmd(struct mm_struct *mm, pud_t *pud,
 
 static void __init alloc_init_pud(struct mm_struct *mm, pgd_t *pgd,
                                unsigned long addr, unsigned long end,
-                               unsigned long phys, int map_io)
+                               unsigned long phys, pgprot_t prot)
 {
      pud_t *pud;
      unsigned long next;

@@ -221,10 +212,11 @@ static void __init alloc_init_pud(struct mm_struct *mm, pgd_t *pgd,
              /*
               * For 4K granule only, attempt to put down a 1GB block
               */
-             if (!map_io && (PAGE_SHIFT == 12) &&
+             if ((PAGE_SHIFT == 12) &&
                  ((addr | next | phys) & ~PUD_MASK) == 0) {
                      pud_t old_pud = *pud;
-                     set_pud(pud, __pud(phys | PROT_SECT_NORMAL_EXEC));
+                     set_pud(pud, __pud(phys |
+                                        pgprot_val(mk_sect_prot(prot))));
 
                      /*
                       * If we have an old value for a pud, it will

@@ -239,7 +231,7 @@ static void __init alloc_init_pud(struct mm_struct *mm, pgd_t *pgd,
                              flush_tlb_all();
                      }
              } else {
-                     alloc_init_pmd(mm, pud, addr, next, phys, map_io);
+                     alloc_init_pmd(mm, pud, addr, next, phys, prot);
              }
              phys += next - addr;
      } while (pud++, addr = next, addr != end);

@@ -251,7 +243,7 @@ static void __init alloc_init_pud(struct mm_struct *mm, pgd_t *pgd,
  */
 static void __init __create_mapping(struct mm_struct *mm, pgd_t *pgd,
                                  phys_addr_t phys, unsigned long virt,
-                                 phys_addr_t size, int map_io)
+                                 phys_addr_t size, pgprot_t prot)
 {
      unsigned long addr, length, end, next;
 

@@ -261,7 +253,8 @@ static void __init __create_mapping(struct mm_struct *mm, pgd_t *pgd,
      end = addr + length;
      do {
              next = pgd_addr_end(addr, end);
-             alloc_init_pud(mm, pgd, addr, next, phys, map_io);
+             alloc_init_pud(mm, pgd, addr, next, phys, prot);
+
              phys += next - addr;
      } while (pgd++, addr = next, addr != end);
 }

@@ -275,7 +268,7 @@ static void __init create_mapping(phys_addr_t phys, unsigned long virt,
              return;
      }
      __create_mapping(&init_mm, pgd_offset_k(virt & PAGE_MASK), phys, virt,
-                      size, 0);
+                      size, PAGE_KERNEL_EXEC);
 }
 
 void __init create_id_mapping(phys_addr_t addr, phys_addr_t size, int map_io)

arch/arm64/mm/mmu.c void __init create_id_mapping(phys_addr_t addr, phys_addr_t size, int map_io) · line 284

Quoted 8 lines

arch/arm64/mm/mmu.c +4 -1

@@ -285,7 +278,16 @@ void __init create_id_mapping(phys_addr_t addr, phys_addr_t size, int map_io)
              return;
      }
      __create_mapping(&init_mm, &idmap_pg_dir[pgd_index(addr)],
-                      addr, addr, size, map_io);
+                      addr, addr, size,
+                      map_io ? __pgprot(PROT_DEVICE_nGnRE)
+                             : PAGE_KERNEL_EXEC);
+}

Could you please also change efi_setup_idmap (it's the only caller I can see for create_id_mapping)?

That way the prototype for create_id_mapping would look like:
void __init create_id_mapping(phys_addr_t addr, phys_addr_t size, pgprot_t prot)

Quoted 10 lines

arch/arm64/mm/mmu.c +6 -1

+
+void __init create_pgd_mapping(struct mm_struct *mm, phys_addr_t phys,
+                            unsigned long virt, phys_addr_t size,
+                            pgprot_t prot)
+{
+     __create_mapping(mm, pgd_offset(mm, virt), phys, virt, size, prot);
 }
 
 static void __init map_mem(void)
--

Quoted 2 lines

1.8.3.2

Cheers,
--
Steve

Quoted 1 line · Steve Capper

On Thu, Nov 06, 2014 at 03:13:18PM +0100, Ard Biesheuvel wrote:

Quoted 14 lines

arch/arm64/mm/mmu.c void __init create_id_mapping(phys_addr_t addr, phys_addr_t size, int map_io) · line 284

Quoted 10 lines

void __init create_id_mapping(phys_addr_t addr, phys_addr_t size, int map_io)

arch/arm64/mm/mmu.c +4 -1

@@ -285,7 +278,16 @@ void __init create_id_mapping(phys_addr_t addr, phys_addr_t size, int map_io)
              return;
      }
      __create_mapping(&init_mm, &idmap_pg_dir[pgd_index(addr)],
-                      addr, addr, size, map_io);
+                      addr, addr, size,
+                      map_io ? __pgprot(PROT_DEVICE_nGnRE)
+                             : PAGE_KERNEL_EXEC);
+}

Could you please also change efi_setup_idmap (it's the only caller I can see for create_id_mapping)?

Quoted 3 lines

That way the prototype for create_id_mapping would look like:
void __init create_id_mapping(phys_addr_t addr, phys_addr_t size, pgprot_t prot)

I didn't bother because a couple of patches later, this stuff is all ripped out anyway
(7/10 arm64/efi: remove idmap manipulations from UEFI code), because there is no longer a need for UEFI to switch to the ID map.

Do you feel I should still change it here, and then remove it later?

This changes the way memblocks are installed based on the contents of the UEFI memory map. Formerly, all regions would be memblock_add()'ed, after which unusable regions would be memblock_reserve()'d as well. To simplify things, but also to allow access to the unusable regions through mmap(/dev/mem), even with CONFIG_STRICT_DEVMEM set, change this so that only usable regions are memblock_add()'ed in the first place.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm64/kernel/efi.c | 69 +++++++++++++++++++------------------------------
 1 file changed, 26 insertions(+), 43 deletions(-)

arch/arm64/kernel/efi.c +26 -43

index 3009c22e2620..af2214c692d3 100644
@@ -40,13 +40,6 @@ static int __init uefi_debug_setup(char *str)
 }
 early_param("uefi_debug", uefi_debug_setup);
 
-static int __init is_normal_ram(efi_memory_desc_t *md)
-{
-	if (md->attribute & EFI_MEMORY_WB)
-		return 1;
-	return 0;
-}
-
 static int __init uefi_init(void)
 {
 	efi_char16_t *c16;

@@ -105,28 +98,11 @@ out:
 	return retval;
 }
 
-/*
- * Return true for RAM regions we want to permanently reserve.
- */
-static __init int is_reserve_region(efi_memory_desc_t *md)
-{
-	switch (md->type) {
-	case EFI_LOADER_CODE:
-	case EFI_LOADER_DATA:
-	case EFI_BOOT_SERVICES_CODE:
-	case EFI_BOOT_SERVICES_DATA:
-	case EFI_CONVENTIONAL_MEMORY:
-		return 0;
-	default:
-		break;
-	}
-	return is_normal_ram(md);
-}
-
-static __init void reserve_regions(void)
+static __init void process_memory_map(void)
 {
 	efi_memory_desc_t *md;
 	u64 paddr, npages, size;
+	u32 lost = 0;
 
 	if (uefi_debug)
 		pr_info("Processing EFI memory map:\n");

@@ -134,31 +110,38 @@ static __init void reserve_regions(void)
 	for_each_efi_memory_desc(&memmap, md) {
 		paddr = md->phys_addr;
 		npages = md->num_pages;
+		size = npages << EFI_PAGE_SHIFT;
 
 		if (uefi_debug) {
 			char buf[64];
 
-			pr_info("  0x%012llx-0x%012llx %s",
-				paddr, paddr + (npages << EFI_PAGE_SHIFT) - 1,
+			pr_info("  0x%012llx-0x%012llx %s\n",
+				paddr, paddr + size - 1,
 				efi_md_typeattr_format(buf, sizeof(buf), md));
 		}
 
-		memrange_efi_to_native(&paddr, &npages);
-		size = npages << PAGE_SHIFT;
-
-		if (is_normal_ram(md))
-			early_init_dt_add_memory_arch(paddr, size);
-
-		if (is_reserve_region(md)) {
-			memblock_reserve(paddr, size);
-			if (uefi_debug)
-				pr_cont("*");
-		}
-
-		if (uefi_debug)
-			pr_cont("\n");
+		if (!efi_mem_is_usable_region(md))
+			continue;
+
+		early_init_dt_add_memory_arch(paddr, size);
+
+		/*
+		 * Keep a tally of how much memory we are losing due to
+		 * rounding of regions that are not aligned to the page
+		 * size. We cannot easily recover this memory without
+		 * sorting the memory map and attempting to merge adjacent
+		 * usable regions.
+		 */
+		if (PAGE_SHIFT != EFI_PAGE_SHIFT)
+			lost += (npages << EFI_PAGE_SHIFT) -
+				round_down(max_t(s64, size - PAGE_ALIGN(paddr) +
+						 md->phys_addr, 0),
+					   PAGE_SIZE);
 	}
 
+	if (lost > SZ_1K)
+		pr_warn("efi: lost %u KB of RAM to rounding\n", lost / SZ_1K);
+
 	set_bit(EFI_MEMMAP, &efi.flags);
 }
 

@@ -182,7 +165,7 @@ void __init efi_init(void)
 
 	WARN_ON(uefi_init() < 0);
 
-	reserve_regions();
+	process_memory_map();
 }
 
 static int __init arm64_enter_virtual_mode(void)

Quoted 7 lines · Ard Biesheuvel

This changes the way memblocks are installed based on the contents of the UEFI memory map. Formerly, all regions would be memblock_add()'ed, after which unusable regions would be memblock_reserve()'d as well. To simplify things, but also to allow access to the unusable regions through mmap(/dev/mem), even with CONFIG_STRICT_DEVMEM set, change this so that only usable regions are memblock_add()'ed in the first place.

This patch is crashing 64K pagesize kernels during boot. I'm not exactly sure why, though. Here is what I get on an APM Mustang box:

[    0.046262] Unhandled fault: alignment fault (0x96000021) at 0xfffffc000004f026
[    0.053863] Internal error: : 96000021 [#1] SMP
[    0.058566] Modules linked in:
[    0.061736] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.18.0-rc3+ #11
[    0.068418] Hardware name: APM X-Gene Mustang board (DT)
[    0.073942] task: fffffe0000f46a20 ti: fffffe0000f10000 task.ti: fffffe0000f10000
[    0.081735] PC is at acpi_ns_lookup+0x520/0x734
[    0.086448] LR is at acpi_ns_lookup+0x4ac/0x734
[    0.091151] pc : [<fffffe0000460458>] lr : [<fffffe00004603e4>] pstate: 60000245
[    0.098843] sp : fffffe0000f13b00
[    0.102293] x29: fffffe0000f13b10 x28: 000000000000000a
[    0.107810] x27: 0000000000000008 x26: 0000000000000000
[    0.113345] x25: fffffe0000f13c38 x24: fffffe0000fae000
[    0.118862] x23: 0000000000000008 x22: 0000000000000001
[    0.124406] x21: fffffe0000972000 x20: 000000000000000a
[    0.129940] x19: fffffc000004f026 x18: 000000000000000b
[    0.135483] x17: 0000000000000023 x16: 000000000000059f
[    0.141026] x15: 0000000000007fff x14: 000000000000038e
[    0.146543] x13: ff00000000000000 x12: ffffffffffffffff
[    0.152060] x11: 0000000000000010 x10: 00000000fffffff6
[    0.157585] x9 : 0000000000000050 x8 : fffffe03fa7401b0
[    0.163111] x7 : fffffe0001115a00 x6 : fffffe0001115a00
[    0.168620] x5 : fffffe0001115000 x4 : 0000000000000010
[    0.174146] x3 : 0000000000000010 x2 : 000000000000000b
[    0.179689] x1 : 00000000fffffff5 x0 : 0000000000000000
[    0.185215]
[    0.186765] Process swapper/0 (pid: 0, stack limit = 0xfffffe0000f10058)
[    0.193724] Stack: (0xfffffe0000f13b00 to 0xfffffe0000f14000)
[    0.199707] 3b00: 00000000 fffffe03 00000666 00000000 00f13bd0 fffffe00 0044f388 fffffe00
...
[    0.539605] Call trace:
[    0.542150] [<fffffe0000460458>] acpi_ns_lookup+0x520/0x734
[    0.547935] [<fffffe000044f384>] acpi_ds_load1_begin_op+0x384/0x4b4
[    0.554445] [<fffffe0000469594>] acpi_ps_build_named_op+0xfc/0x228
[    0.560868] [<fffffe00004698c8>] acpi_ps_create_op+0x208/0x340
[    0.566945] [<fffffe0000468ea8>] acpi_ps_parse_loop+0x208/0x7f8
[    0.573092] [<fffffe000046a6b8>] acpi_ps_parse_aml+0x1c0/0x434
[    0.579153] [<fffffe0000463cb0>] acpi_ns_one_complete_parse+0x1a4/0x1ec
[    0.586017] [<fffffe0000463d88>] acpi_ns_parse_table+0x90/0x130
[    0.592181] [<fffffe0000462f94>] acpi_ns_load_table+0xc8/0x1b0
[    0.598243] [<fffffe0000e71848>] acpi_load_tables+0xf4/0x234
[    0.604122] [<fffffe0000e70a0c>] acpi_early_init+0x78/0xc8
[    0.609828] [<fffffe0000e40928>] start_kernel+0x334/0x3ac
[    0.615431] Code: 2a00037b b9009fbc 36180057 321d037b (b9400260)
[    0.621777] ---[ end trace cb88537fdc8fa200 ]---
[    0.626586] Kernel panic - not syncing: Attempted to kill the idle task!

I also get a different crash when booting with device tree instead of ACPI.

One thing is that early_init_dt_add_memory_arch() is called with UEFI (4K) pagesize alignment and size and it clips the passed in range such that what is added is the 64K aligned range which fits completely within the given range. This may mean nothing gets added if the given range is smaller than 64K. And only a subset of the desired range is added in other cases. This could theoretically leave bits of devicetree and/or initrd unmapped, but that doesn't seem to be the case here. I'll keep poking at it...

Quoted 118 lines

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm64/kernel/efi.c | 69 +++++++++++++++++++------------------------------
 1 file changed, 26 insertions(+), 43 deletions(-)

arch/arm64/kernel/efi.c +26 -43

index 3009c22e2620..af2214c692d3 100644
@@ -40,13 +40,6 @@ static int __init uefi_debug_setup(char *str)
 }
 early_param("uefi_debug", uefi_debug_setup);
 
-static int __init is_normal_ram(efi_memory_desc_t *md)
-{
-     if (md->attribute & EFI_MEMORY_WB)
-             return 1;
-     return 0;
-}
-
 static int __init uefi_init(void)
 {
      efi_char16_t *c16;

@@ -105,28 +98,11 @@ out:
      return retval;
 }
 
-/*
- * Return true for RAM regions we want to permanently reserve.
- */
-static __init int is_reserve_region(efi_memory_desc_t *md)
-{
-     switch (md->type) {
-     case EFI_LOADER_CODE:
-     case EFI_LOADER_DATA:
-     case EFI_BOOT_SERVICES_CODE:
-     case EFI_BOOT_SERVICES_DATA:
-     case EFI_CONVENTIONAL_MEMORY:
-             return 0;
-     default:
-             break;
-     }
-     return is_normal_ram(md);
-}
-
-static __init void reserve_regions(void)
+static __init void process_memory_map(void)
 {
      efi_memory_desc_t *md;
      u64 paddr, npages, size;
+     u32 lost = 0;
 
      if (uefi_debug)
              pr_info("Processing EFI memory map:\n");

@@ -134,31 +110,38 @@ static __init void reserve_regions(void)
      for_each_efi_memory_desc(&memmap, md) {
              paddr = md->phys_addr;
              npages = md->num_pages;
+             size = npages << EFI_PAGE_SHIFT;
 
              if (uefi_debug) {
                      char buf[64];
 
-                     pr_info("  0x%012llx-0x%012llx %s",
-                             paddr, paddr + (npages << EFI_PAGE_SHIFT) - 1,
+                     pr_info("  0x%012llx-0x%012llx %s\n",
+                             paddr, paddr + size - 1,
                              efi_md_typeattr_format(buf, sizeof(buf), md));
              }
 
-             memrange_efi_to_native(&paddr, &npages);
-             size = npages << PAGE_SHIFT;
-
-             if (is_normal_ram(md))
-                     early_init_dt_add_memory_arch(paddr, size);
-
-             if (is_reserve_region(md)) {
-                     memblock_reserve(paddr, size);
-                     if (uefi_debug)
-                             pr_cont("*");
-             }
-
-             if (uefi_debug)
-                     pr_cont("\n");
+             if (!efi_mem_is_usable_region(md))
+                     continue;
+
+             early_init_dt_add_memory_arch(paddr, size);
+
+             /*
+              * Keep a tally of how much memory we are losing due to
+              * rounding of regions that are not aligned to the page
+              * size. We cannot easily recover this memory without
+              * sorting the memory map and attempting to merge adjacent
+              * usable regions.
+              */
+             if (PAGE_SHIFT != EFI_PAGE_SHIFT)
+                     lost += (npages << EFI_PAGE_SHIFT) -
+                             round_down(max_t(s64, size - PAGE_ALIGN(paddr) +
+                                              md->phys_addr, 0),
+                                        PAGE_SIZE);
      }
 
+     if (lost > SZ_1K)
+             pr_warn("efi: lost %u KB of RAM to rounding\n", lost / SZ_1K);
+
      set_bit(EFI_MEMMAP, &efi.flags);
 }
 

@@ -182,7 +165,7 @@ void __init efi_init(void)
 
      WARN_ON(uefi_init() < 0);
 
-     reserve_regions();
+     process_memory_map();
 }
 
 static int __init arm64_enter_virtual_mode(void)

Quoted 2 lines · Mark Salter

Quoted 1 line · Ard Biesheuvel

This changes the way memblocks are installed based on the contents

of

Quoted 1 line

Quoted 1 line

the UEFI memory map. Formerly, all regions would be

memblock_add()'ed,

Quoted 2 lines

Quoted 2 lines

after which unusable regions would be memblock_reserve()'d as well. To simplify things, but also to allow access to the unusable

regions

Quoted 5 lines

Quoted 3 lines

through mmap(/dev/mem), even with CONFIG_STRICT_DEVMEM set, change this so that only usable regions are memblock_add()'ed in the first place.

This patch is crashing 64K pagesize kernels during boot. I'm not

exactly

Quoted 2 lines

sure why, though. Here is what I get on an APM Mustang box:

Ah, yes, I meant to mention this patch

https://git.kernel.org/cgit/linux/kernel/git/glikely/linux.git/commit/?id=8cccffc52694938fc88f3d90bc7fed8460e27191

in the cover letter, which addresses this issue at least for the DT case.

Quoted 18 lines · Ard Biesheuvel

Quoted 2 lines · Mark Salter

Quoted 1 line · Ard Biesheuvel

This changes the way memblocks are installed based on the contents

of

Quoted 1 line

Quoted 1 line

the UEFI memory map. Formerly, all regions would be

memblock_add()'ed,

Quoted 2 lines

Quoted 2 lines

after which unusable regions would be memblock_reserve()'d as well. To simplify things, but also to allow access to the unusable

regions

Quoted 5 lines

Quoted 3 lines

through mmap(/dev/mem), even with CONFIG_STRICT_DEVMEM set, change this so that only usable regions are memblock_add()'ed in the first place.

This patch is crashing 64K pagesize kernels during boot. I'm not

exactly

Quoted 2 lines

sure why, though. Here is what I get on an APM Mustang box:

Quoted 7 lines

Ah, yes, I meant to mention this patch

https://git.kernel.org/cgit/linux/kernel/git/glikely/linux.git/commit/?id=8cccffc52694938fc88f3d90bc7fed8460e27191

in the cover letter, which addresses this issue at least for the DT case.

That isn't the problem. In general, with 64K kernel pages, you can't be sure if you leave something you need out of the kernel linear mapping. If you have Loader Code/Data regions begin and/or end at something other than a 64K boundary and that region is adjacent to a region not being added, then you end up leaving out the unaligned bits from the linear mapping. This could be bits of the initramfs or devicetree.

What I don't get with this failure is that it is an alignment fault which should be masked at EL1 for the kernel. The same unaligned access happens without this patch and it doesn't generate a fault.

Quoted 21 lines · Mark Salter

Quoted 20 lines · Ard Biesheuvel

Quoted 2 lines · Mark Salter

Quoted 1 line · Ard Biesheuvel

This changes the way memblocks are installed based on the contents

of

Quoted 1 line

Quoted 1 line

the UEFI memory map. Formerly, all regions would be

memblock_add()'ed,

Quoted 2 lines

Quoted 2 lines

after which unusable regions would be memblock_reserve()'d as well. To simplify things, but also to allow access to the unusable

regions

Quoted 5 lines

Quoted 3 lines

through mmap(/dev/mem), even with CONFIG_STRICT_DEVMEM set, change this so that only usable regions are memblock_add()'ed in the first place.

This patch is crashing 64K pagesize kernels during boot. I'm not

exactly

Quoted 2 lines

sure why, though. Here is what I get on an APM Mustang box:

Ah, yes, I meant to mention this patch

Quoted 8 lines

Quoted 5 lines

https://git.kernel.org/cgit/linux/kernel/git/glikely/linux.git/commit/?id=8cccffc52694938fc88f3d90bc7fed8460e27191

in the cover letter, which addresses this issue at least for the DT case.

That isn't the problem. In general, with 64K kernel pages, you can't be sure if you leave something you need out of the kernel linear mapping.

Quoted 9 lines

If you have Loader Code/Data regions begin and/or end at something other than a 64K boundary and that region is adjacent to a region not being added, then you end up leaving out the unaligned bits from the linear mapping. This could be bits of the initramfs or devicetree.

What I don't get with this failure is that it is an alignment fault which should be masked at EL1 for the kernel. The same unaligned access happens without this patch and it doesn't generate a fault.

Ah, but unaligned accesses are not ignored for device memory. I have this in include/acpi/acpi_io.h:

static inline void __iomem *acpi_os_ioremap(acpi_physical_address phys,
acpi_size size)
{

#ifdef CONFIG_ARM64
	if (!page_is_ram(phys >> PAGE_SHIFT))
		return ioremap(phys, size);

#endif

       return ioremap_cache(phys, size);
}

Because the table isn't in the linear mapping, it fails the page_is_ram() test and it gits mapped with ioremap() leading to the alignment fault.

If I take out the code inside the #ifdef, I get a different fault:

[    0.350057] Unhandled fault: synchronous external abort (0x96000010) at 0xfffffe0000fae6f4
[    0.358704] pgd = fffffe0001160000
[    0.362276] [fffffe0000fae6f4] *pgd=0000004001370003, *pud=0000004001370003, *pmd=0000004001370003, *pte=02c00040011a0713
[    0.373746] Internal error: : 96000010 [#1] SMP
[    0.378484] Modules linked in:
[    0.381601] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 3.18.0-rc4+ #15
[    0.388248] Hardware name: APM X-Gene Mustang board (DT)
[    0.393738] task: fffffe03dbe10000 ti: fffffe03dbf00000 task.ti: fffffe03dbf00000
[    0.401503] PC is at acpi_ex_system_memory_space_handler+0x238/0x2e0
[    0.408160] LR is at acpi_ex_system_memory_space_handler+0x130/0x2e0

That happens because AML is trying to access a hardware register which has been mapped as normal memory.

So, we need a way to tell a table in ram from an io address in AML. And page_is_ram() no longer cuts it if the tables are not in the linear mapping.

Quoted 22 lines · Mark Salter

Quoted 21 lines · Mark Salter

Quoted 20 lines · Ard Biesheuvel

Quoted 2 lines · Mark Salter

Quoted 1 line · Ard Biesheuvel

This changes the way memblocks are installed based on the contents

of

Quoted 1 line

Quoted 1 line

the UEFI memory map. Formerly, all regions would be

memblock_add()'ed,

Quoted 2 lines

Quoted 2 lines

after which unusable regions would be memblock_reserve()'d as well. To simplify things, but also to allow access to the unusable

regions

Quoted 5 lines

Quoted 3 lines

through mmap(/dev/mem), even with CONFIG_STRICT_DEVMEM set, change this so that only usable regions are memblock_add()'ed in the first place.

This patch is crashing 64K pagesize kernels during boot. I'm not

exactly

Quoted 2 lines

sure why, though. Here is what I get on an APM Mustang box:

Ah, yes, I meant to mention this patch

Quoted 8 lines

Quoted 8 lines

Quoted 5 lines

https://git.kernel.org/cgit/linux/kernel/git/glikely/linux.git/commit/?id=8cccffc52694938fc88f3d90bc7fed8460e27191

in the cover letter, which addresses this issue at least for the DT case.

That isn't the problem. In general, with 64K kernel pages, you can't be sure if you leave something you need out of the kernel linear mapping.

Regardless of 64k pages you can never assume that anything will be in the linear mapping due to the current relationship between the start of the linear map and the address the kernel was loaded at.

Quoted 42 lines

Quoted 9 lines

If you have Loader Code/Data regions begin and/or end at something other than a 64K boundary and that region is adjacent to a region not being added, then you end up leaving out the unaligned bits from the linear mapping. This could be bits of the initramfs or devicetree.

What I don't get with this failure is that it is an alignment fault which should be masked at EL1 for the kernel. The same unaligned access happens without this patch and it doesn't generate a fault.

Ah, but unaligned accesses are not ignored for device memory. I have this in include/acpi/acpi_io.h:

static inline void __iomem *acpi_os_ioremap(acpi_physical_address phys,
acpi_size size)
{

#ifdef CONFIG_ARM64
      if (!page_is_ram(phys >> PAGE_SHIFT))
              return ioremap(phys, size);

#endif

       return ioremap_cache(phys, size);
}

Because the table isn't in the linear mapping, it fails the page_is_ram() test and it gits mapped with ioremap() leading to the alignment fault.

If I take out the code inside the #ifdef, I get a different fault:

[ 0.350057] Unhandled fault: synchronous external abort (0x96000010) at 0xfffffe0000fae6f4
[    0.358704] pgd = fffffe0001160000
[    0.362276] [fffffe0000fae6f4] *pgd=0000004001370003, *pud=0000004001370003, *pmd=0000004001370003, *pte=02c00040011a0713
[    0.373746] Internal error: : 96000010 [#1] SMP
[    0.378484] Modules linked in:
[    0.381601] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 3.18.0-rc4+ #15
[    0.388248] Hardware name: APM X-Gene Mustang board (DT)
[    0.393738] task: fffffe03dbe10000 ti: fffffe03dbf00000 task.ti: fffffe03dbf00000
[    0.401503] PC is at acpi_ex_system_memory_space_handler+0x238/0x2e0
[    0.408160] LR is at acpi_ex_system_memory_space_handler+0x130/0x2e0

Quoted 2 lines

That happens because AML is trying to access a hardware register which has been mapped as normal memory.

Which is why removing the check in the ifdef is completely nonsensical.

We already knew we can't map everything cacheable -- regardless of what AML does the CPU can prefetch from anything mapped cacheable (or simply executable) at any time.

Quoted 3 lines

So, we need a way to tell a table in ram from an io address in AML. And page_is_ram() no longer cuts it if the tables are not in the linear mapping.

If the kernel were loaded at an address above the tables, it would fail similarly. So the page_is_ram was never sufficient to ensure tables would be mapped cacheable.

As we haven't decoupled the kernel text mapping from the linear mapping, and that doesn't look to be happening any time soon, we can't fix up page_is_ram -- we need something else entirely.

Quoted 78 lines · Mark Rutland

Quoted 22 lines · Mark Salter

Quoted 21 lines · Mark Salter

Quoted 20 lines · Ard Biesheuvel

Quoted 2 lines · Mark Salter

Quoted 1 line · Ard Biesheuvel

This changes the way memblocks are installed based on the contents

of

Quoted 1 line

Quoted 1 line

the UEFI memory map. Formerly, all regions would be

memblock_add()'ed,

Quoted 2 lines

Quoted 2 lines

after which unusable regions would be memblock_reserve()'d as well. To simplify things, but also to allow access to the unusable

regions

Quoted 5 lines

Quoted 3 lines

through mmap(/dev/mem), even with CONFIG_STRICT_DEVMEM set, change this so that only usable regions are memblock_add()'ed in the first place.

This patch is crashing 64K pagesize kernels during boot. I'm not

exactly

Quoted 2 lines

sure why, though. Here is what I get on an APM Mustang box:

Ah, yes, I meant to mention this patch

Quoted 8 lines

Quoted 8 lines

Quoted 5 lines

https://git.kernel.org/cgit/linux/kernel/git/glikely/linux.git/commit/?id=8cccffc52694938fc88f3d90bc7fed8460e27191

in the cover letter, which addresses this issue at least for the DT case.

That isn't the problem. In general, with 64K kernel pages, you can't be sure if you leave something you need out of the kernel linear mapping.

Regardless of 64k pages you can never assume that anything will be in the linear mapping due to the current relationship between the start of the linear map and the address the kernel was loaded at.

Quoted 42 lines

Quoted 9 lines

If you have Loader Code/Data regions begin and/or end at something other than a 64K boundary and that region is adjacent to a region not being added, then you end up leaving out the unaligned bits from the linear mapping. This could be bits of the initramfs or devicetree.

What I don't get with this failure is that it is an alignment fault which should be masked at EL1 for the kernel. The same unaligned access happens without this patch and it doesn't generate a fault.

Ah, but unaligned accesses are not ignored for device memory. I have this in include/acpi/acpi_io.h:

static inline void __iomem *acpi_os_ioremap(acpi_physical_address phys,
acpi_size size)
{

#ifdef CONFIG_ARM64
      if (!page_is_ram(phys >> PAGE_SHIFT))
              return ioremap(phys, size);

#endif

       return ioremap_cache(phys, size);
}

Because the table isn't in the linear mapping, it fails the page_is_ram() test and it gits mapped with ioremap() leading to the alignment fault.

If I take out the code inside the #ifdef, I get a different fault:

[ 0.350057] Unhandled fault: synchronous external abort (0x96000010) at 0xfffffe0000fae6f4
[    0.358704] pgd = fffffe0001160000
[    0.362276] [fffffe0000fae6f4] *pgd=0000004001370003, *pud=0000004001370003, *pmd=0000004001370003, *pte=02c00040011a0713
[    0.373746] Internal error: : 96000010 [#1] SMP
[    0.378484] Modules linked in:
[    0.381601] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 3.18.0-rc4+ #15
[    0.388248] Hardware name: APM X-Gene Mustang board (DT)
[    0.393738] task: fffffe03dbe10000 ti: fffffe03dbf00000 task.ti: fffffe03dbf00000
[    0.401503] PC is at acpi_ex_system_memory_space_handler+0x238/0x2e0
[    0.408160] LR is at acpi_ex_system_memory_space_handler+0x130/0x2e0

Quoted 4 lines

Quoted 2 lines

That happens because AML is trying to access a hardware register which has been mapped as normal memory.

Which is why removing the check in the ifdef is completely nonsensical.

I am sure we are all in agreement on this part.

Quoted 8 lines

We already knew we can't map everything cacheable -- regardless of what AML does the CPU can prefetch from anything mapped cacheable (or simply executable) at any time.

Quoted 3 lines

So, we need a way to tell a table in ram from an io address in AML. And page_is_ram() no longer cuts it if the tables are not in the linear mapping.

Quoted 8 lines

If the kernel were loaded at an address above the tables, it would fail similarly. So the page_is_ram was never sufficient to ensure tables would be mapped cacheable.

As we haven't decoupled the kernel text mapping from the linear mapping, and that doesn't look to be happening any time soon, we can't fix up page_is_ram -- we need something else entirely.

Well, if you look at the series, and in particular at the /dev/mem handling, there is already some code that classifies physical addresses based on whether they appear in the UEFI memory map, and with which attributes. I suppose reimplementing page_is_ram() [whose default implementation is conveniently __weak] to return true for EFI_MEMORY_WB ranges and false for everything else would do the trick here, and would arguably be more elegant than matching the string "System RAM" in the resource table. And, putting the UEFI memory map central in a range of RAM remapping related functions ensures that they are always in agreement (or so the theory goes)

Quoted 28 lines · Mark Salter

Quoted 27 lines · Mark Salter

Quoted 25 lines · Ard Biesheuvel

Quoted 2 lines · Mark Salter

Quoted 1 line · Ard Biesheuvel

This changes the way memblocks are installed based on the contents

of

Quoted 1 line

Quoted 1 line

the UEFI memory map. Formerly, all regions would be

memblock_add()'ed,

Quoted 2 lines

Quoted 2 lines

after which unusable regions would be memblock_reserve()'d as well. To simplify things, but also to allow access to the unusable

regions

Quoted 5 lines

Quoted 3 lines

through mmap(/dev/mem), even with CONFIG_STRICT_DEVMEM set, change this so that only usable regions are memblock_add()'ed in the first place.

This patch is crashing 64K pagesize kernels during boot. I'm not

exactly

Quoted 2 lines

sure why, though. Here is what I get on an APM Mustang box:

Ah, yes, I meant to mention this patch

https://git.kernel.org/cgit/linux/kernel/git/glikely/linux.git/commit/?id=8cccffc52694938fc88f3d90bc7fed8460e27191

in the cover letter, which addresses this issue at least for the DT case.

Quoted 2 lines

Quoted 2 lines

That isn't the problem. In general, with 64K kernel pages, you can't be sure if you leave something you need out of the kernel linear mapping.

Regardless of 64k pages you can never assume that anything will be in the linear mapping due to the current relationship between the start of the linear map and the address the kernel was loaded at.

Quoted 43 lines

Quoted 10 lines

If you have Loader Code/Data regions begin and/or end at something other than a 64K boundary and that region is adjacent to a region not being added, then you end up leaving out the unaligned bits from the linear mapping. This could be bits of the initramfs or devicetree.

What I don't get with this failure is that it is an alignment fault which should be masked at EL1 for the kernel. The same unaligned access happens without this patch and it doesn't generate a fault.

Ah, but unaligned accesses are not ignored for device memory. I have this in include/acpi/acpi_io.h:

static inline void __iomem *acpi_os_ioremap(acpi_physical_address phys,
acpi_size size)
{

#ifdef CONFIG_ARM64
	if (!page_is_ram(phys >> PAGE_SHIFT))
		return ioremap(phys, size);

#endif

       return ioremap_cache(phys, size);
}

Because the table isn't in the linear mapping, it fails the page_is_ram() test and it gits mapped with ioremap() leading to the alignment fault.

If I take out the code inside the #ifdef, I get a different fault:

[    0.350057] Unhandled fault: synchronous external abort (0x96000010) at 0xfffffe0000fae6f4
[    0.358704] pgd = fffffe0001160000
[    0.362276] [fffffe0000fae6f4] *pgd=0000004001370003, *pud=0000004001370003, *pmd=0000004001370003, *pte=02c00040011a0713
[    0.373746] Internal error: : 96000010 [#1] SMP
[    0.378484] Modules linked in:
[    0.381601] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 3.18.0-rc4+ #15
[    0.388248] Hardware name: APM X-Gene Mustang board (DT)
[    0.393738] task: fffffe03dbe10000 ti: fffffe03dbf00000 task.ti: fffffe03dbf00000
[    0.401503] PC is at acpi_ex_system_memory_space_handler+0x238/0x2e0
[    0.408160] LR is at acpi_ex_system_memory_space_handler+0x130/0x2e0

Quoted 2 lines

That happens because AML is trying to access a hardware register which has been mapped as normal memory.

Which is why removing the check in the ifdef is completely nonsensical. We already knew we can't map everything cacheable -- regardless of what AML does the CPU can prefetch from anything mapped cacheable (or simply executable) at any time.

Quoted 3 lines

So, we need a way to tell a table in ram from an io address in AML. And page_is_ram() no longer cuts it if the tables are not in the linear mapping.

If the kernel were loaded at an address above the tables, it would fail similarly. So the page_is_ram was never sufficient to ensure tables would be mapped cacheable.

As we haven't decoupled the kernel text mapping from the linear mapping, and that doesn't look to be happening any time soon, we can't fix up page_is_ram -- we need something else entirely.