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

This splits off the reservation of the memory occupied by the FDT binary itself from the processing of the memory reservations it contains. This is necessary because the physical address of the FDT, which is needed to perform the reservation, may not be known to the FDT driver core, i.e., it may be mapped outside the linear direct mapping, in which case __pa() returns a bogus value.

Cc Russell King <[email protected]>

Cc Benjamin Herrenschmidt <[email protected]>

Cc Paul Mackerras <[email protected]>

Acked-by Rob Herring <[email protected]>

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

---
 arch/arm/mm/init.c         |  1 +
 arch/arm64/mm/init.c       |  1 +
 arch/powerpc/kernel/prom.c |  1 +
 drivers/of/fdt.c           | 19 ++++++++++++++-----
 include/linux/of_fdt.h     |  2 ++
 5 files changed, 19 insertions(+), 5 deletions(-)

arch/arm/mm/init.c +1 -0

index 1609b022a72f..0b8657c36fe4 100644
@@ -317,6 +317,7 @@ void __init arm_memblock_init(const struct machine_desc *mdesc)
 	if (mdesc->reserve)
 		mdesc->reserve();
 
+	early_init_fdt_reserve_self();
 	early_init_fdt_scan_reserved_mem();
 
 	/* reserve memory for DMA contiguous allocations */

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

index ae85da6307bb..fa2389b0f7f0 100644
@@ -170,6 +170,7 @@ void __init arm64_memblock_init(void)
 		memblock_reserve(__virt_to_phys(initrd_start), initrd_end - initrd_start);
 #endif
 
+	early_init_fdt_reserve_self();
 	early_init_fdt_scan_reserved_mem();
 
 	/* 4GB maximum for 32-bit only capable devices */

arch/powerpc/kernel/prom.c +1 -0

index b8e15c678960..093ccfb384af 100644
@@ -573,6 +573,7 @@ static void __init early_reserve_mem_dt(void)
 	int len;
 	const __be32 *prop;
 
+	early_init_fdt_reserve_self();
 	early_init_fdt_scan_reserved_mem();
 
 	dt_root = of_get_flat_dt_root();

drivers/of/fdt.c +14 -5

index 3a896c9aeb74..bbb35cdb06e8 100644
@@ -561,11 +561,6 @@ void __init early_init_fdt_scan_reserved_mem(void)
 	if (!initial_boot_params)
 		return;
 
-	/* Reserve the dtb region */
-	early_init_dt_reserve_memory_arch(__pa(initial_boot_params),
-					  fdt_totalsize(initial_boot_params),
-					  0);
-
 	/* Process header /memreserve/ fields */
 	for (n = 0; ; n++) {
 		fdt_get_mem_rsv(initial_boot_params, n, &base, &size);

@@ -579,6 +574,20 @@ void __init early_init_fdt_scan_reserved_mem(void)
 }
 
 /**
+ * early_init_fdt_reserve_self() - reserve the memory used by the FDT blob
+ */
+void __init early_init_fdt_reserve_self(void)
+{
+	if (!initial_boot_params)
+		return;
+
+	/* Reserve the dtb region */
+	early_init_dt_reserve_memory_arch(__pa(initial_boot_params),
+					  fdt_totalsize(initial_boot_params),
+					  0);
+}
+
+/**
  * of_scan_flat_dt - scan flattened tree blob and call callback on each.
  * @it: callback function
  * @data: context data pointer

include/linux/of_fdt.h +2 -0

index 0ff360d5b3b3..6ef6b33238d3 100644
@@ -62,6 +62,7 @@ extern int early_init_dt_scan_chosen(unsigned long node, const char *uname,
 extern int early_init_dt_scan_memory(unsigned long node, const char *uname,
 				     int depth, void *data);
 extern void early_init_fdt_scan_reserved_mem(void);
+extern void early_init_fdt_reserve_self(void);
 extern void early_init_dt_add_memory_arch(u64 base, u64 size);
 extern int early_init_dt_reserve_memory_arch(phys_addr_t base, phys_addr_t size,
 					     bool no_map);

@@ -89,6 +90,7 @@ extern u64 fdt_translate_address(const void *blob, int node_offset);
 extern void of_fdt_limit_memory(int limit);
 #else /* CONFIG_OF_FLATTREE */
 static inline void early_init_fdt_scan_reserved_mem(void) {}
