| Message ID | CAKv+Gu_ivkemFva-Fu0E24mQqE-m0jPWBzo5OZAr7cyM08hqug@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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,