On 4/20/19 10:37 AM, Heinrich Schuchardt wrote: > On 4/19/19 5:22 AM, AKASHI Takahiro wrote: >> In the current implementation, bootefi command and EFI boot manager >> don't use load_image API, instead, use more primitive and internal >> functions. This will introduce duplicated code and potentially >> unknown bugs as well as inconsistent behaviours. >> >> With this patch, do_efibootmgr() and do_boot_efi() are completely >> overhauled and re-implemented using load_image API.
Hello Takahiro, with this patch applied iPXE cannot find a chained file on the EFI partition. Did you test booting GRUB with this patch? Best regards Heinrich EFI: Entry efi_load_image(0, 00000000b9f92020, /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(1,MBR,0x800512bd,0x800,0x200000)/dtb, 0000000040080000, 21738, 00000000b9f33fc8) EFI: Exit: efi_load_image: 0 EFI: Entry efi_start_image(00000000b9f95360, 0000000000000000, 0000000000000000) EFI: Call: efi_open_protocol(image_handle, &efi_guid_loaded_image, &info, NULL, NULL, EFI_OPEN_PROTOCOL_GET_PROTOCOL) EFI: 0 returned by efi_open_protocol(image_handle, &efi_guid_loaded_image, &info, NULL, NULL, EFI_OPEN_PROTOCOL_GET_PROTOCOL) EFI: Jumping into 0x00000000b8e7861c EFI: Call: image_obj->entry(image_handle, &systab) EFI: Entry efi_load_image(0, 00000000b9f95360, /, 00000000b8e9fbc0, 177, 00000000b9f33e38) efi_load_pe: Invalid DOS Signature EFI: Exit: efi_load_image: 1 iPXE initialising devices...ok iPXE 1.0.0+ (ac4fb) -- Open Source Network Boot Firmware -- http://ipxe.org Features: DNS FTP HTTP HTTPS iSCSI NFS TFTP VLAN AoE EFI Menu iPXE script started Trying to chain file:/boot.ipxe Could not start download: No such file or directory (http://ipxe.org/2d4de08e) Could not find file:/boot.ipxe Opening shell iPXE> >> >> Signed-off-by: AKASHI Takahiro <takahiro.aka...@linaro.org> >> --- >> cmd/bootefi.c | 187 ++++++++++++++++++---------------- >> include/efi_loader.h | 5 +- >> lib/efi_loader/efi_bootmgr.c | 43 ++++---- >> lib/efi_loader/efi_boottime.c | 2 + >> 4 files changed, 127 insertions(+), 110 deletions(-) >> >> diff --git a/cmd/bootefi.c b/cmd/bootefi.c >> index e2ca68c4dc12..8c84fff590a7 100644 >> --- a/cmd/bootefi.c >> +++ b/cmd/bootefi.c >> @@ -242,89 +242,36 @@ static efi_status_t efi_install_fdt(const char >> *fdt_opt) >> /** >> * do_bootefi_exec() - execute EFI binary >> * >> - * @efi: address of the binary >> - * @device_path: path of the device from which the binary was loaded >> - * @image_path: device path of the binary >> + * @handle: handle of loaded image >> * Return: status code >> * >> * Load the EFI binary into a newly assigned memory unwinding the >> relocation >> * information, install the loaded image protocol, and call the binary. >> */ >> -static efi_status_t do_bootefi_exec(void *efi, >> - struct efi_device_path *device_path, >> - struct efi_device_path *image_path) >> +static efi_status_t do_bootefi_exec(efi_handle_t handle) >> { >> - efi_handle_t mem_handle = NULL; >> - struct efi_device_path *memdp = NULL; >> efi_status_t ret; >> - struct efi_loaded_image_obj *image_obj = NULL; >> - struct efi_loaded_image *loaded_image_info = NULL; >> - >> - /* >> - * Special case for efi payload not loaded from disk, such as >> - * 'bootefi hello' or for example payload loaded directly into >> - * memory via JTAG, etc: >> - */ >> - if (!device_path && !image_path) { >> - printf("WARNING: using memory device/image path, this may >> confuse some payloads!\n"); >> - /* actual addresses filled in after efi_load_pe() */ >> - memdp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, 0, 0); >> - device_path = image_path = memdp; >> - /* >> - * Grub expects that the device path of the loaded image is >> - * installed on a handle. >> - */ >> - ret = efi_create_handle(&mem_handle); >> - if (ret != EFI_SUCCESS) >> - return ret; /* TODO: leaks device_path */ >> - ret = efi_add_protocol(mem_handle, &efi_guid_device_path, >> - device_path); >> - if (ret != EFI_SUCCESS) >> - goto err_add_protocol; >> - } else { >> - assert(device_path && image_path); >> - } >> - >> - ret = efi_setup_loaded_image(device_path, image_path, &image_obj, >> - &loaded_image_info); >> - if (ret) >> - goto err_prepare; >> >> /* Transfer environment variable as load options */ >> - ret = set_load_options((efi_handle_t)image_obj, "bootargs"); >> - if (ret != EFI_SUCCESS) >> - goto err_prepare; >> - >> - /* Load the EFI payload */ >> - ret = efi_load_pe(image_obj, efi, loaded_image_info); >> + ret = set_load_options(handle, "bootargs"); >> if (ret != EFI_SUCCESS) >> - goto err_prepare; >> - >> - if (memdp) { >> - struct efi_device_path_memory *mdp = (void *)memdp; >> - mdp->memory_type = loaded_image_info->image_code_type; >> - mdp->start_address = (uintptr_t)loaded_image_info->image_base; >> - mdp->end_address = mdp->start_address + >> - loaded_image_info->image_size; >> - } >> + return ret; >> >> /* we don't support much: */ >> >> env_set("efi_8be4df61-93ca-11d2-aa0d-00e098032b8c_OsIndicationsSupported", >> >> "{ro,boot}(blob)0000000000000000"); >> >> /* Call our payload! */ >> - debug("%s: Jumping to 0x%p\n", __func__, image_obj->entry); >> - ret = EFI_CALL(efi_start_image(&image_obj->header, NULL, NULL)); >> + ret = EFI_CALL(efi_start_image(handle, NULL, NULL)); >> >> -err_prepare: >> - /* image has returned, loaded-image obj goes *poof*: */ >> efi_restore_gd(); >> - free(loaded_image_info->load_options); >> - efi_delete_handle(&image_obj->header); >> >> -err_add_protocol: >> - if (mem_handle) >> - efi_delete_handle(mem_handle); >> + /* >> + * FIXME: Who is responsible for >> + * free(loaded_image_info->load_options); >> + * Once efi_exit() is implemented correctly, >> + * handle itself doesn't exist here. >> + */ >> >> return ret; >> } >> @@ -339,8 +286,7 @@ err_add_protocol: >> */ >> static int do_efibootmgr(const char *fdt_opt) >> { >> - struct efi_device_path *device_path, *file_path; >> - void *addr; >> + efi_handle_t handle; >> efi_status_t ret; >> >> /* Allow unaligned memory access */ >> @@ -362,19 +308,19 @@ static int do_efibootmgr(const char *fdt_opt) >> else if (ret != EFI_SUCCESS) >> return CMD_RET_FAILURE; >> >> - addr = efi_bootmgr_load(&device_path, &file_path); >> - if (!addr) >> - return 1; >> + ret = efi_bootmgr_load(&handle); >> + if (ret != EFI_SUCCESS) { >> + printf("EFI boot manager: Cannot load any image\n"); >> + return CMD_RET_FAILURE; >> + } >> >> - printf("## Starting EFI application at %p ...\n", addr); >> - ret = do_bootefi_exec(addr, device_path, file_path); >> - printf("## Application terminated, r = %lu\n", >> - ret & ~EFI_ERROR_MASK); >> + ret = do_bootefi_exec(handle); >> + printf("## Application terminated, r = %lu\n", ret & >> ~EFI_ERROR_MASK); >> >> if (ret != EFI_SUCCESS) >> - return 1; >> + return CMD_RET_FAILURE; >> >> - return 0; >> + return CMD_RET_SUCCESS; >> } >> >> /* >> @@ -389,7 +335,12 @@ static int do_efibootmgr(const char *fdt_opt) >> */ >> static int do_bootefi_image(const char *image_opt, const char *fdt_opt) >> { >> - unsigned long addr; >> + void *image_buf; >> + struct efi_device_path *device_path, *image_path; >> + struct efi_device_path *memdp = NULL, *file_path = NULL; >> + unsigned long addr, size; >> + const char *size_str; >> + efi_handle_t mem_handle = NULL, handle; >> efi_status_t ret; >> >> /* Allow unaligned memory access */ >> @@ -414,33 +365,90 @@ static int do_bootefi_image(const char >> *image_opt, const char *fdt_opt) >> #ifdef CONFIG_CMD_BOOTEFI_HELLO >> if (!strcmp(image_opt, "hello")) { >> char *saddr; >> - ulong size = __efi_helloworld_end - __efi_helloworld_begin; >> >> saddr = env_get("loadaddr"); >> + size = __efi_helloworld_end - __efi_helloworld_begin; >> + >> if (saddr) >> addr = simple_strtoul(saddr, NULL, 16); >> else >> addr = CONFIG_SYS_LOAD_ADDR; >> - memcpy(map_sysmem(addr, size), __efi_helloworld_begin, size); >> + >> + image_buf = map_sysmem(addr, size); >> + memcpy(image_buf, __efi_helloworld_begin, size); >> + >> + device_path = NULL; >> + image_path = NULL; >> } else >> #endif >> { >> + size_str = env_get("filesize"); >> + if (size_str) >> + size = simple_strtoul(size_str, NULL, 16); >> + else >> + size = 0; >> + >> addr = simple_strtoul(image_opt, NULL, 16); >> /* Check that a numeric value was passed */ >> if (!addr && *image_opt != '0') >> return CMD_RET_USAGE; >> + >> + image_buf = map_sysmem(addr, size); >> + >> + device_path = bootefi_device_path; >> + image_path = bootefi_image_path; >> } >> >> - printf("## Starting EFI application at %08lx ...\n", addr); >> - ret = do_bootefi_exec(map_sysmem(addr, 0), bootefi_device_path, >> - bootefi_image_path); >> - printf("## Application terminated, r = %lu\n", >> - ret & ~EFI_ERROR_MASK); >> + if (!device_path && !image_path) { >> + /* >> + * Special case for efi payload not loaded from disk, >> + * such as 'bootefi hello' or for example payload >> + * loaded directly into memory via JTAG, etc: >> + */ >> + memdp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, >> + (uintptr_t)image_buf, size); >> + file_path = memdp; /* append(memdp, NULL) */ >> + >> + /* >> + * Make sure that device for device_path exist >> + * in load_image(). Otherwise, shell and grub will fail. >> + */ >> + ret = efi_create_handle(&mem_handle); >> + if (ret != EFI_SUCCESS) >> + /* TODO: leaks device_path */ >> + goto err_add_protocol; >> + >> + ret = efi_add_protocol(mem_handle, &efi_guid_device_path, >> + memdp); >> + if (ret != EFI_SUCCESS) >> + goto err_add_protocol; >> + } else { >> + assert(device_path && image_path); >> + file_path = efi_dp_append(device_path, image_path); >> + } >> + >> + ret = EFI_CALL(efi_load_image(false, efi_root, >> + file_path, image_buf, size, &handle)); >> + if (ret != EFI_SUCCESS) >> + goto err_prepare; >> + >> + ret = do_bootefi_exec(handle); >> + printf("## Application terminated, r = %lu\n", ret & >> ~EFI_ERROR_MASK); >> + >> +err_prepare: >> + if (device_path) >> + efi_free_pool(file_path); >> + >> +err_add_protocol: >> + if (mem_handle) >> + efi_delete_handle(mem_handle); >> + if (file_path) /* hence memdp */ >> + efi_free_pool(file_path); >> >> if (ret != EFI_SUCCESS) >> return CMD_RET_FAILURE; >> - else >> - return CMD_RET_SUCCESS; >> + >> + return CMD_RET_SUCCESS; >> } >> >> #ifdef CONFIG_CMD_BOOTEFI_SELFTEST >> @@ -598,7 +606,7 @@ static char bootefi_help_text[] = >> " Use environment variable efi_selftest to select a single >> test.\n" >> " Use 'setenv efi_selftest list' to enumerate all tests.\n" >> #endif >> - "bootefi bootmgr [fdt addr]\n" >> + "bootefi bootmgr [fdt address]\n" >> " - load and boot EFI payload based on BootOrder/BootXXXX >> variables.\n" >> "\n" >> " If specified, the device tree located at <fdt address> gets\n" >> @@ -623,6 +631,13 @@ void efi_set_bootdev(const char *dev, const char >> *devnr, const char *path) >> ret = efi_dp_from_name(dev, devnr, path, &device, &image); >> if (ret == EFI_SUCCESS) { >> bootefi_device_path = device; >> + if (image) { >> + /* FIXME: image should not contain device */ >> + struct efi_device_path *image_tmp = image; >> + >> + efi_dp_split_file_path(image, &device, &image); >> + efi_free_pool(image_tmp); >> + } >> bootefi_image_path = image; >> } else { >> bootefi_device_path = NULL; >> diff --git a/include/efi_loader.h b/include/efi_loader.h >> index 93f7672aecbc..39ed8a6fa592 100644 >> --- a/include/efi_loader.h >> +++ b/include/efi_loader.h >> @@ -412,8 +412,6 @@ efi_status_t efi_setup_loaded_image(struct >> efi_device_path *device_path, >> struct efi_device_path *file_path, >> struct efi_loaded_image_obj **handle_ptr, >> struct efi_loaded_image **info_ptr); >> -efi_status_t efi_load_image_from_path(struct efi_device_path *file_path, >> - void **buffer, efi_uintn_t *size); >> /* Print information about all loaded images */ >> void efi_print_image_infos(void *pc); >> >> @@ -567,8 +565,7 @@ struct efi_load_option { >> >> void efi_deserialize_load_option(struct efi_load_option *lo, u8 *data); >> unsigned long efi_serialize_load_option(struct efi_load_option *lo, >> u8 **data); >> -void *efi_bootmgr_load(struct efi_device_path **device_path, >> - struct efi_device_path **file_path); >> +efi_status_t efi_bootmgr_load(efi_handle_t *handle); >> >> #else /* CONFIG_IS_ENABLED(EFI_LOADER) */ >> >> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c >> index 4fccadc5483d..13ec79b2098b 100644 >> --- a/lib/efi_loader/efi_bootmgr.c >> +++ b/lib/efi_loader/efi_bootmgr.c >> @@ -120,14 +120,14 @@ static void *get_var(u16 *name, const efi_guid_t >> *vendor, >> * if successful. This checks that the EFI_LOAD_OPTION is active >> (enabled) >> * and that the specified file to boot exists. >> */ >> -static void *try_load_entry(uint16_t n, struct efi_device_path >> **device_path, >> - struct efi_device_path **file_path) >> +static efi_status_t try_load_entry(u16 n, efi_handle_t *handle) >> { >> struct efi_load_option lo; >> u16 varname[] = L"Boot0000"; >> u16 hexmap[] = L"0123456789ABCDEF"; >> - void *load_option, *image = NULL; >> + void *load_option; >> efi_uintn_t size; >> + efi_status_t ret; >> >> varname[4] = hexmap[(n & 0xf000) >> 12]; >> varname[5] = hexmap[(n & 0x0f00) >> 8]; >> @@ -136,19 +136,19 @@ static void *try_load_entry(uint16_t n, struct >> efi_device_path **device_path, >> >> load_option = get_var(varname, &efi_global_variable_guid, &size); >> if (!load_option) >> - return NULL; >> + return EFI_LOAD_ERROR; >> >> efi_deserialize_load_option(&lo, load_option); >> >> if (lo.attributes & LOAD_OPTION_ACTIVE) { >> u32 attributes; >> - efi_status_t ret; >> >> debug("%s: trying to load \"%ls\" from %pD\n", >> __func__, lo.label, lo.file_path); >> >> - ret = efi_load_image_from_path(lo.file_path, &image, &size); >> - >> + ret = EFI_CALL(efi_load_image(false, (void *)0x1 /* FIXME */, > > In cmd/booefi.c you use efi_root. So we shall do the same here. > > I will fix that. No need to resend the patch. > > >> + lo.file_path, NULL, 0, >> + handle)); >> if (ret != EFI_SUCCESS) >> goto error; > TODO: > Now that you have the loaded image protocol you should update the load > options with the value in load_option. I guess you do not want to > overwrite it with the content of bootargs. > > A the problem was not introduced with this patch series: > > Reviewed-by: Heinrich Schuchardt <xypron.g...@gmx.de> > >> >> @@ -159,17 +159,22 @@ static void *try_load_entry(uint16_t n, struct >> efi_device_path **device_path, >> L"BootCurrent", >> (efi_guid_t *)&efi_global_variable_guid, >> attributes, size, &n)); >> - if (ret != EFI_SUCCESS) >> + if (ret != EFI_SUCCESS) { >> + if (EFI_CALL(efi_unload_image(*handle)) >> + != EFI_SUCCESS) >> + printf("Unloading image failed\n"); >> goto error; >> + } >> >> printf("Booting: %ls\n", lo.label); >> - efi_dp_split_file_path(lo.file_path, device_path, file_path); >> + } else { >> + ret = EFI_LOAD_ERROR; >> } >> >> error: >> free(load_option); >> >> - return image; >> + return ret; >> } >> >> /* >> @@ -177,12 +182,10 @@ error: >> * EFI variable, the available load-options, finding and returning >> * the first one that can be loaded successfully. >> */ >> -void *efi_bootmgr_load(struct efi_device_path **device_path, >> - struct efi_device_path **file_path) >> +efi_status_t efi_bootmgr_load(efi_handle_t *handle) >> { >> u16 bootnext, *bootorder; >> efi_uintn_t size; >> - void *image = NULL; >> int i, num; >> efi_status_t ret; >> >> @@ -209,10 +212,9 @@ void *efi_bootmgr_load(struct efi_device_path >> **device_path, >> /* load BootNext */ >> if (ret == EFI_SUCCESS) { >> if (size == sizeof(u16)) { >> - image = try_load_entry(bootnext, device_path, >> - file_path); >> - if (image) >> - return image; >> + ret = try_load_entry(bootnext, handle); >> + if (ret == EFI_SUCCESS) >> + return ret; >> } >> } else { >> printf("Deleting BootNext failed\n"); >> @@ -223,19 +225,20 @@ void *efi_bootmgr_load(struct efi_device_path >> **device_path, >> bootorder = get_var(L"BootOrder", &efi_global_variable_guid, >> &size); >> if (!bootorder) { >> printf("BootOrder not defined\n"); >> + ret = EFI_NOT_FOUND; >> goto error; >> } >> >> num = size / sizeof(uint16_t); >> for (i = 0; i < num; i++) { >> debug("%s: trying to load Boot%04X\n", __func__, bootorder[i]); >> - image = try_load_entry(bootorder[i], device_path, file_path); >> - if (image) >> + ret = try_load_entry(bootorder[i], handle); >> + if (ret == EFI_SUCCESS) >> break; >> } >> >> free(bootorder); >> >> error: >> - return image; >> + return ret; >> } >> diff --git a/lib/efi_loader/efi_boottime.c >> b/lib/efi_loader/efi_boottime.c >> index abc295e392e9..19a58144cf4b 100644 >> --- a/lib/efi_loader/efi_boottime.c >> +++ b/lib/efi_loader/efi_boottime.c >> @@ -1591,6 +1591,7 @@ failure: >> * @size: size of the loaded image >> * Return: status code >> */ >> +static >> efi_status_t efi_load_image_from_path(struct efi_device_path >> *file_path, >> void **buffer, efi_uintn_t *size) >> { >> @@ -2664,6 +2665,7 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t >> image_handle, >> } >> >> current_image = image_handle; >> + EFI_PRINT("Jumping into 0x%p\n", image_obj->entry); >> ret = EFI_CALL(image_obj->entry(image_handle, &systab)); >> >> /* >> > > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot