From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heinrich Schuchardt Date: Sat, 20 Apr 2019 10:37:54 +0200 Subject: [U-Boot] [PATCH 8/9] efi_loader: rework bootmgr/bootefi using load_image API In-Reply-To: <20190419032236.8242-9-takahiro.akashi@linaro.org> References: <20190419032236.8242-1-takahiro.akashi@linaro.org> <20190419032236.8242-9-takahiro.akashi@linaro.org> Message-ID: <797e7736-32dd-8bbf-457e-33d112fbe27d@gmx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 4/19/19 5:22 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 | 187 ++++++++++++++++++---------------- > include/efi_loader.h | 5 +- > lib/efi_loader/efi_bootmgr.c | 43 ++++---- > lib/efi_loader/efi_boottime.c | 2 + > 4 files changed, 127 insertions(+), 110 deletions(-) > > diff --git a/cmd/bootefi.c b/cmd/bootefi.c > index e2ca68c4dc12..8c84fff590a7 100644 > --- a/cmd/bootefi.c > +++ b/cmd/bootefi.c > @@ -242,89 +242,36 @@ 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; > > /* Transfer environment variable as load options */ > - ret = set_load_options((efi_handle_t)image_obj, "bootargs"); > - if (ret != EFI_SUCCESS) > - goto err_prepare; > - > - /* Load the EFI payload */ > - ret = efi_load_pe(image_obj, efi, loaded_image_info); > + ret = set_load_options(handle, "bootargs"); > 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; > - } > + return ret; > > /* we don't support much: */ > env_set("efi_8be4df61-93ca-11d2-aa0d-00e098032b8c_OsIndicationsSupported", > "{ro,boot}(blob)0000000000000000"); > > /* 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); > + /* > + * FIXME: Who is responsible for > + * free(loaded_image_info->load_options); > + * Once efi_exit() is implemented correctly, > + * handle itself doesn't exist here. > + */ > > return ret; > } > @@ -339,8 +286,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 */ > @@ -362,19 +308,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; > } > > /* > @@ -389,7 +335,12 @@ static int do_efibootmgr(const char *fdt_opt) > */ > static int do_bootefi_image(const char *image_opt, const char *fdt_opt) > { > - unsigned long addr; > + void *image_buf; > + struct efi_device_path *device_path, *image_path; > + struct efi_device_path *memdp = NULL, *file_path = NULL; > + unsigned long addr, size; > + const char *size_str; > + efi_handle_t mem_handle = NULL, handle; > efi_status_t ret; > > /* Allow unaligned memory access */ > @@ -414,33 +365,90 @@ static int do_bootefi_image(const char *image_opt, const char *fdt_opt) > #ifdef CONFIG_CMD_BOOTEFI_HELLO > if (!strcmp(image_opt, "hello")) { > char *saddr; > - ulong size = __efi_helloworld_end - __efi_helloworld_begin; > > 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 > { > + size_str = env_get("filesize"); > + if (size_str) > + size = simple_strtoul(size_str, NULL, 16); > + else > + size = 0; > + > addr = simple_strtoul(image_opt, NULL, 16); > /* Check that a numeric value was passed */ > if (!addr && *image_opt != '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: > + */ > + memdp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, > + (uintptr_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, 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 (file_path) /* hence memdp */ > + efi_free_pool(file_path); > > if (ret != EFI_SUCCESS) > return CMD_RET_FAILURE; > - else > - return CMD_RET_SUCCESS; > + > + return CMD_RET_SUCCESS; > } > > #ifdef CONFIG_CMD_BOOTEFI_SELFTEST > @@ -598,7 +606,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" > @@ -623,6 +631,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 93f7672aecbc..39ed8a6fa592 100644 > --- a/include/efi_loader.h > +++ b/include/efi_loader.h > @@ -412,8 +412,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); > > @@ -567,8 +565,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 */, In cmd/booefi.c you use efi_root. So we shall do the same here. I will fix that. No need to resend the patch. > + lo.file_path, NULL, 0, > + handle)); > if (ret != EFI_SUCCESS) > goto error; TODO: Now that you have the loaded image protocol you should update the load options with the value in load_option. I guess you do not want to overwrite it with the content of bootargs. A the problem was not introduced with this patch series: Reviewed-by: Heinrich Schuchardt > > @@ -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 abc295e392e9..19a58144cf4b 100644 > --- a/lib/efi_loader/efi_boottime.c > +++ b/lib/efi_loader/efi_boottime.c > @@ -1591,6 +1591,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) > { > @@ -2664,6 +2665,7 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle, > } > > current_image = image_handle; > + EFI_PRINT("Jumping into 0x%p\n", image_obj->entry); > ret = EFI_CALL(image_obj->entry(image_handle, &systab)); > > /* >