[PATCH v3 0/5] arm64: update/clarify/relax Image and FDT placement rules

[PATCH v3 0/5] arm64: update/clarify/relax Image and FDT placement rules

[Ard Biesheuvel]

This series came about after Mark Rutland brought up the fact that the current
FDT placement logic used by the EFI stub is flawed. But actually, it turned out
that the documentation for both the Image and FDT placement was incorrect as
well, or confusing at the very least.

Patch #1 should preferably get some acks from the ARM and ppc maintainers,
although it is basically a partial revert of Rob's patch d1552ce449eb ("of/fdt:
move memreserve and dtb memory reservations into core") which none of them
acked either.

Patches #2, #4 and #5 need acks as well.

So this series does two things:
- It relaxes the FDT placement requirements, by using a fixmap region mapping
  for the FDT which is disjoint from the virtual mapping of the 512 MB or 1 GB
  region (depending on page size) covering the kernel Image.
- It updates the documentation and EFI stub FDT placement logic accordingly.
- It clarifies the Image placement requirements in the documentation, and brings
  the EFI stub Image placement logic in line with it

Changes since v2:
- added Rob's ack to patch #1

Changes since v1:
- patch #1: split off reservation of the FDT binary itself from the memreserve
  processing, since the former assumes the FDT is accessed via the linear
  mapping, which we are about to change
- patch #2: mention the older, stricter FDT placement rules in booting.txt,
  get rid of early_print,
  use correct format specifier for phys_addr_t,
  use R/O mapping for FDT,
- patches #3 .. #5: add R-b, minor style and grammar tweaks

Ard Biesheuvel (5):
  of/fdt: split off FDT self reservation from memreserve processing
  arm64: use fixmap region for permanent FDT mapping
  arm64: Documentation: clarify Image placement in physical RAM
  arm64/efi: ensure that Image does not cross a 512 MB boundary
  arm64/efi: adapt to relaxed FDT placement requirements

 Documentation/arm64/booting.txt         | 15 ++++---
 arch/arm/mm/init.c                      |  1 +
 arch/arm64/include/asm/efi.h            |  8 ++--
 arch/arm64/include/asm/fixmap.h         |  9 +++++
 arch/arm64/kernel/Makefile              |  1 +
 arch/arm64/kernel/efi-stub.c            | 41 +++++++++++++++----
 arch/arm64/kernel/head.S                | 38 +----------------
 arch/arm64/kernel/setup.c               | 72 ++++++++++++++++++++++++---------
 arch/powerpc/kernel/prom.c              |  1 +
 drivers/firmware/efi/libstub/arm-stub.c |  5 +--
 drivers/firmware/efi/libstub/efistub.h  |  1 -
 drivers/firmware/efi/libstub/fdt.c      | 10 ++---
 drivers/of/fdt.c                        | 19 ++++++---
 include/linux/of_fdt.h                  |  2 +
 14 files changed, 131 insertions(+), 92 deletions(-)

-- 
1.8.3.2

[PATCH v3 1/5] of/fdt: split off FDT self reservation from memreserve processing

[Ard Biesheuvel]

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 <linux@arm.linux.org.uk>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 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(-)

diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
index 1609b022a72f..0b8657c36fe4 100644
--- a/arch/arm/mm/init.c
+++ b/arch/arm/mm/init.c
@@ -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 */
diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index ae85da6307bb..fa2389b0f7f0 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -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 */
diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
index b8e15c678960..093ccfb384af 100644
--- a/arch/powerpc/kernel/prom.c
+++ b/arch/powerpc/kernel/prom.c
@@ -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();
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 3a896c9aeb74..bbb35cdb06e8 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -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
diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
index 0ff360d5b3b3..6ef6b33238d3 100644
--- a/include/linux/of_fdt.h
+++ b/include/linux/of_fdt.h
@@ -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) {}
-- 
1.8.3.2

[PATCH v3 2/5] arm64: use fixmap region for permanent FDT mapping

[Ard Biesheuvel]

Currently, the FDT blob needs to be in the same naturally aligned
512 MB region as the kernel, so that it can be mapped into the
kernel virtual memory space very early on using a minimal set of
statically allocated translation tables.

