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

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;

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;

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;

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)

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();

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[]) {

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

On 4/16/19 6:24 AM, AKASHI Takahiro wrote:

In the current implementation, bootefi command and EFI boot manager don't use load_image API, instead, use more primitive and internal functions. This will introduce duplicated code and potentially unknown bugs as well as inconsistent behaviours.

With this patch, do_efibootmgr() and do_boot_efi() are completely overhauled and re-implemented using load_image API.

Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org

cmd/bootefi.c | 197 +++++++++++++++++++--------------- include/efi_loader.h | 5 +- lib/efi_loader/efi_bootmgr.c | 43 ++++---- lib/efi_loader/efi_boottime.c | 2 + 4 files changed, 134 insertions(+), 113 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c index f14966961b23..97d49a53a44b 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -222,88 +222,39 @@ static efi_status_t efi_install_fdt(const char *fdt_opt) /**

• do_bootefi_exec() - execute EFI binary

• • @efi: address of the binary
• • @device_path: path of the device from which the binary was loaded
• • @image_path: device path of the binary

• • @handle: handle of loaded image
  • Return: status code
  • Load the EFI binary into a newly assigned memory unwinding the relocation
  • information, install the loaded image protocol, and call the binary.
  */

-static efi_status_t do_bootefi_exec(void *efi,

• 		    struct efi_device_path *device_path,
  

• 		    struct efi_device_path *image_path)
  

+static efi_status_t do_bootefi_exec(efi_handle_t handle) {

• efi_handle_t mem_handle = NULL;
• struct efi_device_path *memdp = NULL;
• efi_status_t ret;
• struct efi_loaded_image_obj *image_obj = NULL; struct efi_loaded_image *loaded_image_info = NULL;
• /*

• * Special case for efi payload not loaded from disk, such as
  

• * 'bootefi hello' or for example payload loaded directly into
  

• * memory via JTAG, etc:
  

• */
  

• if (!device_path && !image_path) {

• printf("WARNING: using memory device/image path, this may confuse some payloads!\n");
  

• /* actual addresses filled in after efi_load_pe() */
  

• memdp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, 0, 0);
  

• device_path = image_path = memdp;
  

• /*
  

•  * Grub expects that the device path of the loaded image is
  

•  * installed on a handle.
  

•  */
  

• ret = efi_create_handle(&mem_handle);
  

• if (ret != EFI_SUCCESS)
  

• 	return ret; /* TODO: leaks device_path */
  

• ret = efi_add_protocol(mem_handle, &efi_guid_device_path,
  

• 		       device_path);
  

• if (ret != EFI_SUCCESS)
  

• 	goto err_add_protocol;
  

• } else {

• assert(device_path && image_path);
  

• }
• ret = efi_setup_loaded_image(device_path, image_path, &image_obj,

• 		     &loaded_image_info);
  

• if (ret)

• goto err_prepare;
  

• efi_status_t ret;

  /* Transfer environment variable as load options */

• set_load_options(loaded_image_info, "bootargs");

In set_load_options() an error could occur (out of memory). So I think it should return a status.

• /* Load the EFI payload */
• ret = efi_load_pe(image_obj, efi, loaded_image_info);

• ret = EFI_CALL(systab.boottime->open_protocol(

• 			handle,
  

• 			&efi_guid_loaded_image,
  

• 			(void **)&loaded_image_info,
  

• 			NULL, NULL,
  

• 			EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL));
  

Shouldn't we move this to set_load_options()?

if (ret != EFI_SUCCESS)

• goto err_prepare;
  

• if (memdp) {

• struct efi_device_path_memory *mdp = (void *)memdp;
  

• mdp->memory_type = loaded_image_info->image_code_type;
  

• mdp->start_address = (uintptr_t)loaded_image_info->image_base;
  

• mdp->end_address = mdp->start_address +
  

• 		loaded_image_info->image_size;
  

• }

• goto err;
  

• set_load_options(loaded_image_info, "bootargs");

  /* we don't support much: */ env_set("efi_8be4df61-93ca-11d2-aa0d-00e098032b8c_OsIndicationsSupported", "{ro,boot}(blob)0000000000000000");

Shouldn't this move to efi_setup.c? Probably we should also set OsIndications. I would prefer using efi_set_variable(). I think moving this could be done in a preparatory patch.

/* Call our payload! */

• debug("%s: Jumping to 0x%p\n", __func__, image_obj->entry);
• ret = EFI_CALL(efi_start_image(&image_obj->header, NULL, NULL));

• ret = EFI_CALL(efi_start_image(handle, NULL, NULL));

-err_prepare:

• /* image has returned, loaded-image obj goes *poof*: */ efi_restore_gd(); free(loaded_image_info->load_options);
• efi_delete_handle(&image_obj->header);

-err_add_protocol:

• if (mem_handle)

• efi_delete_handle(mem_handle);
  
  

+err: return ret; }

