[U-Boot] [RFC 1/1] efi_loader: in situ relocation

[U-Boot] [RFC 1/1] efi_loader: in situ relocation

[Heinrich Schuchardt]

All code and data sections of PE images are already in the correct relative
location when loaded into memory. There is not need to copy them once
again.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 lib/efi_loader/efi_image_loader.c | 64 ++++++-------------------------
 1 file changed, 11 insertions(+), 53 deletions(-)

diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
index fe66e7b9ffe..6146acc6b6f 100644
--- a/lib/efi_loader/efi_image_loader.c
+++ b/lib/efi_loader/efi_image_loader.c
@@ -2,7 +2,7 @@
 /*
  *  EFI image loader
  *
- *  based partly on wine code
+ *  based partly on Wine code
  *
  *  Copyright (c) 2016 Alexander Graf
  */
@@ -219,16 +219,12 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, void *efi,
 {
 	IMAGE_NT_HEADERS32 *nt;
 	IMAGE_DOS_HEADER *dos;
-	IMAGE_SECTION_HEADER *sections;
-	int num_sections;
-	void *efi_reloc;
 	int i;
 	const IMAGE_BASE_RELOCATION *rel;
 	unsigned long rel_size;
 	int rel_idx = IMAGE_DIRECTORY_ENTRY_BASERELOC;
 	uint64_t image_base;
 	uint64_t image_size;
-	unsigned long virt_size = 0;
 	int supported = 0;
 
 	dos = efi;
@@ -255,85 +251,47 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, void *efi,
 		return EFI_LOAD_ERROR;
 	}
 
-	/* Calculate upper virtual address boundary */
-	num_sections = nt->FileHeader.NumberOfSections;
-	sections = (void *)&nt->OptionalHeader +
-			    nt->FileHeader.SizeOfOptionalHeader;
-
-	for (i = num_sections - 1; i >= 0; i--) {
-		IMAGE_SECTION_HEADER *sec = &sections[i];
-		virt_size = max_t(unsigned long, virt_size,
-				  sec->VirtualAddress + sec->Misc.VirtualSize);
-	}
-
 	/* Read 32/64bit specific header bits */
 	if (nt->OptionalHeader.Magic == IMAGE_NT_OPTIONAL_HDR64_MAGIC) {
 		IMAGE_NT_HEADERS64 *nt64 = (void *)nt;
 		IMAGE_OPTIONAL_HEADER64 *opt = &nt64->OptionalHeader;
+
 		image_base = opt->ImageBase;
 		image_size = opt->SizeOfImage;
 		efi_set_code_and_data_type(loaded_image_info, opt->Subsystem);
-		efi_reloc = efi_alloc(virt_size,
-				      loaded_image_info->image_code_type);
-		if (!efi_reloc) {
-			printf("%s: Could not allocate %lu bytes\n",
-			       __func__, virt_size);
-			return EFI_OUT_OF_RESOURCES;
-		}
-		handle->entry = efi_reloc + opt->AddressOfEntryPoint;
+		handle->entry = efi + opt->AddressOfEntryPoint;
 		rel_size = opt->DataDirectory[rel_idx].Size;
-		rel = efi_reloc + opt->DataDirectory[rel_idx].VirtualAddress;
-		virt_size = ALIGN(virt_size, opt->SectionAlignment);
+		rel = efi + opt->DataDirectory[rel_idx].VirtualAddress;
 	} else if (nt->OptionalHeader.Magic == IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
 		IMAGE_OPTIONAL_HEADER32 *opt = &nt->OptionalHeader;
+
 		image_base = opt->ImageBase;
 		image_size = opt->SizeOfImage;
 		efi_set_code_and_data_type(loaded_image_info, opt->Subsystem);
-		efi_reloc = efi_alloc(virt_size,
-				      loaded_image_info->image_code_type);
-		if (!efi_reloc) {
-			printf("%s: Could not allocate %lu bytes\n",
-			       __func__, virt_size);
-			return EFI_OUT_OF_RESOURCES;
-		}
-		handle->entry = efi_reloc + opt->AddressOfEntryPoint;
+		handle->entry = efi + opt->AddressOfEntryPoint;
 		rel_size = opt->DataDirectory[rel_idx].Size;
-		rel = efi_reloc + opt->DataDirectory[rel_idx].VirtualAddress;
-		virt_size = ALIGN(virt_size, opt->SectionAlignment);
+		rel = efi + opt->DataDirectory[rel_idx].VirtualAddress;
 	} else {
 		printf("%s: Invalid optional header magic %x\n", __func__,
 		       nt->OptionalHeader.Magic);
 		return EFI_LOAD_ERROR;
 	}
 