+static inline void early_init_fdt_reserve_self(void) {}
 static inline const char *of_flat_dt_get_machine_name(void) { return NULL; }
 static inline void unflatten_device_tree(void) {}
 static inline void unflatten_and_copy_device_tree(void) {}

Signature · 1 line

1.8.3.2

Documentation/arm64/booting.txt +0 -0

index f3c05b5f9f08..ab5a90adece3 100644

Documentation/arm64/booting.txt +9 -0

 enum fixed_addresses {
 	FIX_HOLE,
+
+	/*
+	 * Reserve 2 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_2M
+	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,

Documentation/arm64/booting.txt +5 -0

+static 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 & ~(FIX_FDT_SIZE - 1);
+

Documentation/arm64/booting.txt +5 -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 2 MB window we just mapped for it.
+	 */

Hi Ard,

This looks good to me (with some minor comments below).

As a heads-up, this clashes with other changes in the arm64 for-next/core branch due to some unrelated changes in head.S and setup.c.

Quoted 8 lines

Documentation/arm64/booting.txt +0 -0

index f3c05b5f9f08..ab5a90adece3 100644
@@ -45,11 +45,13 @@ sees fit.)
 
 Requirement: MANDATORY
 

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

Quoted 7 lines

Documentation/arm64/booting.txt +3 -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 cross a 2-megabyte boundary. This is to allow the kernel to map the
+blob using a single section mapping in the fixmap region.

Please drop the mention of the fixmap. If we ever move this out of the fixmap I don't want to have to update booting.txt again, and it shouldn't matter to bootloader authors either way.

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

Quoted 3 lines

Documentation/arm64/booting.txt +3 -0

+NOTE: versions prior to v4.1 require, in addition to the requirements
+listed above, that the dtb be placed above the kernel Image inside the
+same naturally aligned 512 MB region.

Minor nit: This would read a little better if we just said "versions prior to v4.1 also require that ...", dropping "in addition to the requirements listed above".

Quoted 14 lines