Now that we have early fixmap support, we can relax this restriction,
by moving the permanent FDT mapping to the fixmap region instead.
This way, the FDT blob may be anywhere in memory.

This also moves the vetting of the FDT to setup.c, since the early
init code in head.S does not handle mapping of the FDT anymore.
At the same time, fix up some comments in head.S that have gone stale.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 Documentation/arm64/booting.txt | 10 +++---
 arch/arm64/include/asm/fixmap.h |  9 ++++++
 arch/arm64/kernel/Makefile      |  1 +
 arch/arm64/kernel/head.S        | 38 +---------------------
 arch/arm64/kernel/setup.c       | 72 +++++++++++++++++++++++++++++------------
 arch/arm64/mm/init.c            |  1 -
 6 files changed, 69 insertions(+), 62 deletions(-)

diff --git a/Documentation/arm64/booting.txt b/Documentation/arm64/booting.txt
index f3c05b5f9f08..ab5a90adece3 100644
--- a/Documentation/arm64/booting.txt
+++ b/Documentation/arm64/booting.txt
@@ -45,11 +45,13 @@ sees fit.)
 
 Requirement: MANDATORY
 
-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.
 
+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.
 
 3. Decompress the kernel image
 ------------------------------
diff --git a/arch/arm64/include/asm/fixmap.h b/arch/arm64/include/asm/fixmap.h
index 926495686554..6e34347e80e3 100644
--- a/arch/arm64/include/asm/fixmap.h
+++ b/arch/arm64/include/asm/fixmap.h
@@ -32,6 +32,15 @@
  */
 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,
diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
index 5ee07eee80c2..e60885766936 100644
--- a/arch/arm64/kernel/Makefile
+++ b/arch/arm64/kernel/Makefile
@@ -6,6 +6,7 @@ CPPFLAGS_vmlinux.lds	:= -DTEXT_OFFSET=$(TEXT_OFFSET)
 AFLAGS_head.o		:= -DTEXT_OFFSET=$(TEXT_OFFSET)
 CFLAGS_efi-stub.o 	:= -DTEXT_OFFSET=$(TEXT_OFFSET)
 CFLAGS_armv8_deprecated.o := -I$(src)
+CFLAGS_setup.o		:= -I$(srctree)/scripts/dtc/libfdt/
 
 CFLAGS_REMOVE_ftrace.o = -pg
 CFLAGS_REMOVE_insn.o = -pg
diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
index d17649d39392..c849cfb157b3 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S
@@ -255,7 +255,6 @@ ENTRY(stext)
 	cbnz	x23, 1f				// invalid processor (x23=0)?
 	b	__error_p
 1:
-	bl	__vet_fdt
 	bl	__create_page_tables		// x25=TTBR0, x26=TTBR1
 	/*
 	 * The following calls CPU specific code in a position independent
@@ -274,24 +273,6 @@ ENTRY(stext)
 ENDPROC(stext)
 
 /*
- * Determine validity of the x21 FDT pointer.
- * The dtb must be 8-byte aligned and live in the first 512M of memory.
- */
-__vet_fdt:
-	tst	x21, #0x7
-	b.ne	1f
-	cmp	x21, x24
-	b.lt	1f
-	mov	x0, #(1 << 29)
-	add	x0, x0, x24
-	cmp	x21, x0
-	b.ge	1f
-	ret
-1:
-	mov	x21, #0
-	ret
-ENDPROC(__vet_fdt)
-/*
  * Macro to create a table entry to the next page.
  *
  *	tbl:	page table address
@@ -352,8 +333,7 @@ ENDPROC(__vet_fdt)
  * required to get the kernel running. The following sections are required:
  *   - identity mapping to enable the MMU (low address, TTBR0)
  *   - first few MB of the kernel linear mapping to jump to once the MMU has
- *     been enabled, including the FDT blob (TTBR1)
- *   - pgd entry for fixed mappings (TTBR1)
+ *     been enabled
  */
 __create_page_tables:
 	pgtbl	x25, x26, x28			// idmap_pg_dir and swapper_pg_dir addresses
