| Message ID | 20190327044042.13707-11-takahiro.akashi@linaro.org |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | efi_loader: rework bootefi/bootmgr | expand |
On 3/27/19 5:40 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 | 199 +++++++++++++++++++--------------- > include/efi_loader.h | 5 +- > lib/efi_loader/efi_bootmgr.c | 43 ++++---- > lib/efi_loader/efi_boottime.c | 1 + > 4 files changed, 138 insertions(+), 110 deletions(-) > > diff --git a/cmd/bootefi.c b/cmd/bootefi.c > index 39a0712af6ad..17dccd718008 100644 > --- a/cmd/bootefi.c > +++ b/cmd/bootefi.c > @@ -224,88 +224,43 @@ 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"); > - > - /* 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)); > 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"); > > /* Call our payload! */ > + image_obj = container_of(handle, struct efi_loaded_image_obj, header); This is not needed, if we remove the debug statement. > debug("%s: Jumping to 0x%p\n", __func__, image_obj->entry); That line should go to StartImage() if needed. Please, drop it here. > - 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); > + efi_free_pool(loaded_image_info); Wouldn't this be done by unload_image()? > > +err: > return ret; > } > > @@ -319,8 +274,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 +294,21 @@ static int do_efibootmgr(const char *fdt_opt) > 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); > + > + EFI_CALL(efi_unload_image(handle)); Only applications have to be unloaded, not drivers. Unloading is to be handled by exit(), see UEFI spec. > > if (ret != EFI_SUCCESS) > - return 1; > + return CMD_RET_FAILURE; > > - return 0; > + return CMD_RET_SUCCESS; > } > > /* > @@ -367,10 +323,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(); > @@ -390,36 +350,96 @@ 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; > + } > + > + 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"); > + > + /* 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. > + */ > + 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, (void *)0x1 /* FIXME */, > + file_path, image_buf, size, &handle)); > + if (ret != EFI_SUCCESS) > + goto err_prepare; > + > 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); > + ret = do_bootefi_exec(handle); > + printf("## Application terminated, r = %lu\n", ret & ~EFI_ERROR_MASK); Why should we need all this duplicate code. Cant we move that to do_bootefi_exec()? > + > + EFI_CALL(efi_unload_image(handle)); That is the task of efi_exit(). Best regards Heinrich > +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 > @@ -577,7 +597,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" > @@ -602,6 +622,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 6dfa2fef3cf0..6c09a7f40d02 100644 > --- a/include/efi_loader.h > +++ b/include/efi_loader.h > @@ -410,8 +410,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); > > @@ -565,8 +563,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 74da6b01054a..8e2fa0111591 100644 > --- a/lib/efi_loader/efi_boottime.c > +++ b/lib/efi_loader/efi_boottime.c > @@ -1610,6 +1610,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) > { >
On Fri, Apr 12, 2019 at 07:53:25AM +0200, Heinrich Schuchardt wrote: > On 3/27/19 5:40 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 | 199 +++++++++++++++++++--------------- > > include/efi_loader.h | 5 +- > > lib/efi_loader/efi_bootmgr.c | 43 ++++---- > > lib/efi_loader/efi_boottime.c | 1 + > > 4 files changed, 138 insertions(+), 110 deletions(-) > > > >diff --git a/cmd/bootefi.c b/cmd/bootefi.c > >index 39a0712af6ad..17dccd718008 100644 > >--- a/cmd/bootefi.c > >+++ b/cmd/bootefi.c > >@@ -224,88 +224,43 @@ 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"); > >- > >- /* 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)); > > 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"); > > > > /* Call our payload! */ > >+ image_obj = container_of(handle, struct efi_loaded_image_obj, header); > This is not needed, if we remove the debug statement. > > > debug("%s: Jumping to 0x%p\n", __func__, image_obj->entry); > That line should go to StartImage() if needed. Please, drop it here. > > >- 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); > >+ efi_free_pool(loaded_image_info); > > Wouldn't this be done by unload_image()? but we should not call unload_image(), efi_exit() does. Right? > > > >+err: > > return ret; > > } > > > >@@ -319,8 +274,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 +294,21 @@ static int do_efibootmgr(const char *fdt_opt) > > 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); > >+ > >+ EFI_CALL(efi_unload_image(handle)); > > Only applications have to be unloaded, not drivers. Unloading is to be > handled by exit(), see UEFI spec. How do we know whether the loaded image is an application, or driver? > > > > if (ret != EFI_SUCCESS) > >- return 1; > >+ return CMD_RET_FAILURE; > > > >- return 0; > >+ return CMD_RET_SUCCESS; > > } > > > > /* > >@@ -367,10 +323,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(); > >@@ -390,36 +350,96 @@ 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; > >+ } > >+ > >+ 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"); > >+ > >+ /* 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. > >+ */ > >+ 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, (void *)0x1 /* FIXME */, > >+ file_path, image_buf, size, &handle)); > >+ if (ret != EFI_SUCCESS) > >+ goto err_prepare; > >+ > > 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); > >+ ret = do_bootefi_exec(handle); > >+ printf("## Application terminated, r = %lu\n", ret & ~EFI_ERROR_MASK); > > Why should we need all this duplicate code. Cant we move that to > do_bootefi_exec()? > > >+ > >+ EFI_CALL(efi_unload_image(handle)); > > That is the task of efi_exit(). Who should be blamed for reclaiming a handle which is created with load_image()? What if an application doesn't call efi_exit() by itself? -Takahiro Akashi > Best regards > > Heinrich > > >+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 > >@@ -577,7 +597,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" > >@@ -602,6 +622,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 6dfa2fef3cf0..6c09a7f40d02 100644 > >--- a/include/efi_loader.h > >+++ b/include/efi_loader.h > >@@ -410,8 +410,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); > > > >@@ -565,8 +563,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 74da6b01054a..8e2fa0111591 100644 > >--- a/lib/efi_loader/efi_boottime.c > >+++ b/lib/efi_loader/efi_boottime.c > >@@ -1610,6 +1610,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) > > { > > >
On 4/12/19 10:38 AM, AKASHI Takahiro wrote: > On Fri, Apr 12, 2019 at 07:53:25AM +0200, Heinrich Schuchardt wrote: >> On 3/27/19 5:40 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 | 199 +++++++++++++++++++--------------- >>> include/efi_loader.h | 5 +- >>> lib/efi_loader/efi_bootmgr.c | 43 ++++---- >>> lib/efi_loader/efi_boottime.c | 1 + >>> 4 files changed, 138 insertions(+), 110 deletions(-) >>> >>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c >>> index 39a0712af6ad..17dccd718008 100644 >>> --- a/cmd/bootefi.c >>> +++ b/cmd/bootefi.c >>> @@ -224,88 +224,43 @@ 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"); >>> - >>> - /* 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)); >>> 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"); >>> >>> /* Call our payload! */ >>> + image_obj = container_of(handle, struct efi_loaded_image_obj, header); >> This is not needed, if we remove the debug statement. >> >>> debug("%s: Jumping to 0x%p\n", __func__, image_obj->entry); >> That line should go to StartImage() if needed. Please, drop it here. >> >>> - 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); >>> + efi_free_pool(loaded_image_info); >> >> Wouldn't this be done by unload_image()? > > but we should not call unload_image(), efi_exit() does. Right? > Yes, correct. >>> >>> +err: >>> return ret; >>> } >>> >>> @@ -319,8 +274,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 +294,21 @@ static int do_efibootmgr(const char *fdt_opt) >>> 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); >>> + >>> + EFI_CALL(efi_unload_image(handle)); >> >> Only applications have to be unloaded, not drivers. Unloading is to be >> handled by exit(), see UEFI spec. > > How do we know whether the loaded image is an application, or driver? In efi_load_pe() we should evaluate the field opt->Subsystem describing the "Windows Subsystem". I once started on this but then I saw that cmd/bootefi.c needed to be cleaned up first: https://github.com/xypron/u-boot-patches/blob/efi-next/0001-efi_loader-implement-UnloadImage.patch > >>> >>> if (ret != EFI_SUCCESS) >>> - return 1; >>> + return CMD_RET_FAILURE; >>> >>> - return 0; >>> + return CMD_RET_SUCCESS; >>> } >>> >>> /* >>> @@ -367,10 +323,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(); >>> @@ -390,36 +350,96 @@ 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; >>> + } >>> + >>> + 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"); >>> + >>> + /* 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. >>> + */ >>> + 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, (void *)0x1 /* FIXME */, >>> + file_path, image_buf, size, &handle)); >>> + if (ret != EFI_SUCCESS) >>> + goto err_prepare; >>> + >>> 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); >>> + ret = do_bootefi_exec(handle); >>> + printf("## Application terminated, r = %lu\n", ret & ~EFI_ERROR_MASK); >> >> Why should we need all this duplicate code. Cant we move that to >> do_bootefi_exec()? >> >>> + >>> + EFI_CALL(efi_unload_image(handle)); >> >> That is the task of efi_exit(). > > Who should be blamed for reclaiming a handle which is created > with load_image()? > What if an application doesn't call efi_exit() by itself? When the last protocol interface is deleted from a handle via UninstallProtocolInterface() the handle is deleted. Best regards Heinrich > > -Takahiro Akashi > > >> Best regards >> >> Heinrich >> >>> +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 >>> @@ -577,7 +597,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" >>> @@ -602,6 +622,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 6dfa2fef3cf0..6c09a7f40d02 100644 >>> --- a/include/efi_loader.h >>> +++ b/include/efi_loader.h >>> @@ -410,8 +410,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); >>> >>> @@ -565,8 +563,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 74da6b01054a..8e2fa0111591 100644 >>> --- a/lib/efi_loader/efi_boottime.c >>> +++ b/lib/efi_loader/efi_boottime.c >>> @@ -1610,6 +1610,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) >>> { >>> >> >
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 39a0712af6ad..17dccd718008 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -224,88 +224,43 @@ 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"); - - /* 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)); 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"); /* Call our payload! */ + image_obj = container_of(handle, struct efi_loaded_image_obj, header); 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); + efi_free_pool(loaded_image_info); +err: return ret; } @@ -319,8 +274,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 +294,21 @@ static int do_efibootmgr(const char *fdt_opt) 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); + + EFI_CALL(efi_unload_image(handle)); if (ret != EFI_SUCCESS) - return 1; + return CMD_RET_FAILURE; - return 0; + return CMD_RET_SUCCESS; } /* @@ -367,10 +323,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(); @@ -390,36 +350,96 @@ 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; + } + + 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"); + + /* 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. + */ + 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, (void *)0x1 /* FIXME */, + file_path, image_buf, size, &handle)); + if (ret != EFI_SUCCESS) + goto err_prepare; + 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); + ret = do_bootefi_exec(handle); + printf("## Application terminated, r = %lu\n", ret & ~EFI_ERROR_MASK); + + EFI_CALL(efi_unload_image(handle)); +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 @@ -577,7 +597,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" @@ -602,6 +622,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 6dfa2fef3cf0..6c09a7f40d02 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -410,8 +410,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); @@ -565,8 +563,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 74da6b01054a..8e2fa0111591 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -1610,6 +1610,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) {
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 | 199 +++++++++++++++++++--------------- include/efi_loader.h | 5 +- lib/efi_loader/efi_bootmgr.c | 43 ++++---- lib/efi_loader/efi_boottime.c | 1 + 4 files changed, 138 insertions(+), 110 deletions(-)