arm64: efi: make sure vmlinux load address aligned on 2MB

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 Timur Tabi <[email protected]>

Tested-by Shanker Donthineni <[email protected]>

---
 arch/arm64/kernel/efi-stub.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

arch/arm64/kernel/efi-stub.c +0 -0

index 816120e..37118f4 100644

arm64: efi: ensure kernel is loaded at correct address

The kernel image needs to be loaded text_offset_bytes from a 2M-aligned base, per Documentation/arm64/booting.txt. If loaded at the wrong offset modulo 2M, __create_page_tables will create incorrect page tables.

The EFI stub implicitly assumes that dram_base (i.e. the lowest address with a EFI_MEMORY_WB attribute) is 2M-aligned, and tries to load the kernel at dram_base + TEXT_OFFSET. If dram_base is not 2M-aligned, the kernel will be loaded at the wrong offset from 2M.

arch/arm64/kernel/efi-stub.c efi_status_t __init handle_kernel_image(efi_system_table_t *sys_table_arg, · line 46

Quoted 3 lines

arch/arm64/kernel/efi-stub.c +2 -1

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

We also need to fix the test for whether we need to relocate the kernel:
(*image_addr != (dram_base + TEXT_OFFSET)).

When dram_base is not 2M aligned, that is broken, and it's been broken since it was introduced in commit 3c7f255039a2ad6e ("arm64: efi: add EFI stub") in v3.16.

It's a bit hideous to fix the general case, though, it seems.

Quoted 10 lines · Mark Rutland

arm64: efi: ensure kernel is loaded at correct address

The kernel image needs to be loaded text_offset_bytes from a 2M-aligned base, per Documentation/arm64/booting.txt. If loaded at the wrong offset modulo 2M, __create_page_tables will create incorrect page tables.

The EFI stub implicitly assumes that dram_base (i.e. the lowest address with a EFI_MEMORY_WB attribute) is 2M-aligned, and tries to load the kernel at dram_base + TEXT_OFFSET. If dram_base is not 2M-aligned, the kernel will be loaded at the wrong offset from 2M.

Thanks, I'll use that. I messed up a couple other things, so I need to send out a v3 anyway.

Quoted 7 lines

arch/arm64/kernel/efi-stub.c efi_status_t __init handle_kernel_image(efi_system_table_t *sys_table_arg, · line 46

Quoted 3 lines

arch/arm64/kernel/efi-stub.c +2 -1

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

We also need to fix the test for whether we need to relocate the kernel:
(*image_addr != (dram_base + TEXT_OFFSET)).

Quoted 5 lines

When dram_base is not 2M aligned, that is broken, and it's been broken since it was introduced in commit 3c7f255039a2ad6e ("arm64: efi: add EFI stub") in v3.16.

It's a bit hideous to fix the general case, though, it seems.

Um, so I should I do something more in my v3 patch, or is this a change for a different patch?

Quoted 27 lines · Timur Tabi

Quoted 10 lines · Mark Rutland

arm64: efi: ensure kernel is loaded at correct address

The kernel image needs to be loaded text_offset_bytes from a 2M-aligned base, per Documentation/arm64/booting.txt. If loaded at the wrong offset modulo 2M, __create_page_tables will create incorrect page tables.

The EFI stub implicitly assumes that dram_base (i.e. the lowest address with a EFI_MEMORY_WB attribute) is 2M-aligned, and tries to load the kernel at dram_base + TEXT_OFFSET. If dram_base is not 2M-aligned, the kernel will be loaded at the wrong offset from 2M.

Thanks, I'll use that. I messed up a couple other things, so I need to send out a v3 anyway.

Quoted 11 lines

Review

Quoted 3 lines

(quoted diff) +2 -1

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

We also need to fix the test for whether we need to relocate the kernel:
(*image_addr != (dram_base + TEXT_OFFSET)).

When dram_base is not 2M aligned, that is broken, and it's been broken since it was introduced in commit 3c7f255039a2ad6e ("arm64: efi: add EFI stub") in v3.16.

Quoted 4 lines

Quoted 1 line

It's a bit hideous to fix the general case, though, it seems.

Um, so I should I do something more in my v3 patch, or is this a change for a different patch?

I think there should be a single patch, but please hold off v3 for a day or so. I think there a few more edge cases here, and I'm currently investigating.

Quoted 28 lines · Mark Rutland

Quoted 27 lines · Timur Tabi

Quoted 10 lines · Mark Rutland

arm64: efi: ensure kernel is loaded at correct address

The kernel image needs to be loaded text_offset_bytes from a 2M-aligned base, per Documentation/arm64/booting.txt. If loaded at the wrong offset modulo 2M, __create_page_tables will create incorrect page tables.

The EFI stub implicitly assumes that dram_base (i.e. the lowest address with a EFI_MEMORY_WB attribute) is 2M-aligned, and tries to load the kernel at dram_base + TEXT_OFFSET. If dram_base is not 2M-aligned, the kernel will be loaded at the wrong offset from 2M.

Thanks, I'll use that. I messed up a couple other things, so I need to send out a v3 anyway.

Quoted 11 lines

Review

Quoted 3 lines

(quoted diff) +2 -1

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

We also need to fix the test for whether we need to relocate the kernel:
(*image_addr != (dram_base + TEXT_OFFSET)).

When dram_base is not 2M aligned, that is broken, and it's been broken since it was introduced in commit 3c7f255039a2ad6e ("arm64: efi: add EFI stub") in v3.16.

Quoted 9 lines

Quoted 4 lines

Quoted 1 line

It's a bit hideous to fix the general case, though, it seems.

Um, so I should I do something more in my v3 patch, or is this a change for a different patch?

I think there should be a single patch, but please hold off v3 for a day or so. I think there a few more edge cases here, and I'm currently investigating.

Apologies for the drive-by nature of my contributions to this thread. I am currently travelling.

I think the below should address both issues (and I even tried to compile it this time)

-----------------8<-----------------

arch/arm64/kernel/efi-stub.c +11 -1

index 816120ece6bc..78dfbd34b6bf 100644
@@ -25,10 +25,20 @@
        unsigned long kernel_size, kernel_memsize = 0;
        unsigned long nr_pages;
        void *old_image_addr = (void *)*image_addr;
+       unsigned long preferred_offset;
+
+       /*
+        * The preferred offset of the kernel Image is TEXT_OFFSET bytes beyond
+        * a 2 MB aligned base, which itself may be lower than dram_base, as
+        * long as the resulting offset equals or exceeds it.
+        */
+       preferred_offset = round_down(dram_base, SZ_2M) + TEXT_OFFSET;
+       if (preferred_offset < dram_base)
+               preferred_offset += SZ_2M;
 
        /* Relocate the image, if required. */
        kernel_size = _edata - _text;
-       if (*image_addr != (dram_base + TEXT_OFFSET)) {
+       if (*image_addr != preferred_offset) {
                kernel_memsize = kernel_size + (_end - _edata);
 
                /*

Quoted 74 lines · Ard Biesheuvel

Quoted 28 lines · Mark Rutland

Quoted 27 lines · Timur Tabi

Quoted 10 lines · Mark Rutland

arm64: efi: ensure kernel is loaded at correct address

The kernel image needs to be loaded text_offset_bytes from a 2M-aligned base, per Documentation/arm64/booting.txt. If loaded at the wrong offset modulo 2M, __create_page_tables will create incorrect page tables.

The EFI stub implicitly assumes that dram_base (i.e. the lowest address with a EFI_MEMORY_WB attribute) is 2M-aligned, and tries to load the kernel at dram_base + TEXT_OFFSET. If dram_base is not 2M-aligned, the kernel will be loaded at the wrong offset from 2M.

Thanks, I'll use that. I messed up a couple other things, so I need to send out a v3 anyway.

Quoted 11 lines

Review

Quoted 3 lines

(quoted diff) +2 -1

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

We also need to fix the test for whether we need to relocate the kernel:
(*image_addr != (dram_base + TEXT_OFFSET)).

When dram_base is not 2M aligned, that is broken, and it's been broken since it was introduced in commit 3c7f255039a2ad6e ("arm64: efi: add EFI stub") in v3.16.

Quoted 9 lines

Quoted 4 lines

Quoted 1 line

It's a bit hideous to fix the general case, though, it seems.

Um, so I should I do something more in my v3 patch, or is this a change for a different patch?

I think there should be a single patch, but please hold off v3 for a day or so. I think there a few more edge cases here, and I'm currently investigating.

Apologies for the drive-by nature of my contributions to this thread. I am currently travelling.

I think the below should address both issues (and I even tried to compile it this time)

-----------------8<-----------------

arch/arm64/kernel/efi-stub.c +11 -1

index 816120ece6bc..78dfbd34b6bf 100644
@@ -25,10 +25,20 @@
        unsigned long kernel_size, kernel_memsize = 0;
        unsigned long nr_pages;
        void *old_image_addr = (void *)*image_addr;
+       unsigned long preferred_offset;
+
+       /*
+        * The preferred offset of the kernel Image is TEXT_OFFSET bytes beyond
+        * a 2 MB aligned base, which itself may be lower than dram_base, as
+        * long as the resulting offset equals or exceeds it.
+        */
+       preferred_offset = round_down(dram_base, SZ_2M) + TEXT_OFFSET;
+       if (preferred_offset < dram_base)
+               preferred_offset += SZ_2M;
 
        /* Relocate the image, if required. */
        kernel_size = _edata - _text;
-       if (*image_addr != (dram_base + TEXT_OFFSET)) {
+       if (*image_addr != preferred_offset) {
                kernel_memsize = kernel_size + (_end - _edata);
 
                /*

arch/arm64/kernel/efi-stub.c line 58

Quoted 8 lines

arch/arm64/kernel/efi-stub.c +1 -1

@@ -42,7 +52,7 @@
                 * 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 = preferred_offset;
                nr_pages = round_up(kernel_memsize, EFI_ALLOC_ALIGN) /
                           EFI_PAGE_SIZE;
                status = efi_call_early(allocate_pages, EFI_ALLOCATE_ADDRESS,

This looks good to me, and I've given it a spin on Juno (though I haven't fiddled with dram_base). I trust that you will respin this as a patch when you get the chance.

There is another (existing) problem I spotted, in that we'll sometimes move the kernel to a worse address. If the kernel was loaded at a valid address (i.e. image_addr % SZ_2MB == TEXT_OFFSET), but not at the preferred offset, we try to relocate it, even if it's already at the lowest possible address.

That doesn't break boot, so it's not as big a problem, and it's probably better sovled with the split VA stuff.

Quoted 6 lines

Quoted 6 lines

There is another (existing) problem I spotted, in that we'll sometimes move the kernel to a worse address. If the kernel was loaded at a valid address (i.e. image_addr % SZ_2MB == TEXT_OFFSET), but not at the preferred offset, we try to relocate it, even if it's already at the lowest possible address.

Indeed. Unlikely to occur in practice, since UEFI mostly allocates top down, but it would indeed be better to drop the new allocation and simply use the existing one if it is not an improvement.

Quoted 6 lines · Ard Biesheuvel

Quoted 6 lines

Quoted 6 lines

There is another (existing) problem I spotted, in that we'll sometimes move the kernel to a worse address. If the kernel was loaded at a valid address (i.e. image_addr % SZ_2MB == TEXT_OFFSET), but not at the preferred offset, we try to relocate it, even if it's already at the lowest possible address.

Quoted 3 lines

Indeed. Unlikely to occur in practice, since UEFI mostly allocates top down, but it would indeed be better to drop the new allocation and simply use the existing one if it is not an improvement.

So you're saying that if we modify our UEFI so that it always loads the kernel at dram_base + TEXT_OFFSET, then one day in the future the kernel will skip the relocation?

Signature · 3 lines

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