[PATCH] arm64: efi: make sure vmlinux load address aligned on 2MBytes

[PATCH] arm64: efi: make sure vmlinux load address aligned on 2MBytes

[Mark Rutland]

On Wed, Oct 28, 2015 at 04:02:23PM -0500, Timur Tabi wrote:
> On 10/28/2015 12:26 PM, Mark Rutland wrote:
> >>>This does make the kernel boot, but we suspect that there may be
> >>>another problem.  We need to investigate it, but we have a suspicion
> >>>that the EFI stub is trying to allocate from the Runtime Data block,
> >>>and the alignment adjustment "fixes" the problem by moving the
> >>>pointer to Conventional Memory.
> >I don't follow. EFI_BOOT_SERVICES.AllocatePages() should only give us
> >pages which are available, so it shouldn't ever return pages which are
> >runtime data -- it would fail and we'd fall back to efi_low_alloc().
> >
> >Could you elaborate?
> 
> So we're still debugging this internally, but it turns out that
> dram_base is equal to 0x4000820000, which also happens to be the
> start of a Runtime Data block:
> 
>   0x004000820000-0x00400085ffff [Runtime Data       |RUN|XP|  |  |
> |WB|WT|WC|UC]
> 
> I think this is not supposed to happen.

It's perfectly valid for that to be detected as dram_base, and the stub may
call AllocatePages() for that region, but AllocatePages() shouldn't
successfully allocate from there.

The stub should fall back to efi_low_alloc, walking through the memory map
until it finds a large enough region to allocate from, with some subsequent
AllocatePages() call eventually succeeding.

Is that not what you're seeing?

Thanks,
Mark.

[PATCH] arm64: efi: make sure vmlinux load address aligned on 2MBytes

[Timur Tabi]

From: Shanker Donthineni <shankerd@codeaurora.org>

The vmlinux image load address must be aligned to 2MB, as documented
in Documentation/arm64/booting.txt. Otherwise, __create_page_tables
in head.S will create incorrect page table entries.

Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
Signed-off-by: Timur Tabi <timur@codeaurora.org>
---
 arch/arm64/kernel/efi-stub.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/kernel/efi-stub.c b/arch/arm64/kernel/efi-stub.c
index 816120e..df1433d 100644
--- a/arch/arm64/kernel/efi-stub.c
+++ b/arch/arm64/kernel/efi-stub.c
@@ -21,7 +21,7 @@ efi_status_t __init handle_kernel_image(efi_system_table_t *sys_table_arg,
 					unsigned long dram_base,
 					efi_loaded_image_t *image)
 {
-	efi_status_t status;
+	efi_status_t status = EFI_LOAD_ERROR;
 	unsigned long kernel_size, kernel_memsize = 0;
 	unsigned long nr_pages;
 	void *old_image_addr = (void *)*image_addr;
@@ -39,15 +39,18 @@ efi_status_t __init handle_kernel_image(efi_system_table_t *sys_table_arg,
 		 * value or a NULL pointer). It will also ensure that, on
 		 * platforms where the [dram_base, dram_base + TEXT_OFFSET)
 		 * interval is partially occupied by the firmware (like on APM
-		 * Mustang), we can still place the kernel at the address
-		 * 'dram_base + TEXT_OFFSET'.
+		 * Mustang) and dram_base is aligned on 2Mbytes, we can still
+		 * place the kernel at the address 'dram_base + TEXT_OFFSET'.
 		 */
