[U-Boot] [PATCH 0/5] efi_loader: rework loading and starting of images
This patch series starts the necessary changes needed to correctly implement the unloading of images in Exit().
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.
Up to now efi_load_pe() returns the entry point or NULL in case of an error. This does not allow to return correct error codes from LoadImage().
Let efi_load_pe() return a status code and fill in the entry point in the corresponding field of the image object.
We use u16* for Unicode strings and efi_uintn_t for UINTN. Correct the signature of efi_exit() and efi_start_image().
Remove the duplicate code in efi_do_enter() and use efi_start_image() to start the image invoked by the bootefi command.
Adjust a debug message.
Further patches will be needed: - use LoadImage() in bootefi and bootmgr - implement correct unloading of images in Exit()
Heinrich Schuchardt (5): efi_loader: LoadImage: always allocate new pages efi_loader: set entry point in efi_load_pe() efi_loader: avoid unnecessary pointer to long conversion efi_loader: signature of StartImage and Exit efi_loader: use efi_start_image() for bootefi
cmd/bootefi.c | 33 ++------------ include/efi_api.h | 6 +-- include/efi_loader.h | 10 +++-- lib/efi_loader/efi_bootmgr.c | 2 +- lib/efi_loader/efi_boottime.c | 74 +++++++++++++++++++------------ lib/efi_loader/efi_image_loader.c | 34 ++++++++------ 6 files changed, 81 insertions(+), 78 deletions(-)
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); + 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); EFI_CALL(f->close(f));
- if (ret != EFI_SUCCESS) { - efi_free_pool(*buffer); - *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; 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); + 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);
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?
Also, why is this RTS code?
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.
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));
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));
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@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;*/
- 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.
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
Up to now efi_load_pe() returns the entry point or NULL in case of an error. This does not allow to return correct error codes from LoadImage().
Let efi_load_pe() return a status code and fill in the entry point in the corresponding field of the image object.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de --- cmd/bootefi.c | 14 +++++-------- include/efi_loader.h | 4 ++-- lib/efi_loader/efi_boottime.c | 6 ++---- lib/efi_loader/efi_image_loader.c | 34 ++++++++++++++++++------------- 4 files changed, 29 insertions(+), 29 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index cc34fe880c..cb4d797e90 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -342,9 +342,6 @@ static efi_status_t do_bootefi_exec(void *efi, struct efi_loaded_image_obj *image_obj = NULL; struct efi_loaded_image *loaded_image_info = NULL;
- EFIAPI efi_status_t (*entry)(efi_handle_t image_handle, - struct efi_system_table *st); - /* * Special case for efi payload not loaded from disk, such as * 'bootefi hello' or for example payload loaded directly into @@ -376,11 +373,9 @@ static efi_status_t do_bootefi_exec(void *efi, goto err_prepare;
/* Load the EFI payload */ - entry = efi_load_pe(image_obj, efi, loaded_image_info); - if (!entry) { - ret = EFI_LOAD_ERROR; + ret = efi_load_pe(image_obj, efi, loaded_image_info); + if (ret != EFI_SUCCESS) goto err_prepare; - }
if (memdp) { struct efi_device_path_memory *mdp = (void *)memdp; @@ -395,14 +390,15 @@ static efi_status_t do_bootefi_exec(void *efi, "{ro,boot}(blob)0000000000000000");
/* Call our payload! */ - debug("%s:%d Jumping to 0x%lx\n", __func__, __LINE__, (long)entry); + debug("%s:%d Jumping to 0x%lx\n", __func__, __LINE__, + (long)image_obj->entry);
if (setjmp(&image_obj->exit_jmp)) { ret = image_obj->exit_status; goto err_prepare; }
- ret = efi_do_enter(&image_obj->header, &systab, entry); + ret = efi_do_enter(&image_obj->header, &systab, image_obj->entry);
err_prepare: /* image has returned, loaded-image obj goes *poof*: */ diff --git a/include/efi_loader.h b/include/efi_loader.h index a4c869b623..ab6dc9b2d7 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -291,8 +291,8 @@ efi_status_t efi_set_watchdog(unsigned long timeout); /* Called from places to check whether a timer expired */ void efi_timer_check(void); /* PE loader implementation */ -void *efi_load_pe(struct efi_loaded_image_obj *handle, void *efi, - struct efi_loaded_image *loaded_image_info); +efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, void *efi, + struct efi_loaded_image *loaded_image_info); /* Called once to store the pristine gd pointer */ void efi_save_gd(void); /* Special case handler for error/abort that just tries to dtrt to get diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index ba76f7ff50..4231e8e02f 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -1690,11 +1690,9 @@ static efi_status_t EFIAPI efi_load_image(bool boot_policy, 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; + ret = efi_load_pe(*image_obj, source_buffer, 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), diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c index a18ce0a570..f5ac6e22e6 100644 --- a/lib/efi_loader/efi_image_loader.c +++ b/lib/efi_loader/efi_image_loader.c @@ -193,13 +193,20 @@ static void efi_set_code_and_data_type( } }
-/* +/** + * efi_load_pe() - relocate EFI binary + * * This function loads all sections from a PE binary into a newly reserved * piece of memory. On successful load it then returns the entry point for * the binary. Otherwise NULL. + * + * @handle: loaded image handle + * @efi: pointer to the EFI binary + * @loaded_image_info: loaded image protocol + * Return: status code */ -void *efi_load_pe(struct efi_loaded_image_obj *handle, void *efi, - struct efi_loaded_image *loaded_image_info) +efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, void *efi, + struct efi_loaded_image *loaded_image_info) { IMAGE_NT_HEADERS32 *nt; IMAGE_DOS_HEADER *dos; @@ -210,7 +217,6 @@ void *efi_load_pe(struct efi_loaded_image_obj *handle, void *efi, const IMAGE_BASE_RELOCATION *rel; unsigned long rel_size; int rel_idx = IMAGE_DIRECTORY_ENTRY_BASERELOC; - void *entry; uint64_t image_base; uint64_t image_size; unsigned long virt_size = 0; @@ -219,13 +225,13 @@ void *efi_load_pe(struct efi_loaded_image_obj *handle, void *efi, dos = efi; if (dos->e_magic != IMAGE_DOS_SIGNATURE) { printf("%s: Invalid DOS Signature\n", __func__); - return NULL; + return EFI_LOAD_ERROR; }
nt = (void *) ((char *)efi + dos->e_lfanew); if (nt->Signature != IMAGE_NT_SIGNATURE) { printf("%s: Invalid NT Signature\n", __func__); - return NULL; + return EFI_LOAD_ERROR; }
for (i = 0; machines[i]; i++) @@ -237,7 +243,7 @@ void *efi_load_pe(struct efi_loaded_image_obj *handle, void *efi, if (!supported) { printf("%s: Machine type 0x%04x is not supported\n", __func__, nt->FileHeader.Machine); - return NULL; + return EFI_LOAD_ERROR; }
/* Calculate upper virtual address boundary */ @@ -263,9 +269,9 @@ void *efi_load_pe(struct efi_loaded_image_obj *handle, void *efi, if (!efi_reloc) { printf("%s: Could not allocate %lu bytes\n", __func__, virt_size); - return NULL; + return EFI_OUT_OF_RESOURCES; } - entry = efi_reloc + opt->AddressOfEntryPoint; + handle->entry = efi_reloc + opt->AddressOfEntryPoint; rel_size = opt->DataDirectory[rel_idx].Size; rel = efi_reloc + opt->DataDirectory[rel_idx].VirtualAddress; virt_size = ALIGN(virt_size, opt->SectionAlignment); @@ -279,16 +285,16 @@ void *efi_load_pe(struct efi_loaded_image_obj *handle, void *efi, if (!efi_reloc) { printf("%s: Could not allocate %lu bytes\n", __func__, virt_size); - return NULL; + return EFI_OUT_OF_RESOURCES; } - entry = efi_reloc + opt->AddressOfEntryPoint; + handle->entry = efi_reloc + opt->AddressOfEntryPoint; rel_size = opt->DataDirectory[rel_idx].Size; rel = efi_reloc + opt->DataDirectory[rel_idx].VirtualAddress; virt_size = ALIGN(virt_size, opt->SectionAlignment); } else { printf("%s: Invalid optional header magic %x\n", __func__, nt->OptionalHeader.Magic); - return NULL; + return EFI_LOAD_ERROR; }
/* Load sections into RAM */ @@ -306,7 +312,7 @@ void *efi_load_pe(struct efi_loaded_image_obj *handle, void *efi, (unsigned long)image_base) != EFI_SUCCESS) { efi_free_pages((uintptr_t) efi_reloc, (virt_size + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT); - return NULL; + return EFI_LOAD_ERROR; }
/* Flush cache */ @@ -320,5 +326,5 @@ void *efi_load_pe(struct efi_loaded_image_obj *handle, void *efi, handle->reloc_base = efi_reloc; handle->reloc_size = virt_size;
- return entry; + return EFI_SUCCESS; }
On 28.12.18 12:41, Heinrich Schuchardt wrote:
Up to now efi_load_pe() returns the entry point or NULL in case of an error. This does not allow to return correct error codes from LoadImage().
Let efi_load_pe() return a status code and fill in the entry point in the corresponding field of the image object.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de
This change makes a lot of sense, yes :). I have no idea what I was thinking when I put up that weird API.
Reviewed-by: Alexander Graf agraf@suse.de
Alex
debug() support supports %p to print pointers.
The debug message is unique. So there is not need to write a possibly distracting line number.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de --- cmd/bootefi.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index cb4d797e90..6931643066 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -390,8 +390,7 @@ static efi_status_t do_bootefi_exec(void *efi, "{ro,boot}(blob)0000000000000000");
/* Call our payload! */ - debug("%s:%d Jumping to 0x%lx\n", __func__, __LINE__, - (long)image_obj->entry); + debug("%s: Jumping to 0x%p\n", __func__, image_obj->entry);
if (setjmp(&image_obj->exit_jmp)) { ret = image_obj->exit_status;
We use u16* for Unicode strings and efi_uintn_t for UINTN. Correct the signature of efi_exit() and efi_start_image().
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de --- include/efi_api.h | 6 +++--- lib/efi_loader/efi_boottime.c | 10 +++++----- 2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/include/efi_api.h b/include/efi_api.h index 0e5c6e92d0..4389dbeddc 100644 --- a/include/efi_api.h +++ b/include/efi_api.h @@ -115,11 +115,11 @@ struct efi_boot_services { struct efi_device_path *file_path, void *source_buffer, efi_uintn_t source_size, efi_handle_t *image); efi_status_t (EFIAPI *start_image)(efi_handle_t handle, - unsigned long *exitdata_size, - s16 **exitdata); + efi_uintn_t *exitdata_size, + u16 **exitdata); efi_status_t (EFIAPI *exit)(efi_handle_t handle, efi_status_t exit_status, - unsigned long exitdata_size, s16 *exitdata); + efi_uintn_t exitdata_size, u16 *exitdata); efi_status_t (EFIAPI *unload_image)(efi_handle_t image_handle); efi_status_t (EFIAPI *exit_boot_services)(efi_handle_t, unsigned long);
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 4231e8e02f..590e6f11a5 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -1724,8 +1724,8 @@ error: * Return: status code */ static efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle, - unsigned long *exit_data_size, - s16 **exit_data) + efi_uintn_t *exit_data_size, + u16 **exit_data) { struct efi_loaded_image_obj *image_obj = (struct efi_loaded_image_obj *)image_handle; @@ -1791,8 +1791,8 @@ static efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle, */ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle, efi_status_t exit_status, - unsigned long exit_data_size, - int16_t *exit_data) + efi_uintn_t exit_data_size, + u16 *exit_data) { /* * TODO: We should call the unload procedure of the loaded @@ -1801,7 +1801,7 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle, struct efi_loaded_image_obj *image_obj = (struct efi_loaded_image_obj *)image_handle;
- EFI_ENTRY("%p, %ld, %ld, %p", image_handle, exit_status, + EFI_ENTRY("%p, %ld, %zu, %p", image_handle, exit_status, exit_data_size, exit_data);
/* Make sure entry/exit counts for EFI world cross-overs match */
Remove the duplicate code in efi_do_enter() and use efi_start_image() to start the image invoked by the bootefi command.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de --- cmd/bootefi.c | 22 +--------------------- include/efi_loader.h | 4 ++++ lib/efi_loader/efi_boottime.c | 6 +++--- 3 files changed, 8 insertions(+), 24 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 6931643066..9aff186615 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -209,20 +209,6 @@ done: return ret; }
-static efi_status_t efi_do_enter( - efi_handle_t image_handle, struct efi_system_table *st, - EFIAPI efi_status_t (*entry)( - efi_handle_t image_handle, - struct efi_system_table *st)) -{ - efi_status_t ret = EFI_LOAD_ERROR; - - if (entry) - ret = entry(image_handle, st); - st->boottime->exit(image_handle, ret, 0, NULL); - return ret; -} - /* * efi_carve_out_dt_rsv() - Carve out DT reserved memory ranges * @@ -391,13 +377,7 @@ static efi_status_t do_bootefi_exec(void *efi,
/* Call our payload! */ debug("%s: Jumping to 0x%p\n", __func__, image_obj->entry); - - if (setjmp(&image_obj->exit_jmp)) { - ret = image_obj->exit_status; - goto err_prepare; - } - - ret = efi_do_enter(&image_obj->header, &systab, image_obj->entry); + ret = efi_start_image(&image_obj->header, NULL, NULL);
err_prepare: /* image has returned, loaded-image obj goes *poof*: */ diff --git a/include/efi_loader.h b/include/efi_loader.h index ab6dc9b2d7..83e3c3e985 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -310,6 +310,10 @@ efi_status_t efi_create_handle(efi_handle_t *handle); void efi_delete_handle(efi_handle_t obj); /* Call this to validate a handle and find the EFI object for it */ struct efi_object *efi_search_obj(const efi_handle_t handle); +/* Start image */ +efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle, + efi_uintn_t *exit_data_size, + u16 **exit_data); /* Find a protocol on a handle */ efi_status_t efi_search_protocol(const efi_handle_t handle, const efi_guid_t *protocol_guid, diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 590e6f11a5..fe0c02296a 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -1723,9 +1723,9 @@ error: * * Return: status code */ -static efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle, - efi_uintn_t *exit_data_size, - u16 **exit_data) +efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle, + efi_uintn_t *exit_data_size, + u16 **exit_data) { struct efi_loaded_image_obj *image_obj = (struct efi_loaded_image_obj *)image_handle;
participants (2)
-
Alexander Graf -
Heinrich Schuchardt