From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexander Graf Date: Thu, 18 Apr 2019 10:29:35 +0200 Subject: [U-Boot] [RFC v3 09/10] efi_loader: rework bootmgr/bootefi using load_image API In-Reply-To: <1a721f37-0fac-b194-eff3-39d0afe089ae@gmx.de> References: <20190416042428.5007-1-takahiro.akashi@linaro.org> <20190416042428.5007-10-takahiro.akashi@linaro.org> <7f0909bb-4921-bd56-a2ba-32f033c2e066@gmx.de> <1a721f37-0fac-b194-eff3-39d0afe089ae@gmx.de> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: u-boot@lists.denx.de On 16.04.19 18:20, Heinrich Schuchardt wrote: > On 4/16/19 12:56 PM, Heinrich Schuchardt wrote: >> On 4/16/19 6:24 AM, AKASHI Takahiro wrote: >>> In the current implementation, bootefi command and EFI boot manager >>> don't use load_image API, instead, use more primitive and internal >>> functions. This will introduce duplicated code and potentially >>> unknown bugs as well as inconsistent behaviours. >>> >>> With this patch, do_efibootmgr() and do_boot_efi() are completely >>> overhauled and re-implemented using load_image API. >>> >>> Signed-off-by: AKASHI Takahiro >>> --- >>>   cmd/bootefi.c                 | 197 >>> +++++++++++++++++++--------------- >>>   include/efi_loader.h          |   5 +- >>>   lib/efi_loader/efi_bootmgr.c  |  43 ++++---- >>>   lib/efi_loader/efi_boottime.c |   2 + >>>   4 files changed, 134 insertions(+), 113 deletions(-) >>> >>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c >>> index f14966961b23..97d49a53a44b 100644 >>> --- a/cmd/bootefi.c >>> +++ b/cmd/bootefi.c >>> @@ -222,88 +222,39 @@ static efi_status_t efi_install_fdt(const char >>> *fdt_opt) >>>   /** >>>    * do_bootefi_exec() - execute EFI binary >>>    * >>> - * @efi:        address of the binary >>> - * @device_path:    path of the device from which the binary was >>> loaded >>> - * @image_path:        device path of the binary >>> + * @handle:        handle of loaded image >>>    * Return:        status code >>>    * >>>    * Load the EFI binary into a newly assigned memory unwinding the >>> relocation >>>    * information, install the loaded image protocol, and call the >>> binary. >>>    */ >>> -static efi_status_t do_bootefi_exec(void *efi, >>> -                    struct efi_device_path *device_path, >>> -                    struct efi_device_path *image_path) >>> +static efi_status_t do_bootefi_exec(efi_handle_t handle) >>>   { >>> -    efi_handle_t mem_handle = NULL; >>> -    struct efi_device_path *memdp = NULL; >>> -    efi_status_t ret; >>> -    struct efi_loaded_image_obj *image_obj = NULL; >>>       struct efi_loaded_image *loaded_image_info = NULL; >>> - >>> -    /* >>> -     * Special case for efi payload not loaded from disk, such as >>> -     * 'bootefi hello' or for example payload loaded directly into >>> -     * memory via JTAG, etc: >>> -     */ >>> -    if (!device_path && !image_path) { >>> -        printf("WARNING: using memory device/image path, this may >>> confuse some payloads!\n"); >>> -        /* actual addresses filled in after efi_load_pe() */ >>> -        memdp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, 0, 0); >>> -        device_path = image_path = memdp; >>> -        /* >>> -         * Grub expects that the device path of the loaded image is >>> -         * installed on a handle. >>> -         */ >>> -        ret = efi_create_handle(&mem_handle); >>> -        if (ret != EFI_SUCCESS) >>> -            return ret; /* TODO: leaks device_path */ >>> -        ret = efi_add_protocol(mem_handle, &efi_guid_device_path, >>> -                       device_path); >>> -        if (ret != EFI_SUCCESS) >>> -            goto err_add_protocol; >>> -    } else { >>> -        assert(device_path && image_path); >>> -    } >>> - >>> -    ret = efi_setup_loaded_image(device_path, image_path, &image_obj, >>> -                     &loaded_image_info); >>> -    if (ret) >>> -        goto err_prepare; >>> +    efi_status_t ret; >>> >>>       /* Transfer environment variable as load options */ >>> -    set_load_options(loaded_image_info, "bootargs"); >> >> In set_load_options() an error could occur (out of memory). So I think >> it should return a status. >> >>> - >>> -    /* Load the EFI payload */ >>> -    ret = efi_load_pe(image_obj, efi, loaded_image_info); >>> +    ret = EFI_CALL(systab.boottime->open_protocol( >>> +                    handle, >>> +                    &efi_guid_loaded_image, >>> +                    (void **)&loaded_image_info, >>> +                    NULL, NULL, >>> +                    EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL)); >> >> Shouldn't we move this to set_load_options()? >> >>>       if (ret != EFI_SUCCESS) >>> -        goto err_prepare; >>> - >>> -    if (memdp) { >>> -        struct efi_device_path_memory *mdp = (void *)memdp; >>> -        mdp->memory_type = loaded_image_info->image_code_type; >>> -        mdp->start_address = (uintptr_t)loaded_image_info->image_base; >>> -        mdp->end_address = mdp->start_address + >>> -                loaded_image_info->image_size; >>> -    } >>> +        goto err; >>> +    set_load_options(loaded_image_info, "bootargs"); >>> >>>       /* we don't support much: */ >>>       >>> env_set("efi_8be4df61-93ca-11d2-aa0d-00e098032b8c_OsIndicationsSupported", >>> >>>           "{ro,boot}(blob)0000000000000000"); >> >> Shouldn't this move to efi_setup.c? Probably we should also set >> OsIndications. I would prefer using efi_set_variable(). I think moving >> this could be done in a preparatory patch. >> >>> >>>       /* Call our payload! */ >>> -    debug("%s: Jumping to 0x%p\n", __func__, image_obj->entry); >>> -    ret = EFI_CALL(efi_start_image(&image_obj->header, NULL, NULL)); >>> +    ret = EFI_CALL(efi_start_image(handle, NULL, NULL)); >>> >>> -err_prepare: >>> -    /* image has returned, loaded-image obj goes *poof*: */ >>>       efi_restore_gd(); >>>       free(loaded_image_info->load_options); >>> -    efi_delete_handle(&image_obj->header); >>> - >>> -err_add_protocol: >>> -    if (mem_handle) >>> -        efi_delete_handle(mem_handle); >>> >>> +err: >>>       return ret; >>>   } >>> >>> @@ -317,8 +268,7 @@ err_add_protocol: >>>    */ >>>   static int do_efibootmgr(const char *fdt_opt) >>>   { >>> -    struct efi_device_path *device_path, *file_path; >>> -    void *addr; >>> +    efi_handle_t handle; >>>       efi_status_t ret; >>> >>>       /* Allow unaligned memory access */ >>> @@ -340,19 +290,19 @@ static int do_efibootmgr(const char *fdt_opt) >>>       else if (ret != EFI_SUCCESS) >>>           return CMD_RET_FAILURE; >>> >>> -    addr = efi_bootmgr_load(&device_path, &file_path); >>> -    if (!addr) >>> -        return 1; >>> +    ret = efi_bootmgr_load(&handle); >>> +    if (ret != EFI_SUCCESS) { >>> +        printf("EFI boot manager: Cannot load any image\n"); >>> +        return CMD_RET_FAILURE; >>> +    } >>> >>> -    printf("## Starting EFI application at %p ...\n", addr); >>> -    ret = do_bootefi_exec(addr, device_path, file_path); >>> -    printf("## Application terminated, r = %lu\n", >>> -           ret & ~EFI_ERROR_MASK); >>> +    ret = do_bootefi_exec(handle); >>> +    printf("## Application terminated, r = %lu\n", ret & >>> ~EFI_ERROR_MASK); >>> >>>       if (ret != EFI_SUCCESS) >>> -        return 1; >>> +        return CMD_RET_FAILURE; >>> >>> -    return 0; >>> +    return CMD_RET_SUCCESS; >>>   } >>> >>>   /* >>> @@ -367,10 +317,14 @@ static int do_efibootmgr(const char *fdt_opt) >>>    */ >>>   static int do_boot_efi(const char *image_opt, const char *fdt_opt) >>>   { >>> -    unsigned long addr; >>> -    char *saddr; >>> +    void *image_buf; >>> +    struct efi_device_path *device_path, *image_path; >>> +    struct efi_device_path *memdp = NULL, *file_path = NULL; >>> +    const char *saddr; >>> +    unsigned long addr, size; >>> +    const char *size_str; >>> +    efi_handle_t mem_handle = NULL, handle; >>>       efi_status_t ret; >>> -    unsigned long fdt_addr; >>> >>>       /* Allow unaligned memory access */ >>>       allow_unaligned(); >>> @@ -392,36 +346,94 @@ static int do_boot_efi(const char *image_opt, >>> const char *fdt_opt) >>>           return CMD_RET_FAILURE; >>> >>>   #ifdef CONFIG_CMD_BOOTEFI_HELLO >>> -    if (!strcmp(argv[1], "hello")) { >>> -        ulong size = __efi_helloworld_end - __efi_helloworld_begin; >>> - >>> +    if (!strcmp(image_opt, "hello")) { >>>           saddr = env_get("loadaddr"); >>> +        size = __efi_helloworld_end - __efi_helloworld_begin; >>> + >>>           if (saddr) >>>               addr = simple_strtoul(saddr, NULL, 16); >>>           else >>>               addr = CONFIG_SYS_LOAD_ADDR; >>> -        memcpy(map_sysmem(addr, size), __efi_helloworld_begin, size); >>> + >>> +        image_buf = map_sysmem(addr, size); >>> +        memcpy(image_buf, __efi_helloworld_begin, size); >>> + >>> +        device_path = NULL; >>> +        image_path = NULL; >>>       } else >>>   #endif >>>       { >>> -        saddr = argv[1]; >>> +        saddr = image_opt; >>> +        size_str = env_get("filesize"); >>> +        if (size_str) >>> +            size = simple_strtoul(size_str, NULL, 16); >>> +        else >>> +            size = 0; >>> >>>           addr = simple_strtoul(saddr, NULL, 16); >>>           /* Check that a numeric value was passed */ >>>           if (!addr && *saddr != '0') >>>               return CMD_RET_USAGE; >>> + >>> +        image_buf = map_sysmem(addr, size); >>> + >>> +        device_path = bootefi_device_path; >>> +        image_path = bootefi_image_path; >>>       } >>> >>> -    printf("## Starting EFI application at %08lx ...\n", addr); >>> -    ret = do_bootefi_exec(map_sysmem(addr, 0), bootefi_device_path, >>> -                  bootefi_image_path); >>> -    printf("## Application terminated, r = %lu\n", >>> -           ret & ~EFI_ERROR_MASK); >>> +    if (!device_path && !image_path) { >>> +        /* >>> +         * Special case for efi payload not loaded from disk, >>> +         * such as 'bootefi hello' or for example payload >>> +         * loaded directly into memory via JTAG, etc: >>> +         */ >>> +        printf("WARNING: using memory device/image path, this may >>> confuse some payloads!\n"); >> >> The EFI spec says that either of SourceBuffer or DevicePath may be NULL >> when calling LoadImage(). >> >>> + >>> +        /* actual addresses filled in after efi_load_image() */ >>> +        memdp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, >>> +                    (uint64_t)image_buf, size); >>> +        file_path = memdp; /* append(memdp, NULL) */ >>> + >>> +        /* >>> +         * Make sure that device for device_path exist >>> +         * in load_image(). Otherwise, shell and grub will fail. >> >> GRUB will fail anyway because it cannot determine the disk from which it >> was loaded. So why are we doing this? > > In Rob's original commit  bf19273e81eb ("efi_loader: Add mem-mapped > for fallback") the comment was: > "This fixes 'bootefi hello' after devicepath refactoring." > > EFI Shell.efi starts fine even when we set device_path and image_path > to NULL. > > When loading GRUB from disk or via tftp the device path and the image > path are set, e.g. for tftp: > > => dhcp grubarm.efi > => bootefi 0x40200000 $fdtcontroladdr > Found 0 disks > ## Starting EFI application at 40200000 ... > device_path: > /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/MAC(525400123456,0x1) > image_path: /grubarm.efi > error: disk `,msdos2' not found. > Entering rescue mode... > grub rescue> > > Maybe GRUB would run if all necessary modules were compiled in to do a > network boot. But how would it find grub.conf? > > So my feeling is that we are creating the dummy memory device path > because: > > - Once `bootefi hello` which is our own test program had a problem. > - GRUB has a bug: it hangs or crashes if the file device path is not >   set in LoadImage(). > > I think the problem should not be fixed in U-Boot but in GRUB. > > I am CC-ing Leif due to all the attention he has paid both to U-Boot > and to GRUB. Please also keep in mind that you can not fix already released versions of GRUB. Alex