Re: [PATCH] efi/libstub/arm: make efi_entry() an ordinary PE/COFF entrypoint

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, 18 Feb 2020 at 02:53, Atish Patra <Atish.Patra@xxxxxxx> wrote:
>
> On Mon, 2020-02-17 at 13:49 +0100, Ard Biesheuvel wrote:
> > Expose efi_entry() as the PE/COFF entrypoint directly, instead of
> > jumping into a wrapper that fiddles with stack buffers and other
> > stuff that the compiler is much better at. The only reason this
> > code exists is to obtain a pointer to the base of the image, but
> > we can get the same value from the loaded_image protocol, which
> > we already need for other reasons anyway.
> >
> > Update the return type as well, to make it consistent with what
> > is required for a PE/COFF executable entrypoint.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@xxxxxxxxxx>
> > ---
> > Apologies to Atish for making him track a moving target, but this is
> > another
> > bit of cleanup work that I would like to merge before taking the
> > RISC-V
> > changes on top.
> >
>
> And I thought I am done rebasing my patches ;). Jokes apart, this is
> actually much cleaner approach in retrieving image base and parsing DT
> compared to what I currently have for RISC-V.
>
>
> > On RISC-V, I would expect efi_enter_kernel() to be implemented as a C
> > routine
> > that finds the hart id in the DT (which is passed as an argument),
> > and pass
> > it to the startup code of the kernel proper.
> >
>
> Yes. I probably won't need efi-entry.S for RISC-V at all which
> simplifies the RISC-V implementation. Yay!!
>

Glad to hear that.

> Is there a tree with this patch ? I tried to find it in your kernel.org
> repo but couldn't find it. As there are 3 efi related patch set in
> flight, I am not sure about the order in which they should be applied.
>
>

Please rebase onto

https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git/log/?h=next


