| Message ID | 1400095837-26128-1-git-send-email-robherring2@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 14 May 2014 20:30, Rob Herring <robherring2@gmail.com> wrote: > From: Rob Herring <rob.herring@linaro.org> > > 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 <rob.herring@linaro.org> > --- > 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
On 15 May 2014 19:54, Peter Maydell <peter.maydell@linaro.org> wrote: > 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... Mark Rutland has now posted those kernel patches: http://archive.arm.linux.org.uk/lurker/message/20140516.095035.4bf1f2bd.en.html -- PMM
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); +} + 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,