[PATCH v2 1/2] efi: arm-stub: Correct FDT and initrd allocation rules for arm64

[PATCH v2 1/2] efi: arm-stub: Correct FDT and initrd allocation rules for arm64

[Ard Biesheuvel]

On arm64, we have made some changes over the past year to the way the
kernel itself is allocated and to how it deals with the initrd and FDT.
This patch brings the allocation logic in the EFI stub in line with that,
which is necessary because the introduction of KASLR has created the
possibility for the initrd to be allocated in a place where the kernel
may not be able to map it. (This is mostly a theoretical scenario, since
it only affects systems where the physical memory footprint exceeds the
size of the linear mapping.)

Since we know the kernel itself will be covered by the linear mapping,
choose a suitably sized window (i.e., based on the size of the linear
region) covering the kernel when allocating memory for the initrd.

The FDT may be anywhere in memory on arm64 now that we map it via the
fixmap, so we can lift the address restriction there completely.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm/include/asm/efi.h              | 14 +++++++++++++-
 arch/arm64/include/asm/efi.h            | 18 +++++++++++++++++-
 drivers/firmware/efi/libstub/arm-stub.c |  7 ++++---
 3 files changed, 34 insertions(+), 5 deletions(-)

diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h
index 0b06f5341b45..2de0195dfd1e 100644
--- a/arch/arm/include/asm/efi.h
+++ b/arch/arm/include/asm/efi.h
@@ -84,6 +84,18 @@ static inline void efifb_setup_from_dmi(struct screen_info *si, const char *opt)
  */
 #define ZIMAGE_OFFSET_LIMIT	SZ_128M
 #define MIN_ZIMAGE_OFFSET	MAX_UNCOMP_KERNEL_SIZE
-#define MAX_FDT_OFFSET		ZIMAGE_OFFSET_LIMIT
+
+/* on ARM, the FDT should be located in the first 128 MB of RAM */
+static inline unsigned long efi_get_max_fdt_addr(unsigned long dram_base)
+{
+	return dram_base + ZIMAGE_OFFSET_LIMIT;
+}
+
+/* on ARM, the initrd should be loaded in a lowmem region */
+static inline unsigned long efi_get_max_initrd_addr(unsigned long dram_base,
+						    unsigned long image_addr)
+{
+	return dram_base + SZ_512M;
+}
 
 #endif /* _ASM_ARM_EFI_H */
diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
index 0b6b1633017f..342e90d6d204 100644
--- a/arch/arm64/include/asm/efi.h
+++ b/arch/arm64/include/asm/efi.h
@@ -46,7 +46,23 @@ int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md);
  * 2MiB so we know it won't cross a 2MiB boundary.
  */
 #define EFI_FDT_ALIGN	SZ_2M   /* used by allocate_new_fdt_and_exit_boot() */
-#define MAX_FDT_OFFSET	SZ_512M
+
+/* on arm64, the FDT may be located anywhere in system RAM */
+static inline unsigned long efi_get_max_fdt_addr(unsigned long dram_base)
+{
+	return ULONG_MAX;
+}
+
+/*
+ * On arm64, we have to ensure that the initrd ends up in the linear region,
+ * which is a 1 GB aligned region of size '1UL << (VA_BITS - 1)' that is
+ * guaranteed to cover the kernel Image.
+ */
+static inline unsigned long efi_get_max_initrd_addr(unsigned long dram_base,
+						    unsigned long image_addr)
+{
+	return (image_addr & ~(SZ_1G - 1UL)) + (1UL << (VA_BITS - 1));
+}
 
 #define efi_call_early(f, ...)		sys_table_arg->boottime->f(__VA_ARGS__)
 #define __efi_call_early(f, ...)	f(__VA_ARGS__)
diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
index b4f7d78f9e8b..557281fe375f 100644
--- a/drivers/firmware/efi/libstub/arm-stub.c
+++ b/drivers/firmware/efi/libstub/arm-stub.c
@@ -333,8 +333,9 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table,
 	if (!fdt_addr)
 		pr_efi(sys_table, "Generating empty DTB\n");
 
-	status = handle_cmdline_files(sys_table, image, cmdline_ptr,
-				      "initrd=", dram_base + SZ_512M,
+	status = handle_cmdline_files(sys_table, image, cmdline_ptr, "initrd=",
+				      efi_get_max_initrd_addr(dram_base,
+							      *image_addr),
 				      (unsigned long *)&initrd_addr,
 				      (unsigned long *)&initrd_size);
 	if (status != EFI_SUCCESS)
@@ -344,7 +345,7 @@ 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,
+				&new_fdt_addr, efi_get_max_fdt_addr(dram_base),
 				initrd_addr, initrd_size, cmdline_ptr,
 				fdt_addr, fdt_size);
 
-- 
2.7.4

[PATCH v2 1/2] efi: arm-stub: Correct FDT and initrd allocation rules for arm64

[Ard Biesheuvel]

On arm64, we have made some changes over the past year to the way the
kernel itself is allocated and to how it deals with the initrd and FDT.
This patch brings the allocation logic in the EFI stub in line with that,
which is necessary because the introduction of KASLR has created the
possibility for the initrd to be allocated in a place where the kernel
may not be able to map it. (This is mostly a theoretical scenario, since
it only affects systems where the physical memory footprint exceeds the
size of the linear mapping.)

Since we know the kernel itself will be covered by the linear mapping,
choose a suitably sized window (i.e., based on the size of the linear
region) covering the kernel when allocating memory for the initrd.

The FDT may be anywhere in memory on arm64 now that we map it via the
fixmap, so we can lift the address restriction there completely.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm/include/asm/efi.h              | 14 +++++++++++++-
 arch/arm64/include/asm/efi.h            | 18 +++++++++++++++++-
 drivers/firmware/efi/libstub/arm-stub.c |  7 ++++---
 3 files changed, 34 insertions(+), 5 deletions(-)

diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h
index 0b06f5341b45..2de0195dfd1e 100644
--- a/arch/arm/include/asm/efi.h
+++ b/arch/arm/include/asm/efi.h
@@ -84,6 +84,18 @@ static inline void efifb_setup_from_dmi(struct screen_info *si, const char *opt)
  */
 #define ZIMAGE_OFFSET_LIMIT	SZ_128M
 #define MIN_ZIMAGE_OFFSET	MAX_UNCOMP_KERNEL_SIZE
-#define MAX_FDT_OFFSET		ZIMAGE_OFFSET_LIMIT
+
+/* on ARM, the FDT should be located in the first 128 MB of RAM */
+static inline unsigned long efi_get_max_fdt_addr(unsigned long dram_base)
+{
+	return dram_base + ZIMAGE_OFFSET_LIMIT;
+}
+
+/* on ARM, the initrd should be loaded in a lowmem region */
+static inline unsigned long efi_get_max_initrd_addr(unsigned long dram_base,
+						    unsigned long image_addr)
+{
+	return dram_base + SZ_512M;
+}
 
 #endif /* _ASM_ARM_EFI_H */
diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
index 0b6b1633017f..342e90d6d204 100644
--- a/arch/arm64/include/asm/efi.h
+++ b/arch/arm64/include/asm/efi.h
@@ -46,7 +46,23 @@ int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md);
  * 2MiB so we know it won't cross a 2MiB boundary.
  */
 #define EFI_FDT_ALIGN	SZ_2M   /* used by allocate_new_fdt_and_exit_boot() */
-#define MAX_FDT_OFFSET	SZ_512M
+
+/* on arm64, the FDT may be located anywhere in system RAM */
+static inline unsigned long efi_get_max_fdt_addr(unsigned long dram_base)
+{
+	return ULONG_MAX;
+}
+
+/*
+ * On arm64, we have to ensure that the initrd ends up in the linear region,
+ * which is a 1 GB aligned region of size '1UL << (VA_BITS - 1)' that is
+ * guaranteed to cover the kernel Image.
+ */
+static inline unsigned long efi_get_max_initrd_addr(unsigned long dram_base,
+						    unsigned long image_addr)
+{
+	return (image_addr & ~(SZ_1G - 1UL)) + (1UL << (VA_BITS - 1));
+}
 
 #define efi_call_early(f, ...)		sys_table_arg->boottime->f(__VA_ARGS__)
 #define __efi_call_early(f, ...)	f(__VA_ARGS__)
diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
index b4f7d78f9e8b..557281fe375f 100644
--- a/drivers/firmware/efi/libstub/arm-stub.c
+++ b/drivers/firmware/efi/libstub/arm-stub.c
@@ -333,8 +333,9 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table,
 	if (!fdt_addr)
 		pr_efi(sys_table, "Generating empty DTB\n");
 
-	status = handle_cmdline_files(sys_table, image, cmdline_ptr,
-				      "initrd=", dram_base + SZ_512M,
+	status = handle_cmdline_files(sys_table, image, cmdline_ptr, "initrd=",
+				      efi_get_max_initrd_addr(dram_base,
+							      *image_addr),
 				      (unsigned long *)&initrd_addr,
 				      (unsigned long *)&initrd_size);
 	if (status != EFI_SUCCESS)
@@ -344,7 +345,7 @@ 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,
+				&new_fdt_addr, efi_get_max_fdt_addr(dram_base),
 				initrd_addr, initrd_size, cmdline_ptr,
 				fdt_addr, fdt_size);
 
-- 
2.7.4

[PATCH v2 2/2] efi: arm-stub: Round up FDT allocation to mapping size

[Ard Biesheuvel]

The FDT is mapped via a fixmap entry that is at least 2 MB in size and
2 MB aligned on 4 KB page size kernels.

On UEFI systems, the FDT allocation may share this 2 MB block with a
reserved region, or another memory region that we should never map,
unless we account for this in the size of the allocation (the alignment
is already 2 MB)

So instead of taking guesses at the needed space, simply allocate 2 MB
immediately. The allocation will be recorded as a EFI_LOADER_DATA, and
the kernel only memblock_reserve()'s the actual size of the FDT, so the
unused space will be released to the kernel.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/include/asm/efi.h       |  1 +
 drivers/firmware/efi/libstub/fdt.c | 57 +++++++++-----------
 2 files changed, 25 insertions(+), 33 deletions(-)

diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
index 342e90d6d204..f642565fc1d0 100644
--- a/arch/arm64/include/asm/efi.h
+++ b/arch/arm64/include/asm/efi.h
@@ -1,6 +1,7 @@
 #ifndef _ASM_EFI_H
 #define _ASM_EFI_H
 
+#include <asm/boot.h>
 #include <asm/cpufeature.h>
 #include <asm/io.h>
 #include <asm/mmu_context.h>
diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
index 260c4b4b492e..41f457be64e8 100644
--- a/drivers/firmware/efi/libstub/fdt.c
+++ b/drivers/firmware/efi/libstub/fdt.c
@@ -206,6 +206,10 @@ static efi_status_t exit_boot_func(efi_system_table_t *sys_table_arg,
 	return update_fdt_memmap(p->new_fdt_addr, map);
 }
 
