[U-Boot] [PATCH 1/5] efi_loader: LoadImage: always allocate new pages

[Alexander Graf]

On 09.01.19 09:35, Heinrich Schuchardt wrote:
> 
> 
> On 1/8/19 2:05 PM, Alexander Graf wrote:
>>
>>
>> On 28.12.18 12:41, Heinrich Schuchardt wrote:
>>> If we want to properly unload images in Exit() the memory should always be
>>> allocated in the same way. As we allocate memory when reading from file we
>>> should do the same when the original image is in memory.
>>>
>>> Further patches will be needed:
>>> - use LoadImage() in bootefi and bootmgr
>>> - implement correct unloading of images in Exit()
>>>
>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
>>> ---
>>>  include/efi_loader.h          |  2 +-
>>>  lib/efi_loader/efi_bootmgr.c  |  2 +-
>>>  lib/efi_loader/efi_boottime.c | 56 ++++++++++++++++++++++++-----------
>>>  3 files changed, 40 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>>> index 53f08161ab..a4c869b623 100644
>>> --- a/include/efi_loader.h
>>> +++ b/include/efi_loader.h
>>> @@ -387,7 +387,7 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path,
>>>  				    struct efi_loaded_image_obj **handle_ptr,
>>>  				    struct efi_loaded_image **info_ptr);
>>>  efi_status_t efi_load_image_from_path(struct efi_device_path *file_path,
>>> -				      void **buffer);
>>> +				      void **buffer, efi_uintn_t *size);
>>>  /* Print information about all loaded images */
>>>  void efi_print_image_infos(void *pc);
>>>  
>>> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
>>> index a095df3f54..196116b547 100644
>>> --- a/lib/efi_loader/efi_bootmgr.c
>>> +++ b/lib/efi_loader/efi_bootmgr.c
>>> @@ -150,7 +150,7 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path,
>>>  		debug("%s: trying to load \"%ls\" from %pD\n",
>>>  		      __func__, lo.label, lo.file_path);
>>>  
>>> -		ret = efi_load_image_from_path(lo.file_path, &image);
>>> +		ret = efi_load_image_from_path(lo.file_path, &image, &size);
>>>  
>>>  		if (ret != EFI_SUCCESS)
>>>  			goto error;
>>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
>>> index f592e4083f..ba76f7ff50 100644
>>> --- a/lib/efi_loader/efi_boottime.c
>>> +++ b/lib/efi_loader/efi_boottime.c
>>> @@ -1566,17 +1566,19 @@ failure:
>>>  
>>>  /**
>>>   * efi_load_image_from_path() - load an image using a file path
>>> - * @file_path: the path of the image to load
>>> - * @buffer:    buffer containing the loaded image
>>> + * @file_path:	the path of the image to load
>>> + * @buffer:	buffer containing the loaded image
>>> + * @size:	size of the loaded image
>>>   *
>>>   * Return: status code
>>>   */
>>>  efi_status_t efi_load_image_from_path(struct efi_device_path *file_path,
>>> -				      void **buffer)
>>> +				      void **buffer, efi_uintn_t *size)
>>>  {
>>>  	struct efi_file_info *info = NULL;
>>>  	struct efi_file_handle *f;
>>>  	static efi_status_t ret;
>>> +	u64 addr;
>>>  	efi_uintn_t bs;
>>>  
>>>  	f = efi_file_from_path(file_path);
>>> @@ -1594,22 +1596,21 @@ efi_status_t efi_load_image_from_path(struct efi_device_path *file_path,
>>>  	if (ret != EFI_SUCCESS)
>>>  		goto error;
>>>  
>>> -	ret = efi_allocate_pool(EFI_LOADER_DATA, info->file_size, buffer);
>>> -	if (ret)
>>> +	ret = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES,
>>> +				 EFI_RUNTIME_SERVICES_CODE,
>>> +				 efi_size_in_pages(), &addr);
>>
>> Doesn't efi_size_in_pages() want an argument?
> 
> Yes.
> 
> What is missing is a test that runs through this code. So before
> resending the patch series I will create a new unit test.

I'm surprised the above even compiled tbh :).

> 
>>
>> Also, why is this RTS code?
> 
> Depending on the image type (runtime driver, application) we need
> different types of memory. The correct type is set via
> efi_add_memory_map() below.

This needs at least a big comment please.

> 
>>
>>> +	if (ret != EFI_SUCCESS) {
>>> +		ret = EFI_OUT_OF_RESOURCES;
>>>  		goto error;
>>> +	}
>>>  
>>> +	*buffer = (void *)(uintptr_t)addr;
>>>  	bs = info->file_size;
>>>  	EFI_CALL(ret = f->read(f, &bs, *buffer));
>>>  
>>>  error:
>>> -	free(info);
>>
>> Why don't you have to free info anymore? It's a local variable, no?
>>
>>>  	EFI_CALL(f->close(f));
>>>  
>>> -	if (ret != EFI_SUCCESS) {
>>> -		efi_free_pool(*buffer);
>>
>> Where do we ever free the pages allocated above?
>>
>>> -		*buffer = NULL;
>>> -	}
>>> -
>>>  	return ret;
>>>  }
>>>  
>>> @@ -1636,6 +1637,7 @@ static efi_status_t EFIAPI efi_load_image(bool boot_policy,
>>>  					  efi_uintn_t source_size,
>>>  					  efi_handle_t *image_handle)
>>>  {
>>> +	struct efi_device_path *dp, *fp;
>>>  	struct efi_loaded_image *info = NULL;
>>>  	struct efi_loaded_image_obj **image_obj =
>>>  		(struct efi_loaded_image_obj **)image_handle;
>>> @@ -1655,9 +1657,10 @@ static efi_status_t EFIAPI efi_load_image(bool boot_policy,
>>>  	}
>>>  
>>>  	if (!source_buffer) {
>>> -		struct efi_device_path *dp, *fp;
>>> -
>>> -		ret = efi_load_image_from_path(file_path, &source_buffer);
>>> +		ret = efi_load_image_from_path(file_path, &source_buffer,
>>> +					       &source_size);
>>> +		if (ret == EFI_OUT_OF_RESOURCES)
>>> +			goto error;
>>
>> How is error different from failure? And why does it depend on the error
>> code of this function? I must be missing something ...
>>
>>>  		if (ret != EFI_SUCCESS)
>>>  			goto failure;
>>>  		/*
>>> @@ -1665,26 +1668,43 @@ static efi_status_t EFIAPI efi_load_image(bool boot_policy,
>>>  		 * file parts:
>>>  		 */
>>>  		efi_dp_split_file_path(file_path, &dp, &fp);
>>> -		ret = efi_setup_loaded_image(dp, fp, image_obj, &info);
>>> -		if (ret != EFI_SUCCESS)
>>> -			goto failure;
>>>  	} else {
>>>  		/* In this case, file_path is the "device" path, i.e.
>>>  		 * something like a HARDWARE_DEVICE:MEMORY_MAPPED
>>>  		 */
>>> -		ret = efi_setup_loaded_image(file_path, NULL, image_obj, &info);
>>> +		u64 addr;
>>> +		void *dest_buffer;
>>> +
>>> +		ret = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES,
>>> +					 EFI_RUNTIME_SERVICES_CODE,
>>> +					 efi_size_in_pages(source_size), &addr);
>>>  		if (ret != EFI_SUCCESS)
>>>  			goto error;
>>> +		dest_buffer = (void *)(uintptr_t)addr;
>>> +		memcpy(dest_buffer, source_buffer, source_size);
>>
>> I'd really by far prefer if we didn't have to memcpy() things around and
>> reserve more memory than we have to.
> 
> Where I want to get to is:
> 
> The image is either copied from memory image or from file to fresh
> memory. The original is not touched.
> In relocation we do not allocate any memory but relocate in place.

Well, the reason I'm reluctant is that I want the zero-copy path to
exist. Imagine someone doing Falcon Boot (SPL based) with EFI_LOADER.
They don't want to copy the kernel around again.

Maybe we can make the copy optional somehow?

> So finally in the case of an image loaded from file we will end up in
> consuming less memory.
> 
> But before I can get there I have to change bootefi and bootmgr to call
> LoadImage().

I agree, it's probably a good idea to consolidate it all onto
LoadImage() and then invent a fast path after we have a unified code path.


Alex