[U-Boot] [PATCH v3 1/1] efi_loader: refactor efi_setup_loaded_image()

[Heinrich Schuchardt]

On 09/23/2018 02:34 PM, Alexander Graf wrote:
> 
> 
> On 23.09.18 13:56, Heinrich Schuchardt wrote:
>> Create the handle of loaded images and the EFI_LOADED_IMAGE_PROTOCOL
>> inside efi_setup_loaded_image(). Do not use local variables.
>>
>> Currently we expect the loaded image handle to point to the loaded image
>> protocol. Additionally we have appended private fields to the protocol.
>>
>> With the patch the handle points to a loaded image object and the private
>> fields are added here. This matches how we handle the net and the gop
>> object.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
>> ---
>> v3
>> 	squashed two patches
>> 	https://lists.denx.de/pipermail/u-boot/2018-September/341130.html
>> 	https://lists.denx.de/pipermail/u-boot/2018-September/341589.html
>> v2
>> 	avoid unused variables if configured without CONFIG_EFI_SELFTEST
>> ---
>>  cmd/bootefi.c                     | 61 +++++++++++---------
>>  include/efi_api.h                 |  8 ---
>>  include/efi_loader.h              | 27 ++++++---
>>  lib/efi_loader/efi_boottime.c     | 92 +++++++++++++++----------------
>>  lib/efi_loader/efi_image_loader.c | 23 ++++----
>>  5 files changed, 111 insertions(+), 100 deletions(-)
>>
>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>> index 8bc6fa49f5..f76f29a7b1 100644
>> --- a/cmd/bootefi.c
>> +++ b/cmd/bootefi.c
>> @@ -325,19 +325,26 @@ static efi_status_t efi_install_fdt(ulong fdt_addr)
>>  	return ret;
>>  }
>>  
>> -/*
>> - * Load an EFI payload into a newly allocated piece of memory, register all
>> - * EFI objects it would want to access and jump to it.
>> +/**
>> + * do_bootefi_exec() - execute EFI binary
>> + *
>> + * @efi:		address of the binary
>> + * @device_path:	path of the device from which the binary was loaded
>> + * @image_path:		device path of the binary
>> + * Return:		status code
>> + *
>> + * Load the EFI binary into a newly assigned memory unwinding the relocation
>> + * information, install the loaded image protocol, and call the binary.
>>   */
>>  static efi_status_t do_bootefi_exec(void *efi,
>>  				    struct efi_device_path *device_path,
>>  				    struct efi_device_path *image_path)
>>  {
>> -	struct efi_loaded_image loaded_image_info = {};
>> -	struct efi_object loaded_image_info_obj = {};
>>  	efi_handle_t mem_handle = NULL;
>>  	struct efi_device_path *memdp = NULL;
>>  	efi_status_t ret;
>> +	struct efi_loaded_image_obj *image_handle;
>> +	struct efi_loaded_image *loaded_image_info;
> 
> Better initialize those to NULL so we don't have to remember to do that
> in efi_setup_loaded_image().
> 
>>  
>>  	EFIAPI efi_status_t (*entry)(efi_handle_t image_handle,
>>  				     struct efi_system_table *st);
>> @@ -367,8 +374,8 @@ static efi_status_t do_bootefi_exec(void *efi,
>>  		assert(device_path && image_path);
>>  	}
>>  
>> -	efi_setup_loaded_image(&loaded_image_info, &loaded_image_info_obj,
>> -			       device_path, image_path);
>> +	efi_setup_loaded_image(device_path, image_path, &image_handle,
>> +			       &loaded_image_info);
>>  
>>  	/*
>>  	 * gd lives in a fixed register which may get clobbered while we execute
>> @@ -377,9 +384,9 @@ static efi_status_t do_bootefi_exec(void *efi,
>>  	efi_save_gd();
>>  
>>  	/* Transfer environment variable bootargs as load options */
>> -	set_load_options(&loaded_image_info, "bootargs");
>> +	set_load_options(loaded_image_info, "bootargs");
>>  	/* Load the EFI payload */
>> -	entry = efi_load_pe(efi, &loaded_image_info);
>> +	entry = efi_load_pe(image_handle, efi, loaded_image_info);
>>  	if (!entry) {
>>  		ret = EFI_LOAD_ERROR;
>>  		goto exit;
>> @@ -387,10 +394,10 @@ 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->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;
>> +				loaded_image_info->image_size;
>>  	}
>>  
>>  	/* we don't support much: */
>> @@ -400,8 +407,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(&loaded_image_info.exit_jmp)) {
>> -		ret = loaded_image_info.exit_status;
>> +	if (setjmp(&image_handle->exit_jmp)) {
>> +		ret = image_handle->exit_status;
>>  		goto exit;
>>  	}
>>  
>> @@ -413,7 +420,7 @@ static efi_status_t do_bootefi_exec(void *efi,
>>  
>>  		/* Move into EL2 and keep running there */
>>  		armv8_switch_to_el2((ulong)entry,
>> -				    (ulong)&loaded_image_info_obj.handle,
>> +				    (ulong)&image_handle,
> 
> This needs to become only "image_handle" now, no?

&loaded_image_info_obj.handle wass of type (void **), i.e. a pointer to
a pointer to a anonymous structure.

&image_handle is of type (struct efi_loaded_image_obj **), i.e also a
pointer to a pointer to a structure

Regards

Heinrich