-	/* Load sections into RAM */
-	for (i = num_sections - 1; i >= 0; i--) {
-		IMAGE_SECTION_HEADER *sec = &sections[i];
-		memset(efi_reloc + sec->VirtualAddress, 0,
-		       sec->Misc.VirtualSize);
-		memcpy(efi_reloc + sec->VirtualAddress,
-		       efi + sec->PointerToRawData,
-		       sec->SizeOfRawData);
-	}
-
 	/* Run through relocations */
-	if (efi_loader_relocate(rel, rel_size, efi_reloc,
+	if (efi_loader_relocate(rel, rel_size, efi,
 				(unsigned long)image_base) != EFI_SUCCESS) {
-		efi_free_pages((uintptr_t) efi_reloc,
-			       (virt_size + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT);
 		return EFI_LOAD_ERROR;
 	}
 
 	/* Flush cache */
-	flush_cache((ulong)efi_reloc,
-		    ALIGN(virt_size, EFI_CACHELINE_SIZE));
+	flush_cache((ulong)efi, ALIGN(image_size, EFI_CACHELINE_SIZE));
 	invalidate_icache_all();
 
 	/* Populate the loaded image interface bits */
 	loaded_image_info->image_base = efi;
 	loaded_image_info->image_size = image_size;
-	handle->reloc_base = efi_reloc;
-	handle->reloc_size = virt_size;
+	handle->reloc_base = efi;
+	handle->reloc_size = image_size;
 
 	return EFI_SUCCESS;
 }
-- 
2.20.1

[U-Boot] [RFC 1/1] efi_loader: in situ relocation

[Heinrich Schuchardt]

All code and data sections of PE images are already in the correct relative
location when loaded into memory. There is not need to copy them once
again.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 lib/efi_loader/efi_image_loader.c | 64 ++++++-------------------------
 1 file changed, 11 insertions(+), 53 deletions(-)

diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
index fe66e7b9ffe..6146acc6b6f 100644
--- a/lib/efi_loader/efi_image_loader.c
+++ b/lib/efi_loader/efi_image_loader.c
@@ -2,7 +2,7 @@
 /*
  *  EFI image loader
  *
- *  based partly on wine code
+ *  based partly on Wine code
  *
  *  Copyright (c) 2016 Alexander Graf
  */
@@ -219,16 +219,12 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, void *efi,
 {
 	IMAGE_NT_HEADERS32 *nt;
 	IMAGE_DOS_HEADER *dos;
-	IMAGE_SECTION_HEADER *sections;
-	int num_sections;
-	void *efi_reloc;
 	int i;
 	const IMAGE_BASE_RELOCATION *rel;
 	unsigned long rel_size;
 	int rel_idx = IMAGE_DIRECTORY_ENTRY_BASERELOC;
 	uint64_t image_base;
 	uint64_t image_size;
-	unsigned long virt_size = 0;
 	int supported = 0;
 
 	dos = efi;
@@ -255,85 +251,47 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, void *efi,
 		return EFI_LOAD_ERROR;
 	}
 
-	/* Calculate upper virtual address boundary */
-	num_sections = nt->FileHeader.NumberOfSections;
-	sections = (void *)&nt->OptionalHeader +
-			    nt->FileHeader.SizeOfOptionalHeader;
-
-	for (i = num_sections - 1; i >= 0; i--) {
-		IMAGE_SECTION_HEADER *sec = &sections[i];
-		virt_size = max_t(unsigned long, virt_size,
-				  sec->VirtualAddress + sec->Misc.VirtualSize);
-	}
-
 	/* Read 32/64bit specific header bits */
 	if (nt->OptionalHeader.Magic == IMAGE_NT_OPTIONAL_HDR64_MAGIC) {
 		IMAGE_NT_HEADERS64 *nt64 = (void *)nt;
 		IMAGE_OPTIONAL_HEADER64 *opt = &nt64->OptionalHeader;
+
 		image_base = opt->ImageBase;
 		image_size = opt->SizeOfImage;
 		efi_set_code_and_data_type(loaded_image_info, opt->Subsystem);
-		efi_reloc = efi_alloc(virt_size,
-				      loaded_image_info->image_code_type);
-		if (!efi_reloc) {
-			printf("%s: Could not allocate %lu bytes\n",
-			       __func__, virt_size);
-			return EFI_OUT_OF_RESOURCES;
-		}
-		handle->entry = efi_reloc + opt->AddressOfEntryPoint;
+		handle->entry = efi + opt->AddressOfEntryPoint;
 		rel_size = opt->DataDirectory[rel_idx].Size;
-		rel = efi_reloc + opt->DataDirectory[rel_idx].VirtualAddress;
-		virt_size = ALIGN(virt_size, opt->SectionAlignment);
+		rel = efi + opt->DataDirectory[rel_idx].VirtualAddress;
 	} else if (nt->OptionalHeader.Magic == IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
 		IMAGE_OPTIONAL_HEADER32 *opt = &nt->OptionalHeader;
+
 		image_base = opt->ImageBase;
 		image_size = opt->SizeOfImage;
 		efi_set_code_and_data_type(loaded_image_info, opt->Subsystem);
-		efi_reloc = efi_alloc(virt_size,
-				      loaded_image_info->image_code_type);
-		if (!efi_reloc) {
-			printf("%s: Could not allocate %lu bytes\n",
-			       __func__, virt_size);
-			return EFI_OUT_OF_RESOURCES;
-		}
-		handle->entry = efi_reloc + opt->AddressOfEntryPoint;
+		handle->entry = efi + opt->AddressOfEntryPoint;
 		rel_size = opt->DataDirectory[rel_idx].Size;
-		rel = efi_reloc + opt->DataDirectory[rel_idx].VirtualAddress;
-		virt_size = ALIGN(virt_size, opt->SectionAlignment);
+		rel = efi + opt->DataDirectory[rel_idx].VirtualAddress;
 	} else {
 		printf("%s: Invalid optional header magic %x\n", __func__,
 		       nt->OptionalHeader.Magic);
 		return EFI_LOAD_ERROR;
 	}
 
-	/* Load sections into RAM */
-	for (i = num_sections - 1; i >= 0; i--) {
-		IMAGE_SECTION_HEADER *sec = &sections[i];
-		memset(efi_reloc + sec->VirtualAddress, 0,
-		       sec->Misc.VirtualSize);
-		memcpy(efi_reloc + sec->VirtualAddress,
-		       efi + sec->PointerToRawData,
-		       sec->SizeOfRawData);
-	}
-
 	/* Run through relocations */
-	if (efi_loader_relocate(rel, rel_size, efi_reloc,
+	if (efi_loader_relocate(rel, rel_size, efi,
 				(unsigned long)image_base) != EFI_SUCCESS) {
-		efi_free_pages((uintptr_t) efi_reloc,
-			       (virt_size + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT);
 		return EFI_LOAD_ERROR;
 	}
 
 	/* Flush cache */
-	flush_cache((ulong)efi_reloc,
-		    ALIGN(virt_size, EFI_CACHELINE_SIZE));
+	flush_cache((ulong)efi, ALIGN(image_size, EFI_CACHELINE_SIZE));
 	invalidate_icache_all();
 
 	/* Populate the loaded image interface bits */
 	loaded_image_info->image_base = efi;
 	loaded_image_info->image_size = image_size;
-	handle->reloc_base = efi_reloc;
-	handle->reloc_size = virt_size;
+	handle->reloc_base = efi;
+	handle->reloc_size = image_size;
 
 	return EFI_SUCCESS;
 }
-- 
2.20.1

[U-Boot] [RFC 1/1] efi_loader: in situ relocation

[AKASHI Takahiro]

Heinrich,

On Sat, Feb 16, 2019 at 08:50:43PM +0100, Heinrich Schuchardt wrote:
> All code and data sections of PE images are already in the correct relative
> location when loaded into memory. There is not need to copy them once
> again.

While I'm not very familiar with how PE image is created (in EDK2),
what I understand in Alex's code is
* All the code and data are located starting 0x0 (in virtual space)
* Sections in PE image may not be sorted in ascending order
* There may be some gaps (more than one page) between sections,
  probably, due to alignment requirements or BSS

Do you say that those assumptions are no longer correct?

Thanks,
-Takahiro Akashi

> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  lib/efi_loader/efi_image_loader.c | 64 ++++++-------------------------
>  1 file changed, 11 insertions(+), 53 deletions(-)
> 
> diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
> index fe66e7b9ffe..6146acc6b6f 100644
> --- a/lib/efi_loader/efi_image_loader.c
> +++ b/lib/efi_loader/efi_image_loader.c
> @@ -2,7 +2,7 @@
>  /*
>   *  EFI image loader
>   *
> - *  based partly on wine code
> + *  based partly on Wine code
>   *
>   *  Copyright (c) 2016 Alexander Graf
>   */
> @@ -219,16 +219,12 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, void *efi,
>  {
>  	IMAGE_NT_HEADERS32 *nt;
>  	IMAGE_DOS_HEADER *dos;
> -	IMAGE_SECTION_HEADER *sections;
> -	int num_sections;
> -	void *efi_reloc;
>  	int i;
>  	const IMAGE_BASE_RELOCATION *rel;
>  	unsigned long rel_size;
>  	int rel_idx = IMAGE_DIRECTORY_ENTRY_BASERELOC;
>  	uint64_t image_base;
>  	uint64_t image_size;
> -	unsigned long virt_size = 0;
>  	int supported = 0;
>  
>  	dos = efi;
> @@ -255,85 +251,47 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, void *efi,
>  		return EFI_LOAD_ERROR;
>  	}
>  
> -	/* Calculate upper virtual address boundary */
> -	num_sections = nt->FileHeader.NumberOfSections;
> -	sections = (void *)&nt->OptionalHeader +
> -			    nt->FileHeader.SizeOfOptionalHeader;
> -
> -	for (i = num_sections - 1; i >= 0; i--) {
> -		IMAGE_SECTION_HEADER *sec = &sections[i];
> -		virt_size = max_t(unsigned long, virt_size,
> -				  sec->VirtualAddress + sec->Misc.VirtualSize);
> -	}
> -
>  	/* Read 32/64bit specific header bits */
>  	if (nt->OptionalHeader.Magic == IMAGE_NT_OPTIONAL_HDR64_MAGIC) {
>  		IMAGE_NT_HEADERS64 *nt64 = (void *)nt;
>  		IMAGE_OPTIONAL_HEADER64 *opt = &nt64->OptionalHeader;
> +
>  		image_base = opt->ImageBase;
>  		image_size = opt->SizeOfImage;
>  		efi_set_code_and_data_type(loaded_image_info, opt->Subsystem);
> -		efi_reloc = efi_alloc(virt_size,
> -				      loaded_image_info->image_code_type);
> -		if (!efi_reloc) {
> -			printf("%s: Could not allocate %lu bytes\n",
> -			       __func__, virt_size);
> -			return EFI_OUT_OF_RESOURCES;
> -		}
> -		handle->entry = efi_reloc + opt->AddressOfEntryPoint;
> +		handle->entry = efi + opt->AddressOfEntryPoint;
>  		rel_size = opt->DataDirectory[rel_idx].Size;
> -		rel = efi_reloc + opt->DataDirectory[rel_idx].VirtualAddress;
> -		virt_size = ALIGN(virt_size, opt->SectionAlignment);
> +		rel = efi + opt->DataDirectory[rel_idx].VirtualAddress;
>  	} else if (nt->OptionalHeader.Magic == IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>  		IMAGE_OPTIONAL_HEADER32 *opt = &nt->OptionalHeader;
> +
>  		image_base = opt->ImageBase;
>  		image_size = opt->SizeOfImage;
>  		efi_set_code_and_data_type(loaded_image_info, opt->Subsystem);
> -		efi_reloc = efi_alloc(virt_size,
> -				      loaded_image_info->image_code_type);
> -		if (!efi_reloc) {
> -			printf("%s: Could not allocate %lu bytes\n",
> -			       __func__, virt_size);
> -			return EFI_OUT_OF_RESOURCES;
> -		}
> -		handle->entry = efi_reloc + opt->AddressOfEntryPoint;
> +		handle->entry = efi + opt->AddressOfEntryPoint;
>  		rel_size = opt->DataDirectory[rel_idx].Size;
> -		rel = efi_reloc + opt->DataDirectory[rel_idx].VirtualAddress;
> -		virt_size = ALIGN(virt_size, opt->SectionAlignment);
> +		rel = efi + opt->DataDirectory[rel_idx].VirtualAddress;
>  	} else {
>  		printf("%s: Invalid optional header magic %x\n", __func__,
>  		       nt->OptionalHeader.Magic);
>  		return EFI_LOAD_ERROR;
>  	}
>  
> -	/* Load sections into RAM */
> -	for (i = num_sections - 1; i >= 0; i--) {
> -		IMAGE_SECTION_HEADER *sec = &sections[i];
> -		memset(efi_reloc + sec->VirtualAddress, 0,
> -		       sec->Misc.VirtualSize);
> -		memcpy(efi_reloc + sec->VirtualAddress,
> -		       efi + sec->PointerToRawData,
> -		       sec->SizeOfRawData);
> -	}
> -
>  	/* Run through relocations */
> -	if (efi_loader_relocate(rel, rel_size, efi_reloc,
> +	if (efi_loader_relocate(rel, rel_size, efi,
>  				(unsigned long)image_base) != EFI_SUCCESS) {
> -		efi_free_pages((uintptr_t) efi_reloc,
> -			       (virt_size + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT);
>  		return EFI_LOAD_ERROR;
>  	}
>  
>  	/* Flush cache */
> -	flush_cache((ulong)efi_reloc,
> -		    ALIGN(virt_size, EFI_CACHELINE_SIZE));
> +	flush_cache((ulong)efi, ALIGN(image_size, EFI_CACHELINE_SIZE));
>  	invalidate_icache_all();
>  
>  	/* Populate the loaded image interface bits */
>  	loaded_image_info->image_base = efi;
>  	loaded_image_info->image_size = image_size;
> -	handle->reloc_base = efi_reloc;
> -	handle->reloc_size = virt_size;
> +	handle->reloc_base = efi;
> +	handle->reloc_size = image_size;
>  
>  	return EFI_SUCCESS;
>  }
> -- 
> 2.20.1
> 

[U-Boot] [RFC 1/1] efi_loader: in situ relocation

[Heinrich Schuchardt]

On 2/18/19 1:52 AM, AKASHI Takahiro wrote:
> Heinrich,
> 
> On Sat, Feb 16, 2019 at 08:50:43PM +0100, Heinrich Schuchardt wrote:
>> All code and data sections of PE images are already in the correct relative
>> location when loaded into memory. There is not need to copy them once
>> again.
> 
> While I'm not very familiar with how PE image is created (in EDK2),
The relevant reference is the Microsoft Portable Executable and
Common Object File Format Specification. The latest version as PDF that
I found is revision 11, Jan 23rd, 2017. The citations are from that
version. Later versions are available as HTML.

> what I understand in Alex's code is
> * All the code and data are located starting 0x0 (in virtual space)
The header provides a field ImageBase. If you load the image at this
address there is no need for relocation. I could not find any rule
saying ImageBase has to be zero. It has be a multiple of 64 KiB. For
Windows non-zero defaults are provided in the spec.

> * Sections in PE image may not be sorted in ascending order
The spec says: "The physical offset for section data is the same as the
RVA". The relative virtual address is defined as "In an image file, the
address of an item after it is loaded into memory, with the base address
of the image file subtracted from it."

> * There may be some gaps (more than one page) between sections,
>   probably, due to alignment requirements or BSS
Yes, due to alignment there may be some gap filling bytes.

> 
> Do you say that those assumptions are no longer correct?
The most important sentence concerning relocations in the spec is:

"If the image is loaded at its preferred base, ... the base relocations
do not have to be applied."

This implies that there is not need to modify the relative position of
sections after loading.

Best regards

Heinrich

> 
> Thanks,
> -Takahiro Akashi
> 
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>>  lib/efi_loader/efi_image_loader.c | 64 ++++++-------------------------
>>  1 file changed, 11 insertions(+), 53 deletions(-)
>>
>> diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
>> index fe66e7b9ffe..6146acc6b6f 100644
>> --- a/lib/efi_loader/efi_image_loader.c
>> +++ b/lib/efi_loader/efi_image_loader.c
>> @@ -2,7 +2,7 @@
>>  /*
>>   *  EFI image loader
>>   *
>> - *  based partly on wine code
>> + *  based partly on Wine code
>>   *
>>   *  Copyright (c) 2016 Alexander Graf
>>   */
>> @@ -219,16 +219,12 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, void *efi,
>>  {
>>  	IMAGE_NT_HEADERS32 *nt;
>>  	IMAGE_DOS_HEADER *dos;
>> -	IMAGE_SECTION_HEADER *sections;
>> -	int num_sections;
>> -	void *efi_reloc;
>>  	int i;
>>  	const IMAGE_BASE_RELOCATION *rel;
>>  	unsigned long rel_size;
>>  	int rel_idx = IMAGE_DIRECTORY_ENTRY_BASERELOC;
>>  	uint64_t image_base;
>>  	uint64_t image_size;
>> -	unsigned long virt_size = 0;
>>  	int supported = 0;
>>  
>>  	dos = efi;
>> @@ -255,85 +251,47 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, void *efi,
>>  		return EFI_LOAD_ERROR;
>>  	}
>>  
>> -	/* Calculate upper virtual address boundary */
>> -	num_sections = nt->FileHeader.NumberOfSections;
>> -	sections = (void *)&nt->OptionalHeader +
>> -			    nt->FileHeader.SizeOfOptionalHeader;
>> -
>> -	for (i = num_sections - 1; i >= 0; i--) {
>> -		IMAGE_SECTION_HEADER *sec = &sections[i];
>> -		virt_size = max_t(unsigned long, virt_size,
>> -				  sec->VirtualAddress + sec->Misc.VirtualSize);
>> -	}
>> -
>>  	/* Read 32/64bit specific header bits */
>>  	if (nt->OptionalHeader.Magic == IMAGE_NT_OPTIONAL_HDR64_MAGIC) {
>>  		IMAGE_NT_HEADERS64 *nt64 = (void *)nt;
>>  		IMAGE_OPTIONAL_HEADER64 *opt = &nt64->OptionalHeader;
>> +
>>  		image_base = opt->ImageBase;
>>  		image_size = opt->SizeOfImage;
>>  		efi_set_code_and_data_type(loaded_image_info, opt->Subsystem);
>> -		efi_reloc = efi_alloc(virt_size,
>> -				      loaded_image_info->image_code_type);
>> -		if (!efi_reloc) {
>> -			printf("%s: Could not allocate %lu bytes\n",
>> -			       __func__, virt_size);
>> -			return EFI_OUT_OF_RESOURCES;
>> -		}
>> -		handle->entry = efi_reloc + opt->AddressOfEntryPoint;
>> +		handle->entry = efi + opt->AddressOfEntryPoint;
>>  		rel_size = opt->DataDirectory[rel_idx].Size;
>> -		rel = efi_reloc + opt->DataDirectory[rel_idx].VirtualAddress;
>> -		virt_size = ALIGN(virt_size, opt->SectionAlignment);
>> +		rel = efi + opt->DataDirectory[rel_idx].VirtualAddress;
>>  	} else if (nt->OptionalHeader.Magic == IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>>  		IMAGE_OPTIONAL_HEADER32 *opt = &nt->OptionalHeader;
>> +
>>  		image_base = opt->ImageBase;
>>  		image_size = opt->SizeOfImage;
>>  		efi_set_code_and_data_type(loaded_image_info, opt->Subsystem);
>> -		efi_reloc = efi_alloc(virt_size,
>> -				      loaded_image_info->image_code_type);
>> -		if (!efi_reloc) {
>> -			printf("%s: Could not allocate %lu bytes\n",
>> -			       __func__, virt_size);
>> -			return EFI_OUT_OF_RESOURCES;
>> -		}
>> -		handle->entry = efi_reloc + opt->AddressOfEntryPoint;
>> +		handle->entry = efi + opt->AddressOfEntryPoint;
>>  		rel_size = opt->DataDirectory[rel_idx].Size;
>> -		rel = efi_reloc + opt->DataDirectory[rel_idx].VirtualAddress;
>> -		virt_size = ALIGN(virt_size, opt->SectionAlignment);
>> +		rel = efi + opt->DataDirectory[rel_idx].VirtualAddress;
>>  	} else {
>>  		printf("%s: Invalid optional header magic %x\n", __func__,
>>  		       nt->OptionalHeader.Magic);
>>  		return EFI_LOAD_ERROR;
>>  	}
>>  
>> -	/* Load sections into RAM */
>> -	for (i = num_sections - 1; i >= 0; i--) {
>> -		IMAGE_SECTION_HEADER *sec = &sections[i];
>> -		memset(efi_reloc + sec->VirtualAddress, 0,
>> -		       sec->Misc.VirtualSize);
>> -		memcpy(efi_reloc + sec->VirtualAddress,
>> -		       efi + sec->PointerToRawData,
>> -		       sec->SizeOfRawData);
>> -	}
>> -
>>  	/* Run through relocations */
>> -	if (efi_loader_relocate(rel, rel_size, efi_reloc,
>> +	if (efi_loader_relocate(rel, rel_size, efi,
>>  				(unsigned long)image_base) != EFI_SUCCESS) {
>> -		efi_free_pages((uintptr_t) efi_reloc,
>> -			       (virt_size + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT);
>>  		return EFI_LOAD_ERROR;
>>  	}
>>  
>>  	/* Flush cache */
>> -	flush_cache((ulong)efi_reloc,
>> -		    ALIGN(virt_size, EFI_CACHELINE_SIZE));
>> +	flush_cache((ulong)efi, ALIGN(image_size, EFI_CACHELINE_SIZE));
>>  	invalidate_icache_all();
>>  
>>  	/* Populate the loaded image interface bits */
>>  	loaded_image_info->image_base = efi;
>>  	loaded_image_info->image_size = image_size;
>> -	handle->reloc_base = efi_reloc;
>> -	handle->reloc_size = virt_size;
>> +	handle->reloc_base = efi;
>> +	handle->reloc_size = image_size;
>>  
>>  	return EFI_SUCCESS;
>>  }
>> -- 
>> 2.20.1
>>
> 

[U-Boot] [RFC 1/1] efi_loader: in situ relocation

[Alexander Graf]

On 02/20/2019 07:12 PM, Heinrich Schuchardt wrote:
> On 2/18/19 1:52 AM, AKASHI Takahiro wrote:
>> Heinrich,
>>
>> On Sat, Feb 16, 2019 at 08:50:43PM +0100, Heinrich Schuchardt wrote:
>>> All code and data sections of PE images are already in the correct relative
>>> location when loaded into memory. There is not need to copy them once
>>> again.
>> While I'm not very familiar with how PE image is created (in EDK2),
> The relevant reference is the Microsoft Portable Executable and
> Common Object File Format Specification. The latest version as PDF that
> I found is revision 11, Jan 23rd, 2017. The citations are from that
> version. Later versions are available as HTML.
>
>> what I understand in Alex's code is
>> * All the code and data are located starting 0x0 (in virtual space)
> The header provides a field ImageBase. If you load the image at this
> address there is no need for relocation. I could not find any rule
> saying ImageBase has to be zero. It has be a multiple of 64 KiB. For
> Windows non-zero defaults are provided in the spec.
>
>> * Sections in PE image may not be sorted in ascending order
> The spec says: "The physical offset for section data is the same as the
> RVA". The relative virtual address is defined as "In an image file, the
> address of an item after it is loaded into memory, with the base address
> of the image file subtracted from it."
>
>> * There may be some gaps (more than one page) between sections,
>>    probably, due to alignment requirements or BSS
> Yes, due to alignment there may be some gap filling bytes.
>
>> Do you say that those assumptions are no longer correct?
> The most important sentence concerning relocations in the spec is:
>
> "If the image is loaded at its preferred base, ... the base relocations
> do not have to be applied."

Yes, but image loading also implies that we actually load the sections 
to particular offsets with particular section alignment. You can have a 
PE binary that aligns its sections in 32 byte granule, but expects the 
sections to get loaded at 4kb alignment. In such a case, I don't see how 
we can get away to not copy the image.


Alex

[U-Boot] [RFC 1/1] efi_loader: in situ relocation

[Heinrich Schuchardt]

On 2/21/19 11:21 AM, Alexander Graf wrote:
> On 02/20/2019 07:12 PM, Heinrich Schuchardt wrote:
>> On 2/18/19 1:52 AM, AKASHI Takahiro wrote:
>>> Heinrich,
>>>
>>> On Sat, Feb 16, 2019 at 08:50:43PM +0100, Heinrich Schuchardt wrote:
>>>> All code and data sections of PE images are already in the correct
>>>> relative
>>>> location when loaded into memory. There is not need to copy them once
>>>> again.
>>> While I'm not very familiar with how PE image is created (in EDK2),
>> The relevant reference is the Microsoft Portable Executable and
>> Common Object File Format Specification. The latest version as PDF that
>> I found is revision 11, Jan 23rd, 2017. The citations are from that
>> version. Later versions are available as HTML.
>>
>>> what I understand in Alex's code is
>>> * All the code and data are located starting 0x0 (in virtual space)
>> The header provides a field ImageBase. If you load the image at this
>> address there is no need for relocation. I could not find any rule
>> saying ImageBase has to be zero. It has be a multiple of 64 KiB. For
>> Windows non-zero defaults are provided in the spec.
>>
>>> * Sections in PE image may not be sorted in ascending order
>> The spec says: "The physical offset for section data is the same as the
>> RVA". The relative virtual address is defined as "In an image file, the
>> address of an item after it is loaded into memory, with the base address
>> of the image file subtracted from it."
>>
>>> * There may be some gaps (more than one page) between sections,
>>>    probably, due to alignment requirements or BSS
>> Yes, due to alignment there may be some gap filling bytes.
>>
>>> Do you say that those assumptions are no longer correct?
>> The most important sentence concerning relocations in the spec is:
>>
>> "If the image is loaded at its preferred base, ... the base relocations
>> do not have to be applied."
> 
> Yes, but image loading also implies that we actually load the sections
> to particular offsets with particular section alignment. You can have a
> PE binary that aligns its sections in 32 byte granule, but expects the
> sections to get loaded at 4kb alignment. In such a case, I don't see how
> we can get away to not copy the image.
> 

Thanks Alex and Takhiro for reviewing.

Reading the fine print in the spec I now saw that segment alignment is
typically smaller than file alignment.

Strange that even the EFI shell would work with my patch in.

What we should be able to do is to release the buffer used for reading
from file once we have done the relocation, so that after LoadImage we
end up with only one memory area allocated for the image.

Best regards

Heinrich

[U-Boot] [RFC 1/1] efi_loader: in situ relocation

[AKASHI Takahiro]

On Thu, Feb 21, 2019 at 07:55:46PM +0100, Heinrich Schuchardt wrote:
> On 2/21/19 11:21 AM, Alexander Graf wrote:
> > On 02/20/2019 07:12 PM, Heinrich Schuchardt wrote:
> >> On 2/18/19 1:52 AM, AKASHI Takahiro wrote:
> >>> Heinrich,
> >>>
> >>> On Sat, Feb 16, 2019 at 08:50:43PM +0100, Heinrich Schuchardt wrote:
> >>>> All code and data sections of PE images are already in the correct
> >>>> relative
> >>>> location when loaded into memory. There is not need to copy them once
> >>>> again.
> >>> While I'm not very familiar with how PE image is created (in EDK2),
> >> The relevant reference is the Microsoft Portable Executable and
> >> Common Object File Format Specification. The latest version as PDF that
> >> I found is revision 11, Jan 23rd, 2017. The citations are from that
> >> version. Later versions are available as HTML.
> >>
> >>> what I understand in Alex's code is
> >>> * All the code and data are located starting 0x0 (in virtual space)
> >> The header provides a field ImageBase. If you load the image at this
> >> address there is no need for relocation. I could not find any rule
> >> saying ImageBase has to be zero. It has be a multiple of 64 KiB. For
> >> Windows non-zero defaults are provided in the spec.
> >>
> >>> * Sections in PE image may not be sorted in ascending order
> >> The spec says: "The physical offset for section data is the same as the
> >> RVA". The relative virtual address is defined as "In an image file, the
> >> address of an item after it is loaded into memory, with the base address
> >> of the image file subtracted from it."
> >>
> >>> * There may be some gaps (more than one page) between sections,
> >>>    probably, due to alignment requirements or BSS
> >> Yes, due to alignment there may be some gap filling bytes.
> >>
> >>> Do you say that those assumptions are no longer correct?
> >> The most important sentence concerning relocations in the spec is:
> >>
> >> "If the image is loaded at its preferred base, ... the base relocations
> >> do not have to be applied."
> > 
> > Yes, but image loading also implies that we actually load the sections
> > to particular offsets with particular section alignment. You can have a
> > PE binary that aligns its sections in 32 byte granule, but expects the
> > sections to get loaded at 4kb alignment. In such a case, I don't see how
> > we can get away to not copy the image.
> > 
> 
> Thanks Alex and Takhiro for reviewing.
> 
> Reading the fine print in the spec I now saw that segment alignment is
> typically smaller than file alignment.
> 
> Strange that even the EFI shell would work with my patch in.
> 
> What we should be able to do is to release the buffer used for reading
> from file once we have done the relocation, so that after LoadImage we
> end up with only one memory area allocated for the image.

Finally, you've got my old patch ("efi_loader: set image_base and image_size
to correct values":
  https://lists.denx.de/pipermail/u-boot/2018-August/337708.html

-Takahiro Akashi

> Best regards
> 
> Heinrich