diff mbox

hw/arm/boot: take Linux/arm64 TEXT_OFFSET header field into account

Message ID 1489079383-11162-1-git-send-email-ard.biesheuvel@linaro.org
State New
Headers show

Commit Message

Ard Biesheuvel March 9, 2017, 5:09 p.m. UTC 
The arm64 boot protocol stipulates that the kernel must be loaded
TEXT_OFFSET bytes beyond a 2 MB aligned base address, where TEXT_OFFSET
could be any 4 KB multiple between 0 and 2 MB, and whose value can be
found in the header of the Image file.

So after attempts to load the kernel image as an ELF file or as a
U-Boot image (both of which have their own way of specifying the load
offset) have failed, try to determine the TEXT_OFFSET from the image
before proceeding with loading it as a gzipped or raw image.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---
 hw/arm/boot.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

-- 
2.7.4

Comments

no-reply@patchew.org March 9, 2017, 5:13 p.m. UTC | #1 
Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Subject: [Qemu-devel] [PATCH] hw/arm/boot: take Linux/arm64 TEXT_OFFSET header field into account
Message-id: 1489079383-11162-1-git-send-email-ard.biesheuvel@linaro.org

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/1489079383-11162-1-git-send-email-ard.biesheuvel@linaro.org -> patchew/1489079383-11162-1-git-send-email-ard.biesheuvel@linaro.org
 - [tag update]      patchew/20170309132216.23482-1-dgilbert@redhat.com -> patchew/20170309132216.23482-1-dgilbert@redhat.com
Switched to a new branch 'test'
55ab14d hw/arm/boot: take Linux/arm64 TEXT_OFFSET header field into account

=== OUTPUT BEGIN ===
Checking PATCH 1/1: hw/arm/boot: take Linux/arm64 TEXT_OFFSET header field into account...
ERROR: braces {} are necessary for all arms of this statement
#55: FILE: hw/arm/boot.c:791:
+        if (fd < 0)
[...]

ERROR: braces {} are necessary for all arms of this statement
#62: FILE: hw/arm/boot.c:798:
+        if (bytes < ARM64_IMAGE_HEADER_SIZE)
[...]

ERROR: braces {} are necessary for all arms of this statement
#67: FILE: hw/arm/boot.c:803:
+    if (memcmp(buffer + ARM64_MAGIC_OFFSET, "ARM\x64", 4) != 0)
[...]

ERROR: braces {} are necessary for all arms of this statement
#75: FILE: hw/arm/boot.c:811:
+    if (headerval == 0)
[...]

total: 4 errors, 0 warnings, 74 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org
Peter Maydell March 13, 2017, 11:22 a.m. UTC | #2 
On 9 March 2017 at 18:09, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> The arm64 boot protocol stipulates that the kernel must be loaded

> TEXT_OFFSET bytes beyond a 2 MB aligned base address, where TEXT_OFFSET

> could be any 4 KB multiple between 0 and 2 MB, and whose value can be

> found in the header of the Image file.

>

> So after attempts to load the kernel image as an ELF file or as a

> U-Boot image (both of which have their own way of specifying the load

> offset) have failed, try to determine the TEXT_OFFSET from the image

> before proceeding with loading it as a gzipped or raw image.

>

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---

>  hw/arm/boot.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++

>  1 file changed, 56 insertions(+)

>

> diff --git a/hw/arm/boot.c b/hw/arm/boot.c

> index ff621e4b6a4b..0824f74c697f 100644

> --- a/hw/arm/boot.c

> +++ b/hw/arm/boot.c

> @@ -31,6 +31,11 @@

>  #define KERNEL_LOAD_ADDR 0x00010000

>  #define KERNEL64_LOAD_ADDR 0x00080000

>

> +#define ARM64_IMAGE_HEADER_SIZE     64

> +#define ARM64_TEXT_OFFSET_OFFSET    8

> +#define ARM64_IMAGE_SIZE_OFFSET     16

> +#define ARM64_MAGIC_OFFSET          56

> +

