RE: [PATCH] efi: libstub/arm: account for firmware reserved memory at the base of RAM

2019-10-15 Thread Guillaume Gardet



> -Original Message-
> From: Ard Biesheuvel 
> Sent: 14 October 2019 18:33
> To: linux-arm-ker...@lists.infradead.org
> Cc: linux-efi@vger.kernel.org; Ard Biesheuvel ;
> Guillaume Gardet ; Chester Lin 
> Subject: [PATCH] efi: libstub/arm: account for firmware reserved memory at the
> base of RAM
>
> The EFI stubloader for ARM starts out by allocating a 32 MB window at the 
> base of
> RAM, in order to ensure that the decompressor (which blindly copies the
> uncompressed kernel into that window) does not overwrite other allocations
> that are made while running in the context of the EFI firmware.
>
> In some cases, (e.g., U-Boot running on the Raspberry Pi 2), this is causing 
> boot
> failures because this initial allocation conflicts with a page of reserved 
> memory at
> the base of RAM that contains the SMP spin tables and other pieces of firmware
> data and which was put there by the bootloader under the assumption that the
> TEXT_OFFSET window right below the kernel is only used partially during early
> boot, and will be left alone once the memory reservations are processed and
> taken into account.
>
> So let's permit reserved memory regions to exist in the region starting at 
> the base
> of RAM, and ending at TEXT_OFFSET - 5 * PAGE_SIZE, which is the window below
> the kernel that is not touched by the early boot code.
>
> Cc: Guillaume Gardet 
> Cc: Chester Lin 
> Signed-off-by: Ard Biesheuvel 

Tested-by: Guillaume Gardet 

