[PATCH 0/2] arm64/efi: fix kernel allocation at base of DRAM

[PATCH 0/2] arm64/efi: fix kernel allocation at base of DRAM

[Ard Biesheuvel]

This series provides an alternative to 'arm64/efi: prefer AllocatePages() over
efi_low_alloc() for vmlinux', of which a v2 was sent out on July 24th.

This series updates efi_low_alloc() to support doing allocations that start
at a fixed offset from an aligned boundary, as is required for allocating the
arm64 kernel 512 KB above a 2 MB boundary.

With these changes, the arm64 placement code can be simplified, and will deal
better with the HiKey platform, whose DRAM starts at 0x0, and APM Mustang, whose
firmware occupies some memory at the base of DRAM, which is currently preventing
the arm64 kernel from using the bottom 2 MB.

Ard Biesheuvel (2):
  efi: add 'offset' param to efi_low_alloc()
  arm64/efi: use efi_low_alloc() with offset to allocate kernel

 arch/arm64/kernel/efi-stub.c                   | 11 ++++++-----
 arch/x86/boot/compressed/eboot.c               |  4 ++--
 drivers/firmware/efi/libstub/efi-stub-helper.c | 20 +++++++++++++++-----
 include/linux/efi.h                            |  2 +-
 4 files changed, 24 insertions(+), 13 deletions(-)

-- 
1.9.1

[PATCH 1/2] efi: add 'offset' param to efi_low_alloc()

[Ard Biesheuvel]

In some cases, e.g., when allocating memory for the arm64 kernel,
we need memory at a certain offset from an aligned boundary. So add
an offset parameter to efi_low_alloc(), and update the existing
callers to pass zero by default.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/kernel/efi-stub.c                   |  2 +-
 arch/x86/boot/compressed/eboot.c               |  4 ++--
 drivers/firmware/efi/libstub/efi-stub-helper.c | 20 +++++++++++++++-----
 include/linux/efi.h                            |  2 +-
 4 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/kernel/efi-stub.c b/arch/arm64/kernel/efi-stub.c
index f5374065ad53..d85a0b2098b3 100644
--- a/arch/arm64/kernel/efi-stub.c
+++ b/arch/arm64/kernel/efi-stub.c
@@ -29,7 +29,7 @@ efi_status_t __init handle_kernel_image(efi_system_table_t *sys_table,
 	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);
+				       SZ_2M, 0, reserve_addr);
 		if (status != EFI_SUCCESS) {
 			pr_efi_err(sys_table, "Failed to relocate kernel\n");
 			return status;
diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index 2c82bd150d43..33d1a72f4266 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -1077,7 +1077,7 @@ struct boot_params *make_boot_params(struct efi_config *c)
 		return NULL;
 	}
 
