On 3/27/19 5:40 AM, AKASHI Takahiro wrote:

To meet UEFI spec v2.7a section 9.2, we should add EFI_LOADED_IMAGE_DEVICE_PATH_PROTOCOL to image handle, instead of EFI_DEVICE_PATH_PROTOCOL.

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

include/efi_api.h | 4 ++++ include/efi_loader.h | 1 + lib/efi_loader/efi_boottime.c | 19 ++++++++++++------- lib/efi_loader/efi_image_loader.c | 2 ++ 4 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/include/efi_api.h b/include/efi_api.h index ccf608653d4a..63b703c951a7 100644 --- a/include/efi_api.h +++ b/include/efi_api.h @@ -333,6 +333,10 @@ struct efi_system_table { EFI_GUID(0x5b1b31a1, 0x9562, 0x11d2, \ 0x8e, 0x3f, 0x00, 0xa0, 0xc9, 0x69, 0x72, 0x3b)

+#define LOADED_IMAGE_DEVICE_PATH_GUID \

• EFI_GUID(0xbc62157e, 0x3e33, 0x4fec, \

•  0x99, 0x20, 0x2d, 0x3b, 0x36, 0xd7, 0x50, 0xdf)
  

#define EFI_LOADED_IMAGE_PROTOCOL_REVISION 0x1000

struct efi_loaded_image { diff --git a/include/efi_loader.h b/include/efi_loader.h index 512880ab8fbf..3f54e354bdcd 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -135,6 +135,7 @@ extern const efi_guid_t efi_guid_event_group_reset_system; /* GUID of the device tree table */ extern const efi_guid_t efi_guid_fdt; extern const efi_guid_t efi_guid_loaded_image; +extern const efi_guid_t efi_guid_loaded_image_device_path; extern const efi_guid_t efi_guid_device_path_to_text_protocol; extern const efi_guid_t efi_simple_file_system_protocol_guid; extern const efi_guid_t efi_file_info_guid; diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 391681260c27..578b32a05ffd 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -1519,6 +1519,7 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path, efi_status_t ret; struct efi_loaded_image *info = NULL; struct efi_loaded_image_obj *obj = NULL;

• struct efi_device_path *dp;

  /* In case of EFI_OUT_OF_RESOURCES avoid illegal free by caller. */ *handle_ptr = NULL;

@@ -1542,15 +1543,19 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path,

if (device_path) { info->device_handle = efi_dp_find_obj(device_path, NULL);

• /*
  

•  * When asking for the device path interface, return
  

•  * bootefi_device_path
  

•  */
  

• ret = efi_add_protocol(&obj->header,
  

• 		       &efi_guid_device_path, device_path);
  

• if (ret != EFI_SUCCESS)
  

• dp = efi_dp_append(device_path, file_path);
  

I think we should pass the original device_path as a parameter to efi_setup_loaded_image instead of first splitting and then recombining devicepath elements.

But let's leave this as a TODO after the rest of the refactoring.

• if (!dp) {
  

• 	ret = EFI_OUT_OF_RESOURCES;
  goto failure;
  

• }
  

• } else {

• dp = NULL;
  

  }
• ret = efi_add_protocol(&obj->header,

• 	       &efi_guid_loaded_image_device_path, dp);
  

I wondered about the loaded imaged device path protocol possibly being NULL. But the spec explicitely states:

It is legal to call LoadImage() for a buffer in memory with a NULL DevicePath parameter. In this case, the Loaded Image Device Path Protocol is installed with a NULL interface pointer.

Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de

• if (ret != EFI_SUCCESS)

• goto failure;
  
  

  /*

  • When asking for the loaded_image interface, just

diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c index fe66e7b9ffe1..c6177b9ff832 100644 --- a/lib/efi_loader/efi_image_loader.c +++ b/lib/efi_loader/efi_image_loader.c @@ -14,6 +14,8 @@ const efi_guid_t efi_global_variable_guid = EFI_GLOBAL_VARIABLE_GUID; const efi_guid_t efi_guid_device_path = DEVICE_PATH_GUID; const efi_guid_t efi_guid_loaded_image = LOADED_IMAGE_GUID; +const efi_guid_t efi_guid_loaded_image_device_path

• = LOADED_IMAGE_DEVICE_PATH_GUID;
  

const efi_guid_t efi_simple_file_system_protocol_guid = EFI_SIMPLE_FILE_SYSTEM_PROTOCOL_GUID; const efi_guid_t efi_file_info_guid = EFI_FILE_INFO_GUID;

On Wed, Mar 27, 2019 at 06:58:31AM +0100, Heinrich Schuchardt wrote:

On 3/27/19 5:40 AM, AKASHI Takahiro wrote:

...

To meet UEFI spec v2.7a section 9.2, we should add EFI_LOADED_IMAGE_DEVICE_PATH_PROTOCOL to image handle, instead of EFI_DEVICE_PATH_PROTOCOL.

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

include/efi_api.h | 4 ++++ include/efi_loader.h | 1 + lib/efi_loader/efi_boottime.c | 19 ++++++++++++------- lib/efi_loader/efi_image_loader.c | 2 ++ 4 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/include/efi_api.h b/include/efi_api.h index ccf608653d4a..63b703c951a7 100644 --- a/include/efi_api.h +++ b/include/efi_api.h @@ -333,6 +333,10 @@ struct efi_system_table { EFI_GUID(0x5b1b31a1, 0x9562, 0x11d2, \ 0x8e, 0x3f, 0x00, 0xa0, 0xc9, 0x69, 0x72, 0x3b)

+#define LOADED_IMAGE_DEVICE_PATH_GUID \

• EFI_GUID(0xbc62157e, 0x3e33, 0x4fec, \

•  0x99, 0x20, 0x2d, 0x3b, 0x36, 0xd7, 0x50, 0xdf)
  

#define EFI_LOADED_IMAGE_PROTOCOL_REVISION 0x1000

struct efi_loaded_image { diff --git a/include/efi_loader.h b/include/efi_loader.h index 512880ab8fbf..3f54e354bdcd 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -135,6 +135,7 @@ extern const efi_guid_t efi_guid_event_group_reset_system; /* GUID of the device tree table */ extern const efi_guid_t efi_guid_fdt; extern const efi_guid_t efi_guid_loaded_image; +extern const efi_guid_t efi_guid_loaded_image_device_path; extern const efi_guid_t efi_guid_device_path_to_text_protocol; extern const efi_guid_t efi_simple_file_system_protocol_guid; extern const efi_guid_t efi_file_info_guid; diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 391681260c27..578b32a05ffd 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -1519,6 +1519,7 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path, efi_status_t ret; struct efi_loaded_image *info = NULL; struct efi_loaded_image_obj *obj = NULL;

• struct efi_device_path *dp;

  /* In case of EFI_OUT_OF_RESOURCES avoid illegal free by caller. */ *handle_ptr = NULL;

@@ -1542,15 +1543,19 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path,

if (device_path) { info->device_handle = efi_dp_find_obj(device_path, NULL);

• /*
  

•  * When asking for the device path interface, return
  

•  * bootefi_device_path
  

•  */
  

• ret = efi_add_protocol(&obj->header,
  

• 		       &efi_guid_device_path, device_path);
  

• if (ret != EFI_SUCCESS)
  

• dp = efi_dp_append(device_path, file_path);
  

I think we should pass the original device_path as a parameter to efi_setup_loaded_image instead of first splitting and then recombining devicepath elements.

Agree, but I didn't modify the code because efi_setup_loaded_image() is still used to load "efi_selftest" in bootefi.

I hope that you will work on do_efi_selftest().

But let's leave this as a TODO after the rest of the refactoring.

...

• if (!dp) {
  

• 	ret = EFI_OUT_OF_RESOURCES;
  goto failure;
  

• }
  

• } else {

• dp = NULL;
  

  }
• ret = efi_add_protocol(&obj->header,

• 	       &efi_guid_loaded_image_device_path, dp);
  

I wondered about the loaded imaged device path protocol possibly being NULL. But the spec explicitely states:

It is legal to call LoadImage() for a buffer in memory with a NULL DevicePath parameter. In this case, the Loaded Image Device Path Protocol is installed with a NULL interface pointer.

Right, I have also found this statement.

-Takahiro Akashi

Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de

...

• if (ret != EFI_SUCCESS)

• goto failure;
  
  

  /*

  • When asking for the loaded_image interface, just

diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c index fe66e7b9ffe1..c6177b9ff832 100644 --- a/lib/efi_loader/efi_image_loader.c +++ b/lib/efi_loader/efi_image_loader.c @@ -14,6 +14,8 @@ const efi_guid_t efi_global_variable_guid = EFI_GLOBAL_VARIABLE_GUID; const efi_guid_t efi_guid_device_path = DEVICE_PATH_GUID; const efi_guid_t efi_guid_loaded_image = LOADED_IMAGE_GUID; +const efi_guid_t efi_guid_loaded_image_device_path

• = LOADED_IMAGE_DEVICE_PATH_GUID;
  

const efi_guid_t efi_simple_file_system_protocol_guid = EFI_SIMPLE_FILE_SYSTEM_PROTOCOL_GUID; const efi_guid_t efi_file_info_guid = EFI_FILE_INFO_GUID;

On 3/27/19 5:40 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.

As already mentioned in a comment to patch 1/11 I would prefer that we pass the original device path to efi_setup_loaded_image() instead of recombining device path and file path there again.

This would avoid special treatment here with possible side effects in efi_fs_from_path().

Best regards

Heinrich

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

On Wed, Mar 27, 2019 at 07:29:50AM +0100, Heinrich Schuchardt wrote:

On 3/27/19 5:40 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

cmd/bootefi.c | 53 ++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 38 insertions(+), 15 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 3619a20e6433..aba8b203d419 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -198,6 +198,40 @@ 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: CMD_RET_SUCCESS on success,

• CMD_RET_USAGE or CMD_RET_FAILURE otherwise
  

This does not match the actual return values.

Oops, re-organizing my patch-set introduced this obvious bug.

...

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

• printf("WARNING: booting without device tree\n");
  

As x86 does not require a device tree, please, remove this message.

Okay, but I didn't change this message from the original :)

...

• }
• 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 +441,10 @@ 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_SUCCESS)

• return r;
  

Now you return EFI_INVALID_PARAMETER instead of CMD_RET_USAGE.

Okay, will fix.

Thanks, -Takahiro Akashi

Best regards

Heinrich

...

#ifdef CONFIG_CMD_BOOTEFI_HELLO if (!strcmp(argv[1], "hello")) { ulong size = __efi_helloworld_end - __efi_helloworld_begin;

On 3/27/19 5:40 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 aba8b203d419..ce0db07f8633 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -165,41 +165,8 @@ 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: CMD_RET_SUCCESS on success, CMD_RET_USAGE or CMD_RET_FAILURE otherwise

@@ -207,21 +174,43 @@ static efi_status_t efi_install_fdt(ulong fdt_addr)

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

@@ -441,7 +430,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_SUCCESS) return r;

We want to return CMD_RET_* from do_bootefi() and not EFI_*.

Best regards

Heinrich

On Wed, Mar 27, 2019 at 07:45:36AM +0100, Heinrich Schuchardt wrote:

On 3/27/19 5:40 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 | 145 +++++++++++++++++++++++++++++++------------------- 1 file changed, 90 insertions(+), 55 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c index ce0db07f8633..1267d895472c 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -221,39 +221,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

@@ -300,11 +267,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)

@@ -328,7 +298,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);

Deletion of the image and the handle should be handled by Exit() or UnloadImage(). But maybe we should leave that to correction to a later patch. Please, add a TODO comment here.

I don't think we need a comment here. Those code will be eventually replaced by efi_unload_image() in patch#10.

...

err_add_protocol: if (mem_handle) @@ -338,6 +310,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

@@ -383,6 +374,62 @@ 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_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_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();

This looks like code duplication to me.

Another "carve out" patch?

I would prefer do_bootefi() to call efi_init_obj_list() before evaluating arguments.

Well, we won't do that so as to make do_efibootmgr() self-contained in a later patch.

-Takahiro Akashi

Best regards

Heinrich

...

• 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_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) @@ -414,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();

@@ -427,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_SUCCESS) return r;

@@ -445,22 +496,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 3/27/19 5:40 AM, AKASHI Takahiro wrote:

This is a preparatory patch for reworking do_bootefi() in later patch.

I would suggest as commit message:

Move do_bootefi_bootmgr_exec() up in code to avoid a forward declaration in a later patch.

Otherwise

Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de

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 1267d895472c..e9d4881997a1 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -309,6 +309,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 Fri, Apr 12, 2019 at 07:55:16AM +0200, Heinrich Schuchardt wrote:

On 3/27/19 5:40 AM, AKASHI Takahiro wrote:

...

This is a preparatory patch for reworking do_bootefi() in later patch. do_bootmgr_exec() is renamed to do_efibootmgr() as we put all the necessary code into this function.

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

cmd/bootefi.c | 42 ++++++++++++++++++++++++++++++++++-------- 1 file changed, 34 insertions(+), 8 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c index e9d4881997a1..94b5bdeed3f1 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -309,22 +309,47 @@ 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();

Shouldn't we move these two call to efi_init_obj_list()?

Given the fact that efi_init_obj_list() is called without invoking any UEFI binary at some places, I'm not sure that it is the right place where switch_to_non_secure_mode() be called.

-Takahiro Akashi

Best regards

Heinrich

...

• /* 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_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 +488,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); @@ -497,9 +525,7 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) memcpy(map_sysmem(addr, size), __efi_helloworld_begin, size); } else #endif

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

• return do_bootefi_bootmgr_exec();
  

• } else {

• { saddr = argv[1];

  addr = simple_strtoul(saddr, NULL, 16);

On Fri, Apr 12, 2019 at 10:58:25AM +0200, Heinrich Schuchardt wrote:

On 4/12/19 9:06 AM, AKASHI Takahiro wrote:

...

On Fri, Apr 12, 2019 at 07:55:16AM +0200, Heinrich Schuchardt wrote:

...

On 3/27/19 5:40 AM, AKASHI Takahiro wrote:

...

This is a preparatory patch for reworking do_bootefi() in later patch. do_bootmgr_exec() is renamed to do_efibootmgr() as we put all the necessary code into this function.

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

cmd/bootefi.c | 42 ++++++++++++++++++++++++++++++++++-------- 1 file changed, 34 insertions(+), 8 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c index e9d4881997a1..94b5bdeed3f1 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -309,22 +309,47 @@ 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();

Shouldn't we move these two call to efi_init_obj_list()?

Given the fact that efi_init_obj_list() is called without invoking any UEFI binary at some places, I'm not sure that it is the right place where switch_to_non_secure_mode() be called.

If this is UEFI(and arm)-specific, it should be placed just before setjmp() in efi_start_image().

But I'm not sure whether we should call switch_to_non_secure_mode() even for a driver, which is logically part of U-Boot UEFI.

-Takahiro Akashi

I think this could even be done in initr_reloc_global_data().

@Alex What are your thoughts.

Best regards

Heinrich

...

-Takahiro Akashi

...

Best regards

Heinrich

...

• /* 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_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 +488,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); @@ -497,9 +525,7 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) memcpy(map_sysmem(addr, size), __efi_helloworld_begin, size); } else #endif

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

• return do_bootefi_bootmgr_exec();
  

• } else {

• { saddr = argv[1];

  addr = simple_strtoul(saddr, NULL, 16);

On Fri, Apr 12, 2019 at 10:28:09PM +0200, Heinrich Schuchardt wrote:

On 4/12/19 4:19 PM, AKASHI Takahiro wrote:

...

On Fri, Apr 12, 2019 at 10:58:25AM +0200, Heinrich Schuchardt wrote:

...

On 4/12/19 9:06 AM, AKASHI Takahiro wrote:

...

On Fri, Apr 12, 2019 at 07:55:16AM +0200, Heinrich Schuchardt wrote:

...

On 3/27/19 5:40 AM, AKASHI Takahiro wrote:

...

This is a preparatory patch for reworking do_bootefi() in later patch. do_bootmgr_exec() is renamed to do_efibootmgr() as we put all the necessary code into this function.

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

cmd/bootefi.c | 42 ++++++++++++++++++++++++++++++++++-------- 1 file changed, 34 insertions(+), 8 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c index e9d4881997a1..94b5bdeed3f1 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -309,22 +309,47 @@ 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();

Shouldn't we move these two call to efi_init_obj_list()?

Given the fact that efi_init_obj_list() is called without invoking any UEFI binary at some places, I'm not sure that it is the right place where switch_to_non_secure_mode() be called.

If this is UEFI(and arm)-specific, it should be placed just before setjmp() in efi_start_image().

But I'm not sure whether we should call switch_to_non_secure_mode() even for a driver, which is logically part of U-Boot UEFI.

switch_to_not_secure_mode() is a weak function which is implemented only for armv7 and armv8.

efi_init_obj_list() would be a safe place to call it.

You also suggested initr_reloc_global_data(). No?

Anyhow, I think you ignored my concern:

...

But I'm not sure whether we should call switch_to_non_secure_mode() even for a driver, which is logically part of U-Boot UEFI.

Since I think that we need discuss more, I will leave the code unchanged in the next version.

-Takahiro Akashi

Best regards

Heinrich

...

-Takahiro Akashi

...

I think this could even be done in initr_reloc_global_data().

@Alex What are your thoughts.

Best regards

Heinrich

...

-Takahiro Akashi

...

Best regards

Heinrich

...

• /* 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_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 +488,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); @@ -497,9 +525,7 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) memcpy(map_sysmem(addr, size), __efi_helloworld_begin, size); } else #endif

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

• return do_bootefi_bootmgr_exec();
  

• } else {

• { saddr = argv[1];

  addr = simple_strtoul(saddr, NULL, 16);

On 3/27/19 5:40 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 | 122 ++++++++++++++++++++++++++++---------------------- 1 file changed, 68 insertions(+), 54 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 94b5bdeed3f1..39a0712af6ad 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -355,6 +355,73 @@ 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();

The two call above should be moved to efi_init_obj_list().

• /* 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_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);

Shouldn't this be debug() in efi_start_image()?

Best regards

Heinrich

• 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,

@@ -481,11 +548,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;

@@ -496,55 +558,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_SUCCESS)

• return r;
  

-#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/12/19 9:22 AM, AKASHI Takahiro wrote:

On Fri, Apr 12, 2019 at 07:59:51AM +0200, Heinrich Schuchardt wrote:

...

On 3/27/19 5:40 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 | 122 ++++++++++++++++++++++++++++---------------------- 1 file changed, 68 insertions(+), 54 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 94b5bdeed3f1..39a0712af6ad 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -355,6 +355,73 @@ 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();

The two call above should be moved to efi_init_obj_list().

Same comment as in patch#8/11

...

...

• /* 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_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);

Shouldn't this be debug() in efi_start_image()?

Okay, and application -> image # but I'm not sure that this information is useful.

When debugging I would be more interested in the address to which the image is loaded so that I have the offset at hand to load the text symbols into gdb.

I am happy if you simply drop this entry address output.

Regards

Heinrich

-Takahiro Akashi

...

Best regards

Heinrich

...

• 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, @@ -481,11 +548,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;

@@ -496,55 +558,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_SUCCESS)

• return r;
  

-#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 3/27/19 5:40 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 | 199 +++++++++++++++++++--------------- include/efi_loader.h | 5 +- lib/efi_loader/efi_bootmgr.c | 43 ++++---- lib/efi_loader/efi_boottime.c | 1 + 4 files changed, 138 insertions(+), 110 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 39a0712af6ad..17dccd718008 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -224,88 +224,43 @@ 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! */

• image_obj = container_of(handle, struct efi_loaded_image_obj, header);

This is not needed, if we remove the debug statement.

debug("%s: Jumping to 0x%p\n", __func__, image_obj->entry);

That line should go to StartImage() if needed. Please, drop it here.

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

• efi_free_pool(loaded_image_info);

Wouldn't this be done by unload_image()?

+err: return ret; }

@@ -319,8 +274,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 +294,21 @@ static int do_efibootmgr(const char *fdt_opt) 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);
• EFI_CALL(efi_unload_image(handle));

Only applications have to be unloaded, not drivers. Unloading is to be handled by exit(), see UEFI spec.

if (ret != EFI_SUCCESS)

• return 1;
  

• return CMD_RET_FAILURE;
  
  

• return 0;

• return CMD_RET_SUCCESS; }

  /*

@@ -367,10 +323,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();

@@ -390,36 +350,96 @@ 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;
  

• }

• 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, (void *)0x1 /* FIXME */,

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

• if (ret != EFI_SUCCESS)

• goto err_prepare;
  

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

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

Why should we need all this duplicate code. Cant we move that to do_bootefi_exec()?

• EFI_CALL(efi_unload_image(handle));

That is the task of efi_exit().

Best regards

Heinrich

+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

@@ -577,7 +597,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"

@@ -602,6 +622,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 6dfa2fef3cf0..6c09a7f40d02 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -410,8 +410,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);

@@ -565,8 +563,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 74da6b01054a..8e2fa0111591 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -1610,6 +1610,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) {

On 4/12/19 10:38 AM, AKASHI Takahiro wrote:

On Fri, Apr 12, 2019 at 07:53:25AM +0200, Heinrich Schuchardt wrote:

...

On 3/27/19 5:40 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 | 199 +++++++++++++++++++--------------- include/efi_loader.h | 5 +- lib/efi_loader/efi_bootmgr.c | 43 ++++---- lib/efi_loader/efi_boottime.c | 1 + 4 files changed, 138 insertions(+), 110 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 39a0712af6ad..17dccd718008 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -224,88 +224,43 @@ 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! */

• image_obj = container_of(handle, struct efi_loaded_image_obj, header);

This is not needed, if we remove the debug statement.

...

debug("%s: Jumping to 0x%p\n", __func__, image_obj->entry);

That line should go to StartImage() if needed. Please, drop it here.

...

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

• efi_free_pool(loaded_image_info);

Wouldn't this be done by unload_image()?

but we should not call unload_image(), efi_exit() does. Right? Yes, correct.

...

...

+err: return ret; }

@@ -319,8 +274,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 +294,21 @@ static int do_efibootmgr(const char *fdt_opt) 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);
• EFI_CALL(efi_unload_image(handle));

Only applications have to be unloaded, not drivers. Unloading is to be handled by exit(), see UEFI spec.

How do we know whether the loaded image is an application, or driver?

In efi_load_pe() we should evaluate the field opt->Subsystem describing the "Windows Subsystem". I once started on this but then I saw that cmd/bootefi.c needed to be cleaned up first:

https://github.com/xypron/u-boot-patches/blob/efi-next/0001-efi_loader-imple...

...

...

if (ret != EFI_SUCCESS)

• return 1;
  

• return CMD_RET_FAILURE;
  
  

• return 0;

• return CMD_RET_SUCCESS;

}

/* @@ -367,10 +323,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();

@@ -390,36 +350,96 @@ 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;
  

• }

• 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, (void *)0x1 /* FIXME */,

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

• if (ret != EFI_SUCCESS)

• goto err_prepare;
  

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

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

Why should we need all this duplicate code. Cant we move that to do_bootefi_exec()?

...

• EFI_CALL(efi_unload_image(handle));

That is the task of efi_exit().

Who should be blamed for reclaiming a handle which is created with load_image()? What if an application doesn't call efi_exit() by itself?

When the last protocol interface is deleted from a handle via UninstallProtocolInterface() the handle is deleted.

Best regards

Heinrich

-Takahiro Akashi

...

Best regards

Heinrich

...

+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 @@ -577,7 +597,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"

@@ -602,6 +622,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 6dfa2fef3cf0..6c09a7f40d02 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -410,8 +410,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);

@@ -565,8 +563,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 74da6b01054a..8e2fa0111591 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -1610,6 +1610,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) {

On 3/27/19 5:40 AM, AKASHI Takahiro wrote:

Add CONFIG_CMD_STANDALONE_EFIBOOTMGR. With this patch, EFI boot manager can be kicked in by a standalone command, efibootmgr.

I miss your comment form 0/11 here:

* 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.

Simply requiring that environment variable $fdtaddr, which is under user control, is used for loading the device tree does not offer any security at all.

For secure boot I would expect that the device tree has to be part of a signed binary and that that signature is checked.

If the user passes in a device tree that should be okay if it has the required signature.

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 4bcc5c45579d..aa27b21956b3 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 17dccd718008..2a2ff4034831 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -635,3 +635,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;
  

It is unclear why you do not allow the user to pass the location of the device tree as a parameter.

• fdt_opt = env_get("fdtaddr");

You are looking at some environment variable $fdtaddr while most boards use $fdt_addr_r as preferred address to load the device tree.

Best regards

Heinrich

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