+#ifndef MAX_FDT_SIZE
+#define MAX_FDT_SIZE	SZ_2M
+#endif
+
 /*
  * Allocate memory for a new FDT, then add EFI, commandline, and
  * initrd related fields to the FDT.  This routine increases the
@@ -233,7 +237,6 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
 	u32 desc_ver;
 	unsigned long mmap_key;
 	efi_memory_desc_t *memory_map, *runtime_map;
-	unsigned long new_fdt_size;
 	efi_status_t status;
 	int runtime_entry_count = 0;
 	struct efi_boot_memmap map;
@@ -262,41 +265,29 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
 	       "Exiting boot services and installing virtual address map...\n");
 
 	map.map = &memory_map;
+	status = efi_high_alloc(sys_table, MAX_FDT_SIZE, EFI_FDT_ALIGN,
+				new_fdt_addr, max_addr);
+	if (status != EFI_SUCCESS) {
+		pr_efi_err(sys_table,
+			   "Unable to allocate memory for new device tree.\n");
+		goto fail;
+	}
+
 	/*
-	 * Estimate size of new FDT, and allocate memory for it. We
-	 * will allocate a bigger buffer if this ends up being too
-	 * small, so a rough guess is OK here.
+	 * Now that we have done our final memory allocation (and free)
+	 * we can get the memory map key needed for exit_boot_services().
 	 */
-	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);
-		if (status != EFI_SUCCESS) {
-			pr_efi_err(sys_table, "Unable to allocate memory for new device tree.\n");
-			goto fail;
-		}
-
-		status = update_fdt(sys_table,
-				    (void *)fdt_addr, fdt_size,
-				    (void *)*new_fdt_addr, new_fdt_size,
-				    cmdline_ptr, initrd_addr, initrd_size);
+	status = efi_get_memory_map(sys_table, &map);
+	if (status != EFI_SUCCESS)
+		goto fail_free_new_fdt;
 
-		/* Succeeding the first time is the expected case. */
-		if (status == EFI_SUCCESS)
-			break;
+	status = update_fdt(sys_table, (void *)fdt_addr, fdt_size,
+			    (void *)*new_fdt_addr, MAX_FDT_SIZE, cmdline_ptr,
+			    initrd_addr, initrd_size);
 
-		if (status == EFI_BUFFER_TOO_SMALL) {
-			/*
-			 * We need to allocate more space for the new
-			 * device tree, so free existing buffer that is
-			 * too small.
-			 */
-			efi_free(sys_table, new_fdt_size, *new_fdt_addr);
-			new_fdt_size += EFI_PAGE_SIZE;
-		} else {
-			pr_efi_err(sys_table, "Unable to construct new device tree.\n");
-			goto fail_free_new_fdt;
-		}
+	if (status != EFI_SUCCESS) {
+		pr_efi_err(sys_table, "Unable to construct new device tree.\n");
+		goto fail_free_new_fdt;
 	}
 
 	priv.runtime_map = runtime_map;
@@ -340,7 +331,7 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
 	pr_efi_err(sys_table, "Exit boot services failed.\n");
 
 fail_free_new_fdt:
-	efi_free(sys_table, new_fdt_size, *new_fdt_addr);
+	efi_free(sys_table, MAX_FDT_SIZE, *new_fdt_addr);
 
 fail:
 	sys_table->boottime->free_pool(runtime_map);
-- 
2.7.4

[PATCH v2 2/2] efi: arm-stub: Round up FDT allocation to mapping size

[Ard Biesheuvel]

The FDT is mapped via a fixmap entry that is at least 2 MB in size and
2 MB aligned on 4 KB page size kernels.

On UEFI systems, the FDT allocation may share this 2 MB block with a
reserved region, or another memory region that we should never map,
unless we account for this in the size of the allocation (the alignment
is already 2 MB)

So instead of taking guesses at the needed space, simply allocate 2 MB
immediately. The allocation will be recorded as a EFI_LOADER_DATA, and
the kernel only memblock_reserve()'s the actual size of the FDT, so the
unused space will be released to the kernel.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/include/asm/efi.h       |  1 +
 drivers/firmware/efi/libstub/fdt.c | 57 +++++++++-----------
 2 files changed, 25 insertions(+), 33 deletions(-)

diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
index 342e90d6d204..f642565fc1d0 100644
--- a/arch/arm64/include/asm/efi.h
+++ b/arch/arm64/include/asm/efi.h
@@ -1,6 +1,7 @@
 #ifndef _ASM_EFI_H
 #define _ASM_EFI_H
 
+#include <asm/boot.h>
 #include <asm/cpufeature.h>
 #include <asm/io.h>
 #include <asm/mmu_context.h>
diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
index 260c4b4b492e..41f457be64e8 100644
--- a/drivers/firmware/efi/libstub/fdt.c
+++ b/drivers/firmware/efi/libstub/fdt.c
@@ -206,6 +206,10 @@ static efi_status_t exit_boot_func(efi_system_table_t *sys_table_arg,
 	return update_fdt_memmap(p->new_fdt_addr, map);
 }
 
+#ifndef MAX_FDT_SIZE
+#define MAX_FDT_SIZE	SZ_2M
+#endif
+
 /*
  * Allocate memory for a new FDT, then add EFI, commandline, and
  * initrd related fields to the FDT.  This routine increases the
@@ -233,7 +237,6 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
 	u32 desc_ver;
 	unsigned long mmap_key;
 	efi_memory_desc_t *memory_map, *runtime_map;
-	unsigned long new_fdt_size;
 	efi_status_t status;
 	int runtime_entry_count = 0;
 	struct efi_boot_memmap map;
@@ -262,41 +265,29 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
 	       "Exiting boot services and installing virtual address map...\n");
 
 	map.map = &memory_map;
+	status = efi_high_alloc(sys_table, MAX_FDT_SIZE, EFI_FDT_ALIGN,
+				new_fdt_addr, max_addr);
+	if (status != EFI_SUCCESS) {
+		pr_efi_err(sys_table,
+			   "Unable to allocate memory for new device tree.\n");
+		goto fail;
+	}
+
 	/*
-	 * Estimate size of new FDT, and allocate memory for it. We
-	 * will allocate a bigger buffer if this ends up being too
-	 * small, so a rough guess is OK here.
+	 * Now that we have done our final memory allocation (and free)
+	 * we can get the memory map key needed for exit_boot_services().
 	 */
-	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);
-		if (status != EFI_SUCCESS) {
-			pr_efi_err(sys_table, "Unable to allocate memory for new device tree.\n");
-			goto fail;
-		}
-
-		status = update_fdt(sys_table,
-				    (void *)fdt_addr, fdt_size,
-				    (void *)*new_fdt_addr, new_fdt_size,
-				    cmdline_ptr, initrd_addr, initrd_size);
+	status = efi_get_memory_map(sys_table, &map);
+	if (status != EFI_SUCCESS)
+		goto fail_free_new_fdt;
 
-		/* Succeeding the first time is the expected case. */
-		if (status == EFI_SUCCESS)
-			break;
+	status = update_fdt(sys_table, (void *)fdt_addr, fdt_size,
+			    (void *)*new_fdt_addr, MAX_FDT_SIZE, cmdline_ptr,
+			    initrd_addr, initrd_size);
 
-		if (status == EFI_BUFFER_TOO_SMALL) {
-			/*
-			 * We need to allocate more space for the new
-			 * device tree, so free existing buffer that is
-			 * too small.
-			 */
-			efi_free(sys_table, new_fdt_size, *new_fdt_addr);
-			new_fdt_size += EFI_PAGE_SIZE;
-		} else {
-			pr_efi_err(sys_table, "Unable to construct new device tree.\n");
-			goto fail_free_new_fdt;
-		}
+	if (status != EFI_SUCCESS) {
+		pr_efi_err(sys_table, "Unable to construct new device tree.\n");
+		goto fail_free_new_fdt;
 	}
 
 	priv.runtime_map = runtime_map;
@@ -340,7 +331,7 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
 	pr_efi_err(sys_table, "Exit boot services failed.\n");
 
 fail_free_new_fdt:
-	efi_free(sys_table, new_fdt_size, *new_fdt_addr);
+	efi_free(sys_table, MAX_FDT_SIZE, *new_fdt_addr);
 
 fail:
 	sys_table->boottime->free_pool(runtime_map);
-- 
2.7.4

Re: [PATCH v2 1/2] efi: arm-stub: Correct FDT and initrd allocation rules for arm64

[Ruigrok, Richard]

On 2/9/2017 2:42 PM, Ard Biesheuvel wrote:
> On arm64, we have made some changes over the past year to the way the
> kernel itself is allocated and to how it deals with the initrd and FDT.
> This patch brings the allocation logic in the EFI stub in line with that,
> which is necessary because the introduction of KASLR has created the
> possibility for the initrd to be allocated in a place where the kernel
> may not be able to map it. (This is mostly a theoretical scenario, since
> it only affects systems where the physical memory footprint exceeds the
> size of the linear mapping.)
>
> Since we know the kernel itself will be covered by the linear mapping,
> choose a suitably sized window (i.e., based on the size of the linear
> region) covering the kernel when allocating memory for the initrd.
>
> The FDT may be anywhere in memory on arm64 now that we map it via the
> fixmap, so we can lift the address restriction there completely.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  arch/arm/include/asm/efi.h              | 14 +++++++++++++-
>  arch/arm64/include/asm/efi.h            | 18 +++++++++++++++++-
>  drivers/firmware/efi/libstub/arm-stub.c |  7 ++++---
>  3 files changed, 34 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h
> index 0b06f5341b45..2de0195dfd1e 100644
> --- a/arch/arm/include/asm/efi.h
> +++ b/arch/arm/include/asm/efi.h
> @@ -84,6 +84,18 @@ static inline void efifb_setup_from_dmi(struct screen_info *si, const char *opt)
>   */
>  #define ZIMAGE_OFFSET_LIMIT	SZ_128M
>  #define MIN_ZIMAGE_OFFSET	MAX_UNCOMP_KERNEL_SIZE
> -#define MAX_FDT_OFFSET		ZIMAGE_OFFSET_LIMIT
> +
> +/* on ARM, the FDT should be located in the first 128 MB of RAM */
> +static inline unsigned long efi_get_max_fdt_addr(unsigned long dram_base)
> +{
> +	return dram_base + ZIMAGE_OFFSET_LIMIT;
> +}
> +
> +/* on ARM, the initrd should be loaded in a lowmem region */
> +static inline unsigned long efi_get_max_initrd_addr(unsigned long dram_base,
> +						    unsigned long image_addr)
> +{
> +	return dram_base + SZ_512M;
> +}
>  
>  #endif /* _ASM_ARM_EFI_H */
> diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
> index 0b6b1633017f..342e90d6d204 100644
> --- a/arch/arm64/include/asm/efi.h
> +++ b/arch/arm64/include/asm/efi.h
> @@ -46,7 +46,23 @@ int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md);
>   * 2MiB so we know it won't cross a 2MiB boundary.
>   */
>  #define EFI_FDT_ALIGN	SZ_2M   /* used by allocate_new_fdt_and_exit_boot() */
> -#define MAX_FDT_OFFSET	SZ_512M
> +
> +/* on arm64, the FDT may be located anywhere in system RAM */
> +static inline unsigned long efi_get_max_fdt_addr(unsigned long dram_base)
> +{
> +	return ULONG_MAX;
> +}
> +
> +/*
> + * On arm64, we have to ensure that the initrd ends up in the linear region,
> + * which is a 1 GB aligned region of size '1UL << (VA_BITS - 1)' that is
> + * guaranteed to cover the kernel Image.
> + */
> +static inline unsigned long efi_get_max_initrd_addr(unsigned long dram_base,
> +						    unsigned long image_addr)
> +{
> +	return (image_addr & ~(SZ_1G - 1UL)) + (1UL << (VA_BITS - 1));
> +}
>  
Please update booting.txt which specifies a window of 32G for ARM64

