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 <ag...@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 = §ions[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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
