[U-Boot] [RFC v3 09/10] efi_loader: rework bootmgr/bootefi using load_image API
Heinrich Schuchardt
xypron.glpk at gmx.de
Tue Apr 16 16:20:44 UTC 2019
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 at 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.
Best regards
Heinrich
>
>> + */
>> + 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);
>
> Couldn't we as well use the device path of the root node as "file_path"?
>
>> + if (ret != EFI_SUCCESS)
>> + goto err_add_protocol;
>> + } else {
>> + assert(device_path && image_path);
>> + file_path = efi_dp_append(device_path, image_path);
>
> Where is file_path freed?
>
>> + }
>> +
>> + 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 (memdp)
>> + efi_free_pool(memdp);
>>
>> if (ret != EFI_SUCCESS)
>> return CMD_RET_FAILURE;
>> - else
>> - return CMD_RET_SUCCESS;
>> +
>> + return CMD_RET_SUCCESS;
>> }
>>
>> #ifdef CONFIG_CMD_BOOTEFI_SELFTEST
>> @@ -581,7 +593,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"
>> @@ -606,6 +618,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 d524dc7e24c1..c3eda2bb05d7 100644
>> --- a/include/efi_loader.h
>> +++ b/include/efi_loader.h
>> @@ -408,8 +408,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);
>>
>> @@ -563,8 +561,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 */,
>> + lo.file_path, NULL, 0,
>> + handle));
>> if (ret != EFI_SUCCESS)
>> goto error;
>>
>> @@ -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 b215bd7723da..65425fabc08f 100644
>> --- a/lib/efi_loader/efi_boottime.c
>> +++ b/lib/efi_loader/efi_boottime.c
>> @@ -1611,6 +1611,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)
>> {
>> @@ -2684,6 +2685,7 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t
>> image_handle,
>> }
>>
>> current_image = image_handle;
>> + debug("EFI: Jumping into 0x%p\n", image_obj->entry);
>
> Please, use EFI_PRINT() here.
>
> Best regards
>
> Heinrich
>
>> ret = EFI_CALL(image_obj->entry(image_handle, &systab));
>>
>> /*
>>
>
>
More information about the U-Boot
mailing list