arm64: update/clarify/relax Image and FDT placement rules

Commit ea8c2e112445 ("arm64: Extend the idmap to the whole kernel image") changed the early page table code so that the entire kernel Image is covered by the identity map. This allows functions that need to enable or disable the MMU to reside anywhere in the kernel Image.

However, this change has the unfortunate side effect that the Image cannot cross a physical 512 MB alignment boundary anymore, since the early page table code cannot deal with the Image crossing a /virtual/ 512 MB alignment boundary.

So instead, reduce the ID map to a single page, that is populated by the contents of the .idmap.text section. Only three functions reside there at the moment: __enable_mmu(), cpu_resume_mmu() and cpu_reset().

If new code is introduced that needs to manipulate the MMU state, it should be added to this section as well.

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

Hi Ard,

Quoted 11 lines · Ard Biesheuvel

Commit ea8c2e112445 ("arm64: Extend the idmap to the whole kernel image") changed the early page table code so that the entire kernel Image is covered by the identity map. This allows functions that need to enable or disable the MMU to reside anywhere in the kernel Image.

However, this change has the unfortunate side effect that the Image cannot cross a physical 512 MB alignment boundary anymore, since the early page table code cannot deal with the Image crossing a /virtual/ 512 MB alignment boundary.

Quoted 3 lines

So instead, reduce the ID map to a single page, that is populated by the contents of the .idmap.text section. Only three functions reside there at the moment: __enable_mmu(), cpu_resume_mmu() and cpu_reset().

It would be worth mentioning in the cover letter which branch this is based on (arm64 for-next/core?), given the __enable_mmu + __turn_mmu_on folding isn't in mainline yet.

Quoted 4 lines

If new code is introduced that needs to manipulate the MMU state, it should be added to this section as well.

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

Other than the minor nit below this looks good to me, and I've given it a spin on Juno with 4K and 64K pages. Assuming you can fix that up:

Reviewed-by Mark Rutland <[email protected]>

Tested-by Mark Rutland <[email protected]>

arch/arm64/kernel/head.S ENDPROC(__secondary_switched) · line 672

Quoted 5 lines

arch/arm64/kernel/head.S +1 -0

@@ -669,6 +669,7 @@ ENDPROC(__secondary_switched)
  *
  * other registers depend on the function called upon completion
  */
+     .section        ".idmap.text", #alloc, #execinstr

We should use "ax" rather than #alloc, #execinstr to keep things consistent, unless there's some difference that I'm missing?

I've tested the patch locally with all instances changed to "ax".

enum fixed_addresses {

arch/arm64/include/asm/fixmap.h +9 -0

 	FIX_HOLE,
+
+	/*
+	 * Reserve 4 MB of virtual space for the FDT at the top of the fixmap
+	 * region. Keep this at the top so it remains 2 MB aligned.
+	 */
+#define FIX_FDT_SIZE		SZ_4M
+	FIX_FDT_END,
+	FIX_FDT = FIX_FDT_END + FIX_FDT_SIZE / PAGE_SIZE - 1,
+
 	FIX_EARLYCON_MEM_BASE,
 	FIX_TEXT_POKE0,
 	__end_of_permanent_fixed_addresses,

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

+	u64 pfn = dt_phys_base >> PAGE_SHIFT;
+	u64 pfn_end = pfn + (FIX_FDT_SIZE >> PAGE_SHIFT);
+	pgprot_t prot = PAGE_KERNEL | PTE_RDONLY;
+

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

+	BUILD_BUG_ON(dt_virt_base & (SZ_2M - 1));
+
+	if (IS_ENABLED(CONFIG_ARM64_64K_PAGES)) {
+		pte_t *pte;
+
+		BUILD_BUG_ON(__fix_to_virt(FIX_FDT_END) >> PMD_SHIFT !=
+			     __fix_to_virt(FIX_BTMAP_BEGIN) >> PMD_SHIFT);
+
+		/* map the FDT using 64k pages */
+		pte = fixmap_pte(dt_virt_base);
+		do {
+			set_pte(pte++, pfn_pte(pfn++, prot));
+		} while (pfn < pfn_end);
+	} else {
+		pmd_t *pmd;
+
+		BUILD_BUG_ON(__fix_to_virt(FIX_FDT_END) >> PUD_SHIFT !=
+			     __fix_to_virt(FIX_BTMAP_BEGIN) >> PUD_SHIFT);
+
+		/* map the FDT using 2 MB sections */
+		pmd = fixmap_pmd(dt_virt_base);
+		do {
+			set_pmd(pmd++, pfn_pmd(pfn, mk_sect_prot(prot)));
+			pfn += PMD_SIZE >> PAGE_SHIFT;
+		} while (pfn < pfn_end);
+	}
+
+	return (void *)(dt_virt_base + dt_phys - dt_phys_base);
+}

Signature · 1 line

1.8.3.2

Hi Ard,

Documentation/arm64/booting.txt sees fit.) · line 52

Quoted 9 lines

Documentation/arm64/booting.txt +4 -4

-The device tree blob (dtb) must be placed on an 8-byte boundary within
-the first 512 megabytes from the start of the kernel image and must not
-cross a 2-megabyte boundary. This is to allow the kernel to map the
-blob using a single section mapping in the initial page tables.
+The device tree blob (dtb) must be placed on an 8-byte boundary and must
+not exceed 2 megabytes in size.
 
+NOTE: versions prior to v4.2 also require that the dtb be placed within
+512 MB of the 2 MB aligned boundary right below the kernel Image.

This reads a little odd (it sounds like you could load the DTB 512M below the boundary too).

How about:

NOTE: versions prior to v4.2 also require that the DTB be placed within the 512 MB region starting at text_offset bytes below the kernel Image.

[...]

Quoted 14 lines

