Re: [Qemu-arm] [PATCH v1 17/17] arm: boot: Support big-endian elfs

[Peter Crosthwaite]

On Tue, Jan 19, 2016 at 10:06 AM, Peter Maydell
<address@hidden> wrote:
> On 18 January 2016 at 07:12, Peter Crosthwaite
> <address@hidden> wrote:
>> Support ARM big-endian ELF files in system-mode emulation. When loading
>> an elf, determine the endianness mode expected by the elf, and set the
>> relevant CPU state accordingly.
>>
>> With this, big-endian modes are now fully supported via system-mode LE,
>> so there is no need to restrict the elf loading to the TARGET
>> endianness so the ifdeffery on TARGET_WORDS_BIGENDIAN goes away.
>>
>> Signed-off-by: Peter Crosthwaite <address@hidden>
>> ---
>>
>>  hw/arm/boot.c        | 96 
>> ++++++++++++++++++++++++++++++++++++++++++----------
>>  include/hw/arm/arm.h |  9 +++++
>>  2 files changed, 88 insertions(+), 17 deletions(-)
>>
>> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
>> index 0de4269..053c9e8 100644
>> --- a/hw/arm/boot.c
>> +++ b/hw/arm/boot.c
>> @@ -465,9 +465,34 @@ static void do_cpu_reset(void *opaque)
>>      cpu_reset(cs);
>>      if (info) {
>>          if (!info->is_linux) {
>> +            int i;
>>              /* Jump to the entry point.  */
>>              uint64_t entry = info->entry;
>>
>> +            switch (info->endianness) {
>> +            case ARM_ENDIANNESS_LE:
>> +                env->cp15.sctlr_el[1] &= ~SCTLR_E0E;
>> +                for (i = 1; i < 4; ++i) {
>> +                    env->cp15.sctlr_el[i] &= ~SCTLR_EE;
>> +                }
>> +                env->uncached_cpsr &= ~CPSR_E;
>> +                break;
>> +            case ARM_ENDIANNESS_BE8:
>> +                env->cp15.sctlr_el[1] |= SCTLR_E0E;
>> +                for (i = 1; i < 4; ++i) {
>> +                    env->cp15.sctlr_el[i] |= SCTLR_EE;
>> +                }
>> +                env->uncached_cpsr |= CPSR_E;
>> +                break;
>> +            case ARM_ENDIANNESS_BE32:
>> +                env->cp15.sctlr_el[1] |= SCTLR_B;
>> +                break;
>> +            case ARM_ENDIANNESS_UNKNOWN:
>> +                break; /* Board's decision */
>> +            default:
>> +                g_assert_not_reached();
>> +            }
>
> Do we really want this much magic for non-linux images?

I think so ...

>  I would
> expect that the image would be intended to run with whatever the
> state the board puts the CPU in from reset (ie the CPU has suitable
> QOM properties for its initial endianness state, corresponding to
> real hardware reset-config signals like the A15's CFGEND/CFGTE).
>

As with this you can handle two use cases:

1: Elfs for firmware development
2: Elfs for a real-world loadable guest.

Firmware elfs should match the hardwired (QOM properties) settings
anyway, but the more interesting case is the real-world loadable guest
ELF. That is, you have some elf-capable bootloader in real HW which
will DTRT based on the elf header before handoff. This will emulate
that case without needing to demote your elf to raw binary data (and
then explicitly load your bootloader).

>> +
>>              if (!env->aarch64) {
>>                  env->thumb = info->entry & 1;
>>                  entry &= 0xfffffffe;
>> @@ -589,16 +614,23 @@ static void arm_load_kernel_notify(Notifier *notifier, 
>> void *data)
>>      int kernel_size;
>>      int initrd_size;
>>      int is_linux = 0;
>> +
>>      uint64_t elf_entry, elf_low_addr, elf_high_addr;
>>      int elf_machine;
>> +    bool elf_is64;
>> +    union {
>> +        Elf32_Ehdr h32;
>> +        Elf64_Ehdr h64;
>> +    } elf_header;
>> +
>>      hwaddr entry, kernel_load_offset;
>> -    int big_endian;
>>      static const ARMInsnFixup *primary_loader;
>>      ArmLoadKernelNotifier *n = DO_UPCAST(ArmLoadKernelNotifier,
>>                                           notifier, notifier);
>>      ARMCPU *cpu = n->cpu;
>>      struct arm_boot_info *info =
>>          container_of(n, struct arm_boot_info, load_kernel_notifier);
>> +    Error *err = NULL;
>>
>>      /* The board code is not supposed to set secure_board_setup unless
>>       * running its code in secure mode is actually possible, and KVM
>> @@ -678,12 +710,6 @@ static void arm_load_kernel_notify(Notifier *notifier, 
>> void *data)
>>      if (info->nb_cpus == 0)
>>          info->nb_cpus = 1;
>>
>> -#ifdef TARGET_WORDS_BIGENDIAN
>> -    big_endian = 1;
>> -#else
>> -    big_endian = 0;
>> -#endif
>
> Was this code ever built with TARGET_WORDS_BIGENDIAN defined?
>

No I believe not. I can do the dead code cleanup as a separate patch?

>> -
>>      /* We want to put the initrd far enough into RAM that when the
>>       * kernel is uncompressed it will not clobber the initrd. However
>>       * on boards without much RAM we must ensure that we still leave
>> @@ -698,16 +724,52 @@ static void arm_load_kernel_notify(Notifier *notifier, 
>> void *data)
>>          MIN(info->ram_size / 2, 128 * 1024 * 1024);
>>
>>      /* Assume that raw images are linux kernels, and ELF images are not.  */
>> -    kernel_size = load_elf(info->kernel_filename, NULL, NULL, &elf_entry,
>> -                           &elf_low_addr, &elf_high_addr, big_endian,
>> -                           elf_machine, 1, 0);
>> -    if (kernel_size > 0 && have_dtb(info)) {
>> -        /* If there is still some room left at the base of RAM, try and put
>> -         * the DTB there like we do for images loaded with -bios or -pflash.
>> -         */
>> -        if (elf_low_addr > info->loader_start
>> -            || elf_high_addr < info->loader_start) {
>> -            /* Pass elf_low_addr as address limit to load_dtb if it may be
>> +
>> +    load_elf_hdr(info->kernel_filename, &elf_header, &elf_is64, &err);
>> +
>> +    if (!err) {
>> +        int data_swab = 0;
>> +        bool big_endian;
>> +
>> +        if (elf_is64) {
>> +            big_endian = elf_header.h64.e_ident[EI_DATA] == ELFDATA2MSB;
>> +            info->endianness = big_endian ? ARM_ENDIANNESS_BE8
>> +                                          : ARM_ENDIANNESS_LE;
>> +        } else {
>> +            big_endian = elf_header.h32.e_ident[EI_DATA] == ELFDATA2MSB;
>> +            if (big_endian) {
>> +                if (bswap32(elf_header.h32.e_flags) & EF_ARM_BE8) {
>> +                    info->endianness = ARM_ENDIANNESS_BE8;
>> +                } else {
>> +                    info->endianness = ARM_ENDIANNESS_BE32;
>> +                    /* In BE32, the CPU has a different view of the per-byte
>> +                     * address map than the rest of the system. BE32 elfs 
>> are
>> +                     * organised such that they can be programmed through 
>> the
>> +                     * CPUs per-word byte-reversed view of the world. QEMU
>> +                     * however loads elfs independently of the CPU. So tell
>> +                     * the elf loader to byte reverse the data for us.
>> +                     */
>> +                    data_swab = 2;
>> +                }
>> +            } else {
>> +                info->endianness = ARM_ENDIANNESS_LE;
>> +            }
>> +        }
>> +
>> +        kernel_size = load_elf(info->kernel_filename, NULL, NULL, 
>> &elf_entry,
>> +                               &elf_low_addr, &elf_high_addr, big_endian,
>> +                               elf_machine, 1, data_swab);
>> +        if (kernel_size <= 0) {
>> +            exit(1);
>> +        }
>> +
>> +        if (have_dtb(info) && (elf_low_addr > info->loader_start ||
>> +                               elf_high_addr < info->loader_start)) {
>> +            /* If there is still some room left at the base of RAM, try and
>> +             * put the DTB there like we do for images loaded with -bios or
>> +             * -pflash.
>> +             *
>> +             * Pass elf_low_addr as address limit to load_dtb if it may be
>>               * pointing into RAM, otherwise pass '0' (no limit)
>>               */
>>              if (elf_low_addr < info->loader_start) {
>
> I think this change would be easier to read if we had a new
> (static) function in this file load_arm_elf() which did the
> "load header; figure out data_swab; load full elf file with
> corresponding swab value". Then the change to this function would
> just be a one-liner turning the load_elf() call into load_arm_elf().
>

Agree, new diff looks better.

"arm_load_elf" would be more consistent with surrounding code:

+static uint64_t arm_load_elf(struct arm_boot_info *info, uint64_t *pentry,
+                             uint64_t *lowaddr, uint64_t *highaddr,
+                             int elf_machine)

load_elf arguments that are constant for ARM (like translate_fn and
clear_lsb) are dropped from the new function prototype and just
hardcoded in arm_load_elf().

Regards,
Peter

> thanks
> -- PMM