>  #define efi_call_early(f, ...)		sys_table_arg->boottime->f(__VA_ARGS__)
>  #define __efi_call_early(f, ...)	f(__VA_ARGS__)
> diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
> index b4f7d78f9e8b..557281fe375f 100644
> --- a/drivers/firmware/efi/libstub/arm-stub.c
> +++ b/drivers/firmware/efi/libstub/arm-stub.c
> @@ -333,8 +333,9 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table,
>  	if (!fdt_addr)
>  		pr_efi(sys_table, "Generating empty DTB\n");
>  
> -	status = handle_cmdline_files(sys_table, image, cmdline_ptr,
> -				      "initrd=", dram_base + SZ_512M,
> +	status = handle_cmdline_files(sys_table, image, cmdline_ptr, "initrd=",
> +				      efi_get_max_initrd_addr(dram_base,
> +							      *image_addr),
>  				      (unsigned long *)&initrd_addr,
>  				      (unsigned long *)&initrd_size);
>  	if (status != EFI_SUCCESS)
> @@ -344,7 +345,7 @@ 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,
> +				&new_fdt_addr, efi_get_max_fdt_addr(dram_base),
>  				initrd_addr, initrd_size, cmdline_ptr,
>  				fdt_addr, fdt_size);
>  

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH v2 1/2] efi: arm-stub: Correct FDT and initrd allocation rules for arm64

[Ruigrok, Richard]

On 2/9/2017 2:42 PM, Ard Biesheuvel wrote:
> On arm64, we have made some changes over the past year to the way the
> kernel itself is allocated and to how it deals with the initrd and FDT.
> This patch brings the allocation logic in the EFI stub in line with that,
> which is necessary because the introduction of KASLR has created the
> possibility for the initrd to be allocated in a place where the kernel
> may not be able to map it. (This is mostly a theoretical scenario, since
> it only affects systems where the physical memory footprint exceeds the
> size of the linear mapping.)
>
> Since we know the kernel itself will be covered by the linear mapping,
> choose a suitably sized window (i.e., based on the size of the linear
> region) covering the kernel when allocating memory for the initrd.
>
> The FDT may be anywhere in memory on arm64 now that we map it via the
> fixmap, so we can lift the address restriction there completely.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm/include/asm/efi.h              | 14 +++++++++++++-
>  arch/arm64/include/asm/efi.h            | 18 +++++++++++++++++-
>  drivers/firmware/efi/libstub/arm-stub.c |  7 ++++---
>  3 files changed, 34 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h
> index 0b06f5341b45..2de0195dfd1e 100644
> --- a/arch/arm/include/asm/efi.h
> +++ b/arch/arm/include/asm/efi.h
> @@ -84,6 +84,18 @@ static inline void efifb_setup_from_dmi(struct screen_info *si, const char *opt)
>   */
>  #define ZIMAGE_OFFSET_LIMIT	SZ_128M
>  #define MIN_ZIMAGE_OFFSET	MAX_UNCOMP_KERNEL_SIZE
> -#define MAX_FDT_OFFSET		ZIMAGE_OFFSET_LIMIT
> +
> +/* on ARM, the FDT should be located in the first 128 MB of RAM */
> +static inline unsigned long efi_get_max_fdt_addr(unsigned long dram_base)
> +{
> +	return dram_base + ZIMAGE_OFFSET_LIMIT;
> +}
> +
> +/* on ARM, the initrd should be loaded in a lowmem region */
> +static inline unsigned long efi_get_max_initrd_addr(unsigned long dram_base,
> +						    unsigned long image_addr)
> +{
> +	return dram_base + SZ_512M;
> +}
>  
>  #endif /* _ASM_ARM_EFI_H */
> diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
> index 0b6b1633017f..342e90d6d204 100644
> --- a/arch/arm64/include/asm/efi.h
> +++ b/arch/arm64/include/asm/efi.h
> @@ -46,7 +46,23 @@ int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md);
>   * 2MiB so we know it won't cross a 2MiB boundary.
>   */
>  #define EFI_FDT_ALIGN	SZ_2M   /* used by allocate_new_fdt_and_exit_boot() */
> -#define MAX_FDT_OFFSET	SZ_512M
> +
> +/* on arm64, the FDT may be located anywhere in system RAM */
> +static inline unsigned long efi_get_max_fdt_addr(unsigned long dram_base)
> +{
> +	return ULONG_MAX;
> +}
> +
> +/*
> + * On arm64, we have to ensure that the initrd ends up in the linear region,
> + * which is a 1 GB aligned region of size '1UL << (VA_BITS - 1)' that is
> + * guaranteed to cover the kernel Image.
> + */
> +static inline unsigned long efi_get_max_initrd_addr(unsigned long dram_base,
> +						    unsigned long image_addr)
> +{
> +	return (image_addr & ~(SZ_1G - 1UL)) + (1UL << (VA_BITS - 1));
> +}
>  
Please update booting.txt which specifies a window of 32G for ARM64

>  #define efi_call_early(f, ...)		sys_table_arg->boottime->f(__VA_ARGS__)
>  #define __efi_call_early(f, ...)	f(__VA_ARGS__)
> diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
> index b4f7d78f9e8b..557281fe375f 100644
> --- a/drivers/firmware/efi/libstub/arm-stub.c
> +++ b/drivers/firmware/efi/libstub/arm-stub.c
> @@ -333,8 +333,9 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table,
>  	if (!fdt_addr)
>  		pr_efi(sys_table, "Generating empty DTB\n");
>  
> -	status = handle_cmdline_files(sys_table, image, cmdline_ptr,
> -				      "initrd=", dram_base + SZ_512M,
> +	status = handle_cmdline_files(sys_table, image, cmdline_ptr, "initrd=",
> +				      efi_get_max_initrd_addr(dram_base,
> +							      *image_addr),
>  				      (unsigned long *)&initrd_addr,
>  				      (unsigned long *)&initrd_size);
>  	if (status != EFI_SUCCESS)
> @@ -344,7 +345,7 @@ 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,
> +				&new_fdt_addr, efi_get_max_fdt_addr(dram_base),
>  				initrd_addr, initrd_size, cmdline_ptr,
>  				fdt_addr, fdt_size);
>  

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH v2 1/2] efi: arm-stub: Correct FDT and initrd allocation rules for arm64

[Ard Biesheuvel]

> On 10 Feb 2017, at 00:42, Ruigrok, Richard <rruigrok-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
> 
> 
> 
>> On 2/9/2017 2:42 PM, Ard Biesheuvel wrote:
>> On arm64, we have made some changes over the past year to the way the
>> kernel itself is allocated and to how it deals with the initrd and FDT.
>> This patch brings the allocation logic in the EFI stub in line with that,
>> which is necessary because the introduction of KASLR has created the
>> possibility for the initrd to be allocated in a place where the kernel
>> may not be able to map it. (This is mostly a theoretical scenario, since
>> it only affects systems where the physical memory footprint exceeds the
>> size of the linear mapping.)
>> 
>> Since we know the kernel itself will be covered by the linear mapping,
>> choose a suitably sized window (i.e., based on the size of the linear
>> region) covering the kernel when allocating memory for the initrd.
>> 
>> The FDT may be anywhere in memory on arm64 now that we map it via the
>> fixmap, so we can lift the address restriction there completely.
>> 
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> ---
>> arch/arm/include/asm/efi.h              | 14 +++++++++++++-
>> arch/arm64/include/asm/efi.h            | 18 +++++++++++++++++-
>> drivers/firmware/efi/libstub/arm-stub.c |  7 ++++---
>> 3 files changed, 34 insertions(+), 5 deletions(-)
>> 
>> diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h
>> index 0b06f5341b45..2de0195dfd1e 100644
>> --- a/arch/arm/include/asm/efi.h
>> +++ b/arch/arm/include/asm/efi.h
>> @@ -84,6 +84,18 @@ static inline void efifb_setup_from_dmi(struct screen_info *si, const char *opt)
>>  */
>> #define ZIMAGE_OFFSET_LIMIT    SZ_128M
>> #define MIN_ZIMAGE_OFFSET    MAX_UNCOMP_KERNEL_SIZE
>> -#define MAX_FDT_OFFSET        ZIMAGE_OFFSET_LIMIT
>> +
>> +/* on ARM, the FDT should be located in the first 128 MB of RAM */
>> +static inline unsigned long efi_get_max_fdt_addr(unsigned long dram_base)
>> +{
>> +    return dram_base + ZIMAGE_OFFSET_LIMIT;
>> +}
>> +
>> +/* on ARM, the initrd should be loaded in a lowmem region */
>> +static inline unsigned long efi_get_max_initrd_addr(unsigned long dram_base,
>> +                            unsigned long image_addr)
>> +{
>> +    return dram_base + SZ_512M;
>> +}
>> 
>> #endif /* _ASM_ARM_EFI_H */
>> diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
>> index 0b6b1633017f..342e90d6d204 100644
>> --- a/arch/arm64/include/asm/efi.h
>> +++ b/arch/arm64/include/asm/efi.h
>> @@ -46,7 +46,23 @@ int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md);
>>  * 2MiB so we know it won't cross a 2MiB boundary.
>>  */
>> #define EFI_FDT_ALIGN    SZ_2M   /* used by allocate_new_fdt_and_exit_boot() */
>> -#define MAX_FDT_OFFSET    SZ_512M
>> +
>> +/* on arm64, the FDT may be located anywhere in system RAM */
>> +static inline unsigned long efi_get_max_fdt_addr(unsigned long dram_base)
>> +{
>> +    return ULONG_MAX;
>> +}
>> +
>> +/*
>> + * On arm64, we have to ensure that the initrd ends up in the linear region,
>> + * which is a 1 GB aligned region of size '1UL << (VA_BITS - 1)' that is
>> + * guaranteed to cover the kernel Image.
>> + */
>> +static inline unsigned long efi_get_max_initrd_addr(unsigned long dram_base,
>> +                            unsigned long image_addr)
>> +{
>> +    return (image_addr & ~(SZ_1G - 1UL)) + (1UL << (VA_BITS - 1));
>> +}
>> 
> Please update booting.txt which specifies a window of 32G for ARM64
> 

No. The efi stub is built into the kernel, so there we can be lax about these things. For boot loaders, 32GB remains a reasonable limit because some configurations require it

>> #define efi_call_early(f, ...)        sys_table_arg->boottime->f(__VA_ARGS__)
>> #define __efi_call_early(f, ...)    f(__VA_ARGS__)
>> diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
>> index b4f7d78f9e8b..557281fe375f 100644
>> --- a/drivers/firmware/efi/libstub/arm-stub.c
>> +++ b/drivers/firmware/efi/libstub/arm-stub.c
>> @@ -333,8 +333,9 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table,
>>    if (!fdt_addr)
>>        pr_efi(sys_table, "Generating empty DTB\n");
>> 
>> -    status = handle_cmdline_files(sys_table, image, cmdline_ptr,
>> -                      "initrd=", dram_base + SZ_512M,
>> +    status = handle_cmdline_files(sys_table, image, cmdline_ptr, "initrd=",
>> +                      efi_get_max_initrd_addr(dram_base,
>> +                                  *image_addr),
>>                      (unsigned long *)&initrd_addr,
>>                      (unsigned long *)&initrd_size);
>>    if (status != EFI_SUCCESS)
>> @@ -344,7 +345,7 @@ 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,
>> +                &new_fdt_addr, efi_get_max_fdt_addr(dram_base),
>>                initrd_addr, initrd_size, cmdline_ptr,
>>                fdt_addr, fdt_size);
>> 
> 
> -- 
> Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
> Qualcomm Technologies, Inc. is a member of the
> Code Aurora Forum, a Linux Foundation Collaborative Project.
> 

