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 <takahiro.aka...@linaro.org>
> > >---
> > > cmd/bootefi.c | 197 +++++++++++++++++++---------------
> > > include/efi_loader.h | 5 +-
> > > lib/efi_loader/efi_bootmgr.c | 43 ++++----
> > > lib/efi_loader/efi_boottime.c | 2 +
> > > 4 files changed, 134 insertions(+), 113 deletions(-)
> > >
> > >diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> > >index f14966961b23..97d49a53a44b 100644
> > >--- a/cmd/bootefi.c
> > >+++ b/cmd/bootefi.c
> > >@@ -222,88 +222,39 @@ static efi_status_t efi_install_fdt(const char
> > >*fdt_opt)
> > > /**
> > > * do_bootefi_exec() - execute EFI binary
> > > *
> > >- * @efi: address of the binary
> > >- * @device_path: path of the device from which the binary was loaded
> > >- * @image_path: device path of the binary
> > >+ * @handle: handle of loaded image
> > > * Return: status code
> > > *
> > > * Load the EFI binary into a newly assigned memory unwinding the
> > > relocation
> > > * information, install the loaded image protocol, and call the binary.
> > > */
> > >-static efi_status_t do_bootefi_exec(void *efi,
> > >- struct efi_device_path *device_path,
> > >- struct efi_device_path *image_path)
> > >+static efi_status_t do_bootefi_exec(efi_handle_t handle)
> > > {
> > >- efi_handle_t mem_handle = NULL;
> > >- struct efi_device_path *memdp = NULL;
> > >- efi_status_t ret;
> > >- struct efi_loaded_image_obj *image_obj = NULL;
> > > struct efi_loaded_image *loaded_image_info = NULL;
> > >-
> > >- /*
> > >- * Special case for efi payload not loaded from disk, such as
> > >- * 'bootefi hello' or for example payload loaded directly into
> > >- * memory via JTAG, etc:
> > >- */
> > >- if (!device_path && !image_path) {
> > >- printf("WARNING: using memory device/image path, this may
> > >confuse some payloads!\n");
> > >- /* actual addresses filled in after efi_load_pe() */
> > >- memdp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, 0, 0);
> > >- device_path = image_path = memdp;
> > >- /*
> > >- * Grub expects that the device path of the loaded image is
> > >- * installed on a handle.
> > >- */
> > >- ret = efi_create_handle(&mem_handle);
> > >- if (ret != EFI_SUCCESS)
> > >- return ret; /* TODO: leaks device_path */
> > >- ret = efi_add_protocol(mem_handle, &efi_guid_device_path,
> > >- device_path);
> > >- if (ret != EFI_SUCCESS)
> > >- goto err_add_protocol;
> > >- } else {
> > >- assert(device_path && image_path);
> > >- }
> > >-
> > >- ret = efi_setup_loaded_image(device_path, image_path, &image_obj,
> > >- &loaded_image_info);
> > >- if (ret)
> > >- goto err_prepare;
> > >+ efi_status_t ret;
> > >
> > > /* Transfer environment variable as load options */
> > >- set_load_options(loaded_image_info, "bootargs");
> >
> > In set_load_options() an error could occur (out of memory). So I think
> > it should return a status.
>
> 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 <fdt address> gets\n"
> > >@@ -606,6 +618,13 @@ void efi_set_bootdev(const char *dev, const char
> > >*devnr, const char *path)
> > > ret = efi_dp_from_name(dev, devnr, path, &device, &image);
> > > if (ret == EFI_SUCCESS) {
> > > bootefi_device_path = device;
> > >+ if (image) {
> > >+ /* FIXME: image should not contain device */
> > >+ struct efi_device_path *image_tmp = image;
> > >+
> > >+ efi_dp_split_file_path(image, &device, &image);
> > >+ efi_free_pool(image_tmp);
> > >+ }
> > > bootefi_image_path = image;
> > > } else {
> > > bootefi_device_path = NULL;
> > >diff --git a/include/efi_loader.h b/include/efi_loader.h
> > >index d524dc7e24c1..c3eda2bb05d7 100644
> > >--- a/include/efi_loader.h
> > >+++ b/include/efi_loader.h
> > >@@ -408,8 +408,6 @@ efi_status_t efi_setup_loaded_image(struct
> > >efi_device_path *device_path,
> > > struct efi_device_path *file_path,
> > > struct efi_loaded_image_obj **handle_ptr,
> > > struct efi_loaded_image **info_ptr);
> > >-efi_status_t efi_load_image_from_path(struct efi_device_path *file_path,
> > >- void **buffer, efi_uintn_t *size);
> > > /* Print information about all loaded images */
> > > void efi_print_image_infos(void *pc);
> > >
> > >@@ -563,8 +561,7 @@ struct efi_load_option {
> > >
> > > void efi_deserialize_load_option(struct efi_load_option *lo, u8 *data);
> > > unsigned long efi_serialize_load_option(struct efi_load_option *lo, u8
> > > **data);
> > >-void *efi_bootmgr_load(struct efi_device_path **device_path,
> > >- struct efi_device_path **file_path);
> > >+efi_status_t efi_bootmgr_load(efi_handle_t *handle);
> > >
> > > #else /* CONFIG_IS_ENABLED(EFI_LOADER) */
> > >
> > >diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> > >index 4fccadc5483d..13ec79b2098b 100644
> > >--- a/lib/efi_loader/efi_bootmgr.c
> > >+++ b/lib/efi_loader/efi_bootmgr.c
> > >@@ -120,14 +120,14 @@ static void *get_var(u16 *name, const efi_guid_t
> > >*vendor,
> > > * if successful. This checks that the EFI_LOAD_OPTION is active
> > > (enabled)
> > > * and that the specified file to boot exists.
> > > */
> > >-static void *try_load_entry(uint16_t n, struct efi_device_path
> > >**device_path,
> > >- struct efi_device_path **file_path)
> > >+static efi_status_t try_load_entry(u16 n, efi_handle_t *handle)
> > > {
> > > struct efi_load_option lo;
> > > u16 varname[] = L"Boot0000";
> > > u16 hexmap[] = L"0123456789ABCDEF";
> > >- void *load_option, *image = NULL;
> > >+ void *load_option;
> > > efi_uintn_t size;
> > >+ efi_status_t ret;
> > >
> > > varname[4] = hexmap[(n & 0xf000) >> 12];
> > > varname[5] = hexmap[(n & 0x0f00) >> 8];
> > >@@ -136,19 +136,19 @@ static void *try_load_entry(uint16_t n, struct
> > >efi_device_path **device_path,
> > >
> > > load_option = get_var(varname, &efi_global_variable_guid, &size);
> > > if (!load_option)
> > >- return NULL;
> > >+ return EFI_LOAD_ERROR;
> > >
> > > efi_deserialize_load_option(&lo, load_option);
> > >
> > > if (lo.attributes & LOAD_OPTION_ACTIVE) {
> > > u32 attributes;
> > >- efi_status_t ret;
> > >
> > > debug("%s: trying to load \"%ls\" from %pD\n",
> > > __func__, lo.label, lo.file_path);
> > >
> > >- ret = efi_load_image_from_path(lo.file_path, &image, &size);
> > >-
> > >+ ret = EFI_CALL(efi_load_image(false, (void *)0x1 /* FIXME */,
> > >+ lo.file_path, NULL, 0,
> > >+ handle));
> > > if (ret != EFI_SUCCESS)
> > > goto error;
> > >
> > >@@ -159,17 +159,22 @@ static void *try_load_entry(uint16_t n, struct
> > >efi_device_path **device_path,
> > > L"BootCurrent",
> > > (efi_guid_t *)&efi_global_variable_guid,
> > > attributes, size, &n));
> > >- if (ret != EFI_SUCCESS)
> > >+ if (ret != EFI_SUCCESS) {
> > >+ if (EFI_CALL(efi_unload_image(*handle))
> > >+ != EFI_SUCCESS)
> > >+ printf("Unloading image failed\n");
> > > goto error;
> > >+ }
> > >
> > > printf("Booting: %ls\n", lo.label);
> > >- efi_dp_split_file_path(lo.file_path, device_path, file_path);
> > >+ } else {
> > >+ ret = EFI_LOAD_ERROR;
> > > }
> > >
> > > error:
> > > free(load_option);
> > >
> > >- return image;
> > >+ return ret;
> > > }
> > >
> > > /*
> > >@@ -177,12 +182,10 @@ error:
> > > * EFI variable, the available load-options, finding and returning
> > > * the first one that can be loaded successfully.
> > > */
> > >-void *efi_bootmgr_load(struct efi_device_path **device_path,
> > >- struct efi_device_path **file_path)
> > >+efi_status_t efi_bootmgr_load(efi_handle_t *handle)
> > > {
> > > u16 bootnext, *bootorder;
> > > efi_uintn_t size;
> > >- void *image = NULL;
> > > int i, num;
> > > efi_status_t ret;
> > >
> > >@@ -209,10 +212,9 @@ void *efi_bootmgr_load(struct efi_device_path
> > >**device_path,
> > > /* load BootNext */
> > > if (ret == EFI_SUCCESS) {
> > > if (size == sizeof(u16)) {
> > >- image = try_load_entry(bootnext, device_path,
> > >- file_path);
> > >- if (image)
> > >- return image;
> > >+ ret = try_load_entry(bootnext, handle);
> > >+ if (ret == EFI_SUCCESS)
> > >+ return ret;
> > > }
> > > } else {
> > > printf("Deleting BootNext failed\n");
> > >@@ -223,19 +225,20 @@ void *efi_bootmgr_load(struct efi_device_path
> > >**device_path,
> > > bootorder = get_var(L"BootOrder", &efi_global_variable_guid, &size);
> > > if (!bootorder) {
> > > printf("BootOrder not defined\n");
> > >+ ret = EFI_NOT_FOUND;
> > > goto error;
> > > }
> > >
> > > num = size / sizeof(uint16_t);
> > > for (i = 0; i < num; i++) {
> > > debug("%s: trying to load Boot%04X\n", __func__, bootorder[i]);
> > >- image = try_load_entry(bootorder[i], device_path, file_path);
> > >- if (image)
> > >+ ret = try_load_entry(bootorder[i], handle);
> > >+ if (ret == EFI_SUCCESS)
> > > break;
> > > }
> > >
> > > free(bootorder);
> > >
> > > error:
> > >- return image;
> > >+ return ret;
> > > }
> > >diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> > >index b215bd7723da..65425fabc08f 100644
> > >--- a/lib/efi_loader/efi_boottime.c
> > >+++ b/lib/efi_loader/efi_boottime.c
> > >@@ -1611,6 +1611,7 @@ failure:
> > > * @size: size of the loaded image
> > > * Return: status code
> > > */
> > >+static
> > > efi_status_t efi_load_image_from_path(struct efi_device_path *file_path,
> > > void **buffer, efi_uintn_t *size)
> > > {
> > >@@ -2684,6 +2685,7 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t
> > >image_handle,
> > > }
> > >
> > > current_image = image_handle;
> > >+ debug("EFI: Jumping into 0x%p\n", image_obj->entry);
> >
> > Please, use EFI_PRINT() here.
>
> 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));
> > >
> > > /*
> > >
> >
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