-		*image_addr = *reserve_addr = dram_base + TEXT_OFFSET;
-		nr_pages = round_up(kernel_memsize, EFI_ALLOC_ALIGN) /
+		if (IS_ALIGNED(dram_base, SZ_2M)) {
+			*image_addr = *reserve_addr = dram_base + TEXT_OFFSET;
+			nr_pages = round_up(kernel_memsize, EFI_ALLOC_ALIGN) /
 			   EFI_PAGE_SIZE;
-		status = efi_call_early(allocate_pages, EFI_ALLOCATE_ADDRESS,
+			status = efi_call_early(allocate_pages,
+					EFI_ALLOCATE_ADDRESS,
 					EFI_LOADER_DATA, nr_pages,
 					(efi_physical_addr_t *)reserve_addr);
+		}
 		if (status != EFI_SUCCESS) {
 			kernel_memsize += TEXT_OFFSET;
 			status = efi_low_alloc(sys_table_arg, kernel_memsize,
-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

[PATCH] arm64: efi: make sure vmlinux load address aligned on 2MBytes

[Ard Biesheuvel]

On 28 October 2015 at 06:24, Timur Tabi <timur@codeaurora.org> wrote:
> From: Shanker Donthineni <shankerd@codeaurora.org>
>
> The vmlinux image load address must be aligned to 2MB, as documented
> in Documentation/arm64/booting.txt. Otherwise, __create_page_tables
> in head.S will create incorrect page table entries.
>
> Signed-off-by: Shanker Donthineni <shankerd@codeaurora.org>
> Signed-off-by: Timur Tabi <timur@codeaurora.org>
> ---
>  arch/arm64/kernel/efi-stub.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm64/kernel/efi-stub.c b/arch/arm64/kernel/efi-stub.c
> index 816120e..df1433d 100644
> --- a/arch/arm64/kernel/efi-stub.c
> +++ b/arch/arm64/kernel/efi-stub.c
> @@ -21,7 +21,7 @@ efi_status_t __init handle_kernel_image(efi_system_table_t *sys_table_arg,
>                                         unsigned long dram_base,
>                                         efi_loaded_image_t *image)
>  {
> -       efi_status_t status;
> +       efi_status_t status = EFI_LOAD_ERROR;
>         unsigned long kernel_size, kernel_memsize = 0;
>         unsigned long nr_pages;
>         void *old_image_addr = (void *)*image_addr;
> @@ -39,15 +39,18 @@ efi_status_t __init handle_kernel_image(efi_system_table_t *sys_table_arg,
>                  * value or a NULL pointer). It will also ensure that, on
>                  * platforms where the [dram_base, dram_base + TEXT_OFFSET)
>                  * interval is partially occupied by the firmware (like on APM
> -                * Mustang), we can still place the kernel at the address
> -                * 'dram_base + TEXT_OFFSET'.
> +                * Mustang) and dram_base is aligned on 2Mbytes, we can still
> +                * place the kernel at the address 'dram_base + TEXT_OFFSET'.
>                  */
> -               *image_addr = *reserve_addr = dram_base + TEXT_OFFSET;
> -               nr_pages = round_up(kernel_memsize, EFI_ALLOC_ALIGN) /
> +               if (IS_ALIGNED(dram_base, SZ_2M)) {
> +                       *image_addr = *reserve_addr = dram_base + TEXT_OFFSET;
> +                       nr_pages = round_up(kernel_memsize, EFI_ALLOC_ALIGN) /
>                            EFI_PAGE_SIZE;
> -               status = efi_call_early(allocate_pages, EFI_ALLOCATE_ADDRESS,
> +                       status = efi_call_early(allocate_pages,
> +                                       EFI_ALLOCATE_ADDRESS,
>                                         EFI_LOADER_DATA, nr_pages,
>                                         (efi_physical_addr_t *)reserve_addr);
> +               }
>                 if (status != EFI_SUCCESS) {
>                         kernel_memsize += TEXT_OFFSET;
>                         status = efi_low_alloc(sys_table_arg, kernel_memsize,

I agree we should fix this, but I would prefer a oneliner such as

diff --git a/arch/arm64/kernel/efi-stub.c b/arch/arm64/kernel/efi-stub.c
index 816120ece6bc..a60ce249cfc0 100644
--- a/arch/arm64/kernel/efi-stub.c
+++ b/arch/arm64/kernel/efi-stub.c
@@ -42,7 +42,8 @@
                 * Mustang), we can still place the kernel at the address
                 * 'dram_base + TEXT_OFFSET'.
                 */
-               *image_addr = *reserve_addr = dram_base + TEXT_OFFSET;
+               *image_addr = *reserve_addr = round_up(dram_base, SZ_2M) +
+                                                      TEXT_OFFSET);
                nr_pages = round_up(kernel_memsize, EFI_ALLOC_ALIGN) /
                           EFI_PAGE_SIZE;
                status = efi_call_early(allocate_pages, EFI_ALLOCATE_ADDRESS,

[PATCH] arm64: efi: make sure vmlinux load address aligned on 2MBytes

[Timur Tabi]

Ard Biesheuvel wrote:
> +               *image_addr = *reserve_addr = round_up(dram_base, SZ_2M) +
> +                                                      TEXT_OFFSET);

Shouldn't this be:

*image_addr = *reserve_addr = round_up(dram_base + TEXT_OFFSET, SZ_2M);

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

[PATCH] arm64: efi: make sure vmlinux load address aligned on 2MBytes

[Ard Biesheuvel]

On 28 October 2015 at 11:10, Timur Tabi <timur@codeaurora.org> wrote:
> Ard Biesheuvel wrote:
>>
>> +               *image_addr = *reserve_addr = round_up(dram_base, SZ_2M) +
>> +                                                      TEXT_OFFSET);
>
>
> Shouldn't this be:
>
> *image_addr = *reserve_addr = round_up(dram_base + TEXT_OFFSET, SZ_2M);
>

No. I screwed up the patch since the trailing ) should not be there,
but we do need to round first, and only then add the offset.

-- 
Ard.

[PATCH] arm64: efi: make sure vmlinux load address aligned on 2MBytes

[Timur Tabi]

Ard Biesheuvel wrote:
> No. I screwed up the patch since the trailing ) should not be there,
> but we do need to round first, and only then add the offset.

But if the offset is not a multiple of 2MB, won't the address passed to 
allocate_pages be unaligned?  I'll test your patch on our system, but I 
thought the problem was that the allocated address must be aligned.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

[PATCH] arm64: efi: make sure vmlinux load address aligned on 2MBytes

[Mark Rutland]

On Tue, Oct 27, 2015 at 09:13:22PM -0500, Timur Tabi wrote:
> Ard Biesheuvel wrote:
> >No. I screwed up the patch since the trailing ) should not be there,
> >but we do need to round first, and only then add the offset.
> 
> But if the offset is not a multiple of 2MB, won't the address passed
> to allocate_pages be unaligned?  I'll test your patch on our system,
> but I thought the problem was that the allocated address must be
> aligned.

