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

[Heinrich Schuchardt]

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.

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

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