@@ -317,8 +268,7 @@ err_add_protocol: */ static int do_efibootmgr(const char *fdt_opt) {

• struct efi_device_path *device_path, *file_path;
• void *addr;

• efi_handle_t handle; efi_status_t ret;

  /* Allow unaligned memory access */

@@ -340,19 +290,19 @@ static int do_efibootmgr(const char *fdt_opt) else if (ret != EFI_SUCCESS) return CMD_RET_FAILURE;

• addr = efi_bootmgr_load(&device_path, &file_path);
• if (!addr)

• return 1;
  

• ret = efi_bootmgr_load(&handle);
• if (ret != EFI_SUCCESS) {

• printf("EFI boot manager: Cannot load any image\n");
  

• return CMD_RET_FAILURE;
  

• }

• printf("## Starting EFI application at %p ...\n", addr);
• ret = do_bootefi_exec(addr, device_path, file_path);
• printf("## Application terminated, r = %lu\n",

•       ret & ~EFI_ERROR_MASK);
  

• ret = do_bootefi_exec(handle);

• printf("## Application terminated, r = %lu\n", ret & ~EFI_ERROR_MASK);

  if (ret != EFI_SUCCESS)

• return 1;
  

• return CMD_RET_FAILURE;
  
  

• return 0;

• return CMD_RET_SUCCESS; }

  /*

@@ -367,10 +317,14 @@ static int do_efibootmgr(const char *fdt_opt) */ static int do_boot_efi(const char *image_opt, const char *fdt_opt) {

• unsigned long addr;
• char *saddr;

• void *image_buf;
• struct efi_device_path *device_path, *image_path;
• struct efi_device_path *memdp = NULL, *file_path = NULL;
• const char *saddr;
• unsigned long addr, size;
• const char *size_str;
• efi_handle_t mem_handle = NULL, handle; efi_status_t ret;

• unsigned long fdt_addr;

  /* Allow unaligned memory access */ allow_unaligned();

@@ -392,36 +346,94 @@ static int do_boot_efi(const char *image_opt, const char *fdt_opt) return CMD_RET_FAILURE;

#ifdef CONFIG_CMD_BOOTEFI_HELLO

• if (!strcmp(argv[1], "hello")) {

• ulong size = __efi_helloworld_end - __efi_helloworld_begin;
  

• if (!strcmp(image_opt, "hello")) { saddr = env_get("loadaddr");

• size = __efi_helloworld_end - __efi_helloworld_begin;
  

• if (saddr) addr = simple_strtoul(saddr, NULL, 16); else addr = CONFIG_SYS_LOAD_ADDR;

• memcpy(map_sysmem(addr, size), __efi_helloworld_begin, size);
  

• image_buf = map_sysmem(addr, size);
  

• memcpy(image_buf, __efi_helloworld_begin, size);
  

• device_path = NULL;
  

• image_path = NULL;
  

  } else #endif {

• saddr = argv[1];
  

• saddr = image_opt;
  

• size_str = env_get("filesize");
  

• if (size_str)
  

• 	size = simple_strtoul(size_str, NULL, 16);
  

• else
  

• 	size = 0;
  
  

  addr = simple_strtoul(saddr, NULL, 16); /* Check that a numeric value was passed */ if (!addr && *saddr != '0') return CMD_RET_USAGE;

• image_buf = map_sysmem(addr, size);
  

• device_path = bootefi_device_path;
  

• image_path = bootefi_image_path;
  

  }

• printf("## Starting EFI application at %08lx ...\n", addr);
• ret = do_bootefi_exec(map_sysmem(addr, 0), bootefi_device_path,

• 	      bootefi_image_path);
  

• printf("## Application terminated, r = %lu\n",

•       ret & ~EFI_ERROR_MASK);
  

• if (!device_path && !image_path) {

• /*
  

•  * Special case for efi payload not loaded from disk,
  

•  * such as 'bootefi hello' or for example payload
  

•  * loaded directly into memory via JTAG, etc:
  

•  */
  

• printf("WARNING: using memory device/image path, this may confuse some payloads!\n");
  

The EFI spec says that either of SourceBuffer or DevicePath may be NULL when calling LoadImage().

• /* actual addresses filled in after efi_load_image() */
  

• memdp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
  

• 			(uint64_t)image_buf, size);
  

• file_path = memdp; /* append(memdp, NULL) */
  

• /*
  

•  * Make sure that device for device_path exist
  

•  * in load_image(). Otherwise, shell and grub will fail.
  

GRUB will fail anyway because it cannot determine the disk from which it was loaded. So why are we doing this?

•  */
  

• ret = efi_create_handle(&mem_handle);
  

• if (ret != EFI_SUCCESS)
  

• 	/* TODO: leaks device_path */
  

• 	goto err_add_protocol;
  

• ret = efi_add_protocol(mem_handle, &efi_guid_device_path,
  

• 		       memdp);
  

Couldn't we as well use the device path of the root node as "file_path"?

• if (ret != EFI_SUCCESS)
  

• 	goto err_add_protocol;
  

• } else {

• assert(device_path && image_path);
  

• file_path = efi_dp_append(device_path, image_path);
  

Where is file_path freed?

• }
• ret = EFI_CALL(efi_load_image(false, efi_root,

• 		      file_path, image_buf, size, &handle));
  

