[U-Boot] [RFC v3 09/10] efi_loader: rework bootmgr/bootefi using load_image API

[Alexander Graf <agraf@csgraf.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 <takahiro.akashi@linaro.org>
>>> ---
>>>   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