Documentation/arm64/booting.txt +9 -0

 enum fixed_addresses {
      FIX_HOLE,
+
+     /*
+      * Reserve 2 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_2M
+     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,

[...]

Quoted 5 lines

Documentation/arm64/booting.txt +5 -0

+static 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 & ~(FIX_FDT_SIZE - 1);
+

Documentation/arm64/booting.txt

Quoted 10 lines

Documentation/arm64/booting.txt +10 -0

+     /*
+      * Make sure that the FDT region can be mapped without the need to
+      * allocate additional translation table pages, so that it is safe
+      * to call create_pgd_mapping() this early.
+      * 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.
+      */
+     BUILD_BUG_ON(dt_virt_base & (SZ_2M - 1));

s/SZ_2M/FIX_FDT_SIZE/

Perhaps we should also have:

BUILD_BUG_ON_NOT_POWER_OF_2(FIX_FDT_SIZE);

... given it's necessary for the address masking we do here.

[...]

Quoted 5 lines

Documentation/arm64/booting.txt +5 -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 2 MB window we just mapped for it.
+      */

Documentation/arm64/booting.txt

Quoted 10 lines

Documentation/arm64/booting.txt +8 -1

+     if (!dt_virt ||
+         fdt_check_header(dt_virt) != 0 ||
+         (dt_phys & (SZ_2M - 1)) + fdt_totalsize(dt_virt) > SZ_2M ||
+         !early_init_dt_scan(dt_virt)) {
+             pr_crit("\n"
+                     "Error: invalid device tree blob at physical address %pa (virtual address 0x%p)\n"
+                     "The dtb must be 8-byte aligned and must not cross a 2 MB alignment boundary\n"
                      "\nPlease check your bootloader.\n",
-                     dt_phys, phys_to_virt(dt_phys));
+                     &dt_phys, dt_virt);

Nit: drop the leading '\n' on the last line. There's no need for it.

I've tested this on my Juno (atop of arm64 for-next/core), and all seems well, so with those points addressed:

Reviewed-by Mark Rutland <[email protected]>

Tested-by Mark Rutland <[email protected]>

Quoted 8 lines

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

Quoted 3 lines

Documentation/arm64/booting.txt +3 -0

+NOTE: versions prior to v4.1 require, in addition to the requirements
+listed above, that the dtb be placed above the kernel Image inside the
+same naturally aligned 512 MB region.

Minor nit: This would read a little better if we just said "versions prior to v4.1 also require that ...", dropping "in addition to the requirements listed above".

OK

Actually, I realized that this is incorrect anyway: it is not the same naturally aligned 512 MB region, it is actually 'within 512 MB of the start of the kernel Image' since that itself is naturally aligned to 512 MB in the virtual address space.

Quoted 18 lines

Quoted 5 lines

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

+static 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 & ~(FIX_FDT_SIZE - 1);
+

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

Quoted 10 lines

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

+     /*
+      * Make sure that the FDT region can be mapped without the need to
+      * allocate additional translation table pages, so that it is safe
+      * to call create_pgd_mapping() this early.
+      * 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.
+      */
+     BUILD_BUG_ON(dt_virt_base & (SZ_2M - 1));

s/SZ_2M/FIX_FDT_SIZE/

Quoted 8 lines

Perhaps we should also have:

BUILD_BUG_ON_NOT_POWER_OF_2(FIX_FDT_SIZE);

... given it's necessary for the address masking we do here.

[...]

I was going to suggest to use SZ_2M for the masking, but map a 4 MB window. That way, we can relax the requirement from "should not cross a 2 MB boundary' to 'should not exceed 2 MB in size', which is arguable easier to adhere to, since the latter is a property of the DTB itself, whereas the former is a property of whatever your malloc() gives you. That means the mask would remain SZ_2M, and the size would become SZ_4M

Quoted 5 lines

I've tested this on my Juno (atop of arm64 for-next/core), and all seems well, so with those points addressed:

Reviewed-by Mark Rutland <[email protected]>

Tested-by Mark Rutland <[email protected]>

Thanks, but this code will likely look quite different if we are going to cater for moving the kernel out of the linear range, so by then I probably won't be able to retain these tags.

Quoted 9 lines

Quoted 8 lines

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

Quoted 3 lines

Documentation/arm64/booting.txt +3 -0

+NOTE: versions prior to v4.1 require, in addition to the requirements
+listed above, that the dtb be placed above the kernel Image inside the
+same naturally aligned 512 MB region.

Minor nit: This would read a little better if we just said "versions prior to v4.1 also require that ...", dropping "in addition to the requirements listed above".

Quoted 6 lines

OK

Actually, I realized that this is incorrect anyway: it is not the same naturally aligned 512 MB region, it is actually 'within 512 MB of the start of the kernel Image' since that itself is naturally aligned to 512 MB in the virtual address space.

Surely starting at TEXT_OFFSET bytes below the kernel image? ;)

Quoted 27 lines

Quoted 18 lines

Quoted 5 lines

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

+static 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 & ~(FIX_FDT_SIZE - 1);
+

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

Quoted 10 lines

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

+     /*
+      * Make sure that the FDT region can be mapped without the need to
+      * allocate additional translation table pages, so that it is safe
+      * to call create_pgd_mapping() this early.
+      * 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.
+      */
+     BUILD_BUG_ON(dt_virt_base & (SZ_2M - 1));

s/SZ_2M/FIX_FDT_SIZE/

Quoted 8 lines

Perhaps we should also have:

BUILD_BUG_ON_NOT_POWER_OF_2(FIX_FDT_SIZE);

... given it's necessary for the address masking we do here.

[...]

Quoted 7 lines

I was going to suggest to use SZ_2M for the masking, but map a 4 MB window. That way, we can relax the requirement from "should not cross a 2 MB boundary' to 'should not exceed 2 MB in size', which is arguable easier to adhere to, since the latter is a property of the DTB itself, whereas the former is a property of whatever your malloc() gives you. That means the mask would remain SZ_2M, and the size would become SZ_4M

That sounds good so long as it doesn't get in the way of increasing FIX_FDT_SIZE in future.