The kernel needs to be loaded at an address which is text_offset bytes
past a 2M-aligned base. It is not loaded at the 2M-aligned base itself.

Ard's patch correctly findd a 2M-aligned base, then adds the text
offset.

Mark.

[PATCH] arm64: efi: make sure vmlinux load address aligned on 2MBytes

[Timur Tabi]

On 10/27/2015 09:06 PM, Ard Biesheuvel wrote:
>
> diff --git a/arch/arm64/kernel/efi-stub.c b/arch/arm64/kernel/efi-stub.c
> index 816120ece6bc..a60ce249cfc0 100644
> --- a/arch/arm64/kernel/efi-stub.c
> +++ b/arch/arm64/kernel/efi-stub.c
> @@ -42,7 +42,8 @@
>                   * Mustang), we can still place the kernel at the address
>                   * 'dram_base + TEXT_OFFSET'.
>                   */
> -               *image_addr = *reserve_addr = dram_base + TEXT_OFFSET;
> +               *image_addr = *reserve_addr = round_up(dram_base, SZ_2M) +
> +                                                      TEXT_OFFSET);
>                  nr_pages = round_up(kernel_memsize, EFI_ALLOC_ALIGN) /
>                             EFI_PAGE_SIZE;
>                  status = efi_call_early(allocate_pages, EFI_ALLOCATE_ADDRESS,

Tested-by: Timur Tabi <timur@codeaurora.org>
Tested-by: Shanker Donthineni <shankerd@codeaurora.org>

However, I think this formatting is easier to read:

		*image_addr = *reserve_addr =
			round_up(dram_base, SZ_2M) + TEXT_OFFSET;

This does make the kernel boot, but we suspect that there may be another 
problem.  We need to investigate it, but we have a suspicion that the 
EFI stub is trying to allocate from the Runtime Data block, and the 
alignment adjustment "fixes" the problem by moving the pointer to 
Conventional Memory.

Anyway, is there any chance we can get this fix into 4.3?  I'd hate to 
have 4.3 released knowing that it's broken on our hardware.

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

[PATCH] arm64: efi: make sure vmlinux load address aligned on 2MBytes

[Mark Rutland]

On Wed, Oct 28, 2015 at 12:11:57PM -0500, Timur Tabi wrote:
> On 10/27/2015 09:06 PM, Ard Biesheuvel wrote:
> >
> >diff --git a/arch/arm64/kernel/efi-stub.c b/arch/arm64/kernel/efi-stub.c
> >index 816120ece6bc..a60ce249cfc0 100644
> >--- a/arch/arm64/kernel/efi-stub.c
> >+++ b/arch/arm64/kernel/efi-stub.c
> >@@ -42,7 +42,8 @@
> >                  * Mustang), we can still place the kernel at the address
> >                  * 'dram_base + TEXT_OFFSET'.
> >                  */
> >-               *image_addr = *reserve_addr = dram_base + TEXT_OFFSET;
> >+               *image_addr = *reserve_addr = round_up(dram_base, SZ_2M) +
> >+                                                      TEXT_OFFSET);
> >                 nr_pages = round_up(kernel_memsize, EFI_ALLOC_ALIGN) /
> >                            EFI_PAGE_SIZE;
> >                 status = efi_call_early(allocate_pages, EFI_ALLOCATE_ADDRESS,
> 
> Tested-by: Timur Tabi <timur@codeaurora.org>
> Tested-by: Shanker Donthineni <shankerd@codeaurora.org>

I assume with the trailing ')' removed. ;)

