From mboxrd@z Thu Jan 1 00:00:00 1970 From: AKASHI Takahiro Date: Thu, 18 Apr 2019 12:06:45 +0900 Subject: [U-Boot] [RFC v3 09/10] efi_loader: rework bootmgr/bootefi using load_image API In-Reply-To: <20190418001351.GU7158@linaro.org> References: <20190416042428.5007-1-takahiro.akashi@linaro.org> <20190416042428.5007-10-takahiro.akashi@linaro.org> <7f0909bb-4921-bd56-a2ba-32f033c2e066@gmx.de> <20190418001351.GU7158@linaro.org> Message-ID: <20190418030643.GW7158@linaro.org> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Correction: On Thu, Apr 18, 2019 at 09:13:52AM +0900, AKASHI Takahiro wrote: > On Tue, Apr 16, 2019 at 12:56:32PM +0200, 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 > > >--- > > > 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. > > I didn't change anything in set_load_options(), but I will follow > your comment. > > > >- > > >- /* 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()? > > No. This line has nothing to do with "load options." Wrong, I will follow your comment. This change, however, uncovers one issue: Who is responsible for freeing loaded_image_info->load_option, particularly, when efi_exit()/unload_image() is fully implemented? In such a case, we have no chance to access this handle, hence loaded_image_info, after returning from efi_start_image(). Thanks, -Takahiro Akashi > > > 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. > > Yeah, I am aware of this issue. > Will fix in a separate 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(). > > So are you suggesting we should remove this message? > > > >+ > > >+ /* 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? > > I will comment on another reply. > > > >+ */ > > >+ 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"? > > I don't get you point very well, but > it will depend on how we should fix a grub issue mentioned above. > > > >+ 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? > > In this function. Will fix. > > > >+ } > > >+ > > >+ 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 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. > > Basically I agree, but there are lots of debug()'s in this file. > We should replace them all at once later. > > Thanks, > -Takahiro Akashi > > > Best regards > > > > Heinrich > > > > > ret = EFI_CALL(image_obj->entry(image_handle, &systab)); > > > > > > /* > > > > >