[PATCH v2 1/2] efi: arm-stub: Correct FDT and initrd allocation rules for arm64

[Ard Biesheuvel]

> On 10 Feb 2017, at 00:42, Ruigrok, Richard <rruigrok@codeaurora.org> wrote:
> 
> 
> 
>> On 2/9/2017 2:42 PM, Ard Biesheuvel wrote:
>> On arm64, we have made some changes over the past year to the way the
>> kernel itself is allocated and to how it deals with the initrd and FDT.
>> This patch brings the allocation logic in the EFI stub in line with that,
>> which is necessary because the introduction of KASLR has created the
>> possibility for the initrd to be allocated in a place where the kernel
>> may not be able to map it. (This is mostly a theoretical scenario, since
>> it only affects systems where the physical memory footprint exceeds the
>> size of the linear mapping.)
>> 
>> Since we know the kernel itself will be covered by the linear mapping,
>> choose a suitably sized window (i.e., based on the size of the linear
>> region) covering the kernel when allocating memory for the initrd.
>> 
>> The FDT may be anywhere in memory on arm64 now that we map it via the
>> fixmap, so we can lift the address restriction there completely.
>> 
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>> arch/arm/include/asm/efi.h              | 14 +++++++++++++-
>> arch/arm64/include/asm/efi.h            | 18 +++++++++++++++++-
>> drivers/firmware/efi/libstub/arm-stub.c |  7 ++++---
>> 3 files changed, 34 insertions(+), 5 deletions(-)
>> 
>> diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h
>> index 0b06f5341b45..2de0195dfd1e 100644
>> --- a/arch/arm/include/asm/efi.h
>> +++ b/arch/arm/include/asm/efi.h
>> @@ -84,6 +84,18 @@ static inline void efifb_setup_from_dmi(struct screen_info *si, const char *opt)
>>  */
>> #define ZIMAGE_OFFSET_LIMIT    SZ_128M
>> #define MIN_ZIMAGE_OFFSET    MAX_UNCOMP_KERNEL_SIZE
>> -#define MAX_FDT_OFFSET        ZIMAGE_OFFSET_LIMIT
>> +
>> +/* on ARM, the FDT should be located in the first 128 MB of RAM */
>> +static inline unsigned long efi_get_max_fdt_addr(unsigned long dram_base)
>> +{
>> +    return dram_base + ZIMAGE_OFFSET_LIMIT;
>> +}
>> +
>> +/* on ARM, the initrd should be loaded in a lowmem region */
>> +static inline unsigned long efi_get_max_initrd_addr(unsigned long dram_base,
>> +                            unsigned long image_addr)
>> +{
>> +    return dram_base + SZ_512M;
>> +}
>> 
>> #endif /* _ASM_ARM_EFI_H */
>> diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
>> index 0b6b1633017f..342e90d6d204 100644
>> --- a/arch/arm64/include/asm/efi.h
>> +++ b/arch/arm64/include/asm/efi.h
>> @@ -46,7 +46,23 @@ int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md);
>>  * 2MiB so we know it won't cross a 2MiB boundary.
>>  */
>> #define EFI_FDT_ALIGN    SZ_2M   /* used by allocate_new_fdt_and_exit_boot() */
>> -#define MAX_FDT_OFFSET    SZ_512M
>> +
>> +/* on arm64, the FDT may be located anywhere in system RAM */
>> +static inline unsigned long efi_get_max_fdt_addr(unsigned long dram_base)
>> +{
>> +    return ULONG_MAX;
>> +}
>> +
>> +/*
>> + * On arm64, we have to ensure that the initrd ends up in the linear region,
>> + * which is a 1 GB aligned region of size '1UL << (VA_BITS - 1)' that is
>> + * guaranteed to cover the kernel Image.
>> + */
>> +static inline unsigned long efi_get_max_initrd_addr(unsigned long dram_base,
>> +                            unsigned long image_addr)
>> +{
>> +    return (image_addr & ~(SZ_1G - 1UL)) + (1UL << (VA_BITS - 1));
>> +}
>> 
> Please update booting.txt which specifies a window of 32G for ARM64
> 

No. The efi stub is built into the kernel, so there we can be lax about these things. For boot loaders, 32GB remains a reasonable limit because some configurations require it

>> #define efi_call_early(f, ...)        sys_table_arg->boottime->f(__VA_ARGS__)
>> #define __efi_call_early(f, ...)    f(__VA_ARGS__)
>> diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
>> index b4f7d78f9e8b..557281fe375f 100644
>> --- a/drivers/firmware/efi/libstub/arm-stub.c
>> +++ b/drivers/firmware/efi/libstub/arm-stub.c
>> @@ -333,8 +333,9 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table,
>>    if (!fdt_addr)
>>        pr_efi(sys_table, "Generating empty DTB\n");
>> 
>> -    status = handle_cmdline_files(sys_table, image, cmdline_ptr,
>> -                      "initrd=", dram_base + SZ_512M,
>> +    status = handle_cmdline_files(sys_table, image, cmdline_ptr, "initrd=",
>> +                      efi_get_max_initrd_addr(dram_base,
>> +                                  *image_addr),
>>                      (unsigned long *)&initrd_addr,
>>                      (unsigned long *)&initrd_size);
>>    if (status != EFI_SUCCESS)
>> @@ -344,7 +345,7 @@ 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,
>> +                &new_fdt_addr, efi_get_max_fdt_addr(dram_base),
>>                initrd_addr, initrd_size, cmdline_ptr,
>>                fdt_addr, fdt_size);
>> 
> 
> -- 
> Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
> Qualcomm Technologies, Inc. is a member of the
> Code Aurora Forum, a Linux Foundation Collaborative Project.
> 

Re: [PATCH v2 1/2] efi: arm-stub: Correct FDT and initrd allocation rules for arm64

[Mark Rutland]

On Fri, Feb 10, 2017 at 06:38:11AM +0000, Ard Biesheuvel wrote:
> > On 10 Feb 2017, at 00:42, Ruigrok, Richard <rruigrok-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
> >> On 2/9/2017 2:42 PM, Ard Biesheuvel wrote:

> >> diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
> >> index 0b6b1633017f..342e90d6d204 100644
> >> --- a/arch/arm64/include/asm/efi.h
> >> +++ b/arch/arm64/include/asm/efi.h
> >> @@ -46,7 +46,23 @@ int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md);
> >>  * 2MiB so we know it won't cross a 2MiB boundary.
> >>  */
> >> #define EFI_FDT_ALIGN    SZ_2M   /* used by allocate_new_fdt_and_exit_boot() */
> >> -#define MAX_FDT_OFFSET    SZ_512M
> >> +
> >> +/* on arm64, the FDT may be located anywhere in system RAM */
> >> +static inline unsigned long efi_get_max_fdt_addr(unsigned long dram_base)
> >> +{
> >> +    return ULONG_MAX;
> >> +}
> >> +
> >> +/*
> >> + * On arm64, we have to ensure that the initrd ends up in the linear region,
> >> + * which is a 1 GB aligned region of size '1UL << (VA_BITS - 1)' that is
> >> + * guaranteed to cover the kernel Image.
> >> + */
> >> +static inline unsigned long efi_get_max_initrd_addr(unsigned long dram_base,
> >> +                            unsigned long image_addr)
> >> +{
> >> +    return (image_addr & ~(SZ_1G - 1UL)) + (1UL << (VA_BITS - 1));
> >> +}
> >> 
> > Please update booting.txt which specifies a window of 32G for ARM64
> > 
> 
> No. The efi stub is built into the kernel, so there we can be lax
> about these things.

Sure.

Given this is a source of confusion, can we drop a comment here as to
how this is deliberate? e.g.

/*
 * On arm64, we have to ensure that the initrd ends up in the linear region,
 * which is a 1 GB aligned region of size '1UL << (VA_BITS - 1)' that is
 * guaranteed to cover the kernel Image.
 *
 * Since the EFI stub is part of the kernel Image, we can relax than the
 * usual requirements in Documentation/arm64/booting.txt, which still
 * apply to other bootloaders, and are required for some kernel
 * configurations.
 */

Thanks,
Mark.

[PATCH v2 1/2] efi: arm-stub: Correct FDT and initrd allocation rules for arm64

[Mark Rutland]

On Fri, Feb 10, 2017 at 06:38:11AM +0000, Ard Biesheuvel wrote:
> > On 10 Feb 2017, at 00:42, Ruigrok, Richard <rruigrok@codeaurora.org> wrote:
> >> On 2/9/2017 2:42 PM, Ard Biesheuvel wrote:

> >> diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
> >> index 0b6b1633017f..342e90d6d204 100644
> >> --- a/arch/arm64/include/asm/efi.h
> >> +++ b/arch/arm64/include/asm/efi.h
> >> @@ -46,7 +46,23 @@ int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md);
> >>  * 2MiB so we know it won't cross a 2MiB boundary.
> >>  */
> >> #define EFI_FDT_ALIGN    SZ_2M   /* used by allocate_new_fdt_and_exit_boot() */
> >> -#define MAX_FDT_OFFSET    SZ_512M
> >> +
> >> +/* on arm64, the FDT may be located anywhere in system RAM */
> >> +static inline unsigned long efi_get_max_fdt_addr(unsigned long dram_base)
> >> +{
> >> +    return ULONG_MAX;
> >> +}
> >> +
> >> +/*
> >> + * On arm64, we have to ensure that the initrd ends up in the linear region,
> >> + * which is a 1 GB aligned region of size '1UL << (VA_BITS - 1)' that is
> >> + * guaranteed to cover the kernel Image.
> >> + */
> >> +static inline unsigned long efi_get_max_initrd_addr(unsigned long dram_base,
> >> +                            unsigned long image_addr)
> >> +{
> >> +    return (image_addr & ~(SZ_1G - 1UL)) + (1UL << (VA_BITS - 1));
> >> +}
> >> 
> > Please update booting.txt which specifies a window of 32G for ARM64
> > 
> 
> No. The efi stub is built into the kernel, so there we can be lax
> about these things.

Sure.

Given this is a source of confusion, can we drop a comment here as to
how this is deliberate? e.g.

/*
 * On arm64, we have to ensure that the initrd ends up in the linear region,
 * which is a 1 GB aligned region of size '1UL << (VA_BITS - 1)' that is
 * guaranteed to cover the kernel Image.
 *
 * Since the EFI stub is part of the kernel Image, we can relax than the
 * usual requirements in Documentation/arm64/booting.txt, which still
 * apply to other bootloaders, and are required for some kernel
 * configurations.
 */

Thanks,
Mark.

Re: [PATCH v2 2/2] efi: arm-stub: Round up FDT allocation to mapping size

