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

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 */