* [U-Boot] [PATCH 1/1] efi_loader: release file buffer after loading image
@ 2019-03-05 19:56 Heinrich Schuchardt
2019-03-06 0:41 ` AKASHI Takahiro
0 siblings, 1 reply; 3+ messages in thread
From: Heinrich Schuchardt @ 2019-03-05 19:56 UTC (permalink / raw)
To: u-boot
efi_load_pe() copies and rebases the UEFI image.
We do not need the buffer with the file contents afterwards.
Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
lib/efi_loader/efi_boottime.c | 50 +++++++++++++----------------------
1 file changed, 18 insertions(+), 32 deletions(-)
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index bd8b8a17ae..391681260c 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -1693,6 +1693,7 @@ static efi_status_t EFIAPI efi_load_image(bool boot_policy,
struct efi_loaded_image_obj **image_obj =
(struct efi_loaded_image_obj **)image_handle;
efi_status_t ret;
+ void *dest_buffer;
EFI_ENTRY("%d, %p, %pD, %p, %zd, %p", boot_policy, parent_image,
file_path, source_buffer, source_size, image_handle);
@@ -1708,7 +1709,7 @@ static efi_status_t EFIAPI efi_load_image(bool boot_policy,
}
if (!source_buffer) {
- ret = efi_load_image_from_path(file_path, &source_buffer,
+ ret = efi_load_image_from_path(file_path, &dest_buffer,
&source_size);
if (ret != EFI_SUCCESS)
goto error;
@@ -1721,41 +1722,26 @@ static efi_status_t EFIAPI efi_load_image(bool boot_policy,
/* In this case, file_path is the "device" path, i.e.
* something like a HARDWARE_DEVICE:MEMORY_MAPPED
*/
- 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;
-
+ dest_buffer = source_buffer;
dp = file_path;
fp = NULL;
}
ret = efi_setup_loaded_image(dp, fp, image_obj, &info);
- if (ret != EFI_SUCCESS)
- goto error_invalid_image;
- ret = efi_load_pe(*image_obj, source_buffer, info);
- if (ret != EFI_SUCCESS)
- goto error_invalid_image;
- /* 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);
-error_invalid_image:
- /* The image is invalid. Release all associated resources. */
- efi_free_pages((uintptr_t)source_buffer,
- efi_size_in_pages(source_size));
- efi_delete_handle(*image_handle);
- *image_handle = NULL;
- free(info);
+ if (ret == EFI_SUCCESS)
+ ret = efi_load_pe(*image_obj, dest_buffer, info);
+ if (!source_buffer)
+ /* Release buffer to which file was loaded */
+ efi_free_pages((uintptr_t)dest_buffer,
+ efi_size_in_pages(source_size));
+ if (ret == EFI_SUCCESS) {
+ info->system_table = &systab;
+ info->parent_handle = parent_image;
+ } else {
+ /* The image is invalid. Release all associated resources. */
+ efi_delete_handle(*image_handle);
+ *image_handle = NULL;
+ free(info);
+ }
error:
return EFI_EXIT(ret);
}
--
2.20.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* [U-Boot] [PATCH 1/1] efi_loader: release file buffer after loading image
2019-03-05 19:56 [U-Boot] [PATCH 1/1] efi_loader: release file buffer after loading image Heinrich Schuchardt
@ 2019-03-06 0:41 ` AKASHI Takahiro
2019-03-06 4:38 ` Heinrich Schuchardt
0 siblings, 1 reply; 3+ messages in thread
From: AKASHI Takahiro @ 2019-03-06 0:41 UTC (permalink / raw)
To: u-boot
On Tue, Mar 05, 2019 at 08:56:12PM +0100, Heinrich Schuchardt wrote:
> efi_load_pe() copies and rebases the UEFI image.
> We do not need the buffer with the file contents afterwards.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> lib/efi_loader/efi_boottime.c | 50 +++++++++++++----------------------
> 1 file changed, 18 insertions(+), 32 deletions(-)
>
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index bd8b8a17ae..391681260c 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -1693,6 +1693,7 @@ static efi_status_t EFIAPI efi_load_image(bool boot_policy,
> struct efi_loaded_image_obj **image_obj =
> (struct efi_loaded_image_obj **)image_handle;
> efi_status_t ret;
> + void *dest_buffer;
>
> EFI_ENTRY("%d, %p, %pD, %p, %zd, %p", boot_policy, parent_image,
> file_path, source_buffer, source_size, image_handle);
> @@ -1708,7 +1709,7 @@ static efi_status_t EFIAPI efi_load_image(bool boot_policy,
> }
>
> if (!source_buffer) {
> - ret = efi_load_image_from_path(file_path, &source_buffer,
> + ret = efi_load_image_from_path(file_path, &dest_buffer,
> &source_size);
> if (ret != EFI_SUCCESS)
> goto error;
> @@ -1721,41 +1722,26 @@ static efi_status_t EFIAPI efi_load_image(bool boot_policy,
> /* In this case, file_path is the "device" path, i.e.
> * something like a HARDWARE_DEVICE:MEMORY_MAPPED
> */
> - 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;
> -
Are you sure? This code is what you added in your recent commit
0e18f584de59 ("efi_loader: LoadImage: always allocate new pages")
at v2019.04-rc2.
IMO, the comment:
> /* In this case, file_path is the "device" path, i.e.
> * something like a HARDWARE_DEVICE:MEMORY_MAPPED
is also not accurate because "file_path" is normally non-NULL,
indicating where the content of "source_buffer" comes from.
In other words, "HARDWARE_DEVICE:MEMORY_MAPPED" is a quite irregular case.
-Takahiro Akashi
> + dest_buffer = source_buffer;
> dp = file_path;
> fp = NULL;
> }
> ret = efi_setup_loaded_image(dp, fp, image_obj, &info);
> - if (ret != EFI_SUCCESS)
> - goto error_invalid_image;
> - ret = efi_load_pe(*image_obj, source_buffer, info);
> - if (ret != EFI_SUCCESS)
> - goto error_invalid_image;
> - /* 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);
> -error_invalid_image:
> - /* The image is invalid. Release all associated resources. */
> - efi_free_pages((uintptr_t)source_buffer,
> - efi_size_in_pages(source_size));
> - efi_delete_handle(*image_handle);
> - *image_handle = NULL;
> - free(info);
> + if (ret == EFI_SUCCESS)
> + ret = efi_load_pe(*image_obj, dest_buffer, info);
> + if (!source_buffer)
> + /* Release buffer to which file was loaded */
> + efi_free_pages((uintptr_t)dest_buffer,
> + efi_size_in_pages(source_size));
> + if (ret == EFI_SUCCESS) {
> + info->system_table = &systab;
> + info->parent_handle = parent_image;
> + } else {
> + /* The image is invalid. Release all associated resources. */
> + efi_delete_handle(*image_handle);
> + *image_handle = NULL;
> + free(info);
> + }
> error:
> return EFI_EXIT(ret);
> }
> --
> 2.20.1
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* [U-Boot] [PATCH 1/1] efi_loader: release file buffer after loading image
2019-03-06 0:41 ` AKASHI Takahiro
@ 2019-03-06 4:38 ` Heinrich Schuchardt
0 siblings, 0 replies; 3+ messages in thread
From: Heinrich Schuchardt @ 2019-03-06 4:38 UTC (permalink / raw)
To: u-boot
On 3/6/19 1:41 AM, AKASHI Takahiro wrote:
> On Tue, Mar 05, 2019 at 08:56:12PM +0100, Heinrich Schuchardt wrote:
>> efi_load_pe() copies and rebases the UEFI image.
>> We do not need the buffer with the file contents afterwards.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>> lib/efi_loader/efi_boottime.c | 50 +++++++++++++----------------------
>> 1 file changed, 18 insertions(+), 32 deletions(-)
>>
>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
>> index bd8b8a17ae..391681260c 100644
>> --- a/lib/efi_loader/efi_boottime.c
>> +++ b/lib/efi_loader/efi_boottime.c
>> @@ -1693,6 +1693,7 @@ static efi_status_t EFIAPI efi_load_image(bool boot_policy,
>> struct efi_loaded_image_obj **image_obj =
>> (struct efi_loaded_image_obj **)image_handle;
>> efi_status_t ret;
>> + void *dest_buffer;
>>
>> EFI_ENTRY("%d, %p, %pD, %p, %zd, %p", boot_policy, parent_image,
>> file_path, source_buffer, source_size, image_handle);
>> @@ -1708,7 +1709,7 @@ static efi_status_t EFIAPI efi_load_image(bool boot_policy,
>> }
>>
>> if (!source_buffer) {
>> - ret = efi_load_image_from_path(file_path, &source_buffer,
>> + ret = efi_load_image_from_path(file_path, &dest_buffer,
>> &source_size);
>> if (ret != EFI_SUCCESS)
>> goto error;
>> @@ -1721,41 +1722,26 @@ static efi_status_t EFIAPI efi_load_image(bool boot_policy,
>> /* In this case, file_path is the "device" path, i.e.
>> * something like a HARDWARE_DEVICE:MEMORY_MAPPED
>> */
>> - 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;
>> -
>
> Are you sure? This code is what you added in your recent commit
> 0e18f584de59 ("efi_loader: LoadImage: always allocate new pages")
> at v2019.04-rc2.
I had that now rejected patch '[U-Boot,RFC,1/1] efi_loader: in situ
relocation' in mind. Maybe I should add a 'Fixes' tag pointing to
0e18f584de59.
If the caller provides a source buffer, we should not delete it. But we
there is also no need to copy it here because it is duplicated by the
relocation code.
If we read from file we need a receiving buffer, but we can delete it
once we have completed relocation.
>
> IMO, the comment:
>> /* In this case, file_path is the "device" path, i.e.
>> * something like a HARDWARE_DEVICE:MEMORY_MAPPED
>
> is also not accurate because "file_path" is normally non-NULL,
> indicating where the content of "source_buffer" comes from.
> In other words, "HARDWARE_DEVICE:MEMORY_MAPPED" is a quite irregular case.
Yes, the comment can be updated. "HARDWARE_DEVICE:MEMORY_MAPPED" is only
what we do in do_bootefi as workaround in some cases.
Regards
Heinrich
>
> -Takahiro Akashi
>
>
>> + dest_buffer = source_buffer;
>> dp = file_path;
>> fp = NULL;
>> }
>> ret = efi_setup_loaded_image(dp, fp, image_obj, &info);
>> - if (ret != EFI_SUCCESS)
>> - goto error_invalid_image;
>> - ret = efi_load_pe(*image_obj, source_buffer, info);
>> - if (ret != EFI_SUCCESS)
>> - goto error_invalid_image;
>> - /* 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);
>> -error_invalid_image:
>> - /* The image is invalid. Release all associated resources. */
>> - efi_free_pages((uintptr_t)source_buffer,
>> - efi_size_in_pages(source_size));
>> - efi_delete_handle(*image_handle);
>> - *image_handle = NULL;
>> - free(info);
>> + if (ret == EFI_SUCCESS)
>> + ret = efi_load_pe(*image_obj, dest_buffer, info);
>> + if (!source_buffer)
>> + /* Release buffer to which file was loaded */
>> + efi_free_pages((uintptr_t)dest_buffer,
>> + efi_size_in_pages(source_size));
>> + if (ret == EFI_SUCCESS) {
>> + info->system_table = &systab;
>> + info->parent_handle = parent_image;
>> + } else {
>> + /* The image is invalid. Release all associated resources. */
>> + efi_delete_handle(*image_handle);
>> + *image_handle = NULL;
>> + free(info);
>> + }
>> error:
>> return EFI_EXIT(ret);
>> }
>> --
>> 2.20.1
>>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-03-06 4:38 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-05 19:56 [U-Boot] [PATCH 1/1] efi_loader: release file buffer after loading image Heinrich Schuchardt
2019-03-06 0:41 ` AKASHI Takahiro
2019-03-06 4:38 ` Heinrich Schuchardt
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.