With that:

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

> However, I think this formatting is easier to read:
> 
> 		*image_addr = *reserve_addr =
> 			round_up(dram_base, SZ_2M) + TEXT_OFFSET;

I have no strong feeling on this either way.

> This does make the kernel boot, but we suspect that there may be
> another problem.  We need to investigate it, but we have a suspicion
> that the EFI stub is trying to allocate from the Runtime Data block,
> and the alignment adjustment "fixes" the problem by moving the
> pointer to Conventional Memory.

I don't follow. EFI_BOOT_SERVICES.AllocatePages() should only give us
pages which are available, so it shouldn't ever return pages which are
runtime data -- it would fail and we'd fall back to efi_low_alloc().

Could you elaborate?

> Anyway, is there any chance we can get this fix into 4.3?  I'd hate
> to have 4.3 released knowing that it's broken on our hardware.

It's probably too late now, but it should certainly be Cc'd stable and
get backported.

Thanks,
Mark.

[PATCH] arm64: efi: make sure vmlinux load address aligned on 2MBytes

[Will Deacon]

On Wed, Oct 28, 2015 at 12:11:57PM -0500, Timur Tabi wrote:
> On 10/27/2015 09:06 PM, Ard Biesheuvel wrote:
> >diff --git a/arch/arm64/kernel/efi-stub.c b/arch/arm64/kernel/efi-stub.c
> >index 816120ece6bc..a60ce249cfc0 100644
> >--- a/arch/arm64/kernel/efi-stub.c
> >+++ b/arch/arm64/kernel/efi-stub.c
> >@@ -42,7 +42,8 @@
> >                  * Mustang), we can still place the kernel at the address
> >                  * 'dram_base + TEXT_OFFSET'.
> >                  */
> >-               *image_addr = *reserve_addr = dram_base + TEXT_OFFSET;
> >+               *image_addr = *reserve_addr = round_up(dram_base, SZ_2M) +
> >+                                                      TEXT_OFFSET);
> >                 nr_pages = round_up(kernel_memsize, EFI_ALLOC_ALIGN) /
> >                            EFI_PAGE_SIZE;
> >                 status = efi_call_early(allocate_pages, EFI_ALLOCATE_ADDRESS,
> 
> Tested-by: Timur Tabi <timur@codeaurora.org>
> Tested-by: Shanker Donthineni <shankerd@codeaurora.org>
> 
> However, I think this formatting is easier to read:
> 
> 		*image_addr = *reserve_addr =
> 			round_up(dram_base, SZ_2M) + TEXT_OFFSET;
> 
> This does make the kernel boot, but we suspect that there may be another
> problem.  We need to investigate it, but we have a suspicion that the EFI
> stub is trying to allocate from the Runtime Data block, and the alignment
> adjustment "fixes" the problem by moving the pointer to Conventional Memory.
> 
> Anyway, is there any chance we can get this fix into 4.3?  I'd hate to have
> 4.3 released knowing that it's broken on our hardware.

If you want something in for 4.3, you'll need to post a new patch in a
separate thread and I'd like to see at least Ard's ack on it.

Will

[PATCH] arm64: efi: make sure vmlinux load address aligned on 2MBytes

[Timur Tabi]

On 10/28/2015 12:26 PM, Mark Rutland wrote:
>> >This does make the kernel boot, but we suspect that there may be
>> >another problem.  We need to investigate it, but we have a suspicion
>> >that the EFI stub is trying to allocate from the Runtime Data block,
>> >and the alignment adjustment "fixes" the problem by moving the
>> >pointer to Conventional Memory.
> I don't follow. EFI_BOOT_SERVICES.AllocatePages() should only give us
> pages which are available, so it shouldn't ever return pages which are
> runtime data -- it would fail and we'd fall back to efi_low_alloc().
>
> Could you elaborate?

So we're still debugging this internally, but it turns out that 
dram_base is equal to 0x4000820000, which also happens to be the start 
of a Runtime Data block:

   0x004000820000-0x00400085ffff [Runtime Data       |RUN|XP|  |  | 
|WB|WT|WC|UC]

I think this is not supposed to happen.

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