[Ard Biesheuvel]

On 10 February 2017 at 00:40, Ruigrok, Richard <rruigrok-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
>
>
> On 2/9/2017 2:42 PM, Ard Biesheuvel wrote:
>
> The FDT is mapped via a fixmap entry that is at least 2 MB in size and
> 2 MB aligned on 4 KB page size kernels.
>
> On UEFI systems, the FDT allocation may share this 2 MB block with a
> reserved region, or another memory region that we should never map,
> unless we account for this in the size of the allocation (the alignment
> is already 2 MB)
>
> So instead of taking guesses at the needed space, simply allocate 2 MB
> immediately. The allocation will be recorded as a EFI_LOADER_DATA, and
> the kernel only memblock_reserve()'s the actual size of the FDT, so the
> unused space will be released to the kernel.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  arch/arm64/include/asm/efi.h       |  1 +
>  drivers/firmware/efi/libstub/fdt.c | 57 +++++++++-----------
>  2 files changed, 25 insertions(+), 33 deletions(-)
>
[..]
> Tested successfully on our QDT2400 system on which we were seeing a failure
> to allocate memory for initrd.
>
> Tested-by: Richard Ruigrok <rruigrok-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
>

Thanks

[PATCH v2 2/2] efi: arm-stub: Round up FDT allocation to mapping size

[Ard Biesheuvel]

On 10 February 2017 at 00:40, Ruigrok, Richard <rruigrok@codeaurora.org> wrote:
>
>
> On 2/9/2017 2:42 PM, Ard Biesheuvel wrote:
>
> The FDT is mapped via a fixmap entry that is at least 2 MB in size and
> 2 MB aligned on 4 KB page size kernels.
>
> On UEFI systems, the FDT allocation may share this 2 MB block with a
> reserved region, or another memory region that we should never map,
> unless we account for this in the size of the allocation (the alignment
> is already 2 MB)
>
> So instead of taking guesses at the needed space, simply allocate 2 MB
> immediately. The allocation will be recorded as a EFI_LOADER_DATA, and
> the kernel only memblock_reserve()'s the actual size of the FDT, so the
> unused space will be released to the kernel.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm64/include/asm/efi.h       |  1 +
>  drivers/firmware/efi/libstub/fdt.c | 57 +++++++++-----------
>  2 files changed, 25 insertions(+), 33 deletions(-)
>
[..]
> Tested successfully on our QDT2400 system on which we were seeing a failure
> to allocate memory for initrd.
>
> Tested-by: Richard Ruigrok <rruigrok@codeaurora.org>
>

Thanks

Re: [PATCH v2 1/2] efi: arm-stub: Correct FDT and initrd allocation rules for arm64

[Ard Biesheuvel]

On 10 February 2017 at 10:05, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
> On Fri, Feb 10, 2017 at 06:38:11AM +0000, Ard Biesheuvel wrote:
>> > On 10 Feb 2017, at 00:42, Ruigrok, Richard <rruigrok-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
>> >> On 2/9/2017 2:42 PM, Ard Biesheuvel wrote:
>
>> >> diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
>> >> index 0b6b1633017f..342e90d6d204 100644
>> >> --- a/arch/arm64/include/asm/efi.h
>> >> +++ b/arch/arm64/include/asm/efi.h
>> >> @@ -46,7 +46,23 @@ int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md);
>> >>  * 2MiB so we know it won't cross a 2MiB boundary.
>> >>  */
>> >> #define EFI_FDT_ALIGN    SZ_2M   /* used by allocate_new_fdt_and_exit_boot() */
>> >> -#define MAX_FDT_OFFSET    SZ_512M
>> >> +
>> >> +/* on arm64, the FDT may be located anywhere in system RAM */
>> >> +static inline unsigned long efi_get_max_fdt_addr(unsigned long dram_base)
>> >> +{
>> >> +    return ULONG_MAX;
>> >> +}
>> >> +
>> >> +/*
>> >> + * On arm64, we have to ensure that the initrd ends up in the linear region,
>> >> + * which is a 1 GB aligned region of size '1UL << (VA_BITS - 1)' that is
>> >> + * guaranteed to cover the kernel Image.
>> >> + */
>> >> +static inline unsigned long efi_get_max_initrd_addr(unsigned long dram_base,
>> >> +                            unsigned long image_addr)
>> >> +{
>> >> +    return (image_addr & ~(SZ_1G - 1UL)) + (1UL << (VA_BITS - 1));
>> >> +}
>> >>
>> > Please update booting.txt which specifies a window of 32G for ARM64
>> >
>>
>> No. The efi stub is built into the kernel, so there we can be lax
>> about these things.
>
> Sure.
>
> Given this is a source of confusion, can we drop a comment here as to
> how this is deliberate? e.g.
>
> /*
>  * On arm64, we have to ensure that the initrd ends up in the linear region,
>  * which is a 1 GB aligned region of size '1UL << (VA_BITS - 1)' that is
>  * guaranteed to cover the kernel Image.
>  *
>  * Since the EFI stub is part of the kernel Image, we can relax than the
>  * usual requirements in Documentation/arm64/booting.txt, which still
>  * apply to other bootloaders, and are required for some kernel
>  * configurations.
>  */
>

Absolutely

[PATCH v2 1/2] efi: arm-stub: Correct FDT and initrd allocation rules for arm64

[Ard Biesheuvel]

On 10 February 2017 at 10:05, Mark Rutland <mark.rutland@arm.com> wrote:
> On Fri, Feb 10, 2017 at 06:38:11AM +0000, Ard Biesheuvel wrote:
>> > On 10 Feb 2017, at 00:42, Ruigrok, Richard <rruigrok@codeaurora.org> wrote:
>> >> On 2/9/2017 2:42 PM, Ard Biesheuvel wrote:
>
>> >> diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
>> >> index 0b6b1633017f..342e90d6d204 100644
>> >> --- a/arch/arm64/include/asm/efi.h
>> >> +++ b/arch/arm64/include/asm/efi.h
>> >> @@ -46,7 +46,23 @@ int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md);
>> >>  * 2MiB so we know it won't cross a 2MiB boundary.
>> >>  */
>> >> #define EFI_FDT_ALIGN    SZ_2M   /* used by allocate_new_fdt_and_exit_boot() */
>> >> -#define MAX_FDT_OFFSET    SZ_512M
>> >> +
>> >> +/* on arm64, the FDT may be located anywhere in system RAM */
>> >> +static inline unsigned long efi_get_max_fdt_addr(unsigned long dram_base)
>> >> +{
>> >> +    return ULONG_MAX;
>> >> +}
>> >> +
>> >> +/*
>> >> + * On arm64, we have to ensure that the initrd ends up in the linear region,
>> >> + * which is a 1 GB aligned region of size '1UL << (VA_BITS - 1)' that is
>> >> + * guaranteed to cover the kernel Image.
>> >> + */
>> >> +static inline unsigned long efi_get_max_initrd_addr(unsigned long dram_base,
>> >> +                            unsigned long image_addr)
>> >> +{
>> >> +    return (image_addr & ~(SZ_1G - 1UL)) + (1UL << (VA_BITS - 1));
>> >> +}
>> >>
>> > Please update booting.txt which specifies a window of 32G for ARM64
>> >
>>
>> No. The efi stub is built into the kernel, so there we can be lax
>> about these things.
>
> Sure.
>
> Given this is a source of confusion, can we drop a comment here as to
> how this is deliberate? e.g.
>
> /*
>  * On arm64, we have to ensure that the initrd ends up in the linear region,
>  * which is a 1 GB aligned region of size '1UL << (VA_BITS - 1)' that is
>  * guaranteed to cover the kernel Image.
>  *
>  * Since the EFI stub is part of the kernel Image, we can relax than the
>  * usual requirements in Documentation/arm64/booting.txt, which still
>  * apply to other bootloaders, and are required for some kernel
>  * configurations.
>  */
>

Absolutely

Re: [PATCH v2 1/2] efi: arm-stub: Correct FDT and initrd allocation rules for arm64

[Jeffrey Hugo]

On 2/9/2017 2:42 PM, Ard Biesheuvel wrote:
> On arm64, we have made some changes over the past year to the way the
> kernel itself is allocated and to how it deals with the initrd and FDT.
> This patch brings the allocation logic in the EFI stub in line with that,
> which is necessary because the introduction of KASLR has created the
> possibility for the initrd to be allocated in a place where the kernel
> may not be able to map it. (This is mostly a theoretical scenario, since
> it only affects systems where the physical memory footprint exceeds the
> size of the linear mapping.)
>
> Since we know the kernel itself will be covered by the linear mapping,
> choose a suitably sized window (i.e., based on the size of the linear
> region) covering the kernel when allocating memory for the initrd.
>
> The FDT may be anywhere in memory on arm64 now that we map it via the
> fixmap, so we can lift the address restriction there completely.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---

Reviewed-By: Jeffrey Hugo <jhugo-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>

>  arch/arm/include/asm/efi.h              | 14 +++++++++++++-
>  arch/arm64/include/asm/efi.h            | 18 +++++++++++++++++-
>  drivers/firmware/efi/libstub/arm-stub.c |  7 ++++---
>  3 files changed, 34 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h
> index 0b06f5341b45..2de0195dfd1e 100644
> --- a/arch/arm/include/asm/efi.h
> +++ b/arch/arm/include/asm/efi.h
> @@ -84,6 +84,18 @@ static inline void efifb_setup_from_dmi(struct screen_info *si, const char *opt)
>   */
>  #define ZIMAGE_OFFSET_LIMIT	SZ_128M
>  #define MIN_ZIMAGE_OFFSET	MAX_UNCOMP_KERNEL_SIZE
> -#define MAX_FDT_OFFSET		ZIMAGE_OFFSET_LIMIT
> +
> +/* on ARM, the FDT should be located in the first 128 MB of RAM */
> +static inline unsigned long efi_get_max_fdt_addr(unsigned long dram_base)
> +{
> +	return dram_base + ZIMAGE_OFFSET_LIMIT;
> +}
> +
> +/* on ARM, the initrd should be loaded in a lowmem region */
> +static inline unsigned long efi_get_max_initrd_addr(unsigned long dram_base,
> +						    unsigned long image_addr)
> +{
> +	return dram_base + SZ_512M;
> +}
>
>  #endif /* _ASM_ARM_EFI_H */
> diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
> index 0b6b1633017f..342e90d6d204 100644
> --- a/arch/arm64/include/asm/efi.h
> +++ b/arch/arm64/include/asm/efi.h
> @@ -46,7 +46,23 @@ int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md);
>   * 2MiB so we know it won't cross a 2MiB boundary.
>   */
>  #define EFI_FDT_ALIGN	SZ_2M   /* used by allocate_new_fdt_and_exit_boot() */
> -#define MAX_FDT_OFFSET	SZ_512M
> +
> +/* on arm64, the FDT may be located anywhere in system RAM */
> +static inline unsigned long efi_get_max_fdt_addr(unsigned long dram_base)
> +{
> +	return ULONG_MAX;
> +}
> +
> +/*
> + * On arm64, we have to ensure that the initrd ends up in the linear region,
> + * which is a 1 GB aligned region of size '1UL << (VA_BITS - 1)' that is
> + * guaranteed to cover the kernel Image.
> + */
> +static inline unsigned long efi_get_max_initrd_addr(unsigned long dram_base,
> +						    unsigned long image_addr)
> +{
> +	return (image_addr & ~(SZ_1G - 1UL)) + (1UL << (VA_BITS - 1));
> +}
>
>  #define efi_call_early(f, ...)		sys_table_arg->boottime->f(__VA_ARGS__)
>  #define __efi_call_early(f, ...)	f(__VA_ARGS__)
> diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
> index b4f7d78f9e8b..557281fe375f 100644
> --- a/drivers/firmware/efi/libstub/arm-stub.c
> +++ b/drivers/firmware/efi/libstub/arm-stub.c
> @@ -333,8 +333,9 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table,
>  	if (!fdt_addr)
>  		pr_efi(sys_table, "Generating empty DTB\n");
>
> -	status = handle_cmdline_files(sys_table, image, cmdline_ptr,
> -				      "initrd=", dram_base + SZ_512M,
> +	status = handle_cmdline_files(sys_table, image, cmdline_ptr, "initrd=",
> +				      efi_get_max_initrd_addr(dram_base,
> +							      *image_addr),
>  				      (unsigned long *)&initrd_addr,
>  				      (unsigned long *)&initrd_size);
>  	if (status != EFI_SUCCESS)
> @@ -344,7 +345,7 @@ 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,
> +				&new_fdt_addr, efi_get_max_fdt_addr(dram_base),
>  				initrd_addr, initrd_size, cmdline_ptr,
>  				fdt_addr, fdt_size);
>
>


