On Mon, Apr 13, 2020 at 8:55 AM Ard Biesheuvel <ardb@xxxxxxxxxx> wrote:
>
> The notion of a 'preferred' load offset for the kernel dates back to the
> times when the kernel's primary mapping overlapped with the linear region,
> and memory below it could not be used at all.
>
> Today, the arm64 kernel does not really care where it is loaded in physical
> memory, as long as the alignment requirements are met, and so there is no
Just for my understanding, why do we need a TEXT_OFFSET in that case ?
Is it just to provide an option for SoC vendors ?
> point in unconditionally moving the kernel to a new location in memory at
> boot. Instead, we can
> - check for a KASLR seed, and randomly reallocate the kernel if one is
> provided
> - otherwise, check whether the alignment requirements are met for the
> current placement of the kernel, and just run it in place if they are
> - finally, do an ordinary page allocation and reallocate the kernel to a
> suitably aligned buffer anywhere in memory.
>
> By the same reasoning, there is no need to take TEXT_OFFSET into account
> if it is a round multiple of the minimum alignment, which is the usual
> case for relocatable kernels with TEXT_OFFSET randomization disabled.
> Otherwise, it suffices to use the relative misaligment of TEXT_OFFSET
> when reallocating the kernel.
>
> Signed-off-by: Ard Biesheuvel <ardb@xxxxxxxxxx>
> ---
> drivers/firmware/efi/libstub/arm64-stub.c | 62 ++++++++------------
> 1 file changed, 25 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/firmware/efi/libstub/arm64-stub.c b/drivers/firmware/efi/libstub/arm64-stub.c
> index fc9f8ab533a7..cfd535c13242 100644
> --- a/drivers/firmware/efi/libstub/arm64-stub.c
> +++ b/drivers/firmware/efi/libstub/arm64-stub.c
> @@ -34,6 +34,15 @@ efi_status_t check_platform_features(void)
> return EFI_SUCCESS;
> }
>
> +/*
> + * Relocatable kernels can fix up the misalignment with respect to
> + * MIN_KIMG_ALIGN, so they only require a minimum alignment of EFI_KIMG_ALIGN
> + * (which accounts for the alignment of statically allocated objects such as
> + * the swapper stack.)
> + */
> +static const u64 min_kimg_align = IS_ENABLED(CONFIG_RELOCATABLE) ? EFI_KIMG_ALIGN
> + : MIN_KIMG_ALIGN;
> +
> efi_status_t handle_kernel_image(unsigned long *image_addr,
> unsigned long *image_size,
> unsigned long *reserve_addr,
> @@ -43,7 +52,6 @@ efi_status_t handle_kernel_image(unsigned long *image_addr,
> {
> efi_status_t status;
> unsigned long kernel_size, kernel_memsize = 0;
> - unsigned long preferred_offset;
> u64 phys_seed = 0;
>
> if (IS_ENABLED(CONFIG_RANDOMIZE_BASE)) {
> @@ -61,14 +69,8 @@ efi_status_t handle_kernel_image(unsigned long *image_addr,
> }
> }
>
> - /*
> - * 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, MIN_KIMG_ALIGN) + TEXT_OFFSET;
> - if (preferred_offset < dram_base)
> - preferred_offset += MIN_KIMG_ALIGN;
> + if (image->image_base != _text)
> + pr_efi_err("FIRMWARE BUG: efi_loaded_image_t::image_base has bogus value\n");
>
> kernel_size = _edata - _text;
> kernel_memsize = kernel_size + (_end - _edata);
> @@ -103,46 +105,32 @@ efi_status_t handle_kernel_image(unsigned long *image_addr,
>
> *image_addr = *reserve_addr + offset;
> } else {
> - /*
> - * Else, try a straight allocation at the preferred offset.
> - * This will work around the issue where, if dram_base == 0x0,
> - * efi_low_alloc() refuses to allocate at 0x0 (to prevent the
> - * address of the allocation to be mistaken for a FAIL return
> - * 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'.
> - */
> - *image_addr = (unsigned long)_text;
> - if (*image_addr == preferred_offset)
> - return EFI_SUCCESS;
> -
> - *image_addr = *reserve_addr = preferred_offset;
> - *reserve_size = round_up(kernel_memsize, EFI_ALLOC_ALIGN);
> -
> - status = efi_bs_call(allocate_pages, EFI_ALLOCATE_ADDRESS,
> - EFI_LOADER_DATA,
> - *reserve_size / EFI_PAGE_SIZE,
> - (efi_physical_addr_t *)reserve_addr);
> + status = EFI_OUT_OF_RESOURCES;
> }
>
> if (status != EFI_SUCCESS) {
> - *reserve_size = kernel_memsize + TEXT_OFFSET;
> + if (IS_ALIGNED((u64)_text - TEXT_OFFSET, min_kimg_align)) {
> + /*
> + * Just execute from wherever we were loaded by the
> + * UEFI PE/COFF loader if the alignment is suitable.
> + */
> + *image_addr = (u64)_text;
> + *reserve_size = 0;
> + return EFI_SUCCESS;
> + }
> +
> + *reserve_size = kernel_memsize + TEXT_OFFSET % min_kimg_align;
> status = efi_low_alloc(*reserve_size,
> - MIN_KIMG_ALIGN, reserve_addr);
> + min_kimg_align, reserve_addr);
>
> if (status != EFI_SUCCESS) {
> pr_efi_err("Failed to relocate kernel\n");
> *reserve_size = 0;
> return status;
> }
> - *image_addr = *reserve_addr + TEXT_OFFSET;
> + *image_addr = *reserve_addr + TEXT_OFFSET % min_kimg_align;
> }
>
> - if (image->image_base != _text)
> - pr_efi_err("FIRMWARE BUG: efi_loaded_image_t::image_base has bogus value\n");
> -
> memcpy((void *)*image_addr, _text, kernel_size);
>
> return EFI_SUCCESS;
> --
> 2.17.1
>
Looks good to me. FWIW,
Reviewed-by: Atish Patra <atish.patra@xxxxxxx>
--
Regards,
Atish