• if (ret != EFI_SUCCESS)

• goto err_prepare;
  

• ret = do_bootefi_exec(handle);
• printf("## Application terminated, r = %lu\n", ret & ~EFI_ERROR_MASK);

+err_prepare:

• if (device_path)

• efi_free_pool(file_path);
  

+err_add_protocol:

• if (mem_handle)

• efi_delete_handle(mem_handle);
  

• if (memdp)

• efi_free_pool(memdp);
  
  

  if (ret != EFI_SUCCESS) return CMD_RET_FAILURE;

• else

• return CMD_RET_SUCCESS;
  

• return CMD_RET_SUCCESS; }

  #ifdef CONFIG_CMD_BOOTEFI_SELFTEST

@@ -581,7 +593,7 @@ static char bootefi_help_text[] = " Use environment variable efi_selftest to select a single test.\n" " Use 'setenv efi_selftest list' to enumerate all tests.\n" #endif

• "bootefi bootmgr [fdt addr]\n"

• "bootefi bootmgr [fdt address]\n" " - load and boot EFI payload based on BootOrder/BootXXXX variables.\n" "\n" " If specified, the device tree located at <fdt address> gets\n"

@@ -606,6 +618,13 @@ void efi_set_bootdev(const char *dev, const char *devnr, const char *path) ret = efi_dp_from_name(dev, devnr, path, &device, &image); if (ret == EFI_SUCCESS) { bootefi_device_path = device;

• if (image) {
  

• 	/* FIXME: image should not contain device */
  

• 	struct efi_device_path *image_tmp = image;
  

• 	efi_dp_split_file_path(image, &device, &image);
  

• 	efi_free_pool(image_tmp);
  

• }
  

  bootefi_image_path = image; } else { bootefi_device_path = NULL;

diff --git a/include/efi_loader.h b/include/efi_loader.h index d524dc7e24c1..c3eda2bb05d7 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -408,8 +408,6 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path, struct efi_device_path *file_path, struct efi_loaded_image_obj **handle_ptr, struct efi_loaded_image **info_ptr); -efi_status_t efi_load_image_from_path(struct efi_device_path *file_path,

• 		      void **buffer, efi_uintn_t *size);
  

  /* Print information about all loaded images */ void efi_print_image_infos(void *pc);

