[U-Boot] [PATCH 8/9] efi_loader: rework bootmgr/bootefi using load_image API

[Heinrich Schuchardt <xypron.glpk@gmx.de>]

On 4/20/19 10:37 AM, Heinrich Schuchardt wrote:
> On 4/19/19 5:22 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.

Hello Takahiro,

with this patch applied iPXE cannot find a chained file on the EFI
partition. Did you test booting GRUB with this patch?

Best regards

Heinrich

EFI: Entry efi_load_image(0, 00000000b9f92020,
/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(1,MBR,0x800512bd,0x800,0x200000)/dtb,
0000000040080000, 21738, 00000000b9f33fc8)
EFI: Exit: efi_load_image: 0
EFI: Entry efi_start_image(00000000b9f95360, 0000000000000000,
0000000000000000)
  EFI: Call: efi_open_protocol(image_handle, &efi_guid_loaded_image,
&info, NULL, NULL, EFI_OPEN_PROTOCOL_GET_PROTOCOL)
  EFI: 0 returned by efi_open_protocol(image_handle,
&efi_guid_loaded_image, &info, NULL, NULL, EFI_OPEN_PROTOCOL_GET_PROTOCOL)
  EFI: Jumping into 0x00000000b8e7861c
  EFI: Call: image_obj->entry(image_handle, &systab)
    EFI: Entry efi_load_image(0, 00000000b9f95360, /, 00000000b8e9fbc0,
177, 00000000b9f33e38)
efi_load_pe: Invalid DOS Signature
    EFI: Exit: efi_load_image: 1
