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