[U-Boot] [RFC v3 05/10] cmd: bootefi: carve out efi_selftest code from do_bootefi()
AKASHI Takahiro
takahiro.akashi at linaro.org
Wed Apr 17 07:19:22 UTC 2019
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 at 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();
> >
>
More information about the U-Boot
mailing list