Re: [Qemu-devel] [PATCH] hw/arm/boot: get AArch64 kernel Image load offset from Image file

[Peter Maydell]

On 14 May 2014 20:30, Rob Herring <address@hidden> wrote:
> From: Rob Herring <address@hidden>
>
> The AArch64 kermel Image format defines the load offset in its header.

"kernel"

> Retrieve the offset from the file instead of hardcoding it to 0x80000.

Cool. I knew this was bogus when I wrote it, so it's nice
to do this properly instead... (however it may not be that
simple, see below).

> Use of the hardcoded value will break when text_offset randomization is
> added to the kernel.

> Signed-off-by: Rob Herring <address@hidden>
> ---
>  hw/arm/boot.c | 32 ++++++++++++++++++++++++++++++--
>  1 file changed, 30 insertions(+), 2 deletions(-)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 3d1f4a2..4adce9e 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -24,7 +24,14 @@
>   */
>  #define KERNEL_ARGS_ADDR 0x100
>  #define KERNEL_LOAD_ADDR 0x00010000
> -#define KERNEL64_LOAD_ADDR 0x00080000
> +
> +typedef struct {
> +    uint32_t code[2];
> +    uint64_t text_offset;
> +    uint64_t res[5];
> +    uint32_t magic;
> +    uint32_t res5;
> +} aarch64_image_header_t;
>
>  typedef enum {
>      FIXUP_NONE = 0,   /* do nothing */
> @@ -441,6 +448,26 @@ static void do_cpu_reset(void *opaque)
>      }
>  }
>
> +static hwaddr aarch64_get_image_offset(const char *filename)
> +{
> +    int fd, size;
> +    aarch64_image_header_t hdr;
> +
> +    fd = open(filename, O_RDONLY | O_BINARY);
> +    if (fd < 0) {
> +        return 0;
> +    }
> +
> +    size = read(fd, &hdr, sizeof(hdr));
> +    close(fd);
> +
> +    if (size < sizeof(aarch64_image_header_t) ||
> +        le32_to_cpu(hdr.magic) != 0x644d5241) {
> +        return 0;
> +    }
> +    return le32_to_cpu(hdr.text_offset);

You mean le64_to_cpu, since text_offset is 64 bits.

I can't see anything in Documentation/arm64/booting.txt that
says the text_offset field is little endian. (The magic
number is explicitly so described.) In particular, current
kernels just say ".quad TEXT_OFFSET", so it's in the endian
order of the target kernel, which I think means it's in
practice not usable.

One suggestion I've seen for dealing with this is that
the semantics become such that bootloader code does this:
     if (hdr.res[0] == 0) {
        /* legacy broken kernels, always this address */
        return 0x80000;
     } else {
        return le64_to_cpu(hdr.text_offset);
     }
(ie hdr.res[0] becomes an indicator of whether the kernel
is consistent about what text_offset means; if it's zero
this is an old legacy kernel, which conveniently always uses
the same fixed address.)

We may need to wait for the kernel patches to appear
(and the decision about how bootloaders should interpret
the text_offset field to be resolved) before we can change
our bootloader code...

> +}
> +
>  void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>  {
>      CPUState *cs = CPU(cpu);
> @@ -463,7 +490,7 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info 
> *info)
>
>      if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
>          primary_loader = bootloader_aarch64;
> -        kernel_load_offset = KERNEL64_LOAD_ADDR;
> +        kernel_load_offset = aarch64_get_image_offset(info->kernel_filename);
>          elf_machine = EM_AARCH64;
>      } else {
>          primary_loader = bootloader;
> @@ -505,6 +532,7 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info 
> *info)
>      /* Assume that raw images are linux kernels, and ELF images are not.  */
>      kernel_size = load_elf(info->kernel_filename, NULL, NULL, &elf_entry,
>                             NULL, NULL, big_endian, elf_machine, 1);
> +
>      entry = elf_entry;
>      if (kernel_size < 0) {
>          kernel_size = load_uimage(info->kernel_filename, &entry, NULL,

Stray whitespace change.

I think we should also not attempt the load_image_targphys()
if kernel_load_offset is zero, since that means the thing we were
passed isn't an Image file at all.

thanks
-- PMM