@@ -404,22 +384,6 @@ __create_page_tables:
 	create_block_map x0, x7, x3, x5, x6
 
 	/*
-	 * Map the FDT blob (maximum 2MB; must be within 512MB of
-	 * PHYS_OFFSET).
-	 */
-	mov	x3, x21				// FDT phys address
-	and	x3, x3, #~((1 << 21) - 1)	// 2MB aligned
-	mov	x6, #PAGE_OFFSET
-	sub	x5, x3, x24			// subtract PHYS_OFFSET
-	tst	x5, #~((1 << 29) - 1)		// within 512MB?
-	csel	x21, xzr, x21, ne		// zero the FDT pointer
-	b.ne	1f
-	add	x5, x5, x6			// __va(FDT blob)
-	add	x6, x5, #1 << 21		// 2MB for the FDT blob
-	sub	x6, x6, #1			// inclusive range
-	create_block_map x0, x7, x3, x5, x6
-1:
-	/*
 	 * Since the page tables have been populated with non-cacheable
 	 * accesses (MMU disabled), invalidate the idmap and swapper page
 	 * tables again to remove any speculatively loaded cache lines.
diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
index 14808947bf46..ef21f6e172f8 100644
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c
@@ -45,6 +45,7 @@
 #include <linux/of_platform.h>
 #include <linux/efi.h>
 #include <linux/personality.h>
+#include <linux/libfdt.h>
 
 #include <asm/fixmap.h>
 #include <asm/cpu.h>
@@ -108,18 +109,6 @@ static struct resource mem_res[] = {
 #define kernel_code mem_res[0]
 #define kernel_data mem_res[1]
 
-void __init early_print(const char *str, ...)
-{
-	char buf[256];
-	va_list ap;
-
-	va_start(ap, str);
-	vsnprintf(buf, sizeof(buf), str, ap);
-	va_end(ap);
-
-	printk("%s", buf);
-}
-
 void __init smp_setup_processor_id(void)
 {
 	u64 mpidr = read_cpuid_mpidr() & MPIDR_HWID_BITMASK;
@@ -332,19 +321,62 @@ static void __init setup_processor(void)
 #endif
 }
 
+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);
+
+	/*
+	 * 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));
+
+	if (IS_ENABLED(CONFIG_ARM64_64K_PAGES))
+		BUILD_BUG_ON(__fix_to_virt(FIX_FDT_END) >> PMD_SHIFT !=
+			     __fix_to_virt(FIX_BTMAP_BEGIN) >> PMD_SHIFT);
+	else
+		BUILD_BUG_ON(__fix_to_virt(FIX_FDT_END) >> PUD_SHIFT !=
+			     __fix_to_virt(FIX_BTMAP_BEGIN) >> PUD_SHIFT);
+
+	create_pgd_mapping(&init_mm, dt_phys_base, dt_virt_base, FIX_FDT_SIZE,
+			   PAGE_KERNEL | PTE_RDONLY);
+
+	return (void *)(dt_virt_base + dt_phys - dt_phys_base);
+}
+
 static void __init setup_machine_fdt(phys_addr_t dt_phys)
 {
-	if (!dt_phys || !early_init_dt_scan(phys_to_virt(dt_phys))) {
-		early_print("\n"
-			"Error: invalid device tree blob at physical address 0x%p (virtual address 0x%p)\n"
-			"The dtb must be 8-byte aligned and passed in the first 512MB of memory\n"
+	void *dt_virt = NULL;
+
+	if (dt_phys && (dt_phys & 7) == 0)
+		dt_virt = fixmap_remap_fdt(dt_phys);
+
+	/*
+	 * 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.
+	 */
+	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);
 
 		while (true)
 			cpu_relax();
 	}
 
+	memblock_reserve(dt_phys, fdt_totalsize(dt_virt));
 	dump_stack_set_arch_desc("%s (DT)", of_flat_dt_get_machine_name());
 }
 
