Message ID | 20190416042428.5007-10-takahiro.akashi@linaro.org |
---|---|
State | Changes Requested, archived |
Headers | show |
Series | efi_loader: rework bootefi/bootmgr | expand |
On 4/16/19 6:24 AM, AKASHI Takahiro wrote: > In the current implementation, bootefi command and EFI boot manager > don't use load_image API, instead, use more primitive and internal > functions. This will introduce duplicated code and potentially > unknown bugs as well as inconsistent behaviours. > > With this patch, do_efibootmgr() and do_boot_efi() are completely > overhauled and re-implemented using load_image API. > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > --- > cmd/bootefi.c | 197 +++++++++++++++++++--------------- > include/efi_loader.h | 5 +- > lib/efi_loader/efi_bootmgr.c | 43 ++++---- > lib/efi_loader/efi_boottime.c | 2 + > 4 files changed, 134 insertions(+), 113 deletions(-) > > diff --git a/cmd/bootefi.c b/cmd/bootefi.c > index f14966961b23..97d49a53a44b 100644 > --- a/cmd/bootefi.c > +++ b/cmd/bootefi.c > @@ -222,88 +222,39 @@ static efi_status_t efi_install_fdt(const char *fdt_opt) > /** > * do_bootefi_exec() - execute EFI binary > * > - * @efi: address of the binary > - * @device_path: path of the device from which the binary was loaded > - * @image_path: device path of the binary > + * @handle: handle of loaded image > * Return: status code > * > * Load the EFI binary into a newly assigned memory unwinding the relocation > * information, install the loaded image protocol, and call the binary. > */ > -static efi_status_t do_bootefi_exec(void *efi, > - struct efi_device_path *device_path, > - struct efi_device_path *image_path) > +static efi_status_t do_bootefi_exec(efi_handle_t handle) > { > - efi_handle_t mem_handle = NULL; > - struct efi_device_path *memdp = NULL; > - efi_status_t ret; > - struct efi_loaded_image_obj *image_obj = NULL; > struct efi_loaded_image *loaded_image_info = NULL; > - > - /* > - * Special case for efi payload not loaded from disk, such as > - * 'bootefi hello' or for example payload loaded directly into > - * memory via JTAG, etc: > - */ > - if (!device_path && !image_path) { > - printf("WARNING: using memory device/image path, this may confuse some payloads!\n"); > - /* actual addresses filled in after efi_load_pe() */ > - memdp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, 0, 0); > - device_path = image_path = memdp; > - /* > - * Grub expects that the device path of the loaded image is > - * installed on a handle. > - */ > - ret = efi_create_handle(&mem_handle); > - if (ret != EFI_SUCCESS) > - return ret; /* TODO: leaks device_path */ > - ret = efi_add_protocol(mem_handle, &efi_guid_device_path, > - device_path); > - if (ret != EFI_SUCCESS) > - goto err_add_protocol; > - } else { > - assert(device_path && image_path); > - } > - > - ret = efi_setup_loaded_image(device_path, image_path, &image_obj, > - &loaded_image_info); > - if (ret) > - goto err_prepare; > + efi_status_t ret; > > /* Transfer environment variable as load options */ > - set_load_options(loaded_image_info, "bootargs"); In set_load_options() an error could occur (out of memory). So I think it should return a status. > - > - /* Load the EFI payload */ > - ret = efi_load_pe(image_obj, efi, loaded_image_info); > + ret = EFI_CALL(systab.boottime->open_protocol( > + handle, > + &efi_guid_loaded_image, > + (void **)&loaded_image_info, > + NULL, NULL, > + EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL)); Shouldn't we move this to set_load_options()? > if (ret != EFI_SUCCESS) > - goto err_prepare; > - > - if (memdp) { > - struct efi_device_path_memory *mdp = (void *)memdp; > - mdp->memory_type = loaded_image_info->image_code_type; > - mdp->start_address = (uintptr_t)loaded_image_info->image_base; > - mdp->end_address = mdp->start_address + > - loaded_image_info->image_size; > - } > + goto err; > + set_load_options(loaded_image_info, "bootargs"); > > /* we don't support much: */ > env_set("efi_8be4df61-93ca-11d2-aa0d-00e098032b8c_OsIndicationsSupported", > "{ro,boot}(blob)0000000000000000"); Shouldn't this move to efi_setup.c? Probably we should also set OsIndications. I would prefer using efi_set_variable(). I think moving this could be done in a preparatory patch. > > /* Call our payload! */ > - debug("%s: Jumping to 0x%p\n", __func__, image_obj->entry); > - ret = EFI_CALL(efi_start_image(&image_obj->header, NULL, NULL)); > + ret = EFI_CALL(efi_start_image(handle, NULL, NULL)); > > -err_prepare: > - /* image has returned, loaded-image obj goes *poof*: */ > efi_restore_gd(); > free(loaded_image_info->load_options); > - efi_delete_handle(&image_obj->header); > - > -err_add_protocol: > - if (mem_handle) > - efi_delete_handle(mem_handle); > > +err: > return ret; > } > > @@ -317,8 +268,7 @@ err_add_protocol: > */ > static int do_efibootmgr(const char *fdt_opt) > { > - struct efi_device_path *device_path, *file_path; > - void *addr; > + efi_handle_t handle; > efi_status_t ret; > > /* Allow unaligned memory access */ > @@ -340,19 +290,19 @@ static int do_efibootmgr(const char *fdt_opt) > else if (ret != EFI_SUCCESS) > return CMD_RET_FAILURE; > > - addr = efi_bootmgr_load(&device_path, &file_path); > - if (!addr) > - return 1; > + ret = efi_bootmgr_load(&handle); > + if (ret != EFI_SUCCESS) { > + printf("EFI boot manager: Cannot load any image\n"); > + return CMD_RET_FAILURE; > + } > > - printf("## Starting EFI application at %p ...\n", addr); > - ret = do_bootefi_exec(addr, device_path, file_path); > - printf("## Application terminated, r = %lu\n", > - ret & ~EFI_ERROR_MASK); > + ret = do_bootefi_exec(handle); > + printf("## Application terminated, r = %lu\n", ret & ~EFI_ERROR_MASK); > > if (ret != EFI_SUCCESS) > - return 1; > + return CMD_RET_FAILURE; > > - return 0; > + return CMD_RET_SUCCESS; > } > > /* > @@ -367,10 +317,14 @@ static int do_efibootmgr(const char *fdt_opt) > */ > static int do_boot_efi(const char *image_opt, const char *fdt_opt) > { > - unsigned long addr; > - char *saddr; > + void *image_buf; > + struct efi_device_path *device_path, *image_path; > + struct efi_device_path *memdp = NULL, *file_path = NULL; > + const char *saddr; > + unsigned long addr, size; > + const char *size_str; > + efi_handle_t mem_handle = NULL, handle; > efi_status_t ret; > - unsigned long fdt_addr; > > /* Allow unaligned memory access */ > allow_unaligned(); > @@ -392,36 +346,94 @@ static int do_boot_efi(const char *image_opt, const char *fdt_opt) > return CMD_RET_FAILURE; > > #ifdef CONFIG_CMD_BOOTEFI_HELLO > - if (!strcmp(argv[1], "hello")) { > - ulong size = __efi_helloworld_end - __efi_helloworld_begin; > - > + if (!strcmp(image_opt, "hello")) { > saddr = env_get("loadaddr"); > + size = __efi_helloworld_end - __efi_helloworld_begin; > + > if (saddr) > addr = simple_strtoul(saddr, NULL, 16); > else > addr = CONFIG_SYS_LOAD_ADDR; > - memcpy(map_sysmem(addr, size), __efi_helloworld_begin, size); > + > + image_buf = map_sysmem(addr, size); > + memcpy(image_buf, __efi_helloworld_begin, size); > + > + device_path = NULL; > + image_path = NULL; > } else > #endif > { > - saddr = argv[1]; > + saddr = image_opt; > + size_str = env_get("filesize"); > + if (size_str) > + size = simple_strtoul(size_str, NULL, 16); > + else > + size = 0; > > addr = simple_strtoul(saddr, NULL, 16); > /* Check that a numeric value was passed */ > if (!addr && *saddr != '0') > return CMD_RET_USAGE; > + > + image_buf = map_sysmem(addr, size); > + > + device_path = bootefi_device_path; > + image_path = bootefi_image_path; > } > > - printf("## Starting EFI application at %08lx ...\n", addr); > - ret = do_bootefi_exec(map_sysmem(addr, 0), bootefi_device_path, > - bootefi_image_path); > - printf("## Application terminated, r = %lu\n", > - ret & ~EFI_ERROR_MASK); > + if (!device_path && !image_path) { > + /* > + * Special case for efi payload not loaded from disk, > + * such as 'bootefi hello' or for example payload > + * loaded directly into memory via JTAG, etc: > + */ > + printf("WARNING: using memory device/image path, this may confuse some payloads!\n"); The EFI spec says that either of SourceBuffer or DevicePath may be NULL when calling LoadImage(). > + > + /* actual addresses filled in after efi_load_image() */ > + memdp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, > + (uint64_t)image_buf, size); > + file_path = memdp; /* append(memdp, NULL) */ > + > + /* > + * Make sure that device for device_path exist > + * in load_image(). Otherwise, shell and grub will fail. GRUB will fail anyway because it cannot determine the disk from which it was loaded. So why are we doing this? > + */ > + 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"? > + 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? > + } > + > + 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. Best regards Heinrich > ret = EFI_CALL(image_obj->entry(image_handle, &systab)); > > /* >
On 4/16/19 12:56 PM, Heinrich Schuchardt wrote: > On 4/16/19 6:24 AM, AKASHI Takahiro wrote: >> In the current implementation, bootefi command and EFI boot manager >> don't use load_image API, instead, use more primitive and internal >> functions. This will introduce duplicated code and potentially >> unknown bugs as well as inconsistent behaviours. >> >> With this patch, do_efibootmgr() and do_boot_efi() are completely >> overhauled and re-implemented using load_image API. >> >> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> >> --- >> cmd/bootefi.c | 197 +++++++++++++++++++--------------- >> include/efi_loader.h | 5 +- >> lib/efi_loader/efi_bootmgr.c | 43 ++++---- >> lib/efi_loader/efi_boottime.c | 2 + >> 4 files changed, 134 insertions(+), 113 deletions(-) >> >> diff --git a/cmd/bootefi.c b/cmd/bootefi.c >> index f14966961b23..97d49a53a44b 100644 >> --- a/cmd/bootefi.c >> +++ b/cmd/bootefi.c >> @@ -222,88 +222,39 @@ static efi_status_t efi_install_fdt(const char >> *fdt_opt) >> /** >> * do_bootefi_exec() - execute EFI binary >> * >> - * @efi: address of the binary >> - * @device_path: path of the device from which the binary was loaded >> - * @image_path: device path of the binary >> + * @handle: handle of loaded image >> * Return: status code >> * >> * Load the EFI binary into a newly assigned memory unwinding the >> relocation >> * information, install the loaded image protocol, and call the binary. >> */ >> -static efi_status_t do_bootefi_exec(void *efi, >> - struct efi_device_path *device_path, >> - struct efi_device_path *image_path) >> +static efi_status_t do_bootefi_exec(efi_handle_t handle) >> { >> - efi_handle_t mem_handle = NULL; >> - struct efi_device_path *memdp = NULL; >> - efi_status_t ret; >> - struct efi_loaded_image_obj *image_obj = NULL; >> struct efi_loaded_image *loaded_image_info = NULL; >> - >> - /* >> - * Special case for efi payload not loaded from disk, such as >> - * 'bootefi hello' or for example payload loaded directly into >> - * memory via JTAG, etc: >> - */ >> - if (!device_path && !image_path) { >> - printf("WARNING: using memory device/image path, this may >> confuse some payloads!\n"); >> - /* actual addresses filled in after efi_load_pe() */ >> - memdp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, 0, 0); >> - device_path = image_path = memdp; >> - /* >> - * Grub expects that the device path of the loaded image is >> - * installed on a handle. >> - */ >> - ret = efi_create_handle(&mem_handle); >> - if (ret != EFI_SUCCESS) >> - return ret; /* TODO: leaks device_path */ >> - ret = efi_add_protocol(mem_handle, &efi_guid_device_path, >> - device_path); >> - if (ret != EFI_SUCCESS) >> - goto err_add_protocol; >> - } else { >> - assert(device_path && image_path); >> - } >> - >> - ret = efi_setup_loaded_image(device_path, image_path, &image_obj, >> - &loaded_image_info); >> - if (ret) >> - goto err_prepare; >> + efi_status_t ret; >> >> /* Transfer environment variable as load options */ >> - set_load_options(loaded_image_info, "bootargs"); > > In set_load_options() an error could occur (out of memory). So I think > it should return a status. > >> - >> - /* Load the EFI payload */ >> - ret = efi_load_pe(image_obj, efi, loaded_image_info); >> + ret = EFI_CALL(systab.boottime->open_protocol( >> + handle, >> + &efi_guid_loaded_image, >> + (void **)&loaded_image_info, >> + NULL, NULL, >> + EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL)); > > Shouldn't we move this to set_load_options()? > >> if (ret != EFI_SUCCESS) >> - goto err_prepare; >> - >> - if (memdp) { >> - struct efi_device_path_memory *mdp = (void *)memdp; >> - mdp->memory_type = loaded_image_info->image_code_type; >> - mdp->start_address = (uintptr_t)loaded_image_info->image_base; >> - mdp->end_address = mdp->start_address + >> - loaded_image_info->image_size; >> - } >> + goto err; >> + set_load_options(loaded_image_info, "bootargs"); >> >> /* we don't support much: */ >> >> env_set("efi_8be4df61-93ca-11d2-aa0d-00e098032b8c_OsIndicationsSupported", >> >> "{ro,boot}(blob)0000000000000000"); > > Shouldn't this move to efi_setup.c? Probably we should also set > OsIndications. I would prefer using efi_set_variable(). I think moving > this could be done in a preparatory patch. > >> >> /* Call our payload! */ >> - debug("%s: Jumping to 0x%p\n", __func__, image_obj->entry); >> - ret = EFI_CALL(efi_start_image(&image_obj->header, NULL, NULL)); >> + ret = EFI_CALL(efi_start_image(handle, NULL, NULL)); >> >> -err_prepare: >> - /* image has returned, loaded-image obj goes *poof*: */ >> efi_restore_gd(); >> free(loaded_image_info->load_options); >> - efi_delete_handle(&image_obj->header); >> - >> -err_add_protocol: >> - if (mem_handle) >> - efi_delete_handle(mem_handle); >> >> +err: >> return ret; >> } >> >> @@ -317,8 +268,7 @@ err_add_protocol: >> */ >> static int do_efibootmgr(const char *fdt_opt) >> { >> - struct efi_device_path *device_path, *file_path; >> - void *addr; >> + efi_handle_t handle; >> efi_status_t ret; >> >> /* Allow unaligned memory access */ >> @@ -340,19 +290,19 @@ static int do_efibootmgr(const char *fdt_opt) >> else if (ret != EFI_SUCCESS) >> return CMD_RET_FAILURE; >> >> - addr = efi_bootmgr_load(&device_path, &file_path); >> - if (!addr) >> - return 1; >> + ret = efi_bootmgr_load(&handle); >> + if (ret != EFI_SUCCESS) { >> + printf("EFI boot manager: Cannot load any image\n"); >> + return CMD_RET_FAILURE; >> + } >> >> - printf("## Starting EFI application at %p ...\n", addr); >> - ret = do_bootefi_exec(addr, device_path, file_path); >> - printf("## Application terminated, r = %lu\n", >> - ret & ~EFI_ERROR_MASK); >> + ret = do_bootefi_exec(handle); >> + printf("## Application terminated, r = %lu\n", ret & >> ~EFI_ERROR_MASK); >> >> if (ret != EFI_SUCCESS) >> - return 1; >> + return CMD_RET_FAILURE; >> >> - return 0; >> + return CMD_RET_SUCCESS; >> } >> >> /* >> @@ -367,10 +317,14 @@ static int do_efibootmgr(const char *fdt_opt) >> */ >> static int do_boot_efi(const char *image_opt, const char *fdt_opt) >> { >> - unsigned long addr; >> - char *saddr; >> + void *image_buf; >> + struct efi_device_path *device_path, *image_path; >> + struct efi_device_path *memdp = NULL, *file_path = NULL; >> + const char *saddr; >> + unsigned long addr, size; >> + const char *size_str; >> + efi_handle_t mem_handle = NULL, handle; >> efi_status_t ret; >> - unsigned long fdt_addr; >> >> /* Allow unaligned memory access */ >> allow_unaligned(); >> @@ -392,36 +346,94 @@ static int do_boot_efi(const char *image_opt, >> const char *fdt_opt) >> return CMD_RET_FAILURE; >> >> #ifdef CONFIG_CMD_BOOTEFI_HELLO >> - if (!strcmp(argv[1], "hello")) { >> - ulong size = __efi_helloworld_end - __efi_helloworld_begin; >> - >> + if (!strcmp(image_opt, "hello")) { >> saddr = env_get("loadaddr"); >> + size = __efi_helloworld_end - __efi_helloworld_begin; >> + >> if (saddr) >> addr = simple_strtoul(saddr, NULL, 16); >> else >> addr = CONFIG_SYS_LOAD_ADDR; >> - memcpy(map_sysmem(addr, size), __efi_helloworld_begin, size); >> + >> + image_buf = map_sysmem(addr, size); >> + memcpy(image_buf, __efi_helloworld_begin, size); >> + >> + device_path = NULL; >> + image_path = NULL; >> } else >> #endif >> { >> - saddr = argv[1]; >> + saddr = image_opt; >> + size_str = env_get("filesize"); >> + if (size_str) >> + size = simple_strtoul(size_str, NULL, 16); >> + else >> + size = 0; >> >> addr = simple_strtoul(saddr, NULL, 16); >> /* Check that a numeric value was passed */ >> if (!addr && *saddr != '0') >> return CMD_RET_USAGE; >> + >> + image_buf = map_sysmem(addr, size); >> + >> + device_path = bootefi_device_path; >> + image_path = bootefi_image_path; >> } >> >> - printf("## Starting EFI application at %08lx ...\n", addr); >> - ret = do_bootefi_exec(map_sysmem(addr, 0), bootefi_device_path, >> - bootefi_image_path); >> - printf("## Application terminated, r = %lu\n", >> - ret & ~EFI_ERROR_MASK); >> + if (!device_path && !image_path) { >> + /* >> + * Special case for efi payload not loaded from disk, >> + * such as 'bootefi hello' or for example payload >> + * loaded directly into memory via JTAG, etc: >> + */ >> + printf("WARNING: using memory device/image path, this may >> confuse some payloads!\n"); > > The EFI spec says that either of SourceBuffer or DevicePath may be NULL > when calling LoadImage(). > >> + >> + /* actual addresses filled in after efi_load_image() */ >> + memdp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, >> + (uint64_t)image_buf, size); >> + file_path = memdp; /* append(memdp, NULL) */ >> + >> + /* >> + * Make sure that device for device_path exist >> + * in load_image(). Otherwise, shell and grub will fail. > > GRUB will fail anyway because it cannot determine the disk from which it > was loaded. So why are we doing this? In Rob's original commit bf19273e81eb ("efi_loader: Add mem-mapped for fallback") the comment was: "This fixes 'bootefi hello' after devicepath refactoring." EFI Shell.efi starts fine even when we set device_path and image_path to NULL. When loading GRUB from disk or via tftp the device path and the image path are set, e.g. for tftp: => dhcp grubarm.efi => bootefi 0x40200000 $fdtcontroladdr Found 0 disks ## Starting EFI application at 40200000 ... device_path: /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/MAC(525400123456,0x1) image_path: /grubarm.efi error: disk `,msdos2' not found. Entering rescue mode... grub rescue> Maybe GRUB would run if all necessary modules were compiled in to do a network boot. But how would it find grub.conf? So my feeling is that we are creating the dummy memory device path because: - Once `bootefi hello` which is our own test program had a problem. - GRUB has a bug: it hangs or crashes if the file device path is not set in LoadImage(). I think the problem should not be fixed in U-Boot but in GRUB. I am CC-ing Leif due to all the attention he has paid both to U-Boot and to GRUB. Best regards Heinrich > >> + */ >> + 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"? > >> + 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? > >> + } >> + >> + 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. > > Best regards > > Heinrich > >> ret = EFI_CALL(image_obj->entry(image_handle, &systab)); >> >> /* >> > >
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.akashi@linaro.org> > >--- > > cmd/bootefi.c | 197 +++++++++++++++++++--------------- > > include/efi_loader.h | 5 +- > > lib/efi_loader/efi_bootmgr.c | 43 ++++---- > > lib/efi_loader/efi_boottime.c | 2 + > > 4 files changed, 134 insertions(+), 113 deletions(-) > > > >diff --git a/cmd/bootefi.c b/cmd/bootefi.c > >index f14966961b23..97d49a53a44b 100644 > >--- a/cmd/bootefi.c > >+++ b/cmd/bootefi.c > >@@ -222,88 +222,39 @@ static efi_status_t efi_install_fdt(const char *fdt_opt) > > /** > > * do_bootefi_exec() - execute EFI binary > > * > >- * @efi: address of the binary > >- * @device_path: path of the device from which the binary was loaded > >- * @image_path: device path of the binary > >+ * @handle: handle of loaded image > > * Return: status code > > * > > * Load the EFI binary into a newly assigned memory unwinding the relocation > > * information, install the loaded image protocol, and call the binary. > > */ > >-static efi_status_t do_bootefi_exec(void *efi, > >- struct efi_device_path *device_path, > >- struct efi_device_path *image_path) > >+static efi_status_t do_bootefi_exec(efi_handle_t handle) > > { > >- efi_handle_t mem_handle = NULL; > >- struct efi_device_path *memdp = NULL; > >- efi_status_t ret; > >- struct efi_loaded_image_obj *image_obj = NULL; > > struct efi_loaded_image *loaded_image_info = NULL; > >- > >- /* > >- * Special case for efi payload not loaded from disk, such as > >- * 'bootefi hello' or for example payload loaded directly into > >- * memory via JTAG, etc: > >- */ > >- if (!device_path && !image_path) { > >- printf("WARNING: using memory device/image path, this may confuse some payloads!\n"); > >- /* actual addresses filled in after efi_load_pe() */ > >- memdp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, 0, 0); > >- device_path = image_path = memdp; > >- /* > >- * Grub expects that the device path of the loaded image is > >- * installed on a handle. > >- */ > >- ret = efi_create_handle(&mem_handle); > >- if (ret != EFI_SUCCESS) > >- return ret; /* TODO: leaks device_path */ > >- ret = efi_add_protocol(mem_handle, &efi_guid_device_path, > >- device_path); > >- if (ret != EFI_SUCCESS) > >- goto err_add_protocol; > >- } else { > >- assert(device_path && image_path); > >- } > >- > >- ret = efi_setup_loaded_image(device_path, image_path, &image_obj, > >- &loaded_image_info); > >- if (ret) > >- goto err_prepare; > >+ efi_status_t ret; > > > > /* Transfer environment variable as load options */ > >- set_load_options(loaded_image_info, "bootargs"); > > In set_load_options() an error could occur (out of memory). So I think > it should return a status. 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." > > 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)); > > > > /* > > >
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.akashi@linaro.org> > > >--- > > > cmd/bootefi.c | 197 +++++++++++++++++++--------------- > > > include/efi_loader.h | 5 +- > > > lib/efi_loader/efi_bootmgr.c | 43 ++++---- > > > lib/efi_loader/efi_boottime.c | 2 + > > > 4 files changed, 134 insertions(+), 113 deletions(-) > > > > > >diff --git a/cmd/bootefi.c b/cmd/bootefi.c > > >index f14966961b23..97d49a53a44b 100644 > > >--- a/cmd/bootefi.c > > >+++ b/cmd/bootefi.c > > >@@ -222,88 +222,39 @@ static efi_status_t efi_install_fdt(const char *fdt_opt) > > > /** > > > * do_bootefi_exec() - execute EFI binary > > > * > > >- * @efi: address of the binary > > >- * @device_path: path of the device from which the binary was loaded > > >- * @image_path: device path of the binary > > >+ * @handle: handle of loaded image > > > * Return: status code > > > * > > > * Load the EFI binary into a newly assigned memory unwinding the relocation > > > * information, install the loaded image protocol, and call the binary. > > > */ > > >-static efi_status_t do_bootefi_exec(void *efi, > > >- struct efi_device_path *device_path, > > >- struct efi_device_path *image_path) > > >+static efi_status_t do_bootefi_exec(efi_handle_t handle) > > > { > > >- efi_handle_t mem_handle = NULL; > > >- struct efi_device_path *memdp = NULL; > > >- efi_status_t ret; > > >- struct efi_loaded_image_obj *image_obj = NULL; > > > struct efi_loaded_image *loaded_image_info = NULL; > > >- > > >- /* > > >- * Special case for efi payload not loaded from disk, such as > > >- * 'bootefi hello' or for example payload loaded directly into > > >- * memory via JTAG, etc: > > >- */ > > >- if (!device_path && !image_path) { > > >- printf("WARNING: using memory device/image path, this may confuse some payloads!\n"); > > >- /* actual addresses filled in after efi_load_pe() */ > > >- memdp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, 0, 0); > > >- device_path = image_path = memdp; > > >- /* > > >- * Grub expects that the device path of the loaded image is > > >- * installed on a handle. > > >- */ > > >- ret = efi_create_handle(&mem_handle); > > >- if (ret != EFI_SUCCESS) > > >- return ret; /* TODO: leaks device_path */ > > >- ret = efi_add_protocol(mem_handle, &efi_guid_device_path, > > >- device_path); > > >- if (ret != EFI_SUCCESS) > > >- goto err_add_protocol; > > >- } else { > > >- assert(device_path && image_path); > > >- } > > >- > > >- ret = efi_setup_loaded_image(device_path, image_path, &image_obj, > > >- &loaded_image_info); > > >- if (ret) > > >- goto err_prepare; > > >+ efi_status_t ret; > > > > > > /* Transfer environment variable as load options */ > > >- set_load_options(loaded_image_info, "bootargs"); > > > > In set_load_options() an error could occur (out of memory). So I think > > it should return a status. > > 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)); > > > > > > /* > > > > >
On 16.04.19 18:20, Heinrich Schuchardt wrote: > On 4/16/19 12:56 PM, Heinrich Schuchardt wrote: >> On 4/16/19 6:24 AM, AKASHI Takahiro wrote: >>> In the current implementation, bootefi command and EFI boot manager >>> don't use load_image API, instead, use more primitive and internal >>> functions. This will introduce duplicated code and potentially >>> unknown bugs as well as inconsistent behaviours. >>> >>> With this patch, do_efibootmgr() and do_boot_efi() are completely >>> overhauled and re-implemented using load_image API. >>> >>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> >>> --- >>> cmd/bootefi.c | 197 >>> +++++++++++++++++++--------------- >>> include/efi_loader.h | 5 +- >>> lib/efi_loader/efi_bootmgr.c | 43 ++++---- >>> lib/efi_loader/efi_boottime.c | 2 + >>> 4 files changed, 134 insertions(+), 113 deletions(-) >>> >>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c >>> index f14966961b23..97d49a53a44b 100644 >>> --- a/cmd/bootefi.c >>> +++ b/cmd/bootefi.c >>> @@ -222,88 +222,39 @@ static efi_status_t efi_install_fdt(const char >>> *fdt_opt) >>> /** >>> * do_bootefi_exec() - execute EFI binary >>> * >>> - * @efi: address of the binary >>> - * @device_path: path of the device from which the binary was >>> loaded >>> - * @image_path: device path of the binary >>> + * @handle: handle of loaded image >>> * Return: status code >>> * >>> * Load the EFI binary into a newly assigned memory unwinding the >>> relocation >>> * information, install the loaded image protocol, and call the >>> binary. >>> */ >>> -static efi_status_t do_bootefi_exec(void *efi, >>> - struct efi_device_path *device_path, >>> - struct efi_device_path *image_path) >>> +static efi_status_t do_bootefi_exec(efi_handle_t handle) >>> { >>> - efi_handle_t mem_handle = NULL; >>> - struct efi_device_path *memdp = NULL; >>> - efi_status_t ret; >>> - struct efi_loaded_image_obj *image_obj = NULL; >>> struct efi_loaded_image *loaded_image_info = NULL; >>> - >>> - /* >>> - * Special case for efi payload not loaded from disk, such as >>> - * 'bootefi hello' or for example payload loaded directly into >>> - * memory via JTAG, etc: >>> - */ >>> - if (!device_path && !image_path) { >>> - printf("WARNING: using memory device/image path, this may >>> confuse some payloads!\n"); >>> - /* actual addresses filled in after efi_load_pe() */ >>> - memdp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, 0, 0); >>> - device_path = image_path = memdp; >>> - /* >>> - * Grub expects that the device path of the loaded image is >>> - * installed on a handle. >>> - */ >>> - ret = efi_create_handle(&mem_handle); >>> - if (ret != EFI_SUCCESS) >>> - return ret; /* TODO: leaks device_path */ >>> - ret = efi_add_protocol(mem_handle, &efi_guid_device_path, >>> - device_path); >>> - if (ret != EFI_SUCCESS) >>> - goto err_add_protocol; >>> - } else { >>> - assert(device_path && image_path); >>> - } >>> - >>> - ret = efi_setup_loaded_image(device_path, image_path, &image_obj, >>> - &loaded_image_info); >>> - if (ret) >>> - goto err_prepare; >>> + efi_status_t ret; >>> >>> /* Transfer environment variable as load options */ >>> - set_load_options(loaded_image_info, "bootargs"); >> >> In set_load_options() an error could occur (out of memory). So I think >> it should return a status. >> >>> - >>> - /* Load the EFI payload */ >>> - ret = efi_load_pe(image_obj, efi, loaded_image_info); >>> + ret = EFI_CALL(systab.boottime->open_protocol( >>> + handle, >>> + &efi_guid_loaded_image, >>> + (void **)&loaded_image_info, >>> + NULL, NULL, >>> + EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL)); >> >> Shouldn't we move this to set_load_options()? >> >>> if (ret != EFI_SUCCESS) >>> - goto err_prepare; >>> - >>> - if (memdp) { >>> - struct efi_device_path_memory *mdp = (void *)memdp; >>> - mdp->memory_type = loaded_image_info->image_code_type; >>> - mdp->start_address = (uintptr_t)loaded_image_info->image_base; >>> - mdp->end_address = mdp->start_address + >>> - loaded_image_info->image_size; >>> - } >>> + goto err; >>> + set_load_options(loaded_image_info, "bootargs"); >>> >>> /* we don't support much: */ >>> >>> env_set("efi_8be4df61-93ca-11d2-aa0d-00e098032b8c_OsIndicationsSupported", >>> >>> "{ro,boot}(blob)0000000000000000"); >> >> Shouldn't this move to efi_setup.c? Probably we should also set >> OsIndications. I would prefer using efi_set_variable(). I think moving >> this could be done in a preparatory patch. >> >>> >>> /* Call our payload! */ >>> - debug("%s: Jumping to 0x%p\n", __func__, image_obj->entry); >>> - ret = EFI_CALL(efi_start_image(&image_obj->header, NULL, NULL)); >>> + ret = EFI_CALL(efi_start_image(handle, NULL, NULL)); >>> >>> -err_prepare: >>> - /* image has returned, loaded-image obj goes *poof*: */ >>> efi_restore_gd(); >>> free(loaded_image_info->load_options); >>> - efi_delete_handle(&image_obj->header); >>> - >>> -err_add_protocol: >>> - if (mem_handle) >>> - efi_delete_handle(mem_handle); >>> >>> +err: >>> return ret; >>> } >>> >>> @@ -317,8 +268,7 @@ err_add_protocol: >>> */ >>> static int do_efibootmgr(const char *fdt_opt) >>> { >>> - struct efi_device_path *device_path, *file_path; >>> - void *addr; >>> + efi_handle_t handle; >>> efi_status_t ret; >>> >>> /* Allow unaligned memory access */ >>> @@ -340,19 +290,19 @@ static int do_efibootmgr(const char *fdt_opt) >>> else if (ret != EFI_SUCCESS) >>> return CMD_RET_FAILURE; >>> >>> - addr = efi_bootmgr_load(&device_path, &file_path); >>> - if (!addr) >>> - return 1; >>> + ret = efi_bootmgr_load(&handle); >>> + if (ret != EFI_SUCCESS) { >>> + printf("EFI boot manager: Cannot load any image\n"); >>> + return CMD_RET_FAILURE; >>> + } >>> >>> - printf("## Starting EFI application at %p ...\n", addr); >>> - ret = do_bootefi_exec(addr, device_path, file_path); >>> - printf("## Application terminated, r = %lu\n", >>> - ret & ~EFI_ERROR_MASK); >>> + ret = do_bootefi_exec(handle); >>> + printf("## Application terminated, r = %lu\n", ret & >>> ~EFI_ERROR_MASK); >>> >>> if (ret != EFI_SUCCESS) >>> - return 1; >>> + return CMD_RET_FAILURE; >>> >>> - return 0; >>> + return CMD_RET_SUCCESS; >>> } >>> >>> /* >>> @@ -367,10 +317,14 @@ static int do_efibootmgr(const char *fdt_opt) >>> */ >>> static int do_boot_efi(const char *image_opt, const char *fdt_opt) >>> { >>> - unsigned long addr; >>> - char *saddr; >>> + void *image_buf; >>> + struct efi_device_path *device_path, *image_path; >>> + struct efi_device_path *memdp = NULL, *file_path = NULL; >>> + const char *saddr; >>> + unsigned long addr, size; >>> + const char *size_str; >>> + efi_handle_t mem_handle = NULL, handle; >>> efi_status_t ret; >>> - unsigned long fdt_addr; >>> >>> /* Allow unaligned memory access */ >>> allow_unaligned(); >>> @@ -392,36 +346,94 @@ static int do_boot_efi(const char *image_opt, >>> const char *fdt_opt) >>> return CMD_RET_FAILURE; >>> >>> #ifdef CONFIG_CMD_BOOTEFI_HELLO >>> - if (!strcmp(argv[1], "hello")) { >>> - ulong size = __efi_helloworld_end - __efi_helloworld_begin; >>> - >>> + if (!strcmp(image_opt, "hello")) { >>> saddr = env_get("loadaddr"); >>> + size = __efi_helloworld_end - __efi_helloworld_begin; >>> + >>> if (saddr) >>> addr = simple_strtoul(saddr, NULL, 16); >>> else >>> addr = CONFIG_SYS_LOAD_ADDR; >>> - memcpy(map_sysmem(addr, size), __efi_helloworld_begin, size); >>> + >>> + image_buf = map_sysmem(addr, size); >>> + memcpy(image_buf, __efi_helloworld_begin, size); >>> + >>> + device_path = NULL; >>> + image_path = NULL; >>> } else >>> #endif >>> { >>> - saddr = argv[1]; >>> + saddr = image_opt; >>> + size_str = env_get("filesize"); >>> + if (size_str) >>> + size = simple_strtoul(size_str, NULL, 16); >>> + else >>> + size = 0; >>> >>> addr = simple_strtoul(saddr, NULL, 16); >>> /* Check that a numeric value was passed */ >>> if (!addr && *saddr != '0') >>> return CMD_RET_USAGE; >>> + >>> + image_buf = map_sysmem(addr, size); >>> + >>> + device_path = bootefi_device_path; >>> + image_path = bootefi_image_path; >>> } >>> >>> - printf("## Starting EFI application at %08lx ...\n", addr); >>> - ret = do_bootefi_exec(map_sysmem(addr, 0), bootefi_device_path, >>> - bootefi_image_path); >>> - printf("## Application terminated, r = %lu\n", >>> - ret & ~EFI_ERROR_MASK); >>> + if (!device_path && !image_path) { >>> + /* >>> + * Special case for efi payload not loaded from disk, >>> + * such as 'bootefi hello' or for example payload >>> + * loaded directly into memory via JTAG, etc: >>> + */ >>> + printf("WARNING: using memory device/image path, this may >>> confuse some payloads!\n"); >> >> The EFI spec says that either of SourceBuffer or DevicePath may be NULL >> when calling LoadImage(). >> >>> + >>> + /* actual addresses filled in after efi_load_image() */ >>> + memdp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, >>> + (uint64_t)image_buf, size); >>> + file_path = memdp; /* append(memdp, NULL) */ >>> + >>> + /* >>> + * Make sure that device for device_path exist >>> + * in load_image(). Otherwise, shell and grub will fail. >> >> GRUB will fail anyway because it cannot determine the disk from which it >> was loaded. So why are we doing this? > > In Rob's original commit bf19273e81eb ("efi_loader: Add mem-mapped > for fallback") the comment was: > "This fixes 'bootefi hello' after devicepath refactoring." > > EFI Shell.efi starts fine even when we set device_path and image_path > to NULL. > > When loading GRUB from disk or via tftp the device path and the image > path are set, e.g. for tftp: > > => dhcp grubarm.efi > => bootefi 0x40200000 $fdtcontroladdr > Found 0 disks > ## Starting EFI application at 40200000 ... > device_path: > /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/MAC(525400123456,0x1) > image_path: /grubarm.efi > error: disk `,msdos2' not found. > Entering rescue mode... > grub rescue> > > Maybe GRUB would run if all necessary modules were compiled in to do a > network boot. But how would it find grub.conf? > > So my feeling is that we are creating the dummy memory device path > because: > > - Once `bootefi hello` which is our own test program had a problem. > - GRUB has a bug: it hangs or crashes if the file device path is not > set in LoadImage(). > > I think the problem should not be fixed in U-Boot but in GRUB. > > I am CC-ing Leif due to all the attention he has paid both to U-Boot > and to GRUB. Please also keep in mind that you can not fix already released versions of GRUB. Alex
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"); - - /* 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! */ - 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"); + + /* 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, 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); ret = EFI_CALL(image_obj->entry(image_handle, &systab)); /*
In the current implementation, bootefi command and EFI boot manager don't use load_image API, instead, use more primitive and internal functions. This will introduce duplicated code and potentially unknown bugs as well as inconsistent behaviours. With this patch, do_efibootmgr() and do_boot_efi() are completely overhauled and re-implemented using load_image API. Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> --- cmd/bootefi.c | 197 +++++++++++++++++++--------------- include/efi_loader.h | 5 +- lib/efi_loader/efi_bootmgr.c | 43 ++++---- lib/efi_loader/efi_boottime.c | 2 + 4 files changed, 134 insertions(+), 113 deletions(-)