[U-Boot,v12,5/6] efi: Create a function to set up for running EFI code

Message ID 20181106225744.139945-6-sjg@chromium.org
State New
Delegated to: Alexander Graf
Headers show
Series
  • efi_loader: Code refactoring and improvement
Related show

Commit Message

Simon Glass Nov. 6, 2018, 10:57 p.m.
There is still duplicated code in efi_loader for tests and normal
operation.

Add a new bootefi_run_prepare() function which holds common code used to
set up U-Boot to run EFI code. Make use of this from the existing
bootefi_test_prepare() function, as well as do_bootefi_exec().

Also shorten a few variable names.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

Changes in v12: None
Changes in v11: None
Changes in v9: None
Changes in v7: None
Changes in v5:
- Drop call to efi_init_obj_list() which is now done in do_bootefi()
- Introduce load_options_path to specifyc U-Boot env var for load_options_path

Changes in v4:
- Rebase to master

Changes in v3:
- Add patch to create a function to set up for running EFI code

 cmd/bootefi.c | 85 +++++++++++++++++++++++++++++----------------------
 1 file changed, 48 insertions(+), 37 deletions(-)

Comments

Alexander Graf Nov. 13, 2018, 8:21 p.m. | #1
On 06.11.18 23:57, Simon Glass wrote:
> There is still duplicated code in efi_loader for tests and normal
> operation.
> 
> Add a new bootefi_run_prepare() function which holds common code used to
> set up U-Boot to run EFI code. Make use of this from the existing
> bootefi_test_prepare() function, as well as do_bootefi_exec().
> 
> Also shorten a few variable names.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
> Changes in v12: None
> Changes in v11: None
> Changes in v9: None
> Changes in v7: None
> Changes in v5:
> - Drop call to efi_init_obj_list() which is now done in do_bootefi()
> - Introduce load_options_path to specifyc U-Boot env var for load_options_path
> 
> Changes in v4:
> - Rebase to master
> 
> Changes in v3:
> - Add patch to create a function to set up for running EFI code
> 
>  cmd/bootefi.c | 85 +++++++++++++++++++++++++++++----------------------
>  1 file changed, 48 insertions(+), 37 deletions(-)
> 
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 0dd18d594d5..779c1f63fa8 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -327,6 +327,30 @@ static efi_status_t efi_install_fdt(ulong fdt_addr)
>  	return ret;
>  }
>  
> +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 **imagep,
> +					struct efi_loaded_image_obj **objp)
> +{
> +	efi_status_t ret;
> +
> +	ret = efi_setup_loaded_image(device_path, image_path, objp, imagep);
> +	if (ret != EFI_SUCCESS)
> +		return ret;
> +
> +	/*
> +	 * gd lives in a fixed register which may get clobbered while we execute
> +	 * the payload. So save it here and restore it on every callback entry
> +	 */
> +	efi_save_gd();
> +
> +	/* Transfer environment variable as load options */
> +	set_load_options(*imagep, load_options_path);
> +
> +	return 0;
> +}
> +
>  /**
>   * do_bootefi_exec() - execute EFI binary
>   *
> @@ -345,8 +369,8 @@ static efi_status_t do_bootefi_exec(void *efi,
>  	efi_handle_t mem_handle = NULL;
>  	struct efi_device_path *memdp = NULL;
>  	efi_status_t ret;
> -	struct efi_loaded_image_obj *image_handle = NULL;

This is called image handle in the UEFI spec, so I'd prefer we keep that
name.

> -	struct efi_loaded_image *loaded_image_info = NULL;

Same here.

Also, as a general rule of thumb, it's not good practice to do renames
(something you could for example reproduce with sed or coccinelle) in
the same patch as something else - code movement in your case. It makes
review pretty hard and makes life harder on you to rebase the patch.

In a nutshell: I like the code movement into a function, that's a nice
cleanup. I don't like the variable renames :). They make it confusing
for people that want to know what these variables mean in context of an
executed application.


Alex

> +	struct efi_loaded_image_obj *obj = NULL;
> +	struct efi_loaded_image *image = NULL;
>  
>  	EFIAPI efi_status_t (*entry)(efi_handle_t image_handle,
>  				     struct efi_system_table *st);
> @@ -376,15 +400,13 @@ static efi_status_t do_bootefi_exec(void *efi,
>  		assert(device_path && image_path);
>  	}
>  
> -	ret = efi_setup_loaded_image(device_path, image_path, &image_handle,
> -				     &loaded_image_info);
> -	if (ret != EFI_SUCCESS)
> -		goto exit;
> +	ret = bootefi_run_prepare("bootargs", device_path, image_path,
> +				  &image, &obj);
> +	if (ret)
> +		return ret;
>  
> -	/* Transfer environment variable bootargs as load options */
> -	set_load_options(loaded_image_info, "bootargs");
>  	/* Load the EFI payload */
> -	entry = efi_load_pe(image_handle, efi, loaded_image_info);
> +	entry = efi_load_pe(obj, efi, image);
>  	if (!entry) {
>  		ret = EFI_LOAD_ERROR;
>  		goto exit;
> @@ -392,10 +414,9 @@ static efi_status_t do_bootefi_exec(void *efi,
>  
>  	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;
> +		mdp->memory_type = image->image_code_type;
> +		mdp->start_address = (uintptr_t)image->image_base;
> +		mdp->end_address = mdp->start_address + image->image_size;
>  	}
>  
>  	/* we don't support much: */
> @@ -405,8 +426,8 @@ static efi_status_t do_bootefi_exec(void *efi,
>  	/* Call our payload! */
>  	debug("%s:%d Jumping to 0x%lx\n", __func__, __LINE__, (long)entry);
>  
> -	if (setjmp(&image_handle->exit_jmp)) {
> -		ret = image_handle->exit_status;
> +	if (setjmp(&obj->exit_jmp)) {
> +		ret = obj->exit_status;
>  		goto exit;
>  	}
>  
> @@ -418,7 +439,7 @@ static efi_status_t do_bootefi_exec(void *efi,
>  
>  		/* Move into EL2 and keep running there */
>  		armv8_switch_to_el2((ulong)entry,
> -				    (ulong)image_handle,
> +				    (ulong)obj,
>  				    (ulong)&systab, 0, (ulong)efi_run_in_el2,
>  				    ES_TO_AARCH64);
>  
> @@ -435,7 +456,7 @@ static efi_status_t do_bootefi_exec(void *efi,
>  		secure_ram_addr(_do_nonsec_entry)(
>  					efi_run_in_hyp,
>  					(uintptr_t)entry,
> -					(uintptr_t)image_handle,
> +					(uintptr_t)obj,
>  					(uintptr_t)&systab);
>  
>  		/* Should never reach here, efi exits with longjmp */
> @@ -443,12 +464,12 @@ static efi_status_t do_bootefi_exec(void *efi,
>  	}
>  #endif
>  
> -	ret = efi_do_enter(image_handle, &systab, entry);
> +	ret = efi_do_enter(obj, &systab, entry);
>  
>  exit:
>  	/* image has returned, loaded-image obj goes *poof*: */
> -	if (image_handle)
> -		efi_delete_handle(&image_handle->parent);
> +	if (obj)
> +		efi_delete_handle(&obj->parent);
>  	if (mem_handle)
>  		efi_delete_handle(mem_handle);
>  
> @@ -469,33 +490,22 @@ exit:
>   * @path: File path to the test being run (often just the test name with a
>   *    backslash before it
>   * @test_func: Address of the test function that is being run
> + * @load_options_path: U-Boot environment variable to use as load options
>   * @return 0 if OK, -ve on error
>   */
>  static efi_status_t bootefi_test_prepare(struct efi_loaded_image **imagep,
>  					 struct efi_loaded_image_obj **objp,
> -					 const char *path, ulong test_func)
> +					 const char *path, ulong test_func,
> +					 const char *load_options_path)
>  {
> -	efi_status_t r;
> -
>  	/* Construct a dummy device path */
>  	bootefi_device_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
>  					      (uintptr_t)test_func,
>  					      (uintptr_t)test_func);
>  	bootefi_image_path = efi_dp_from_file(NULL, 0, path);
> -	r = efi_setup_loaded_image(bootefi_device_path, bootefi_image_path,
> -				   objp, imagep);
> -	if (r)
> -		return r;
> -	/*
> -	 * gd lives in a fixed register which may get clobbered while we execute
> -	 * the payload. So save it here and restore it on every callback entry
> -	 */
> -	efi_save_gd();
> -
> -	/* Transfer environment variable efi_selftest as load options */
> -	set_load_options(*imagep, "efi_selftest");
>  
> -	return 0;
> +	return bootefi_run_prepare(load_options_path, bootefi_device_path,
> +				   bootefi_image_path, imagep, objp);
>  }
>  
>  /**
> @@ -589,7 +599,8 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  		struct efi_loaded_image *image_prot;
>  
>  		if (bootefi_test_prepare(&image_prot, &obj, "\\selftest",
> -					 (uintptr_t)&efi_selftest))
> +					 (uintptr_t)&efi_selftest,
> +					 "efi_selftest"))
>  			return CMD_RET_FAILURE;
>  
>  		/* Execute the test */
>

Patch

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 0dd18d594d5..779c1f63fa8 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -327,6 +327,30 @@  static efi_status_t efi_install_fdt(ulong fdt_addr)
 	return ret;
 }
 
+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 **imagep,
+					struct efi_loaded_image_obj **objp)
+{
+	efi_status_t ret;
+
+	ret = efi_setup_loaded_image(device_path, image_path, objp, imagep);
+	if (ret != EFI_SUCCESS)
+		return ret;
+
+	/*
+	 * gd lives in a fixed register which may get clobbered while we execute
+	 * the payload. So save it here and restore it on every callback entry
+	 */
+	efi_save_gd();
+
+	/* Transfer environment variable as load options */
+	set_load_options(*imagep, load_options_path);
+
+	return 0;
+}
+
 /**
  * do_bootefi_exec() - execute EFI binary
  *
@@ -345,8 +369,8 @@  static efi_status_t do_bootefi_exec(void *efi,
 	efi_handle_t mem_handle = NULL;
 	struct efi_device_path *memdp = NULL;
 	efi_status_t ret;
-	struct efi_loaded_image_obj *image_handle = NULL;
-	struct efi_loaded_image *loaded_image_info = NULL;
+	struct efi_loaded_image_obj *obj = NULL;
+	struct efi_loaded_image *image = NULL;
 
 	EFIAPI efi_status_t (*entry)(efi_handle_t image_handle,
 				     struct efi_system_table *st);
@@ -376,15 +400,13 @@  static efi_status_t do_bootefi_exec(void *efi,
 		assert(device_path && image_path);
 	}
 
-	ret = efi_setup_loaded_image(device_path, image_path, &image_handle,
-				     &loaded_image_info);
-	if (ret != EFI_SUCCESS)
-		goto exit;
+	ret = bootefi_run_prepare("bootargs", device_path, image_path,
+				  &image, &obj);
+	if (ret)
+		return ret;
 
-	/* Transfer environment variable bootargs as load options */
-	set_load_options(loaded_image_info, "bootargs");
 	/* Load the EFI payload */
-	entry = efi_load_pe(image_handle, efi, loaded_image_info);
+	entry = efi_load_pe(obj, efi, image);
 	if (!entry) {
 		ret = EFI_LOAD_ERROR;
 		goto exit;
@@ -392,10 +414,9 @@  static efi_status_t do_bootefi_exec(void *efi,
 
 	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;
+		mdp->memory_type = image->image_code_type;
+		mdp->start_address = (uintptr_t)image->image_base;
+		mdp->end_address = mdp->start_address + image->image_size;
 	}
 
 	/* we don't support much: */
@@ -405,8 +426,8 @@  static efi_status_t do_bootefi_exec(void *efi,
 	/* Call our payload! */
 	debug("%s:%d Jumping to 0x%lx\n", __func__, __LINE__, (long)entry);
 
-	if (setjmp(&image_handle->exit_jmp)) {
-		ret = image_handle->exit_status;
+	if (setjmp(&obj->exit_jmp)) {
+		ret = obj->exit_status;
 		goto exit;
 	}
 
@@ -418,7 +439,7 @@  static efi_status_t do_bootefi_exec(void *efi,
 
 		/* Move into EL2 and keep running there */
 		armv8_switch_to_el2((ulong)entry,
-				    (ulong)image_handle,
+				    (ulong)obj,
 				    (ulong)&systab, 0, (ulong)efi_run_in_el2,
 				    ES_TO_AARCH64);
 
@@ -435,7 +456,7 @@  static efi_status_t do_bootefi_exec(void *efi,
 		secure_ram_addr(_do_nonsec_entry)(
 					efi_run_in_hyp,
 					(uintptr_t)entry,
-					(uintptr_t)image_handle,
+					(uintptr_t)obj,
 					(uintptr_t)&systab);
 
 		/* Should never reach here, efi exits with longjmp */
@@ -443,12 +464,12 @@  static efi_status_t do_bootefi_exec(void *efi,
 	}
 #endif
 
-	ret = efi_do_enter(image_handle, &systab, entry);
+	ret = efi_do_enter(obj, &systab, entry);
 
 exit:
 	/* image has returned, loaded-image obj goes *poof*: */
-	if (image_handle)
-		efi_delete_handle(&image_handle->parent);
+	if (obj)
+		efi_delete_handle(&obj->parent);
 	if (mem_handle)
 		efi_delete_handle(mem_handle);
 
@@ -469,33 +490,22 @@  exit:
  * @path: File path to the test being run (often just the test name with a
  *    backslash before it
  * @test_func: Address of the test function that is being run
+ * @load_options_path: U-Boot environment variable to use as load options
  * @return 0 if OK, -ve on error
  */
 static efi_status_t bootefi_test_prepare(struct efi_loaded_image **imagep,
 					 struct efi_loaded_image_obj **objp,
-					 const char *path, ulong test_func)
+					 const char *path, ulong test_func,
+					 const char *load_options_path)
 {
-	efi_status_t r;
-
 	/* Construct a dummy device path */
 	bootefi_device_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
 					      (uintptr_t)test_func,
 					      (uintptr_t)test_func);
 	bootefi_image_path = efi_dp_from_file(NULL, 0, path);
-	r = efi_setup_loaded_image(bootefi_device_path, bootefi_image_path,
-				   objp, imagep);
-	if (r)
-		return r;
-	/*
-	 * gd lives in a fixed register which may get clobbered while we execute
-	 * the payload. So save it here and restore it on every callback entry
-	 */
-	efi_save_gd();
-
-	/* Transfer environment variable efi_selftest as load options */
-	set_load_options(*imagep, "efi_selftest");
 
-	return 0;
+	return bootefi_run_prepare(load_options_path, bootefi_device_path,
+				   bootefi_image_path, imagep, objp);
 }
 
 /**
@@ -589,7 +599,8 @@  static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		struct efi_loaded_image *image_prot;
 
 		if (bootefi_test_prepare(&image_prot, &obj, "\\selftest",
-					 (uintptr_t)&efi_selftest))
+					 (uintptr_t)&efi_selftest,
+					 "efi_selftest"))
 			return CMD_RET_FAILURE;
 
 		/* Execute the test */