> ---
>  drivers/firmware/efi/libstub/Makefile |  1 +
>  drivers/firmware/efi/libstub/arm32-stub.c | 16 +---
>  2 files changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/firmware/efi/libstub/Makefile
> b/drivers/firmware/efi/libstub/Makefile
> index 0460c7581220..ee0661ddb25b 100644
> --- a/drivers/firmware/efi/libstub/Makefile
> +++ b/drivers/firmware/efi/libstub/Makefile
> @@ -52,6 +52,7 @@ lib-$(CONFIG_EFI_ARMSTUB)   += arm-stub.o fdt.o
> string.o random.o \
>
>  lib-$(CONFIG_ARM)+= arm32-stub.o
>  lib-$(CONFIG_ARM64)  += arm64-stub.o
> +CFLAGS_arm32-stub.o  := -DTEXT_OFFSET=$(TEXT_OFFSET)
>  CFLAGS_arm64-stub.o  := -DTEXT_OFFSET=$(TEXT_OFFSET)
>
>  #
> diff --git a/drivers/firmware/efi/libstub/arm32-stub.c
> b/drivers/firmware/efi/libstub/arm32-stub.c
> index e8f7aefb6813..47aafeff3e01 100644
> --- a/drivers/firmware/efi/libstub/arm32-stub.c
> +++ b/drivers/firmware/efi/libstub/arm32-stub.c
> @@ -195,6 +195,7 @@ efi_status_t handle_kernel_image(efi_system_table_t
> *sys_table,
>unsigned long dram_base,
>efi_loaded_image_t *image)
>  {
> + unsigned long kernel_base;
>   efi_status_t status;
>
>   /*
> @@ -204,9 +205,18 @@ efi_status_t handle_kernel_image(efi_system_table_t
> *sys_table,
>* loaded. These assumptions are made by the decompressor,
>* before any memory map is available.
>*/
> - dram_base = round_up(dram_base, SZ_128M);
> + kernel_base = round_up(dram_base, SZ_128M);
>
> - status = reserve_kernel_base(sys_table, dram_base, reserve_addr,
> + /*
> +  * Note that some platforms (notably, the Raspberry Pi 2) put
> +  * spin-tables and other pieces of firmware at the base of RAM,
> +  * abusing the fact that the window of TEXT_OFFSET bytes at the
> +  * base of the kernel image is only partially used at the moment.
> +  * (Up to 5 pages are used for the swapper page table)
> +  */
> + kernel_base += TEXT_OFFSET - 5 * PAGE_SIZE;
> +
> + status = reserve_kernel_base(sys_table, kernel_base, reserve_addr,
>reserve_size);
>   if (status != EFI_SUCCESS) {
>   pr_efi_err(sys_table, "Unable to allocate memory for
> uncompressed kernel.\n"); @@ -220,7 +230,7 @@ efi_status_t
> handle_kernel_image(efi_system_table_t *sys_table,
>   *image_size = image->image_size;
>   status = efi_relocate_kernel(sys_table, image_addr, *image_size,
>*image_size,
> -  dram_base + MAX_UNCOMP_KERNEL_SIZE, 0);
> +  kernel_base + MAX_UNCOMP_KERNEL_SIZE,
> 0);
>   if (status != EFI_SUCCESS) {
>   pr_efi_err(sys_table, "Failed to relocate kernel.\n");
>   efi_free(sys_table, *reserve_size, *reserve_addr);
> --
> 2.20.1

IMPORTANT NOTICE: The contents of this email and any attachments are 
confidential and may also be privileged. If you are not the intended recipient, 
please notify the sender immediately and do not disclose the contents to any 
other person, use it for any purpose, or store or copy the information in any 
medium. Thank you.


RE: [PATCH] efi/arm: fix allocation failure when reserving the kernel base

2019-08-14 Thread Guillaume Gardet


> -Original Message-
> From: Ard Biesheuvel 
> Sent: 04 August 2019 09:57
> To: Chester Lin ; li...@armlinux.org.uk
> Cc: a...@linux-foundation.org; r...@linux.ibm.com; ren_...@c-sky.com;
> Juergen Gross ; ge...@linux-m68k.org; mi...@kernel.org;
> linux-arm-ker...@lists.infradead.org; linux-ker...@vger.kernel.org; linux-
> e...@vger.kernel.org; Guillaume Gardet ; Joey Lee
> ; Gary Lin 
> Subject: Re: [PATCH] efi/arm: fix allocation failure when reserving the kernel
> base
>
> Hello Chester,
>
> On Fri, 2 Aug 2019 at 08:40, Chester Lin  wrote:
> >
> > In some cases the arm32 efistub could fail to allocate memory for
> > uncompressed kernel. For example, we got the following error message
> > when verifying EFI stub on Raspberry Pi-2 [kernel-5.2.1 + grub-2.04] :
> >
> >   EFI stub: Booting Linux Kernel...
> >   EFI stub: ERROR: Unable to allocate memory for uncompressed kernel.
> >   EFI stub: ERROR: Failed to relocate kernel
> >
> > After checking the EFI memory map we found that the first page [0 -
> > 0xfff] had been reserved by Raspberry Pi-2's firmware, and the efistub
> > tried to set the dram base at 0, which was actually in a reserved region.
> >
>
> This by itself is a violation of the Linux boot protocol for 32-bit ARM when 
> using
> the decompressor. The decompressor rounds down its own base address to a
> multiple of 128 MB, and assumes the whole area is available for the
> decompressed kernel and related data structures.
> (The first TEXT_OFFSET bytes are no longer used in practice, which is why 
> putting
> a reserved region of 4 KB bytes works at the moment, but this is fragile). 
> Note
> that the decompressor does not look at any DT or EFI provided memory maps
> *at all*.
>
> So unfortunately, this is not something we can fix in the kernel, but we 
> should fix
> it in the bootloader or in GRUB, so it does not put any reserved regions in 
> the
> first 128 MB of memory,

FYI, this is in Raspberry Pi firmware: 
https://github.com/raspberrypi/firmware/issues/1199


>
>
> >   grub> lsefimmap
> >   Type  Physical start  - end #PagesSize Attributes
> >   reserved  -0fff 0001  4KiB WB
> >   conv-mem  1000-07ef5fff 7ef5 130004KiB WB
> >   RT-data   07ef6000-07f09fff 0014 80KiB RT WB
> >   conv-mem  07f0a000-2d871fff 00025968 615840KiB WB
> >   .
> >
> > To avoid a reserved address, we have to ignore the memory regions
> > which are marked as EFI_RESERVED_TYPE, and only conventional memory
> > regions can be chosen. If the region before the kernel base is
> > unaligned, it will be marked as EFI_RESERVED_TYPE and let kernel
> > ignore it so that memblock_limit will not be sticked with a very low address
> such as 0x1000.
> >
> > Signed-off-by: Chester Lin 
> > ---
> >  arch/arm/mm/mmu.c |  3 ++
> >  drivers/firmware/efi/libstub/arm32-stub.c | 43
> > ++-
> >  2 files changed, 37 insertions(+), 9 deletions(-)
> >
> > diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c index
> > f3ce34113f89..909b11ba48d8 100644
> > --- a/arch/arm/mm/mmu.c
> > +++ b/arch/arm/mm/mmu.c
> > @@ -1184,6 +1184,9 @@ void __init adjust_lowmem_bounds(void)
> > phys_addr_t block_start = reg->base;
> > phys_addr_t block_end = reg->base + reg->size;
> >
> > +   if (memblock_is_nomap(reg))
> > +   continue;
> > +
> > if (reg->base < vmalloc_limit) {
> > if (block_end > lowmem_limit)
> > /*
> > diff --git a/drivers/firmware/efi/libstub/arm32-stub.c
> > b/drivers/firmware/efi/libstub/arm32-stub.c
> > index e8f7aefb6813..10d33d36df00 100644
> > --- a/drivers/firmware/efi/libstub/arm32-stub.c
> > +++ b/drivers/firmware/efi/libstub/arm32-stub.c
> > @@ -128,7 +128,7 @@ static efi_status_t
> > reserve_kernel_base(efi_system_table_t *sys_table_arg,
> >
> > for (l = 0; l < map_size; l += desc_size) {
> > efi_memory_desc_t *desc;
> > -   u64 start, end;
> > +   u64 start, end, spare, kernel_base;
> >
> > desc = (void *)memory_map + l;
> > start = desc->phys_addr; @@ -144,27 +144,52 @@ static
> > efi_status_t reserve_kernel_base(efi_system_table_t *sys_table_arg,
> > case EFI_BOOT_SERVICES_DATA:
> >