>  typedef enum {

>      FIXUP_NONE = 0,     /* do nothing */

>      FIXUP_TERMINATOR,   /* end of insns */

> @@ -768,6 +773,51 @@ static uint64_t arm_load_elf(struct arm_boot_info *info, uint64_t *pentry,

>      return ret;

>  }

>

> +static void aarch64_get_text_offset(struct arm_boot_info *info,

> +                                    hwaddr *text_offset)

> +{


Hmm, this isn't quite what I had in mind when we talked about this.
I think what we want is a function like
static uint64_t load_aarch64_image(const char *filename, hwaddr *entry,
                                   other stuff you need)
which loads the image if it's the right format and writes
the entry point into *entry, and returns a negative value
if the image isn't the right format.

ie whose API is basically parallel to the load_uimage(),
load_image_gzipped(), etc functions. You can make it handle
both gzipped and non-gzipped images (and then drop the code
we currently have to call load_image_gzipped()).

PS: you'll want to use g_file_get_contents() for the load
in the non-compressed image case, and as the patchew
robot points out our coding style preferences mandate braces.

thanks
-- PMM
Ard Biesheuvel March 13, 2017, 11:25 a.m. UTC | #3 
On 13 March 2017 at 11:22, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 9 March 2017 at 18:09, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

>> The arm64 boot protocol stipulates that the kernel must be loaded

>> TEXT_OFFSET bytes beyond a 2 MB aligned base address, where TEXT_OFFSET

>> could be any 4 KB multiple between 0 and 2 MB, and whose value can be

>> found in the header of the Image file.

>>

>> So after attempts to load the kernel image as an ELF file or as a

>> U-Boot image (both of which have their own way of specifying the load

>> offset) have failed, try to determine the TEXT_OFFSET from the image

>> before proceeding with loading it as a gzipped or raw image.

>>

>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>> ---

>>  hw/arm/boot.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++

>>  1 file changed, 56 insertions(+)

>>

>> diff --git a/hw/arm/boot.c b/hw/arm/boot.c

>> index ff621e4b6a4b..0824f74c697f 100644

>> --- a/hw/arm/boot.c

>> +++ b/hw/arm/boot.c

>> @@ -31,6 +31,11 @@

>>  #define KERNEL_LOAD_ADDR 0x00010000

>>  #define KERNEL64_LOAD_ADDR 0x00080000

>>

>> +#define ARM64_IMAGE_HEADER_SIZE     64

>> +#define ARM64_TEXT_OFFSET_OFFSET    8

>> +#define ARM64_IMAGE_SIZE_OFFSET     16

>> +#define ARM64_MAGIC_OFFSET          56

>> +

>>  typedef enum {

>>      FIXUP_NONE = 0,     /* do nothing */

>>      FIXUP_TERMINATOR,   /* end of insns */

>> @@ -768,6 +773,51 @@ static uint64_t arm_load_elf(struct arm_boot_info *info, uint64_t *pentry,

>>      return ret;

>>  }

>>

>> +static void aarch64_get_text_offset(struct arm_boot_info *info,

>> +                                    hwaddr *text_offset)

>> +{

>

> Hmm, this isn't quite what I had in mind when we talked about this.

> I think what we want is a function like

> static uint64_t load_aarch64_image(const char *filename, hwaddr *entry,

>                                    other stuff you need)

> which loads the image if it's the right format and writes

> the entry point into *entry, and returns a negative value

> if the image isn't the right format.

>

> ie whose API is basically parallel to the load_uimage(),

> load_image_gzipped(), etc functions. You can make it handle

> both gzipped and non-gzipped images (and then drop the code

> we currently have to call load_image_gzipped()).

>


OK, that sounds reasonable.

> PS: you'll want to use g_file_get_contents() for the load

> in the non-compressed image case, and as the patchew

> robot points out our coding style preferences mandate braces.

>


Yes. TBH I contribute to so many different projects, it's hard to keep
track of such things, and so I simply looked elsewhere in the file for
examples.
diff mbox

Patch

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index ff621e4b6a4b..0824f74c697f 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -31,6 +31,11 @@ 
 #define KERNEL_LOAD_ADDR 0x00010000
 #define KERNEL64_LOAD_ADDR 0x00080000
 
+#define ARM64_IMAGE_HEADER_SIZE     64
+#define ARM64_TEXT_OFFSET_OFFSET    8
+#define ARM64_IMAGE_SIZE_OFFSET     16
+#define ARM64_MAGIC_OFFSET          56
+
 typedef enum {
     FIXUP_NONE = 0,     /* do nothing */
     FIXUP_TERMINATOR,   /* end of insns */
@@ -768,6 +773,51 @@  static uint64_t arm_load_elf(struct arm_boot_info *info, uint64_t *pentry,
     return ret;
 }
 
+static void aarch64_get_text_offset(struct arm_boot_info *info,
+                                    hwaddr *text_offset)
+{
+    uint8_t *buffer;
+    uint64_t headerval;
+    int size;
+
+    size = load_image_gzipped_buffer(info->kernel_filename,
+                                     LOAD_IMAGE_MAX_GUNZIP_BYTES,
+                                     &buffer);
+
+    if (size < 0) {
+        int fd, bytes;
+
+        fd = open(info->kernel_filename, O_RDONLY | O_BINARY);
+        if (fd < 0)
+            return;
+
+        buffer = g_malloc(ARM64_IMAGE_HEADER_SIZE);
+        bytes = read(fd, buffer, ARM64_IMAGE_HEADER_SIZE);
+        close(fd);
+
+        if (bytes < ARM64_IMAGE_HEADER_SIZE)
+            goto free_buffer;
+    }
+
+    /* check the arm64 magic header value */
+    if (memcmp(buffer + ARM64_MAGIC_OFFSET, "ARM\x64", 4) != 0)
+        goto free_buffer;
+
+    /* The arm64 Image header has text_offset and image_size fields at 8 and
+     * 16 bytes into the Image header, respectively. The text_offset field is
+     * only valid if the image_size is non-zero.
+     */
+    memcpy(&headerval, buffer + ARM64_IMAGE_SIZE_OFFSET, sizeof(headerval));
+    if (headerval == 0)
+        goto free_buffer;
+
+    memcpy(&headerval, buffer + ARM64_TEXT_OFFSET_OFFSET, sizeof(headerval));
+    *text_offset = le64_to_cpu(headerval);
+
+free_buffer:
+    g_free(buffer);
+}
+
 static void arm_load_kernel_notify(Notifier *notifier, void *data)
 {
     CPUState *cs;
@@ -900,6 +950,12 @@  static void arm_load_kernel_notify(Notifier *notifier, void *data)
         kernel_size = load_uimage(info->kernel_filename, &entry, NULL,
                                   &is_linux, NULL, NULL);
     }
+
+    /* A bare Linux/arm64 kernel carries the load offset in the Image header */
+    if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64) && kernel_size < 0) {
+        aarch64_get_text_offset(info, &kernel_load_offset);
+    }
+
     /* On aarch64, it's the bootloader's job to uncompress the kernel. */
     if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64) && kernel_size < 0) {
         entry = info->loader_start + kernel_load_offset;