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

[Alexander Graf]

On 23.09.18 15:19, Heinrich Schuchardt wrote:
> 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().

No answer here?

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

The arguments to entry(...) have to be the same across all 3 calling
mechanisms: drop-to-el2, drop-to-hyp, call-native. Currently 2 out of 3
pass "image_handle", one passes "&image_handle".


Alex