* [U-Boot] [RFC v3 00/10] efi_loader: rework bootefi/bootmgr
@ 2019-04-16 4:24 AKASHI Takahiro
2019-04-16 4:24 ` [U-Boot] [RFC v3 01/10] efi_loader: device_path: handle special case of loading AKASHI Takahiro
` (9 more replies)
0 siblings, 10 replies; 32+ messages in thread
From: AKASHI Takahiro @ 2019-04-16 4:24 UTC (permalink / raw)
To: u-boot
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 #8 are preparatory patches for patch#9.
Patch#9 is a core part of reworking.
Patch#10 is for standalone boot manager.
# Please note that some patches, say patch#3 and #4, can be combined into one
# but I intentionally keep them separated to clarify my intentions of changes.
Issues:
* The semantics of efi_dp_from_name() should be changed.
(See FIXME in patch#9.)
-Takahiro Akashi
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 (10):
efi_loader: device_path: handle special case of loading
efi_loader: export root node handle
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_boot_efi() from do_bootefi()
efi_loader: rework bootmgr/bootefi using load_image API
cmd: add efibootmgr command
cmd/Kconfig | 8 +
cmd/bootefi.c | 515 ++++++++++++++++++++-----------
include/efi_loader.h | 7 +-
lib/efi_loader/efi_bootmgr.c | 43 +--
lib/efi_loader/efi_boottime.c | 2 +
lib/efi_loader/efi_device_path.c | 8 +
lib/efi_loader/efi_root_node.c | 13 +-
7 files changed, 381 insertions(+), 215 deletions(-)
--
2.20.1
^ permalink raw reply [flat|nested] 32+ messages in thread
* [U-Boot] [RFC v3 01/10] efi_loader: device_path: handle special case of loading
2019-04-16 4:24 [U-Boot] [RFC v3 00/10] efi_loader: rework bootefi/bootmgr AKASHI Takahiro
@ 2019-04-16 4:24 ` AKASHI Takahiro
2019-04-16 4:44 ` Heinrich Schuchardt
2019-04-16 4:24 ` [U-Boot] [RFC v3 02/10] efi_loader: export root node handle AKASHI Takahiro
` (8 subsequent siblings)
9 siblings, 1 reply; 32+ messages in thread
From: AKASHI Takahiro @ 2019-04-16 4:24 UTC (permalink / raw)
To: u-boot
This is a preparatory patch.
efi_dp_split_file_path() is used to create device_path and file_path
from file_path for efi_setup_loaded_image().
In a special case, however, of HARDWARE_DEVICE/MEMORY, it doesn't
work expectedly since this path doesn't contain any FILE_PATH sub-type.
This patch makes a workaround.
Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
lib/efi_loader/efi_device_path.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
index 53b40c8c3c2d..e283fad767ed 100644
--- a/lib/efi_loader/efi_device_path.c
+++ b/lib/efi_loader/efi_device_path.c
@@ -933,6 +933,14 @@ efi_status_t efi_dp_split_file_path(struct efi_device_path *full_path,
dp = efi_dp_dup(full_path);
if (!dp)
return EFI_OUT_OF_RESOURCES;
+
+ if (EFI_DP_TYPE(dp, HARDWARE_DEVICE, MEMORY)) {
+ /* no FILE_PATH */
+ *device_path = dp;
+
+ return EFI_SUCCESS;
+ }
+
p = dp;
while (!EFI_DP_TYPE(p, MEDIA_DEVICE, FILE_PATH)) {
p = efi_dp_next(p);
--
2.20.1
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [U-Boot] [RFC v3 02/10] efi_loader: export root node handle
2019-04-16 4:24 [U-Boot] [RFC v3 00/10] efi_loader: rework bootefi/bootmgr AKASHI Takahiro
2019-04-16 4:24 ` [U-Boot] [RFC v3 01/10] efi_loader: device_path: handle special case of loading AKASHI Takahiro
@ 2019-04-16 4:24 ` AKASHI Takahiro
2019-04-16 4:48 ` Heinrich Schuchardt
2019-04-16 4:24 ` [U-Boot] [RFC v3 03/10] cmd: bootefi: carve out fdt handling from do_bootefi() AKASHI Takahiro
` (7 subsequent siblings)
9 siblings, 1 reply; 32+ messages in thread
From: AKASHI Takahiro @ 2019-04-16 4:24 UTC (permalink / raw)
To: u-boot
This is a preparatory patch.
The root node handle will be used as a dummy parent handle when invoking
an EFI image from bootefi/bootmgr command.
Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
include/efi_loader.h | 2 ++
lib/efi_loader/efi_root_node.c | 13 +++++++------
2 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h
index 00b81c6010ff..d524dc7e24c1 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -25,6 +25,8 @@
EFI_GUID(0xe61d73b9, 0xa384, 0x4acc, \
0xae, 0xab, 0x82, 0xe8, 0x28, 0xf3, 0x62, 0x8b)
+extern efi_handle_t efi_root;
+
int __efi_entry_check(void);
int __efi_exit_check(void);
const char *__efi_nesting(void);
diff --git a/lib/efi_loader/efi_root_node.c b/lib/efi_loader/efi_root_node.c
index b056ba3ee880..f2521a64091c 100644
--- a/lib/efi_loader/efi_root_node.c
+++ b/lib/efi_loader/efi_root_node.c
@@ -10,6 +10,7 @@
#include <efi_loader.h>
const efi_guid_t efi_u_boot_guid = U_BOOT_GUID;
+efi_handle_t efi_root;
struct efi_root_dp {
struct efi_device_path_vendor vendor;
@@ -26,12 +27,11 @@ struct efi_root_dp {
*/
efi_status_t efi_root_node_register(void)
{
- efi_handle_t root;
efi_status_t ret;
struct efi_root_dp *dp;
/* Create handle */
- ret = efi_create_handle(&root);
+ ret = efi_create_handle(&efi_root);
if (ret != EFI_SUCCESS)
return ret;
@@ -52,24 +52,25 @@ efi_status_t efi_root_node_register(void)
dp->end.length = sizeof(struct efi_device_path);
/* Install device path protocol */
- ret = efi_add_protocol(root, &efi_guid_device_path, dp);
+ ret = efi_add_protocol(efi_root, &efi_guid_device_path, dp);
if (ret != EFI_SUCCESS)
goto failure;
/* Install device path to text protocol */
- ret = efi_add_protocol(root, &efi_guid_device_path_to_text_protocol,
+ ret = efi_add_protocol(efi_root, &efi_guid_device_path_to_text_protocol,
(void *)&efi_device_path_to_text);
if (ret != EFI_SUCCESS)
goto failure;
/* Install device path utilities protocol */
- ret = efi_add_protocol(root, &efi_guid_device_path_utilities_protocol,
+ ret = efi_add_protocol(efi_root,
+ &efi_guid_device_path_utilities_protocol,
(void *)&efi_device_path_utilities);
if (ret != EFI_SUCCESS)
goto failure;
/* Install Unicode collation protocol */
- ret = efi_add_protocol(root, &efi_guid_unicode_collation_protocol,
+ ret = efi_add_protocol(efi_root, &efi_guid_unicode_collation_protocol,
(void *)&efi_unicode_collation_protocol);
if (ret != EFI_SUCCESS)
goto failure;
--
2.20.1
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [U-Boot] [RFC v3 03/10] cmd: bootefi: carve out fdt handling from do_bootefi()
2019-04-16 4:24 [U-Boot] [RFC v3 00/10] efi_loader: rework bootefi/bootmgr AKASHI Takahiro
2019-04-16 4:24 ` [U-Boot] [RFC v3 01/10] efi_loader: device_path: handle special case of loading AKASHI Takahiro
2019-04-16 4:24 ` [U-Boot] [RFC v3 02/10] efi_loader: export root node handle AKASHI Takahiro
@ 2019-04-16 4:24 ` AKASHI Takahiro
2019-04-16 4:54 ` Heinrich Schuchardt
2019-04-16 4:24 ` [U-Boot] [RFC v3 04/10] cmd: bootefi: merge efi_install_fdt() and efi_process_fdt() AKASHI Takahiro
` (6 subsequent siblings)
9 siblings, 1 reply; 32+ messages in thread
From: AKASHI Takahiro @ 2019-04-16 4:24 UTC (permalink / raw)
To: u-boot
This is a preparatory patch for reworking do_bootefi() in later patch.
Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
cmd/bootefi.c | 53 ++++++++++++++++++++++++++++++++++++---------------
1 file changed, 38 insertions(+), 15 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 3619a20e6433..8cd9644115eb 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -198,6 +198,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,21 +439,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 = fdt_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;
--
2.20.1
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [U-Boot] [RFC v3 04/10] cmd: bootefi: merge efi_install_fdt() and efi_process_fdt()
2019-04-16 4:24 [U-Boot] [RFC v3 00/10] efi_loader: rework bootefi/bootmgr AKASHI Takahiro
` (2 preceding siblings ...)
2019-04-16 4:24 ` [U-Boot] [RFC v3 03/10] cmd: bootefi: carve out fdt handling from do_bootefi() AKASHI Takahiro
@ 2019-04-16 4:24 ` AKASHI Takahiro
2019-04-16 5:05 ` Heinrich Schuchardt
2019-04-17 10:53 ` Heinrich Schuchardt
2019-04-16 4:24 ` [U-Boot] [RFC v3 05/10] cmd: bootefi: carve out efi_selftest code from do_bootefi() AKASHI Takahiro
` (5 subsequent siblings)
9 siblings, 2 replies; 32+ messages in thread
From: AKASHI Takahiro @ 2019-04-16 4:24 UTC (permalink / raw)
To: u-boot
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 8cd9644115eb..0f58f51cbc76 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -165,62 +165,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 */
@@ -439,7 +428,7 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
if (argc < 2)
return CMD_RET_USAGE;
- r = fdt_process_fdt(argc > 2 ? argv[2] : NULL);
+ r = fdt_install_fdt(argc > 2 ? argv[2] : NULL);
if (r == EFI_INVALID_PARAMETER)
return CMD_RET_USAGE;
else if (r != EFI_SUCCESS)
--
2.20.1
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [U-Boot] [RFC v3 05/10] cmd: bootefi: carve out efi_selftest code from do_bootefi()
2019-04-16 4:24 [U-Boot] [RFC v3 00/10] efi_loader: rework bootefi/bootmgr AKASHI Takahiro
` (3 preceding siblings ...)
2019-04-16 4:24 ` [U-Boot] [RFC v3 04/10] cmd: bootefi: merge efi_install_fdt() and efi_process_fdt() AKASHI Takahiro
@ 2019-04-16 4:24 ` AKASHI Takahiro
2019-04-16 5:21 ` Heinrich Schuchardt
2019-04-16 4:24 ` [U-Boot] [RFC v3 06/10] cmd: bootefi: move do_bootefi_bootmgr_exec() forward AKASHI Takahiro
` (4 subsequent siblings)
9 siblings, 1 reply; 32+ messages in thread
From: AKASHI Takahiro @ 2019-04-16 4:24 UTC (permalink / raw)
To: u-boot
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 | 147 +++++++++++++++++++++++++++++++-------------------
1 file changed, 92 insertions(+), 55 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 0f58f51cbc76..a5dba6645ca2 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -219,39 +219,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 */
- set_load_options(*loaded_image_infop, load_options_path);
-
- return 0;
-}
-
-/**
- * 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
*
@@ -298,11 +265,14 @@ 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 */
+ set_load_options(loaded_image_info, "bootargs");
+
/* Load the EFI payload */
ret = efi_load_pe(image_obj, efi, loaded_image_info);
if (ret != EFI_SUCCESS)
@@ -326,7 +296,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)
@@ -336,6 +308,25 @@ 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 */
+ set_load_options(*loaded_image_infop, load_options_path);
+
+ return 0;
+}
+
/**
* bootefi_test_prepare() - prepare to run an EFI test
*
@@ -381,6 +372,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)
@@ -412,6 +461,13 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
efi_status_t r;
unsigned long fdt_addr;
+ 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();
@@ -425,9 +481,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 = fdt_install_fdt(argc > 2 ? argv[2] : NULL);
if (r == EFI_INVALID_PARAMETER)
return CMD_RET_USAGE;
@@ -445,22 +498,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();
--
2.20.1
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [U-Boot] [RFC v3 06/10] cmd: bootefi: move do_bootefi_bootmgr_exec() forward
2019-04-16 4:24 [U-Boot] [RFC v3 00/10] efi_loader: rework bootefi/bootmgr AKASHI Takahiro
` (4 preceding siblings ...)
2019-04-16 4:24 ` [U-Boot] [RFC v3 05/10] cmd: bootefi: carve out efi_selftest code from do_bootefi() AKASHI Takahiro
@ 2019-04-16 4:24 ` AKASHI Takahiro
2019-04-16 5:22 ` Heinrich Schuchardt
2019-04-16 4:24 ` [U-Boot] [RFC v3 07/10] cmd: bootefi: carve out bootmgr code from do_bootefi() AKASHI Takahiro
` (3 subsequent siblings)
9 siblings, 1 reply; 32+ messages in thread
From: AKASHI Takahiro @ 2019-04-16 4:24 UTC (permalink / raw)
To: u-boot
This is a preparatory patch for reworking do_bootefi() in later patch.
Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
cmd/bootefi.c | 42 +++++++++++++++++++++---------------------
1 file changed, 21 insertions(+), 21 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index a5dba6645ca2..10fe10cb4daf 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -307,6 +307,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,
@@ -432,27 +453,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[])
{
--
2.20.1
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [U-Boot] [RFC v3 07/10] cmd: bootefi: carve out bootmgr code from do_bootefi()
2019-04-16 4:24 [U-Boot] [RFC v3 00/10] efi_loader: rework bootefi/bootmgr AKASHI Takahiro
` (5 preceding siblings ...)
2019-04-16 4:24 ` [U-Boot] [RFC v3 06/10] cmd: bootefi: move do_bootefi_bootmgr_exec() forward AKASHI Takahiro
@ 2019-04-16 4:24 ` AKASHI Takahiro
2019-04-16 4:24 ` [U-Boot] [RFC v3 08/10] cmd: bootefi: carve out do_boot_efi() " AKASHI Takahiro
` (2 subsequent siblings)
9 siblings, 0 replies; 32+ messages in thread
From: AKASHI Takahiro @ 2019-04-16 4:24 UTC (permalink / raw)
To: u-boot
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 10fe10cb4daf..7cc6f0ee5ac8 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -307,22 +307,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;
@@ -463,6 +490,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);
@@ -499,9 +529,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);
--
2.20.1
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [U-Boot] [RFC v3 08/10] cmd: bootefi: carve out do_boot_efi() from do_bootefi()
2019-04-16 4:24 [U-Boot] [RFC v3 00/10] efi_loader: rework bootefi/bootmgr AKASHI Takahiro
` (6 preceding siblings ...)
2019-04-16 4:24 ` [U-Boot] [RFC v3 07/10] cmd: bootefi: carve out bootmgr code from do_bootefi() AKASHI Takahiro
@ 2019-04-16 4:24 ` AKASHI Takahiro
2019-04-16 5:31 ` Heinrich Schuchardt
2019-04-16 4:24 ` [U-Boot] [RFC v3 09/10] efi_loader: rework bootmgr/bootefi using load_image API AKASHI Takahiro
2019-04-16 4:24 ` [U-Boot] [RFC v3 10/10] cmd: add efibootmgr command AKASHI Takahiro
9 siblings, 1 reply; 32+ messages in thread
From: AKASHI Takahiro @ 2019-04-16 4:24 UTC (permalink / raw)
To: u-boot
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_boot_efi().
Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
cmd/bootefi.c | 126 ++++++++++++++++++++++++++++----------------------
1 file changed, 70 insertions(+), 56 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 7cc6f0ee5ac8..f14966961b23 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -355,6 +355,75 @@ static int do_efibootmgr(const char *fdt_opt)
return 0;
}
+/*
+ * do_boot_efi() - 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_boot_efi(const char *image_opt, const char *fdt_opt)
+{
+ unsigned long addr;
+ char *saddr;
+ efi_status_t ret;
+ unsigned long fdt_addr;
+
+ /* 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(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);
+ 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,
@@ -483,11 +552,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;
- unsigned long fdt_addr;
-
if (argc < 2)
return CMD_RET_USAGE;
@@ -498,57 +562,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 = fdt_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_boot_efi(argv[1], argc > 2 ? argv[2] : NULL);
}
#ifdef CONFIG_SYS_LONGHELP
--
2.20.1
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [U-Boot] [RFC v3 09/10] efi_loader: rework bootmgr/bootefi using load_image API
2019-04-16 4:24 [U-Boot] [RFC v3 00/10] efi_loader: rework bootefi/bootmgr AKASHI Takahiro
` (7 preceding siblings ...)
2019-04-16 4:24 ` [U-Boot] [RFC v3 08/10] cmd: bootefi: carve out do_boot_efi() " AKASHI Takahiro
@ 2019-04-16 4:24 ` AKASHI Takahiro
2019-04-16 10:56 ` Heinrich Schuchardt
2019-04-16 4:24 ` [U-Boot] [RFC v3 10/10] cmd: add efibootmgr command AKASHI Takahiro
9 siblings, 1 reply; 32+ messages in thread
From: AKASHI Takahiro @ 2019-04-16 4:24 UTC (permalink / raw)
To: u-boot
In the current implementation, bootefi command and EFI boot manager
don't use load_image API, instead, use more primitive and internal
functions. This will introduce duplicated code and potentially
unknown bugs as well as inconsistent behaviours.
With this patch, do_efibootmgr() and do_boot_efi() are completely
overhauled and re-implemented using load_image API.
Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
cmd/bootefi.c | 197 +++++++++++++++++++---------------
include/efi_loader.h | 5 +-
lib/efi_loader/efi_bootmgr.c | 43 ++++----
lib/efi_loader/efi_boottime.c | 2 +
4 files changed, 134 insertions(+), 113 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index f14966961b23..97d49a53a44b 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -222,88 +222,39 @@ static efi_status_t efi_install_fdt(const char *fdt_opt)
/**
* do_bootefi_exec() - execute EFI binary
*
- * @efi: address of the binary
- * @device_path: path of the device from which the binary was loaded
- * @image_path: device path of the binary
+ * @handle: handle of loaded image
* Return: status code
*
* Load the EFI binary into a newly assigned memory unwinding the relocation
* information, install the loaded image protocol, and call the binary.
*/
-static efi_status_t do_bootefi_exec(void *efi,
- struct efi_device_path *device_path,
- struct efi_device_path *image_path)
+static efi_status_t do_bootefi_exec(efi_handle_t handle)
{
- efi_handle_t mem_handle = NULL;
- struct efi_device_path *memdp = NULL;
- efi_status_t ret;
- struct efi_loaded_image_obj *image_obj = NULL;
struct efi_loaded_image *loaded_image_info = NULL;
-
- /*
- * Special case for efi payload not loaded from disk, such as
- * 'bootefi hello' or for example payload loaded directly into
- * memory via JTAG, etc:
- */
- if (!device_path && !image_path) {
- printf("WARNING: using memory device/image path, this may confuse some payloads!\n");
- /* actual addresses filled in after efi_load_pe() */
- memdp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, 0, 0);
- device_path = image_path = memdp;
- /*
- * Grub expects that the device path of the loaded image is
- * installed on a handle.
- */
- ret = efi_create_handle(&mem_handle);
- if (ret != EFI_SUCCESS)
- return ret; /* TODO: leaks device_path */
- ret = efi_add_protocol(mem_handle, &efi_guid_device_path,
- device_path);
- if (ret != EFI_SUCCESS)
- goto err_add_protocol;
- } else {
- assert(device_path && image_path);
- }
-
- ret = efi_setup_loaded_image(device_path, image_path, &image_obj,
- &loaded_image_info);
- if (ret)
- goto err_prepare;
+ efi_status_t ret;
/* Transfer environment variable as load options */
- set_load_options(loaded_image_info, "bootargs");
-
- /* Load the EFI payload */
- ret = efi_load_pe(image_obj, efi, loaded_image_info);
+ ret = EFI_CALL(systab.boottime->open_protocol(
+ handle,
+ &efi_guid_loaded_image,
+ (void **)&loaded_image_info,
+ NULL, NULL,
+ EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL));
if (ret != EFI_SUCCESS)
- goto err_prepare;
-
- if (memdp) {
- struct efi_device_path_memory *mdp = (void *)memdp;
- mdp->memory_type = loaded_image_info->image_code_type;
- mdp->start_address = (uintptr_t)loaded_image_info->image_base;
- mdp->end_address = mdp->start_address +
- loaded_image_info->image_size;
- }
+ goto err;
+ set_load_options(loaded_image_info, "bootargs");
/* we don't support much: */
env_set("efi_8be4df61-93ca-11d2-aa0d-00e098032b8c_OsIndicationsSupported",
"{ro,boot}(blob)0000000000000000");
/* Call our payload! */
- debug("%s: Jumping to 0x%p\n", __func__, image_obj->entry);
- ret = EFI_CALL(efi_start_image(&image_obj->header, NULL, NULL));
+ ret = EFI_CALL(efi_start_image(handle, NULL, NULL));
-err_prepare:
- /* image has returned, loaded-image obj goes *poof*: */
efi_restore_gd();
free(loaded_image_info->load_options);
- efi_delete_handle(&image_obj->header);
-
-err_add_protocol:
- if (mem_handle)
- efi_delete_handle(mem_handle);
+err:
return ret;
}
@@ -317,8 +268,7 @@ err_add_protocol:
*/
static int do_efibootmgr(const char *fdt_opt)
{
- struct efi_device_path *device_path, *file_path;
- void *addr;
+ efi_handle_t handle;
efi_status_t ret;
/* Allow unaligned memory access */
@@ -340,19 +290,19 @@ static int do_efibootmgr(const char *fdt_opt)
else if (ret != EFI_SUCCESS)
return CMD_RET_FAILURE;
- addr = efi_bootmgr_load(&device_path, &file_path);
- if (!addr)
- return 1;
+ ret = efi_bootmgr_load(&handle);
+ if (ret != EFI_SUCCESS) {
+ printf("EFI boot manager: Cannot load any image\n");
+ return CMD_RET_FAILURE;
+ }
- printf("## Starting EFI application at %p ...\n", addr);
- ret = do_bootefi_exec(addr, device_path, file_path);
- printf("## Application terminated, r = %lu\n",
- ret & ~EFI_ERROR_MASK);
+ ret = do_bootefi_exec(handle);
+ printf("## Application terminated, r = %lu\n", ret & ~EFI_ERROR_MASK);
if (ret != EFI_SUCCESS)
- return 1;
+ return CMD_RET_FAILURE;
- return 0;
+ return CMD_RET_SUCCESS;
}
/*
@@ -367,10 +317,14 @@ static int do_efibootmgr(const char *fdt_opt)
*/
static int do_boot_efi(const char *image_opt, const char *fdt_opt)
{
- unsigned long addr;
- char *saddr;
+ void *image_buf;
+ struct efi_device_path *device_path, *image_path;
+ struct efi_device_path *memdp = NULL, *file_path = NULL;
+ const char *saddr;
+ unsigned long addr, size;
+ const char *size_str;
+ efi_handle_t mem_handle = NULL, handle;
efi_status_t ret;
- unsigned long fdt_addr;
/* Allow unaligned memory access */
allow_unaligned();
@@ -392,36 +346,94 @@ static int do_boot_efi(const char *image_opt, const char *fdt_opt)
return CMD_RET_FAILURE;
#ifdef CONFIG_CMD_BOOTEFI_HELLO
- if (!strcmp(argv[1], "hello")) {
- ulong size = __efi_helloworld_end - __efi_helloworld_begin;
-
+ if (!strcmp(image_opt, "hello")) {
saddr = env_get("loadaddr");
+ size = __efi_helloworld_end - __efi_helloworld_begin;
+
if (saddr)
addr = simple_strtoul(saddr, NULL, 16);
else
addr = CONFIG_SYS_LOAD_ADDR;
- memcpy(map_sysmem(addr, size), __efi_helloworld_begin, size);
+
+ image_buf = map_sysmem(addr, size);
+ memcpy(image_buf, __efi_helloworld_begin, size);
+
+ device_path = NULL;
+ image_path = NULL;
} else
#endif
{
- saddr = argv[1];
+ saddr = image_opt;
+ size_str = env_get("filesize");
+ if (size_str)
+ size = simple_strtoul(size_str, NULL, 16);
+ else
+ size = 0;
addr = simple_strtoul(saddr, NULL, 16);
/* Check that a numeric value was passed */
if (!addr && *saddr != '0')
return CMD_RET_USAGE;
+
+ image_buf = map_sysmem(addr, size);
+
+ device_path = bootefi_device_path;
+ image_path = bootefi_image_path;
}
- printf("## Starting EFI application at %08lx ...\n", addr);
- ret = do_bootefi_exec(map_sysmem(addr, 0), bootefi_device_path,
- bootefi_image_path);
- printf("## Application terminated, r = %lu\n",
- ret & ~EFI_ERROR_MASK);
+ if (!device_path && !image_path) {
+ /*
+ * Special case for efi payload not loaded from disk,
+ * such as 'bootefi hello' or for example payload
+ * loaded directly into memory via JTAG, etc:
+ */
+ printf("WARNING: using memory device/image path, this may confuse some payloads!\n");
+
+ /* actual addresses filled in after efi_load_image() */
+ memdp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
+ (uint64_t)image_buf, size);
+ file_path = memdp; /* append(memdp, NULL) */
+
+ /*
+ * Make sure that device for device_path exist
+ * in load_image(). Otherwise, shell and grub will fail.
+ */
+ ret = efi_create_handle(&mem_handle);
+ if (ret != EFI_SUCCESS)
+ /* TODO: leaks device_path */
+ goto err_add_protocol;
+
+ ret = efi_add_protocol(mem_handle, &efi_guid_device_path,
+ memdp);
+ if (ret != EFI_SUCCESS)
+ goto err_add_protocol;
+ } else {
+ assert(device_path && image_path);
+ file_path = efi_dp_append(device_path, image_path);
+ }
+
+ ret = EFI_CALL(efi_load_image(false, efi_root,
+ file_path, image_buf, size, &handle));
+ if (ret != EFI_SUCCESS)
+ goto err_prepare;
+
+ ret = do_bootefi_exec(handle);
+ printf("## Application terminated, r = %lu\n", ret & ~EFI_ERROR_MASK);
+
+err_prepare:
+ if (device_path)
+ efi_free_pool(file_path);
+
+err_add_protocol:
+ if (mem_handle)
+ efi_delete_handle(mem_handle);
+ if (memdp)
+ efi_free_pool(memdp);
if (ret != EFI_SUCCESS)
return CMD_RET_FAILURE;
- else
- return CMD_RET_SUCCESS;
+
+ return CMD_RET_SUCCESS;
}
#ifdef CONFIG_CMD_BOOTEFI_SELFTEST
@@ -581,7 +593,7 @@ static char bootefi_help_text[] =
" Use environment variable efi_selftest to select a single test.\n"
" Use 'setenv efi_selftest list' to enumerate all tests.\n"
#endif
- "bootefi bootmgr [fdt addr]\n"
+ "bootefi bootmgr [fdt address]\n"
" - load and boot EFI payload based on BootOrder/BootXXXX variables.\n"
"\n"
" If specified, the device tree located at <fdt address> gets\n"
@@ -606,6 +618,13 @@ void efi_set_bootdev(const char *dev, const char *devnr, const char *path)
ret = efi_dp_from_name(dev, devnr, path, &device, &image);
if (ret == EFI_SUCCESS) {
bootefi_device_path = device;
+ if (image) {
+ /* FIXME: image should not contain device */
+ struct efi_device_path *image_tmp = image;
+
+ efi_dp_split_file_path(image, &device, &image);
+ efi_free_pool(image_tmp);
+ }
bootefi_image_path = image;
} else {
bootefi_device_path = NULL;
diff --git a/include/efi_loader.h b/include/efi_loader.h
index d524dc7e24c1..c3eda2bb05d7 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -408,8 +408,6 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path,
struct efi_device_path *file_path,
struct efi_loaded_image_obj **handle_ptr,
struct efi_loaded_image **info_ptr);
-efi_status_t efi_load_image_from_path(struct efi_device_path *file_path,
- void **buffer, efi_uintn_t *size);
/* Print information about all loaded images */
void efi_print_image_infos(void *pc);
@@ -563,8 +561,7 @@ struct efi_load_option {
void efi_deserialize_load_option(struct efi_load_option *lo, u8 *data);
unsigned long efi_serialize_load_option(struct efi_load_option *lo, u8 **data);
-void *efi_bootmgr_load(struct efi_device_path **device_path,
- struct efi_device_path **file_path);
+efi_status_t efi_bootmgr_load(efi_handle_t *handle);
#else /* CONFIG_IS_ENABLED(EFI_LOADER) */
diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
index 4fccadc5483d..13ec79b2098b 100644
--- a/lib/efi_loader/efi_bootmgr.c
+++ b/lib/efi_loader/efi_bootmgr.c
@@ -120,14 +120,14 @@ static void *get_var(u16 *name, const efi_guid_t *vendor,
* if successful. This checks that the EFI_LOAD_OPTION is active (enabled)
* and that the specified file to boot exists.
*/
-static void *try_load_entry(uint16_t n, struct efi_device_path **device_path,
- struct efi_device_path **file_path)
+static efi_status_t try_load_entry(u16 n, efi_handle_t *handle)
{
struct efi_load_option lo;
u16 varname[] = L"Boot0000";
u16 hexmap[] = L"0123456789ABCDEF";
- void *load_option, *image = NULL;
+ void *load_option;
efi_uintn_t size;
+ efi_status_t ret;
varname[4] = hexmap[(n & 0xf000) >> 12];
varname[5] = hexmap[(n & 0x0f00) >> 8];
@@ -136,19 +136,19 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path,
load_option = get_var(varname, &efi_global_variable_guid, &size);
if (!load_option)
- return NULL;
+ return EFI_LOAD_ERROR;
efi_deserialize_load_option(&lo, load_option);
if (lo.attributes & LOAD_OPTION_ACTIVE) {
u32 attributes;
- efi_status_t ret;
debug("%s: trying to load \"%ls\" from %pD\n",
__func__, lo.label, lo.file_path);
- ret = efi_load_image_from_path(lo.file_path, &image, &size);
-
+ ret = EFI_CALL(efi_load_image(false, (void *)0x1 /* FIXME */,
+ lo.file_path, NULL, 0,
+ handle));
if (ret != EFI_SUCCESS)
goto error;
@@ -159,17 +159,22 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path,
L"BootCurrent",
(efi_guid_t *)&efi_global_variable_guid,
attributes, size, &n));
- if (ret != EFI_SUCCESS)
+ if (ret != EFI_SUCCESS) {
+ if (EFI_CALL(efi_unload_image(*handle))
+ != EFI_SUCCESS)
+ printf("Unloading image failed\n");
goto error;
+ }
printf("Booting: %ls\n", lo.label);
- efi_dp_split_file_path(lo.file_path, device_path, file_path);
+ } else {
+ ret = EFI_LOAD_ERROR;
}
error:
free(load_option);
- return image;
+ return ret;
}
/*
@@ -177,12 +182,10 @@ error:
* EFI variable, the available load-options, finding and returning
* the first one that can be loaded successfully.
*/
-void *efi_bootmgr_load(struct efi_device_path **device_path,
- struct efi_device_path **file_path)
+efi_status_t efi_bootmgr_load(efi_handle_t *handle)
{
u16 bootnext, *bootorder;
efi_uintn_t size;
- void *image = NULL;
int i, num;
efi_status_t ret;
@@ -209,10 +212,9 @@ void *efi_bootmgr_load(struct efi_device_path **device_path,
/* load BootNext */
if (ret == EFI_SUCCESS) {
if (size == sizeof(u16)) {
- image = try_load_entry(bootnext, device_path,
- file_path);
- if (image)
- return image;
+ ret = try_load_entry(bootnext, handle);
+ if (ret == EFI_SUCCESS)
+ return ret;
}
} else {
printf("Deleting BootNext failed\n");
@@ -223,19 +225,20 @@ void *efi_bootmgr_load(struct efi_device_path **device_path,
bootorder = get_var(L"BootOrder", &efi_global_variable_guid, &size);
if (!bootorder) {
printf("BootOrder not defined\n");
+ ret = EFI_NOT_FOUND;
goto error;
}
num = size / sizeof(uint16_t);
for (i = 0; i < num; i++) {
debug("%s: trying to load Boot%04X\n", __func__, bootorder[i]);
- image = try_load_entry(bootorder[i], device_path, file_path);
- if (image)
+ ret = try_load_entry(bootorder[i], handle);
+ if (ret == EFI_SUCCESS)
break;
}
free(bootorder);
error:
- return image;
+ return ret;
}
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index b215bd7723da..65425fabc08f 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -1611,6 +1611,7 @@ failure:
* @size: size of the loaded image
* Return: status code
*/
+static
efi_status_t efi_load_image_from_path(struct efi_device_path *file_path,
void **buffer, efi_uintn_t *size)
{
@@ -2684,6 +2685,7 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
}
current_image = image_handle;
+ debug("EFI: Jumping into 0x%p\n", image_obj->entry);
ret = EFI_CALL(image_obj->entry(image_handle, &systab));
/*
--
2.20.1
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [U-Boot] [RFC v3 10/10] cmd: add efibootmgr command
2019-04-16 4:24 [U-Boot] [RFC v3 00/10] efi_loader: rework bootefi/bootmgr AKASHI Takahiro
` (8 preceding siblings ...)
2019-04-16 4:24 ` [U-Boot] [RFC v3 09/10] efi_loader: rework bootmgr/bootefi using load_image API AKASHI Takahiro
@ 2019-04-16 4:24 ` AKASHI Takahiro
2019-04-16 5:27 ` Heinrich Schuchardt
9 siblings, 1 reply; 32+ messages in thread
From: AKASHI Takahiro @ 2019-04-16 4:24 UTC (permalink / raw)
To: u-boot
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 0b07b3b9d777..6c9a9f821e54 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 97d49a53a44b..1afa86256670 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -631,3 +631,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 */
--
2.20.1
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [U-Boot] [RFC v3 01/10] efi_loader: device_path: handle special case of loading
2019-04-16 4:24 ` [U-Boot] [RFC v3 01/10] efi_loader: device_path: handle special case of loading AKASHI Takahiro
@ 2019-04-16 4:44 ` Heinrich Schuchardt
0 siblings, 0 replies; 32+ messages in thread
From: Heinrich Schuchardt @ 2019-04-16 4:44 UTC (permalink / raw)
To: u-boot
On 4/16/19 6:24 AM, AKASHI Takahiro wrote:
> This is a preparatory patch.
>
> efi_dp_split_file_path() is used to create device_path and file_path
> from file_path for efi_setup_loaded_image().
> In a special case, however, of HARDWARE_DEVICE/MEMORY, it doesn't
> work expectedly since this path doesn't contain any FILE_PATH sub-type.
>
> This patch makes a workaround.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
> lib/efi_loader/efi_device_path.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
> index 53b40c8c3c2d..e283fad767ed 100644
> --- a/lib/efi_loader/efi_device_path.c
> +++ b/lib/efi_loader/efi_device_path.c
> @@ -933,6 +933,14 @@ efi_status_t efi_dp_split_file_path(struct efi_device_path *full_path,
> dp = efi_dp_dup(full_path);
> if (!dp)
> return EFI_OUT_OF_RESOURCES;
> +
> + if (EFI_DP_TYPE(dp, HARDWARE_DEVICE, MEMORY)) {
> + /* no FILE_PATH */
> + *device_path = dp;
> +
> + return EFI_SUCCESS;
> + }
> +
> p = dp;
> while (!EFI_DP_TYPE(p, MEDIA_DEVICE, FILE_PATH)) {
> p = efi_dp_next(p);
>
Should we really give device paths *starting* with a memory node a
special treatment?
Or should we simply handle all cases where the device path does not end
on a file path the same, e.g.:
diff --git a/lib/efi_loader/efi_device_path.c
b/lib/efi_loader/efi_device_path.c
index d8c052d6ec..d0fd154f54 100644
--- a/lib/efi_loader/efi_device_path.c
+++ b/lib/efi_loader/efi_device_path.c
@@ -924,7 +924,7 @@ efi_status_t efi_dp_split_file_path(struct
efi_device_path *full_path,
struct efi_device_path **device_path,
struct efi_device_path **file_path)
{
- struct efi_device_path *p, *dp, *fp;
+ struct efi_device_path *p, *dp, *fp = NULL;
*device_path = NULL;
*file_path = NULL;
@@ -935,7 +935,7 @@ efi_status_t efi_dp_split_file_path(struct
efi_device_path *full_path,
while (!EFI_DP_TYPE(p, MEDIA_DEVICE, FILE_PATH)) {
p = efi_dp_next(p);
if (!p)
- return EFI_INVALID_PARAMETER;
+ goto out;
}
fp = efi_dp_dup(p);
if (!fp)
@@ -944,6 +944,7 @@ efi_status_t efi_dp_split_file_path(struct
efi_device_path *full_path,
p->sub_type = DEVICE_PATH_SUB_TYPE_END;
p->length = sizeof(*p);
+out:
*device_path = dp;
*file_path = fp;
return EFI_SUCCESS;
I would prefer to go for the more generic version.
Best regards
Heinrich
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [U-Boot] [RFC v3 02/10] efi_loader: export root node handle
2019-04-16 4:24 ` [U-Boot] [RFC v3 02/10] efi_loader: export root node handle AKASHI Takahiro
@ 2019-04-16 4:48 ` Heinrich Schuchardt
2019-04-17 6:57 ` AKASHI Takahiro
0 siblings, 1 reply; 32+ messages in thread
From: Heinrich Schuchardt @ 2019-04-16 4:48 UTC (permalink / raw)
To: u-boot
On 4/16/19 6:24 AM, AKASHI Takahiro wrote:
> This is a preparatory patch.
> The root node handle will be used as a dummy parent handle when invoking
> an EFI image from bootefi/bootmgr command.
This patch is not based on the efi-2019-07 branch.
Please, rebase your patch series.
Best regards
Heinrich
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
> include/efi_loader.h | 2 ++
> lib/efi_loader/efi_root_node.c | 13 +++++++------
> 2 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 00b81c6010ff..d524dc7e24c1 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -25,6 +25,8 @@
> EFI_GUID(0xe61d73b9, 0xa384, 0x4acc, \
> 0xae, 0xab, 0x82, 0xe8, 0x28, 0xf3, 0x62, 0x8b)
>
> +extern efi_handle_t efi_root;
> +
> int __efi_entry_check(void);
> int __efi_exit_check(void);
> const char *__efi_nesting(void);
> diff --git a/lib/efi_loader/efi_root_node.c b/lib/efi_loader/efi_root_node.c
> index b056ba3ee880..f2521a64091c 100644
> --- a/lib/efi_loader/efi_root_node.c
> +++ b/lib/efi_loader/efi_root_node.c
> @@ -10,6 +10,7 @@
> #include <efi_loader.h>
>
> const efi_guid_t efi_u_boot_guid = U_BOOT_GUID;
> +efi_handle_t efi_root;
>
> struct efi_root_dp {
> struct efi_device_path_vendor vendor;
> @@ -26,12 +27,11 @@ struct efi_root_dp {
> */
> efi_status_t efi_root_node_register(void)
> {
> - efi_handle_t root;
> efi_status_t ret;
> struct efi_root_dp *dp;
>
> /* Create handle */
> - ret = efi_create_handle(&root);
> + ret = efi_create_handle(&efi_root);
> if (ret != EFI_SUCCESS)
> return ret;
>
> @@ -52,24 +52,25 @@ efi_status_t efi_root_node_register(void)
> dp->end.length = sizeof(struct efi_device_path);
>
> /* Install device path protocol */
> - ret = efi_add_protocol(root, &efi_guid_device_path, dp);
> + ret = efi_add_protocol(efi_root, &efi_guid_device_path, dp);
> if (ret != EFI_SUCCESS)
> goto failure;
>
> /* Install device path to text protocol */
> - ret = efi_add_protocol(root, &efi_guid_device_path_to_text_protocol,
> + ret = efi_add_protocol(efi_root, &efi_guid_device_path_to_text_protocol,
> (void *)&efi_device_path_to_text);
> if (ret != EFI_SUCCESS)
> goto failure;
>
> /* Install device path utilities protocol */
> - ret = efi_add_protocol(root, &efi_guid_device_path_utilities_protocol,
> + ret = efi_add_protocol(efi_root,
> + &efi_guid_device_path_utilities_protocol,
> (void *)&efi_device_path_utilities);
> if (ret != EFI_SUCCESS)
> goto failure;
>
> /* Install Unicode collation protocol */
> - ret = efi_add_protocol(root, &efi_guid_unicode_collation_protocol,
> + ret = efi_add_protocol(efi_root, &efi_guid_unicode_collation_protocol,
> (void *)&efi_unicode_collation_protocol);
> if (ret != EFI_SUCCESS)
> goto failure;
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [U-Boot] [RFC v3 03/10] cmd: bootefi: carve out fdt handling from do_bootefi()
2019-04-16 4:24 ` [U-Boot] [RFC v3 03/10] cmd: bootefi: carve out fdt handling from do_bootefi() AKASHI Takahiro
@ 2019-04-16 4:54 ` Heinrich Schuchardt
2019-04-17 7:01 ` AKASHI Takahiro
0 siblings, 1 reply; 32+ messages in thread
From: Heinrich Schuchardt @ 2019-04-16 4:54 UTC (permalink / raw)
To: u-boot
On 4/16/19 6:24 AM, AKASHI Takahiro wrote:
> This is a preparatory patch for reworking do_bootefi() in later patch.
I would prefer a more informative commit message like:
Carve out a function to handle the installation of the device tree as a
configuration table.
Otherwise
Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
> cmd/bootefi.c | 53 ++++++++++++++++++++++++++++++++++++---------------
> 1 file changed, 38 insertions(+), 15 deletions(-)
>
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 3619a20e6433..8cd9644115eb 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -198,6 +198,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,21 +439,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 = fdt_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;
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [U-Boot] [RFC v3 04/10] cmd: bootefi: merge efi_install_fdt() and efi_process_fdt()
2019-04-16 4:24 ` [U-Boot] [RFC v3 04/10] cmd: bootefi: merge efi_install_fdt() and efi_process_fdt() AKASHI Takahiro
@ 2019-04-16 5:05 ` Heinrich Schuchardt
2019-04-17 10:53 ` Heinrich Schuchardt
1 sibling, 0 replies; 32+ messages in thread
From: Heinrich Schuchardt @ 2019-04-16 5:05 UTC (permalink / raw)
To: u-boot
On 4/16/19 6:24 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>
Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> cmd/bootefi.c | 67 +++++++++++++++++++++------------------------------
> 1 file changed, 28 insertions(+), 39 deletions(-)
>
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 8cd9644115eb..0f58f51cbc76 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -165,62 +165,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 */
> @@ -439,7 +428,7 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> if (argc < 2)
> return CMD_RET_USAGE;
>
> - r = fdt_process_fdt(argc > 2 ? argv[2] : NULL);
> + r = fdt_install_fdt(argc > 2 ? argv[2] : NULL);
> if (r == EFI_INVALID_PARAMETER)
> return CMD_RET_USAGE;
> else if (r != EFI_SUCCESS)
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [U-Boot] [RFC v3 05/10] cmd: bootefi: carve out efi_selftest code from do_bootefi()
2019-04-16 4:24 ` [U-Boot] [RFC v3 05/10] cmd: bootefi: carve out efi_selftest code from do_bootefi() AKASHI Takahiro
@ 2019-04-16 5:21 ` Heinrich Schuchardt
2019-04-17 7:19 ` AKASHI Takahiro
0 siblings, 1 reply; 32+ messages in thread
From: Heinrich Schuchardt @ 2019-04-16 5:21 UTC (permalink / raw)
To: u-boot
On 4/16/19 6:24 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 Takahiro <takahiro.akashi@linaro.org>
> ---
> cmd/bootefi.c | 147 +++++++++++++++++++++++++++++++-------------------
> 1 file changed, 92 insertions(+), 55 deletions(-)
>
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 0f58f51cbc76..a5dba6645ca2 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -219,39 +219,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 */
> - set_load_options(*loaded_image_infop, load_options_path);
> -
> - return 0;
> -}
> -
> -/**
> - * 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
> *
> @@ -298,11 +265,14 @@ 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 */
> + set_load_options(loaded_image_info, "bootargs");
> +
> /* Load the EFI payload */
> ret = efi_load_pe(image_obj, efi, loaded_image_info);
> if (ret != EFI_SUCCESS)
> @@ -326,7 +296,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)
> @@ -336,6 +308,25 @@ 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 */
> + set_load_options(*loaded_image_infop, load_options_path);
> +
> + return 0;
> +}
> +
> /**
> * bootefi_test_prepare() - prepare to run an EFI test
> *
> @@ -381,6 +372,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;
> + }
allow_unaligned(), switch_to_non_secure_mode(), and efi_init_obj_list()
are needed for any invocation do_bootefi(). Putting them here makes only
sense if you want to create a completely separate command for the selftest.
> +
> + 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)
> @@ -412,6 +461,13 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> efi_status_t r;
> unsigned long fdt_addr;
>
> + 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);
doc/README.commands describes the default way to handle sub-commands.
I think that is where we should move in the long run. Maybe not in this
patch series.
Best regards
Heinrich
> +#endif
> +
> /* Allow unaligned memory access */
> allow_unaligned();
>
> @@ -425,9 +481,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 = fdt_install_fdt(argc > 2 ? argv[2] : NULL);
> if (r == EFI_INVALID_PARAMETER)
> return CMD_RET_USAGE;
> @@ -445,22 +498,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();
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [U-Boot] [RFC v3 06/10] cmd: bootefi: move do_bootefi_bootmgr_exec() forward
2019-04-16 4:24 ` [U-Boot] [RFC v3 06/10] cmd: bootefi: move do_bootefi_bootmgr_exec() forward AKASHI Takahiro
@ 2019-04-16 5:22 ` Heinrich Schuchardt
0 siblings, 0 replies; 32+ messages in thread
From: Heinrich Schuchardt @ 2019-04-16 5:22 UTC (permalink / raw)
To: u-boot
On 4/16/19 6:24 AM, AKASHI Takahiro wrote:
> 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 a5dba6645ca2..10fe10cb4daf 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -307,6 +307,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,
> @@ -432,27 +453,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[])
> {
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [U-Boot] [RFC v3 10/10] cmd: add efibootmgr command
2019-04-16 4:24 ` [U-Boot] [RFC v3 10/10] cmd: add efibootmgr command AKASHI Takahiro
@ 2019-04-16 5:27 ` Heinrich Schuchardt
2019-04-17 8:07 ` AKASHI Takahiro
0 siblings, 1 reply; 32+ messages in thread
From: Heinrich Schuchardt @ 2019-04-16 5:27 UTC (permalink / raw)
To: u-boot
On 4/16/19 6:24 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>
> ---
> cmd/Kconfig | 8 ++++++++
> cmd/bootefi.c | 35 +++++++++++++++++++++++++++++++++++
> 2 files changed, 43 insertions(+)
>
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index 0b07b3b9d777..6c9a9f821e54 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 97d49a53a44b..1afa86256670 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -631,3 +631,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;
Why would you not allow to pass a device tree address?
I think we do not need to different ways to invoke the boot manager.
We should either have either `bootefi bootmgr` or `efibootmgr`.
Best regards
Heinrich
> +
> + 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 */
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [U-Boot] [RFC v3 08/10] cmd: bootefi: carve out do_boot_efi() from do_bootefi()
2019-04-16 4:24 ` [U-Boot] [RFC v3 08/10] cmd: bootefi: carve out do_boot_efi() " AKASHI Takahiro
@ 2019-04-16 5:31 ` Heinrich Schuchardt
2019-04-17 7:43 ` AKASHI Takahiro
0 siblings, 1 reply; 32+ messages in thread
From: Heinrich Schuchardt @ 2019-04-16 5:31 UTC (permalink / raw)
To: u-boot
On 4/16/19 6:24 AM, AKASHI Takahiro wrote:
> 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_boot_efi().
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
> cmd/bootefi.c | 126 ++++++++++++++++++++++++++++----------------------
> 1 file changed, 70 insertions(+), 56 deletions(-)
>
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 7cc6f0ee5ac8..f14966961b23 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -355,6 +355,75 @@ static int do_efibootmgr(const char *fdt_opt)
> return 0;
> }
>
> +/*
> + * do_boot_efi() - execute EFI binary from command line
Having a function do_boot_efi() and a function do_bootefi() is a bit
confusing. Please, use something different, like efi_do_boot().
Otherwise
Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> + *
> + * @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_boot_efi(const char *image_opt, const char *fdt_opt)
> +{
> + unsigned long addr;
> + char *saddr;
> + efi_status_t ret;
> + unsigned long fdt_addr;
> +
> + /* 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(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);
> + 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,
> @@ -483,11 +552,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;
> - unsigned long fdt_addr;
> -
> if (argc < 2)
> return CMD_RET_USAGE;
>
> @@ -498,57 +562,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 = fdt_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_boot_efi(argv[1], argc > 2 ? argv[2] : NULL);
> }
>
> #ifdef CONFIG_SYS_LONGHELP
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [U-Boot] [RFC v3 09/10] efi_loader: rework bootmgr/bootefi using load_image API
2019-04-16 4:24 ` [U-Boot] [RFC v3 09/10] efi_loader: rework bootmgr/bootefi using load_image API AKASHI Takahiro
@ 2019-04-16 10:56 ` Heinrich Schuchardt
2019-04-16 16:20 ` Heinrich Schuchardt
2019-04-18 0:13 ` AKASHI Takahiro
0 siblings, 2 replies; 32+ messages in thread
From: Heinrich Schuchardt @ 2019-04-16 10:56 UTC (permalink / raw)
To: u-boot
On 4/16/19 6:24 AM, AKASHI Takahiro wrote:
> In the current implementation, bootefi command and EFI boot manager
> don't use load_image API, instead, use more primitive and internal
> functions. This will introduce duplicated code and potentially
> unknown bugs as well as inconsistent behaviours.
>
> With this patch, do_efibootmgr() and do_boot_efi() are completely
> overhauled and re-implemented using load_image API.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
> cmd/bootefi.c | 197 +++++++++++++++++++---------------
> include/efi_loader.h | 5 +-
> lib/efi_loader/efi_bootmgr.c | 43 ++++----
> lib/efi_loader/efi_boottime.c | 2 +
> 4 files changed, 134 insertions(+), 113 deletions(-)
>
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index f14966961b23..97d49a53a44b 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -222,88 +222,39 @@ static efi_status_t efi_install_fdt(const char *fdt_opt)
> /**
> * do_bootefi_exec() - execute EFI binary
> *
> - * @efi: address of the binary
> - * @device_path: path of the device from which the binary was loaded
> - * @image_path: device path of the binary
> + * @handle: handle of loaded image
> * Return: status code
> *
> * Load the EFI binary into a newly assigned memory unwinding the relocation
> * information, install the loaded image protocol, and call the binary.
> */
> -static efi_status_t do_bootefi_exec(void *efi,
> - struct efi_device_path *device_path,
> - struct efi_device_path *image_path)
> +static efi_status_t do_bootefi_exec(efi_handle_t handle)
> {
> - efi_handle_t mem_handle = NULL;
> - struct efi_device_path *memdp = NULL;
> - efi_status_t ret;
> - struct efi_loaded_image_obj *image_obj = NULL;
> struct efi_loaded_image *loaded_image_info = NULL;
> -
> - /*
> - * Special case for efi payload not loaded from disk, such as
> - * 'bootefi hello' or for example payload loaded directly into
> - * memory via JTAG, etc:
> - */
> - if (!device_path && !image_path) {
> - printf("WARNING: using memory device/image path, this may confuse some payloads!\n");
> - /* actual addresses filled in after efi_load_pe() */
> - memdp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, 0, 0);
> - device_path = image_path = memdp;
> - /*
> - * Grub expects that the device path of the loaded image is
> - * installed on a handle.
> - */
> - ret = efi_create_handle(&mem_handle);
> - if (ret != EFI_SUCCESS)
> - return ret; /* TODO: leaks device_path */
> - ret = efi_add_protocol(mem_handle, &efi_guid_device_path,
> - device_path);
> - if (ret != EFI_SUCCESS)
> - goto err_add_protocol;
> - } else {
> - assert(device_path && image_path);
> - }
> -
> - ret = efi_setup_loaded_image(device_path, image_path, &image_obj,
> - &loaded_image_info);
> - if (ret)
> - goto err_prepare;
> + efi_status_t ret;
>
> /* Transfer environment variable as load options */
> - set_load_options(loaded_image_info, "bootargs");
In set_load_options() an error could occur (out of memory). So I think
it should return a status.
> -
> - /* Load the EFI payload */
> - ret = efi_load_pe(image_obj, efi, loaded_image_info);
> + ret = EFI_CALL(systab.boottime->open_protocol(
> + handle,
> + &efi_guid_loaded_image,
> + (void **)&loaded_image_info,
> + NULL, NULL,
> + EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL));
Shouldn't we move this to set_load_options()?
> if (ret != EFI_SUCCESS)
> - goto err_prepare;
> -
> - if (memdp) {
> - struct efi_device_path_memory *mdp = (void *)memdp;
> - mdp->memory_type = loaded_image_info->image_code_type;
> - mdp->start_address = (uintptr_t)loaded_image_info->image_base;
> - mdp->end_address = mdp->start_address +
> - loaded_image_info->image_size;
> - }
> + goto err;
> + set_load_options(loaded_image_info, "bootargs");
>
> /* we don't support much: */
> env_set("efi_8be4df61-93ca-11d2-aa0d-00e098032b8c_OsIndicationsSupported",
> "{ro,boot}(blob)0000000000000000");
Shouldn't this move to efi_setup.c? Probably we should also set
OsIndications. I would prefer using efi_set_variable(). I think moving
this could be done in a preparatory patch.
>
> /* Call our payload! */
> - debug("%s: Jumping to 0x%p\n", __func__, image_obj->entry);
> - ret = EFI_CALL(efi_start_image(&image_obj->header, NULL, NULL));
> + ret = EFI_CALL(efi_start_image(handle, NULL, NULL));
>
> -err_prepare:
> - /* image has returned, loaded-image obj goes *poof*: */
> efi_restore_gd();
> free(loaded_image_info->load_options);
> - efi_delete_handle(&image_obj->header);
> -
> -err_add_protocol:
> - if (mem_handle)
> - efi_delete_handle(mem_handle);
>
> +err:
> return ret;
> }
>
> @@ -317,8 +268,7 @@ err_add_protocol:
> */
> static int do_efibootmgr(const char *fdt_opt)
> {
> - struct efi_device_path *device_path, *file_path;
> - void *addr;
> + efi_handle_t handle;
> efi_status_t ret;
>
> /* Allow unaligned memory access */
> @@ -340,19 +290,19 @@ static int do_efibootmgr(const char *fdt_opt)
> else if (ret != EFI_SUCCESS)
> return CMD_RET_FAILURE;
>
> - addr = efi_bootmgr_load(&device_path, &file_path);
> - if (!addr)
> - return 1;
> + ret = efi_bootmgr_load(&handle);
> + if (ret != EFI_SUCCESS) {
> + printf("EFI boot manager: Cannot load any image\n");
> + return CMD_RET_FAILURE;
> + }
>
> - printf("## Starting EFI application at %p ...\n", addr);
> - ret = do_bootefi_exec(addr, device_path, file_path);
> - printf("## Application terminated, r = %lu\n",
> - ret & ~EFI_ERROR_MASK);
> + ret = do_bootefi_exec(handle);
> + printf("## Application terminated, r = %lu\n", ret & ~EFI_ERROR_MASK);
>
> if (ret != EFI_SUCCESS)
> - return 1;
> + return CMD_RET_FAILURE;
>
> - return 0;
> + return CMD_RET_SUCCESS;
> }
>
> /*
> @@ -367,10 +317,14 @@ static int do_efibootmgr(const char *fdt_opt)
> */
> static int do_boot_efi(const char *image_opt, const char *fdt_opt)
> {
> - unsigned long addr;
> - char *saddr;
> + void *image_buf;
> + struct efi_device_path *device_path, *image_path;
> + struct efi_device_path *memdp = NULL, *file_path = NULL;
> + const char *saddr;
> + unsigned long addr, size;
> + const char *size_str;
> + efi_handle_t mem_handle = NULL, handle;
> efi_status_t ret;
> - unsigned long fdt_addr;
>
> /* Allow unaligned memory access */
> allow_unaligned();
> @@ -392,36 +346,94 @@ static int do_boot_efi(const char *image_opt, const char *fdt_opt)
> return CMD_RET_FAILURE;
>
> #ifdef CONFIG_CMD_BOOTEFI_HELLO
> - if (!strcmp(argv[1], "hello")) {
> - ulong size = __efi_helloworld_end - __efi_helloworld_begin;
> -
> + if (!strcmp(image_opt, "hello")) {
> saddr = env_get("loadaddr");
> + size = __efi_helloworld_end - __efi_helloworld_begin;
> +
> if (saddr)
> addr = simple_strtoul(saddr, NULL, 16);
> else
> addr = CONFIG_SYS_LOAD_ADDR;
> - memcpy(map_sysmem(addr, size), __efi_helloworld_begin, size);
> +
> + image_buf = map_sysmem(addr, size);
> + memcpy(image_buf, __efi_helloworld_begin, size);
> +
> + device_path = NULL;
> + image_path = NULL;
> } else
> #endif
> {
> - saddr = argv[1];
> + saddr = image_opt;
> + size_str = env_get("filesize");
> + if (size_str)
> + size = simple_strtoul(size_str, NULL, 16);
> + else
> + size = 0;
>
> addr = simple_strtoul(saddr, NULL, 16);
> /* Check that a numeric value was passed */
> if (!addr && *saddr != '0')
> return CMD_RET_USAGE;
> +
> + image_buf = map_sysmem(addr, size);
> +
> + device_path = bootefi_device_path;
> + image_path = bootefi_image_path;
> }
>
> - printf("## Starting EFI application at %08lx ...\n", addr);
> - ret = do_bootefi_exec(map_sysmem(addr, 0), bootefi_device_path,
> - bootefi_image_path);
> - printf("## Application terminated, r = %lu\n",
> - ret & ~EFI_ERROR_MASK);
> + if (!device_path && !image_path) {
> + /*
> + * Special case for efi payload not loaded from disk,
> + * such as 'bootefi hello' or for example payload
> + * loaded directly into memory via JTAG, etc:
> + */
> + printf("WARNING: using memory device/image path, this may confuse some payloads!\n");
The EFI spec says that either of SourceBuffer or DevicePath may be NULL
when calling LoadImage().
> +
> + /* actual addresses filled in after efi_load_image() */
> + memdp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
> + (uint64_t)image_buf, size);
> + file_path = memdp; /* append(memdp, NULL) */
> +
> + /*
> + * Make sure that device for device_path exist
> + * in load_image(). Otherwise, shell and grub will fail.
GRUB will fail anyway because it cannot determine the disk from which it
was loaded. So why are we doing this?
> + */
> + ret = efi_create_handle(&mem_handle);
> + if (ret != EFI_SUCCESS)
> + /* TODO: leaks device_path */
> + goto err_add_protocol;
> +
> + ret = efi_add_protocol(mem_handle, &efi_guid_device_path,
> + memdp);
Couldn't we as well use the device path of the root node as "file_path"?
> + if (ret != EFI_SUCCESS)
> + goto err_add_protocol;
> + } else {
> + assert(device_path && image_path);
> + file_path = efi_dp_append(device_path, image_path);
Where is file_path freed?
> + }
> +
> + ret = EFI_CALL(efi_load_image(false, efi_root,
> + file_path, image_buf, size, &handle));
> + if (ret != EFI_SUCCESS)
> + goto err_prepare;
> +
> + ret = do_bootefi_exec(handle);
> + printf("## Application terminated, r = %lu\n", ret & ~EFI_ERROR_MASK);
> +
> +err_prepare:
> + if (device_path)
> + efi_free_pool(file_path);
> +
> +err_add_protocol:
> + if (mem_handle)
> + efi_delete_handle(mem_handle);
> + if (memdp)
> + efi_free_pool(memdp);
>
> if (ret != EFI_SUCCESS)
> return CMD_RET_FAILURE;
> - else
> - return CMD_RET_SUCCESS;
> +
> + return CMD_RET_SUCCESS;
> }
>
> #ifdef CONFIG_CMD_BOOTEFI_SELFTEST
> @@ -581,7 +593,7 @@ static char bootefi_help_text[] =
> " Use environment variable efi_selftest to select a single test.\n"
> " Use 'setenv efi_selftest list' to enumerate all tests.\n"
> #endif
> - "bootefi bootmgr [fdt addr]\n"
> + "bootefi bootmgr [fdt address]\n"
> " - load and boot EFI payload based on BootOrder/BootXXXX variables.\n"
> "\n"
> " If specified, the device tree located at <fdt address> gets\n"
> @@ -606,6 +618,13 @@ void efi_set_bootdev(const char *dev, const char *devnr, const char *path)
> ret = efi_dp_from_name(dev, devnr, path, &device, &image);
> if (ret == EFI_SUCCESS) {
> bootefi_device_path = device;
> + if (image) {
> + /* FIXME: image should not contain device */
> + struct efi_device_path *image_tmp = image;
> +
> + efi_dp_split_file_path(image, &device, &image);
> + efi_free_pool(image_tmp);
> + }
> bootefi_image_path = image;
> } else {
> bootefi_device_path = NULL;
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index d524dc7e24c1..c3eda2bb05d7 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -408,8 +408,6 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path,
> struct efi_device_path *file_path,
> struct efi_loaded_image_obj **handle_ptr,
> struct efi_loaded_image **info_ptr);
> -efi_status_t efi_load_image_from_path(struct efi_device_path *file_path,
> - void **buffer, efi_uintn_t *size);
> /* Print information about all loaded images */
> void efi_print_image_infos(void *pc);
>
> @@ -563,8 +561,7 @@ struct efi_load_option {
>
> void efi_deserialize_load_option(struct efi_load_option *lo, u8 *data);
> unsigned long efi_serialize_load_option(struct efi_load_option *lo, u8 **data);
> -void *efi_bootmgr_load(struct efi_device_path **device_path,
> - struct efi_device_path **file_path);
> +efi_status_t efi_bootmgr_load(efi_handle_t *handle);
>
> #else /* CONFIG_IS_ENABLED(EFI_LOADER) */
>
> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> index 4fccadc5483d..13ec79b2098b 100644
> --- a/lib/efi_loader/efi_bootmgr.c
> +++ b/lib/efi_loader/efi_bootmgr.c
> @@ -120,14 +120,14 @@ static void *get_var(u16 *name, const efi_guid_t *vendor,
> * if successful. This checks that the EFI_LOAD_OPTION is active (enabled)
> * and that the specified file to boot exists.
> */
> -static void *try_load_entry(uint16_t n, struct efi_device_path **device_path,
> - struct efi_device_path **file_path)
> +static efi_status_t try_load_entry(u16 n, efi_handle_t *handle)
> {
> struct efi_load_option lo;
> u16 varname[] = L"Boot0000";
> u16 hexmap[] = L"0123456789ABCDEF";
> - void *load_option, *image = NULL;
> + void *load_option;
> efi_uintn_t size;
> + efi_status_t ret;
>
> varname[4] = hexmap[(n & 0xf000) >> 12];
> varname[5] = hexmap[(n & 0x0f00) >> 8];
> @@ -136,19 +136,19 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path,
>
> load_option = get_var(varname, &efi_global_variable_guid, &size);
> if (!load_option)
> - return NULL;
> + return EFI_LOAD_ERROR;
>
> efi_deserialize_load_option(&lo, load_option);
>
> if (lo.attributes & LOAD_OPTION_ACTIVE) {
> u32 attributes;
> - efi_status_t ret;
>
> debug("%s: trying to load \"%ls\" from %pD\n",
> __func__, lo.label, lo.file_path);
>
> - ret = efi_load_image_from_path(lo.file_path, &image, &size);
> -
> + ret = EFI_CALL(efi_load_image(false, (void *)0x1 /* FIXME */,
> + lo.file_path, NULL, 0,
> + handle));
> if (ret != EFI_SUCCESS)
> goto error;
>
> @@ -159,17 +159,22 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path,
> L"BootCurrent",
> (efi_guid_t *)&efi_global_variable_guid,
> attributes, size, &n));
> - if (ret != EFI_SUCCESS)
> + if (ret != EFI_SUCCESS) {
> + if (EFI_CALL(efi_unload_image(*handle))
> + != EFI_SUCCESS)
> + printf("Unloading image failed\n");
> goto error;
> + }
>
> printf("Booting: %ls\n", lo.label);
> - efi_dp_split_file_path(lo.file_path, device_path, file_path);
> + } else {
> + ret = EFI_LOAD_ERROR;
> }
>
> error:
> free(load_option);
>
> - return image;
> + return ret;
> }
>
> /*
> @@ -177,12 +182,10 @@ error:
> * EFI variable, the available load-options, finding and returning
> * the first one that can be loaded successfully.
> */
> -void *efi_bootmgr_load(struct efi_device_path **device_path,
> - struct efi_device_path **file_path)
> +efi_status_t efi_bootmgr_load(efi_handle_t *handle)
> {
> u16 bootnext, *bootorder;
> efi_uintn_t size;
> - void *image = NULL;
> int i, num;
> efi_status_t ret;
>
> @@ -209,10 +212,9 @@ void *efi_bootmgr_load(struct efi_device_path **device_path,
> /* load BootNext */
> if (ret == EFI_SUCCESS) {
> if (size == sizeof(u16)) {
> - image = try_load_entry(bootnext, device_path,
> - file_path);
> - if (image)
> - return image;
> + ret = try_load_entry(bootnext, handle);
> + if (ret == EFI_SUCCESS)
> + return ret;
> }
> } else {
> printf("Deleting BootNext failed\n");
> @@ -223,19 +225,20 @@ void *efi_bootmgr_load(struct efi_device_path **device_path,
> bootorder = get_var(L"BootOrder", &efi_global_variable_guid, &size);
> if (!bootorder) {
> printf("BootOrder not defined\n");
> + ret = EFI_NOT_FOUND;
> goto error;
> }
>
> num = size / sizeof(uint16_t);
> for (i = 0; i < num; i++) {
> debug("%s: trying to load Boot%04X\n", __func__, bootorder[i]);
> - image = try_load_entry(bootorder[i], device_path, file_path);
> - if (image)
> + ret = try_load_entry(bootorder[i], handle);
> + if (ret == EFI_SUCCESS)
> break;
> }
>
> free(bootorder);
>
> error:
> - return image;
> + return ret;
> }
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index b215bd7723da..65425fabc08f 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -1611,6 +1611,7 @@ failure:
> * @size: size of the loaded image
> * Return: status code
> */
> +static
> efi_status_t efi_load_image_from_path(struct efi_device_path *file_path,
> void **buffer, efi_uintn_t *size)
> {
> @@ -2684,6 +2685,7 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
> }
>
> current_image = image_handle;
> + debug("EFI: Jumping into 0x%p\n", image_obj->entry);
Please, use EFI_PRINT() here.
Best regards
Heinrich
> ret = EFI_CALL(image_obj->entry(image_handle, &systab));
>
> /*
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [U-Boot] [RFC v3 09/10] efi_loader: rework bootmgr/bootefi using load_image API
2019-04-16 10:56 ` Heinrich Schuchardt
@ 2019-04-16 16:20 ` Heinrich Schuchardt
2019-04-18 8:29 ` Alexander Graf
2019-04-18 0:13 ` AKASHI Takahiro
1 sibling, 1 reply; 32+ messages in thread
From: Heinrich Schuchardt @ 2019-04-16 16:20 UTC (permalink / raw)
To: u-boot
On 4/16/19 12:56 PM, Heinrich Schuchardt wrote:
> On 4/16/19 6:24 AM, AKASHI Takahiro wrote:
>> In the current implementation, bootefi command and EFI boot manager
>> don't use load_image API, instead, use more primitive and internal
>> functions. This will introduce duplicated code and potentially
>> unknown bugs as well as inconsistent behaviours.
>>
>> With this patch, do_efibootmgr() and do_boot_efi() are completely
>> overhauled and re-implemented using load_image API.
>>
>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>> ---
>> cmd/bootefi.c | 197 +++++++++++++++++++---------------
>> include/efi_loader.h | 5 +-
>> lib/efi_loader/efi_bootmgr.c | 43 ++++----
>> lib/efi_loader/efi_boottime.c | 2 +
>> 4 files changed, 134 insertions(+), 113 deletions(-)
>>
>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>> index f14966961b23..97d49a53a44b 100644
>> --- a/cmd/bootefi.c
>> +++ b/cmd/bootefi.c
>> @@ -222,88 +222,39 @@ static efi_status_t efi_install_fdt(const char
>> *fdt_opt)
>> /**
>> * do_bootefi_exec() - execute EFI binary
>> *
>> - * @efi: address of the binary
>> - * @device_path: path of the device from which the binary was loaded
>> - * @image_path: device path of the binary
>> + * @handle: handle of loaded image
>> * Return: status code
>> *
>> * Load the EFI binary into a newly assigned memory unwinding the
>> relocation
>> * information, install the loaded image protocol, and call the binary.
>> */
>> -static efi_status_t do_bootefi_exec(void *efi,
>> - struct efi_device_path *device_path,
>> - struct efi_device_path *image_path)
>> +static efi_status_t do_bootefi_exec(efi_handle_t handle)
>> {
>> - efi_handle_t mem_handle = NULL;
>> - struct efi_device_path *memdp = NULL;
>> - efi_status_t ret;
>> - struct efi_loaded_image_obj *image_obj = NULL;
>> struct efi_loaded_image *loaded_image_info = NULL;
>> -
>> - /*
>> - * Special case for efi payload not loaded from disk, such as
>> - * 'bootefi hello' or for example payload loaded directly into
>> - * memory via JTAG, etc:
>> - */
>> - if (!device_path && !image_path) {
>> - printf("WARNING: using memory device/image path, this may
>> confuse some payloads!\n");
>> - /* actual addresses filled in after efi_load_pe() */
>> - memdp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, 0, 0);
>> - device_path = image_path = memdp;
>> - /*
>> - * Grub expects that the device path of the loaded image is
>> - * installed on a handle.
>> - */
>> - ret = efi_create_handle(&mem_handle);
>> - if (ret != EFI_SUCCESS)
>> - return ret; /* TODO: leaks device_path */
>> - ret = efi_add_protocol(mem_handle, &efi_guid_device_path,
>> - device_path);
>> - if (ret != EFI_SUCCESS)
>> - goto err_add_protocol;
>> - } else {
>> - assert(device_path && image_path);
>> - }
>> -
>> - ret = efi_setup_loaded_image(device_path, image_path, &image_obj,
>> - &loaded_image_info);
>> - if (ret)
>> - goto err_prepare;
>> + efi_status_t ret;
>>
>> /* Transfer environment variable as load options */
>> - set_load_options(loaded_image_info, "bootargs");
>
> In set_load_options() an error could occur (out of memory). So I think
> it should return a status.
>
>> -
>> - /* Load the EFI payload */
>> - ret = efi_load_pe(image_obj, efi, loaded_image_info);
>> + ret = EFI_CALL(systab.boottime->open_protocol(
>> + handle,
>> + &efi_guid_loaded_image,
>> + (void **)&loaded_image_info,
>> + NULL, NULL,
>> + EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL));
>
> Shouldn't we move this to set_load_options()?
>
>> if (ret != EFI_SUCCESS)
>> - goto err_prepare;
>> -
>> - if (memdp) {
>> - struct efi_device_path_memory *mdp = (void *)memdp;
>> - mdp->memory_type = loaded_image_info->image_code_type;
>> - mdp->start_address = (uintptr_t)loaded_image_info->image_base;
>> - mdp->end_address = mdp->start_address +
>> - loaded_image_info->image_size;
>> - }
>> + goto err;
>> + set_load_options(loaded_image_info, "bootargs");
>>
>> /* we don't support much: */
>>
>> env_set("efi_8be4df61-93ca-11d2-aa0d-00e098032b8c_OsIndicationsSupported",
>>
>> "{ro,boot}(blob)0000000000000000");
>
> Shouldn't this move to efi_setup.c? Probably we should also set
> OsIndications. I would prefer using efi_set_variable(). I think moving
> this could be done in a preparatory patch.
>
>>
>> /* Call our payload! */
>> - debug("%s: Jumping to 0x%p\n", __func__, image_obj->entry);
>> - ret = EFI_CALL(efi_start_image(&image_obj->header, NULL, NULL));
>> + ret = EFI_CALL(efi_start_image(handle, NULL, NULL));
>>
>> -err_prepare:
>> - /* image has returned, loaded-image obj goes *poof*: */
>> efi_restore_gd();
>> free(loaded_image_info->load_options);
>> - efi_delete_handle(&image_obj->header);
>> -
>> -err_add_protocol:
>> - if (mem_handle)
>> - efi_delete_handle(mem_handle);
>>
>> +err:
>> return ret;
>> }
>>
>> @@ -317,8 +268,7 @@ err_add_protocol:
>> */
>> static int do_efibootmgr(const char *fdt_opt)
>> {
>> - struct efi_device_path *device_path, *file_path;
>> - void *addr;
>> + efi_handle_t handle;
>> efi_status_t ret;
>>
>> /* Allow unaligned memory access */
>> @@ -340,19 +290,19 @@ static int do_efibootmgr(const char *fdt_opt)
>> else if (ret != EFI_SUCCESS)
>> return CMD_RET_FAILURE;
>>
>> - addr = efi_bootmgr_load(&device_path, &file_path);
>> - if (!addr)
>> - return 1;
>> + ret = efi_bootmgr_load(&handle);
>> + if (ret != EFI_SUCCESS) {
>> + printf("EFI boot manager: Cannot load any image\n");
>> + return CMD_RET_FAILURE;
>> + }
>>
>> - printf("## Starting EFI application at %p ...\n", addr);
>> - ret = do_bootefi_exec(addr, device_path, file_path);
>> - printf("## Application terminated, r = %lu\n",
>> - ret & ~EFI_ERROR_MASK);
>> + ret = do_bootefi_exec(handle);
>> + printf("## Application terminated, r = %lu\n", ret &
>> ~EFI_ERROR_MASK);
>>
>> if (ret != EFI_SUCCESS)
>> - return 1;
>> + return CMD_RET_FAILURE;
>>
>> - return 0;
>> + return CMD_RET_SUCCESS;
>> }
>>
>> /*
>> @@ -367,10 +317,14 @@ static int do_efibootmgr(const char *fdt_opt)
>> */
>> static int do_boot_efi(const char *image_opt, const char *fdt_opt)
>> {
>> - unsigned long addr;
>> - char *saddr;
>> + void *image_buf;
>> + struct efi_device_path *device_path, *image_path;
>> + struct efi_device_path *memdp = NULL, *file_path = NULL;
>> + const char *saddr;
>> + unsigned long addr, size;
>> + const char *size_str;
>> + efi_handle_t mem_handle = NULL, handle;
>> efi_status_t ret;
>> - unsigned long fdt_addr;
>>
>> /* Allow unaligned memory access */
>> allow_unaligned();
>> @@ -392,36 +346,94 @@ static int do_boot_efi(const char *image_opt,
>> const char *fdt_opt)
>> return CMD_RET_FAILURE;
>>
>> #ifdef CONFIG_CMD_BOOTEFI_HELLO
>> - if (!strcmp(argv[1], "hello")) {
>> - ulong size = __efi_helloworld_end - __efi_helloworld_begin;
>> -
>> + if (!strcmp(image_opt, "hello")) {
>> saddr = env_get("loadaddr");
>> + size = __efi_helloworld_end - __efi_helloworld_begin;
>> +
>> if (saddr)
>> addr = simple_strtoul(saddr, NULL, 16);
>> else
>> addr = CONFIG_SYS_LOAD_ADDR;
>> - memcpy(map_sysmem(addr, size), __efi_helloworld_begin, size);
>> +
>> + image_buf = map_sysmem(addr, size);
>> + memcpy(image_buf, __efi_helloworld_begin, size);
>> +
>> + device_path = NULL;
>> + image_path = NULL;
>> } else
>> #endif
>> {
>> - saddr = argv[1];
>> + saddr = image_opt;
>> + size_str = env_get("filesize");
>> + if (size_str)
>> + size = simple_strtoul(size_str, NULL, 16);
>> + else
>> + size = 0;
>>
>> addr = simple_strtoul(saddr, NULL, 16);
>> /* Check that a numeric value was passed */
>> if (!addr && *saddr != '0')
>> return CMD_RET_USAGE;
>> +
>> + image_buf = map_sysmem(addr, size);
>> +
>> + device_path = bootefi_device_path;
>> + image_path = bootefi_image_path;
>> }
>>
>> - printf("## Starting EFI application at %08lx ...\n", addr);
>> - ret = do_bootefi_exec(map_sysmem(addr, 0), bootefi_device_path,
>> - bootefi_image_path);
>> - printf("## Application terminated, r = %lu\n",
>> - ret & ~EFI_ERROR_MASK);
>> + if (!device_path && !image_path) {
>> + /*
>> + * Special case for efi payload not loaded from disk,
>> + * such as 'bootefi hello' or for example payload
>> + * loaded directly into memory via JTAG, etc:
>> + */
>> + printf("WARNING: using memory device/image path, this may
>> confuse some payloads!\n");
>
> The EFI spec says that either of SourceBuffer or DevicePath may be NULL
> when calling LoadImage().
>
>> +
>> + /* actual addresses filled in after efi_load_image() */
>> + memdp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
>> + (uint64_t)image_buf, size);
>> + file_path = memdp; /* append(memdp, NULL) */
>> +
>> + /*
>> + * Make sure that device for device_path exist
>> + * in load_image(). Otherwise, shell and grub will fail.
>
> GRUB will fail anyway because it cannot determine the disk from which it
> was loaded. So why are we doing this?
In Rob's original commit bf19273e81eb ("efi_loader: Add mem-mapped for
fallback") the comment was:
"This fixes 'bootefi hello' after devicepath refactoring."
EFI Shell.efi starts fine even when we set device_path and image_path to
NULL.
When loading GRUB from disk or via tftp the device path and the image
path are set, e.g. for tftp:
=> dhcp grubarm.efi
=> bootefi 0x40200000 $fdtcontroladdr
Found 0 disks
## Starting EFI application at 40200000 ...
device_path:
/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/MAC(525400123456,0x1)
image_path: /grubarm.efi
error: disk `,msdos2' not found.
Entering rescue mode...
grub rescue>
Maybe GRUB would run if all necessary modules were compiled in to do a
network boot. But how would it find grub.conf?
So my feeling is that we are creating the dummy memory device path because:
- Once `bootefi hello` which is our own test program had a problem.
- GRUB has a bug: it hangs or crashes if the file device path is not
set in LoadImage().
I think the problem should not be fixed in U-Boot but in GRUB.
I am CC-ing Leif due to all the attention he has paid both to U-Boot and
to GRUB.
Best regards
Heinrich
>
>> + */
>> + ret = efi_create_handle(&mem_handle);
>> + if (ret != EFI_SUCCESS)
>> + /* TODO: leaks device_path */
>> + goto err_add_protocol;
>> +
>> + ret = efi_add_protocol(mem_handle, &efi_guid_device_path,
>> + memdp);
>
> Couldn't we as well use the device path of the root node as "file_path"?
>
>> + if (ret != EFI_SUCCESS)
>> + goto err_add_protocol;
>> + } else {
>> + assert(device_path && image_path);
>> + file_path = efi_dp_append(device_path, image_path);
>
> Where is file_path freed?
>
>> + }
>> +
>> + ret = EFI_CALL(efi_load_image(false, efi_root,
>> + file_path, image_buf, size, &handle));
>> + if (ret != EFI_SUCCESS)
>> + goto err_prepare;
>> +
>> + ret = do_bootefi_exec(handle);
>> + printf("## Application terminated, r = %lu\n", ret &
>> ~EFI_ERROR_MASK);
>> +
>> +err_prepare:
>> + if (device_path)
>> + efi_free_pool(file_path);
>> +
>> +err_add_protocol:
>> + if (mem_handle)
>> + efi_delete_handle(mem_handle);
>> + if (memdp)
>> + efi_free_pool(memdp);
>>
>> if (ret != EFI_SUCCESS)
>> return CMD_RET_FAILURE;
>> - else
>> - return CMD_RET_SUCCESS;
>> +
>> + return CMD_RET_SUCCESS;
>> }
>>
>> #ifdef CONFIG_CMD_BOOTEFI_SELFTEST
>> @@ -581,7 +593,7 @@ static char bootefi_help_text[] =
>> " Use environment variable efi_selftest to select a single
>> test.\n"
>> " Use 'setenv efi_selftest list' to enumerate all tests.\n"
>> #endif
>> - "bootefi bootmgr [fdt addr]\n"
>> + "bootefi bootmgr [fdt address]\n"
>> " - load and boot EFI payload based on BootOrder/BootXXXX
>> variables.\n"
>> "\n"
>> " If specified, the device tree located at <fdt address> gets\n"
>> @@ -606,6 +618,13 @@ void efi_set_bootdev(const char *dev, const char
>> *devnr, const char *path)
>> ret = efi_dp_from_name(dev, devnr, path, &device, &image);
>> if (ret == EFI_SUCCESS) {
>> bootefi_device_path = device;
>> + if (image) {
>> + /* FIXME: image should not contain device */
>> + struct efi_device_path *image_tmp = image;
>> +
>> + efi_dp_split_file_path(image, &device, &image);
>> + efi_free_pool(image_tmp);
>> + }
>> bootefi_image_path = image;
>> } else {
>> bootefi_device_path = NULL;
>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>> index d524dc7e24c1..c3eda2bb05d7 100644
>> --- a/include/efi_loader.h
>> +++ b/include/efi_loader.h
>> @@ -408,8 +408,6 @@ efi_status_t efi_setup_loaded_image(struct
>> efi_device_path *device_path,
>> struct efi_device_path *file_path,
>> struct efi_loaded_image_obj **handle_ptr,
>> struct efi_loaded_image **info_ptr);
>> -efi_status_t efi_load_image_from_path(struct efi_device_path *file_path,
>> - void **buffer, efi_uintn_t *size);
>> /* Print information about all loaded images */
>> void efi_print_image_infos(void *pc);
>>
>> @@ -563,8 +561,7 @@ struct efi_load_option {
>>
>> void efi_deserialize_load_option(struct efi_load_option *lo, u8 *data);
>> unsigned long efi_serialize_load_option(struct efi_load_option *lo,
>> u8 **data);
>> -void *efi_bootmgr_load(struct efi_device_path **device_path,
>> - struct efi_device_path **file_path);
>> +efi_status_t efi_bootmgr_load(efi_handle_t *handle);
>>
>> #else /* CONFIG_IS_ENABLED(EFI_LOADER) */
>>
>> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
>> index 4fccadc5483d..13ec79b2098b 100644
>> --- a/lib/efi_loader/efi_bootmgr.c
>> +++ b/lib/efi_loader/efi_bootmgr.c
>> @@ -120,14 +120,14 @@ static void *get_var(u16 *name, const efi_guid_t
>> *vendor,
>> * if successful. This checks that the EFI_LOAD_OPTION is active
>> (enabled)
>> * and that the specified file to boot exists.
>> */
>> -static void *try_load_entry(uint16_t n, struct efi_device_path
>> **device_path,
>> - struct efi_device_path **file_path)
>> +static efi_status_t try_load_entry(u16 n, efi_handle_t *handle)
>> {
>> struct efi_load_option lo;
>> u16 varname[] = L"Boot0000";
>> u16 hexmap[] = L"0123456789ABCDEF";
>> - void *load_option, *image = NULL;
>> + void *load_option;
>> efi_uintn_t size;
>> + efi_status_t ret;
>>
>> varname[4] = hexmap[(n & 0xf000) >> 12];
>> varname[5] = hexmap[(n & 0x0f00) >> 8];
>> @@ -136,19 +136,19 @@ static void *try_load_entry(uint16_t n, struct
>> efi_device_path **device_path,
>>
>> load_option = get_var(varname, &efi_global_variable_guid, &size);
>> if (!load_option)
>> - return NULL;
>> + return EFI_LOAD_ERROR;
>>
>> efi_deserialize_load_option(&lo, load_option);
>>
>> if (lo.attributes & LOAD_OPTION_ACTIVE) {
>> u32 attributes;
>> - efi_status_t ret;
>>
>> debug("%s: trying to load \"%ls\" from %pD\n",
>> __func__, lo.label, lo.file_path);
>>
>> - ret = efi_load_image_from_path(lo.file_path, &image, &size);
>> -
>> + ret = EFI_CALL(efi_load_image(false, (void *)0x1 /* FIXME */,
>> + lo.file_path, NULL, 0,
>> + handle));
>> if (ret != EFI_SUCCESS)
>> goto error;
>>
>> @@ -159,17 +159,22 @@ static void *try_load_entry(uint16_t n, struct
>> efi_device_path **device_path,
>> L"BootCurrent",
>> (efi_guid_t *)&efi_global_variable_guid,
>> attributes, size, &n));
>> - if (ret != EFI_SUCCESS)
>> + if (ret != EFI_SUCCESS) {
>> + if (EFI_CALL(efi_unload_image(*handle))
>> + != EFI_SUCCESS)
>> + printf("Unloading image failed\n");
>> goto error;
>> + }
>>
>> printf("Booting: %ls\n", lo.label);
>> - efi_dp_split_file_path(lo.file_path, device_path, file_path);
>> + } else {
>> + ret = EFI_LOAD_ERROR;
>> }
>>
>> error:
>> free(load_option);
>>
>> - return image;
>> + return ret;
>> }
>>
>> /*
>> @@ -177,12 +182,10 @@ error:
>> * EFI variable, the available load-options, finding and returning
>> * the first one that can be loaded successfully.
>> */
>> -void *efi_bootmgr_load(struct efi_device_path **device_path,
>> - struct efi_device_path **file_path)
>> +efi_status_t efi_bootmgr_load(efi_handle_t *handle)
>> {
>> u16 bootnext, *bootorder;
>> efi_uintn_t size;
>> - void *image = NULL;
>> int i, num;
>> efi_status_t ret;
>>
>> @@ -209,10 +212,9 @@ void *efi_bootmgr_load(struct efi_device_path
>> **device_path,
>> /* load BootNext */
>> if (ret == EFI_SUCCESS) {
>> if (size == sizeof(u16)) {
>> - image = try_load_entry(bootnext, device_path,
>> - file_path);
>> - if (image)
>> - return image;
>> + ret = try_load_entry(bootnext, handle);
>> + if (ret == EFI_SUCCESS)
>> + return ret;
>> }
>> } else {
>> printf("Deleting BootNext failed\n");
>> @@ -223,19 +225,20 @@ void *efi_bootmgr_load(struct efi_device_path
>> **device_path,
>> bootorder = get_var(L"BootOrder", &efi_global_variable_guid,
>> &size);
>> if (!bootorder) {
>> printf("BootOrder not defined\n");
>> + ret = EFI_NOT_FOUND;
>> goto error;
>> }
>>
>> num = size / sizeof(uint16_t);
>> for (i = 0; i < num; i++) {
>> debug("%s: trying to load Boot%04X\n", __func__, bootorder[i]);
>> - image = try_load_entry(bootorder[i], device_path, file_path);
>> - if (image)
>> + ret = try_load_entry(bootorder[i], handle);
>> + if (ret == EFI_SUCCESS)
>> break;
>> }
>>
>> free(bootorder);
>>
>> error:
>> - return image;
>> + return ret;
>> }
>> diff --git a/lib/efi_loader/efi_boottime.c
>> b/lib/efi_loader/efi_boottime.c
>> index b215bd7723da..65425fabc08f 100644
>> --- a/lib/efi_loader/efi_boottime.c
>> +++ b/lib/efi_loader/efi_boottime.c
>> @@ -1611,6 +1611,7 @@ failure:
>> * @size: size of the loaded image
>> * Return: status code
>> */
>> +static
>> efi_status_t efi_load_image_from_path(struct efi_device_path
>> *file_path,
>> void **buffer, efi_uintn_t *size)
>> {
>> @@ -2684,6 +2685,7 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t
>> image_handle,
>> }
>>
>> current_image = image_handle;
>> + debug("EFI: Jumping into 0x%p\n", image_obj->entry);
>
> Please, use EFI_PRINT() here.
>
> Best regards
>
> Heinrich
>
>> ret = EFI_CALL(image_obj->entry(image_handle, &systab));
>>
>> /*
>>
>
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [U-Boot] [RFC v3 02/10] efi_loader: export root node handle
2019-04-16 4:48 ` Heinrich Schuchardt
@ 2019-04-17 6:57 ` AKASHI Takahiro
0 siblings, 0 replies; 32+ messages in thread
From: AKASHI Takahiro @ 2019-04-17 6:57 UTC (permalink / raw)
To: u-boot
On Tue, Apr 16, 2019 at 06:48:46AM +0200, Heinrich Schuchardt wrote:
> On 4/16/19 6:24 AM, AKASHI Takahiro wrote:
> >This is a preparatory patch.
> >The root node handle will be used as a dummy parent handle when invoking
> >an EFI image from bootefi/bootmgr command.
>
> This patch is not based on the efi-2019-07 branch.
This is intentional.
> Please, rebase your patch series.
As efi-2019-07 can be updated frequently and get old quickly (day by day),
in particular before or around rc1 and rc2, I don't want to rebase my patch,
instead look forward to the consensus to my approach first.
That is why I have sent out this series as a RFC.
-Takahiro Akashi
> Best regards
>
> Heinrich
>
> >
> >Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >---
> > include/efi_loader.h | 2 ++
> > lib/efi_loader/efi_root_node.c | 13 +++++++------
> > 2 files changed, 9 insertions(+), 6 deletions(-)
> >
> >diff --git a/include/efi_loader.h b/include/efi_loader.h
> >index 00b81c6010ff..d524dc7e24c1 100644
> >--- a/include/efi_loader.h
> >+++ b/include/efi_loader.h
> >@@ -25,6 +25,8 @@
> > EFI_GUID(0xe61d73b9, 0xa384, 0x4acc, \
> > 0xae, 0xab, 0x82, 0xe8, 0x28, 0xf3, 0x62, 0x8b)
> >
> >+extern efi_handle_t efi_root;
> >+
> > int __efi_entry_check(void);
> > int __efi_exit_check(void);
> > const char *__efi_nesting(void);
> >diff --git a/lib/efi_loader/efi_root_node.c b/lib/efi_loader/efi_root_node.c
> >index b056ba3ee880..f2521a64091c 100644
> >--- a/lib/efi_loader/efi_root_node.c
> >+++ b/lib/efi_loader/efi_root_node.c
> >@@ -10,6 +10,7 @@
> > #include <efi_loader.h>
> >
> > const efi_guid_t efi_u_boot_guid = U_BOOT_GUID;
> >+efi_handle_t efi_root;
> >
> > struct efi_root_dp {
> > struct efi_device_path_vendor vendor;
> >@@ -26,12 +27,11 @@ struct efi_root_dp {
> > */
> > efi_status_t efi_root_node_register(void)
> > {
> >- efi_handle_t root;
> > efi_status_t ret;
> > struct efi_root_dp *dp;
> >
> > /* Create handle */
> >- ret = efi_create_handle(&root);
> >+ ret = efi_create_handle(&efi_root);
> > if (ret != EFI_SUCCESS)
> > return ret;
> >
> >@@ -52,24 +52,25 @@ efi_status_t efi_root_node_register(void)
> > dp->end.length = sizeof(struct efi_device_path);
> >
> > /* Install device path protocol */
> >- ret = efi_add_protocol(root, &efi_guid_device_path, dp);
> >+ ret = efi_add_protocol(efi_root, &efi_guid_device_path, dp);
> > if (ret != EFI_SUCCESS)
> > goto failure;
> >
> > /* Install device path to text protocol */
> >- ret = efi_add_protocol(root, &efi_guid_device_path_to_text_protocol,
> >+ ret = efi_add_protocol(efi_root, &efi_guid_device_path_to_text_protocol,
> > (void *)&efi_device_path_to_text);
> > if (ret != EFI_SUCCESS)
> > goto failure;
> >
> > /* Install device path utilities protocol */
> >- ret = efi_add_protocol(root, &efi_guid_device_path_utilities_protocol,
> >+ ret = efi_add_protocol(efi_root,
> >+ &efi_guid_device_path_utilities_protocol,
> > (void *)&efi_device_path_utilities);
> > if (ret != EFI_SUCCESS)
> > goto failure;
> >
> > /* Install Unicode collation protocol */
> >- ret = efi_add_protocol(root, &efi_guid_unicode_collation_protocol,
> >+ ret = efi_add_protocol(efi_root, &efi_guid_unicode_collation_protocol,
> > (void *)&efi_unicode_collation_protocol);
> > if (ret != EFI_SUCCESS)
> > goto failure;
> >
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [U-Boot] [RFC v3 03/10] cmd: bootefi: carve out fdt handling from do_bootefi()
2019-04-16 4:54 ` Heinrich Schuchardt
@ 2019-04-17 7:01 ` AKASHI Takahiro
2019-04-17 16:35 ` Heinrich Schuchardt
0 siblings, 1 reply; 32+ messages in thread
From: AKASHI Takahiro @ 2019-04-17 7:01 UTC (permalink / raw)
To: u-boot
On Tue, Apr 16, 2019 at 06:54:58AM +0200, Heinrich Schuchardt wrote:
> On 4/16/19 6:24 AM, AKASHI Takahiro wrote:
> >This is a preparatory patch for reworking do_bootefi() in later patch.
>
> I would prefer a more informative commit message like:
>
> Carve out a function to handle the installation of the device tree as a
> configuration table.
Okay, but it doesn't convey more than the subject.
-Takahiro Akashi
> Otherwise
> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>
> >
> >Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >---
> > cmd/bootefi.c | 53 ++++++++++++++++++++++++++++++++++++---------------
> > 1 file changed, 38 insertions(+), 15 deletions(-)
> >
> >diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> >index 3619a20e6433..8cd9644115eb 100644
> >--- a/cmd/bootefi.c
> >+++ b/cmd/bootefi.c
> >@@ -198,6 +198,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,21 +439,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 = fdt_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;
> >
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [U-Boot] [RFC v3 05/10] cmd: bootefi: carve out efi_selftest code from do_bootefi()
2019-04-16 5:21 ` Heinrich Schuchardt
@ 2019-04-17 7:19 ` AKASHI Takahiro
0 siblings, 0 replies; 32+ messages in thread
From: AKASHI Takahiro @ 2019-04-17 7:19 UTC (permalink / raw)
To: u-boot
On Tue, Apr 16, 2019 at 07:21:26AM +0200, Heinrich Schuchardt wrote:
> On 4/16/19 6:24 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 Takahiro <takahiro.akashi@linaro.org>
> >---
> > cmd/bootefi.c | 147 +++++++++++++++++++++++++++++++-------------------
> > 1 file changed, 92 insertions(+), 55 deletions(-)
> >
> >diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> >index 0f58f51cbc76..a5dba6645ca2 100644
> >--- a/cmd/bootefi.c
> >+++ b/cmd/bootefi.c
> >@@ -219,39 +219,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 */
> >- set_load_options(*loaded_image_infop, load_options_path);
> >-
> >- return 0;
> >-}
> >-
> >-/**
> >- * 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
> > *
> >@@ -298,11 +265,14 @@ 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 */
> >+ set_load_options(loaded_image_info, "bootargs");
> >+
> > /* Load the EFI payload */
> > ret = efi_load_pe(image_obj, efi, loaded_image_info);
> > if (ret != EFI_SUCCESS)
> >@@ -326,7 +296,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)
> >@@ -336,6 +308,25 @@ 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 */
> >+ set_load_options(*loaded_image_infop, load_options_path);
> >+
> >+ return 0;
> >+}
> >+
> > /**
> > * bootefi_test_prepare() - prepare to run an EFI test
> > *
> >@@ -381,6 +372,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;
> >+ }
>
> allow_unaligned(), switch_to_non_secure_mode(), and efi_init_obj_list()
> are needed for any invocation do_bootefi(). Putting them here makes only
> sense if you want to create a completely separate command for the selftest.
First of all, this is a preparation for do_efibootmgr().
In addition, as I suggested, we should also convert selftest to
a standalone application.
> >+
> >+ 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)
> >@@ -412,6 +461,13 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> > efi_status_t r;
> > unsigned long fdt_addr;
> >
> >+ 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);
>
> doc/README.commands describes the default way to handle sub-commands.
> I think that is where we should move in the long run. Maybe not in this
> patch series.
Quit easy to follow.
-Takahiro Akashi
> Best regards
>
> Heinrich
>
> >+#endif
> >+
> > /* Allow unaligned memory access */
> > allow_unaligned();
> >
> >@@ -425,9 +481,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 = fdt_install_fdt(argc > 2 ? argv[2] : NULL);
> > if (r == EFI_INVALID_PARAMETER)
> > return CMD_RET_USAGE;
> >@@ -445,22 +498,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();
> >
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [U-Boot] [RFC v3 08/10] cmd: bootefi: carve out do_boot_efi() from do_bootefi()
2019-04-16 5:31 ` Heinrich Schuchardt
@ 2019-04-17 7:43 ` AKASHI Takahiro
0 siblings, 0 replies; 32+ messages in thread
From: AKASHI Takahiro @ 2019-04-17 7:43 UTC (permalink / raw)
To: u-boot
On Tue, Apr 16, 2019 at 07:31:28AM +0200, Heinrich Schuchardt wrote:
> On 4/16/19 6:24 AM, AKASHI Takahiro wrote:
> >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_boot_efi().
> >
> >Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >---
> > cmd/bootefi.c | 126 ++++++++++++++++++++++++++++----------------------
> > 1 file changed, 70 insertions(+), 56 deletions(-)
> >
> >diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> >index 7cc6f0ee5ac8..f14966961b23 100644
> >--- a/cmd/bootefi.c
> >+++ b/cmd/bootefi.c
> >@@ -355,6 +355,75 @@ static int do_efibootmgr(const char *fdt_opt)
> > return 0;
> > }
> >
> >+/*
> >+ * do_boot_efi() - execute EFI binary from command line
>
> Having a function do_boot_efi() and a function do_bootefi() is a bit
> confusing. Please, use something different, like efi_do_boot().
Okay, but I don't like that any function in this file starts with
"efi_".
So do_bootefi_image(). This will reflect more precise functionality.
-Takahiro Akashi
> Otherwise
> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>
> >+ *
> >+ * @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_boot_efi(const char *image_opt, const char *fdt_opt)
> >+{
> >+ unsigned long addr;
> >+ char *saddr;
> >+ efi_status_t ret;
> >+ unsigned long fdt_addr;
> >+
> >+ /* 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(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);
> >+ 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,
> >@@ -483,11 +552,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;
> >- unsigned long fdt_addr;
> >-
> > if (argc < 2)
> > return CMD_RET_USAGE;
> >
> >@@ -498,57 +562,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 = fdt_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_boot_efi(argv[1], argc > 2 ? argv[2] : NULL);
> > }
> >
> > #ifdef CONFIG_SYS_LONGHELP
> >
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [U-Boot] [RFC v3 10/10] cmd: add efibootmgr command
2019-04-16 5:27 ` Heinrich Schuchardt
@ 2019-04-17 8:07 ` AKASHI Takahiro
0 siblings, 0 replies; 32+ messages in thread
From: AKASHI Takahiro @ 2019-04-17 8:07 UTC (permalink / raw)
To: u-boot
On Tue, Apr 16, 2019 at 07:27:10AM +0200, Heinrich Schuchardt wrote:
> On 4/16/19 6:24 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>
> >---
> > cmd/Kconfig | 8 ++++++++
> > cmd/bootefi.c | 35 +++++++++++++++++++++++++++++++++++
> > 2 files changed, 43 insertions(+)
> >
> >diff --git a/cmd/Kconfig b/cmd/Kconfig
> >index 0b07b3b9d777..6c9a9f821e54 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 97d49a53a44b..1afa86256670 100644
> >--- a/cmd/bootefi.c
> >+++ b/cmd/bootefi.c
> >@@ -631,3 +631,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;
>
> Why would you not allow to pass a device tree address?
I think I explained in the past:
The current *secure boot* feature in U-Boot assumes that a single
command without any arguments be invoked. See cli_secure_boot_cmd()
in common/cli.c.
> I think we do not need to different ways to invoke the boot manager.
>
> We should either have either `bootefi bootmgr` or `efibootmgr`.
This patch is not essential in this patch series, so it's up to you
if it's merged or not.
I will re-visit this issue when I submit *secure boot* patch.
Thanks,
-Takahiro Akashi
> Best regards
>
> Heinrich
>
> >+
> >+ 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 */
> >
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [U-Boot] [RFC v3 04/10] cmd: bootefi: merge efi_install_fdt() and efi_process_fdt()
2019-04-16 4:24 ` [U-Boot] [RFC v3 04/10] cmd: bootefi: merge efi_install_fdt() and efi_process_fdt() AKASHI Takahiro
2019-04-16 5:05 ` Heinrich Schuchardt
@ 2019-04-17 10:53 ` Heinrich Schuchardt
1 sibling, 0 replies; 32+ messages in thread
From: Heinrich Schuchardt @ 2019-04-17 10:53 UTC (permalink / raw)
To: u-boot
On 4/16/19 6:24 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 8cd9644115eb..0f58f51cbc76 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -165,62 +165,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 */
> @@ -439,7 +428,7 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> if (argc < 2)
> return CMD_RET_USAGE;
>
> - r = fdt_process_fdt(argc > 2 ? argv[2] : NULL);
> + r = fdt_install_fdt(argc > 2 ? argv[2] : NULL);
cmd/bootefi.c: In function ‘do_bootefi’:
cmd/bootefi.c:431:6: warning: implicit declaration of function
‘fdt_install_fdt’; did you mean ‘efi_install_fdt’?
[-Wimplicit-function-declaration]
r = fdt_install_fdt(argc > 2 ? argv[2] : NULL);
^~~~~~~~~~~~~~~
efi_install_fdt
cmd/bootefi.c:413:16: warning: unused variable ‘fdt_addr’
[-Wunused-variable]
unsigned long fdt_addr;
^~~~~~~~
At top level:
cmd/bootefi.c:176:21: warning: ‘efi_install_fdt’ defined but not used
[-Wunused-function]
static efi_status_t efi_install_fdt(const char *fdt_opt)
Please, check.
Best regards
Heinrich
> if (r == EFI_INVALID_PARAMETER)
> return CMD_RET_USAGE;
> else if (r != EFI_SUCCESS)
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [U-Boot] [RFC v3 03/10] cmd: bootefi: carve out fdt handling from do_bootefi()
2019-04-17 7:01 ` AKASHI Takahiro
@ 2019-04-17 16:35 ` Heinrich Schuchardt
2019-04-18 0:16 ` AKASHI Takahiro
0 siblings, 1 reply; 32+ messages in thread
From: Heinrich Schuchardt @ 2019-04-17 16:35 UTC (permalink / raw)
To: u-boot
On 4/17/19 9:01 AM, AKASHI Takahiro wrote:
> On Tue, Apr 16, 2019 at 06:54:58AM +0200, Heinrich Schuchardt wrote:
>> On 4/16/19 6:24 AM, AKASHI Takahiro wrote:
>>> This is a preparatory patch for reworking do_bootefi() in later patch.
>>
>> I would prefer a more informative commit message like:
>>
>> Carve out a function to handle the installation of the device tree as a
>> configuration table.
>
> Okay, but it doesn't convey more than the subject.
>
> -Takahiro Akashi
>
>> Otherwise
>> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>
>>>
>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>> ---
>>> cmd/bootefi.c | 53 ++++++++++++++++++++++++++++++++++++---------------
>>> 1 file changed, 38 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>>> index 3619a20e6433..8cd9644115eb 100644
>>> --- a/cmd/bootefi.c
>>> +++ b/cmd/bootefi.c
>>> @@ -198,6 +198,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,21 +439,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 = fdt_process_fdt(argc > 2 ? argv[2] : NULL);
cmd/bootefi.c: In function ‘do_bootefi’:
cmd/bootefi.c:442:6: warning: implicit declaration of function
‘fdt_process_fdt’; did you mean ‘efi_process_fdt’?
[-Wimplicit-function-declaration]
r = fdt_process_fdt(argc > 2 ? argv[2] : NULL);
^~~~~~~~~~~~~~~
efi_process_fdt
cmd/bootefi.c:424:16: warning: unused variable ‘fdt_addr’
[-Wunused-variable]
unsigned long fdt_addr;
^~~~~~~~
At top level:
CC drivers/virtio/virtio_pci_legacy.o
cmd/bootefi.c:209:21: warning: ‘efi_process_fdt’ defined but not used
[-Wunused-function]
static efi_status_t efi_process_fdt(const char *fdt_opt)
^~~~~~~~~~~~~~~
Please, check.
Best regards
Heinrich
>>> + 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;
>>>
>>
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [U-Boot] [RFC v3 09/10] efi_loader: rework bootmgr/bootefi using load_image API
2019-04-16 10:56 ` Heinrich Schuchardt
2019-04-16 16:20 ` Heinrich Schuchardt
@ 2019-04-18 0:13 ` AKASHI Takahiro
2019-04-18 3:06 ` AKASHI Takahiro
1 sibling, 1 reply; 32+ messages in thread
From: AKASHI Takahiro @ 2019-04-18 0:13 UTC (permalink / raw)
To: u-boot
On Tue, Apr 16, 2019 at 12:56:32PM +0200, Heinrich Schuchardt wrote:
> On 4/16/19 6:24 AM, AKASHI Takahiro wrote:
> >In the current implementation, bootefi command and EFI boot manager
> >don't use load_image API, instead, use more primitive and internal
> >functions. This will introduce duplicated code and potentially
> >unknown bugs as well as inconsistent behaviours.
> >
> >With this patch, do_efibootmgr() and do_boot_efi() are completely
> >overhauled and re-implemented using load_image API.
> >
> >Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >---
> > cmd/bootefi.c | 197 +++++++++++++++++++---------------
> > include/efi_loader.h | 5 +-
> > lib/efi_loader/efi_bootmgr.c | 43 ++++----
> > lib/efi_loader/efi_boottime.c | 2 +
> > 4 files changed, 134 insertions(+), 113 deletions(-)
> >
> >diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> >index f14966961b23..97d49a53a44b 100644
> >--- a/cmd/bootefi.c
> >+++ b/cmd/bootefi.c
> >@@ -222,88 +222,39 @@ static efi_status_t efi_install_fdt(const char *fdt_opt)
> > /**
> > * do_bootefi_exec() - execute EFI binary
> > *
> >- * @efi: address of the binary
> >- * @device_path: path of the device from which the binary was loaded
> >- * @image_path: device path of the binary
> >+ * @handle: handle of loaded image
> > * Return: status code
> > *
> > * Load the EFI binary into a newly assigned memory unwinding the relocation
> > * information, install the loaded image protocol, and call the binary.
> > */
> >-static efi_status_t do_bootefi_exec(void *efi,
> >- struct efi_device_path *device_path,
> >- struct efi_device_path *image_path)
> >+static efi_status_t do_bootefi_exec(efi_handle_t handle)
> > {
> >- efi_handle_t mem_handle = NULL;
> >- struct efi_device_path *memdp = NULL;
> >- efi_status_t ret;
> >- struct efi_loaded_image_obj *image_obj = NULL;
> > struct efi_loaded_image *loaded_image_info = NULL;
> >-
> >- /*
> >- * Special case for efi payload not loaded from disk, such as
> >- * 'bootefi hello' or for example payload loaded directly into
> >- * memory via JTAG, etc:
> >- */
> >- if (!device_path && !image_path) {
> >- printf("WARNING: using memory device/image path, this may confuse some payloads!\n");
> >- /* actual addresses filled in after efi_load_pe() */
> >- memdp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, 0, 0);
> >- device_path = image_path = memdp;
> >- /*
> >- * Grub expects that the device path of the loaded image is
> >- * installed on a handle.
> >- */
> >- ret = efi_create_handle(&mem_handle);
> >- if (ret != EFI_SUCCESS)
> >- return ret; /* TODO: leaks device_path */
> >- ret = efi_add_protocol(mem_handle, &efi_guid_device_path,
> >- device_path);
> >- if (ret != EFI_SUCCESS)
> >- goto err_add_protocol;
> >- } else {
> >- assert(device_path && image_path);
> >- }
> >-
> >- ret = efi_setup_loaded_image(device_path, image_path, &image_obj,
> >- &loaded_image_info);
> >- if (ret)
> >- goto err_prepare;
> >+ efi_status_t ret;
> >
> > /* Transfer environment variable as load options */
> >- set_load_options(loaded_image_info, "bootargs");
>
> In set_load_options() an error could occur (out of memory). So I think
> it should return a status.
I didn't change anything in set_load_options(), but I will follow
your comment.
> >-
> >- /* Load the EFI payload */
> >- ret = efi_load_pe(image_obj, efi, loaded_image_info);
> >+ ret = EFI_CALL(systab.boottime->open_protocol(
> >+ handle,
> >+ &efi_guid_loaded_image,
> >+ (void **)&loaded_image_info,
> >+ NULL, NULL,
> >+ EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL));
>
> Shouldn't we move this to set_load_options()?
No. This line has nothing to do with "load options."
> > if (ret != EFI_SUCCESS)
> >- goto err_prepare;
> >-
> >- if (memdp) {
> >- struct efi_device_path_memory *mdp = (void *)memdp;
> >- mdp->memory_type = loaded_image_info->image_code_type;
> >- mdp->start_address = (uintptr_t)loaded_image_info->image_base;
> >- mdp->end_address = mdp->start_address +
> >- loaded_image_info->image_size;
> >- }
> >+ goto err;
> >+ set_load_options(loaded_image_info, "bootargs");
> >
> > /* we don't support much: */
> > env_set("efi_8be4df61-93ca-11d2-aa0d-00e098032b8c_OsIndicationsSupported",
> > "{ro,boot}(blob)0000000000000000");
>
> Shouldn't this move to efi_setup.c? Probably we should also set
> OsIndications. I would prefer using efi_set_variable(). I think moving
> this could be done in a preparatory patch.
Yeah, I am aware of this issue.
Will fix in a separate patch.
> >
> > /* Call our payload! */
> >- debug("%s: Jumping to 0x%p\n", __func__, image_obj->entry);
> >- ret = EFI_CALL(efi_start_image(&image_obj->header, NULL, NULL));
> >+ ret = EFI_CALL(efi_start_image(handle, NULL, NULL));
> >
> >-err_prepare:
> >- /* image has returned, loaded-image obj goes *poof*: */
> > efi_restore_gd();
> > free(loaded_image_info->load_options);
> >- efi_delete_handle(&image_obj->header);
> >-
> >-err_add_protocol:
> >- if (mem_handle)
> >- efi_delete_handle(mem_handle);
> >
> >+err:
> > return ret;
> > }
> >
> >@@ -317,8 +268,7 @@ err_add_protocol:
> > */
> > static int do_efibootmgr(const char *fdt_opt)
> > {
> >- struct efi_device_path *device_path, *file_path;
> >- void *addr;
> >+ efi_handle_t handle;
> > efi_status_t ret;
> >
> > /* Allow unaligned memory access */
> >@@ -340,19 +290,19 @@ static int do_efibootmgr(const char *fdt_opt)
> > else if (ret != EFI_SUCCESS)
> > return CMD_RET_FAILURE;
> >
> >- addr = efi_bootmgr_load(&device_path, &file_path);
> >- if (!addr)
> >- return 1;
> >+ ret = efi_bootmgr_load(&handle);
> >+ if (ret != EFI_SUCCESS) {
> >+ printf("EFI boot manager: Cannot load any image\n");
> >+ return CMD_RET_FAILURE;
> >+ }
> >
> >- printf("## Starting EFI application at %p ...\n", addr);
> >- ret = do_bootefi_exec(addr, device_path, file_path);
> >- printf("## Application terminated, r = %lu\n",
> >- ret & ~EFI_ERROR_MASK);
> >+ ret = do_bootefi_exec(handle);
> >+ printf("## Application terminated, r = %lu\n", ret & ~EFI_ERROR_MASK);
> >
> > if (ret != EFI_SUCCESS)
> >- return 1;
> >+ return CMD_RET_FAILURE;
> >
> >- return 0;
> >+ return CMD_RET_SUCCESS;
> > }
> >
> > /*
> >@@ -367,10 +317,14 @@ static int do_efibootmgr(const char *fdt_opt)
> > */
> > static int do_boot_efi(const char *image_opt, const char *fdt_opt)
> > {
> >- unsigned long addr;
> >- char *saddr;
> >+ void *image_buf;
> >+ struct efi_device_path *device_path, *image_path;
> >+ struct efi_device_path *memdp = NULL, *file_path = NULL;
> >+ const char *saddr;
> >+ unsigned long addr, size;
> >+ const char *size_str;
> >+ efi_handle_t mem_handle = NULL, handle;
> > efi_status_t ret;
> >- unsigned long fdt_addr;
> >
> > /* Allow unaligned memory access */
> > allow_unaligned();
> >@@ -392,36 +346,94 @@ static int do_boot_efi(const char *image_opt, const char *fdt_opt)
> > return CMD_RET_FAILURE;
> >
> > #ifdef CONFIG_CMD_BOOTEFI_HELLO
> >- if (!strcmp(argv[1], "hello")) {
> >- ulong size = __efi_helloworld_end - __efi_helloworld_begin;
> >-
> >+ if (!strcmp(image_opt, "hello")) {
> > saddr = env_get("loadaddr");
> >+ size = __efi_helloworld_end - __efi_helloworld_begin;
> >+
> > if (saddr)
> > addr = simple_strtoul(saddr, NULL, 16);
> > else
> > addr = CONFIG_SYS_LOAD_ADDR;
> >- memcpy(map_sysmem(addr, size), __efi_helloworld_begin, size);
> >+
> >+ image_buf = map_sysmem(addr, size);
> >+ memcpy(image_buf, __efi_helloworld_begin, size);
> >+
> >+ device_path = NULL;
> >+ image_path = NULL;
> > } else
> > #endif
> > {
> >- saddr = argv[1];
> >+ saddr = image_opt;
> >+ size_str = env_get("filesize");
> >+ if (size_str)
> >+ size = simple_strtoul(size_str, NULL, 16);
> >+ else
> >+ size = 0;
> >
> > addr = simple_strtoul(saddr, NULL, 16);
> > /* Check that a numeric value was passed */
> > if (!addr && *saddr != '0')
> > return CMD_RET_USAGE;
> >+
> >+ image_buf = map_sysmem(addr, size);
> >+
> >+ device_path = bootefi_device_path;
> >+ image_path = bootefi_image_path;
> > }
> >
> >- printf("## Starting EFI application at %08lx ...\n", addr);
> >- ret = do_bootefi_exec(map_sysmem(addr, 0), bootefi_device_path,
> >- bootefi_image_path);
> >- printf("## Application terminated, r = %lu\n",
> >- ret & ~EFI_ERROR_MASK);
> >+ if (!device_path && !image_path) {
> >+ /*
> >+ * Special case for efi payload not loaded from disk,
> >+ * such as 'bootefi hello' or for example payload
> >+ * loaded directly into memory via JTAG, etc:
> >+ */
> >+ printf("WARNING: using memory device/image path, this may confuse some payloads!\n");
>
> The EFI spec says that either of SourceBuffer or DevicePath may be NULL
> when calling LoadImage().
So are you suggesting we should remove this message?
> >+
> >+ /* actual addresses filled in after efi_load_image() */
> >+ memdp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
> >+ (uint64_t)image_buf, size);
> >+ file_path = memdp; /* append(memdp, NULL) */
> >+
> >+ /*
> >+ * Make sure that device for device_path exist
> >+ * in load_image(). Otherwise, shell and grub will fail.
>
> GRUB will fail anyway because it cannot determine the disk from which it
> was loaded. So why are we doing this?
I will comment on another reply.
> >+ */
> >+ ret = efi_create_handle(&mem_handle);
> >+ if (ret != EFI_SUCCESS)
> >+ /* TODO: leaks device_path */
> >+ goto err_add_protocol;
> >+
> >+ ret = efi_add_protocol(mem_handle, &efi_guid_device_path,
> >+ memdp);
>
> Couldn't we as well use the device path of the root node as "file_path"?
I don't get you point very well, but
it will depend on how we should fix a grub issue mentioned above.
> >+ if (ret != EFI_SUCCESS)
> >+ goto err_add_protocol;
> >+ } else {
> >+ assert(device_path && image_path);
> >+ file_path = efi_dp_append(device_path, image_path);
>
> Where is file_path freed?
In this function. Will fix.
> >+ }
> >+
> >+ ret = EFI_CALL(efi_load_image(false, efi_root,
> >+ file_path, image_buf, size, &handle));
> >+ if (ret != EFI_SUCCESS)
> >+ goto err_prepare;
> >+
> >+ ret = do_bootefi_exec(handle);
> >+ printf("## Application terminated, r = %lu\n", ret & ~EFI_ERROR_MASK);
> >+
> >+err_prepare:
> >+ if (device_path)
> >+ efi_free_pool(file_path);
> >+
> >+err_add_protocol:
> >+ if (mem_handle)
> >+ efi_delete_handle(mem_handle);
> >+ if (memdp)
> >+ efi_free_pool(memdp);
> >
> > if (ret != EFI_SUCCESS)
> > return CMD_RET_FAILURE;
> >- else
> >- return CMD_RET_SUCCESS;
> >+
> >+ return CMD_RET_SUCCESS;
> > }
> >
> > #ifdef CONFIG_CMD_BOOTEFI_SELFTEST
> >@@ -581,7 +593,7 @@ static char bootefi_help_text[] =
> > " Use environment variable efi_selftest to select a single test.\n"
> > " Use 'setenv efi_selftest list' to enumerate all tests.\n"
> > #endif
> >- "bootefi bootmgr [fdt addr]\n"
> >+ "bootefi bootmgr [fdt address]\n"
> > " - load and boot EFI payload based on BootOrder/BootXXXX variables.\n"
> > "\n"
> > " If specified, the device tree located at <fdt address> gets\n"
> >@@ -606,6 +618,13 @@ void efi_set_bootdev(const char *dev, const char *devnr, const char *path)
> > ret = efi_dp_from_name(dev, devnr, path, &device, &image);
> > if (ret == EFI_SUCCESS) {
> > bootefi_device_path = device;
> >+ if (image) {
> >+ /* FIXME: image should not contain device */
> >+ struct efi_device_path *image_tmp = image;
> >+
> >+ efi_dp_split_file_path(image, &device, &image);
> >+ efi_free_pool(image_tmp);
> >+ }
> > bootefi_image_path = image;
> > } else {
> > bootefi_device_path = NULL;
> >diff --git a/include/efi_loader.h b/include/efi_loader.h
> >index d524dc7e24c1..c3eda2bb05d7 100644
> >--- a/include/efi_loader.h
> >+++ b/include/efi_loader.h
> >@@ -408,8 +408,6 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path,
> > struct efi_device_path *file_path,
> > struct efi_loaded_image_obj **handle_ptr,
> > struct efi_loaded_image **info_ptr);
> >-efi_status_t efi_load_image_from_path(struct efi_device_path *file_path,
> >- void **buffer, efi_uintn_t *size);
> > /* Print information about all loaded images */
> > void efi_print_image_infos(void *pc);
> >
> >@@ -563,8 +561,7 @@ struct efi_load_option {
> >
> > void efi_deserialize_load_option(struct efi_load_option *lo, u8 *data);
> > unsigned long efi_serialize_load_option(struct efi_load_option *lo, u8 **data);
> >-void *efi_bootmgr_load(struct efi_device_path **device_path,
> >- struct efi_device_path **file_path);
> >+efi_status_t efi_bootmgr_load(efi_handle_t *handle);
> >
> > #else /* CONFIG_IS_ENABLED(EFI_LOADER) */
> >
> >diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> >index 4fccadc5483d..13ec79b2098b 100644
> >--- a/lib/efi_loader/efi_bootmgr.c
> >+++ b/lib/efi_loader/efi_bootmgr.c
> >@@ -120,14 +120,14 @@ static void *get_var(u16 *name, const efi_guid_t *vendor,
> > * if successful. This checks that the EFI_LOAD_OPTION is active (enabled)
> > * and that the specified file to boot exists.
> > */
> >-static void *try_load_entry(uint16_t n, struct efi_device_path **device_path,
> >- struct efi_device_path **file_path)
> >+static efi_status_t try_load_entry(u16 n, efi_handle_t *handle)
> > {
> > struct efi_load_option lo;
> > u16 varname[] = L"Boot0000";
> > u16 hexmap[] = L"0123456789ABCDEF";
> >- void *load_option, *image = NULL;
> >+ void *load_option;
> > efi_uintn_t size;
> >+ efi_status_t ret;
> >
> > varname[4] = hexmap[(n & 0xf000) >> 12];
> > varname[5] = hexmap[(n & 0x0f00) >> 8];
> >@@ -136,19 +136,19 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path,
> >
> > load_option = get_var(varname, &efi_global_variable_guid, &size);
> > if (!load_option)
> >- return NULL;
> >+ return EFI_LOAD_ERROR;
> >
> > efi_deserialize_load_option(&lo, load_option);
> >
> > if (lo.attributes & LOAD_OPTION_ACTIVE) {
> > u32 attributes;
> >- efi_status_t ret;
> >
> > debug("%s: trying to load \"%ls\" from %pD\n",
> > __func__, lo.label, lo.file_path);
> >
> >- ret = efi_load_image_from_path(lo.file_path, &image, &size);
> >-
> >+ ret = EFI_CALL(efi_load_image(false, (void *)0x1 /* FIXME */,
> >+ lo.file_path, NULL, 0,
> >+ handle));
> > if (ret != EFI_SUCCESS)
> > goto error;
> >
> >@@ -159,17 +159,22 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path,
> > L"BootCurrent",
> > (efi_guid_t *)&efi_global_variable_guid,
> > attributes, size, &n));
> >- if (ret != EFI_SUCCESS)
> >+ if (ret != EFI_SUCCESS) {
> >+ if (EFI_CALL(efi_unload_image(*handle))
> >+ != EFI_SUCCESS)
> >+ printf("Unloading image failed\n");
> > goto error;
> >+ }
> >
> > printf("Booting: %ls\n", lo.label);
> >- efi_dp_split_file_path(lo.file_path, device_path, file_path);
> >+ } else {
> >+ ret = EFI_LOAD_ERROR;
> > }
> >
> > error:
> > free(load_option);
> >
> >- return image;
> >+ return ret;
> > }
> >
> > /*
> >@@ -177,12 +182,10 @@ error:
> > * EFI variable, the available load-options, finding and returning
> > * the first one that can be loaded successfully.
> > */
> >-void *efi_bootmgr_load(struct efi_device_path **device_path,
> >- struct efi_device_path **file_path)
> >+efi_status_t efi_bootmgr_load(efi_handle_t *handle)
> > {
> > u16 bootnext, *bootorder;
> > efi_uintn_t size;
> >- void *image = NULL;
> > int i, num;
> > efi_status_t ret;
> >
> >@@ -209,10 +212,9 @@ void *efi_bootmgr_load(struct efi_device_path **device_path,
> > /* load BootNext */
> > if (ret == EFI_SUCCESS) {
> > if (size == sizeof(u16)) {
> >- image = try_load_entry(bootnext, device_path,
> >- file_path);
> >- if (image)
> >- return image;
> >+ ret = try_load_entry(bootnext, handle);
> >+ if (ret == EFI_SUCCESS)
> >+ return ret;
> > }
> > } else {
> > printf("Deleting BootNext failed\n");
> >@@ -223,19 +225,20 @@ void *efi_bootmgr_load(struct efi_device_path **device_path,
> > bootorder = get_var(L"BootOrder", &efi_global_variable_guid, &size);
> > if (!bootorder) {
> > printf("BootOrder not defined\n");
> >+ ret = EFI_NOT_FOUND;
> > goto error;
> > }
> >
> > num = size / sizeof(uint16_t);
> > for (i = 0; i < num; i++) {
> > debug("%s: trying to load Boot%04X\n", __func__, bootorder[i]);
> >- image = try_load_entry(bootorder[i], device_path, file_path);
> >- if (image)
> >+ ret = try_load_entry(bootorder[i], handle);
> >+ if (ret == EFI_SUCCESS)
> > break;
> > }
> >
> > free(bootorder);
> >
> > error:
> >- return image;
> >+ return ret;
> > }
> >diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> >index b215bd7723da..65425fabc08f 100644
> >--- a/lib/efi_loader/efi_boottime.c
> >+++ b/lib/efi_loader/efi_boottime.c
> >@@ -1611,6 +1611,7 @@ failure:
> > * @size: size of the loaded image
> > * Return: status code
> > */
> >+static
> > efi_status_t efi_load_image_from_path(struct efi_device_path *file_path,
> > void **buffer, efi_uintn_t *size)
> > {
> >@@ -2684,6 +2685,7 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
> > }
> >
> > current_image = image_handle;
> >+ debug("EFI: Jumping into 0x%p\n", image_obj->entry);
>
> Please, use EFI_PRINT() here.
Basically I agree, but there are lots of debug()'s in this file.
We should replace them all at once later.
Thanks,
-Takahiro Akashi
> Best regards
>
> Heinrich
>
> > ret = EFI_CALL(image_obj->entry(image_handle, &systab));
> >
> > /*
> >
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [U-Boot] [RFC v3 03/10] cmd: bootefi: carve out fdt handling from do_bootefi()
2019-04-17 16:35 ` Heinrich Schuchardt
@ 2019-04-18 0:16 ` AKASHI Takahiro
0 siblings, 0 replies; 32+ messages in thread
From: AKASHI Takahiro @ 2019-04-18 0:16 UTC (permalink / raw)
To: u-boot
On Wed, Apr 17, 2019 at 06:35:57PM +0200, Heinrich Schuchardt wrote:
> On 4/17/19 9:01 AM, AKASHI Takahiro wrote:
> >On Tue, Apr 16, 2019 at 06:54:58AM +0200, Heinrich Schuchardt wrote:
> >>On 4/16/19 6:24 AM, AKASHI Takahiro wrote:
> >>>This is a preparatory patch for reworking do_bootefi() in later patch.
> >>
> >>I would prefer a more informative commit message like:
> >>
> >>Carve out a function to handle the installation of the device tree as a
> >>configuration table.
> >
> >Okay, but it doesn't convey more than the subject.
> >
> >-Takahiro Akashi
> >
> >>Otherwise
> >>Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >>
> >>>
> >>>Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >>>---
> >>> cmd/bootefi.c | 53 ++++++++++++++++++++++++++++++++++++---------------
> >>> 1 file changed, 38 insertions(+), 15 deletions(-)
> >>>
> >>>diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> >>>index 3619a20e6433..8cd9644115eb 100644
> >>>--- a/cmd/bootefi.c
> >>>+++ b/cmd/bootefi.c
> >>>@@ -198,6 +198,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,21 +439,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 = fdt_process_fdt(argc > 2 ? argv[2] : NULL);
>
> cmd/bootefi.c: In function ‘do_bootefi’:
> cmd/bootefi.c:442:6: warning: implicit declaration of function
> ‘fdt_process_fdt’; did you mean ‘efi_process_fdt’?
> [-Wimplicit-function-declaration]
> r = fdt_process_fdt(argc > 2 ? argv[2] : NULL);
> ^~~~~~~~~~~~~~~
> efi_process_fdt
Oops. I think that I fixed it before.
Will fix along with patch#4.
> cmd/bootefi.c:424:16: warning: unused variable ‘fdt_addr’
> [-Wunused-variable]
> unsigned long fdt_addr;
> ^~~~~~~~
Okay.
Thanks,
-Takahiro Akashi
> At top level:
> CC drivers/virtio/virtio_pci_legacy.o
> cmd/bootefi.c:209:21: warning: ‘efi_process_fdt’ defined but not used
> [-Wunused-function]
> static efi_status_t efi_process_fdt(const char *fdt_opt)
> ^~~~~~~~~~~~~~~
>
> Please, check.
>
> Best regards
>
> Heinrich
>
> >>>+ 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;
> >>>
> >>
> >
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [U-Boot] [RFC v3 09/10] efi_loader: rework bootmgr/bootefi using load_image API
2019-04-18 0:13 ` AKASHI Takahiro
@ 2019-04-18 3:06 ` AKASHI Takahiro
0 siblings, 0 replies; 32+ messages in thread
From: AKASHI Takahiro @ 2019-04-18 3:06 UTC (permalink / raw)
To: u-boot
Correction:
On Thu, Apr 18, 2019 at 09:13:52AM +0900, AKASHI Takahiro wrote:
> On Tue, Apr 16, 2019 at 12:56:32PM +0200, Heinrich Schuchardt wrote:
> > On 4/16/19 6:24 AM, AKASHI Takahiro wrote:
> > >In the current implementation, bootefi command and EFI boot manager
> > >don't use load_image API, instead, use more primitive and internal
> > >functions. This will introduce duplicated code and potentially
> > >unknown bugs as well as inconsistent behaviours.
> > >
> > >With this patch, do_efibootmgr() and do_boot_efi() are completely
> > >overhauled and re-implemented using load_image API.
> > >
> > >Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > >---
> > > cmd/bootefi.c | 197 +++++++++++++++++++---------------
> > > include/efi_loader.h | 5 +-
> > > lib/efi_loader/efi_bootmgr.c | 43 ++++----
> > > lib/efi_loader/efi_boottime.c | 2 +
> > > 4 files changed, 134 insertions(+), 113 deletions(-)
> > >
> > >diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> > >index f14966961b23..97d49a53a44b 100644
> > >--- a/cmd/bootefi.c
> > >+++ b/cmd/bootefi.c
> > >@@ -222,88 +222,39 @@ static efi_status_t efi_install_fdt(const char *fdt_opt)
> > > /**
> > > * do_bootefi_exec() - execute EFI binary
> > > *
> > >- * @efi: address of the binary
> > >- * @device_path: path of the device from which the binary was loaded
> > >- * @image_path: device path of the binary
> > >+ * @handle: handle of loaded image
> > > * Return: status code
> > > *
> > > * Load the EFI binary into a newly assigned memory unwinding the relocation
> > > * information, install the loaded image protocol, and call the binary.
> > > */
> > >-static efi_status_t do_bootefi_exec(void *efi,
> > >- struct efi_device_path *device_path,
> > >- struct efi_device_path *image_path)
> > >+static efi_status_t do_bootefi_exec(efi_handle_t handle)
> > > {
> > >- efi_handle_t mem_handle = NULL;
> > >- struct efi_device_path *memdp = NULL;
> > >- efi_status_t ret;
> > >- struct efi_loaded_image_obj *image_obj = NULL;
> > > struct efi_loaded_image *loaded_image_info = NULL;
> > >-
> > >- /*
> > >- * Special case for efi payload not loaded from disk, such as
> > >- * 'bootefi hello' or for example payload loaded directly into
> > >- * memory via JTAG, etc:
> > >- */
> > >- if (!device_path && !image_path) {
> > >- printf("WARNING: using memory device/image path, this may confuse some payloads!\n");
> > >- /* actual addresses filled in after efi_load_pe() */
> > >- memdp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, 0, 0);
> > >- device_path = image_path = memdp;
> > >- /*
> > >- * Grub expects that the device path of the loaded image is
> > >- * installed on a handle.
> > >- */
> > >- ret = efi_create_handle(&mem_handle);
> > >- if (ret != EFI_SUCCESS)
> > >- return ret; /* TODO: leaks device_path */
> > >- ret = efi_add_protocol(mem_handle, &efi_guid_device_path,
> > >- device_path);
> > >- if (ret != EFI_SUCCESS)
> > >- goto err_add_protocol;
> > >- } else {
> > >- assert(device_path && image_path);
> > >- }
> > >-
> > >- ret = efi_setup_loaded_image(device_path, image_path, &image_obj,
> > >- &loaded_image_info);
> > >- if (ret)
> > >- goto err_prepare;
> > >+ efi_status_t ret;
> > >
> > > /* Transfer environment variable as load options */
> > >- set_load_options(loaded_image_info, "bootargs");
> >
> > In set_load_options() an error could occur (out of memory). So I think
> > it should return a status.
>
> I didn't change anything in set_load_options(), but I will follow
> your comment.
>
> > >-
> > >- /* Load the EFI payload */
> > >- ret = efi_load_pe(image_obj, efi, loaded_image_info);
> > >+ ret = EFI_CALL(systab.boottime->open_protocol(
> > >+ handle,
> > >+ &efi_guid_loaded_image,
> > >+ (void **)&loaded_image_info,
> > >+ NULL, NULL,
> > >+ EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL));
> >
> > Shouldn't we move this to set_load_options()?
>
> No. This line has nothing to do with "load options."
Wrong, I will follow your comment.
This change, however, uncovers one issue: Who is responsible
for freeing loaded_image_info->load_option, particularly,
when efi_exit()/unload_image() is fully implemented?
In such a case, we have no chance to access this handle,
hence loaded_image_info, after returning from efi_start_image().
Thanks,
-Takahiro Akashi
> > > if (ret != EFI_SUCCESS)
> > >- goto err_prepare;
> > >-
> > >- if (memdp) {
> > >- struct efi_device_path_memory *mdp = (void *)memdp;
> > >- mdp->memory_type = loaded_image_info->image_code_type;
> > >- mdp->start_address = (uintptr_t)loaded_image_info->image_base;
> > >- mdp->end_address = mdp->start_address +
> > >- loaded_image_info->image_size;
> > >- }
> > >+ goto err;
> > >+ set_load_options(loaded_image_info, "bootargs");
> > >
> > > /* we don't support much: */
> > > env_set("efi_8be4df61-93ca-11d2-aa0d-00e098032b8c_OsIndicationsSupported",
> > > "{ro,boot}(blob)0000000000000000");
> >
> > Shouldn't this move to efi_setup.c? Probably we should also set
> > OsIndications. I would prefer using efi_set_variable(). I think moving
> > this could be done in a preparatory patch.
>
> Yeah, I am aware of this issue.
> Will fix in a separate patch.
>
> > >
> > > /* Call our payload! */
> > >- debug("%s: Jumping to 0x%p\n", __func__, image_obj->entry);
> > >- ret = EFI_CALL(efi_start_image(&image_obj->header, NULL, NULL));
> > >+ ret = EFI_CALL(efi_start_image(handle, NULL, NULL));
> > >
> > >-err_prepare:
> > >- /* image has returned, loaded-image obj goes *poof*: */
> > > efi_restore_gd();
> > > free(loaded_image_info->load_options);
> > >- efi_delete_handle(&image_obj->header);
> > >-
> > >-err_add_protocol:
> > >- if (mem_handle)
> > >- efi_delete_handle(mem_handle);
> > >
> > >+err:
> > > return ret;
> > > }
> > >
> > >@@ -317,8 +268,7 @@ err_add_protocol:
> > > */
> > > static int do_efibootmgr(const char *fdt_opt)
> > > {
> > >- struct efi_device_path *device_path, *file_path;
> > >- void *addr;
> > >+ efi_handle_t handle;
> > > efi_status_t ret;
> > >
> > > /* Allow unaligned memory access */
> > >@@ -340,19 +290,19 @@ static int do_efibootmgr(const char *fdt_opt)
> > > else if (ret != EFI_SUCCESS)
> > > return CMD_RET_FAILURE;
> > >
> > >- addr = efi_bootmgr_load(&device_path, &file_path);
> > >- if (!addr)
> > >- return 1;
> > >+ ret = efi_bootmgr_load(&handle);
> > >+ if (ret != EFI_SUCCESS) {
> > >+ printf("EFI boot manager: Cannot load any image\n");
> > >+ return CMD_RET_FAILURE;
> > >+ }
> > >
> > >- printf("## Starting EFI application at %p ...\n", addr);
> > >- ret = do_bootefi_exec(addr, device_path, file_path);
> > >- printf("## Application terminated, r = %lu\n",
> > >- ret & ~EFI_ERROR_MASK);
> > >+ ret = do_bootefi_exec(handle);
> > >+ printf("## Application terminated, r = %lu\n", ret & ~EFI_ERROR_MASK);
> > >
> > > if (ret != EFI_SUCCESS)
> > >- return 1;
> > >+ return CMD_RET_FAILURE;
> > >
> > >- return 0;
> > >+ return CMD_RET_SUCCESS;
> > > }
> > >
> > > /*
> > >@@ -367,10 +317,14 @@ static int do_efibootmgr(const char *fdt_opt)
> > > */
> > > static int do_boot_efi(const char *image_opt, const char *fdt_opt)
> > > {
> > >- unsigned long addr;
> > >- char *saddr;
> > >+ void *image_buf;
> > >+ struct efi_device_path *device_path, *image_path;
> > >+ struct efi_device_path *memdp = NULL, *file_path = NULL;
> > >+ const char *saddr;
> > >+ unsigned long addr, size;
> > >+ const char *size_str;
> > >+ efi_handle_t mem_handle = NULL, handle;
> > > efi_status_t ret;
> > >- unsigned long fdt_addr;
> > >
> > > /* Allow unaligned memory access */
> > > allow_unaligned();
> > >@@ -392,36 +346,94 @@ static int do_boot_efi(const char *image_opt, const char *fdt_opt)
> > > return CMD_RET_FAILURE;
> > >
> > > #ifdef CONFIG_CMD_BOOTEFI_HELLO
> > >- if (!strcmp(argv[1], "hello")) {
> > >- ulong size = __efi_helloworld_end - __efi_helloworld_begin;
> > >-
> > >+ if (!strcmp(image_opt, "hello")) {
> > > saddr = env_get("loadaddr");
> > >+ size = __efi_helloworld_end - __efi_helloworld_begin;
> > >+
> > > if (saddr)
> > > addr = simple_strtoul(saddr, NULL, 16);
> > > else
> > > addr = CONFIG_SYS_LOAD_ADDR;
> > >- memcpy(map_sysmem(addr, size), __efi_helloworld_begin, size);
> > >+
> > >+ image_buf = map_sysmem(addr, size);
> > >+ memcpy(image_buf, __efi_helloworld_begin, size);
> > >+
> > >+ device_path = NULL;
> > >+ image_path = NULL;
> > > } else
> > > #endif
> > > {
> > >- saddr = argv[1];
> > >+ saddr = image_opt;
> > >+ size_str = env_get("filesize");
> > >+ if (size_str)
> > >+ size = simple_strtoul(size_str, NULL, 16);
> > >+ else
> > >+ size = 0;
> > >
> > > addr = simple_strtoul(saddr, NULL, 16);
> > > /* Check that a numeric value was passed */
> > > if (!addr && *saddr != '0')
> > > return CMD_RET_USAGE;
> > >+
> > >+ image_buf = map_sysmem(addr, size);
> > >+
> > >+ device_path = bootefi_device_path;
> > >+ image_path = bootefi_image_path;
> > > }
> > >
> > >- printf("## Starting EFI application at %08lx ...\n", addr);
> > >- ret = do_bootefi_exec(map_sysmem(addr, 0), bootefi_device_path,
> > >- bootefi_image_path);
> > >- printf("## Application terminated, r = %lu\n",
> > >- ret & ~EFI_ERROR_MASK);
> > >+ if (!device_path && !image_path) {
> > >+ /*
> > >+ * Special case for efi payload not loaded from disk,
> > >+ * such as 'bootefi hello' or for example payload
> > >+ * loaded directly into memory via JTAG, etc:
> > >+ */
> > >+ printf("WARNING: using memory device/image path, this may confuse some payloads!\n");
> >
> > The EFI spec says that either of SourceBuffer or DevicePath may be NULL
> > when calling LoadImage().
>
> So are you suggesting we should remove this message?
>
> > >+
> > >+ /* actual addresses filled in after efi_load_image() */
> > >+ memdp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
> > >+ (uint64_t)image_buf, size);
> > >+ file_path = memdp; /* append(memdp, NULL) */
> > >+
> > >+ /*
> > >+ * Make sure that device for device_path exist
> > >+ * in load_image(). Otherwise, shell and grub will fail.
> >
> > GRUB will fail anyway because it cannot determine the disk from which it
> > was loaded. So why are we doing this?
>
> I will comment on another reply.
>
> > >+ */
> > >+ ret = efi_create_handle(&mem_handle);
> > >+ if (ret != EFI_SUCCESS)
> > >+ /* TODO: leaks device_path */
> > >+ goto err_add_protocol;
> > >+
> > >+ ret = efi_add_protocol(mem_handle, &efi_guid_device_path,
> > >+ memdp);
> >
> > Couldn't we as well use the device path of the root node as "file_path"?
>
> I don't get you point very well, but
> it will depend on how we should fix a grub issue mentioned above.
>
> > >+ if (ret != EFI_SUCCESS)
> > >+ goto err_add_protocol;
> > >+ } else {
> > >+ assert(device_path && image_path);
> > >+ file_path = efi_dp_append(device_path, image_path);
> >
> > Where is file_path freed?
>
> In this function. Will fix.
>
> > >+ }
> > >+
> > >+ ret = EFI_CALL(efi_load_image(false, efi_root,
> > >+ file_path, image_buf, size, &handle));
> > >+ if (ret != EFI_SUCCESS)
> > >+ goto err_prepare;
> > >+
> > >+ ret = do_bootefi_exec(handle);
> > >+ printf("## Application terminated, r = %lu\n", ret & ~EFI_ERROR_MASK);
> > >+
> > >+err_prepare:
> > >+ if (device_path)
> > >+ efi_free_pool(file_path);
> > >+
> > >+err_add_protocol:
> > >+ if (mem_handle)
> > >+ efi_delete_handle(mem_handle);
> > >+ if (memdp)
> > >+ efi_free_pool(memdp);
> > >
> > > if (ret != EFI_SUCCESS)
> > > return CMD_RET_FAILURE;
> > >- else
> > >- return CMD_RET_SUCCESS;
> > >+
> > >+ return CMD_RET_SUCCESS;
> > > }
> > >
> > > #ifdef CONFIG_CMD_BOOTEFI_SELFTEST
> > >@@ -581,7 +593,7 @@ static char bootefi_help_text[] =
> > > " Use environment variable efi_selftest to select a single test.\n"
> > > " Use 'setenv efi_selftest list' to enumerate all tests.\n"
> > > #endif
> > >- "bootefi bootmgr [fdt addr]\n"
> > >+ "bootefi bootmgr [fdt address]\n"
> > > " - load and boot EFI payload based on BootOrder/BootXXXX variables.\n"
> > > "\n"
> > > " If specified, the device tree located at <fdt address> gets\n"
> > >@@ -606,6 +618,13 @@ void efi_set_bootdev(const char *dev, const char *devnr, const char *path)
> > > ret = efi_dp_from_name(dev, devnr, path, &device, &image);
> > > if (ret == EFI_SUCCESS) {
> > > bootefi_device_path = device;
> > >+ if (image) {
> > >+ /* FIXME: image should not contain device */
> > >+ struct efi_device_path *image_tmp = image;
> > >+
> > >+ efi_dp_split_file_path(image, &device, &image);
> > >+ efi_free_pool(image_tmp);
> > >+ }
> > > bootefi_image_path = image;
> > > } else {
> > > bootefi_device_path = NULL;
> > >diff --git a/include/efi_loader.h b/include/efi_loader.h
> > >index d524dc7e24c1..c3eda2bb05d7 100644
> > >--- a/include/efi_loader.h
> > >+++ b/include/efi_loader.h
> > >@@ -408,8 +408,6 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path,
> > > struct efi_device_path *file_path,
> > > struct efi_loaded_image_obj **handle_ptr,
> > > struct efi_loaded_image **info_ptr);
> > >-efi_status_t efi_load_image_from_path(struct efi_device_path *file_path,
> > >- void **buffer, efi_uintn_t *size);
> > > /* Print information about all loaded images */
> > > void efi_print_image_infos(void *pc);
> > >
> > >@@ -563,8 +561,7 @@ struct efi_load_option {
> > >
> > > void efi_deserialize_load_option(struct efi_load_option *lo, u8 *data);
> > > unsigned long efi_serialize_load_option(struct efi_load_option *lo, u8 **data);
> > >-void *efi_bootmgr_load(struct efi_device_path **device_path,
> > >- struct efi_device_path **file_path);
> > >+efi_status_t efi_bootmgr_load(efi_handle_t *handle);
> > >
> > > #else /* CONFIG_IS_ENABLED(EFI_LOADER) */
> > >
> > >diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> > >index 4fccadc5483d..13ec79b2098b 100644
> > >--- a/lib/efi_loader/efi_bootmgr.c
> > >+++ b/lib/efi_loader/efi_bootmgr.c
> > >@@ -120,14 +120,14 @@ static void *get_var(u16 *name, const efi_guid_t *vendor,
> > > * if successful. This checks that the EFI_LOAD_OPTION is active (enabled)
> > > * and that the specified file to boot exists.
> > > */
> > >-static void *try_load_entry(uint16_t n, struct efi_device_path **device_path,
> > >- struct efi_device_path **file_path)
> > >+static efi_status_t try_load_entry(u16 n, efi_handle_t *handle)
> > > {
> > > struct efi_load_option lo;
> > > u16 varname[] = L"Boot0000";
> > > u16 hexmap[] = L"0123456789ABCDEF";
> > >- void *load_option, *image = NULL;
> > >+ void *load_option;
> > > efi_uintn_t size;
> > >+ efi_status_t ret;
> > >
> > > varname[4] = hexmap[(n & 0xf000) >> 12];
> > > varname[5] = hexmap[(n & 0x0f00) >> 8];
> > >@@ -136,19 +136,19 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path,
> > >
> > > load_option = get_var(varname, &efi_global_variable_guid, &size);
> > > if (!load_option)
> > >- return NULL;
> > >+ return EFI_LOAD_ERROR;
> > >
> > > efi_deserialize_load_option(&lo, load_option);
> > >
> > > if (lo.attributes & LOAD_OPTION_ACTIVE) {
> > > u32 attributes;
> > >- efi_status_t ret;
> > >
> > > debug("%s: trying to load \"%ls\" from %pD\n",
> > > __func__, lo.label, lo.file_path);
> > >
> > >- ret = efi_load_image_from_path(lo.file_path, &image, &size);
> > >-
> > >+ ret = EFI_CALL(efi_load_image(false, (void *)0x1 /* FIXME */,
> > >+ lo.file_path, NULL, 0,
> > >+ handle));
> > > if (ret != EFI_SUCCESS)
> > > goto error;
> > >
> > >@@ -159,17 +159,22 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path,
> > > L"BootCurrent",
> > > (efi_guid_t *)&efi_global_variable_guid,
> > > attributes, size, &n));
> > >- if (ret != EFI_SUCCESS)
> > >+ if (ret != EFI_SUCCESS) {
> > >+ if (EFI_CALL(efi_unload_image(*handle))
> > >+ != EFI_SUCCESS)
> > >+ printf("Unloading image failed\n");
> > > goto error;
> > >+ }
> > >
> > > printf("Booting: %ls\n", lo.label);
> > >- efi_dp_split_file_path(lo.file_path, device_path, file_path);
> > >+ } else {
> > >+ ret = EFI_LOAD_ERROR;
> > > }
> > >
> > > error:
> > > free(load_option);
> > >
> > >- return image;
> > >+ return ret;
> > > }
> > >
> > > /*
> > >@@ -177,12 +182,10 @@ error:
> > > * EFI variable, the available load-options, finding and returning
> > > * the first one that can be loaded successfully.
> > > */
> > >-void *efi_bootmgr_load(struct efi_device_path **device_path,
> > >- struct efi_device_path **file_path)
> > >+efi_status_t efi_bootmgr_load(efi_handle_t *handle)
> > > {
> > > u16 bootnext, *bootorder;
> > > efi_uintn_t size;
> > >- void *image = NULL;
> > > int i, num;
> > > efi_status_t ret;
> > >
> > >@@ -209,10 +212,9 @@ void *efi_bootmgr_load(struct efi_device_path **device_path,
> > > /* load BootNext */
> > > if (ret == EFI_SUCCESS) {
> > > if (size == sizeof(u16)) {
> > >- image = try_load_entry(bootnext, device_path,
> > >- file_path);
> > >- if (image)
> > >- return image;
> > >+ ret = try_load_entry(bootnext, handle);
> > >+ if (ret == EFI_SUCCESS)
> > >+ return ret;
> > > }
> > > } else {
> > > printf("Deleting BootNext failed\n");
> > >@@ -223,19 +225,20 @@ void *efi_bootmgr_load(struct efi_device_path **device_path,
> > > bootorder = get_var(L"BootOrder", &efi_global_variable_guid, &size);
> > > if (!bootorder) {
> > > printf("BootOrder not defined\n");
> > >+ ret = EFI_NOT_FOUND;
> > > goto error;
> > > }
> > >
> > > num = size / sizeof(uint16_t);
> > > for (i = 0; i < num; i++) {
> > > debug("%s: trying to load Boot%04X\n", __func__, bootorder[i]);
> > >- image = try_load_entry(bootorder[i], device_path, file_path);
> > >- if (image)
> > >+ ret = try_load_entry(bootorder[i], handle);
> > >+ if (ret == EFI_SUCCESS)
> > > break;
> > > }
> > >
> > > free(bootorder);
> > >
> > > error:
> > >- return image;
> > >+ return ret;
> > > }
> > >diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> > >index b215bd7723da..65425fabc08f 100644
> > >--- a/lib/efi_loader/efi_boottime.c
> > >+++ b/lib/efi_loader/efi_boottime.c
> > >@@ -1611,6 +1611,7 @@ failure:
> > > * @size: size of the loaded image
> > > * Return: status code
> > > */
> > >+static
> > > efi_status_t efi_load_image_from_path(struct efi_device_path *file_path,
> > > void **buffer, efi_uintn_t *size)
> > > {
> > >@@ -2684,6 +2685,7 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
> > > }
> > >
> > > current_image = image_handle;
> > >+ debug("EFI: Jumping into 0x%p\n", image_obj->entry);
> >
> > Please, use EFI_PRINT() here.
>
> Basically I agree, but there are lots of debug()'s in this file.
> We should replace them all at once later.
>
> Thanks,
> -Takahiro Akashi
>
> > Best regards
> >
> > Heinrich
> >
> > > ret = EFI_CALL(image_obj->entry(image_handle, &systab));
> > >
> > > /*
> > >
> >
^ permalink raw reply [flat|nested] 32+ messages in thread
* [U-Boot] [RFC v3 09/10] efi_loader: rework bootmgr/bootefi using load_image API
2019-04-16 16:20 ` Heinrich Schuchardt
@ 2019-04-18 8:29 ` Alexander Graf
0 siblings, 0 replies; 32+ messages in thread
From: Alexander Graf @ 2019-04-18 8:29 UTC (permalink / raw)
To: u-boot
On 16.04.19 18:20, Heinrich Schuchardt wrote:
> On 4/16/19 12:56 PM, Heinrich Schuchardt wrote:
>> On 4/16/19 6:24 AM, AKASHI Takahiro wrote:
>>> In the current implementation, bootefi command and EFI boot manager
>>> don't use load_image API, instead, use more primitive and internal
>>> functions. This will introduce duplicated code and potentially
>>> unknown bugs as well as inconsistent behaviours.
>>>
>>> With this patch, do_efibootmgr() and do_boot_efi() are completely
>>> overhauled and re-implemented using load_image API.
>>>
>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>> ---
>>> cmd/bootefi.c | 197
>>> +++++++++++++++++++---------------
>>> include/efi_loader.h | 5 +-
>>> lib/efi_loader/efi_bootmgr.c | 43 ++++----
>>> lib/efi_loader/efi_boottime.c | 2 +
>>> 4 files changed, 134 insertions(+), 113 deletions(-)
>>>
>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>>> index f14966961b23..97d49a53a44b 100644
>>> --- a/cmd/bootefi.c
>>> +++ b/cmd/bootefi.c
>>> @@ -222,88 +222,39 @@ static efi_status_t efi_install_fdt(const char
>>> *fdt_opt)
>>> /**
>>> * do_bootefi_exec() - execute EFI binary
>>> *
>>> - * @efi: address of the binary
>>> - * @device_path: path of the device from which the binary was
>>> loaded
>>> - * @image_path: device path of the binary
>>> + * @handle: handle of loaded image
>>> * Return: status code
>>> *
>>> * Load the EFI binary into a newly assigned memory unwinding the
>>> relocation
>>> * information, install the loaded image protocol, and call the
>>> binary.
>>> */
>>> -static efi_status_t do_bootefi_exec(void *efi,
>>> - struct efi_device_path *device_path,
>>> - struct efi_device_path *image_path)
>>> +static efi_status_t do_bootefi_exec(efi_handle_t handle)
>>> {
>>> - efi_handle_t mem_handle = NULL;
>>> - struct efi_device_path *memdp = NULL;
>>> - efi_status_t ret;
>>> - struct efi_loaded_image_obj *image_obj = NULL;
>>> struct efi_loaded_image *loaded_image_info = NULL;
>>> -
>>> - /*
>>> - * Special case for efi payload not loaded from disk, such as
>>> - * 'bootefi hello' or for example payload loaded directly into
>>> - * memory via JTAG, etc:
>>> - */
>>> - if (!device_path && !image_path) {
>>> - printf("WARNING: using memory device/image path, this may
>>> confuse some payloads!\n");
>>> - /* actual addresses filled in after efi_load_pe() */
>>> - memdp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, 0, 0);
>>> - device_path = image_path = memdp;
>>> - /*
>>> - * Grub expects that the device path of the loaded image is
>>> - * installed on a handle.
>>> - */
>>> - ret = efi_create_handle(&mem_handle);
>>> - if (ret != EFI_SUCCESS)
>>> - return ret; /* TODO: leaks device_path */
>>> - ret = efi_add_protocol(mem_handle, &efi_guid_device_path,
>>> - device_path);
>>> - if (ret != EFI_SUCCESS)
>>> - goto err_add_protocol;
>>> - } else {
>>> - assert(device_path && image_path);
>>> - }
>>> -
>>> - ret = efi_setup_loaded_image(device_path, image_path, &image_obj,
>>> - &loaded_image_info);
>>> - if (ret)
>>> - goto err_prepare;
>>> + efi_status_t ret;
>>>
>>> /* Transfer environment variable as load options */
>>> - set_load_options(loaded_image_info, "bootargs");
>>
>> In set_load_options() an error could occur (out of memory). So I think
>> it should return a status.
>>
>>> -
>>> - /* Load the EFI payload */
>>> - ret = efi_load_pe(image_obj, efi, loaded_image_info);
>>> + ret = EFI_CALL(systab.boottime->open_protocol(
>>> + handle,
>>> + &efi_guid_loaded_image,
>>> + (void **)&loaded_image_info,
>>> + NULL, NULL,
>>> + EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL));
>>
>> Shouldn't we move this to set_load_options()?
>>
>>> if (ret != EFI_SUCCESS)
>>> - goto err_prepare;
>>> -
>>> - if (memdp) {
>>> - struct efi_device_path_memory *mdp = (void *)memdp;
>>> - mdp->memory_type = loaded_image_info->image_code_type;
>>> - mdp->start_address = (uintptr_t)loaded_image_info->image_base;
>>> - mdp->end_address = mdp->start_address +
>>> - loaded_image_info->image_size;
>>> - }
>>> + goto err;
>>> + set_load_options(loaded_image_info, "bootargs");
>>>
>>> /* we don't support much: */
>>>
>>> env_set("efi_8be4df61-93ca-11d2-aa0d-00e098032b8c_OsIndicationsSupported",
>>>
>>> "{ro,boot}(blob)0000000000000000");
>>
>> Shouldn't this move to efi_setup.c? Probably we should also set
>> OsIndications. I would prefer using efi_set_variable(). I think moving
>> this could be done in a preparatory patch.
>>
>>>
>>> /* Call our payload! */
>>> - debug("%s: Jumping to 0x%p\n", __func__, image_obj->entry);
>>> - ret = EFI_CALL(efi_start_image(&image_obj->header, NULL, NULL));
>>> + ret = EFI_CALL(efi_start_image(handle, NULL, NULL));
>>>
>>> -err_prepare:
>>> - /* image has returned, loaded-image obj goes *poof*: */
>>> efi_restore_gd();
>>> free(loaded_image_info->load_options);
>>> - efi_delete_handle(&image_obj->header);
>>> -
>>> -err_add_protocol:
>>> - if (mem_handle)
>>> - efi_delete_handle(mem_handle);
>>>
>>> +err:
>>> return ret;
>>> }
>>>
>>> @@ -317,8 +268,7 @@ err_add_protocol:
>>> */
>>> static int do_efibootmgr(const char *fdt_opt)
>>> {
>>> - struct efi_device_path *device_path, *file_path;
>>> - void *addr;
>>> + efi_handle_t handle;
>>> efi_status_t ret;
>>>
>>> /* Allow unaligned memory access */
>>> @@ -340,19 +290,19 @@ static int do_efibootmgr(const char *fdt_opt)
>>> else if (ret != EFI_SUCCESS)
>>> return CMD_RET_FAILURE;
>>>
>>> - addr = efi_bootmgr_load(&device_path, &file_path);
>>> - if (!addr)
>>> - return 1;
>>> + ret = efi_bootmgr_load(&handle);
>>> + if (ret != EFI_SUCCESS) {
>>> + printf("EFI boot manager: Cannot load any image\n");
>>> + return CMD_RET_FAILURE;
>>> + }
>>>
>>> - printf("## Starting EFI application at %p ...\n", addr);
>>> - ret = do_bootefi_exec(addr, device_path, file_path);
>>> - printf("## Application terminated, r = %lu\n",
>>> - ret & ~EFI_ERROR_MASK);
>>> + ret = do_bootefi_exec(handle);
>>> + printf("## Application terminated, r = %lu\n", ret &
>>> ~EFI_ERROR_MASK);
>>>
>>> if (ret != EFI_SUCCESS)
>>> - return 1;
>>> + return CMD_RET_FAILURE;
>>>
>>> - return 0;
>>> + return CMD_RET_SUCCESS;
>>> }
>>>
>>> /*
>>> @@ -367,10 +317,14 @@ static int do_efibootmgr(const char *fdt_opt)
>>> */
>>> static int do_boot_efi(const char *image_opt, const char *fdt_opt)
>>> {
>>> - unsigned long addr;
>>> - char *saddr;
>>> + void *image_buf;
>>> + struct efi_device_path *device_path, *image_path;
>>> + struct efi_device_path *memdp = NULL, *file_path = NULL;
>>> + const char *saddr;
>>> + unsigned long addr, size;
>>> + const char *size_str;
>>> + efi_handle_t mem_handle = NULL, handle;
>>> efi_status_t ret;
>>> - unsigned long fdt_addr;
>>>
>>> /* Allow unaligned memory access */
>>> allow_unaligned();
>>> @@ -392,36 +346,94 @@ static int do_boot_efi(const char *image_opt,
>>> const char *fdt_opt)
>>> return CMD_RET_FAILURE;
>>>
>>> #ifdef CONFIG_CMD_BOOTEFI_HELLO
>>> - if (!strcmp(argv[1], "hello")) {
>>> - ulong size = __efi_helloworld_end - __efi_helloworld_begin;
>>> -
>>> + if (!strcmp(image_opt, "hello")) {
>>> saddr = env_get("loadaddr");
>>> + size = __efi_helloworld_end - __efi_helloworld_begin;
>>> +
>>> if (saddr)
>>> addr = simple_strtoul(saddr, NULL, 16);
>>> else
>>> addr = CONFIG_SYS_LOAD_ADDR;
>>> - memcpy(map_sysmem(addr, size), __efi_helloworld_begin, size);
>>> +
>>> + image_buf = map_sysmem(addr, size);
>>> + memcpy(image_buf, __efi_helloworld_begin, size);
>>> +
>>> + device_path = NULL;
>>> + image_path = NULL;
>>> } else
>>> #endif
>>> {
>>> - saddr = argv[1];
>>> + saddr = image_opt;
>>> + size_str = env_get("filesize");
>>> + if (size_str)
>>> + size = simple_strtoul(size_str, NULL, 16);
>>> + else
>>> + size = 0;
>>>
>>> addr = simple_strtoul(saddr, NULL, 16);
>>> /* Check that a numeric value was passed */
>>> if (!addr && *saddr != '0')
>>> return CMD_RET_USAGE;
>>> +
>>> + image_buf = map_sysmem(addr, size);
>>> +
>>> + device_path = bootefi_device_path;
>>> + image_path = bootefi_image_path;
>>> }
>>>
>>> - printf("## Starting EFI application at %08lx ...\n", addr);
>>> - ret = do_bootefi_exec(map_sysmem(addr, 0), bootefi_device_path,
>>> - bootefi_image_path);
>>> - printf("## Application terminated, r = %lu\n",
>>> - ret & ~EFI_ERROR_MASK);
>>> + if (!device_path && !image_path) {
>>> + /*
>>> + * Special case for efi payload not loaded from disk,
>>> + * such as 'bootefi hello' or for example payload
>>> + * loaded directly into memory via JTAG, etc:
>>> + */
>>> + printf("WARNING: using memory device/image path, this may
>>> confuse some payloads!\n");
>>
>> The EFI spec says that either of SourceBuffer or DevicePath may be NULL
>> when calling LoadImage().
>>
>>> +
>>> + /* actual addresses filled in after efi_load_image() */
>>> + memdp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
>>> + (uint64_t)image_buf, size);
>>> + file_path = memdp; /* append(memdp, NULL) */
>>> +
>>> + /*
>>> + * Make sure that device for device_path exist
>>> + * in load_image(). Otherwise, shell and grub will fail.
>>
>> GRUB will fail anyway because it cannot determine the disk from which it
>> was loaded. So why are we doing this?
>
> In Rob's original commit bf19273e81eb ("efi_loader: Add mem-mapped
> for fallback") the comment was:
> "This fixes 'bootefi hello' after devicepath refactoring."
>
> EFI Shell.efi starts fine even when we set device_path and image_path
> to NULL.
>
> When loading GRUB from disk or via tftp the device path and the image
> path are set, e.g. for tftp:
>
> => dhcp grubarm.efi
> => bootefi 0x40200000 $fdtcontroladdr
> Found 0 disks
> ## Starting EFI application at 40200000 ...
> device_path:
> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/MAC(525400123456,0x1)
> image_path: /grubarm.efi
> error: disk `,msdos2' not found.
> Entering rescue mode...
> grub rescue>
>
> Maybe GRUB would run if all necessary modules were compiled in to do a
> network boot. But how would it find grub.conf?
>
> So my feeling is that we are creating the dummy memory device path
> because:
>
> - Once `bootefi hello` which is our own test program had a problem.
> - GRUB has a bug: it hangs or crashes if the file device path is not
> set in LoadImage().
>
> I think the problem should not be fixed in U-Boot but in GRUB.
>
> I am CC-ing Leif due to all the attention he has paid both to U-Boot
> and to GRUB.
Please also keep in mind that you can not fix already released versions
of GRUB.
Alex
^ permalink raw reply [flat|nested] 32+ messages in thread
end of thread, other threads:[~2019-04-18 8:29 UTC | newest]
Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-16 4:24 [U-Boot] [RFC v3 00/10] efi_loader: rework bootefi/bootmgr AKASHI Takahiro
2019-04-16 4:24 ` [U-Boot] [RFC v3 01/10] efi_loader: device_path: handle special case of loading AKASHI Takahiro
2019-04-16 4:44 ` Heinrich Schuchardt
2019-04-16 4:24 ` [U-Boot] [RFC v3 02/10] efi_loader: export root node handle AKASHI Takahiro
2019-04-16 4:48 ` Heinrich Schuchardt
2019-04-17 6:57 ` AKASHI Takahiro
2019-04-16 4:24 ` [U-Boot] [RFC v3 03/10] cmd: bootefi: carve out fdt handling from do_bootefi() AKASHI Takahiro
2019-04-16 4:54 ` Heinrich Schuchardt
2019-04-17 7:01 ` AKASHI Takahiro
2019-04-17 16:35 ` Heinrich Schuchardt
2019-04-18 0:16 ` AKASHI Takahiro
2019-04-16 4:24 ` [U-Boot] [RFC v3 04/10] cmd: bootefi: merge efi_install_fdt() and efi_process_fdt() AKASHI Takahiro
2019-04-16 5:05 ` Heinrich Schuchardt
2019-04-17 10:53 ` Heinrich Schuchardt
2019-04-16 4:24 ` [U-Boot] [RFC v3 05/10] cmd: bootefi: carve out efi_selftest code from do_bootefi() AKASHI Takahiro
2019-04-16 5:21 ` Heinrich Schuchardt
2019-04-17 7:19 ` AKASHI Takahiro
2019-04-16 4:24 ` [U-Boot] [RFC v3 06/10] cmd: bootefi: move do_bootefi_bootmgr_exec() forward AKASHI Takahiro
2019-04-16 5:22 ` Heinrich Schuchardt
2019-04-16 4:24 ` [U-Boot] [RFC v3 07/10] cmd: bootefi: carve out bootmgr code from do_bootefi() AKASHI Takahiro
2019-04-16 4:24 ` [U-Boot] [RFC v3 08/10] cmd: bootefi: carve out do_boot_efi() " AKASHI Takahiro
2019-04-16 5:31 ` Heinrich Schuchardt
2019-04-17 7:43 ` AKASHI Takahiro
2019-04-16 4:24 ` [U-Boot] [RFC v3 09/10] efi_loader: rework bootmgr/bootefi using load_image API AKASHI Takahiro
2019-04-16 10:56 ` Heinrich Schuchardt
2019-04-16 16:20 ` Heinrich Schuchardt
2019-04-18 8:29 ` Alexander Graf
2019-04-18 0:13 ` AKASHI Takahiro
2019-04-18 3:06 ` AKASHI Takahiro
2019-04-16 4:24 ` [U-Boot] [RFC v3 10/10] cmd: add efibootmgr command AKASHI Takahiro
2019-04-16 5:27 ` Heinrich Schuchardt
2019-04-17 8:07 ` AKASHI Takahiro
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.