-- 
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH v2 1/2] efi: arm-stub: Correct FDT and initrd allocation rules for arm64

[Jeffrey Hugo]

On 2/9/2017 2:42 PM, Ard Biesheuvel wrote:
> On arm64, we have made some changes over the past year to the way the
> kernel itself is allocated and to how it deals with the initrd and FDT.
> This patch brings the allocation logic in the EFI stub in line with that,
> which is necessary because the introduction of KASLR has created the
> possibility for the initrd to be allocated in a place where the kernel
> may not be able to map it. (This is mostly a theoretical scenario, since
> it only affects systems where the physical memory footprint exceeds the
> size of the linear mapping.)
>
> Since we know the kernel itself will be covered by the linear mapping,
> choose a suitably sized window (i.e., based on the size of the linear
> region) covering the kernel when allocating memory for the initrd.
>
> The FDT may be anywhere in memory on arm64 now that we map it via the
> fixmap, so we can lift the address restriction there completely.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---

Reviewed-By: Jeffrey Hugo <jhugo@codeaurora.org>

>  arch/arm/include/asm/efi.h              | 14 +++++++++++++-
>  arch/arm64/include/asm/efi.h            | 18 +++++++++++++++++-
>  drivers/firmware/efi/libstub/arm-stub.c |  7 ++++---
>  3 files changed, 34 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h
> index 0b06f5341b45..2de0195dfd1e 100644
> --- a/arch/arm/include/asm/efi.h
> +++ b/arch/arm/include/asm/efi.h
> @@ -84,6 +84,18 @@ static inline void efifb_setup_from_dmi(struct screen_info *si, const char *opt)
>   */
>  #define ZIMAGE_OFFSET_LIMIT	SZ_128M
>  #define MIN_ZIMAGE_OFFSET	MAX_UNCOMP_KERNEL_SIZE
> -#define MAX_FDT_OFFSET		ZIMAGE_OFFSET_LIMIT
> +
> +/* on ARM, the FDT should be located in the first 128 MB of RAM */
> +static inline unsigned long efi_get_max_fdt_addr(unsigned long dram_base)
> +{
> +	return dram_base + ZIMAGE_OFFSET_LIMIT;
> +}
> +
> +/* on ARM, the initrd should be loaded in a lowmem region */
> +static inline unsigned long efi_get_max_initrd_addr(unsigned long dram_base,
> +						    unsigned long image_addr)
> +{
> +	return dram_base + SZ_512M;
> +}
>
>  #endif /* _ASM_ARM_EFI_H */
> diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
> index 0b6b1633017f..342e90d6d204 100644
> --- a/arch/arm64/include/asm/efi.h
> +++ b/arch/arm64/include/asm/efi.h
> @@ -46,7 +46,23 @@ int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md);
>   * 2MiB so we know it won't cross a 2MiB boundary.
>   */
>  #define EFI_FDT_ALIGN	SZ_2M   /* used by allocate_new_fdt_and_exit_boot() */
> -#define MAX_FDT_OFFSET	SZ_512M
> +
> +/* on arm64, the FDT may be located anywhere in system RAM */
> +static inline unsigned long efi_get_max_fdt_addr(unsigned long dram_base)
> +{
> +	return ULONG_MAX;
> +}
> +
> +/*
> + * On arm64, we have to ensure that the initrd ends up in the linear region,
> + * which is a 1 GB aligned region of size '1UL << (VA_BITS - 1)' that is
> + * guaranteed to cover the kernel Image.
> + */
> +static inline unsigned long efi_get_max_initrd_addr(unsigned long dram_base,
> +						    unsigned long image_addr)
> +{
> +	return (image_addr & ~(SZ_1G - 1UL)) + (1UL << (VA_BITS - 1));
> +}
>
>  #define efi_call_early(f, ...)		sys_table_arg->boottime->f(__VA_ARGS__)
>  #define __efi_call_early(f, ...)	f(__VA_ARGS__)
> diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
> index b4f7d78f9e8b..557281fe375f 100644
> --- a/drivers/firmware/efi/libstub/arm-stub.c
> +++ b/drivers/firmware/efi/libstub/arm-stub.c
> @@ -333,8 +333,9 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table,
>  	if (!fdt_addr)
>  		pr_efi(sys_table, "Generating empty DTB\n");
>
> -	status = handle_cmdline_files(sys_table, image, cmdline_ptr,
> -				      "initrd=", dram_base + SZ_512M,
> +	status = handle_cmdline_files(sys_table, image, cmdline_ptr, "initrd=",
> +				      efi_get_max_initrd_addr(dram_base,
> +							      *image_addr),
>  				      (unsigned long *)&initrd_addr,
>  				      (unsigned long *)&initrd_size);
>  	if (status != EFI_SUCCESS)
> @@ -344,7 +345,7 @@ 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,
> +				&new_fdt_addr, efi_get_max_fdt_addr(dram_base),
>  				initrd_addr, initrd_size, cmdline_ptr,
>  				fdt_addr, fdt_size);
>
>


-- 
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH v2 2/2] efi: arm-stub: Round up FDT allocation to mapping size

[Jeffrey Hugo]

On 2/9/2017 2:42 PM, Ard Biesheuvel wrote:
> The FDT is mapped via a fixmap entry that is at least 2 MB in size and
> 2 MB aligned on 4 KB page size kernels.
>
> On UEFI systems, the FDT allocation may share this 2 MB block with a
> reserved region, or another memory region that we should never map,
> unless we account for this in the size of the allocation (the alignment
> is already 2 MB)
>
> So instead of taking guesses at the needed space, simply allocate 2 MB
> immediately. The allocation will be recorded as a EFI_LOADER_DATA, and
> the kernel only memblock_reserve()'s the actual size of the FDT, so the
> unused space will be released to the kernel.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---

Reviewed-By: Jeffrey Hugo <jhugo-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>

>  arch/arm64/include/asm/efi.h       |  1 +
>  drivers/firmware/efi/libstub/fdt.c | 57 +++++++++-----------
>  2 files changed, 25 insertions(+), 33 deletions(-)
>
> diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
> index 342e90d6d204..f642565fc1d0 100644
> --- a/arch/arm64/include/asm/efi.h
> +++ b/arch/arm64/include/asm/efi.h
> @@ -1,6 +1,7 @@
>  #ifndef _ASM_EFI_H
>  #define _ASM_EFI_H
>
> +#include <asm/boot.h>
>  #include <asm/cpufeature.h>
>  #include <asm/io.h>
>  #include <asm/mmu_context.h>
> diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
> index 260c4b4b492e..41f457be64e8 100644
> --- a/drivers/firmware/efi/libstub/fdt.c
> +++ b/drivers/firmware/efi/libstub/fdt.c
> @@ -206,6 +206,10 @@ static efi_status_t exit_boot_func(efi_system_table_t *sys_table_arg,
>  	return update_fdt_memmap(p->new_fdt_addr, map);
>  }
>
> +#ifndef MAX_FDT_SIZE
> +#define MAX_FDT_SIZE	SZ_2M
> +#endif
> +
>  /*
>   * Allocate memory for a new FDT, then add EFI, commandline, and
>   * initrd related fields to the FDT.  This routine increases the
> @@ -233,7 +237,6 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
>  	u32 desc_ver;
>  	unsigned long mmap_key;
>  	efi_memory_desc_t *memory_map, *runtime_map;
> -	unsigned long new_fdt_size;
>  	efi_status_t status;
>  	int runtime_entry_count = 0;
>  	struct efi_boot_memmap map;
> @@ -262,41 +265,29 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
>  	       "Exiting boot services and installing virtual address map...\n");
>
>  	map.map = &memory_map;
> +	status = efi_high_alloc(sys_table, MAX_FDT_SIZE, EFI_FDT_ALIGN,
> +				new_fdt_addr, max_addr);
> +	if (status != EFI_SUCCESS) {
> +		pr_efi_err(sys_table,
> +			   "Unable to allocate memory for new device tree.\n");
> +		goto fail;
> +	}
> +
>  	/*
> -	 * Estimate size of new FDT, and allocate memory for it. We
> -	 * will allocate a bigger buffer if this ends up being too
> -	 * small, so a rough guess is OK here.
> +	 * Now that we have done our final memory allocation (and free)
> +	 * we can get the memory map key needed for exit_boot_services().
>  	 */
> -	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);
> -		if (status != EFI_SUCCESS) {
> -			pr_efi_err(sys_table, "Unable to allocate memory for new device tree.\n");
> -			goto fail;
> -		}
> -
> -		status = update_fdt(sys_table,
> -				    (void *)fdt_addr, fdt_size,
> -				    (void *)*new_fdt_addr, new_fdt_size,
> -				    cmdline_ptr, initrd_addr, initrd_size);
> +	status = efi_get_memory_map(sys_table, &map);
> +	if (status != EFI_SUCCESS)
> +		goto fail_free_new_fdt;
>
> -		/* Succeeding the first time is the expected case. */
> -		if (status == EFI_SUCCESS)
> -			break;
> +	status = update_fdt(sys_table, (void *)fdt_addr, fdt_size,
> +			    (void *)*new_fdt_addr, MAX_FDT_SIZE, cmdline_ptr,
> +			    initrd_addr, initrd_size);
>
> -		if (status == EFI_BUFFER_TOO_SMALL) {
> -			/*
> -			 * We need to allocate more space for the new
> -			 * device tree, so free existing buffer that is
> -			 * too small.
> -			 */
> -			efi_free(sys_table, new_fdt_size, *new_fdt_addr);
> -			new_fdt_size += EFI_PAGE_SIZE;
> -		} else {
> -			pr_efi_err(sys_table, "Unable to construct new device tree.\n");
> -			goto fail_free_new_fdt;
> -		}
> +	if (status != EFI_SUCCESS) {
> +		pr_efi_err(sys_table, "Unable to construct new device tree.\n");
> +		goto fail_free_new_fdt;
>  	}
>
>  	priv.runtime_map = runtime_map;
> @@ -340,7 +331,7 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
>  	pr_efi_err(sys_table, "Exit boot services failed.\n");
>
>  fail_free_new_fdt:
> -	efi_free(sys_table, new_fdt_size, *new_fdt_addr);
> +	efi_free(sys_table, MAX_FDT_SIZE, *new_fdt_addr);
>
>  fail:
>  	sys_table->boottime->free_pool(runtime_map);
>