iPXE initialising devices...ok
iPXE 1.0.0+ (ac4fb) -- Open Source Network Boot Firmware -- http://ipxe.org
Features: DNS FTP HTTP HTTPS iSCSI NFS TFTP VLAN AoE EFI Menu
iPXE script started
Trying to chain file:/boot.ipxe
Could not start download: No such file or directory
(http://ipxe.org/2d4de08e)
Could not find file:/boot.ipxe
Opening shell
iPXE>


>>
>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>> ---
>>   cmd/bootefi.c                 | 187 ++++++++++++++++++----------------
>>   include/efi_loader.h          |   5 +-
>>   lib/efi_loader/efi_bootmgr.c  |  43 ++++----
>>   lib/efi_loader/efi_boottime.c |   2 +
>>   4 files changed, 127 insertions(+), 110 deletions(-)
>>
>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>> index e2ca68c4dc12..8c84fff590a7 100644
>> --- a/cmd/bootefi.c
>> +++ b/cmd/bootefi.c
>> @@ -242,89 +242,36 @@ 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;
>>
>>       /* Transfer environment variable as load options */
>> -    ret = set_load_options((efi_handle_t)image_obj, "bootargs");
>> -    if (ret != EFI_SUCCESS)
>> -        goto err_prepare;
>> -
>> -    /* Load the EFI payload */
>> -    ret = efi_load_pe(image_obj, efi, loaded_image_info);
>> +    ret = set_load_options(handle, "bootargs");
>>       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;
>> -    }
>> +        return ret;
>>
>>       /* we don't support much: */
>>      
>> env_set("efi_8be4df61-93ca-11d2-aa0d-00e098032b8c_OsIndicationsSupported",
>>
>>           "{ro,boot}(blob)0000000000000000");
>>
>>       /* 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);
>> +    /*
>> +     * FIXME: Who is responsible for
>> +     *    free(loaded_image_info->load_options);
>> +     * Once efi_exit() is implemented correctly,
>> +     * handle itself doesn't exist here.
>> +     */
>>
>>       return ret;
>>   }
>> @@ -339,8 +286,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 */
>> @@ -362,19 +308,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;
>>   }
>>
>>   /*
>> @@ -389,7 +335,12 @@ static int do_efibootmgr(const char *fdt_opt)
>>    */
>>   static int do_bootefi_image(const char *image_opt, const char *fdt_opt)
>>   {
>> -    unsigned long addr;
>> +    void *image_buf;
>> +    struct efi_device_path *device_path, *image_path;
>> +    struct efi_device_path *memdp = NULL, *file_path = NULL;
>> +    unsigned long addr, size;
>> +    const char *size_str;
>> +    efi_handle_t mem_handle = NULL, handle;
>>       efi_status_t ret;
>>
>>       /* Allow unaligned memory access */
>> @@ -414,33 +365,90 @@ static int do_bootefi_image(const char
>> *image_opt, const char *fdt_opt)
>>   #ifdef CONFIG_CMD_BOOTEFI_HELLO
>>       if (!strcmp(image_opt, "hello")) {
>>           char *saddr;
>> -        ulong size = __efi_helloworld_end - __efi_helloworld_begin;
>>
>>           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
>>       {
>> +        size_str = env_get("filesize");
>> +        if (size_str)
>> +            size = simple_strtoul(size_str, NULL, 16);
>> +        else
>> +            size = 0;
>> +
>>           addr = simple_strtoul(image_opt, NULL, 16);
>>           /* Check that a numeric value was passed */
>>           if (!addr && *image_opt != '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:
>> +         */
>> +        memdp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
>> +                    (uintptr_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.
>> +         */
>> +        ret = efi_create_handle(&mem_handle);
>> +        if (ret != EFI_SUCCESS)
>> +            /* TODO: leaks device_path */
>> +            goto err_add_protocol;
>> +
>> +        ret = efi_add_protocol(mem_handle, &efi_guid_device_path,
>> +                       memdp);
>> +        if (ret != EFI_SUCCESS)
>> +            goto err_add_protocol;
>> +    } else {
>> +        assert(device_path && image_path);
>> +        file_path = efi_dp_append(device_path, image_path);
>> +    }
>> +
>> +    ret = EFI_CALL(efi_load_image(false, efi_root,
>> +                      file_path, image_buf, size, &handle));
>> +    if (ret != EFI_SUCCESS)
>> +        goto err_prepare;
>> +
>> +    ret = do_bootefi_exec(handle);
>> +    printf("## Application terminated, r = %lu\n", ret &
>> ~EFI_ERROR_MASK);
>> +
>> +err_prepare:
>> +    if (device_path)
>> +        efi_free_pool(file_path);
>> +
>> +err_add_protocol:
>> +    if (mem_handle)
>> +        efi_delete_handle(mem_handle);
>> +    if (file_path) /* hence memdp */
>> +        efi_free_pool(file_path);
>>
>>       if (ret != EFI_SUCCESS)
>>           return CMD_RET_FAILURE;
>> -    else
>> -        return CMD_RET_SUCCESS;
>> +
>> +    return CMD_RET_SUCCESS;
>>   }
>>
>>   #ifdef CONFIG_CMD_BOOTEFI_SELFTEST
>> @@ -598,7 +606,7 @@ static char bootefi_help_text[] =
>>       "    Use environment variable efi_selftest to select a single
>> test.\n"
>>       "    Use 'setenv efi_selftest list' to enumerate all tests.\n"
>>   #endif
>> -    "bootefi bootmgr [fdt addr]\n"
>> +    "bootefi bootmgr [fdt address]\n"
>>       "  - load and boot EFI payload based on BootOrder/BootXXXX
>> variables.\n"
>>       "\n"
>>       "    If specified, the device tree located at <fdt address> gets\n"
>> @@ -623,6 +631,13 @@ void efi_set_bootdev(const char *dev, const char
>> *devnr, const char *path)
>>       ret = efi_dp_from_name(dev, devnr, path, &device, &image);
>>       if (ret == EFI_SUCCESS) {
>>           bootefi_device_path = device;
>> +        if (image) {
>> +            /* FIXME: image should not contain device */
>> +            struct efi_device_path *image_tmp = image;
>> +
>> +            efi_dp_split_file_path(image, &device, &image);
>> +            efi_free_pool(image_tmp);
>> +        }
>>           bootefi_image_path = image;
>>       } else {
>>           bootefi_device_path = NULL;
>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>> index 93f7672aecbc..39ed8a6fa592 100644
>> --- a/include/efi_loader.h
>> +++ b/include/efi_loader.h
>> @@ -412,8 +412,6 @@ efi_status_t efi_setup_loaded_image(struct
>> efi_device_path *device_path,
>>                       struct efi_device_path *file_path,
>>                       struct efi_loaded_image_obj **handle_ptr,
>>                       struct efi_loaded_image **info_ptr);
>> -efi_status_t efi_load_image_from_path(struct efi_device_path *file_path,
>> -                      void **buffer, efi_uintn_t *size);
>>   /* Print information about all loaded images */
>>   void efi_print_image_infos(void *pc);
>>
>> @@ -567,8 +565,7 @@ struct efi_load_option {
>>
>>   void efi_deserialize_load_option(struct efi_load_option *lo, u8 *data);
>>   unsigned long efi_serialize_load_option(struct efi_load_option *lo,
>> u8 **data);
>> -void *efi_bootmgr_load(struct efi_device_path **device_path,
>> -               struct efi_device_path **file_path);
>> +efi_status_t efi_bootmgr_load(efi_handle_t *handle);
>>
>>   #else /* CONFIG_IS_ENABLED(EFI_LOADER) */
>>
>> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
>> index 4fccadc5483d..13ec79b2098b 100644
>> --- a/lib/efi_loader/efi_bootmgr.c
>> +++ b/lib/efi_loader/efi_bootmgr.c
>> @@ -120,14 +120,14 @@ static void *get_var(u16 *name, const efi_guid_t
>> *vendor,
>>    * if successful.  This checks that the EFI_LOAD_OPTION is active
>> (enabled)
>>    * and that the specified file to boot exists.
>>    */
>> -static void *try_load_entry(uint16_t n, struct efi_device_path
>> **device_path,
>> -                struct efi_device_path **file_path)
>> +static efi_status_t try_load_entry(u16 n, efi_handle_t *handle)
>>   {
>>       struct efi_load_option lo;
>>       u16 varname[] = L"Boot0000";
>>       u16 hexmap[] = L"0123456789ABCDEF";
>> -    void *load_option, *image = NULL;
>> +    void *load_option;
>>       efi_uintn_t size;
>> +    efi_status_t ret;
>>
>>       varname[4] = hexmap[(n & 0xf000) >> 12];
>>       varname[5] = hexmap[(n & 0x0f00) >> 8];
>> @@ -136,19 +136,19 @@ static void *try_load_entry(uint16_t n, struct
>> efi_device_path **device_path,
>>
>>       load_option = get_var(varname, &efi_global_variable_guid, &size);
>>       if (!load_option)
>> -        return NULL;
>> +        return EFI_LOAD_ERROR;
>>
>>       efi_deserialize_load_option(&lo, load_option);
>>
>>       if (lo.attributes & LOAD_OPTION_ACTIVE) {
>>           u32 attributes;
>> -        efi_status_t ret;
>>
>>           debug("%s: trying to load \"%ls\" from %pD\n",
>>                 __func__, lo.label, lo.file_path);
>>
>> -        ret = efi_load_image_from_path(lo.file_path, &image, &size);
>> -
>> +        ret = EFI_CALL(efi_load_image(false, (void *)0x1 /* FIXME */,
> 
> In cmd/booefi.c you use efi_root. So we shall do the same here.
> 
> I will fix that. No need to resend the patch.
> 
> 
>> +                          lo.file_path, NULL, 0,
>> +                          handle));
>>           if (ret != EFI_SUCCESS)
>>               goto error;
> TODO:
> Now that you have the loaded image protocol you should update the load
> options with the value in load_option. I guess you do not want to
> overwrite it with the content of bootargs.
> 
> A the problem was not introduced with this patch series:
> 
> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> 
>>
>> @@ -159,17 +159,22 @@ static void *try_load_entry(uint16_t n, struct
>> efi_device_path **device_path,
>>                   L"BootCurrent",
>>                   (efi_guid_t *)&efi_global_variable_guid,
>>                   attributes, size, &n));
>> -        if (ret != EFI_SUCCESS)
>> +        if (ret != EFI_SUCCESS) {
>> +            if (EFI_CALL(efi_unload_image(*handle))
>> +                != EFI_SUCCESS)
>> +                printf("Unloading image failed\n");
>>               goto error;
>> +        }
>>
>>           printf("Booting: %ls\n", lo.label);
>> -        efi_dp_split_file_path(lo.file_path, device_path, file_path);
>> +    } else {
>> +        ret = EFI_LOAD_ERROR;
>>       }
>>
>>   error:
>>       free(load_option);
>>
>> -    return image;
>> +    return ret;
>>   }
>>
>>   /*
>> @@ -177,12 +182,10 @@ error:
>>    * EFI variable, the available load-options, finding and returning
>>    * the first one that can be loaded successfully.
>>    */
>> -void *efi_bootmgr_load(struct efi_device_path **device_path,
>> -               struct efi_device_path **file_path)
>> +efi_status_t efi_bootmgr_load(efi_handle_t *handle)
>>   {
>>       u16 bootnext, *bootorder;
>>       efi_uintn_t size;
>> -    void *image = NULL;
>>       int i, num;
>>       efi_status_t ret;
>>
>> @@ -209,10 +212,9 @@ void *efi_bootmgr_load(struct efi_device_path
>> **device_path,
>>           /* load BootNext */
>>           if (ret == EFI_SUCCESS) {
>>               if (size == sizeof(u16)) {
>> -                image = try_load_entry(bootnext, device_path,
>> -                               file_path);
>> -                if (image)
>> -                    return image;
>> +                ret = try_load_entry(bootnext, handle);
>> +                if (ret == EFI_SUCCESS)
>> +                    return ret;
>>               }
>>           } else {
>>               printf("Deleting BootNext failed\n");
>> @@ -223,19 +225,20 @@ void *efi_bootmgr_load(struct efi_device_path
>> **device_path,
>>       bootorder = get_var(L"BootOrder", &efi_global_variable_guid,
>> &size);
>>       if (!bootorder) {
>>           printf("BootOrder not defined\n");
>> +        ret = EFI_NOT_FOUND;
>>           goto error;
>>       }
>>
>>       num = size / sizeof(uint16_t);
>>       for (i = 0; i < num; i++) {
>>           debug("%s: trying to load Boot%04X\n", __func__, bootorder[i]);
>> -        image = try_load_entry(bootorder[i], device_path, file_path);
>> -        if (image)
>> +        ret = try_load_entry(bootorder[i], handle);
>> +        if (ret == EFI_SUCCESS)
>>               break;
>>       }
>>
>>       free(bootorder);
>>
>>   error:
>> -    return image;
>> +    return ret;
>>   }
>> diff --git a/lib/efi_loader/efi_boottime.c
>> b/lib/efi_loader/efi_boottime.c
>> index abc295e392e9..19a58144cf4b 100644
>> --- a/lib/efi_loader/efi_boottime.c
>> +++ b/lib/efi_loader/efi_boottime.c
>> @@ -1591,6 +1591,7 @@ failure:
>>    * @size:    size of the loaded image
>>    * Return:    status code
>>    */
>> +static
>>   efi_status_t efi_load_image_from_path(struct efi_device_path
>> *file_path,
>>                         void **buffer, efi_uintn_t *size)
>>   {
>> @@ -2664,6 +2665,7 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t
>> image_handle,
>>       }
>>
>>       current_image = image_handle;
>> +    EFI_PRINT("Jumping into 0x%p\n", image_obj->entry);
>>       ret = EFI_CALL(image_obj->entry(image_handle, &systab));
>>
>>       /*
>>
> 
>