@@ -382,6 +414,9 @@ void __init setup_arch(char **cmdline_p)
 {
 	setup_processor();
 
+	early_fixmap_init();
+	early_ioremap_init();
+
 	setup_machine_fdt(__fdt_pointer);
 
 	init_mm.start_code = (unsigned long) _text;
@@ -391,9 +426,6 @@ void __init setup_arch(char **cmdline_p)
 
 	*cmdline_p = boot_command_line;
 
-	early_fixmap_init();
-	early_ioremap_init();
-
 	parse_early_param();
 
 	/*
diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index fa2389b0f7f0..ae85da6307bb 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -170,7 +170,6 @@ 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 */
-- 
1.8.3.2

[PATCH v3 3/5] arm64: Documentation: clarify Image placement in physical RAM

[Ard Biesheuvel]

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 <mark.rutland@arm.com>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 Documentation/arm64/booting.txt | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/Documentation/arm64/booting.txt b/Documentation/arm64/booting.txt
index ab5a90adece3..5949bdbe7aac 100644
--- a/Documentation/arm64/booting.txt
+++ b/Documentation/arm64/booting.txt
@@ -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

[PATCH v3 4/5] arm64/efi: ensure that Image does not cross a 512 MB boundary

[Ard Biesheuvel]

Update the Image placement logic used by the stub to make absolutely
sure that the Image is placed such that the early init code will
always be able to map it. This means the entire static memory footprint
of the Image should be inside the same naturally aligned 512 MB region.

First of all, the preferred offset of dram_base + TEXT_OFFSET is only
suitable if it doesn't result in the Image crossing a 512 MB
alignment boundary, which could be the case if dram_base itself is
close to the end of a naturally aligned 512 MB region.

Also, when moving the kernel Image, we need to verify that the new
destination region does not cross a 512 MB alignment boundary either.
If that is the case, we retry the allocation with the alignment
chosen such that the resulting region will always be suitable.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/kernel/efi-stub.c | 41 +++++++++++++++++++++++++++++++++--------
 1 file changed, 33 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/kernel/efi-stub.c b/arch/arm64/kernel/efi-stub.c
index f5374065ad53..3b67ca4e2f2e 100644
--- a/arch/arm64/kernel/efi-stub.c
+++ b/arch/arm64/kernel/efi-stub.c
@@ -21,15 +21,40 @@ efi_status_t __init handle_kernel_image(efi_system_table_t *sys_table,
 					unsigned long dram_base,
 					efi_loaded_image_t *image)
 {
+	const unsigned long kernel_size = _edata - _text;
+	const unsigned long kernel_memsize = _end - _text;
+	unsigned long preferred_offset;
 	efi_status_t status;
-	unsigned long kernel_size, kernel_memsize = 0;
-
-	/* Relocate the image, if required. */
-	kernel_size = _edata - _text;
-	if (*image_addr != (dram_base + TEXT_OFFSET)) {
-		kernel_memsize = kernel_size + (_end - _edata);
-		status = efi_low_alloc(sys_table, kernel_memsize + TEXT_OFFSET,
-				       SZ_2M, reserve_addr);
+
+	/*
+	 * The kernel Image should be located as close as possible to the
+	 * base of system RAM, but its static memory footprint must not
+	 * cross a 512 MB alignment boundary.
+	 */
+	preferred_offset = dram_base + TEXT_OFFSET;
+	if ((preferred_offset & (SZ_512M - 1)) + kernel_memsize > SZ_512M)
+		preferred_offset = round_up(dram_base, SZ_512M) + TEXT_OFFSET;
+
+	if (*image_addr != preferred_offset) {
+		const unsigned long alloc_size = kernel_memsize + TEXT_OFFSET;
+
+		status = efi_low_alloc(sys_table, alloc_size, SZ_2M,
+				       reserve_addr);
+
+		/*
+		 * Check whether the new allocation crosses a 512 MB alignment
+		 * boundary. If so, retry with the alignment set to a power of
+		 * two upper bound of the allocation size. That is guaranteed
+		 * to produce a suitable allocation, but may waste more memory.
+		 */
+		if (status == EFI_SUCCESS &&
+		    ((*reserve_addr & (SZ_512M - 1)) + alloc_size) > SZ_512M) {
+			efi_free(sys_table, alloc_size, *reserve_addr);
+
+			status = efi_low_alloc(sys_table, alloc_size,
+					       roundup_pow_of_two(alloc_size),
+					       reserve_addr);
+		}
 		if (status != EFI_SUCCESS) {
 			pr_efi_err(sys_table, "Failed to relocate kernel\n");
 			return status;
-- 
1.8.3.2

[PATCH v3 5/5] arm64/efi: adapt to relaxed FDT placement requirements

[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 a
power of 2 upper bound of the size: this way, we are still guaranteed
not to cross a 2 MB boundary, but will potentially waste less memory.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 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      | 10 +++-------
 4 files changed, 8 insertions(+), 16 deletions(-)

diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
index ef572206f1c3..6f526fcc6b70 100644
--- a/arch/arm64/include/asm/efi.h
+++ b/arch/arm64/include/asm/efi.h
@@ -39,12 +39,10 @@ extern void efi_init(void);
 /* arch specific definitions used by the stub code */
 
 /*
- * 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 cross a 2MiB boundary.
+ * So align to a power of 2 upper bound of the FDT 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(x)	roundup_pow_of_two(x)
 
 #define efi_call_early(f, ...) sys_table_arg->boottime->f(__VA_ARGS__)
 
diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
index dcae482a9a17..f54c76a4fd32 100644
--- a/drivers/firmware/efi/libstub/arm-stub.c
+++ b/drivers/firmware/efi/libstub/arm-stub.c
@@ -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
diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
index 47437b16b186..c8e096094ea9 100644
--- a/drivers/firmware/efi/libstub/efistub.h
+++ b/drivers/firmware/efi/libstub/efistub.h
@@ -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,
diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
index 91da56c4fd54..48218e91cce1 100644
--- a/drivers/firmware/efi/libstub/fdt.c
+++ b/drivers/firmware/efi/libstub/fdt.c
@@ -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,
@@ -223,8 +218,9 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
 	 */
 	new_fdt_size = fdt_size + EFI_PAGE_SIZE;
 	while (1) {
-		status = efi_high_alloc(sys_table, new_fdt_size, EFI_FDT_ALIGN,
-					new_fdt_addr, max_addr);
+		status = efi_high_alloc(sys_table, new_fdt_size,
+					EFI_FDT_ALIGN(new_fdt_size),
+					new_fdt_addr, ULONG_MAX);
 		if (status != EFI_SUCCESS) {
 			pr_efi_err(sys_table, "Unable to allocate memory for new device tree.\n");
 			goto fail;
-- 
1.8.3.2

[PATCH v3 2/5] arm64: use fixmap region for permanent FDT mapping

[Mark Rutland]

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.

> diff --git a/Documentation/arm64/booting.txt b/Documentation/arm64/booting.txt
> index f3c05b5f9f08..ab5a90adece3 100644
> --- a/Documentation/arm64/booting.txt
> +++ b/Documentation/arm64/booting.txt
> @@ -45,11 +45,13 @@ sees fit.)
>  
>  Requirement: MANDATORY
>  
> -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.

> +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".

[...]

>  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,

[...]

> +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);
> +
> +	/*
> +	 * 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.

[...]

> +	/*
> +	 * 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.
> +	 */
> +	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 <mark.rutland@arm.com>
Tested-by: Mark Rutland <mark.rutland@arm.com>

Thanks,
Mark.

[PATCH v3 1/5] of/fdt: split off FDT self reservation from memreserve processing

[Mark Rutland]

On Wed, Mar 18, 2015 at 05:05:04PM +0000, Ard Biesheuvel wrote:
> 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 <linux@arm.linux.org.uk>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Acked-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

FWIW:

Acked-by: Mark Rutland <mark.rutland@arm.com>

Thanks,
Mark.

> ---
>  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(-)
> 
> diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
> index 1609b022a72f..0b8657c36fe4 100644
> --- a/arch/arm/mm/init.c
> +++ b/arch/arm/mm/init.c
> @@ -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 */
> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
> index ae85da6307bb..fa2389b0f7f0 100644
> --- a/arch/arm64/mm/init.c
> +++ b/arch/arm64/mm/init.c
> @@ -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 */
> diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
> index b8e15c678960..093ccfb384af 100644
> --- a/arch/powerpc/kernel/prom.c
> +++ b/arch/powerpc/kernel/prom.c
> @@ -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();
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index 3a896c9aeb74..bbb35cdb06e8 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -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
> diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
> index 0ff360d5b3b3..6ef6b33238d3 100644
> --- a/include/linux/of_fdt.h
> +++ b/include/linux/of_fdt.h
> @@ -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) {}
> -- 
> 1.8.3.2
> 
> 

[PATCH v3 3/5] arm64: Documentation: clarify Image placement in physical RAM

[Mark Rutland]

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?

Thanks,
Mark.

On Wed, Mar 18, 2015 at 05:05:06PM +0000, Ard Biesheuvel wrote:
> 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 <mark.rutland@arm.com>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  Documentation/arm64/booting.txt | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/arm64/booting.txt b/Documentation/arm64/booting.txt
> index ab5a90adece3..5949bdbe7aac 100644
> --- a/Documentation/arm64/booting.txt
> +++ b/Documentation/arm64/booting.txt
> @@ -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
> 
> 

[PATCH v3 2/5] arm64: use fixmap region for permanent FDT mapping

[Ard Biesheuvel]

On 9 April 2015 at 13:49, Mark Rutland <mark.rutland@arm.com> wrote:
> 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.
>
>> diff --git a/Documentation/arm64/booting.txt b/Documentation/arm64/booting.txt
>> index f3c05b5f9f08..ab5a90adece3 100644
>> --- a/Documentation/arm64/booting.txt
>> +++ b/Documentation/arm64/booting.txt
>> @@ -45,11 +45,13 @@ sees fit.)
>>
>>  Requirement: MANDATORY
>>
>> -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.
>

OK.

>> +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.

>>  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,
>
> [...]
>
>> +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);
>> +
>> +     /*
>> +      * 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.
>
> [...]
>

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


>> +     /*
>> +      * 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.
>> +      */
>> +     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.
>

OK

> 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 <mark.rutland@arm.com>
> Tested-by: Mark Rutland <mark.rutland@arm.com>

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.

Cheers,
Ard.

[PATCH v3 3/5] arm64: Documentation: clarify Image placement in physical RAM

[Ard Biesheuvel]

On 9 April 2015 at 13:54, Mark Rutland <mark.rutland@arm.com> wrote:
> 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?
>

indeed.

-- 
Ard.


> On Wed, Mar 18, 2015 at 05:05:06PM +0000, Ard Biesheuvel wrote:
>> 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 <mark.rutland@arm.com>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  Documentation/arm64/booting.txt | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/arm64/booting.txt b/Documentation/arm64/booting.txt
>> index ab5a90adece3..5949bdbe7aac 100644
>> --- a/Documentation/arm64/booting.txt
>> +++ b/Documentation/arm64/booting.txt
>> @@ -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
>>
>>

[PATCH v3 2/5] arm64: use fixmap region for permanent FDT mapping

[Mark Rutland]

> >> +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.

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

> >> +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);
> >> +
> >> +     /*
> >> +      * 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.
> >
> > [...]
> >
> 
> 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.

> > 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 <mark.rutland@arm.com>
> > Tested-by: Mark Rutland <mark.rutland@arm.com>
> 
> 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?

Thanks,
Mark.

[PATCH v3 2/5] arm64: use fixmap region for permanent FDT mapping

[Ard Biesheuvel]

On 9 April 2015 at 15:12, Mark Rutland <mark.rutland@arm.com> wrote:
>> >> +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.
>
> 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 ... :-)

>> >> +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);
>> >> +
>> >> +     /*
>> >> +      * 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.
>> >
>> > [...]
>> >
>>
>> 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.
>

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)

>> > 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 <mark.rutland@arm.com>
>> > Tested-by: Mark Rutland <mark.rutland@arm.com>
>>
>> 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?
>

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.

-- 
Ard.

[PATCH v3 2/5] arm64: use fixmap region for permanent FDT mapping

[Mark Rutland]

On Thu, Apr 09, 2015 at 02:16:23PM +0100, Ard Biesheuvel wrote:
> On 9 April 2015 at 15:12, Mark Rutland <mark.rutland@arm.com> wrote:
> >> >> +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.
> >
> > 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 ... :-)

Unfortunately people are already doing such things (which is one reason
I moved the page tables out of the region).

It also means that the region of TEXT_OFFSET bytes below 512M from the
start of the image is unusable...

[...]

> >> 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.
> >
> 
> 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)

Sounds good to me then.

> 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.

Ok. I'll keep an eye out then.

Mark.