-	status = efi_low_alloc(sys_table, 0x4000, 1,
+	status = efi_low_alloc(sys_table, 0x4000, 1, 0,
 			       (unsigned long *)&boot_params);
 	if (status != EFI_SUCCESS) {
 		efi_printk(sys_table, "Failed to alloc lowmem for boot params\n");
@@ -1424,7 +1424,7 @@ struct boot_params *efi_main(struct efi_config *c,
 	}
 
 	gdt->size = 0x800;
-	status = efi_low_alloc(sys_table, gdt->size, 8,
+	status = efi_low_alloc(sys_table, gdt->size, 8, 0,
 			   (unsigned long *)&gdt->address);
 	if (status != EFI_SUCCESS) {
 		efi_printk(sys_table, "Failed to alloc mem for gdt\n");
diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index f07d4a67fa76..67d1759781c5 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -226,7 +226,7 @@ fail:
  */
 efi_status_t efi_low_alloc(efi_system_table_t *sys_table_arg,
 			   unsigned long size, unsigned long align,
-			   unsigned long *addr)
+			   unsigned long offset, unsigned long *addr)
 {
 	unsigned long map_size, desc_size;
 	efi_memory_desc_t *map;
@@ -269,10 +269,19 @@ efi_status_t efi_low_alloc(efi_system_table_t *sys_table_arg,
 		 * checks pointers against NULL. Skip the first 8
 		 * bytes so we start at a nice even number.
 		 */
-		if (start == 0x0)
+		if (start + offset == 0x0)
 			start += 8;
 
-		start = round_up(start, align);
+		/*
+		 * Check if the offset exceeds the misalignment of this region.
+		 * In that case, we can round down instead of up, and the
+		 * resulting start value will be correctly aligned and still
+		 * point past the start of the region.
+		 */
+		if (offset >= (start & (align - 1)))
+			start = round_down(start, align) + offset;
+		else
+			start = round_up(start, align) + offset;
 		if ((start + size) > end)
 			continue;
 
@@ -580,7 +589,7 @@ efi_status_t efi_relocate_kernel(efi_system_table_t *sys_table_arg,
 	 * possible.
 	 */
 	if (status != EFI_SUCCESS) {
-		status = efi_low_alloc(sys_table_arg, alloc_size, alignment,
+		status = efi_low_alloc(sys_table_arg, alloc_size, alignment, 0,
 				       &new_addr);
 	}
 	if (status != EFI_SUCCESS) {
@@ -684,7 +693,8 @@ char *efi_convert_cmdline(efi_system_table_t *sys_table_arg,
 
 	options_bytes++;	/* NUL termination */
 
-	status = efi_low_alloc(sys_table_arg, options_bytes, 0, &cmdline_addr);
+	status = efi_low_alloc(sys_table_arg, options_bytes, 0, 0,
+			       &cmdline_addr);
 	if (status != EFI_SUCCESS)
 		return NULL;
 
diff --git a/include/linux/efi.h b/include/linux/efi.h
index faafa1ad6ea7..e738e97632ba 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1245,7 +1245,7 @@ efi_status_t efi_get_memory_map(efi_system_table_t *sys_table_arg,
 
 efi_status_t efi_low_alloc(efi_system_table_t *sys_table_arg,
 			   unsigned long size, unsigned long align,
-			   unsigned long *addr);
+			   unsigned long offset, unsigned long *addr);
 
 efi_status_t efi_high_alloc(efi_system_table_t *sys_table_arg,
 			    unsigned long size, unsigned long align,
-- 
1.9.1

[PATCH 2/2] arm64/efi: use efi_low_alloc() with offset to allocate kernel

[Ard Biesheuvel]

Now that efi_low_alloc() supports an offset parameter, we can use it
to allocate memory for the kernel at TEXT_OFFSET bytes above a 2 MB
aligned boundary directly. This prevents issues when the base of
DRAM is at 0x0, which efi_low_alloc() refuses to use as the address
of an allocation. It also helps when fewer than TEXT_OFFSET bytes are
occupied by the firmware at the base of DRAM, since efi_low_alloc()
will now do the rounding for us, and return 'base of DRAM + TEXT_OFFSET'
directly, regardless of whether any regions below it are in use.

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

diff --git a/arch/arm64/kernel/efi-stub.c b/arch/arm64/kernel/efi-stub.c
index d85a0b2098b3..e3085b391570 100644
--- a/arch/arm64/kernel/efi-stub.c
+++ b/arch/arm64/kernel/efi-stub.c
@@ -23,21 +23,22 @@ efi_status_t __init handle_kernel_image(efi_system_table_t *sys_table,
 {
 	efi_status_t status;
 	unsigned long kernel_size, kernel_memsize = 0;
+	unsigned long new_image_addr;
 
 	/* 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, 0, reserve_addr);
+		status = efi_low_alloc(sys_table, kernel_memsize, SZ_2M,
+				       TEXT_OFFSET, &new_image_addr);
 		if (status != EFI_SUCCESS) {
 			pr_efi_err(sys_table, "Failed to relocate kernel\n");
 			return status;
 		}
-		memcpy((void *)*reserve_addr + TEXT_OFFSET, (void *)*image_addr,
+		memcpy((void *)new_image_addr, (void *)*image_addr,
 		       kernel_size);
-		*image_addr = *reserve_addr + TEXT_OFFSET;
-		*reserve_size = kernel_memsize + TEXT_OFFSET;
+		*image_addr = new_image_addr;
+		*image_size = kernel_memsize;
 	}
 
 
-- 
1.9.1

[PATCH 1/2] efi: add 'offset' param to efi_low_alloc()

[Matt Fleming]

On Wed, 29 Jul, at 12:04:18PM, Ard Biesheuvel wrote:
> In some cases, e.g., when allocating memory for the arm64 kernel,
> we need memory at a certain offset from an aligned boundary. So add
> an offset parameter to efi_low_alloc(), and update the existing
> callers to pass zero by default.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  arch/arm64/kernel/efi-stub.c                   |  2 +-
>  arch/x86/boot/compressed/eboot.c               |  4 ++--
>  drivers/firmware/efi/libstub/efi-stub-helper.c | 20 +++++++++++++++-----
>  include/linux/efi.h                            |  2 +-
>  4 files changed, 19 insertions(+), 9 deletions(-)
 
[...]

> @@ -269,10 +269,19 @@ efi_status_t efi_low_alloc(efi_system_table_t *sys_table_arg,
>  		 * checks pointers against NULL. Skip the first 8
>  		 * bytes so we start at a nice even number.
>  		 */
> -		if (start == 0x0)
> +		if (start + offset == 0x0)
>  			start += 8;
>  
> -		start = round_up(start, align);
> +		/*
> +		 * Check if the offset exceeds the misalignment of this region.
> +		 * In that case, we can round down instead of up, and the
> +		 * resulting start value will be correctly aligned and still
> +		 * point past the start of the region.
> +		 */
> +		if (offset >= (start & (align - 1)))
> +			start = round_down(start, align) + offset;
> +		else
> +			start = round_up(start, align) + offset;
>  		if ((start + size) > end)
>  			continue;

Aha, now I see what you mean. Thanks for doing this Ard, these are much
more polished than what I was expecting.

I'm gonna have to NAK this because it's just too much of a special case
to support directly in efi_low_alloc(), which I think was the exact
point that you made originally, and which I was too tired/dumb to
understand. Sorry.

In particular, the fact that you can use the offset argument to violate
the requested alignment seems like it would trip up most users. 

-- 
Matt Fleming, Intel Open Source Technology Center

[PATCH 1/2] efi: add 'offset' param to efi_low_alloc()

[Ard Biesheuvel]

On 30 July 2015 at 16:01, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> On Wed, 29 Jul, at 12:04:18PM, Ard Biesheuvel wrote:
>> In some cases, e.g., when allocating memory for the arm64 kernel,
>> we need memory at a certain offset from an aligned boundary. So add
>> an offset parameter to efi_low_alloc(), and update the existing
>> callers to pass zero by default.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  arch/arm64/kernel/efi-stub.c                   |  2 +-
>>  arch/x86/boot/compressed/eboot.c               |  4 ++--
>>  drivers/firmware/efi/libstub/efi-stub-helper.c | 20 +++++++++++++++-----
>>  include/linux/efi.h                            |  2 +-
>>  4 files changed, 19 insertions(+), 9 deletions(-)
>
> [...]
>
>> @@ -269,10 +269,19 @@ efi_status_t efi_low_alloc(efi_system_table_t *sys_table_arg,
>>                * checks pointers against NULL. Skip the first 8
>>                * bytes so we start at a nice even number.
>>                */
>> -             if (start == 0x0)
>> +             if (start + offset == 0x0)
>>                       start += 8;
>>
>> -             start = round_up(start, align);
>> +             /*
>> +              * Check if the offset exceeds the misalignment of this region.
>> +              * In that case, we can round down instead of up, and the
>> +              * resulting start value will be correctly aligned and still
>> +              * point past the start of the region.
>> +              */
>> +             if (offset >= (start & (align - 1)))
>> +                     start = round_down(start, align) + offset;
>> +             else
>> +                     start = round_up(start, align) + offset;
>>               if ((start + size) > end)
>>                       continue;
>
> Aha, now I see what you mean. Thanks for doing this Ard, these are much
> more polished than what I was expecting.
>
> I'm gonna have to NAK this because it's just too much of a special case
> to support directly in efi_low_alloc(), which I think was the exact
> point that you made originally, and which I was too tired/dumb to
> understand. Sorry.
>

No worries. Will has already queued the original patch, which solves
all know issues regarding the placement of the kernel image by the EFI
stub.

> In particular, the fact that you can use the offset argument to violate
> the requested alignment seems like it would trip up most users.
>

Yes. We could always rename this enhanced efi_low_alloc() to
efi_low_alloc_with_offset() and introduce a new efi_low_alloc() which
calls it using an offset of zero. But only if you insist.

-- 
Ard.