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

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

So this series does two things:
- It relaxes the FDT placement requirements, and 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

Ard Biesheuvel (5):
  of/fdt: allow FDT virtual address outside of linear direct mapping
  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         | 12 +++----
 arch/arm64/include/asm/efi.h            |  9 +++--
 arch/arm64/include/asm/fixmap.h         |  9 +++++
 arch/arm64/kernel/Makefile              |  1 +
 arch/arm64/kernel/efi-stub.c            | 38 ++++++++++++++++----
 arch/arm64/kernel/head.S                | 38 +-------------------
 arch/arm64/kernel/setup.c               | 62 +++++++++++++++++++++++++++++----
 drivers/firmware/efi/libstub/arm-stub.c |  2 +-
 drivers/firmware/efi/libstub/fdt.c      |  7 ++--
 drivers/of/fdt.c                        | 14 +++++++-
 10 files changed, 125 insertions(+), 67 deletions(-)

-- 
1.8.3.2

[PATCH 1/5] of/fdt: allow FDT virtual address outside of linear direct mapping

[Ard Biesheuvel]

The early FDT code reserves the physical region that contains the
FDT, and uses __pa() to calculate the FDT's physical address.
However, if the FDT is mapped outside of the linear direct mapping,
__pa() cannot be used.

So create a __weak default wrapper called fdt_virt_to_phys() around
__pa(), and use that instead. This allows architectures to drop in
their own virt/phys mapping for the FDT blob.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---

I am aware that __weak functions are generally frowned upon, but in this
case, I wonder if it is worth the trouble to add arch specific header files
so we can include them here.

 drivers/of/fdt.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 3a896c9aeb74..b10ce880000b 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -547,6 +547,18 @@ static int __init __fdt_scan_reserved_mem(unsigned long node, const char *uname,
 }
 
 /**
+ * fdt_virt_to_phys - translate a virtual address inside the FDT image
+ *                    to its corresponding physical address
+ *
+ * This needs to be overridden by the architecture if the virtual mapping
+ * of the FDT is separate from the linear direct mapping of system RAM
+ */
+phys_addr_t __weak __init fdt_virt_to_phys(void *virt)
+{
+	return __pa(virt);
+}
+
+/**
  * early_init_fdt_scan_reserved_mem() - create reserved memory regions
  *
  * This function grabs memory from early allocator for device exclusive use
@@ -562,7 +574,7 @@ void __init early_init_fdt_scan_reserved_mem(void)
 		return;
 
 	/* Reserve the dtb region */
