[U-Boot] [PATCH 0/9] efi_loader: rework bootefi/bootmgr
There are several reasons that I want to rework/refactor bootefi command as well as bootmgr: * Some previous commits on bootefi.c have made the code complicated and a bit hard to understand.
* do_bootefi_exec() would better be implemented using load_image() along with start_image() to be aligned with UEFI interfaces.
* Contrary to the other part, efi_selftest part of the code is unusual in terms of loading/execution path in do_bootefi().
* When we will support "secure boot" in the future, EFI Boot Manager is expected to be invoked as a standalone command without any arguments to mitigate security surfaces.
In this patch set, Patch#1 to #7 are preparatory patches for patch#8. Patch#8 is a core part of reworking. Patch#9 is for standalone boot manager.
# Please note that some patches, say patch#2 and #3, can be combined into one # but I intentionally keep them separated to clarify my intentions of changes.
Issues: * It would be better off to change the semantics of efi_dp_from_name(). no chance to free loaded_image_info->load_options. (see patch #8)
-Takahiro Akashi
Changes in v1 (Apr 19, 2019) * Travis CI: https://travis-ci.org/t-akashi/u-boot/builds/521984187 * rebased on efi-2019-07 * delete already-merged patches * rework set_load_options() (patch#1 and #8) * fix compile/Travis CI errors (patch#2, #7 and #8) * rename do_boot_efi() to do_bootefi_image() (patch#7) * free file_path in do_bootefi_image() (patch#8) * use EFI_PRINT instead of debug() (patch#8) * remove improper warning messages (patch#8)
Changes in RFC v3 (Apr 16, 2019) * rebased on v2019.04 * delete already-merged patches * add patch#2 for exporting root node * use correct/more appropriate return code (CMD_RET_xxx) (patch#3,5,7) * remove a message at starting an image in bootefi command, instead adding a debug message in efi_start_image() * use root node as a dummy parent handle when calling efi_start_image() * remove efi_unload_image() in bootefi command
Changes in RFC v2 (Mar 27, 2019) * rebased on v2019.04-rc4 * use load_image API in do_bootmgr_load() * merge efi_install_fdt() and efi_process_fdt() * add EFI_LOADED_IMAGE_DEVICE_PATH_PROTOCOL to image (patch#1) * lots of minor changes
AKASHI Takahiro (9): cmd: bootefi: rework set_load_options() cmd: bootefi: carve out fdt handling from do_bootefi() cmd: bootefi: merge efi_install_fdt() and efi_process_fdt() cmd: bootefi: carve out efi_selftest code from do_bootefi() cmd: bootefi: move do_bootefi_bootmgr_exec() forward cmd: bootefi: carve out bootmgr code from do_bootefi() cmd: bootefi: carve out do_bootefi_image() from do_bootefi() efi_loader: rework bootmgr/bootefi using load_image API cmd: add efibootmgr command
cmd/Kconfig | 8 + cmd/bootefi.c | 540 ++++++++++++++++++++++------------ include/efi_loader.h | 5 +- lib/efi_loader/efi_bootmgr.c | 43 +-- lib/efi_loader/efi_boottime.c | 2 + 5 files changed, 383 insertions(+), 215 deletions(-)
set_load_options() can fail, so it should return error code to stop invoking an image. In addition, set_load_options() now takes a handle, instead of loaded_image_info, to utilize efi_load_image() in a later patch.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org --- cmd/bootefi.c | 36 +++++++++++++++++++++++++++--------- 1 file changed, 27 insertions(+), 9 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 15ee4af45667..d8ca4ed703ef 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -39,29 +39,49 @@ void __weak allow_unaligned(void) /* * Set the load options of an image from an environment variable. * - * @loaded_image_info: the image - * @env_var: name of the environment variable + * @handle: the image handle + * @env_var: name of the environment variable + * Return: status code */ -static void set_load_options(struct efi_loaded_image *loaded_image_info, - const char *env_var) +static efi_status_t set_load_options(efi_handle_t handle, const char *env_var) { + struct efi_loaded_image *loaded_image_info; size_t size; const char *env = env_get(env_var); u16 *pos; + efi_status_t ret; + + ret = EFI_CALL(systab.boottime->open_protocol( + handle, + &efi_guid_loaded_image, + (void **)&loaded_image_info, + efi_root, NULL, + EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL)); + if (ret != EFI_SUCCESS) + return EFI_INVALID_PARAMETER;
loaded_image_info->load_options = NULL; loaded_image_info->load_options_size = 0; if (!env) - return; + goto out; + size = utf8_utf16_strlen(env) + 1; loaded_image_info->load_options = calloc(size, sizeof(u16)); if (!loaded_image_info->load_options) { printf("ERROR: Out of memory\n"); - return; + EFI_CALL(systab.boottime->close_protocol(handle, + &efi_guid_loaded_image, + efi_root, NULL)); + return EFI_OUT_OF_RESOURCES; } pos = loaded_image_info->load_options; utf8_utf16_strcpy(&pos, env); loaded_image_info->load_options_size = size * 2; + +out: + return EFI_CALL(systab.boottime->close_protocol(handle, + &efi_guid_loaded_image, + efi_root, NULL)); }
/** @@ -212,9 +232,7 @@ static efi_status_t bootefi_run_prepare(const char *load_options_path, return ret;
/* Transfer environment variable as load options */ - set_load_options(*loaded_image_infop, load_options_path); - - return 0; + return set_load_options((efi_handle_t)*image_objp, load_options_path); }
/**
On 4/19/19 5:22 AM, AKASHI Takahiro wrote:
set_load_options() can fail, so it should return error code to stop invoking an image. In addition, set_load_options() now takes a handle, instead of loaded_image_info, to utilize efi_load_image() in a later patch.
Signed-off-by: AKASHI Takahirotakahiro.akashi@linaro.org
Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de
This is a preparatory patch for reworking do_bootefi() in later patch.
Carve out a function to handle the installation of the device tree as a configuration table in system table.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org --- cmd/bootefi.c | 54 ++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 38 insertions(+), 16 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index d8ca4ed703ef..fb6703ce84f3 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -218,6 +218,38 @@ static efi_status_t efi_install_fdt(ulong fdt_addr) return ret; }
+/** + * efi_process_fdt() - process fdt passed by a command argument + * @fdt_opt: pointer to argument + * Return: status code + * + * If specified, fdt will be installed as configuration table, + * otherwise no fdt will be passed. + */ +static efi_status_t efi_process_fdt(const char *fdt_opt) +{ + unsigned long fdt_addr; + efi_status_t ret; + + if (fdt_opt) { + fdt_addr = simple_strtoul(fdt_opt, NULL, 16); + if (!fdt_addr && *fdt_opt != '0') + return EFI_INVALID_PARAMETER; + + /* Install device tree */ + ret = efi_install_fdt(fdt_addr); + if (ret != EFI_SUCCESS) { + printf("ERROR: failed to install device tree\n"); + return ret; + } + } else { + /* Remove device tree. EFI_NOT_FOUND can be ignored here */ + efi_install_configuration_table(&efi_guid_fdt, NULL); + } + + return EFI_SUCCESS; +} + static efi_status_t bootefi_run_prepare(const char *load_options_path, struct efi_device_path *device_path, struct efi_device_path *image_path, @@ -407,7 +439,6 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) unsigned long addr; char *saddr; efi_status_t r; - unsigned long fdt_addr;
/* Allow unaligned memory access */ allow_unaligned(); @@ -425,21 +456,12 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) if (argc < 2) return CMD_RET_USAGE;
- if (argc > 2) { - fdt_addr = simple_strtoul(argv[2], NULL, 16); - if (!fdt_addr && *argv[2] != '0') - return CMD_RET_USAGE; - /* Install device tree */ - r = efi_install_fdt(fdt_addr); - if (r != EFI_SUCCESS) { - printf("ERROR: failed to install device tree\n"); - return CMD_RET_FAILURE; - } - } else { - /* Remove device tree. EFI_NOT_FOUND can be ignored here */ - efi_install_configuration_table(&efi_guid_fdt, NULL); - printf("WARNING: booting without device tree\n"); - } + r = efi_process_fdt(argc > 2 ? argv[2] : NULL); + if (r == EFI_INVALID_PARAMETER) + return CMD_RET_USAGE; + else if (r != EFI_SUCCESS) + return CMD_RET_FAILURE; + #ifdef CONFIG_CMD_BOOTEFI_HELLO if (!strcmp(argv[1], "hello")) { ulong size = __efi_helloworld_end - __efi_helloworld_begin;
On 4/19/19 5:22 AM, AKASHI Takahiro wrote:
This is a preparatory patch for reworking do_bootefi() in later patch.
Carve out a function to handle the installation of the device tree as a configuration table in system table.
Signed-off-by: AKASHI Takahirotakahiro.akashi@linaro.org
Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de
This is a preparatory patch for reworking do_bootefi() in later patch. For simplicity, merge two functions.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org --- cmd/bootefi.c | 67 +++++++++++++++++++++------------------------------ 1 file changed, 28 insertions(+), 39 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index fb6703ce84f3..c6d6eb7312e8 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -185,62 +185,51 @@ static void efi_carve_out_dt_rsv(void *fdt) } }
-static efi_status_t efi_install_fdt(ulong fdt_addr) -{ - bootm_headers_t img = { 0 }; - efi_status_t ret; - void *fdt; - - fdt = map_sysmem(fdt_addr, 0); - if (fdt_check_header(fdt)) { - printf("ERROR: invalid device tree\n"); - return EFI_INVALID_PARAMETER; - } - - /* Create memory reservation as indicated by the device tree */ - efi_carve_out_dt_rsv(fdt); - - /* Prepare fdt for payload */ - ret = copy_fdt(&fdt); - if (ret) - return ret; - - if (image_setup_libfdt(&img, fdt, 0, NULL)) { - printf("ERROR: failed to process device tree\n"); - return EFI_LOAD_ERROR; - } - - /* Link to it in the efi tables */ - ret = efi_install_configuration_table(&efi_guid_fdt, fdt); - if (ret != EFI_SUCCESS) - return EFI_OUT_OF_RESOURCES; - - return ret; -} - /** - * efi_process_fdt() - process fdt passed by a command argument + * efi_install_fdt() - install fdt passed by a command argument * @fdt_opt: pointer to argument * Return: status code * * If specified, fdt will be installed as configuration table, * otherwise no fdt will be passed. */ -static efi_status_t efi_process_fdt(const char *fdt_opt) +static efi_status_t efi_install_fdt(const char *fdt_opt) { unsigned long fdt_addr; + void *fdt; + bootm_headers_t img = { 0 }; efi_status_t ret;
if (fdt_opt) { + /* Install device tree */ fdt_addr = simple_strtoul(fdt_opt, NULL, 16); if (!fdt_addr && *fdt_opt != '0') return EFI_INVALID_PARAMETER;
- /* Install device tree */ - ret = efi_install_fdt(fdt_addr); + fdt = map_sysmem(fdt_addr, 0); + if (fdt_check_header(fdt)) { + printf("ERROR: invalid device tree\n"); + return EFI_INVALID_PARAMETER; + } + + /* Create memory reservation as indicated by the device tree */ + efi_carve_out_dt_rsv(fdt); + + /* Prepare fdt for payload */ + ret = copy_fdt(&fdt); + if (ret) + return ret; + + if (image_setup_libfdt(&img, fdt, 0, NULL)) { + printf("ERROR: failed to process device tree\n"); + return EFI_LOAD_ERROR; + } + + /* Link to it in the efi tables */ + ret = efi_install_configuration_table(&efi_guid_fdt, fdt); if (ret != EFI_SUCCESS) { printf("ERROR: failed to install device tree\n"); - return ret; + return EFI_OUT_OF_RESOURCES; } } else { /* Remove device tree. EFI_NOT_FOUND can be ignored here */ @@ -456,7 +445,7 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) if (argc < 2) return CMD_RET_USAGE;
- r = efi_process_fdt(argc > 2 ? argv[2] : NULL); + r = efi_install_fdt(argc > 2 ? argv[2] : NULL); if (r == EFI_INVALID_PARAMETER) return CMD_RET_USAGE; else if (r != EFI_SUCCESS)
On 4/19/19 5:22 AM, AKASHI Takahiro wrote:
This is a preparatory patch for reworking do_bootefi() in later patch. For simplicity, merge two functions.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
cmd/bootefi.c | 67 +++++++++++++++++++++------------------------------ 1 file changed, 28 insertions(+), 39 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index fb6703ce84f3..c6d6eb7312e8 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -185,62 +185,51 @@ static void efi_carve_out_dt_rsv(void *fdt) } }
-static efi_status_t efi_install_fdt(ulong fdt_addr) -{
- bootm_headers_t img = { 0 };
- efi_status_t ret;
- void *fdt;
- fdt = map_sysmem(fdt_addr, 0);
- if (fdt_check_header(fdt)) {
printf("ERROR: invalid device tree\n");return EFI_INVALID_PARAMETER;- }
- /* Create memory reservation as indicated by the device tree */
- efi_carve_out_dt_rsv(fdt);
- /* Prepare fdt for payload */
- ret = copy_fdt(&fdt);
- if (ret)
return ret;- if (image_setup_libfdt(&img, fdt, 0, NULL)) {
printf("ERROR: failed to process device tree\n");return EFI_LOAD_ERROR;- }
- /* Link to it in the efi tables */
- ret = efi_install_configuration_table(&efi_guid_fdt, fdt);
- if (ret != EFI_SUCCESS)
return EFI_OUT_OF_RESOURCES;- return ret;
-}
- /**
- efi_process_fdt() - process fdt passed by a command argument
*/
- efi_install_fdt() - install fdt passed by a command argument
- @fdt_opt: pointer to argument
- Return: status code
- If specified, fdt will be installed as configuration table,
- otherwise no fdt will be passed.
-static efi_status_t efi_process_fdt(const char *fdt_opt) +static efi_status_t efi_install_fdt(const char *fdt_opt) { unsigned long fdt_addr;
void *fdt;
bootm_headers_t img = { 0 }; efi_status_t ret;
if (fdt_opt) {
/* Install device tree */fdt_addr = simple_strtoul(fdt_opt, NULL, 16); if (!fdt_addr && *fdt_opt != '0') return EFI_INVALID_PARAMETER;
/* Install device tree */ret = efi_install_fdt(fdt_addr);
fdt = map_sysmem(fdt_addr, 0);if (fdt_check_header(fdt)) {printf("ERROR: invalid device tree\n");return EFI_INVALID_PARAMETER;}/* Create memory reservation as indicated by the device tree */efi_carve_out_dt_rsv(fdt);/* Prepare fdt for payload */ret = copy_fdt(&fdt);if (ret)return ret;if (image_setup_libfdt(&img, fdt, 0, NULL)) {printf("ERROR: failed to process device tree\n");return EFI_LOAD_ERROR;}/* Link to it in the efi tables */ret = efi_install_configuration_table(&efi_guid_fdt, fdt);
return ret;
return EFI_OUT_OF_RESOURCES;
Why do you want to change the return value here? If there is not enough space efi_install_configuration_table() will return EFI_OUT_OF_RESOURCES by itself. Other possible error codes are:
* EFI_INVALID_PARAMETER if you do not provide a GUID. This cannot occur. * EFI_NOT_FOUND if fdt is NULL.
I you are ok with it I would apply the patch with the original line return ret;
Otherwise
Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de
}} else { /* Remove device tree. EFI_NOT_FOUND can be ignored here */ @@ -456,7 +445,7 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) if (argc < 2) return CMD_RET_USAGE;
- r = efi_process_fdt(argc > 2 ? argv[2] : NULL);
- r = efi_install_fdt(argc > 2 ? argv[2] : NULL); if (r == EFI_INVALID_PARAMETER) return CMD_RET_USAGE; else if (r != EFI_SUCCESS)
On Fri, Apr 19, 2019 at 06:06:08AM +0200, Heinrich Schuchardt wrote:
On 4/19/19 5:22 AM, AKASHI Takahiro wrote:
This is a preparatory patch for reworking do_bootefi() in later patch. For simplicity, merge two functions.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
cmd/bootefi.c | 67 +++++++++++++++++++++------------------------------ 1 file changed, 28 insertions(+), 39 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index fb6703ce84f3..c6d6eb7312e8 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -185,62 +185,51 @@ static void efi_carve_out_dt_rsv(void *fdt) } }
-static efi_status_t efi_install_fdt(ulong fdt_addr) -{
- bootm_headers_t img = { 0 };
- efi_status_t ret;
- void *fdt;
- fdt = map_sysmem(fdt_addr, 0);
- if (fdt_check_header(fdt)) {
printf("ERROR: invalid device tree\n");return EFI_INVALID_PARAMETER;- }
- /* Create memory reservation as indicated by the device tree */
- efi_carve_out_dt_rsv(fdt);
- /* Prepare fdt for payload */
- ret = copy_fdt(&fdt);
- if (ret)
return ret;- if (image_setup_libfdt(&img, fdt, 0, NULL)) {
printf("ERROR: failed to process device tree\n");return EFI_LOAD_ERROR;- }
- /* Link to it in the efi tables */
- ret = efi_install_configuration_table(&efi_guid_fdt, fdt);
- if (ret != EFI_SUCCESS)
return EFI_OUT_OF_RESOURCES;- return ret;
-}
/**
- efi_process_fdt() - process fdt passed by a command argument
*/
- efi_install_fdt() - install fdt passed by a command argument
- @fdt_opt: pointer to argument
- Return: status code
- If specified, fdt will be installed as configuration table,
- otherwise no fdt will be passed.
-static efi_status_t efi_process_fdt(const char *fdt_opt) +static efi_status_t efi_install_fdt(const char *fdt_opt) { unsigned long fdt_addr;
void *fdt;
bootm_headers_t img = { 0 }; efi_status_t ret;
if (fdt_opt) {
/* Install device tree */fdt_addr = simple_strtoul(fdt_opt, NULL, 16); if (!fdt_addr && *fdt_opt != '0') return EFI_INVALID_PARAMETER;
/* Install device tree */ret = efi_install_fdt(fdt_addr);
fdt = map_sysmem(fdt_addr, 0);if (fdt_check_header(fdt)) {printf("ERROR: invalid device tree\n");return EFI_INVALID_PARAMETER;}/* Create memory reservation as indicated by the device tree */efi_carve_out_dt_rsv(fdt);/* Prepare fdt for payload */ret = copy_fdt(&fdt);if (ret)return ret;if (image_setup_libfdt(&img, fdt, 0, NULL)) {printf("ERROR: failed to process device tree\n");return EFI_LOAD_ERROR;}/* Link to it in the efi tables */ret = efi_install_configuration_table(&efi_guid_fdt, fdt);
return ret;
return EFI_OUT_OF_RESOURCES;Why do you want to change the return value here? If there is not enough space efi_install_configuration_table() will return EFI_OUT_OF_RESOURCES by itself. Other possible error codes are:
The original code returns EFI_LOAD_ERROR, so I changed it.
- EFI_INVALID_PARAMETER if you do not provide a GUID. This cannot occur.
- EFI_NOT_FOUND if fdt is NULL.
I you are ok with it I would apply the patch with the original line return ret;
Go ahead. # You don't need additional patch, Just leave a note in commit message.
Thanks, -Takahiro Akashi
Otherwise
Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de
}} else { /* Remove device tree. EFI_NOT_FOUND can be ignored here */ @@ -456,7 +445,7 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) if (argc < 2) return CMD_RET_USAGE;
- r = efi_process_fdt(argc > 2 ? argv[2] : NULL);
- r = efi_install_fdt(argc > 2 ? argv[2] : NULL); if (r == EFI_INVALID_PARAMETER) return CMD_RET_USAGE; else if (r != EFI_SUCCESS)
This is a preparatory patch for reworking do_bootefi() in later patch.
Efi_selftest code is unusual in terms of execution path in do_bootefi(), which make that function complicated and hard to understand. With this patch, all efi_selftest related code will be put in a separate function.
The change also includes expanding efi_run_prepare() and efi_run_finish() in do_bootefi_exec().
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org --- cmd/bootefi.c | 145 ++++++++++++++++++++++++++++++++------------------ 1 file changed, 92 insertions(+), 53 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index c6d6eb7312e8..e73b4cb563cd 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -239,37 +239,6 @@ static efi_status_t efi_install_fdt(const char *fdt_opt) return EFI_SUCCESS; }
-static efi_status_t bootefi_run_prepare(const char *load_options_path, - struct efi_device_path *device_path, - struct efi_device_path *image_path, - struct efi_loaded_image_obj **image_objp, - struct efi_loaded_image **loaded_image_infop) -{ - efi_status_t ret; - - ret = efi_setup_loaded_image(device_path, image_path, image_objp, - loaded_image_infop); - if (ret != EFI_SUCCESS) - return ret; - - /* Transfer environment variable as load options */ - return set_load_options((efi_handle_t)*image_objp, load_options_path); -} - -/** - * bootefi_run_finish() - finish up after running an EFI test - * - * @loaded_image_info: Pointer to a struct which holds the loaded image info - * @image_objj: Pointer to a struct which holds the loaded image object - */ -static void bootefi_run_finish(struct efi_loaded_image_obj *image_obj, - struct efi_loaded_image *loaded_image_info) -{ - efi_restore_gd(); - free(loaded_image_info->load_options); - efi_delete_handle(&image_obj->header); -} - /** * do_bootefi_exec() - execute EFI binary * @@ -316,11 +285,16 @@ static efi_status_t do_bootefi_exec(void *efi, assert(device_path && image_path); }
- ret = bootefi_run_prepare("bootargs", device_path, image_path, - &image_obj, &loaded_image_info); + 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); if (ret != EFI_SUCCESS) @@ -344,7 +318,9 @@ static efi_status_t do_bootefi_exec(void *efi,
err_prepare: /* image has returned, loaded-image obj goes *poof*: */ - bootefi_run_finish(image_obj, loaded_image_info); + efi_restore_gd(); + free(loaded_image_info->load_options); + efi_delete_handle(&image_obj->header);
err_add_protocol: if (mem_handle) @@ -354,6 +330,23 @@ err_add_protocol: }
#ifdef CONFIG_CMD_BOOTEFI_SELFTEST +static efi_status_t bootefi_run_prepare(const char *load_options_path, + struct efi_device_path *device_path, + struct efi_device_path *image_path, + struct efi_loaded_image_obj **image_objp, + struct efi_loaded_image **loaded_image_infop) +{ + efi_status_t ret; + + ret = efi_setup_loaded_image(device_path, image_path, image_objp, + loaded_image_infop); + if (ret != EFI_SUCCESS) + return ret; + + /* Transfer environment variable as load options */ + return set_load_options((efi_handle_t)*image_objp, load_options_path); +} + /** * bootefi_test_prepare() - prepare to run an EFI test * @@ -399,6 +392,64 @@ failure: return ret; }
+/** + * bootefi_run_finish() - finish up after running an EFI test + * + * @loaded_image_info: Pointer to a struct which holds the loaded image info + * @image_obj: Pointer to a struct which holds the loaded image object + */ +static void bootefi_run_finish(struct efi_loaded_image_obj *image_obj, + struct efi_loaded_image *loaded_image_info) +{ + efi_restore_gd(); + free(loaded_image_info->load_options); + efi_delete_handle(&image_obj->header); +} + +/** + * do_efi_selftest() - execute EFI Selftest + * + * @fdt_opt: string of fdt start address + * Return: status code + * + * Execute EFI Selftest + */ +static int do_efi_selftest(const char *fdt_opt) +{ + struct efi_loaded_image_obj *image_obj; + struct efi_loaded_image *loaded_image_info; + efi_status_t ret; + + /* Allow unaligned memory access */ + allow_unaligned(); + + switch_to_non_secure_mode(); + + /* Initialize EFI drivers */ + ret = efi_init_obj_list(); + if (ret != EFI_SUCCESS) { + printf("Error: Cannot initialize UEFI sub-system, r = %lu\n", + ret & ~EFI_ERROR_MASK); + return CMD_RET_FAILURE; + } + + ret = efi_install_fdt(fdt_opt); + if (ret == EFI_INVALID_PARAMETER) + return CMD_RET_USAGE; + else if (ret != EFI_SUCCESS) + return CMD_RET_FAILURE; + + ret = bootefi_test_prepare(&image_obj, &loaded_image_info, + "\selftest", "efi_selftest"); + if (ret != EFI_SUCCESS) + return CMD_RET_FAILURE; + + /* Execute the test */ + ret = EFI_CALL(efi_selftest(&image_obj->header, &systab)); + bootefi_run_finish(image_obj, loaded_image_info); + + return ret != EFI_SUCCESS; +} #endif /* CONFIG_CMD_BOOTEFI_SELFTEST */
static int do_bootefi_bootmgr_exec(void) @@ -429,6 +480,13 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) char *saddr; efi_status_t r;
+ if (argc < 2) + return CMD_RET_USAGE; +#ifdef CONFIG_CMD_BOOTEFI_SELFTEST + else if (!strcmp(argv[1], "selftest")) + return do_efi_selftest(argc > 2 ? argv[2] : NULL); +#endif + /* Allow unaligned memory access */ allow_unaligned();
@@ -442,9 +500,6 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) return CMD_RET_FAILURE; }
- if (argc < 2) - return CMD_RET_USAGE; - r = efi_install_fdt(argc > 2 ? argv[2] : NULL); if (r == EFI_INVALID_PARAMETER) return CMD_RET_USAGE; @@ -462,22 +517,6 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) addr = CONFIG_SYS_LOAD_ADDR; memcpy(map_sysmem(addr, size), __efi_helloworld_begin, size); } else -#endif -#ifdef CONFIG_CMD_BOOTEFI_SELFTEST - if (!strcmp(argv[1], "selftest")) { - struct efi_loaded_image_obj *image_obj; - struct efi_loaded_image *loaded_image_info; - - r = bootefi_test_prepare(&image_obj, &loaded_image_info, - "\selftest", "efi_selftest"); - if (r != EFI_SUCCESS) - return CMD_RET_FAILURE; - - /* Execute the test */ - r = EFI_CALL(efi_selftest(&image_obj->header, &systab)); - bootefi_run_finish(image_obj, loaded_image_info); - return r != EFI_SUCCESS; - } else #endif if (!strcmp(argv[1], "bootmgr")) { return do_bootefi_bootmgr_exec();
On 4/19/19 5:22 AM, AKASHI Takahiro wrote:
This is a preparatory patch for reworking do_bootefi() in later patch.
Efi_selftest code is unusual in terms of execution path in do_bootefi(), which make that function complicated and hard to understand. With this patch, all efi_selftest related code will be put in a separate function.
The change also includes expanding efi_run_prepare() and efi_run_finish() in do_bootefi_exec().
Signed-off-by: AKASHI Takahirotakahiro.akashi@linaro.org
Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de
This is a preparatory patch for reworking do_bootefi() in later patch.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de --- cmd/bootefi.c | 42 +++++++++++++++++++++--------------------- 1 file changed, 21 insertions(+), 21 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index e73b4cb563cd..d8ddd770e031 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -329,6 +329,27 @@ err_add_protocol: return ret; }
+static int do_bootefi_bootmgr_exec(void) +{ + struct efi_device_path *device_path, *file_path; + void *addr; + efi_status_t r; + + addr = efi_bootmgr_load(&device_path, &file_path); + if (!addr) + return 1; + + printf("## Starting EFI application at %p ...\n", addr); + r = do_bootefi_exec(addr, device_path, file_path); + printf("## Application terminated, r = %lu\n", + r & ~EFI_ERROR_MASK); + + if (r != EFI_SUCCESS) + return 1; + + return 0; +} + #ifdef CONFIG_CMD_BOOTEFI_SELFTEST static efi_status_t bootefi_run_prepare(const char *load_options_path, struct efi_device_path *device_path, @@ -452,27 +473,6 @@ static int do_efi_selftest(const char *fdt_opt) } #endif /* CONFIG_CMD_BOOTEFI_SELFTEST */
-static int do_bootefi_bootmgr_exec(void) -{ - struct efi_device_path *device_path, *file_path; - void *addr; - efi_status_t r; - - addr = efi_bootmgr_load(&device_path, &file_path); - if (!addr) - return 1; - - printf("## Starting EFI application at %p ...\n", addr); - r = do_bootefi_exec(addr, device_path, file_path); - printf("## Application terminated, r = %lu\n", - r & ~EFI_ERROR_MASK); - - if (r != EFI_SUCCESS) - return 1; - - return 0; -} - /* Interpreter command to boot an arbitrary EFI image from memory */ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) {
This is a preparatory patch for reworking do_bootefi() in later patch. do_bootmgr_exec() is renamed to do_efibootmgr() as we put all the necessary code into this function.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org --- cmd/bootefi.c | 44 ++++++++++++++++++++++++++++++++++++-------- 1 file changed, 36 insertions(+), 8 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index d8ddd770e031..2822456d8128 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -329,22 +329,49 @@ err_add_protocol: return ret; }
-static int do_bootefi_bootmgr_exec(void) +/** + * do_efibootmgr() - execute EFI Boot Manager + * + * @fdt_opt: string of fdt start address + * Return: status code + * + * Execute EFI Boot Manager + */ +static int do_efibootmgr(const char *fdt_opt) { struct efi_device_path *device_path, *file_path; void *addr; - efi_status_t r; + efi_status_t ret; + + /* Allow unaligned memory access */ + allow_unaligned(); + + switch_to_non_secure_mode(); + + /* Initialize EFI drivers */ + ret = efi_init_obj_list(); + if (ret != EFI_SUCCESS) { + printf("Error: Cannot initialize UEFI sub-system, r = %lu\n", + ret & ~EFI_ERROR_MASK); + return CMD_RET_FAILURE; + } + + ret = efi_install_fdt(fdt_opt); + if (ret == EFI_INVALID_PARAMETER) + return CMD_RET_USAGE; + else if (ret != EFI_SUCCESS) + return CMD_RET_FAILURE;
addr = efi_bootmgr_load(&device_path, &file_path); if (!addr) return 1;
printf("## Starting EFI application at %p ...\n", addr); - r = do_bootefi_exec(addr, device_path, file_path); + ret = do_bootefi_exec(addr, device_path, file_path); printf("## Application terminated, r = %lu\n", - r & ~EFI_ERROR_MASK); + ret & ~EFI_ERROR_MASK);
- if (r != EFI_SUCCESS) + if (ret != EFI_SUCCESS) return 1;
return 0; @@ -482,6 +509,9 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
if (argc < 2) return CMD_RET_USAGE; + + if (!strcmp(argv[1], "bootmgr")) + return do_efibootmgr(argc > 2 ? argv[2] : NULL); #ifdef CONFIG_CMD_BOOTEFI_SELFTEST else if (!strcmp(argv[1], "selftest")) return do_efi_selftest(argc > 2 ? argv[2] : NULL); @@ -518,9 +548,7 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) memcpy(map_sysmem(addr, size), __efi_helloworld_begin, size); } else #endif - if (!strcmp(argv[1], "bootmgr")) { - return do_bootefi_bootmgr_exec(); - } else { + { saddr = argv[1];
addr = simple_strtoul(saddr, NULL, 16);
On 4/19/19 5:22 AM, AKASHI Takahiro wrote:
This is a preparatory patch for reworking do_bootefi() in later patch. do_bootmgr_exec() is renamed to do_efibootmgr() as we put all the necessary code into this function.
Signed-off-by: AKASHI Takahirotakahiro.akashi@linaro.org
Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de
This is a preparatory patch for reworking do_bootefi() in later patch. All the non-boot-manager-based (that is, bootefi <addr>) code is put into one function, do_bootefi_image().
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de --- cmd/bootefi.c | 122 +++++++++++++++++++++++++++----------------------- 1 file changed, 67 insertions(+), 55 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 2822456d8128..e2ca68c4dc12 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -377,6 +377,72 @@ static int do_efibootmgr(const char *fdt_opt) return 0; }
+/* + * do_bootefi_image() - execute EFI binary from command line + * + * @image_opt: string of image start address + * @fdt_opt: string of fdt start address + * Return: status code + * + * Set up memory image for the binary to be loaded, prepare + * device path and then call do_bootefi_exec() to execute it. + */ +static int do_bootefi_image(const char *image_opt, const char *fdt_opt) +{ + unsigned long addr; + efi_status_t ret; + + /* Allow unaligned memory access */ + allow_unaligned(); + + switch_to_non_secure_mode(); + + /* Initialize EFI drivers */ + ret = efi_init_obj_list(); + if (ret != EFI_SUCCESS) { + printf("Error: Cannot initialize UEFI sub-system, r = %lu\n", + ret & ~EFI_ERROR_MASK); + return CMD_RET_FAILURE; + } + + ret = efi_install_fdt(fdt_opt); + if (ret == EFI_INVALID_PARAMETER) + return CMD_RET_USAGE; + else if (ret != EFI_SUCCESS) + return CMD_RET_FAILURE; + +#ifdef CONFIG_CMD_BOOTEFI_HELLO + if (!strcmp(image_opt, "hello")) { + char *saddr; + ulong size = __efi_helloworld_end - __efi_helloworld_begin; + + saddr = env_get("loadaddr"); + if (saddr) + addr = simple_strtoul(saddr, NULL, 16); + else + addr = CONFIG_SYS_LOAD_ADDR; + memcpy(map_sysmem(addr, size), __efi_helloworld_begin, size); + } else +#endif + { + addr = simple_strtoul(image_opt, NULL, 16); + /* Check that a numeric value was passed */ + if (!addr && *image_opt != '0') + return CMD_RET_USAGE; + } + + 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 (ret != EFI_SUCCESS) + return CMD_RET_FAILURE; + else + return CMD_RET_SUCCESS; +} + #ifdef CONFIG_CMD_BOOTEFI_SELFTEST static efi_status_t bootefi_run_prepare(const char *load_options_path, struct efi_device_path *device_path, @@ -503,10 +569,6 @@ static int do_efi_selftest(const char *fdt_opt) /* Interpreter command to boot an arbitrary EFI image from memory */ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { - unsigned long addr; - char *saddr; - efi_status_t r; - if (argc < 2) return CMD_RET_USAGE;
@@ -517,57 +579,7 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) return do_efi_selftest(argc > 2 ? argv[2] : NULL); #endif
- /* Allow unaligned memory access */ - allow_unaligned(); - - switch_to_non_secure_mode(); - - /* Initialize EFI drivers */ - r = efi_init_obj_list(); - if (r != EFI_SUCCESS) { - printf("Error: Cannot set up EFI drivers, r = %lu\n", - r & ~EFI_ERROR_MASK); - return CMD_RET_FAILURE; - } - - r = efi_install_fdt(argc > 2 ? argv[2] : NULL); - if (r == EFI_INVALID_PARAMETER) - return CMD_RET_USAGE; - else if (r != EFI_SUCCESS) - return CMD_RET_FAILURE; - -#ifdef CONFIG_CMD_BOOTEFI_HELLO - if (!strcmp(argv[1], "hello")) { - ulong size = __efi_helloworld_end - __efi_helloworld_begin; - - saddr = env_get("loadaddr"); - if (saddr) - addr = simple_strtoul(saddr, NULL, 16); - else - addr = CONFIG_SYS_LOAD_ADDR; - memcpy(map_sysmem(addr, size), __efi_helloworld_begin, size); - } else -#endif - { - saddr = argv[1]; - - addr = simple_strtoul(saddr, NULL, 16); - /* Check that a numeric value was passed */ - if (!addr && *saddr != '0') - return CMD_RET_USAGE; - - } - - printf("## Starting EFI application at %08lx ...\n", addr); - r = do_bootefi_exec(map_sysmem(addr, 0), bootefi_device_path, - bootefi_image_path); - printf("## Application terminated, r = %lu\n", - r & ~EFI_ERROR_MASK); - - if (r != EFI_SUCCESS) - return 1; - else - return 0; + return do_bootefi_image(argv[1], argc > 2 ? argv[2] : NULL); }
#ifdef CONFIG_SYS_LONGHELP
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 | 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 */, + 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 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));
/*
On 4/19/19 5:22 AM, AKASHI Takahiro wrote:
- /*
* FIXME: Who is responsible for* free(loaded_image_info->load_options);* Once efi_exit() is implemented correctly,* handle itself doesn't exist here.*/
Load option can only be freed when the image is unloaded. For applications that will happen when the application call Exit(). For drivers there is not guarantee that they will be ever unloaded.
In Unload() we should not free load options if they were not allocated by us but by an EFI binary.
In your implementation you have allocated the load options via calloc() and not from the EFI pool. So this may allow us to determine whether we allocated the load options in the Unload() service.
Best regards
Heinrich
On Sat, Apr 20, 2019 at 10:07:20AM +0200, Heinrich Schuchardt wrote:
On 4/19/19 5:22 AM, AKASHI Takahiro wrote:
- /*
* FIXME: Who is responsible for* free(loaded_image_info->load_options);* Once efi_exit() is implemented correctly,* handle itself doesn't exist here.*/Load option can only be freed when the image is unloaded. For applications that will happen when the application call Exit(). For drivers there is not guarantee that they will be ever unloaded.
Should we be able to utilize "bootargs" even for drivers?
In Unload() we should not free load options if they were not allocated by us but by an EFI binary.
In your implementation you have allocated the load options via calloc() and not from the EFI pool. So this may allow us to determine whether we allocated the load options in the Unload() service.
DO you mean: efi_unload_image(...) ret = EFI_CALL(efi_pool_free(info->load_options)); if (ret != EFI_SUCCESS) /* * Don't care, assuming that load_options have been allocated * by application. */
It looks fragile.
-Takahiro Akashi
Best regards
Heinrich
On 4/19/19 5:22 AM, AKASHI Takahiro wrote:
In the current implementation, bootefi command and EFI boot manager don't use load_image API, instead, use more primitive and internal functions. This will introduce duplicated code and potentially unknown bugs as well as inconsistent behaviours.
With this patch, do_efibootmgr() and do_boot_efi() are completely overhauled and re-implemented using load_image API.
Signed-off-by: AKASHI Takahiro takahiro.akashi@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;size_str = env_get("filesize");if (size_str)size = simple_strtoul(size_str, NULL, 16);elsesize = 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);}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);@@ -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));
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.glpk@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; }@@ -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));
/*
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.akashi@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.glpk@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));
/*
On Sat, Apr 20, 2019 at 07:37:47PM +0200, Heinrich Schuchardt wrote:
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?
No. I test my patch only with Shell.efi right now.
-Takahiro Akashi
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.akashi@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.glpk@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));
/*
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.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
<snip />
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 */,
boot_policy should be true for requests originating from the boot manager.
Best regards
Heinrich
On Sat, Apr 20, 2019 at 10:37:54AM +0200, 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.
Signed-off-by: AKASHI Takahiro takahiro.akashi@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;#endif {
size_str = env_get("filesize");if (size_str)size = simple_strtoul(size_str, NULL, 16);elsesize = 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);}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.
Thanks, but only if we need more fixes in this patch.
lo.file_path, NULL, 0,handle));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.
Right, I have not noticed this issue.
-Takahiro Akashi
A the problem was not introduced with this patch series:
Reviewed-by: Heinrich Schuchardt xypron.glpk@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; }@@ -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));
/*
On 4/19/19 5:22 AM, AKASHI Takahiro wrote:
In the current implementation, bootefi command and EFI boot manager don't use load_image API, instead, use more primitive and internal functions. This will introduce duplicated code and potentially unknown bugs as well as inconsistent behaviours.
With this patch, do_efibootmgr() and do_boot_efi() are completely overhauled and re-implemented using load_image API.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
<snip />
+err_prepare:
- if (device_path)
efi_free_pool(file_path);
This free is duplicate to the one below. And why should freeing file_path depend on device_path?
Regards Heinrich
+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;
}
Add CONFIG_CMD_STANDALONE_EFIBOOTMGR. With this patch, EFI boot manager can be kicked in by a standalone command, efibootmgr.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org --- cmd/Kconfig | 8 ++++++++ cmd/bootefi.c | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+)
diff --git a/cmd/Kconfig b/cmd/Kconfig index 2bdbfcb3d091..46222be4a3c6 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -224,6 +224,14 @@ config CMD_BOOTEFI help Boot an EFI image from memory.
+config CMD_STANDALONE_EFIBOOTMGR + bool "Enable EFI Boot Manager as a standalone command" + depends on CMD_BOOTEFI + default n + help + With this option enabled, EFI Boot Manager can be invoked + as a standalone command. + config CMD_BOOTEFI_HELLO_COMPILE bool "Compile a standard EFI hello world binary for testing" depends on CMD_BOOTEFI && !CPU_V7M && !SANDBOX diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 8c84fff590a7..348c6fdbe28d 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -644,3 +644,38 @@ void efi_set_bootdev(const char *dev, const char *devnr, const char *path) bootefi_image_path = NULL; } } + +#ifdef CONFIG_CMD_STANDALONE_EFIBOOTMGR +static int do_efibootmgr_cmd(cmd_tbl_t *cmdtp, int flag, int argc, + char * const argv[]) +{ + char *fdt_opt; + int ret; + + if (argc != 1) + return CMD_RET_USAGE; + + fdt_opt = env_get("fdtaddr"); + if (!fdt_opt) + fdt_opt = env_get("fdtcontroladdr"); + + ret = do_efibootmgr(fdt_opt); + + free(fdt_opt); + + return ret; +} + +#ifdef CONFIG_SYS_LONGHELP +static char efibootmgr_help_text[] = + "(no arguments)\n" + " - Launch EFI boot manager and execute EFI application\n" + " based on BootNext and BootOrder variables\n"; +#endif + +U_BOOT_CMD( + efibootmgr, 1, 0, do_efibootmgr_cmd, + "Launch EFI boot manager", + efibootmgr_help_text +); +#endif /* CONFIG_CMD_STANDALONE_EFIBOOTMGR */
On 4/19/19 5:22 AM, AKASHI Takahiro wrote:
Add CONFIG_CMD_STANDALONE_EFIBOOTMGR. With this patch, EFI boot manager can be kicked in by a standalone command, efibootmgr.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
I still do not see that we really need a command efibootmgr duplicationg `bootefi bootmgr`.
What I think is worthwhile in your patch is falling back to $fdt_control_addr.
The Embedded Base Boot Requirements Specification (EBBR) requires that we always have either an ACPI table or a device tree but not both.
So if no ACPI table is provided we should fallback to $fdtcontroladdr. If an ACPI table is present (i.e. on x86) we should not allow an FDT to be passed to bootefi.
Best regards
Heinrich
cmd/Kconfig | 8 ++++++++ cmd/bootefi.c | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+)
diff --git a/cmd/Kconfig b/cmd/Kconfig index 2bdbfcb3d091..46222be4a3c6 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -224,6 +224,14 @@ config CMD_BOOTEFI help Boot an EFI image from memory.
+config CMD_STANDALONE_EFIBOOTMGR
- bool "Enable EFI Boot Manager as a standalone command"
- depends on CMD_BOOTEFI
- default n
- help
With this option enabled, EFI Boot Manager can be invokedas a standalone command.- config CMD_BOOTEFI_HELLO_COMPILE bool "Compile a standard EFI hello world binary for testing" depends on CMD_BOOTEFI && !CPU_V7M && !SANDBOX
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 8c84fff590a7..348c6fdbe28d 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -644,3 +644,38 @@ void efi_set_bootdev(const char *dev, const char *devnr, const char *path) bootefi_image_path = NULL; } }
+#ifdef CONFIG_CMD_STANDALONE_EFIBOOTMGR +static int do_efibootmgr_cmd(cmd_tbl_t *cmdtp, int flag, int argc,
char * const argv[])+{
- char *fdt_opt;
- int ret;
- if (argc != 1)
return CMD_RET_USAGE;- fdt_opt = env_get("fdtaddr");
- if (!fdt_opt)
fdt_opt = env_get("fdtcontroladdr");- ret = do_efibootmgr(fdt_opt);
- free(fdt_opt);
- return ret;
+}
+#ifdef CONFIG_SYS_LONGHELP +static char efibootmgr_help_text[] =
- "(no arguments)\n"
- " - Launch EFI boot manager and execute EFI application\n"
- " based on BootNext and BootOrder variables\n";
+#endif
+U_BOOT_CMD(
- efibootmgr, 1, 0, do_efibootmgr_cmd,
- "Launch EFI boot manager",
- efibootmgr_help_text
+); +#endif /* CONFIG_CMD_STANDALONE_EFIBOOTMGR */
On 4/19/19 5:22 AM, AKASHI Takahiro wrote:
There are several reasons that I want to rework/refactor bootefi command as well as bootmgr:
Some previous commits on bootefi.c have made the code complicated and a bit hard to understand.
do_bootefi_exec() would better be implemented using load_image() along with start_image() to be aligned with UEFI interfaces.
Contrary to the other part, efi_selftest part of the code is unusual in terms of loading/execution path in do_bootefi().
When we will support "secure boot" in the future, EFI Boot Manager is expected to be invoked as a standalone command without any arguments to mitigate security surfaces.
In this patch set, Patch#1 to #7 are preparatory patches for patch#8. Patch#8 is a core part of reworking. Patch#9 is for standalone boot manager.
Hello Takahiro,
your patches 1-8 are now (with some modifications) in the efi-2019-07 branch. I have added some more patches.
https://github.com/xypron2/u-boot/commits/efi-2019-07
Travis CI testing was ok: https://travis-ci.org/xypron2/u-boot/builds/522947698
I have tested on real hardware without error: * TinkerBoard (32 bit) boot via GRUB * Pine64 A64 LTS (64 bit) boot via iPXE and GRUB * Odroid C2 (64 bit) boot via GRUB
If you are ok with the adjustments to your patch series I will create a pull request.
Best regards
Heinrich
Heinrich,
On Tue, 23 Apr 2019 at 03:16, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 4/19/19 5:22 AM, AKASHI Takahiro wrote:
There are several reasons that I want to rework/refactor bootefi command as well as bootmgr:
Some previous commits on bootefi.c have made the code complicated and a bit hard to understand.
do_bootefi_exec() would better be implemented using load_image() along with start_image() to be aligned with UEFI interfaces.
Contrary to the other part, efi_selftest part of the code is unusual in terms of loading/execution path in do_bootefi().
When we will support "secure boot" in the future, EFI Boot Manager is expected to be invoked as a standalone command without any arguments to mitigate security surfaces.
In this patch set, Patch#1 to #7 are preparatory patches for patch#8. Patch#8 is a core part of reworking. Patch#9 is for standalone boot manager.
Hello Takahiro,
your patches 1-8 are now (with some modifications) in the efi-2019-07 branch. I have added some more patches.
https://github.com/xypron2/u-boot/commits/efi-2019-07
Travis CI testing was ok: https://travis-ci.org/xypron2/u-boot/builds/522947698
I have tested on real hardware without error:
- TinkerBoard (32 bit) boot via GRUB
- Pine64 A64 LTS (64 bit) boot via iPXE and GRUB
- Odroid C2 (64 bit) boot via GRUB
If you are ok with the adjustments to your patch series I will create a pull request.
Thank you for the merge. One question: how did you fix a grub error that you reported before?
-Takahiro Akashi
Best regards
Heinrich
On 4/23/19 2:24 AM, AKASHI, Takahiro wrote:
Heinrich,
On Tue, 23 Apr 2019 at 03:16, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 4/19/19 5:22 AM, AKASHI Takahiro wrote:
There are several reasons that I want to rework/refactor bootefi command as well as bootmgr:
Some previous commits on bootefi.c have made the code complicated and a bit hard to understand.
do_bootefi_exec() would better be implemented using load_image() along with start_image() to be aligned with UEFI interfaces.
Contrary to the other part, efi_selftest part of the code is unusual in terms of loading/execution path in do_bootefi().
When we will support "secure boot" in the future, EFI Boot Manager is expected to be invoked as a standalone command without any arguments to mitigate security surfaces.
In this patch set, Patch#1 to #7 are preparatory patches for patch#8. Patch#8 is a core part of reworking. Patch#9 is for standalone boot manager.
Hello Takahiro,
your patches 1-8 are now (with some modifications) in the efi-2019-07 branch. I have added some more patches.
https://github.com/xypron2/u-boot/commits/efi-2019-07
Travis CI testing was ok: https://travis-ci.org/xypron2/u-boot/builds/522947698
I have tested on real hardware without error:
- TinkerBoard (32 bit) boot via GRUB
- Pine64 A64 LTS (64 bit) boot via iPXE and GRUB
- Odroid C2 (64 bit) boot via GRUB
If you are ok with the adjustments to your patch series I will create a pull request.
Thank you for the merge. One question: how did you fix a grub error that you reported before?
I fixed the issue that iPXE could not find a chained script via efi_loader: correctly split device path of loaded image https://lists.denx.de/pipermail/u-boot/2019-April/365907.html
Furthermore I fixed a bug in Debian's QEMU by applying Alex's https://github.com/qemu/qemu/commit/2d2a4549cc29850aab891495685a7b31f5254b12
Best regards
Heinrich
-Takahiro Akashi
Best regards
Heinrich
Okay. Once your pull is done, I will submit "efi variable" patch, which is more experimental than bootefi/bootmgr rework.
-Takahiro Akashi
On Tue, 23 Apr 2019 at 13:57, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 4/23/19 2:24 AM, AKASHI, Takahiro wrote:
Heinrich,
On Tue, 23 Apr 2019 at 03:16, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 4/19/19 5:22 AM, AKASHI Takahiro wrote:
There are several reasons that I want to rework/refactor bootefi command as well as bootmgr:
Some previous commits on bootefi.c have made the code complicated and a bit hard to understand.
do_bootefi_exec() would better be implemented using load_image() along with start_image() to be aligned with UEFI interfaces.
Contrary to the other part, efi_selftest part of the code is unusual in terms of loading/execution path in do_bootefi().
When we will support "secure boot" in the future, EFI Boot Manager is expected to be invoked as a standalone command without any arguments to mitigate security surfaces.
In this patch set, Patch#1 to #7 are preparatory patches for patch#8. Patch#8 is a core part of reworking. Patch#9 is for standalone boot manager.
Hello Takahiro,
your patches 1-8 are now (with some modifications) in the efi-2019-07 branch. I have added some more patches.
https://github.com/xypron2/u-boot/commits/efi-2019-07
Travis CI testing was ok: https://travis-ci.org/xypron2/u-boot/builds/522947698
I have tested on real hardware without error:
- TinkerBoard (32 bit) boot via GRUB
- Pine64 A64 LTS (64 bit) boot via iPXE and GRUB
- Odroid C2 (64 bit) boot via GRUB
If you are ok with the adjustments to your patch series I will create a pull request.
Thank you for the merge. One question: how did you fix a grub error that you reported before?
I fixed the issue that iPXE could not find a chained script via efi_loader: correctly split device path of loaded image https://lists.denx.de/pipermail/u-boot/2019-April/365907.html
Furthermore I fixed a bug in Debian's QEMU by applying Alex's https://github.com/qemu/qemu/commit/2d2a4549cc29850aab891495685a7b31f5254b12
Best regards
Heinrich
-Takahiro Akashi
Best regards
Heinrich
On 4/19/19 5:22 AM, AKASHI Takahiro wrote:
There are several reasons that I want to rework/refactor bootefi command as well as bootmgr:
Some previous commits on bootefi.c have made the code complicated and a bit hard to understand.
do_bootefi_exec() would better be implemented using load_image() along with start_image() to be aligned with UEFI interfaces.
Contrary to the other part, efi_selftest part of the code is unusual in terms of loading/execution path in do_bootefi().
When we will support "secure boot" in the future, EFI Boot Manager is expected to be invoked as a standalone command without any arguments to mitigate security surfaces.
In this patch set, Patch#1 to #7 are preparatory patches for patch#8. Patch#8 is a core part of reworking. Patch#9 is for standalone boot manager.
# Please note that some patches, say patch#2 and #3, can be combined into one # but I intentionally keep them separated to clarify my intentions of changes.
Issues:
- It would be better off to change the semantics of efi_dp_from_name().
no chance to free loaded_image_info->load_options. (see patch #8)
-Takahiro Akashi
Hello Takahiro,
with the `efidebug boot add` command we can define load options for the BootXXXX variables.
But in do_efibootmgr() we call do_bootefi_exec() which calls set_load_options() and passes the value of environment variable bootargs as load options or if the variable is not set an empty string.
Here is an example console output:
=> setenv bootargs This is a value from bootargs => efidebug boot add 0000 hello scsi 0:1 helloworld.efi 'This is a value from efidebug' => efidebug boot order 0000 => bootefi bootmgr Booting: hello Hello, world! Running on UEFI 2.8 Have SMBIOS table Have device tree Load options: This is a value from bootargs ## Application terminated, r = 0
Now the same after deleting variable bootargs:
=> setenv bootargs => bootefi bootmgr Booting: hello Hello, world! Running on UEFI 2.8 Have SMBIOS table Have device tree Load options: <none> ## Application terminated, r = 0 =>
What behavior would you expect:
a) if the boot option has a load options value, b) if the boot option has no load options value?
One solution would be to define that bootargs is always ignored if the boot manager is used.
Best regards
Heinrich
Heinrich,
On Fri, Jan 03, 2020 at 01:17:05AM +0100, Heinrich Schuchardt wrote:
On 4/19/19 5:22 AM, AKASHI Takahiro wrote:
There are several reasons that I want to rework/refactor bootefi command as well as bootmgr:
Some previous commits on bootefi.c have made the code complicated and a bit hard to understand.
do_bootefi_exec() would better be implemented using load_image() along with start_image() to be aligned with UEFI interfaces.
Contrary to the other part, efi_selftest part of the code is unusual in terms of loading/execution path in do_bootefi().
When we will support "secure boot" in the future, EFI Boot Manager is expected to be invoked as a standalone command without any arguments to mitigate security surfaces.
In this patch set, Patch#1 to #7 are preparatory patches for patch#8. Patch#8 is a core part of reworking. Patch#9 is for standalone boot manager.
# Please note that some patches, say patch#2 and #3, can be combined into one # but I intentionally keep them separated to clarify my intentions of changes.
Issues:
- It would be better off to change the semantics of efi_dp_from_name().
no chance to free loaded_image_info->load_options. (see patch #8)
-Takahiro Akashi
Hello Takahiro,
with the `efidebug boot add` command we can define load options for the BootXXXX variables.
But in do_efibootmgr() we call do_bootefi_exec() which calls set_load_options() and passes the value of environment variable bootargs as load options or if the variable is not set an empty string.
Here is an example console output:
=> setenv bootargs This is a value from bootargs => efidebug boot add 0000 hello scsi 0:1 helloworld.efi 'This is a value from efidebug' => efidebug boot order 0000 => bootefi bootmgr Booting: hello Hello, world! Running on UEFI 2.8 Have SMBIOS table Have device tree Load options: This is a value from bootargs ## Application terminated, r = 0
Now the same after deleting variable bootargs:
=> setenv bootargs => bootefi bootmgr Booting: hello Hello, world! Running on UEFI 2.8 Have SMBIOS table Have device tree Load options: <none> ## Application terminated, r = 0 =>
Yeah, this is not what I intended.
What behavior would you expect:
a) if the boot option has a load options value, b) if the boot option has no load options value?
One solution would be to define that bootargs is always ignored if the boot manager is used.
I agree. Basically, "bootargs" is only for "bootefi <addr>" command, while "BootXXXX" should work in the exact same way as the UEFI specification defines.
Thanks, -Takahiro Akashi
Best regards
Heinrich
participants (3)
-
AKASHI Takahiro -
AKASHI, Takahiro
-
Heinrich Schuchardt