| Message ID | 1489079383-11162-1-git-send-email-ard.biesheuvel@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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 --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;
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(+)