Re: [U-Boot] [PATCH 03/16] efi_loader: Add PE image loader

On 02.02.16 18:49, Mark Rutland wrote:

On Tue, Feb 02, 2016 at 03:45:01AM +0100, Alexander Graf wrote:

...

EFI uses the PE binary format for its application images. Add support to EFI PE binaries as well as all necessary bits for the "EFI image loader" interfaces.

Signed-off-by: Alexander Graf agraf@suse.de

---

v1 -> v2:

• move memory allocation to separate patch
• limit 32/64 to hosts that support it
• check 32bit optional nt header magic
• switch to GPL2+

v2 -> v3:

• use efi_alloc
• add EFIAPI to function prototypes
• remove unused macros
• reorder header inclusion
• split relocation code into function
• flush cache after loading

---

include/efi_loader.h | 20 +++ include/pe.h | 263 ++++++++++++++++++++++++++++++++++++++ lib/efi_loader/efi_image_loader.c | 182 ++++++++++++++++++++++++++ 3 files changed, 465 insertions(+) create mode 100644 include/efi_loader.h create mode 100644 include/pe.h create mode 100644 lib/efi_loader/efi_image_loader.c

[...]

...

+static void efi_loader_relocate(const IMAGE_BASE_RELOCATION *rel,

• 	unsigned long rel_size, void *efi_reloc)
  

+{

• const IMAGE_BASE_RELOCATION *end;
• int i;
• end = (const IMAGE_BASE_RELOCATION *)((const char *)rel + rel_size);
• while (rel < end - 1 && rel->SizeOfBlock) {

• const uint16_t *relocs = (const uint16_t *)(rel + 1);
  

• i = (rel->SizeOfBlock - sizeof(*rel)) / sizeof(uint16_t);
  

• while (i--) {
  

• 	uint16_t offset = (*relocs & 0xfff) +
  

• 			  rel->VirtualAddress;
  

• 	int type = *relocs >> 12;
  

• 	unsigned long delta = (unsigned long)efi_reloc;
  

• 	uint64_t *x64 = efi_reloc + offset;
  

• 	uint32_t *x32 = efi_reloc + offset;
  

• 	uint16_t *x16 = efi_reloc + offset;
  

• 	switch (type) {
  

• 	case IMAGE_REL_BASED_ABSOLUTE:
  

• 		break;
  

• 	case IMAGE_REL_BASED_HIGH:
  

• 		*x16 += ((uint32_t)delta) >> 16;
  

• 		break;
  

• 	case IMAGE_REL_BASED_LOW:
  

• 		*x16 += (uint16_t)delta;
  

• 		break;
  

• 	case IMAGE_REL_BASED_HIGHLOW:
  

• 		*x32 += (uint32_t)delta;
  

• 		break;
  

• 	case IMAGE_REL_BASED_DIR64:
  

• 		*x64 += (uint64_t)delta;
  

• 		break;
  

• 	default:
  

• 		printf("Unknown Relocation off %x type %x\n",
  

• 		       offset, type);
  

• 	}
  

• 	relocs++;
  

• }
  

• rel = (const IMAGE_BASE_RELOCATION *)relocs;
  

• }

+}

[...]

...

• /* 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 */
• efi_loader_relocate(rel, rel_size, efi_reloc);
• /* Flush cache */
• flush_cache((ulong)efi_reloc, virt_size);

Where's the I-cache maintenance for the image performed? I can't see it here and I didn't spot it in later patches.

I've added a call to invalidate_icache_all(); at this spot now.

Given that speculative instruction fetches can happen at any time for anything not marked NX, there may already be stale entries in the I-caches.

Also, flush_cache seems to perform DC CIVAC in a loop, which is excessively expensive. To make the instructions visible to instruction fetches you only need DC CVAU (i.e. clean by VA to the PoU), and you only need to do that for executable sections.

While I see where you're trying to aim, I don't think the performance overhead will be really worth premature optimization. flush_cache is a globally available operation for all u-boot target platforms. Having something that is platform agnostic (and very simple) is worth more than a few us performance for now imho.

Alex