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.
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));
/*