-	early_init_dt_reserve_memory_arch(__pa(initial_boot_params),
+	early_init_dt_reserve_memory_arch(fdt_virt_to_phys(initial_boot_params),
 					  fdt_totalsize(initial_boot_params),
 					  0);
 
-- 
1.8.3.2

[PATCH 1/5] of/fdt: allow FDT virtual address outside of linear direct mapping

[Rob Herring]

On Tue, Mar 3, 2015 at 5:03 AM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> The early FDT code reserves the physical region that contains the
> FDT, and uses __pa() to calculate the FDT's physical address.
> However, if the FDT is mapped outside of the linear direct mapping,
> __pa() cannot be used.
>
> So create a __weak default wrapper called fdt_virt_to_phys() around
> __pa(), and use that instead. This allows architectures to drop in
> their own virt/phys mapping for the FDT blob.
>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>
> I am aware that __weak functions are generally frowned upon, but in this
> case, I wonder if it is worth the trouble to add arch specific header files
> so we can include them here.
>
>  drivers/of/fdt.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index 3a896c9aeb74..b10ce880000b 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -547,6 +547,18 @@ static int __init __fdt_scan_reserved_mem(unsigned long node, const char *uname,
>  }
>
>  /**
> + * fdt_virt_to_phys - translate a virtual address inside the FDT image
> + *                    to its corresponding physical address
> + *
> + * This needs to be overridden by the architecture if the virtual mapping
> + * of the FDT is separate from the linear direct mapping of system RAM
> + */
> +phys_addr_t __weak __init fdt_virt_to_phys(void *virt)
> +{
> +       return __pa(virt);
> +}
> +
> +/**
>   * early_init_fdt_scan_reserved_mem() - create reserved memory regions
>   *
>   * This function grabs memory from early allocator for device exclusive use
> @@ -562,7 +574,7 @@ void __init early_init_fdt_scan_reserved_mem(void)
>                 return;
>
>         /* Reserve the dtb region */
> -       early_init_dt_reserve_memory_arch(__pa(initial_boot_params),
> +       early_init_dt_reserve_memory_arch(fdt_virt_to_phys(initial_boot_params),

This is already a weak function call, so I'd rather change
early_init_dt_reserve_memory_arch to take the virt address and do the
conversion inside it. Or we could just pass both addresses from the
arch code to the core code.

Rob

[PATCH 1/5] of/fdt: allow FDT virtual address outside of linear direct mapping

[Ard Biesheuvel]

On 10 March 2015 at 22:47, Rob Herring <robh@kernel.org> wrote:
> On Tue, Mar 3, 2015 at 5:03 AM, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
>> The early FDT code reserves the physical region that contains the
>> FDT, and uses __pa() to calculate the FDT's physical address.
>> However, if the FDT is mapped outside of the linear direct mapping,
>> __pa() cannot be used.
>>
>> So create a __weak default wrapper called fdt_virt_to_phys() around
>> __pa(), and use that instead. This allows architectures to drop in
>> their own virt/phys mapping for the FDT blob.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>
>> I am aware that __weak functions are generally frowned upon, but in this
>> case, I wonder if it is worth the trouble to add arch specific header files
>> so we can include them here.
>>
>>  drivers/of/fdt.c | 14 +++++++++++++-
>>  1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
>> index 3a896c9aeb74..b10ce880000b 100644
>> --- a/drivers/of/fdt.c
>> +++ b/drivers/of/fdt.c
>> @@ -547,6 +547,18 @@ static int __init __fdt_scan_reserved_mem(unsigned long node, const char *uname,
>>  }
>>
>>  /**
>> + * fdt_virt_to_phys - translate a virtual address inside the FDT image
>> + *                    to its corresponding physical address
>> + *
>> + * This needs to be overridden by the architecture if the virtual mapping
>> + * of the FDT is separate from the linear direct mapping of system RAM
>> + */
>> +phys_addr_t __weak __init fdt_virt_to_phys(void *virt)
>> +{
>> +       return __pa(virt);
>> +}
>> +
>> +/**
>>   * early_init_fdt_scan_reserved_mem() - create reserved memory regions
>>   *
>>   * This function grabs memory from early allocator for device exclusive use
>> @@ -562,7 +574,7 @@ void __init early_init_fdt_scan_reserved_mem(void)
>>                 return;
>>
>>         /* Reserve the dtb region */
>> -       early_init_dt_reserve_memory_arch(__pa(initial_boot_params),
>> +       early_init_dt_reserve_memory_arch(fdt_virt_to_phys(initial_boot_params),
>
> This is already a weak function call, so I'd rather change
> early_init_dt_reserve_memory_arch to take the virt address and do the
> conversion inside it.
> Or we could just pass both addresses from the
> arch code to the core code.
>

Alternatively, could we just have a way to tell the core FDT code
/not/ to do the reservation at all? It would make sense in the
non-linear mapped FDT case to round up the reservation to the mapped
size so that the remainder is not mapped elsewhere with different
attributes, so it is not just the physical address but also the size
that is different, and it is trivial to add the memblock_reserve() to
the remapping code.

[PATCH 1/5] of/fdt: allow FDT virtual address outside of linear direct mapping

[Ard Biesheuvel]

On 11 March 2015 at 09:34, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 10 March 2015 at 22:47, Rob Herring <robh@kernel.org> wrote:
>> On Tue, Mar 3, 2015 at 5:03 AM, Ard Biesheuvel
>> <ard.biesheuvel@linaro.org> wrote:
>>> The early FDT code reserves the physical region that contains the
>>> FDT, and uses __pa() to calculate the FDT's physical address.
>>> However, if the FDT is mapped outside of the linear direct mapping,
>>> __pa() cannot be used.
>>>
>>> So create a __weak default wrapper called fdt_virt_to_phys() around
>>> __pa(), and use that instead. This allows architectures to drop in
>>> their own virt/phys mapping for the FDT blob.
>>>
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> ---
>>>
>>> I am aware that __weak functions are generally frowned upon, but in this
>>> case, I wonder if it is worth the trouble to add arch specific header files
>>> so we can include them here.
>>>
>>>  drivers/of/fdt.c | 14 +++++++++++++-
>>>  1 file changed, 13 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
>>> index 3a896c9aeb74..b10ce880000b 100644
>>> --- a/drivers/of/fdt.c
>>> +++ b/drivers/of/fdt.c
>>> @@ -547,6 +547,18 @@ static int __init __fdt_scan_reserved_mem(unsigned long node, const char *uname,
>>>  }
>>>
>>>  /**
>>> + * fdt_virt_to_phys - translate a virtual address inside the FDT image
>>> + *                    to its corresponding physical address
>>> + *
>>> + * This needs to be overridden by the architecture if the virtual mapping
>>> + * of the FDT is separate from the linear direct mapping of system RAM
>>> + */
>>> +phys_addr_t __weak __init fdt_virt_to_phys(void *virt)
>>> +{
>>> +       return __pa(virt);
>>> +}
>>> +
>>> +/**
>>>   * early_init_fdt_scan_reserved_mem() - create reserved memory regions
>>>   *
>>>   * This function grabs memory from early allocator for device exclusive use
>>> @@ -562,7 +574,7 @@ void __init early_init_fdt_scan_reserved_mem(void)
>>>                 return;
>>>
>>>         /* Reserve the dtb region */
>>> -       early_init_dt_reserve_memory_arch(__pa(initial_boot_params),
>>> +       early_init_dt_reserve_memory_arch(fdt_virt_to_phys(initial_boot_params),
>>
>> This is already a weak function call, so I'd rather change
>> early_init_dt_reserve_memory_arch to take the virt address and do the
>> conversion inside it.
>> Or we could just pass both addresses from the
>> arch code to the core code.
>>
>
> Alternatively, could we just have a way to tell the core FDT code
> /not/ to do the reservation at all? It would make sense in the
> non-linear mapped FDT case to round up the reservation to the mapped
> size so that the remainder is not mapped elsewhere with different
> attributes, so it is not just the physical address but also the size
> that is different, and it is trivial to add the memblock_reserve() to
> the remapping code.

Actually, having the FDT region retain duplicate mappings of physical
pages that are not covered by the FDT memory reservation itself is not
recognized as a big deal, so memblock_reserve()'ing the actual size of
the FDT is fine.

But still, what you are proposing (I think) is update the prototype of
early_init_dt_scan() to take an additional physical address, which
would also involve updating a handful of architectures to modify the
call sites (yay). And the alternative to change
early_init_dt_reserve_memory_arch() to take a virtual address won't
work for high memory.

So what about just doing this instead?

@@ -574,7 +562,8 @@ void __init early_init_fdt_scan_reserved_mem(void)
                return;

        /* Reserve the dtb region */
-       early_init_dt_reserve_memory_arch(fdt_virt_to_phys(initial_boot_params),
+       if (!IS_ENABLED(CONFIG_OF_HAVE_VIRT_REMAPPED_FDT))
+               early_init_dt_reserve_memory_arch(__pa(initial_boot_params),
                                          fdt_totalsize(initial_boot_params),
                                          0);

This way, it is left up to the architecture to decide which memory
(and how much) to reserve.

[PATCH 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 |  7 ++---
 arch/arm64/include/asm/fixmap.h |  9 ++++++
 arch/arm64/kernel/Makefile      |  1 +
 arch/arm64/kernel/head.S        | 38 +------------------------
 arch/arm64/kernel/setup.c       | 62 +++++++++++++++++++++++++++++++++++++----
 5 files changed, 70 insertions(+), 47 deletions(-)

diff --git a/Documentation/arm64/booting.txt b/Documentation/arm64/booting.txt
index f3c05b5f9f08..bdc35fc97ac8 100644
--- a/Documentation/arm64/booting.txt
+++ b/Documentation/arm64/booting.txt
@@ -45,10 +45,9 @@ 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.
 
 
 3. Decompress the kernel image
diff --git a/arch/arm64/include/asm/fixmap.h b/arch/arm64/include/asm/fixmap.h
index defa0ff98250..4ad240a60898 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,
 	__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 8ce88e08c030..66675d27fea3 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 e8420f635bd4..5c675a09116e 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>
@@ -307,14 +308,63 @@ static void __init setup_processor(void)
 #endif
 }
 
+static unsigned long const dt_virt_base = __fix_to_virt(FIX_FDT);
+static phys_addr_t dt_phys_base;
+
+phys_addr_t __init fdt_virt_to_phys(void *virt)
+{
+	return (phys_addr_t)virt - dt_virt_base + dt_phys_base;
+}
+
+static void *__init fixmap_remap_fdt(phys_addr_t dt_phys)
+{
+	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 a section mapping for the 2 MB 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 & (FIX_FDT_SIZE - 1));
+
+	if (IS_ENABLED(CONFIG_ARM64_64K_PAGES))
+		BUILD_BUG_ON(dt_virt_base >> PMD_SHIFT !=
+			     __fix_to_virt(FIX_BTMAP_BEGIN) >> PMD_SHIFT);
+	else
+		BUILD_BUG_ON(dt_virt_base >> 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);
+
+	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))) {
+	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)) {
 		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"
+			"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();
@@ -357,6 +407,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;
@@ -366,9 +419,6 @@ void __init setup_arch(char **cmdline_p)
 
 	*cmdline_p = boot_command_line;
 
-	early_fixmap_init();
-	early_ioremap_init();
-
 	parse_early_param();
 
 	/*
-- 
1.8.3.2

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

[Rob Herring]

On Tue, Mar 3, 2015 at 5:03 AM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> 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 |  7 ++---
>  arch/arm64/include/asm/fixmap.h |  9 ++++++
>  arch/arm64/kernel/Makefile      |  1 +
>  arch/arm64/kernel/head.S        | 38 +------------------------
>  arch/arm64/kernel/setup.c       | 62 +++++++++++++++++++++++++++++++++++++----
>  5 files changed, 70 insertions(+), 47 deletions(-)
>
> diff --git a/Documentation/arm64/booting.txt b/Documentation/arm64/booting.txt
> index f3c05b5f9f08..bdc35fc97ac8 100644
> --- a/Documentation/arm64/booting.txt
> +++ b/Documentation/arm64/booting.txt
> @@ -45,10 +45,9 @@ 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.
>
>
>  3. Decompress the kernel image
> diff --git a/arch/arm64/include/asm/fixmap.h b/arch/arm64/include/asm/fixmap.h
> index defa0ff98250..4ad240a60898 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.
> +        */

We should not fix a location restriction by creating a size
restriction. You could embed firmware images within a DTB (which I
think PPC does).

> +#define FIX_FDT_SIZE           SZ_2M
> +       FIX_FDT_END,
> +       FIX_FDT = FIX_FDT_END + FIX_FDT_SIZE / PAGE_SIZE - 1,
> +
>         FIX_EARLYCON_MEM_BASE,
>         __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 8ce88e08c030..66675d27fea3 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 e8420f635bd4..5c675a09116e 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>
> @@ -307,14 +308,63 @@ static void __init setup_processor(void)
>  #endif
>  }
>
> +static unsigned long const dt_virt_base = __fix_to_virt(FIX_FDT);
> +static phys_addr_t dt_phys_base;
> +
> +phys_addr_t __init fdt_virt_to_phys(void *virt)
> +{
> +       return (phys_addr_t)virt - dt_virt_base + dt_phys_base;
> +}
> +
> +static void *__init fixmap_remap_fdt(phys_addr_t dt_phys)
> +{
> +       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 a section mapping for the 2 MB 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 & (FIX_FDT_SIZE - 1));
> +
> +       if (IS_ENABLED(CONFIG_ARM64_64K_PAGES))
> +               BUILD_BUG_ON(dt_virt_base >> PMD_SHIFT !=
> +                            __fix_to_virt(FIX_BTMAP_BEGIN) >> PMD_SHIFT);
> +       else
> +               BUILD_BUG_ON(dt_virt_base >> 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);
> +
> +       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))) {
> +       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)) {
>                 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"
> +                       "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();
> @@ -357,6 +407,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;
> @@ -366,9 +419,6 @@ void __init setup_arch(char **cmdline_p)
>
>         *cmdline_p = boot_command_line;
>
> -       early_fixmap_init();
> -       early_ioremap_init();
> -
>         parse_early_param();
>
>         /*
> --
> 1.8.3.2
>

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

[Ard Biesheuvel]

On 10 March 2015 at 22:37, Rob Herring <robh@kernel.org> wrote:
> On Tue, Mar 3, 2015 at 5:03 AM, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
>> 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 |  7 ++---
>>  arch/arm64/include/asm/fixmap.h |  9 ++++++
>>  arch/arm64/kernel/Makefile      |  1 +
>>  arch/arm64/kernel/head.S        | 38 +------------------------
>>  arch/arm64/kernel/setup.c       | 62 +++++++++++++++++++++++++++++++++++++----
>>  5 files changed, 70 insertions(+), 47 deletions(-)
>>
>> diff --git a/Documentation/arm64/booting.txt b/Documentation/arm64/booting.txt
>> index f3c05b5f9f08..bdc35fc97ac8 100644
>> --- a/Documentation/arm64/booting.txt
>> +++ b/Documentation/arm64/booting.txt
>> @@ -45,10 +45,9 @@ 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.
>>
>>
>>  3. Decompress the kernel image
>> diff --git a/arch/arm64/include/asm/fixmap.h b/arch/arm64/include/asm/fixmap.h
>> index defa0ff98250..4ad240a60898 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.
>> +        */
>
> We should not fix a location restriction by creating a size
> restriction. You could embed firmware images within a DTB (which I
> think PPC does).
>

The size restriction existed on arm64 before this patch, so I didn't
think twice about it.
So what would be a reasonable upper bound? We could go up to ~256 MB
without much trouble, but I guess that's a bit excessive, no?


>> +#define FIX_FDT_SIZE           SZ_2M
>> +       FIX_FDT_END,
>> +       FIX_FDT = FIX_FDT_END + FIX_FDT_SIZE / PAGE_SIZE - 1,
>> +
>>         FIX_EARLYCON_MEM_BASE,
>>         __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 8ce88e08c030..66675d27fea3 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 e8420f635bd4..5c675a09116e 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>
>> @@ -307,14 +308,63 @@ static void __init setup_processor(void)
>>  #endif
>>  }
>>
>> +static unsigned long const dt_virt_base = __fix_to_virt(FIX_FDT);
>> +static phys_addr_t dt_phys_base;
>> +
>> +phys_addr_t __init fdt_virt_to_phys(void *virt)
>> +{
>> +       return (phys_addr_t)virt - dt_virt_base + dt_phys_base;
>> +}
>> +
>> +static void *__init fixmap_remap_fdt(phys_addr_t dt_phys)
>> +{
>> +       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 a section mapping for the 2 MB 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 & (FIX_FDT_SIZE - 1));
>> +
>> +       if (IS_ENABLED(CONFIG_ARM64_64K_PAGES))
>> +               BUILD_BUG_ON(dt_virt_base >> PMD_SHIFT !=
>> +                            __fix_to_virt(FIX_BTMAP_BEGIN) >> PMD_SHIFT);
>> +       else
>> +               BUILD_BUG_ON(dt_virt_base >> 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);
>> +
>> +       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))) {
>> +       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)) {
>>                 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"
>> +                       "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();
>> @@ -357,6 +407,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;
>> @@ -366,9 +419,6 @@ void __init setup_arch(char **cmdline_p)
>>
>>         *cmdline_p = boot_command_line;
>>
>> -       early_fixmap_init();
>> -       early_ioremap_init();
>> -
>>         parse_early_param();
>>
>>         /*
>> --
>> 1.8.3.2
>>

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

[Mark Rutland]

> >> +       /*
> >> +        * 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.
> >> +        */
> >
> > We should not fix a location restriction by creating a size
> > restriction. You could embed firmware images within a DTB (which I
> > think PPC does).
> >
> 
> The size restriction existed on arm64 before this patch, so I didn't
> think twice about it.
> So what would be a reasonable upper bound? We could go up to ~256 MB
> without much trouble, but I guess that's a bit excessive, no?

Given the existing code had the same 2MB restriction (documented in
booting.txt), I think retaining that limitation for now is fine unless
we have some reasonable example of a DTB approaching or exceeding 2MB.

As far as I am aware, on arm64 we're not currently embeddeding FW images
in the DTB passed to the kernel, and I'm not sure why we would.
Anything that's critical to the system and requires FW should be
initialised prior to the kernel, and if we need FW to drive a particular
device I'd expect we'd store that in the filesystem (or perhaps
initrd/initramfs).

Mark.

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

[Ard Biesheuvel]

On 11 March 2015 at 10:50, Mark Rutland <mark.rutland@arm.com> wrote:
>> >> +       /*
>> >> +        * 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.
>> >> +        */
>> >
>> > We should not fix a location restriction by creating a size
>> > restriction. You could embed firmware images within a DTB (which I
>> > think PPC does).
>> >
>>
>> The size restriction existed on arm64 before this patch, so I didn't
>> think twice about it.
>> So what would be a reasonable upper bound? We could go up to ~256 MB
>> without much trouble, but I guess that's a bit excessive, no?
>
> Given the existing code had the same 2MB restriction (documented in
> booting.txt), I think retaining that limitation for now is fine unless
> we have some reasonable example of a DTB approaching or exceeding 2MB.
>
> As far as I am aware, on arm64 we're not currently embeddeding FW images
> in the DTB passed to the kernel, and I'm not sure why we would.
> Anything that's critical to the system and requires FW should be
> initialised prior to the kernel, and if we need FW to drive a particular
> device I'd expect we'd store that in the filesystem (or perhaps
> initrd/initramfs).
>

I agree with Mark's observation that 2 MB seems reasonable at the
moment, but on the other hand, the code only requires minor tweaking
to make the limit configurable simply by changing the FIX_FDT_SIZE
define, so I don't mind changing that.

-- 
Ard.

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

[Mark Rutland]

> I agree with Mark's observation that 2 MB seems reasonable at the
> moment, but on the other hand, the code only requires minor tweaking
> to make the limit configurable simply by changing the FIX_FDT_SIZE
> define, so I don't mind changing that.

We should have a common limit rather than a configurable option; there's
no point building a compatibility nightmare for ourselves when the only
thing we lose is a few entries in the fixmap.

I'm happy to bump the max size for the DTB, but I'd like to see a
reasonable example of a DTB approaching or exceeding that first.

Mark.

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

[Rob Herring]

On Wed, Mar 11, 2015 at 4:50 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>> >> +       /*
>> >> +        * 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.
>> >> +        */
>> >
>> > We should not fix a location restriction by creating a size
>> > restriction. You could embed firmware images within a DTB (which I
>> > think PPC does).
>> >
>>
>> The size restriction existed on arm64 before this patch, so I didn't
>> think twice about it.
>> So what would be a reasonable upper bound? We could go up to ~256 MB
>> without much trouble, but I guess that's a bit excessive, no?
>
> Given the existing code had the same 2MB restriction (documented in
> booting.txt), I think retaining that limitation for now is fine unless
> we have some reasonable example of a DTB approaching or exceeding 2MB.
>
> As far as I am aware, on arm64 we're not currently embeddeding FW images
> in the DTB passed to the kernel, and I'm not sure why we would.
> Anything that's critical to the system and requires FW should be
> initialised prior to the kernel, and if we need FW to drive a particular
> device I'd expect we'd store that in the filesystem (or perhaps
> initrd/initramfs).

Okay, as long as there is an easy path to growing the region which
seems there is.

Rob

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

[Mark Rutland]

Hi Ard,

The below is modulo Rob's comments regarding fdt_to_phys and the
associated memory reservation. I'm not too worried where those live.

On Tue, Mar 03, 2015 at 11:03:47AM +0000, Ard Biesheuvel wrote:
> 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.

Nit: s/anymore/any more/

> 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 |  7 ++---
>  arch/arm64/include/asm/fixmap.h |  9 ++++++
>  arch/arm64/kernel/Makefile      |  1 +
>  arch/arm64/kernel/head.S        | 38 +------------------------
>  arch/arm64/kernel/setup.c       | 62 +++++++++++++++++++++++++++++++++++++----
>  5 files changed, 70 insertions(+), 47 deletions(-)
> 
> diff --git a/Documentation/arm64/booting.txt b/Documentation/arm64/booting.txt
> index f3c05b5f9f08..bdc35fc97ac8 100644
> --- a/Documentation/arm64/booting.txt
> +++ b/Documentation/arm64/booting.txt
> @@ -45,10 +45,9 @@ 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.

As we do elsewhere in booting.txt I'd prefer that we kept a note
regarding the restriction expected by older kernels, so bootloader/VM
authors can do the right thing for those on a best-effort basis.

[...]

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

I was going to say it would be nice to keep these ordered, but I see
from the rest of the includes that's a foregone hope. Never mind :(

[...]

> +static unsigned long const dt_virt_base = __fix_to_virt(FIX_FDT);

I'd prefer "static const unsigned long".

[...]

>  static void __init setup_machine_fdt(phys_addr_t dt_phys)
>  {
> -	if (!dt_phys || !early_init_dt_scan(phys_to_virt(dt_phys))) {
> +	void *dt_virt = NULL;
> +
> +	if (dt_phys && (dt_phys & 7) == 0)
> +		dt_virt = fixmap_remap_fdt(dt_phys);
> +

It might be worth checking that dt_phys is sufficiently far from the end
of a 2MB boundary that we can read the totalsize field below. Trivially
that means 8 bytes below, the header is 40 bytes, and any real DTB will
be larger than that.

It's a shame the arley DTB verification functions don't take a limit
parameter or we could prevent them from making potentially bad accesses.

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

I'm surprised the toolchain doesn't scream about dt_phys being a
phys_addr_t rather than a pointer here, given that's alway been wrong. I
guess the early_print wrapper managed to hide that from us -- can we
nuke that and use pr_crit here? 

With that we'd need to use %pa for the phys_addr_t, passing &dt_phys
rather than dt_phys.

Other than those points, this looks good to me.

Thanks,
Mark.

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

[Ard Biesheuvel]

On 11 March 2015 at 11:43, Mark Rutland <mark.rutland@arm.com> wrote:
> Hi Ard,
>
> The below is modulo Rob's comments regarding fdt_to_phys and the
> associated memory reservation. I'm not too worried where those live.
>
> On Tue, Mar 03, 2015 at 11:03:47AM +0000, Ard Biesheuvel wrote:
>> 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.
>
> Nit: s/anymore/any more/
>
>> 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 |  7 ++---
>>  arch/arm64/include/asm/fixmap.h |  9 ++++++
>>  arch/arm64/kernel/Makefile      |  1 +
>>  arch/arm64/kernel/head.S        | 38 +------------------------
>>  arch/arm64/kernel/setup.c       | 62 +++++++++++++++++++++++++++++++++++++----
>>  5 files changed, 70 insertions(+), 47 deletions(-)
>>
>> diff --git a/Documentation/arm64/booting.txt b/Documentation/arm64/booting.txt
>> index f3c05b5f9f08..bdc35fc97ac8 100644
>> --- a/Documentation/arm64/booting.txt
>> +++ b/Documentation/arm64/booting.txt
>> @@ -45,10 +45,9 @@ 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.
>
> As we do elsewhere in booting.txt I'd prefer that we kept a note
> regarding the restriction expected by older kernels, so bootloader/VM
> authors can do the right thing for those on a best-effort basis.
>

OK

>> --- 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>
>
> I was going to say it would be nice to keep these ordered, but I see
> from the rest of the includes that's a foregone hope. Never mind :(
>
> [...]
>
>> +static unsigned long const dt_virt_base = __fix_to_virt(FIX_FDT);
>
> I'd prefer "static const unsigned long".
>
> [...]
>
>>  static void __init setup_machine_fdt(phys_addr_t dt_phys)
>>  {
>> -     if (!dt_phys || !early_init_dt_scan(phys_to_virt(dt_phys))) {
>> +     void *dt_virt = NULL;
>> +
>> +     if (dt_phys && (dt_phys & 7) == 0)
>> +             dt_virt = fixmap_remap_fdt(dt_phys);
>> +
>
> It might be worth checking that dt_phys is sufficiently far from the end
> of a 2MB boundary that we can read the totalsize field below. Trivially
> that means 8 bytes below, the header is 40 bytes, and any real DTB will
> be larger than that.
>

Y i kind of cheated by putting the alignment check first: this means
the first 8 bytes will always be readable


> It's a shame the arley DTB verification functions don't take a limit
> parameter or we could prevent them from making potentially bad accesses.
>
>> +     /*
>> +      * 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)) {
>>               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"
>> +                     "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);
>
> I'm surprised the toolchain doesn't scream about dt_phys being a
> phys_addr_t rather than a pointer here, given that's alway been wrong. I
> guess the early_print wrapper managed to hide that from us -- can we
> nuke that and use pr_crit here?
>

Sure, why not. Nobody is going to be able to read it anyway, I
suppose, unless you are dumping __log_buf from gdb

> With that we'd need to use %pa for the phys_addr_t, passing &dt_phys
> rather than dt_phys.
>
> Other than those points, this looks good to me.
>

Thanks

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

[Mark Rutland]

> >>  static void __init setup_machine_fdt(phys_addr_t dt_phys)
> >>  {
> >> -     if (!dt_phys || !early_init_dt_scan(phys_to_virt(dt_phys))) {
> >> +     void *dt_virt = NULL;
> >> +
> >> +     if (dt_phys && (dt_phys & 7) == 0)
> >> +             dt_virt = fixmap_remap_fdt(dt_phys);
> >> +
> >
> > It might be worth checking that dt_phys is sufficiently far from the end
> > of a 2MB boundary that we can read the totalsize field below. Trivially
> > that means 8 bytes below, the header is 40 bytes, and any real DTB will
> > be larger than that.
> >
> 
> Y i kind of cheated by putting the alignment check first: this means
> the first 8 bytes will always be readable

Ah, good point. Given that it could possibly explode in the core DT
verification I guess it's not too big a deal either way.

> > It's a shame the arley DTB verification functions don't take a limit
> > parameter or we could prevent them from making potentially bad accesses.
> >
> >> +     /*
> >> +      * 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)) {
> >>               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"
> >> +                     "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);
> >
> > I'm surprised the toolchain doesn't scream about dt_phys being a
> > phys_addr_t rather than a pointer here, given that's alway been wrong. I
> > guess the early_print wrapper managed to hide that from us -- can we
> > nuke that and use pr_crit here?
> >
> 
> Sure, why not. Nobody is going to be able to read it anyway, I
> suppose, unless you are dumping __log_buf from gdb

I was under the mistaken impression you could get ouptut if you'd
hardcoded earlycon=whatever with CNFIG_CMDLINE, but obviously that's not
the case given we won't have called parse_early_param() yet.

I'd like to nuke early_print regardless.

Thanks.
Mark.

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

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 bdc35fc97ac8..49f17b1632f1 100644
--- a/Documentation/arm64/booting.txt
+++ b/Documentation/arm64/booting.txt
@@ -112,8 +112,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 3/5] arm64: Documentation: clarify Image placement in physical RAM

[Mark Rutland]

On Tue, Mar 03, 2015 at 11:03:48AM +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.
> 
> 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 bdc35fc97ac8..49f17b1632f1 100644
> --- a/Documentation/arm64/booting.txt
> +++ b/Documentation/arm64/booting.txt
> @@ -112,8 +112,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.

This is correct.

I had a go at rewording this so as to move all the address restrictions
together, but I couldn't make it any clearer than the wording above. So
FWIW:

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

Mark.

[PATCH 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 Image is placed in such a way that the early init code will
always be able to map it. This means the entire static memory footprint
of 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 | 38 ++++++++++++++++++++++++++++++++------
 1 file changed, 32 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/kernel/efi-stub.c b/arch/arm64/kernel/efi-stub.c
index f5374065ad53..5f8175979be8 100644
--- a/arch/arm64/kernel/efi-stub.c
+++ b/arch/arm64/kernel/efi-stub.c
@@ -22,14 +22,40 @@ efi_status_t __init handle_kernel_image(efi_system_table_t *sys_table,
 					efi_loaded_image_t *image)
 {
 	efi_status_t status;
-	unsigned long kernel_size, kernel_memsize = 0;
+	unsigned long kernel_size, kernel_memsize;
+	unsigned long preferred_offset;
 
-	/* 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);
+	kernel_memsize = kernel_size + (_end - _edata);
+
+	/*
+	 * The kernel Image should be located as close as possible to the base
+	 * of system RAM, but 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) {
+		unsigned long alloc_size = kernel_memsize + TEXT_OFFSET;
+		unsigned long alloc_align = SZ_2M;
+
+again:
+		status = efi_low_alloc(sys_table, alloc_size, alloc_align,
+				       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);
+			alloc_align = roundup_pow_of_two(alloc_size);
+			goto again;
+		}
 		if (status != EFI_SUCCESS) {
 			pr_efi_err(sys_table, "Failed to relocate kernel\n");
 			return status;
-- 
1.8.3.2

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

[Mark Rutland]

Hi Ard,

On Tue, Mar 03, 2015 at 11:03:49AM +0000, Ard Biesheuvel wrote:
> Update the Image placement logic used by the stub to make absolutely
> sure that Image is placed in such a way that the early init code will

Minor grammatical nits:

s/that Image/that the Image/

s/in such a way that/such that/

> always be able to map it. This means the entire static memory footprint
> of Image should be inside the same naturally aligned 512 MB region.

s/Image/the Image/

> 
> 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 | 38 ++++++++++++++++++++++++++++++++------
>  1 file changed, 32 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm64/kernel/efi-stub.c b/arch/arm64/kernel/efi-stub.c
> index f5374065ad53..5f8175979be8 100644
> --- a/arch/arm64/kernel/efi-stub.c
> +++ b/arch/arm64/kernel/efi-stub.c
> @@ -22,14 +22,40 @@ efi_status_t __init handle_kernel_image(efi_system_table_t *sys_table,
>  					efi_loaded_image_t *image)
>  {
>  	efi_status_t status;
> -	unsigned long kernel_size, kernel_memsize = 0;
> +	unsigned long kernel_size, kernel_memsize;
> +	unsigned long preferred_offset;
>  
> -	/* 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);
> +	kernel_memsize = kernel_size + (_end - _edata);
> +
> +	/*
> +	 * The kernel Image should be located as close as possible to the base
> +	 * of system RAM, but must not cross a 512 MB alignment boundary.

It might be better to say "but its static memory footprint must not
cross a 512MB boundary" or something to that effect, to avoid any
ambiguity regarding the Image binary vs the runtime memory footprint
thereof.

> +	 */
> +	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) {
> +		unsigned long alloc_size = kernel_memsize + TEXT_OFFSET;

This could be const.

> +		unsigned long alloc_align = SZ_2M;
> +
> +again:
> +		status = efi_low_alloc(sys_table, alloc_size, alloc_align,
> +				       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);
> +			alloc_align = roundup_pow_of_two(alloc_size);
> +			goto again;
> +		}

If you move this check after the status != EFI_SUCCESS check below then
you don't need to check status == EFI_SUCCESS, which would make the
condition a little more legible.

Other than those comments this looks sane to me.

Thanks,
Mark.

>  		if (status != EFI_SUCCESS) {
>  			pr_efi_err(sys_table, "Failed to relocate kernel\n");
>  			return status;
> -- 
> 1.8.3.2
> 
> 

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

[Ard Biesheuvel]

On 11 March 2015 at 12:50, Mark Rutland <mark.rutland@arm.com> wrote:
> Hi Ard,
>
> On Tue, Mar 03, 2015 at 11:03:49AM +0000, Ard Biesheuvel wrote:
>> Update the Image placement logic used by the stub to make absolutely
>> sure that Image is placed in such a way that the early init code will
>
> Minor grammatical nits:
>
> s/that Image/that the Image/
>
> s/in such a way that/such that/
>
>> always be able to map it. This means the entire static memory footprint
>> of Image should be inside the same naturally aligned 512 MB region.
>
> s/Image/the Image/
>

ok

>>
>> 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 | 38 ++++++++++++++++++++++++++++++++------
>>  1 file changed, 32 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/efi-stub.c b/arch/arm64/kernel/efi-stub.c
>> index f5374065ad53..5f8175979be8 100644
>> --- a/arch/arm64/kernel/efi-stub.c
>> +++ b/arch/arm64/kernel/efi-stub.c
>> @@ -22,14 +22,40 @@ efi_status_t __init handle_kernel_image(efi_system_table_t *sys_table,
>>                                       efi_loaded_image_t *image)
>>  {
>>       efi_status_t status;
>> -     unsigned long kernel_size, kernel_memsize = 0;
>> +     unsigned long kernel_size, kernel_memsize;
>> +     unsigned long preferred_offset;
>>
>> -     /* 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);
>> +     kernel_memsize = kernel_size + (_end - _edata);
>> +
>> +     /*
>> +      * The kernel Image should be located as close as possible to the base
>> +      * of system RAM, but must not cross a 512 MB alignment boundary.
>
> It might be better to say "but its static memory footprint must not
> cross a 512MB boundary" or something to that effect, to avoid any
> ambiguity regarding the Image binary vs the runtime memory footprint
> thereof.
>

ok

>> +      */
>> +     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) {
>> +             unsigned long alloc_size = kernel_memsize + TEXT_OFFSET;
>
> This could be const.
>

yep

>> +             unsigned long alloc_align = SZ_2M;
>> +
>> +again:
>> +             status = efi_low_alloc(sys_table, alloc_size, alloc_align,
>> +                                    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);
>> +                     alloc_align = roundup_pow_of_two(alloc_size);
>> +                     goto again;
>> +             }
>
> If you move this check after the status != EFI_SUCCESS check below then
> you don't need to check status == EFI_SUCCESS, which would make the
> condition a little more legible.
>

Actually, I was going to drop the goto and put another invocation of
efi_low_alloc() inside the if {} block, that makes it a lot clearer
imo


> Other than those comments this looks sane to me.
>

Thanks

[PATCH 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
doing so.

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

diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
index ef572206f1c3..68f7bb2ddad7 100644
--- a/arch/arm64/include/asm/efi.h
+++ b/arch/arm64/include/asm/efi.h
@@ -39,12 +39,11 @@ 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 MAX_FDT_OFFSET		ULONG_MAX
 
 #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..c58c21c22dbe 100644
--- a/drivers/firmware/efi/libstub/arm-stub.c
+++ b/drivers/firmware/efi/libstub/arm-stub.c
@@ -269,7 +269,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, MAX_FDT_OFFSET,
 				initrd_addr, initrd_size, cmdline_ptr,
 				fdt_addr, fdt_size);
 
diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
index 91da56c4fd54..1a44d410a63b 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
@@ -223,7 +219,8 @@ 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,
+		status = efi_high_alloc(sys_table, new_fdt_size,
+					EFI_FDT_ALIGN(new_fdt_size),
 					new_fdt_addr, max_addr);
 		if (status != EFI_SUCCESS) {
 			pr_efi_err(sys_table, "Unable to allocate memory for new device tree.\n");
-- 
1.8.3.2

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

[Mark Rutland]

On Tue, Mar 03, 2015 at 11:03:50AM +0000, Ard Biesheuvel wrote:
> 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
> doing so.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm64/include/asm/efi.h            | 9 ++++-----
>  drivers/firmware/efi/libstub/arm-stub.c | 2 +-
>  drivers/firmware/efi/libstub/fdt.c      | 7 ++-----
>  3 files changed, 7 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
> index ef572206f1c3..68f7bb2ddad7 100644
> --- a/arch/arm64/include/asm/efi.h
> +++ b/arch/arm64/include/asm/efi.h
> @@ -39,12 +39,11 @@ 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 MAX_FDT_OFFSET		ULONG_MAX
>  
>  #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..c58c21c22dbe 100644
> --- a/drivers/firmware/efi/libstub/arm-stub.c
> +++ b/drivers/firmware/efi/libstub/arm-stub.c
> @@ -269,7 +269,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, MAX_FDT_OFFSET,

Do we still need the max_addr parameter and MAX_FDT_OFFSET, or can we
just have allocate_new_fdt_and_exit_boot assume it can allocate anywhere
in the physical address space (or ULONG_MAX if we want to allocate below
4GB whenever we get 32-bit EFI stub support).

Otherwise this looks fine.

Mark.

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

[Ard Biesheuvel]

On 11 March 2015 at 13:09, Mark Rutland <mark.rutland@arm.com> wrote:
> On Tue, Mar 03, 2015 at 11:03:50AM +0000, Ard Biesheuvel wrote:
>> 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
>> doing so.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  arch/arm64/include/asm/efi.h            | 9 ++++-----
>>  drivers/firmware/efi/libstub/arm-stub.c | 2 +-
>>  drivers/firmware/efi/libstub/fdt.c      | 7 ++-----
>>  3 files changed, 7 insertions(+), 11 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
>> index ef572206f1c3..68f7bb2ddad7 100644
>> --- a/arch/arm64/include/asm/efi.h
>> +++ b/arch/arm64/include/asm/efi.h
>> @@ -39,12 +39,11 @@ 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 MAX_FDT_OFFSET               ULONG_MAX
>>
>>  #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..c58c21c22dbe 100644
>> --- a/drivers/firmware/efi/libstub/arm-stub.c
>> +++ b/drivers/firmware/efi/libstub/arm-stub.c
>> @@ -269,7 +269,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, MAX_FDT_OFFSET,
>
> Do we still need the max_addr parameter and MAX_FDT_OFFSET, or can we
> just have allocate_new_fdt_and_exit_boot assume it can allocate anywhere
> in the physical address space (or ULONG_MAX if we want to allocate below
> 4GB whenever we get 32-bit EFI stub support).
>

Yes, I could probably drop the parameter and hardcode ULONG_MAX in there.
(32-bit EFI stub support is still on our todo list, but nobody is
actually working on it atm)

-- 
Ard.

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

[Ard Biesheuvel]

On 3 March 2015 at 12:03, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> 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.
>
> So this series does two things:
> - It relaxes the FDT placement requirements, and 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
>

Anyone care to comment on these patches?

Patch #4 is arguably a bug fix, although it is unlikely that the base
of visible (to EL2) DRAM would be so close to a 512 MB boundary that
the Image would end up astride it.

However, the FDT placement issue is more likely to cause trouble: if
EL3 takes a couple of megs from the bottom of DRAM to give to the
secure world (in which case it is perfectly legal for it not to turn
up in the UEFI memory map at all), the current FDT placement logic
will prefer an offset which sits in the next 512 MB block, putting the
FDT out of reach for the initial page tables.

If we prefer not to use the fixmap region for the FDT,  the latter
issue should be addressed in a different way.

Regards,
Ard.

> Ard Biesheuvel (5):
>   of/fdt: allow FDT virtual address outside of linear direct mapping
>   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         | 12 +++----
>  arch/arm64/include/asm/efi.h            |  9 +++--
>  arch/arm64/include/asm/fixmap.h         |  9 +++++
>  arch/arm64/kernel/Makefile              |  1 +
>  arch/arm64/kernel/efi-stub.c            | 38 ++++++++++++++++----
>  arch/arm64/kernel/head.S                | 38 +-------------------
>  arch/arm64/kernel/setup.c               | 62 +++++++++++++++++++++++++++++----
>  drivers/firmware/efi/libstub/arm-stub.c |  2 +-
>  drivers/firmware/efi/libstub/fdt.c      |  7 ++--
>  drivers/of/fdt.c                        | 14 +++++++-
>  10 files changed, 125 insertions(+), 67 deletions(-)
>
> --
> 1.8.3.2
>

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

[Mark Rutland]

Hi Ard,

On Tue, Mar 10, 2015 at 10:51:03AM +0000, Ard Biesheuvel wrote:
> On 3 March 2015 at 12:03, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> > 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.
> >
> > So this series does two things:
> > - It relaxes the FDT placement requirements, and 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
> >
> 
> Anyone care to comment on these patches?

Sorry; I'd been intending to look at these but haven't yet had the
chance to give them a thorough review. From my glances so far they look
good, though.

I'll try to give this a proper look shortly.

[...]

> If we prefer not to use the fixmap region for the FDT,  the latter
> issue should be addressed in a different way.

I'm quite keen on using the fixmap for the FDT. It makes matters simpler
for other bootloaders too, and means we can do more thorough sanity
checking from C code (which is easier than in asm).

Thanks,
Mark.