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.aka...@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();
> >
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
