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.g...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