Quoted 9 lines

Quoted 5 lines

I've tested this on my Juno (atop of arm64 for-next/core), and all seems well, so with those points addressed:

Reviewed-by Mark Rutland <[email protected]>

Tested-by Mark Rutland <[email protected]>

Thanks, but this code will likely look quite different if we are going to cater for moving the kernel out of the linear range, so by then I probably won't be able to retain these tags.

Ok. I guess you'll merge the two series together?

Quoted 16 lines · Mark Rutland

Quoted 15 lines

Quoted 8 lines

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

Quoted 3 lines

Documentation/arm64/booting.txt +3 -0

+NOTE: versions prior to v4.1 require, in addition to the requirements
+listed above, that the dtb be placed above the kernel Image inside the
+same naturally aligned 512 MB region.

Minor nit: This would read a little better if we just said "versions prior to v4.1 also require that ...", dropping "in addition to the requirements listed above".

OK

Actually, I realized that this is incorrect anyway: it is not the same naturally aligned 512 MB region, it is actually 'within 512 MB of the start of the kernel Image' since that itself is naturally aligned to 512 MB in the virtual address space.

Quoted 2 lines

Surely starting at TEXT_OFFSET bytes below the kernel image? ;)

Only if you find it acceptable that people start putting their FDT in the TEXT_OFFSET region ... :-)

Quoted 8 lines

Quoted 7 lines

I was going to suggest to use SZ_2M for the masking, but map a 4 MB window. That way, we can relax the requirement from "should not cross a 2 MB boundary' to 'should not exceed 2 MB in size', which is arguable easier to adhere to, since the latter is a property of the DTB itself, whereas the former is a property of whatever your malloc() gives you. That means the mask would remain SZ_2M, and the size would become SZ_4M

Quoted 3 lines

That sounds good so long as it doesn't get in the way of increasing FIX_FDT_SIZE in future.

No, it shouldn't. And it actually makes it more obvious when we do increase it later which values should be left alone (SZ_2M) and which values should be increased (the higher ones)

Yes. I am currently looking into a way to make early_fixmap_init() not rely on __va(), and if that is easily doable, I will propose something for the FDT which is quite close to the latest submitted version. But otherwise, it may be a bit more complicated, and the FDT mapping needs more work.

The early init code maps the kernel image using statically allocated page tables. This means that we can only allow Image to be placed such that we can map its entire static footprint using a single table entry at all but the lowest level. So update the documentation to reflect that the Image should not cross a 512 MB boundary, which ensures the above on both 4k and 64k pages kernels.

Reviewed-by Mark Rutland <[email protected]>

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

---
 Documentation/arm64/booting.txt | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Documentation/arm64/booting.txt +3 -2

index ab5a90adece3..5949bdbe7aac 100644
@@ -115,8 +115,9 @@ 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.
+The physical memory region consisting of image_size bytes counting from
+the start of the image must be free for use by the kernel, and must not
+cross a 512 MB physical alignment boundary.
 
 Any memory described to the kernel (even that below the 2MB aligned base
 address) which is not marked as reserved from the kernel e.g. with a

Signature · 1 line

1.8.3.2

Hi Ard,

I take it that this (and patch 4) are superseded by the single-page idmap approach, and when this is next posted you'll fold the series?

Quoted 34 lines

The early init code maps the kernel image using statically allocated page tables. This means that we can only allow Image to be placed such that we can map its entire static footprint using a single table entry at all but the lowest level. So update the documentation to reflect that the Image should not cross a 512 MB boundary, which ensures the above on both 4k and 64k pages kernels.

Reviewed-by Mark Rutland <[email protected]>

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

---
 Documentation/arm64/booting.txt | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Documentation/arm64/booting.txt +3 -3

index ab5a90adece3..5949bdbe7aac 100644
@@ -115,8 +115,9 @@ 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.
+The physical memory region consisting of image_size bytes counting from
+the start of the image must be free for use by the kernel, and must not
+cross a 512 MB physical alignment boundary.
 
 Any memory described to the kernel (even that below the 2MB aligned base
 address) which is not marked as reserved from the kernel e.g. with a
--

1.8.3.2