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

[Heinrich Schuchardt]

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.

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

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

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().

Best regards

Heinrich

> 
> Can't we just instead of this create a destructor that we attach to the
> LoadedImage that does different things depending on the way it got loaded?
> 
> 
> Alex
> 
>> +		source_buffer = dest_buffer;
>> +
>> +		dp = file_path;
>> +		fp = NULL;
>>  	}
>> +	ret = efi_setup_loaded_image(dp, fp, image_obj, &info);
>> +	if (ret != EFI_SUCCESS)
>> +		goto failure;
>>  	(*image_obj)->entry = efi_load_pe(*image_obj, source_buffer, info);
>>  	if (!(*image_obj)->entry) {
>>  		ret = EFI_UNSUPPORTED;
>>  		goto failure;
>>  	}
>> +	/* Update the type of the allocated memory */
>> +	efi_add_memory_map((uintptr_t)source_buffer,
>> +			   efi_size_in_pages(source_size),
>> +			   info->image_code_type, false);
>>  	info->system_table = &systab;
>>  	info->parent_handle = parent_image;
>>  	return EFI_EXIT(EFI_SUCCESS);
>>  failure:
>> +	efi_free_pages((uintptr_t)source_buffer,
>> +		       efi_size_in_pages(source_size));
>>  	efi_delete_handle(*image_handle);
>>  	*image_handle = NULL;
>>  	free(info);
>>
>