-- 
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH v2 2/2] efi: arm-stub: Round up FDT allocation to mapping size

[Jeffrey Hugo]

On 2/9/2017 2:42 PM, Ard Biesheuvel wrote:
> The FDT is mapped via a fixmap entry that is at least 2 MB in size and
> 2 MB aligned on 4 KB page size kernels.
>
> On UEFI systems, the FDT allocation may share this 2 MB block with a
> reserved region, or another memory region that we should never map,
> unless we account for this in the size of the allocation (the alignment
> is already 2 MB)
>
> So instead of taking guesses at the needed space, simply allocate 2 MB
> immediately. The allocation will be recorded as a EFI_LOADER_DATA, and
> the kernel only memblock_reserve()'s the actual size of the FDT, so the
> unused space will be released to the kernel.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---

Reviewed-By: Jeffrey Hugo <jhugo@codeaurora.org>

>  arch/arm64/include/asm/efi.h       |  1 +
>  drivers/firmware/efi/libstub/fdt.c | 57 +++++++++-----------
>  2 files changed, 25 insertions(+), 33 deletions(-)
>
> diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
> index 342e90d6d204..f642565fc1d0 100644
> --- a/arch/arm64/include/asm/efi.h
> +++ b/arch/arm64/include/asm/efi.h
> @@ -1,6 +1,7 @@
>  #ifndef _ASM_EFI_H
>  #define _ASM_EFI_H
>
> +#include <asm/boot.h>
>  #include <asm/cpufeature.h>
>  #include <asm/io.h>
>  #include <asm/mmu_context.h>
> diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
> index 260c4b4b492e..41f457be64e8 100644
> --- a/drivers/firmware/efi/libstub/fdt.c
> +++ b/drivers/firmware/efi/libstub/fdt.c
> @@ -206,6 +206,10 @@ static efi_status_t exit_boot_func(efi_system_table_t *sys_table_arg,
>  	return update_fdt_memmap(p->new_fdt_addr, map);
>  }
>
> +#ifndef MAX_FDT_SIZE
> +#define MAX_FDT_SIZE	SZ_2M
> +#endif
> +
>  /*
>   * Allocate memory for a new FDT, then add EFI, commandline, and
>   * initrd related fields to the FDT.  This routine increases the
> @@ -233,7 +237,6 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
>  	u32 desc_ver;
>  	unsigned long mmap_key;
>  	efi_memory_desc_t *memory_map, *runtime_map;
> -	unsigned long new_fdt_size;
>  	efi_status_t status;
>  	int runtime_entry_count = 0;
>  	struct efi_boot_memmap map;
> @@ -262,41 +265,29 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
>  	       "Exiting boot services and installing virtual address map...\n");
>
>  	map.map = &memory_map;
> +	status = efi_high_alloc(sys_table, MAX_FDT_SIZE, EFI_FDT_ALIGN,
> +				new_fdt_addr, max_addr);
> +	if (status != EFI_SUCCESS) {
> +		pr_efi_err(sys_table,
> +			   "Unable to allocate memory for new device tree.\n");
> +		goto fail;
> +	}
> +
>  	/*
> -	 * Estimate size of new FDT, and allocate memory for it. We
> -	 * will allocate a bigger buffer if this ends up being too
> -	 * small, so a rough guess is OK here.
> +	 * Now that we have done our final memory allocation (and free)
> +	 * we can get the memory map key needed for exit_boot_services().
>  	 */
> -	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);
> -		if (status != EFI_SUCCESS) {
> -			pr_efi_err(sys_table, "Unable to allocate memory for new device tree.\n");
> -			goto fail;
> -		}
> -
> -		status = update_fdt(sys_table,
> -				    (void *)fdt_addr, fdt_size,
> -				    (void *)*new_fdt_addr, new_fdt_size,
> -				    cmdline_ptr, initrd_addr, initrd_size);
> +	status = efi_get_memory_map(sys_table, &map);
> +	if (status != EFI_SUCCESS)
> +		goto fail_free_new_fdt;
>
> -		/* Succeeding the first time is the expected case. */
> -		if (status == EFI_SUCCESS)
> -			break;
> +	status = update_fdt(sys_table, (void *)fdt_addr, fdt_size,
> +			    (void *)*new_fdt_addr, MAX_FDT_SIZE, cmdline_ptr,
> +			    initrd_addr, initrd_size);
>
> -		if (status == EFI_BUFFER_TOO_SMALL) {
> -			/*
> -			 * We need to allocate more space for the new
> -			 * device tree, so free existing buffer that is
> -			 * too small.
> -			 */
> -			efi_free(sys_table, new_fdt_size, *new_fdt_addr);
> -			new_fdt_size += EFI_PAGE_SIZE;
> -		} else {
> -			pr_efi_err(sys_table, "Unable to construct new device tree.\n");
> -			goto fail_free_new_fdt;
> -		}
> +	if (status != EFI_SUCCESS) {
> +		pr_efi_err(sys_table, "Unable to construct new device tree.\n");
> +		goto fail_free_new_fdt;
>  	}
>
>  	priv.runtime_map = runtime_map;
> @@ -340,7 +331,7 @@ efi_status_t allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
>  	pr_efi_err(sys_table, "Exit boot services failed.\n");
>
>  fail_free_new_fdt:
> -	efi_free(sys_table, new_fdt_size, *new_fdt_addr);
> +	efi_free(sys_table, MAX_FDT_SIZE, *new_fdt_addr);
>
>  fail:
>  	sys_table->boottime->free_pool(runtime_map);
>


-- 
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH v2 2/2] efi: arm-stub: Round up FDT allocation to mapping size

[Ard Biesheuvel]

On 12 February 2017 at 20:10, Jeffrey Hugo <jhugo-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
> On 2/9/2017 2:42 PM, Ard Biesheuvel wrote:
>>
>> The FDT is mapped via a fixmap entry that is at least 2 MB in size and
>> 2 MB aligned on 4 KB page size kernels.
>>
>> On UEFI systems, the FDT allocation may share this 2 MB block with a
>> reserved region, or another memory region that we should never map,
>> unless we account for this in the size of the allocation (the alignment
>> is already 2 MB)
>>
>> So instead of taking guesses at the needed space, simply allocate 2 MB
>> immediately. The allocation will be recorded as a EFI_LOADER_DATA, and
>> the kernel only memblock_reserve()'s the actual size of the FDT, so the
>> unused space will be released to the kernel.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> ---
>
>
> Reviewed-By: Jeffrey Hugo <jhugo-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
>

Thanks, Jeffrey.

Mark: anything to add? Thanks.

