arm64: efi: make sure vmlinux load address aligned on 2MBytes
PATCH 1/1 arm64: efi: make sure vmlinux load address aligned on 2MBytes
From: Shanker Donthineni <[email protected]>
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.
--- arch/arm64/kernel/efi-stub.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)
arch/arm64/kernel/efi-stub.c +9 -6
@@ -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,
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.
PATCH 1/1 arm64: efi: make sure vmlinux load address aligned on 2MBytes
Quoted 39 lines · Timur Tabi
From: Shanker Donthineni <[email protected]>
The vmlinux image load address must be aligned to 2MB, as documented in
Documentation/arm64/booting.txt. Otherwise,__create_page_tablesinhead.Swill create incorrect page table entries.Signed-off-by Shanker Donthineni <[email protected]>Signed-off-by Timur Tabi <[email protected]>--- arch/arm64/kernel/efi-stub.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)arch/arm64/kernel/efi-stub.c +6
-5@@ -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) /
arch/arm64/kernel/efi-stub.c efi_status_t __init handle_kernel_image(efi_system_table_t *sys_table_arg, · line 56
Quoted 10 lines
arch/arm64/kernel/efi-stub.c +3
-1EFI_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
arch/arm64/kernel/efi-stub.c +2 -1
@@ -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 1/1 arm64: efi: make sure vmlinux load address aligned on 2MBytes
Quoted 6 lines · Ard Biesheuvel
arch/arm64/kernel/efi-stub.c +0
-0
arch/arm64/kernel/efi-stub.c line 49
Quoted 9 lines
arch/arm64/kernel/efi-stub.c +2
-1@@ -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,
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.
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.
Mark Rutland
reviewerPATCH 1/1 arm64: efi: make sure vmlinux load address aligned on 2MBytes
Quoted 11 lines · Timur Tabi
Quoted 10 lines · Ard Biesheuvel
arch/arm64/kernel/efi-stub.c +0
-1@@ -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 ;
Quoted 8 lines
arch/arm64/kernel/efi-stub.c line 49
Quoted 5 lines
arch/arm64/kernel/efi-stub.c +2
-0+ *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 <[email protected]>Tested-by Shanker Donthineni <[email protected]>
I assume with the trailing ')' removed. ;)
With that:
Quoted 4 lines
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.
Quoted 5 lines
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?
Quoted 2 lines
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 1/1 arm64: efi: make sure vmlinux load address aligned on 2MBytes
Quoted 10 lines · Mark Rutland
Quoted 5 lines
Quoted 5 lines
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 toefi_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.
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.
Mark Rutland
reviewerPATCH 1/1 arm64: efi: make sure vmlinux load address aligned on 2MBytes
Quoted 12 lines · Timur Tabi
Quoted 10 lines · Mark Rutland
Quoted 5 lines
Quoted 5 lines
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 toefi_low_alloc().Could you elaborate?
Quoted 8 lines
So we're still debugging this internally, but it turns out that
dram_baseis equal to0x4000820000, 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 1/1 arm64: efi: make sure vmlinux load address aligned on 2MBytes
In reply to Timur TabiQuoted 24 lines · Timur Tabi
Quoted 5 lines · Ard Biesheuvel
arch/arm64/kernel/efi-stub.c +0
-0arch/arm64/kernel/efi-stub.c line 49
Quoted 9 lines
arch/arm64/kernel/efi-stub.c +2
-1@@ -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 <[email protected]>Tested-by Shanker Donthineni <[email protected]>However, I think this formatting is easier to read:
*image_addr = *reserve_addr = round_up(dram_base, SZ_2M) + TEXT_OFFSET;
Quoted 7 lines
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 1/1 arm64: efi: make sure vmlinux load address aligned on 2MBytes
In reply to Ard Biesheuvelarch/arm64/kernel/efi-stub.c line 46
Quoted 2 lines · Ard Biesheuvel
arch/arm64/kernel/efi-stub.c +2
-0+ *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);
Signature · 3 lines
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 1/1 arm64: efi: make sure vmlinux load address aligned on 2MBytes
Quoted 10 lines · Timur Tabi
arch/arm64/kernel/efi-stub.c line 46
Quoted 3 lines · Ard Biesheuvel
arch/arm64/kernel/efi-stub.c +2
-0+ *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.
Signature · 1 line
Ard.
PATCH 1/1 arm64: efi: make sure vmlinux load address aligned on 2MBytes
Quoted 2 lines · Ard Biesheuvel
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.
Signature · 3 lines
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.
Mark Rutland
reviewerPATCH 1/1 arm64: efi: make sure vmlinux load address aligned on 2MBytes
Quoted 8 lines · Timur Tabi
Quoted 2 lines · Ard Biesheuvel
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_pagesbe 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.