On Thu, Oct 29, 2015 at 11:59:46AM +0900, Ard Biesheuvel wrote:
> On 29 October 2015 at 03:21, Mark Rutland <mark.rutland@xxxxxxx> wrote:
> > On Wed, Oct 28, 2015 at 01:12:36PM -0500, Timur Tabi wrote:
> >> On 10/28/2015 01:08 PM, Mark Rutland wrote:
> >>
> >> >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.
> >>
> >> >>- *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.
> >>
> >> 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<-----------------
> diff --git a/arch/arm64/kernel/efi-stub.c b/arch/arm64/kernel/efi-stub.c
> index 816120ece6bc..78dfbd34b6bf 100644
> --- a/arch/arm64/kernel/efi-stub.c
> +++ b/arch/arm64/kernel/efi-stub.c
> @@ -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);
>
> /*
> @@ -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.
Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
