| Message ID | 20181106225744.139945-6-sjg@chromium.org |
|---|---|
| State | Superseded, archived |
| Delegated to: | Alexander Graf |
| Headers | show |
| Series | efi_loader: Code refactoring and improvement | expand |
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 */ >
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 */
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(-)