@@ -563,8 +561,7 @@ struct efi_load_option {

void efi_deserialize_load_option(struct efi_load_option *lo, u8 *data); unsigned long efi_serialize_load_option(struct efi_load_option *lo, u8 **data); -void *efi_bootmgr_load(struct efi_device_path **device_path,

•        struct efi_device_path **file_path);
  

+efi_status_t efi_bootmgr_load(efi_handle_t *handle);

#else /* CONFIG_IS_ENABLED(EFI_LOADER) */

diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c index 4fccadc5483d..13ec79b2098b 100644 --- a/lib/efi_loader/efi_bootmgr.c +++ b/lib/efi_loader/efi_bootmgr.c @@ -120,14 +120,14 @@ static void *get_var(u16 *name, const efi_guid_t *vendor,

• if successful. This checks that the EFI_LOAD_OPTION is active (enabled)
• and that the specified file to boot exists.

*/ -static void *try_load_entry(uint16_t n, struct efi_device_path **device_path,

• 	    struct efi_device_path **file_path)
  

+static efi_status_t try_load_entry(u16 n, efi_handle_t *handle) { struct efi_load_option lo; u16 varname[] = L"Boot0000"; u16 hexmap[] = L"0123456789ABCDEF";

• void *load_option, *image = NULL;

• void *load_option; efi_uintn_t size;

• efi_status_t ret;

  varname[4] = hexmap[(n & 0xf000) >> 12]; varname[5] = hexmap[(n & 0x0f00) >> 8];

@@ -136,19 +136,19 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path,

load_option = get_var(varname, &efi_global_variable_guid, &size); if (!load_option)

• return NULL;
  

• return EFI_LOAD_ERROR;
  
  

  efi_deserialize_load_option(&lo, load_option);

  if (lo.attributes & LOAD_OPTION_ACTIVE) { u32 attributes;

• efi_status_t ret;
  
  

  debug("%s: trying to load "%ls" from %pD\n", __func__, lo.label, lo.file_path);

• ret = efi_load_image_from_path(lo.file_path, &image, &size);
  

• ret = EFI_CALL(efi_load_image(false, (void *)0x1 /* FIXME */,
  

• 			      lo.file_path, NULL, 0,
  

• 			      handle));
  

  if (ret != EFI_SUCCESS) goto error;

@@ -159,17 +159,22 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path, L"BootCurrent", (efi_guid_t *)&efi_global_variable_guid, attributes, size, &n));

• if (ret != EFI_SUCCESS)
  

• if (ret != EFI_SUCCESS) {
  

• 	if (EFI_CALL(efi_unload_image(*handle))
  

• 	    != EFI_SUCCESS)
  

• 		printf("Unloading image failed\n");
  goto error;
  

• }
  
  

  printf("Booting: %ls\n", lo.label);

• efi_dp_split_file_path(lo.file_path, device_path, file_path);
  

• } else {

• ret = EFI_LOAD_ERROR;
  

  }

  error: free(load_option);

• return image;

• return ret; }

  /*

@@ -177,12 +182,10 @@ error:

• EFI variable, the available load-options, finding and returning
• the first one that can be loaded successfully.

*/ -void *efi_bootmgr_load(struct efi_device_path **device_path,

•        struct efi_device_path **file_path)
  