> >  arch/arm/boot/compressed/efi-header.S     |  2 +-
> >  arch/arm/boot/compressed/head.S           | 44 +++-----------
> >  arch/arm64/kernel/efi-entry.S             | 64 +++-----------------
> >  arch/arm64/kernel/efi-header.S            |  2 +-
> >  drivers/firmware/efi/libstub/arm-stub.c   | 37 +++++------
> >  drivers/firmware/efi/libstub/arm32-stub.c |  1 +
> >  drivers/firmware/efi/libstub/arm64-stub.c |  3 +-
> >  7 files changed, 42 insertions(+), 111 deletions(-)
> >
> > diff --git a/arch/arm/boot/compressed/efi-header.S
> > b/arch/arm/boot/compressed/efi-header.S
> > index a5983588f96b..e38fbda02b93 100644
> > --- a/arch/arm/boot/compressed/efi-header.S
> > +++ b/arch/arm/boot/compressed/efi-header.S
> > @@ -60,7 +60,7 @@ optional_header:
> >               .long   __pecoff_code_size              @ SizeOfCode
> >               .long   __pecoff_data_size              @
> > SizeOfInitializedData
> >               .long   0                               @
> > SizeOfUninitializedData
> > -             .long   efi_stub_entry - start          @
> > AddressOfEntryPoint
> > +             .long   efi_entry - start               @
> > AddressOfEntryPoint
> >               .long   start_offset                    @ BaseOfCode
> >               .long   __pecoff_data_start - start     @ BaseOfData
> >
> > diff --git a/arch/arm/boot/compressed/head.S
> > b/arch/arm/boot/compressed/head.S
> > index 088b0a060876..941cd28a13c3 100644
> > --- a/arch/arm/boot/compressed/head.S
> > +++ b/arch/arm/boot/compressed/head.S
> > @@ -1437,50 +1437,22 @@ __enter_kernel:
> >  reloc_code_end:
> >
> >  #ifdef CONFIG_EFI_STUB
> > -             .align  2
> > -_start:              .long   start - .
> > -
> > -ENTRY(efi_stub_entry)
> > -             @ allocate space on stack for passing current zImage
> > address
> > -             @ and for the EFI stub to return of new entry point of
> > -             @ zImage, as EFI stub may copy the kernel. Pointer
> > address
> > -             @ is passed in r2. r0 and r1 are passed through from
> > the
> > -             @ EFI firmware to efi_entry
> > -             adr     ip, _start
> > -             ldr     r3, [ip]
> > -             add     r3, r3, ip
> > -             stmfd   sp!, {r3, lr}
> > -             mov     r2, sp                  @ pass zImage address
> > in r2
> > -             bl      efi_entry
> > -
> > -             @ Check for error return from EFI stub. r0 has FDT
> > address
> > -             @ or error code.
> > -             cmn     r0, #1
> > -             beq     efi_load_fail
> > -
> > -             @ Preserve return value of efi_entry() in r4
> > -             mov     r4, r0
> > +ENTRY(efi_enter_kernel)
> > +             mov     r4, r0                  @ preserve entrypoint
> > +             mov     r5, r1                  @ preserve DT pointer
> >               bl      cache_clean_flush
> >               bl      cache_off
> >
> >               @ Set parameters for booting zImage according to boot
> > protocol
> > -             @ put FDT address in r2, it was returned by efi_entry()
> > -             @ r1 is the machine type, and r0 needs to be 0
> >               mov     r0, #0
> >               mov     r1, #0xFFFFFFFF
> > -             mov     r2, r4
> > +             mov     r2, r5
> >
> > -             @ Branch to (possibly) relocated zImage that is in [sp]
> > -             ldr     lr, [sp]
> > +             @ Branch to (possibly) relocated zImage @ [r4]
> >               ldr     ip, =start_offset
> > -             add     lr, lr, ip
> > -             mov     pc, lr                          @ no mode
> > switch
> > -
> > -efi_load_fail:
> > -             @ Return EFI_LOAD_ERROR to EFI firmware on error.
> > -             ldr     r0, =0x80000001
> > -             ldmfd   sp!, {ip, pc}
> > -ENDPROC(efi_stub_entry)
> > +             add     r4, r4, ip
> > +             mov     pc, r4                  @ no mode switch
> > +ENDPROC(efi_enter_kernel)
> >  #endif
> >
> >               .align
> > diff --git a/arch/arm64/kernel/efi-entry.S b/arch/arm64/kernel/efi-
> > entry.S
> > index 304d5b02ca67..49e3b1ad1b36 100644
> > --- a/arch/arm64/kernel/efi-entry.S
> > +++ b/arch/arm64/kernel/efi-entry.S
> > @@ -10,55 +10,18 @@
> >
> >  #include <asm/assembler.h>
> >
> > -#define EFI_LOAD_ERROR 0x8000000000000001
> > -
> >       __INIT
> >
> > -     /*
> > -      * We arrive here from the EFI boot manager with:
> > -      *
> > -      *    * CPU in little-endian mode
> > -      *    * MMU on with identity-mapped RAM
> > -      *    * Icache and Dcache on
> > -      *
> > -      * We will most likely be running from some place other than
> > where
> > -      * we want to be. The kernel image wants to be placed at
> > TEXT_OFFSET
> > -      * from start of RAM.
> > -      */
> > -ENTRY(entry)
> > -     /*
> > -      * Create a stack frame to save FP/LR with extra space
> > -      * for image_addr variable passed to efi_entry().
> > -      */
> > -     stp     x29, x30, [sp, #-32]!
> > -     mov     x29, sp
> > -
> > -     /*
> > -      * Call efi_entry to do the real work.
> > -      * x0 and x1 are already set up by firmware. Current runtime
> > -      * address of image is calculated and passed via *image_addr.
> > -      *
> > -      * unsigned long efi_entry(void *handle,
> > -      *                         efi_system_table_t *sys_table,
> > -      *                         unsigned long *image_addr) ;
> > -      */
> > -     adr_l   x8, _text
> > -     add     x2, sp, 16
> > -     str     x8, [x2]
> > -     bl      efi_entry
> > -     cmn     x0, #1
> > -     b.eq    efi_load_fail
> > -
> > +ENTRY(efi_enter_kernel)
> >       /*
> >        * efi_entry() will have copied the kernel image if necessary
> > and we
> > -      * return here with device tree address in x0 and the kernel
> > entry
> > -      * point stored at *image_addr. Save those values in registers
> > which
> > -      * are callee preserved.
> > +      * end up here with device tree address in x1 and the kernel
> > entry
> > +      * point stored in x0. Save those values in registers which are
> > +      * callee preserved.
> >        */
> > -     mov     x20, x0         // DTB address
> > -     ldr     x0, [sp, #16]   // relocated _text address
> > +     mov     x20, x1                 // DTB address
> >       ldr     w21, =stext_offset
> > -     add     x21, x0, x21
> > +     add     x21, x0, x21            // relocated Image address
> >
> >       /*
> >        * Calculate size of the kernel Image (same for original and
> > copy).
> > @@ -80,9 +43,8 @@ ENTRY(entry)
> >        * entries for the VA range of the current image, so no
> > maintenance is
> >        * necessary.
> >        */
> > -     adr     x0, entry
> > -     adr     x1, entry_end
> > -     sub     x1, x1, x0
> > +     adr     x0, efi_enter_kernel
> > +     mov     x1, efi_enter_kernel_size
> >       bl      __flush_dcache_area
> >
> >       /* Turn off Dcache and MMU */
> > @@ -110,11 +72,5 @@ ENTRY(entry)
> >       mov     x2, xzr
> >       mov     x3, xzr
> >       br      x21
> > -
> > -efi_load_fail:
> > -     mov     x0, #EFI_LOAD_ERROR
> > -     ldp     x29, x30, [sp], #32
> > -     ret
> > -
> > -entry_end:
> > -ENDPROC(entry)
> > +ENDPROC(efi_enter_kernel)
> > +     .set    efi_enter_kernel_size, . - efi_enter_kernel
> > diff --git a/arch/arm64/kernel/efi-header.S b/arch/arm64/kernel/efi-
> > header.S
> > index a7cfacce3e15..40c704c5d3a5 100644
> > --- a/arch/arm64/kernel/efi-header.S
> > +++ b/arch/arm64/kernel/efi-header.S
> > @@ -27,7 +27,7 @@ optional_header:
> >       .long   __initdata_begin - efi_header_end       // SizeOfCode
> >       .long   __pecoff_data_size                      //
> > SizeOfInitializedData
> >       .long   0                                       //
> > SizeOfUninitializedData
> > -     .long   __efistub_entry - _head                 //
> > AddressOfEntryPoint
> > +     .long   __efistub_efi_entry - _head             //
> > AddressOfEntryPoint
> >       .long   efi_header_end - _head                  // BaseOfCode
> >
> >  extra_header_fields:
> > diff --git a/drivers/firmware/efi/libstub/arm-stub.c
> > b/drivers/firmware/efi/libstub/arm-stub.c
> > index 56e475c1aa55..439c094f7bf2 100644
> > --- a/drivers/firmware/efi/libstub/arm-stub.c
> > +++ b/drivers/firmware/efi/libstub/arm-stub.c
> > @@ -133,17 +133,20 @@ efi_status_t handle_kernel_image(unsigned long
> > *image_addr,
> >                                unsigned long *reserve_size,
> >                                unsigned long dram_base,
> >                                efi_loaded_image_t *image);
> > +
> > +void __noreturn efi_enter_kernel(unsigned long entrypoint, unsigned
> > long fdt);
> > +
> >  /*
> >   * EFI entry point for the arm/arm64 EFI stubs.  This is the
> > entrypoint
> >   * that is described in the PE/COFF header.  Most of the code is the
> > same
> >   * for both archictectures, with the arch-specific code provided in
> > the
> >   * handle_kernel_image() function.
> >   */
> > -unsigned long efi_entry(void *handle, efi_system_table_t
> > *sys_table_arg,
> > -                            unsigned long *image_addr)
> > +efi_status_t efi_entry(efi_handle_t handle, efi_system_table_t
> > *sys_table_arg)
> >  {
> >       efi_loaded_image_t *image;
> >       efi_status_t status;
> > +     unsigned long image_addr;
> >       unsigned long image_size = 0;
> >       unsigned long dram_base;
> >       /* addr/point and size pairs for memory management*/
> > @@ -153,7 +156,6 @@ unsigned long efi_entry(void *handle,
> > efi_system_table_t *sys_table_arg,
> >       unsigned long fdt_size = 0;
> >       char *cmdline_ptr = NULL;
> >       int cmdline_size = 0;
> > -     unsigned long new_fdt_addr;
> >       efi_guid_t loaded_image_proto = LOADED_IMAGE_PROTOCOL_GUID;
> >       unsigned long reserve_addr = 0;
> >       unsigned long reserve_size = 0;
> > @@ -165,8 +167,10 @@ unsigned long efi_entry(void *handle,
> > efi_system_table_t *sys_table_arg,
> >       sys_table = sys_table_arg;
> >
> >       /* Check if we were booted by the EFI firmware */
> > -     if (sys_table->hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE)
> > +     if (sys_table->hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE) {
> > +             status = EFI_INVALID_PARAMETER;
> >               goto fail;
> > +     }
> >
> >       status = check_platform_features();
> >       if (status != EFI_SUCCESS)
> > @@ -187,6 +191,7 @@ unsigned long efi_entry(void *handle,
> > efi_system_table_t *sys_table_arg,
> >       dram_base = get_dram_base();
> >       if (dram_base == EFI_ERROR) {
> >               pr_efi_err("Failed to find DRAM base\n");
> > +             status = EFI_LOAD_ERROR;
> >               goto fail;
> >       }
> >
> > @@ -198,6 +203,7 @@ unsigned long efi_entry(void *handle,
> > efi_system_table_t *sys_table_arg,
> >       cmdline_ptr = efi_convert_cmdline(image, &cmdline_size,
> > ULONG_MAX);
> >       if (!cmdline_ptr) {
> >               pr_efi_err("getting command line via
> > LOADED_IMAGE_PROTOCOL\n");
> > +             status = EFI_OUT_OF_RESOURCES;
> >               goto fail;
> >       }
> >
> > @@ -213,7 +219,7 @@ unsigned long efi_entry(void *handle,
> > efi_system_table_t *sys_table_arg,
> >
> >       si = setup_graphics();
> >
> > -     status = handle_kernel_image(image_addr, &image_size,
> > +     status = handle_kernel_image(&image_addr, &image_size,
> >                                    &reserve_addr,
> >                                    &reserve_size,
> >                                    dram_base, image);
> > @@ -260,7 +266,7 @@ unsigned long efi_entry(void *handle,
> > efi_system_table_t *sys_table_arg,
> >               pr_efi("Generating empty DTB\n");
> >
> >       if (!noinitrd()) {
> > -             max_addr = efi_get_max_initrd_addr(dram_base,
> > *image_addr);
> > +             max_addr = efi_get_max_initrd_addr(dram_base,
> > image_addr);
> >               status = efi_load_initrd_dev_path(&initrd_addr,
> > &initrd_size,
> >                                                 max_addr);
> >               if (status == EFI_SUCCESS) {
> > @@ -311,33 +317,30 @@ unsigned long efi_entry(void *handle,
> > efi_system_table_t *sys_table_arg,
> >
> >       install_memreserve_table();
> >
> > -     new_fdt_addr = fdt_addr;
> >       status = allocate_new_fdt_and_exit_boot(handle,
> > -                             &new_fdt_addr,
> > efi_get_max_fdt_addr(dram_base),
> > +                             &fdt_addr,
> > efi_get_max_fdt_addr(dram_base),
> >                               initrd_addr, initrd_size, cmdline_ptr,
> >                               fdt_addr, fdt_size);
> > +     if (status != EFI_SUCCESS)
> > +             goto fail_free_initrd;
> >
> > -     /*
> > -      * If all went well, we need to return the FDT address to the
> > -      * calling function so it can be passed to kernel as part of
> > -      * the kernel boot protocol.
> > -      */
> > -     if (status == EFI_SUCCESS)
> > -             return new_fdt_addr;
> > +     efi_enter_kernel(image_addr, fdt_addr);
> > +     /* not reached */
> >
> > +fail_free_initrd:
> >       pr_efi_err("Failed to update FDT and exit boot services\n");
> >
> >       efi_free(initrd_size, initrd_addr);
> >       efi_free(fdt_size, fdt_addr);
> >
> >  fail_free_image:
> > -     efi_free(image_size, *image_addr);
> > +     efi_free(image_size, image_addr);
> >       efi_free(reserve_size, reserve_addr);
> >  fail_free_cmdline:
> >       free_screen_info(si);
> >       efi_free(cmdline_size, (unsigned long)cmdline_ptr);
> >  fail:
> > -     return EFI_ERROR;
> > +     return status;
> >  }
> >
> >  /*
> > diff --git a/drivers/firmware/efi/libstub/arm32-stub.c
> > b/drivers/firmware/efi/libstub/arm32-stub.c
> > index 7b2a6382b647..113298b8ea65 100644
> > --- a/drivers/firmware/efi/libstub/arm32-stub.c
> > +++ b/drivers/firmware/efi/libstub/arm32-stub.c
> > @@ -227,6 +227,7 @@ efi_status_t handle_kernel_image(unsigned long
> > *image_addr,
> >        * Relocate the zImage, so that it appears in the lowest 128 MB
> >        * memory window.
> >        */
> > +     *image_addr = image->image_base;
> >       *image_size = image->image_size;
> >       status = efi_relocate_kernel(image_addr, *image_size,
> > *image_size,
> >                                    kernel_base +
> > MAX_UNCOMP_KERNEL_SIZE, 0, 0);
> > diff --git a/drivers/firmware/efi/libstub/arm64-stub.c
> > b/drivers/firmware/efi/libstub/arm64-stub.c
> > index 719d03a64329..9254cd8ab2d3 100644
> > --- a/drivers/firmware/efi/libstub/arm64-stub.c
> > +++ b/drivers/firmware/efi/libstub/arm64-stub.c
> > @@ -43,7 +43,6 @@ efi_status_t handle_kernel_image(unsigned long
> > *image_addr,
> >  {
> >       efi_status_t status;
> >       unsigned long kernel_size, kernel_memsize = 0;
> > -     void *old_image_addr = (void *)*image_addr;
> >       unsigned long preferred_offset;
> >       u64 phys_seed = 0;
> >
> > @@ -141,7 +140,7 @@ efi_status_t handle_kernel_image(unsigned long
> > *image_addr,
> >               }
> >               *image_addr = *reserve_addr + TEXT_OFFSET;
> >       }
> > -     memcpy((void *)*image_addr, old_image_addr, kernel_size);
> > +     memcpy((void *)*image_addr, image->image_base, kernel_size);
> >
> >       return EFI_SUCCESS;
> >  }
>
> --
> Regards,
> Atish



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux