Re: [U-Boot] [RFC v3 05/10] cmd: bootefi: carve out efi_selftest code from do_bootefi()

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