+efi_status_t efi_bootmgr_load(efi_handle_t *handle) { u16 bootnext, *bootorder; efi_uintn_t size;

• void *image = NULL; int i, num; efi_status_t ret;

@@ -209,10 +212,9 @@ void *efi_bootmgr_load(struct efi_device_path **device_path, /* load BootNext */ if (ret == EFI_SUCCESS) { if (size == sizeof(u16)) {

• 		image = try_load_entry(bootnext, device_path,
  

• 				       file_path);
  

• 		if (image)
  

• 			return image;
  

• 		ret = try_load_entry(bootnext, handle);
  

• 		if (ret == EFI_SUCCESS)
  

• 			return ret;
  }
  

  } else { printf("Deleting BootNext failed\n");

@@ -223,19 +225,20 @@ void *efi_bootmgr_load(struct efi_device_path **device_path, bootorder = get_var(L"BootOrder", &efi_global_variable_guid, &size); if (!bootorder) { printf("BootOrder not defined\n");

• ret = EFI_NOT_FOUND;
  

  goto error; }

  num = size / sizeof(uint16_t); for (i = 0; i < num; i++) { debug("%s: trying to load Boot%04X\n", __func__, bootorder[i]);

• image = try_load_entry(bootorder[i], device_path, file_path);
  

• if (image)
  

• ret = try_load_entry(bootorder[i], handle);
  

• if (ret == EFI_SUCCESS)
  break;
  

  }

  free(bootorder);

  error:

• return image;

• return ret; }

diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index b215bd7723da..65425fabc08f 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -1611,6 +1611,7 @@ failure:

• @size: size of the loaded image
• Return: status code

*/ +static efi_status_t efi_load_image_from_path(struct efi_device_path *file_path, void **buffer, efi_uintn_t *size) { @@ -2684,6 +2685,7 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle, }

current_image = image_handle;

• debug("EFI: Jumping into 0x%p\n", image_obj->entry);

Please, use EFI_PRINT() here.

Best regards

Heinrich

ret = EFI_CALL(image_obj->entry(image_handle, &systab));

/*

On 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

Correction:

On Thu, Apr 18, 2019 at 09:13:52AM +0900, AKASHI Takahiro wrote:

On Tue, Apr 16, 2019 at 12:56:32PM +0200, Heinrich Schuchardt wrote:

...

On 4/16/19 6:24 AM, AKASHI Takahiro wrote:

...

In the current implementation, bootefi command and EFI boot manager don't use load_image API, instead, use more primitive and internal functions. This will introduce duplicated code and potentially unknown bugs as well as inconsistent behaviours.

With this patch, do_efibootmgr() and do_boot_efi() are completely overhauled and re-implemented using load_image API.

Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org

cmd/bootefi.c | 197 +++++++++++++++++++--------------- include/efi_loader.h | 5 +- lib/efi_loader/efi_bootmgr.c | 43 ++++---- lib/efi_loader/efi_boottime.c | 2 + 4 files changed, 134 insertions(+), 113 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c index f14966961b23..97d49a53a44b 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -222,88 +222,39 @@ static efi_status_t efi_install_fdt(const char *fdt_opt) /**

• do_bootefi_exec() - execute EFI binary

• • @efi: address of the binary
• • @device_path: path of the device from which the binary was loaded
• • @image_path: device path of the binary

• • @handle: handle of loaded image
  • Return: status code
  • Load the EFI binary into a newly assigned memory unwinding the relocation
  • information, install the loaded image protocol, and call the binary.
  */

-static efi_status_t do_bootefi_exec(void *efi,

• 		    struct efi_device_path *device_path,
  

• 		    struct efi_device_path *image_path)
  

+static efi_status_t do_bootefi_exec(efi_handle_t handle) {

• efi_handle_t mem_handle = NULL;
• struct efi_device_path *memdp = NULL;
• efi_status_t ret;
• struct efi_loaded_image_obj *image_obj = NULL; struct efi_loaded_image *loaded_image_info = NULL;
• /*

• * Special case for efi payload not loaded from disk, such as
  

• * 'bootefi hello' or for example payload loaded directly into
  

• * memory via JTAG, etc:
  

• */
  

• if (!device_path && !image_path) {

• printf("WARNING: using memory device/image path, this may confuse some payloads!\n");
  

• /* actual addresses filled in after efi_load_pe() */
  

• memdp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, 0, 0);
  

• device_path = image_path = memdp;
  

• /*
  

•  * Grub expects that the device path of the loaded image is
  

•  * installed on a handle.
  

•  */
  

• ret = efi_create_handle(&mem_handle);
  

• if (ret != EFI_SUCCESS)
  

• 	return ret; /* TODO: leaks device_path */
  

• ret = efi_add_protocol(mem_handle, &efi_guid_device_path,
  

• 		       device_path);
  

• if (ret != EFI_SUCCESS)
  

• 	goto err_add_protocol;
  

• } else {

• assert(device_path && image_path);
  

• }
• ret = efi_setup_loaded_image(device_path, image_path, &image_obj,

• 		     &loaded_image_info);
  

• if (ret)

• goto err_prepare;
  

• efi_status_t ret;

  /* Transfer environment variable as load options */

• set_load_options(loaded_image_info, "bootargs");

In set_load_options() an error could occur (out of memory). So I think it should return a status.

I didn't change anything in set_load_options(), but I will follow your comment.

...

...

• /* Load the EFI payload */
• ret = efi_load_pe(image_obj, efi, loaded_image_info);

• ret = EFI_CALL(systab.boottime->open_protocol(

• 			handle,
  

• 			&efi_guid_loaded_image,
  

• 			(void **)&loaded_image_info,
  

• 			NULL, NULL,
  

• 			EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL));
  

Shouldn't we move this to set_load_options()?

No. This line has nothing to do with "load options."

Wrong, I will follow your comment. This change, however, uncovers one issue: Who is responsible for freeing loaded_image_info->load_option, particularly, when efi_exit()/unload_image() is fully implemented? In such a case, we have no chance to access this handle, hence loaded_image_info, after returning from efi_start_image().

Thanks, -Takahiro Akashi

...

...

if (ret != EFI_SUCCESS)

• goto err_prepare;
  

• if (memdp) {

• struct efi_device_path_memory *mdp = (void *)memdp;
  

• mdp->memory_type = loaded_image_info->image_code_type;
  

• mdp->start_address = (uintptr_t)loaded_image_info->image_base;
  

• mdp->end_address = mdp->start_address +
  

• 		loaded_image_info->image_size;
  

• }

• goto err;
  

• set_load_options(loaded_image_info, "bootargs");

  /* we don't support much: */ env_set("efi_8be4df61-93ca-11d2-aa0d-00e098032b8c_OsIndicationsSupported", "{ro,boot}(blob)0000000000000000");

Shouldn't this move to efi_setup.c? Probably we should also set OsIndications. I would prefer using efi_set_variable(). I think moving this could be done in a preparatory patch.

Yeah, I am aware of this issue. Will fix in a separate patch.

...

...

/* Call our payload! */

• debug("%s: Jumping to 0x%p\n", __func__, image_obj->entry);
• ret = EFI_CALL(efi_start_image(&image_obj->header, NULL, NULL));

• ret = EFI_CALL(efi_start_image(handle, NULL, NULL));

-err_prepare:

• /* image has returned, loaded-image obj goes *poof*: */ efi_restore_gd(); free(loaded_image_info->load_options);
• efi_delete_handle(&image_obj->header);

-err_add_protocol:

• if (mem_handle)

• efi_delete_handle(mem_handle);
  
  

+err: return ret; }

@@ -317,8 +268,7 @@ err_add_protocol: */ static int do_efibootmgr(const char *fdt_opt) {

• struct efi_device_path *device_path, *file_path;
• void *addr;

• efi_handle_t handle; efi_status_t ret;

  /* Allow unaligned memory access */

@@ -340,19 +290,19 @@ static int do_efibootmgr(const char *fdt_opt) else if (ret != EFI_SUCCESS) return CMD_RET_FAILURE;

• addr = efi_bootmgr_load(&device_path, &file_path);
• if (!addr)

• return 1;
  

• ret = efi_bootmgr_load(&handle);
• if (ret != EFI_SUCCESS) {

• printf("EFI boot manager: Cannot load any image\n");
  

• return CMD_RET_FAILURE;
  

• }

• printf("## Starting EFI application at %p ...\n", addr);
• ret = do_bootefi_exec(addr, device_path, file_path);
• printf("## Application terminated, r = %lu\n",

•       ret & ~EFI_ERROR_MASK);
  

• ret = do_bootefi_exec(handle);

• printf("## Application terminated, r = %lu\n", ret & ~EFI_ERROR_MASK);

  if (ret != EFI_SUCCESS)

• return 1;
  

• return CMD_RET_FAILURE;
  
  

• return 0;

• return CMD_RET_SUCCESS;

}

/* @@ -367,10 +317,14 @@ static int do_efibootmgr(const char *fdt_opt) */ static int do_boot_efi(const char *image_opt, const char *fdt_opt) {

• unsigned long addr;
• char *saddr;

• void *image_buf;
• struct efi_device_path *device_path, *image_path;
• struct efi_device_path *memdp = NULL, *file_path = NULL;
• const char *saddr;
• unsigned long addr, size;
• const char *size_str;
• efi_handle_t mem_handle = NULL, handle; efi_status_t ret;

• unsigned long fdt_addr;

  /* Allow unaligned memory access */ allow_unaligned();

@@ -392,36 +346,94 @@ static int do_boot_efi(const char *image_opt, const char *fdt_opt) return CMD_RET_FAILURE;

#ifdef CONFIG_CMD_BOOTEFI_HELLO

• if (!strcmp(argv[1], "hello")) {

• ulong size = __efi_helloworld_end - __efi_helloworld_begin;
  

• if (!strcmp(image_opt, "hello")) { saddr = env_get("loadaddr");

• size = __efi_helloworld_end - __efi_helloworld_begin;
  

• if (saddr) addr = simple_strtoul(saddr, NULL, 16); else addr = CONFIG_SYS_LOAD_ADDR;

• memcpy(map_sysmem(addr, size), __efi_helloworld_begin, size);
  

• image_buf = map_sysmem(addr, size);
  

• memcpy(image_buf, __efi_helloworld_begin, size);
  

• device_path = NULL;
  

• image_path = NULL;
  

  } else

#endif {

• saddr = argv[1];
  

• saddr = image_opt;
  

• size_str = env_get("filesize");
  

• if (size_str)
  

• 	size = simple_strtoul(size_str, NULL, 16);
  

• else
  

• 	size = 0;
  
  

  addr = simple_strtoul(saddr, NULL, 16); /* Check that a numeric value was passed */ if (!addr && *saddr != '0') return CMD_RET_USAGE;

• image_buf = map_sysmem(addr, size);
  

• device_path = bootefi_device_path;
  

• image_path = bootefi_image_path;
  

  }

• printf("## Starting EFI application at %08lx ...\n", addr);
• ret = do_bootefi_exec(map_sysmem(addr, 0), bootefi_device_path,

• 	      bootefi_image_path);
  

• printf("## Application terminated, r = %lu\n",

•       ret & ~EFI_ERROR_MASK);
  

• if (!device_path && !image_path) {

• /*
  

•  * Special case for efi payload not loaded from disk,
  

•  * such as 'bootefi hello' or for example payload
  

•  * loaded directly into memory via JTAG, etc:
  

•  */
  

• printf("WARNING: using memory device/image path, this may confuse some payloads!\n");
  

The EFI spec says that either of SourceBuffer or DevicePath may be NULL when calling LoadImage().

So are you suggesting we should remove this message?

...

...

• /* actual addresses filled in after efi_load_image() */
  

• memdp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
  

• 			(uint64_t)image_buf, size);
  

• file_path = memdp; /* append(memdp, NULL) */
  

• /*
  

•  * Make sure that device for device_path exist
  

•  * in load_image(). Otherwise, shell and grub will fail.
  

GRUB will fail anyway because it cannot determine the disk from which it was loaded. So why are we doing this?

I will comment on another reply.

...

...

•  */
  

• ret = efi_create_handle(&mem_handle);
  

• if (ret != EFI_SUCCESS)
  

• 	/* TODO: leaks device_path */
  

• 	goto err_add_protocol;
  

• ret = efi_add_protocol(mem_handle, &efi_guid_device_path,
  

• 		       memdp);
  

Couldn't we as well use the device path of the root node as "file_path"?

I don't get you point very well, but it will depend on how we should fix a grub issue mentioned above.

...

...

• if (ret != EFI_SUCCESS)
  

• 	goto err_add_protocol;
  

• } else {

• assert(device_path && image_path);
  

• file_path = efi_dp_append(device_path, image_path);
  

Where is file_path freed?

In this function. Will fix.

...

...

• }
• ret = EFI_CALL(efi_load_image(false, efi_root,

• 		      file_path, image_buf, size, &handle));
  

• if (ret != EFI_SUCCESS)

• goto err_prepare;
  

• ret = do_bootefi_exec(handle);
• printf("## Application terminated, r = %lu\n", ret & ~EFI_ERROR_MASK);

+err_prepare:

• if (device_path)

• efi_free_pool(file_path);
  

+err_add_protocol:

• if (mem_handle)

• efi_delete_handle(mem_handle);
  

• if (memdp)

• efi_free_pool(memdp);
  
  

  if (ret != EFI_SUCCESS) return CMD_RET_FAILURE;

• else

• return CMD_RET_SUCCESS;
  

• return CMD_RET_SUCCESS;

}

#ifdef CONFIG_CMD_BOOTEFI_SELFTEST @@ -581,7 +593,7 @@ static char bootefi_help_text[] = " Use environment variable efi_selftest to select a single test.\n" " Use 'setenv efi_selftest list' to enumerate all tests.\n" #endif

• "bootefi bootmgr [fdt addr]\n"

• "bootefi bootmgr [fdt address]\n" " - load and boot EFI payload based on BootOrder/BootXXXX variables.\n" "\n" " If specified, the device tree located at <fdt address> gets\n"

@@ -606,6 +618,13 @@ void efi_set_bootdev(const char *dev, const char *devnr, const char *path) ret = efi_dp_from_name(dev, devnr, path, &device, &image); if (ret == EFI_SUCCESS) { bootefi_device_path = device;

• if (image) {
  

• 	/* FIXME: image should not contain device */
  

• 	struct efi_device_path *image_tmp = image;
  

• 	efi_dp_split_file_path(image, &device, &image);
  

• 	efi_free_pool(image_tmp);
  

• }
  

  bootefi_image_path = image; } else { bootefi_device_path = NULL;

diff --git a/include/efi_loader.h b/include/efi_loader.h index d524dc7e24c1..c3eda2bb05d7 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -408,8 +408,6 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path, struct efi_device_path *file_path, struct efi_loaded_image_obj **handle_ptr, struct efi_loaded_image **info_ptr); -efi_status_t efi_load_image_from_path(struct efi_device_path *file_path,

• 		      void **buffer, efi_uintn_t *size);
  

/* Print information about all loaded images */ void efi_print_image_infos(void *pc);

@@ -563,8 +561,7 @@ struct efi_load_option {

void efi_deserialize_load_option(struct efi_load_option *lo, u8 *data); unsigned long efi_serialize_load_option(struct efi_load_option *lo, u8 **data); -void *efi_bootmgr_load(struct efi_device_path **device_path,

•        struct efi_device_path **file_path);
  

+efi_status_t efi_bootmgr_load(efi_handle_t *handle);

#else /* CONFIG_IS_ENABLED(EFI_LOADER) */

diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c index 4fccadc5483d..13ec79b2098b 100644 --- a/lib/efi_loader/efi_bootmgr.c +++ b/lib/efi_loader/efi_bootmgr.c @@ -120,14 +120,14 @@ static void *get_var(u16 *name, const efi_guid_t *vendor,

• if successful. This checks that the EFI_LOAD_OPTION is active (enabled)
• and that the specified file to boot exists.

*/ -static void *try_load_entry(uint16_t n, struct efi_device_path **device_path,

• 	    struct efi_device_path **file_path)
  

+static efi_status_t try_load_entry(u16 n, efi_handle_t *handle) { struct efi_load_option lo; u16 varname[] = L"Boot0000"; u16 hexmap[] = L"0123456789ABCDEF";

• void *load_option, *image = NULL;

• void *load_option; efi_uintn_t size;

• efi_status_t ret;

  varname[4] = hexmap[(n & 0xf000) >> 12]; varname[5] = hexmap[(n & 0x0f00) >> 8];

@@ -136,19 +136,19 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path,

load_option = get_var(varname, &efi_global_variable_guid, &size); if (!load_option)

• return NULL;
  

• return EFI_LOAD_ERROR;
  
  

  efi_deserialize_load_option(&lo, load_option);

  if (lo.attributes & LOAD_OPTION_ACTIVE) { u32 attributes;

• efi_status_t ret;
  
  

  debug("%s: trying to load "%ls" from %pD\n", __func__, lo.label, lo.file_path);

• ret = efi_load_image_from_path(lo.file_path, &image, &size);
  

• ret = EFI_CALL(efi_load_image(false, (void *)0x1 /* FIXME */,
  

• 			      lo.file_path, NULL, 0,
  

• 			      handle));
  

  if (ret != EFI_SUCCESS) goto error;

@@ -159,17 +159,22 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path, L"BootCurrent", (efi_guid_t *)&efi_global_variable_guid, attributes, size, &n));

• if (ret != EFI_SUCCESS)
  

• if (ret != EFI_SUCCESS) {
  

• 	if (EFI_CALL(efi_unload_image(*handle))
  

• 	    != EFI_SUCCESS)
  

• 		printf("Unloading image failed\n");
  goto error;
  

• }
  
  

  printf("Booting: %ls\n", lo.label);

• efi_dp_split_file_path(lo.file_path, device_path, file_path);
  

• } else {

• ret = EFI_LOAD_ERROR;
  

  }

error: free(load_option);

• return image;

• return ret;

}

/* @@ -177,12 +182,10 @@ error:

• EFI variable, the available load-options, finding and returning
• the first one that can be loaded successfully.

*/ -void *efi_bootmgr_load(struct efi_device_path **device_path,

•        struct efi_device_path **file_path)
  

+efi_status_t efi_bootmgr_load(efi_handle_t *handle) { u16 bootnext, *bootorder; efi_uintn_t size;

• void *image = NULL; int i, num; efi_status_t ret;

@@ -209,10 +212,9 @@ void *efi_bootmgr_load(struct efi_device_path **device_path, /* load BootNext */ if (ret == EFI_SUCCESS) { if (size == sizeof(u16)) {

• 		image = try_load_entry(bootnext, device_path,
  

• 				       file_path);
  

• 		if (image)
  

• 			return image;
  

• 		ret = try_load_entry(bootnext, handle);
  

• 		if (ret == EFI_SUCCESS)
  

• 			return ret;
  }
  

  } else { printf("Deleting BootNext failed\n");

@@ -223,19 +225,20 @@ void *efi_bootmgr_load(struct efi_device_path **device_path, bootorder = get_var(L"BootOrder", &efi_global_variable_guid, &size); if (!bootorder) { printf("BootOrder not defined\n");

• ret = EFI_NOT_FOUND;
  

  goto error; }

  num = size / sizeof(uint16_t); for (i = 0; i < num; i++) { debug("%s: trying to load Boot%04X\n", __func__, bootorder[i]);

• image = try_load_entry(bootorder[i], device_path, file_path);
  

• if (image)
  

• ret = try_load_entry(bootorder[i], handle);
  

• if (ret == EFI_SUCCESS)
  break;
  

  }

  free(bootorder);

error:

• return image;

• return ret;

} diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index b215bd7723da..65425fabc08f 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -1611,6 +1611,7 @@ failure:

• @size: size of the loaded image
• Return: status code

*/ +static efi_status_t efi_load_image_from_path(struct efi_device_path *file_path, void **buffer, efi_uintn_t *size) { @@ -2684,6 +2685,7 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle, }

current_image = image_handle;

• debug("EFI: Jumping into 0x%p\n", image_obj->entry);

Please, use EFI_PRINT() here.

Basically I agree, but there are lots of debug()'s in this file. We should replace them all at once later.

Thanks, -Takahiro Akashi

...

Best regards

Heinrich

...

ret = EFI_CALL(image_obj->entry(image_handle, &systab));

/*

On 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 */