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@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;*/
- 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);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;- /* 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);failure:
- efi_free_pages((uintptr_t)source_buffer,
efi_size_in_pages(source_size));