>
>>  arch/arm64/include/asm/efi.h       |  1 +
>>  drivers/firmware/efi/libstub/fdt.c | 57 +++++++++-----------
>>  2 files changed, 25 insertions(+), 33 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
>> index 342e90d6d204..f642565fc1d0 100644
>> --- a/arch/arm64/include/asm/efi.h
>> +++ b/arch/arm64/include/asm/efi.h
>> @@ -1,6 +1,7 @@
>>  #ifndef _ASM_EFI_H
>>  #define _ASM_EFI_H
>>
>> +#include <asm/boot.h>
>>  #include <asm/cpufeature.h>
>>  #include <asm/io.h>
>>  #include <asm/mmu_context.h>
>> diff --git a/drivers/firmware/efi/libstub/fdt.c
>> b/drivers/firmware/efi/libstub/fdt.c
>> index 260c4b4b492e..41f457be64e8 100644
>> --- a/drivers/firmware/efi/libstub/fdt.c
>> +++ b/drivers/firmware/efi/libstub/fdt.c
>> @@ -206,6 +206,10 @@ static efi_status_t exit_boot_func(efi_system_table_t
>> *sys_table_arg,
>>         return update_fdt_memmap(p->new_fdt_addr, map);
>>  }
>>
>> +#ifndef MAX_FDT_SIZE
>> +#define MAX_FDT_SIZE   SZ_2M
>> +#endif
>> +
>>  /*
>>   * Allocate memory for a new FDT, then add EFI, commandline, and
>>   * initrd related fields to the FDT.  This routine increases the
>> @@ -233,7 +237,6 @@ efi_status_t
>> allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
>>         u32 desc_ver;
>>         unsigned long mmap_key;
>>         efi_memory_desc_t *memory_map, *runtime_map;
>> -       unsigned long new_fdt_size;
>>         efi_status_t status;
>>         int runtime_entry_count = 0;
>>         struct efi_boot_memmap map;
>> @@ -262,41 +265,29 @@ efi_status_t
>> allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
>>                "Exiting boot services and installing virtual address
>> map...\n");
>>
>>         map.map = &memory_map;
>> +       status = efi_high_alloc(sys_table, MAX_FDT_SIZE, EFI_FDT_ALIGN,
>> +                               new_fdt_addr, max_addr);
>> +       if (status != EFI_SUCCESS) {
>> +               pr_efi_err(sys_table,
>> +                          "Unable to allocate memory for new device
>> tree.\n");
>> +               goto fail;
>> +       }
>> +
>>         /*
>> -        * Estimate size of new FDT, and allocate memory for it. We
>> -        * will allocate a bigger buffer if this ends up being too
>> -        * small, so a rough guess is OK here.
>> +        * Now that we have done our final memory allocation (and free)
>> +        * we can get the memory map key needed for exit_boot_services().
>>          */
>> -       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);
>> -               if (status != EFI_SUCCESS) {
>> -                       pr_efi_err(sys_table, "Unable to allocate memory
>> for new device tree.\n");
>> -                       goto fail;
>> -               }
>> -
>> -               status = update_fdt(sys_table,
>> -                                   (void *)fdt_addr, fdt_size,
>> -                                   (void *)*new_fdt_addr, new_fdt_size,
>> -                                   cmdline_ptr, initrd_addr,
>> initrd_size);
>> +       status = efi_get_memory_map(sys_table, &map);
>> +       if (status != EFI_SUCCESS)
>> +               goto fail_free_new_fdt;
>>
>> -               /* Succeeding the first time is the expected case. */
>> -               if (status == EFI_SUCCESS)
>> -                       break;
>> +       status = update_fdt(sys_table, (void *)fdt_addr, fdt_size,
>> +                           (void *)*new_fdt_addr, MAX_FDT_SIZE,
>> cmdline_ptr,
>> +                           initrd_addr, initrd_size);
>>
>> -               if (status == EFI_BUFFER_TOO_SMALL) {
>> -                       /*
>> -                        * We need to allocate more space for the new
>> -                        * device tree, so free existing buffer that is
>> -                        * too small.
>> -                        */
>> -                       efi_free(sys_table, new_fdt_size, *new_fdt_addr);
>> -                       new_fdt_size += EFI_PAGE_SIZE;
>> -               } else {
>> -                       pr_efi_err(sys_table, "Unable to construct new
>> device tree.\n");
>> -                       goto fail_free_new_fdt;
>> -               }
>> +       if (status != EFI_SUCCESS) {
>> +               pr_efi_err(sys_table, "Unable to construct new device
>> tree.\n");
>> +               goto fail_free_new_fdt;
>>         }
>>
>>         priv.runtime_map = runtime_map;
>> @@ -340,7 +331,7 @@ efi_status_t
>> allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
>>         pr_efi_err(sys_table, "Exit boot services failed.\n");
>>
>>  fail_free_new_fdt:
>> -       efi_free(sys_table, new_fdt_size, *new_fdt_addr);
>> +       efi_free(sys_table, MAX_FDT_SIZE, *new_fdt_addr);
>>
>>  fail:
>>         sys_table->boottime->free_pool(runtime_map);
>>
>
>
> --
> Jeffrey Hugo
>
> Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies,
> Inc.
> Qualcomm Technologies, Inc. is a member of the
> Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH v2 2/2] efi: arm-stub: Round up FDT allocation to mapping size

[Ard Biesheuvel]

On 12 February 2017 at 20:10, Jeffrey Hugo <jhugo@codeaurora.org> wrote:
> On 2/9/2017 2:42 PM, Ard Biesheuvel wrote:
>>
>> The FDT is mapped via a fixmap entry that is at least 2 MB in size and
>> 2 MB aligned on 4 KB page size kernels.
>>
>> On UEFI systems, the FDT allocation may share this 2 MB block with a
>> reserved region, or another memory region that we should never map,
>> unless we account for this in the size of the allocation (the alignment
>> is already 2 MB)
>>
>> So instead of taking guesses at the needed space, simply allocate 2 MB
>> immediately. The allocation will be recorded as a EFI_LOADER_DATA, and
>> the kernel only memblock_reserve()'s the actual size of the FDT, so the
>> unused space will be released to the kernel.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>
>
> Reviewed-By: Jeffrey Hugo <jhugo@codeaurora.org>
>

Thanks, Jeffrey.

Mark: anything to add? Thanks.

>
>>  arch/arm64/include/asm/efi.h       |  1 +
>>  drivers/firmware/efi/libstub/fdt.c | 57 +++++++++-----------
>>  2 files changed, 25 insertions(+), 33 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
>> index 342e90d6d204..f642565fc1d0 100644
>> --- a/arch/arm64/include/asm/efi.h
>> +++ b/arch/arm64/include/asm/efi.h
>> @@ -1,6 +1,7 @@
>>  #ifndef _ASM_EFI_H
>>  #define _ASM_EFI_H
>>
>> +#include <asm/boot.h>
>>  #include <asm/cpufeature.h>
>>  #include <asm/io.h>
>>  #include <asm/mmu_context.h>
>> diff --git a/drivers/firmware/efi/libstub/fdt.c
>> b/drivers/firmware/efi/libstub/fdt.c
>> index 260c4b4b492e..41f457be64e8 100644
>> --- a/drivers/firmware/efi/libstub/fdt.c
>> +++ b/drivers/firmware/efi/libstub/fdt.c
>> @@ -206,6 +206,10 @@ static efi_status_t exit_boot_func(efi_system_table_t
>> *sys_table_arg,
>>         return update_fdt_memmap(p->new_fdt_addr, map);
>>  }
>>
>> +#ifndef MAX_FDT_SIZE
>> +#define MAX_FDT_SIZE   SZ_2M
>> +#endif
>> +
>>  /*
>>   * Allocate memory for a new FDT, then add EFI, commandline, and
>>   * initrd related fields to the FDT.  This routine increases the
>> @@ -233,7 +237,6 @@ efi_status_t
>> allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
>>         u32 desc_ver;
>>         unsigned long mmap_key;
>>         efi_memory_desc_t *memory_map, *runtime_map;
>> -       unsigned long new_fdt_size;
>>         efi_status_t status;
>>         int runtime_entry_count = 0;
>>         struct efi_boot_memmap map;
>> @@ -262,41 +265,29 @@ efi_status_t
>> allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
>>                "Exiting boot services and installing virtual address
>> map...\n");
>>
>>         map.map = &memory_map;
>> +       status = efi_high_alloc(sys_table, MAX_FDT_SIZE, EFI_FDT_ALIGN,
>> +                               new_fdt_addr, max_addr);
>> +       if (status != EFI_SUCCESS) {
>> +               pr_efi_err(sys_table,
>> +                          "Unable to allocate memory for new device
>> tree.\n");
>> +               goto fail;
>> +       }
>> +
>>         /*
>> -        * Estimate size of new FDT, and allocate memory for it. We
>> -        * will allocate a bigger buffer if this ends up being too
>> -        * small, so a rough guess is OK here.
>> +        * Now that we have done our final memory allocation (and free)
>> +        * we can get the memory map key needed for exit_boot_services().
>>          */
>> -       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);
>> -               if (status != EFI_SUCCESS) {
>> -                       pr_efi_err(sys_table, "Unable to allocate memory
>> for new device tree.\n");
>> -                       goto fail;
>> -               }
>> -
>> -               status = update_fdt(sys_table,
>> -                                   (void *)fdt_addr, fdt_size,
>> -                                   (void *)*new_fdt_addr, new_fdt_size,
>> -                                   cmdline_ptr, initrd_addr,
>> initrd_size);
>> +       status = efi_get_memory_map(sys_table, &map);
>> +       if (status != EFI_SUCCESS)
>> +               goto fail_free_new_fdt;
>>
>> -               /* Succeeding the first time is the expected case. */
>> -               if (status == EFI_SUCCESS)
>> -                       break;
>> +       status = update_fdt(sys_table, (void *)fdt_addr, fdt_size,
>> +                           (void *)*new_fdt_addr, MAX_FDT_SIZE,
>> cmdline_ptr,
>> +                           initrd_addr, initrd_size);
>>
>> -               if (status == EFI_BUFFER_TOO_SMALL) {
>> -                       /*
>> -                        * We need to allocate more space for the new
>> -                        * device tree, so free existing buffer that is
>> -                        * too small.
>> -                        */
>> -                       efi_free(sys_table, new_fdt_size, *new_fdt_addr);
>> -                       new_fdt_size += EFI_PAGE_SIZE;
>> -               } else {
>> -                       pr_efi_err(sys_table, "Unable to construct new
>> device tree.\n");
>> -                       goto fail_free_new_fdt;
>> -               }
>> +       if (status != EFI_SUCCESS) {
>> +               pr_efi_err(sys_table, "Unable to construct new device
>> tree.\n");
>> +               goto fail_free_new_fdt;
>>         }
>>
>>         priv.runtime_map = runtime_map;
>> @@ -340,7 +331,7 @@ efi_status_t
>> allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table,
>>         pr_efi_err(sys_table, "Exit boot services failed.\n");
>>
>>  fail_free_new_fdt:
>> -       efi_free(sys_table, new_fdt_size, *new_fdt_addr);
>> +       efi_free(sys_table, MAX_FDT_SIZE, *new_fdt_addr);
>>
>>  fail:
>>         sys_table->boottime->free_pool(runtime_map);
>>
>
>
> --
> Jeffrey Hugo
>
> Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies,
> Inc.
> Qualcomm Technologies, Inc. is a member of the
> Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH v2 2/2] efi: arm-stub: Round up FDT allocation to mapping size

[Timur Tabi]

On Thu, Feb 9, 2017 at 3:42 PM, Ard Biesheuvel
<ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> The FDT is mapped via a fixmap entry that is at least 2 MB in size and
> 2 MB aligned on 4 KB page size kernels.
>
> On UEFI systems, the FDT allocation may share this 2 MB block with a
> reserved region, or another memory region that we should never map,
> unless we account for this in the size of the allocation (the alignment
> is already 2 MB)
>
> So instead of taking guesses at the needed space, simply allocate 2 MB
> immediately. The allocation will be recorded as a EFI_LOADER_DATA, and
> the kernel only memblock_reserve()'s the actual size of the FDT, so the
> unused space will be released to the kernel.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

This patch is over a month old, and it's been reviewed and tested.  We
really need this in 4.11.  Please get this merged into 4.11-rc4.

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

[PATCH v2 2/2] efi: arm-stub: Round up FDT allocation to mapping size

[Timur Tabi]

On Thu, Feb 9, 2017 at 3:42 PM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> The FDT is mapped via a fixmap entry that is at least 2 MB in size and
> 2 MB aligned on 4 KB page size kernels.
>
> On UEFI systems, the FDT allocation may share this 2 MB block with a
> reserved region, or another memory region that we should never map,
> unless we account for this in the size of the allocation (the alignment
> is already 2 MB)
>
> So instead of taking guesses at the needed space, simply allocate 2 MB
> immediately. The allocation will be recorded as a EFI_LOADER_DATA, and
> the kernel only memblock_reserve()'s the actual size of the FDT, so the
> unused space will be released to the kernel.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

This patch is over a month old, and it's been reviewed and tested.  We
really need this in 4.11.  Please get this merged into 4.11-rc4.

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH v2 2/2] efi: arm-stub: Round up FDT allocation to mapping size

[Ard Biesheuvel]

On 20 March 2017 at 23:25, Timur Tabi <timur@codeaurora.org> wrote:
> On Thu, Feb 9, 2017 at 3:42 PM, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
>> The FDT is mapped via a fixmap entry that is at least 2 MB in size and
>> 2 MB aligned on 4 KB page size kernels.
>>
>> On UEFI systems, the FDT allocation may share this 2 MB block with a
>> reserved region, or another memory region that we should never map,
>> unless we account for this in the size of the allocation (the alignment
>> is already 2 MB)
>>
>> So instead of taking guesses at the needed space, simply allocate 2 MB
>> immediately. The allocation will be recorded as a EFI_LOADER_DATA, and
>> the kernel only memblock_reserve()'s the actual size of the FDT, so the
>> unused space will be released to the kernel.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> This patch is over a month old, and it's been reviewed and tested.  We
> really need this in 4.11.  Please get this merged into 4.11-rc4.
>

We never queue non-critical changes right before the merge window
opens, so it was queued for v4.12 instead.

If it is so important that this ends up in v4.11, you should have
mentioned this at the time, or over the course of the past four weeks.

[PATCH v2 2/2] efi: arm-stub: Round up FDT allocation to mapping size

[Ard Biesheuvel]

On 20 March 2017 at 23:25, Timur Tabi <timur@codeaurora.org> wrote:
> On Thu, Feb 9, 2017 at 3:42 PM, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
>> The FDT is mapped via a fixmap entry that is at least 2 MB in size and
>> 2 MB aligned on 4 KB page size kernels.
>>
>> On UEFI systems, the FDT allocation may share this 2 MB block with a
>> reserved region, or another memory region that we should never map,
>> unless we account for this in the size of the allocation (the alignment
>> is already 2 MB)
>>
>> So instead of taking guesses at the needed space, simply allocate 2 MB
>> immediately. The allocation will be recorded as a EFI_LOADER_DATA, and
>> the kernel only memblock_reserve()'s the actual size of the FDT, so the
>> unused space will be released to the kernel.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> This patch is over a month old, and it's been reviewed and tested.  We
> really need this in 4.11.  Please get this merged into 4.11-rc4.
>

We never queue non-critical changes right before the merge window
opens, so it was queued for v4.12 instead.

If it is so important that this ends up in v4.11, you should have
mentioned this at the time, or over the course of the past four weeks.