enum fixed_addresses {
FIX_HOLE,

arch/arm64/include/asm/fixmap.h +9 -0

+
+     /*
+      * Reserve 4 MB of virtual space for the FDT at the top of the fixmap
+      * region. Keep this at the top so it remains 2 MB aligned.
+      */
+#define FIX_FDT_SIZE         SZ_4M
+     FIX_FDT_END,
+     FIX_FDT = FIX_FDT_END + FIX_FDT_SIZE / PAGE_SIZE - 1,
+
      FIX_EARLYCON_MEM_BASE,
      FIX_TEXT_POKE0,
      __end_of_permanent_fixed_addresses,

[...]

arch/arm64/kernel/setup.c static void __init setup_processor(void) · line 329

Quoted 9 lines

arch/arm64/kernel/setup.c +9 -0

+     /*
+      * Before passing the dt_virt pointer to early_init_dt_scan(), we have
+      * to ensure that the FDT size as reported in the FDT itself does not
+      * exceed the window we just mapped for it.
+      */
+     if (!dt_virt ||
+         fdt_check_header(dt_virt) != 0 ||
+         (dt_phys & (SZ_2M - 1)) + fdt_totalsize(dt_virt) > FIX_FDT_SIZE ||
+         !early_init_dt_scan(dt_virt)) {

Doesn't this allow a DTB > 2M (violating the boot protocol's 2M restriction)?

Perhaps we want a MAX_FDT_SIZE, and have FIX_FDT_SIZE be MAX_FDT_SIZE + 2M. Then we can check that the DTB <= MAX_FDT_SIZE, and we know we will always be able to map it.

Otherwise we could get intermittent failures for DTBs larger than 2MB, depending on where they're loaded. Always failing those for now would give us a consistent early warning that we need to bump MAX_FDT_SIZE.

[...]

arch/arm64/mm/mmu.c void __set_fixmap(enum fixed_addresses idx, · line 650

Quoted 4 lines

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

+void *__init fixmap_remap_fdt(phys_addr_t dt_phys)
+{
+     const u64 dt_virt_base = __fix_to_virt(FIX_FDT);
+     phys_addr_t dt_phys_base = dt_phys & ~(SZ_2M - 1);

Do we definitely retain the high bits here? I don't have the C integer promotion rules paged into my head, but the definition in include/linux/sizes.h makes me suspicious, given it should have an int type:

#define SZ_2M 0x00200000

Quoted 4 lines

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

+     u64 pfn = dt_phys_base >> PAGE_SHIFT;
+     u64 pfn_end = pfn + (FIX_FDT_SIZE >> PAGE_SHIFT);
+     pgprot_t prot = PAGE_KERNEL | PTE_RDONLY;
+

arch/arm64/mm/mmu.c void __set_fixmap(enum fixed_addresses idx, · line 664

Quoted 10 lines

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

+     /*
+      * Make sure that the FDT region can be mapped without the need to
+      * allocate additional translation table pages, or perform physical
+      * to virtual address translations via the linear mapping.
+      *
+      * On 4k pages, we'll use section mappings for the region so we
+      * only have to be in the same PUD as the rest of the fixmap.
+      * On 64k pages, we need to be in the same PMD as well, as the region
+      * will be mapped using PTEs.
+      */

It would be nice to have the last two statements swapped (with appropriate wording fixups) to match the order of the if branches.

Otherwise this looks good to me.

Quoted 32 lines

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

+     BUILD_BUG_ON(dt_virt_base & (SZ_2M - 1));
+
+     if (IS_ENABLED(CONFIG_ARM64_64K_PAGES)) {
+             pte_t *pte;
+
+             BUILD_BUG_ON(__fix_to_virt(FIX_FDT_END) >> PMD_SHIFT !=
+                          __fix_to_virt(FIX_BTMAP_BEGIN) >> PMD_SHIFT);
+
+             /* map the FDT using 64k pages */
+             pte = fixmap_pte(dt_virt_base);
+             do {
+                     set_pte(pte++, pfn_pte(pfn++, prot));
+             } while (pfn < pfn_end);
+     } else {
+             pmd_t *pmd;
+
+             BUILD_BUG_ON(__fix_to_virt(FIX_FDT_END) >> PUD_SHIFT !=
+                          __fix_to_virt(FIX_BTMAP_BEGIN) >> PUD_SHIFT);
+
+             /* map the FDT using 2 MB sections */
+             pmd = fixmap_pmd(dt_virt_base);
+             do {
+                     set_pmd(pmd++, pfn_pmd(pfn, mk_sect_prot(prot)));
+                     pfn += PMD_SIZE >> PAGE_SHIFT;
+             } while (pfn < pfn_end);
+     }
+
+     return (void *)(dt_virt_base + dt_phys - dt_phys_base);
+}
--

1.8.3.2

Quoted 10 lines

arch/arm64/kernel/setup.c static void __init setup_processor(void) · line 329

Quoted 9 lines

arch/arm64/kernel/setup.c +9 -0

+     /*
+      * Before passing the dt_virt pointer to early_init_dt_scan(), we have
+      * to ensure that the FDT size as reported in the FDT itself does not
+      * exceed the window we just mapped for it.
+      */
+     if (!dt_virt ||
+         fdt_check_header(dt_virt) != 0 ||
+         (dt_phys & (SZ_2M - 1)) + fdt_totalsize(dt_virt) > FIX_FDT_SIZE ||
+         !early_init_dt_scan(dt_virt)) {

Quoted 3 lines

Doesn't this allow a DTB > 2M (violating the boot protocol's 2M restriction)?

Yes it does, and I was wondering what to do about it. We could be pedantic, and fail here with no user visible feedback about what went wrong, or we could be lax here, and add a warning later (similar to the x1/x2/x3 check) that the boot protocol was violated if the FDT exceeds 2 MB but otherwise proceed normally.

Quoted 4 lines

Perhaps we want a MAX_FDT_SIZE, and have FIX_FDT_SIZE be MAX_FDT_SIZE + 2M. Then we can check that the DTB <= MAX_FDT_SIZE, and we know we will always be able to map it.

Quoted 4 lines

Otherwise we could get intermittent failures for DTBs larger than 2MB, depending on where they're loaded. Always failing those for now would give us a consistent early warning that we need to bump MAX_FDT_SIZE.

Consistent, early, but emitted before we even have a console ...

Quoted 10 lines

arch/arm64/mm/mmu.c void __set_fixmap(enum fixed_addresses idx, · line 650

Quoted 4 lines

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

+void *__init fixmap_remap_fdt(phys_addr_t dt_phys)
+{
+     const u64 dt_virt_base = __fix_to_virt(FIX_FDT);
+     phys_addr_t dt_phys_base = dt_phys & ~(SZ_2M - 1);

Do we definitely retain the high bits here? I don't have the C integer promotion rules paged into my head, but the definition in include/linux/sizes.h makes me suspicious, given it should have an int type:

Quoted 2 lines

#define SZ_2M 0x00200000

I will use round_down() instead

Quoted 29 lines

Quoted 10 lines

arch/arm64/kernel/setup.c static void __init setup_processor(void) · line 329

Quoted 9 lines

arch/arm64/kernel/setup.c +9 -0

+     /*
+      * Before passing the dt_virt pointer to early_init_dt_scan(), we have
+      * to ensure that the FDT size as reported in the FDT itself does not
+      * exceed the window we just mapped for it.
+      */
+     if (!dt_virt ||
+         fdt_check_header(dt_virt) != 0 ||
+         (dt_phys & (SZ_2M - 1)) + fdt_totalsize(dt_virt) > FIX_FDT_SIZE ||
+         !early_init_dt_scan(dt_virt)) {

Quoted 3 lines

Doesn't this allow a DTB > 2M (violating the boot protocol's 2M restriction)?

Yes it does, and I was wondering what to do about it. We could be pedantic, and fail here with no user visible feedback about what went wrong, or we could be lax here, and add a warning later (similar to the x1/x2/x3 check) that the boot protocol was violated if the FDT exceeds 2 MB but otherwise proceed normally.

Quoted 8 lines

Perhaps we want a MAX_FDT_SIZE, and have FIX_FDT_SIZE be MAX_FDT_SIZE + 2M. Then we can check that the DTB <= MAX_FDT_SIZE, and we know we will always be able to map it.

Otherwise we could get intermittent failures for DTBs larger than 2MB, depending on where they're loaded. Always failing those for now would give us a consistent early warning that we need to bump MAX_FDT_SIZE.

Quoted 1 line

Consistent, early, but emitted before we even have a console ...

When I say warning, I mean that boot will consistently fail, rather than there being a pr_warn.

People seem to wait until their platform is hilariously broken before reporting bugs...

Quoted 10 lines

Quoted 10 lines

arch/arm64/mm/mmu.c void __set_fixmap(enum fixed_addresses idx, · line 650

Quoted 4 lines

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

+void *__init fixmap_remap_fdt(phys_addr_t dt_phys)
+{
+     const u64 dt_virt_base = __fix_to_virt(FIX_FDT);
+     phys_addr_t dt_phys_base = dt_phys & ~(SZ_2M - 1);

Do we definitely retain the high bits here? I don't have the C integer promotion rules paged into my head, but the definition in include/linux/sizes.h makes me suspicious, given it should have an int type:

Quoted 4 lines

Quoted 2 lines

#define SZ_2M 0x00200000

I will use round_down() instead

Ok.

Mark.

I was wondering: this code now always maps the full 4 MB, also on 64k pages, even if in the majority of cases, the actual FDT is much smaller. Do you think there is a downside to that?

Quoted 3 lines

I was wondering: this code now always maps the full 4 MB, also on 64k pages, even if in the majority of cases, the actual FDT is much smaller. Do you think there is a downside to that?

The two cases I can think of as problematic are when the FDT is in the last 2M of RAM, or in a 2M region immediately before some carveout which won't use MT_NORMAL attributes.

We could solve those by only mapping however many 2M chunks are necessary, or we could keep the requirement that it fits within a naturally-aligned 2M region for now.

I don't see that this should yield significant problems otherwise.

Mark.

Quoted 4 lines · Mark Rutland

Quoted 3 lines

I was wondering: this code now always maps the full 4 MB, also on 64k pages, even if in the majority of cases, the actual FDT is much smaller. Do you think there is a downside to that?

Quoted 8 lines

The two cases I can think of as problematic are when the FDT is in the last 2M of RAM, or in a 2M region immediately before some carveout which won't use MT_NORMAL attributes.

We could solve those by only mapping however many 2M chunks are necessary, or we could keep the requirement that it fits within a naturally-aligned 2M region for now.

I strongly prefer the former, since otherwise, you always need to verify the allocation explicitly, for an fdt of any size, and it turns out the logic is easily updated to move the vetting into fixmap_remap_fdt() and check the magic and size after mapping the first chunk, and set pfn_end accordingly

With the relaxed FDT placement requirements in place, we can change the allocation strategy used by the stub to put the FDT image higher up in memory. At the same time, reduce the minimal alignment to 8 bytes, and impose a 2 MB size limit, as per the new requirements.

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

---
 arch/arm64/include/asm/efi.h            |  8 +++-----
 drivers/firmware/efi/libstub/arm-stub.c |  5 ++---
 drivers/firmware/efi/libstub/efistub.h  |  1 -
 drivers/firmware/efi/libstub/fdt.c      | 23 +++++++++++++----------
 4 files changed, 18 insertions(+), 19 deletions(-)

arch/arm64/include/asm/efi.h +0 -0

index ef572206f1c3..bd513dd663b9 100644
@@ -39,12 +39,10 @@ extern void efi_init(void);
 /* arch specific definitions used by the stub code */
 

arch/arm64/include/asm/efi.h +0 -0

 #define efi_call_early(f, ...) sys_table_arg->boottime->f(__VA_ARGS__)
 

drivers/firmware/efi/libstub/arm-stub.c +2 -3

index dcae482a9a17..f54c76a4fd32 100644
@@ -269,9 +269,8 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table,
 
 	new_fdt_addr = fdt_addr;
 	status = allocate_new_fdt_and_exit_boot(sys_table, handle,
-				&new_fdt_addr, dram_base + MAX_FDT_OFFSET,
-				initrd_addr, initrd_size, cmdline_ptr,
-				fdt_addr, fdt_size);
+				&new_fdt_addr, initrd_addr, initrd_size,
+				cmdline_ptr, fdt_addr, fdt_size);
 
 	/*
 	 * If all went well, we need to return the FDT address to the

drivers/firmware/efi/libstub/efistub.h +0 -1

index 47437b16b186..c8e096094ea9 100644
@@ -35,7 +35,6 @@ efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt,
 efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
 					    void *handle,
 					    unsigned long *new_fdt_addr,
-					    unsigned long max_addr,
 					    u64 initrd_addr, u64 initrd_size,
 					    char *cmdline_ptr,
 					    unsigned long fdt_addr,

drivers/firmware/efi/libstub/fdt.c +13 -10

index 91da56c4fd54..ace5ed70a88e 100644
@@ -165,10 +165,6 @@ fdt_set_fail:
 	return EFI_LOAD_ERROR;
 }
 
-#ifndef EFI_FDT_ALIGN
-#define EFI_FDT_ALIGN EFI_PAGE_SIZE
-#endif
-
 /*
  * Allocate memory for a new FDT, then add EFI, commandline, and
  * initrd related fields to the FDT.  This routine increases the

@@ -186,7 +182,6 @@ fdt_set_fail:
 efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
 					    void *handle,
 					    unsigned long *new_fdt_addr,
-					    unsigned long max_addr,
 					    u64 initrd_addr, u64 initrd_size,
 					    char *cmdline_ptr,
 					    unsigned long fdt_addr,

@@ -197,6 +192,7 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
 	unsigned long mmap_key;
 	efi_memory_desc_t *memory_map, *runtime_map;
 	unsigned long new_fdt_size;
+	void *fdt_alloc;
 	efi_status_t status;
 	int runtime_entry_count = 0;
 

@@ -221,14 +217,21 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
 	 * will allocate a bigger buffer if this ends up being too
 	 * small, so a rough guess is OK here.
 	 */
-	new_fdt_size = fdt_size + EFI_PAGE_SIZE;
+	new_fdt_size = fdt_size + EFI_PAGE_SIZE + EFI_FDT_ALIGN;
 	while (1) {
-		status = efi_high_alloc(sys_table, new_fdt_size, EFI_FDT_ALIGN,
-					new_fdt_addr, max_addr);
+		if (new_fdt_size > EFI_FDT_MAX_SIZE) {
+			pr_efi_err(sys_table, "FDT size exceeds EFI_FDT_MAX_SIZE.\n");
+			goto fail;
+		}
+		status = sys_table->boottime->allocate_pool(EFI_LOADER_DATA,
+							    new_fdt_size,
+							    &fdt_alloc);
 		if (status != EFI_SUCCESS) {
 			pr_efi_err(sys_table, "Unable to allocate memory for new device tree.\n");
 			goto fail;
 		}
+		*new_fdt_addr = round_up((unsigned long)fdt_alloc,
+					 EFI_FDT_ALIGN);
 
 		/*
 		 * Now that we have done our final memory allocation (and free)

@@ -258,7 +261,7 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
 			 * to get new one that reflects the free/alloc we do
 			 * on the device tree buffer.
 			 */
-			efi_free(sys_table, new_fdt_size, *new_fdt_addr);
+			sys_table->boottime->free_pool(&fdt_alloc);
 			sys_table->boottime->free_pool(memory_map);
 			new_fdt_size += EFI_PAGE_SIZE;
 		} else {

@@ -316,7 +319,7 @@ fail_free_mmap:
 	sys_table->boottime->free_pool(memory_map);
 
 fail_free_new_fdt:
-	efi_free(sys_table, new_fdt_size, *new_fdt_addr);
+	sys_table->boottime->free_pool(&fdt_alloc);
 
 fail:
 	sys_table->boottime->free_pool(runtime_map);

Signature · 1 line

1.8.3.2

Quoted 20 lines · Ard Biesheuvel

With the relaxed FDT placement requirements in place, we can change the allocation strategy used by the stub to put the FDT image higher up in memory. At the same time, reduce the minimal alignment to 8 bytes, and impose a 2 MB size limit, as per the new requirements.

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

---
 arch/arm64/include/asm/efi.h            |  8 +++-----
 drivers/firmware/efi/libstub/arm-stub.c |  5 ++---
 drivers/firmware/efi/libstub/efistub.h  |  1 -
 drivers/firmware/efi/libstub/fdt.c      | 23 +++++++++++++----------
 4 files changed, 18 insertions(+), 19 deletions(-)

arch/arm64/include/asm/efi.h +0 -0

index ef572206f1c3..bd513dd663b9 100644
@@ -39,12 +39,10 @@ extern void efi_init(void);
 /* arch specific definitions used by the stub code */
 

arch/arm64/include/asm/efi.h extern void efi_init(void); · line 45

Quoted 10 lines

arch/arm64/include/asm/efi.h +3 -5

 /*
- * AArch64 requires the DTB to be 8-byte aligned in the first 512MiB from
- * start of kernel and may not cross a 2MiB boundary. We set alignment to
- * 2MiB so we know it won't cross a 2MiB boundary.
+ * AArch64 requires the DTB to be 8-byte aligned and not exceed 2MiB in size.
  */
-#define EFI_FDT_ALIGN        SZ_2M   /* used by allocate_new_fdt_and_exit_boot() */
-#define MAX_FDT_OFFSET       SZ_512M
+#define EFI_FDT_ALIGN                8
+#define EFI_FDT_MAX_SIZE     SZ_2M

Is there any way we can organise things so we can share the kernel's MAX_FDT_SIZE definition, so as to keep the stub and kernel in sync?

Otherwise, the patch looks good to me.

Mark.

Quoted 108 lines

arch/arm64/include/asm/efi.h +0 -0

 #define efi_call_early(f, ...) sys_table_arg->boottime->f(__VA_ARGS__)
 

drivers/firmware/efi/libstub/arm-stub.c +2 -3

index dcae482a9a17..f54c76a4fd32 100644
@@ -269,9 +269,8 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table,
 
      new_fdt_addr = fdt_addr;
      status = allocate_new_fdt_and_exit_boot(sys_table, handle,
-                             &new_fdt_addr, dram_base + MAX_FDT_OFFSET,
-                             initrd_addr, initrd_size, cmdline_ptr,
-                             fdt_addr, fdt_size);
+                             &new_fdt_addr, initrd_addr, initrd_size,
+                             cmdline_ptr, fdt_addr, fdt_size);
 
      /*
       * If all went well, we need to return the FDT address to the

drivers/firmware/efi/libstub/efistub.h +0 -1

index 47437b16b186..c8e096094ea9 100644
@@ -35,7 +35,6 @@ efi_status_t update_fdt(efi_system_table_t *sys_table, void *orig_fdt,
 efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
                                          void *handle,
                                          unsigned long *new_fdt_addr,
-                                         unsigned long max_addr,
                                          u64 initrd_addr, u64 initrd_size,
                                          char *cmdline_ptr,
                                          unsigned long fdt_addr,

drivers/firmware/efi/libstub/fdt.c +13 -11

index 91da56c4fd54..ace5ed70a88e 100644
@@ -165,10 +165,6 @@ fdt_set_fail:
      return EFI_LOAD_ERROR;
 }
 
-#ifndef EFI_FDT_ALIGN
-#define EFI_FDT_ALIGN EFI_PAGE_SIZE
-#endif
-
 /*
  * Allocate memory for a new FDT, then add EFI, commandline, and
  * initrd related fields to the FDT.  This routine increases the

@@ -186,7 +182,6 @@ fdt_set_fail:
 efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
                                          void *handle,
                                          unsigned long *new_fdt_addr,
-                                         unsigned long max_addr,
                                          u64 initrd_addr, u64 initrd_size,
                                          char *cmdline_ptr,
                                          unsigned long fdt_addr,

@@ -197,6 +192,7 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
      unsigned long mmap_key;
      efi_memory_desc_t *memory_map, *runtime_map;
      unsigned long new_fdt_size;
+     void *fdt_alloc;
      efi_status_t status;
      int runtime_entry_count = 0;
 

@@ -221,14 +217,21 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
       * will allocate a bigger buffer if this ends up being too
       * small, so a rough guess is OK here.
       */
-     new_fdt_size = fdt_size + EFI_PAGE_SIZE;
+     new_fdt_size = fdt_size + EFI_PAGE_SIZE + EFI_FDT_ALIGN;
      while (1) {
-             status = efi_high_alloc(sys_table, new_fdt_size, EFI_FDT_ALIGN,
-                                     new_fdt_addr, max_addr);
+             if (new_fdt_size > EFI_FDT_MAX_SIZE) {
+                     pr_efi_err(sys_table, "FDT size exceeds EFI_FDT_MAX_SIZE.\n");
+                     goto fail;
+             }
+             status = sys_table->boottime->allocate_pool(EFI_LOADER_DATA,
+                                                         new_fdt_size,
+                                                         &fdt_alloc);
              if (status != EFI_SUCCESS) {
                      pr_efi_err(sys_table, "Unable to allocate memory for new device tree.\n");
                      goto fail;
              }
+             *new_fdt_addr = round_up((unsigned long)fdt_alloc,
+                                      EFI_FDT_ALIGN);
 
              /*
               * Now that we have done our final memory allocation (and free)

@@ -258,7 +261,7 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
                       * to get new one that reflects the free/alloc we do
                       * on the device tree buffer.
                       */
-                     efi_free(sys_table, new_fdt_size, *new_fdt_addr);
+                     sys_table->boottime->free_pool(&fdt_alloc);
                      sys_table->boottime->free_pool(memory_map);
                      new_fdt_size += EFI_PAGE_SIZE;
              } else {

@@ -316,7 +319,7 @@ fail_free_mmap:
      sys_table->boottime->free_pool(memory_map);
 
 fail_free_new_fdt:
-     efi_free(sys_table, new_fdt_size, *new_fdt_addr);
+     sys_table->boottime->free_pool(&fdt_alloc);
 
 fail:
      sys_table->boottime->free_pool(runtime_map);
--

1.8.3.2

Quoted 21 lines · Mark Rutland

Quoted 20 lines · Ard Biesheuvel

With the relaxed FDT placement requirements in place, we can change the allocation strategy used by the stub to put the FDT image higher up in memory. At the same time, reduce the minimal alignment to 8 bytes, and impose a 2 MB size limit, as per the new requirements.

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

---
 arch/arm64/include/asm/efi.h            |  8 +++-----
 drivers/firmware/efi/libstub/arm-stub.c |  5 ++---
 drivers/firmware/efi/libstub/efistub.h  |  1 -
 drivers/firmware/efi/libstub/fdt.c      | 23 +++++++++++++----------
 4 files changed, 18 insertions(+), 19 deletions(-)

arch/arm64/include/asm/efi.h +0 -0

index ef572206f1c3..bd513dd663b9 100644
@@ -39,12 +39,10 @@ extern void efi_init(void);
 /* arch specific definitions used by the stub code */
 

Quoted 14 lines

arch/arm64/include/asm/efi.h extern void efi_init(void); · line 45

Quoted 10 lines

arch/arm64/include/asm/efi.h +3 -5

 /*
- * AArch64 requires the DTB to be 8-byte aligned in the first 512MiB from
- * start of kernel and may not cross a 2MiB boundary. We set alignment to
- * 2MiB so we know it won't cross a 2MiB boundary.
+ * AArch64 requires the DTB to be 8-byte aligned and not exceed 2MiB in size.
  */
-#define EFI_FDT_ALIGN        SZ_2M   /* used by allocate_new_fdt_and_exit_boot() */
-#define MAX_FDT_OFFSET       SZ_512M
+#define EFI_FDT_ALIGN                8
+#define EFI_FDT_MAX_SIZE     SZ_2M

Is there any way we can organise things so we can share the kernel's MAX_FDT_SIZE definition, so as to keep the stub and kernel in sync?

I'll put MAX_FDT_SIZE in asm/page.h (unless you can propose a better place) That way, both fixmap.h and efi.h can refer to it.

Quoted 37 lines · Ard Biesheuvel

Quoted 21 lines · Mark Rutland

Quoted 20 lines · Ard Biesheuvel

With the relaxed FDT placement requirements in place, we can change the allocation strategy used by the stub to put the FDT image higher up in memory. At the same time, reduce the minimal alignment to 8 bytes, and impose a 2 MB size limit, as per the new requirements.

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

---
 arch/arm64/include/asm/efi.h            |  8 +++-----
 drivers/firmware/efi/libstub/arm-stub.c |  5 ++---
 drivers/firmware/efi/libstub/efistub.h  |  1 -
 drivers/firmware/efi/libstub/fdt.c      | 23 +++++++++++++----------
 4 files changed, 18 insertions(+), 19 deletions(-)

arch/arm64/include/asm/efi.h +0 -0

index ef572206f1c3..bd513dd663b9 100644
@@ -39,12 +39,10 @@ extern void efi_init(void);
 /* arch specific definitions used by the stub code */
 

Quoted 14 lines

arch/arm64/include/asm/efi.h extern void efi_init(void); · line 45

Quoted 10 lines

arch/arm64/include/asm/efi.h +3 -5

 /*
- * AArch64 requires the DTB to be 8-byte aligned in the first 512MiB from
- * start of kernel and may not cross a 2MiB boundary. We set alignment to
- * 2MiB so we know it won't cross a 2MiB boundary.
+ * AArch64 requires the DTB to be 8-byte aligned and not exceed 2MiB in size.
  */
-#define EFI_FDT_ALIGN        SZ_2M   /* used by allocate_new_fdt_and_exit_boot() */
-#define MAX_FDT_OFFSET       SZ_512M
+#define EFI_FDT_ALIGN                8
+#define EFI_FDT_MAX_SIZE     SZ_2M

Is there any way we can organise things so we can share the kernel's MAX_FDT_SIZE definition, so as to keep the stub and kernel in sync?

Quoted 2 lines

I'll put MAX_FDT_SIZE in asm/page.h (unless you can propose a better place) That way, both fixmap.h and efi.h can refer to it.

Using page.h feels a little weird, but I can't see that a better header exists currently.

We could add asm/boot.h for things related to boot protocol that we want to have shareable with the stub, but I can't see anything else that we'd want to put in there at the moment. If there are other things we could deduplicate it would probably make sense, but it seems overkill for a single definition. :/

This reworks early_ioremap_init() so it populates the various levels of translation tables while taking the following into account:
- be prepared for any of the levels to have been populated already, as this may be the case once we move the kernel text mapping out of the linear mapping;
- don't rely on __va() to translate the physical address in a page table entry to a virtual address, since this produces linear mapping addresses; instead, use the fact that at any level, we know exactly which page in swapper_pg_dir an entry could be pointing to if it points anywhere.

This allows us to defer the initialization of the linear mapping until after we have figured out where our RAM resides in the physical address space.

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

---
 arch/arm64/include/asm/compiler.h |   2 +
 arch/arm64/kernel/vmlinux.lds.S   |  14 +++--
 arch/arm64/mm/mmu.c               | 117 +++++++++++++++++++++++++-------------
 3 files changed, 90 insertions(+), 43 deletions(-)

arch/arm64/include/asm/compiler.h +2 -0

index ee35fd0f2236..dd342af63673 100644
@@ -27,4 +27,6 @@
  */
 #define __asmeq(x, y)  ".ifnc " x "," y " ; .err ; .endif\n\t"
 
+#define __pgdir		__attribute__((section(".pgdir"),aligned(PAGE_SIZE)))
+
 #endif	/* __ASM_COMPILER_H */

arch/arm64/kernel/vmlinux.lds.S +9 -5

index 98073332e2d0..604f285d3832 100644
@@ -160,11 +160,15 @@ SECTIONS
 
 	BSS_SECTION(0, 0, 0)
 
-	. = ALIGN(PAGE_SIZE);
-	idmap_pg_dir = .;
-	. += IDMAP_DIR_SIZE;
-	swapper_pg_dir = .;
-	. += SWAPPER_DIR_SIZE;
+	.pgdir (NOLOAD) : {
+		. = ALIGN(PAGE_SIZE);
+		idmap_pg_dir = .;
+		. += IDMAP_DIR_SIZE;
+		swapper_pg_dir = .;
+		__swapper_bs_region = . + PAGE_SIZE;
+		. += SWAPPER_DIR_SIZE;
+		*(.pgdir)
+	}
 
 	_end = .;
 

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

index 60be58a160a2..c0427b5c90c7 100644

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

+#if CONFIG_ARM64_PGTABLE_LEVELS > 3
+	pud_t	pud[PTRS_PER_PUD];
+#endif
+#if CONFIG_ARM64_PGTABLE_LEVELS > 2
+	pmd_t	pmd[PTRS_PER_PMD];
+#endif
+	pte_t	pte[PTRS_PER_PTE];
+};
+
+static void __init bootstrap_mem_region(unsigned long addr,
+					struct mem_bootstrap_region *reg,
+					pmd_t **ppmd, pte_t **ppte)
+{
+	/*
+	 * Avoid using the linear phys-to-virt translation __va() so that we
+	 * can use this code before the linear mapping is set up. Note that
+	 * any populated entries at any level can only point into swapper_pg_dir
+	 * since no other translation table pages have been allocated yet.
+	 * So at each level, we either need to populate it, or it has already
+	 * been populated by a swapper_pg_dir table at the same level, in which
+	 * case we can figure out its virtual address without applying __va()
+	 * on the contents of the entry, using the following struct.
+	 */

Hi Ard,

Quoted 9 lines · Ard Biesheuvel

This reworks early_ioremap_init() so it populates the various levels of translation tables while taking the following into account:
- be prepared for any of the levels to have been populated already, as this may be the case once we move the kernel text mapping out of the linear mapping;
- don't rely on __va() to translate the physical address in a page table entry to a virtual address, since this produces linear mapping addresses; instead, use the fact that at any level, we know exactly which page in swapper_pg_dir an entry could be pointing to if it points anywhere.

Can we not use Catalin's PHYS_OFFSET swizzling trick instead? i.e.

* Set PHYS_OFFSET so __va hits in the text mapping.

* Create the fixmap entries.

* Parse the DTB or UEFI memory map, figure out the real PHYS_OFFSET.

* Create linear mapping for the initial tables.

* Swap PHYS_OFFSET for the real version, and update init_mm->pgd to point at the linear map alias of the swapper pgd.

It seemed like that would require less open-coding of table manipulation code, as we could use __va() early.

Is there a limitation with that approach that I'm missing?

Otherwise, comments below.

Quoted 55 lines

This allows us to defer the initialization of the linear mapping until after we have figured out where our RAM resides in the physical address space.

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

---
 arch/arm64/include/asm/compiler.h |   2 +
 arch/arm64/kernel/vmlinux.lds.S   |  14 +++--
 arch/arm64/mm/mmu.c               | 117 +++++++++++++++++++++++++-------------
 3 files changed, 90 insertions(+), 43 deletions(-)

arch/arm64/include/asm/compiler.h +2 -0

index ee35fd0f2236..dd342af63673 100644
@@ -27,4 +27,6 @@
  */
 #define __asmeq(x, y)  ".ifnc " x "," y " ; .err ; .endif\n\t"
 
+#define __pgdir              __attribute__((section(".pgdir"),aligned(PAGE_SIZE)))
+
 #endif       /* __ASM_COMPILER_H */

arch/arm64/kernel/vmlinux.lds.S +9 -5

index 98073332e2d0..604f285d3832 100644
@@ -160,11 +160,15 @@ SECTIONS
 
      BSS_SECTION(0, 0, 0)
 
-     . = ALIGN(PAGE_SIZE);
-     idmap_pg_dir = .;
-     . += IDMAP_DIR_SIZE;
-     swapper_pg_dir = .;
-     . += SWAPPER_DIR_SIZE;
+     .pgdir (NOLOAD) : {
+             . = ALIGN(PAGE_SIZE);
+             idmap_pg_dir = .;
+             . += IDMAP_DIR_SIZE;
+             swapper_pg_dir = .;
+             __swapper_bs_region = . + PAGE_SIZE;
+             . += SWAPPER_DIR_SIZE;
+             *(.pgdir)
+     }
 
      _end = .;
 

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

index 60be58a160a2..c0427b5c90c7 100644
@@ -341,6 +341,70 @@ static void __init __map_memblock(phys_addr_t start, phys_addr_t end)
 }
 #endif
 

arch/arm64/mm/mmu.c static void __init __map_memblock(phys_addr_t start, phys_addr_t end) · line 344

Quoted 1 line

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

+struct mem_bootstrap_region {

The "region" naming is a little confusing IMO. To me it sounds like something akin to a VMA rather than a set of tables.

Quoted 23 lines

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

+#if CONFIG_ARM64_PGTABLE_LEVELS > 3
+     pud_t   pud[PTRS_PER_PUD];
+#endif
+#if CONFIG_ARM64_PGTABLE_LEVELS > 2
+     pmd_t   pmd[PTRS_PER_PMD];
+#endif
+     pte_t   pte[PTRS_PER_PTE];
+};
+
+static void __init bootstrap_mem_region(unsigned long addr,
+                                     struct mem_bootstrap_region *reg,
+                                     pmd_t **ppmd, pte_t **ppte)
+{
+     /*
+      * Avoid using the linear phys-to-virt translation __va() so that we
+      * can use this code before the linear mapping is set up. Note that
+      * any populated entries at any level can only point into swapper_pg_dir
+      * since no other translation table pages have been allocated yet.
+      * So at each level, we either need to populate it, or it has already
+      * been populated by a swapper_pg_dir table at the same level, in which
+      * case we can figure out its virtual address without applying __va()
+      * on the contents of the entry, using the following struct.
+      */

arch/arm64/mm/mmu.c static void __init __map_memblock(phys_addr_t start, phys_addr_t end) · line 377

Quoted 10 lines

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

+     extern struct mem_bootstrap_region __swapper_bs_region;
+
+     pgd_t *pgd = pgd_offset_k(addr);
+     pud_t *pud = (pud_t *)pgd;
+     pmd_t *pmd = (pmd_t *)pud;
+
+#if CONFIG_ARM64_PGTABLE_LEVELS > 3
+     if (pgd_none(*pgd)) {
+             clear_page(reg->pud);
+             pgd_populate(&init_mm, pgd, reg->pud);

What's PHYS_OFFSET expected to be at this point (for the purposes of __pa() in the *_populate*() macros)?

If we're relying on __pa() to convert text->phys, won't __va() convert phys->text at this point?

arch/arm64/mm/mmu.c static void __init __map_memblock(phys_addr_t start, phys_addr_t end) · line 383

Quoted 6 lines

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

+             pud = reg->pud;
+     } else {
+             pud = __swapper_bs_region.pud;
+     }
+     pud += pud_index(addr);
+#endif

Can we free the unused reg tables in the else cases? If __pa() works we should be able to hand them to memblock, no?

[...]

arch/arm64/mm/mmu.c void vmemmap_free(unsigned long start, unsigned long end) · line 649

Quoted 4 lines

/*
* The boot-ioremap range spans multiple pmds, for which

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

-      * we are not preparted:
+      * we are not prepared:

This typo has been bugging me for ages. I'll be glad to see it go!

Quoted 29 lines · Mark Rutland

Hi Ard,

Quoted 9 lines · Ard Biesheuvel

This reworks early_ioremap_init() so it populates the various levels of translation tables while taking the following into account:
- be prepared for any of the levels to have been populated already, as this may be the case once we move the kernel text mapping out of the linear mapping;
- don't rely on __va() to translate the physical address in a page table entry to a virtual address, since this produces linear mapping addresses; instead, use the fact that at any level, we know exactly which page in swapper_pg_dir an entry could be pointing to if it points anywhere.

Can we not use Catalin's PHYS_OFFSET swizzling trick instead? i.e.

* Set PHYS_OFFSET so __va hits in the text mapping.

* Create the fixmap entries.

* Parse the DTB or UEFI memory map, figure out the real PHYS_OFFSET.

* Create linear mapping for the initial tables.

* Swap PHYS_OFFSET for the real version, and update init_mm->pgd to point at the linear map alias of the swapper pgd.

It seemed like that would require less open-coding of table manipulation code, as we could use __va() early.

Quoted 2 lines

Is there a limitation with that approach that I'm missing?

I didn't quite catch Catalin's suggestion to mean the above, but the way you put it seems viable to me. I'll have a go and see how far I get with it.

Quoted 37 lines

Quoted 23 lines

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

+#if CONFIG_ARM64_PGTABLE_LEVELS > 3
+     pud_t   pud[PTRS_PER_PUD];
+#endif
+#if CONFIG_ARM64_PGTABLE_LEVELS > 2
+     pmd_t   pmd[PTRS_PER_PMD];
+#endif
+     pte_t   pte[PTRS_PER_PTE];
+};
+
+static void __init bootstrap_mem_region(unsigned long addr,
+                                     struct mem_bootstrap_region *reg,
+                                     pmd_t **ppmd, pte_t **ppte)
+{
+     /*
+      * Avoid using the linear phys-to-virt translation __va() so that we
+      * can use this code before the linear mapping is set up. Note that
+      * any populated entries at any level can only point into swapper_pg_dir
+      * since no other translation table pages have been allocated yet.
+      * So at each level, we either need to populate it, or it has already
+      * been populated by a swapper_pg_dir table at the same level, in which
+      * case we can figure out its virtual address without applying __va()
+      * on the contents of the entry, using the following struct.
+      */

arch/arm64/mm/mmu.c static void __init __map_memblock(phys_addr_t start, phys_addr_t end) · line 377

Quoted 10 lines

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

+     extern struct mem_bootstrap_region __swapper_bs_region;
+
+     pgd_t *pgd = pgd_offset_k(addr);
+     pud_t *pud = (pud_t *)pgd;
+     pmd_t *pmd = (pmd_t *)pud;
+
+#if CONFIG_ARM64_PGTABLE_LEVELS > 3
+     if (pgd_none(*pgd)) {
+             clear_page(reg->pud);
+             pgd_populate(&init_mm, pgd, reg->pud);

What's PHYS_OFFSET expected to be at this point (for the purposes of __pa() in the *_populate*() macros)?

Quoted 3 lines

If we're relying on __pa() to convert text->phys, won't __va() convert phys->text at this point?

At this points, yes. But later on, when the kernel text moves out of the linear region, __pa() takes into account whether the input VA is above or below PAGE_OFFSET, and adds the kernel image offset in the latter case.

Changing __va() so it implements the inverse would be a can of worms i'd rather keep closed.

Quoted 7 lines

Quoted 6 lines

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

+             pud = reg->pud;
+     } else {
+             pud = __swapper_bs_region.pud;
+     }
+     pud += pud_index(addr);
+#endif

Quoted 3 lines

Can we free the unused reg tables in the else cases? If __pa() works we should be able to hand them to memblock, no?

Only if we put the memblock_reserve() of the kernel image before early_fixmap_init(), otherwise we are freeing only to reserve it again later.

This relaxes the kernel Image placement requirements, so that it may be placed at any 2 MB aligned offset in physical memory.

This is accomplished by ignoring PHYS_OFFSET when installing memblocks, and accounting for the apparent virtual offset of the kernel Image (in addition to the 64 MB that is is moved below PAGE_OFFSET). As a result, virtual address references below PAGE_OFFSET are correctly mapped onto physical references into the kernel Image regardless of where it sits in memory.

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

---
 Documentation/arm64/booting.txt | 17 +++++++----------
 arch/arm64/mm/init.c            | 32 +++++++++++++++++++-------------
 2 files changed, 26 insertions(+), 23 deletions(-)

Documentation/arm64/booting.txt +0 -7

index 6396460f6085..811d93548bdc 100644
@@ -110,16 +110,13 @@ Header notes:
   depending on selected features, and is effectively unbound.
 
 The Image must be placed text_offset bytes from a 2MB aligned base
-address near the start of usable system RAM and called there. Memory
-below that base address is currently unusable by Linux, and therefore it
-is strongly recommended that this location is the start of system RAM.
-At least image_size bytes from the start of the image must be free for
-use by the kernel.
-
-Any memory described to the kernel (even that below the 2MB aligned base

Documentation/arm64/booting.txt +8 -0

+	/*
+	 * Set memstart_addr to the base of the lowest physical memory region,
+	 * rounded down to PUD/PMD alignment so we can map it efficiently.
+	 * Since this also affects the apparent offset of the kernel image in
+	 * the virtual address space, increase image_offset by the same amount
+	 * that we decrease memstart_addr.
+	 */
+	if (!memstart_addr || memstart_addr > base) {