[Qemu-devel] [PATCH v4] Support u-boot noload images for arm as used by, NetBSD/evbarm GENERIC kernel.

Nick Hudson posted 1 patch 5 years, 2 months ago
Test asan passed
Test checkpatch failed
Test docker-mingw@fedora passed
Test docker-quick@centos7 passed
Test docker-clang@ubuntu passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/d19529f5-841e-ea06-fe7d-86ccfd528817@gmail.com
There is a newer version of this series
hw/arm/boot.c          |  8 +++++---
hw/core/loader.c       | 19 ++++++++++++++++---
hw/core/uboot_image.h  |  1 +
hw/microblaze/boot.c   |  2 +-
hw/nios2/boot.c        |  2 +-
hw/ppc/e500.c          |  1 +
hw/ppc/ppc440_bamboo.c |  2 +-
hw/ppc/sam460ex.c      |  2 +-
include/hw/loader.h    |  7 ++++++-
9 files changed, 33 insertions(+), 11 deletions(-)
[Qemu-devel] [PATCH v4] Support u-boot noload images for arm as used by, NetBSD/evbarm GENERIC kernel.
Posted by Nick Hudson 5 years, 2 months ago

noload kernels are loaded with the u-boot image header and as a result
the header size needs adding to the entry point.  Fake up a hdr so the
kernel image is loaded at the right address and the entry point is
adjusted appropriately.

The default location for the uboot file is 32MiB above bottom of DRAM.
This matches the recommendation in Documentation/arm/Booting.

Clarify the load_uimage API to state the passing of a load address when an
image doesn't specify one, or when loading a ramdisk is expected.

Adjust callers of load_uimage, etc.
---
 hw/arm/boot.c          |  8 +++++---
 hw/core/loader.c       | 19 ++++++++++++++++---
 hw/core/uboot_image.h  |  1 +
 hw/microblaze/boot.c   |  2 +-
 hw/nios2/boot.c        |  2 +-
 hw/ppc/e500.c          |  1 +
 hw/ppc/ppc440_bamboo.c |  2 +-
 hw/ppc/sam460ex.c      |  2 +-
 include/hw/loader.h    |  7 ++++++-
 9 files changed, 33 insertions(+), 11 deletions(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 94fce12802..c7a67af7a9 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -30,8 +30,9 @@
  * Documentation/arm/Booting and Documentation/arm64/booting.txt
  * They have different preferred image load offsets from system RAM base.
  */
-#define KERNEL_ARGS_ADDR 0x100
-#define KERNEL_LOAD_ADDR 0x00010000
+#define KERNEL_ARGS_ADDR   0x100
+#define KERNEL_NOLOAD_ADDR 0x02000000
+#define KERNEL_LOAD_ADDR   0x00010000
 #define KERNEL64_LOAD_ADDR 0x00080000
 
 #define ARM64_TEXT_OFFSET_OFFSET    8
@@ -1082,7 +1083,8 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
     }
     entry = elf_entry;
     if (kernel_size < 0) {
-        kernel_size = load_uimage_as(info->kernel_filename, &entry, NULL,
+        uint64_t loadaddr = info->loader_start + KERNEL_NOLOAD_ADDR;
+        kernel_size = load_uimage_as(info->kernel_filename, &entry, &loadaddr,
                                      &is_linux, NULL, NULL, as);
     }
     if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64) && kernel_size < 0) {
diff --git a/hw/core/loader.c b/hw/core/loader.c
index fa41842280..c7182dfa64 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -613,13 +613,26 @@ static int load_uboot_image(const char *filename, hwaddr *ep, hwaddr *loadaddr,
         goto out;
 
     if (hdr->ih_type != image_type) {
-        fprintf(stderr, "Wrong image type %d, expected %d\n", hdr->ih_type,
-                image_type);
-        goto out;
+        if (!(image_type == IH_TYPE_KERNEL &&
+            hdr->ih_type == IH_TYPE_KERNEL_NOLOAD)) {
+            fprintf(stderr, "Wrong image type %d, expected %d\n", hdr->ih_type,
+                    image_type);
+            goto out;
+        }
     }
 
     /* TODO: Implement other image types.  */
     switch (hdr->ih_type) {
+    case IH_TYPE_KERNEL_NOLOAD:
+        if (!loadaddr || *loadaddr == LOAD_UIMAGE_LOADADDR_INVALID) {
+            fprintf(stderr, "this image format (kernel_noload) cannot be "
+                    "loaded on this machine type");
+            goto out;
+        }
+
+        hdr->ih_load = *loadaddr + sizeof(*hdr);
+        hdr->ih_ep += hdr->ih_load;
+        /* fall through */
     case IH_TYPE_KERNEL:
         address = hdr->ih_load;
         if (translate_fn) {
diff --git a/hw/core/uboot_image.h b/hw/core/uboot_image.h
index 34c11a70a6..608022de6e 100644
--- a/hw/core/uboot_image.h
+++ b/hw/core/uboot_image.h
@@ -124,6 +124,7 @@
 #define IH_TYPE_SCRIPT		6	/* Script file			*/
 #define IH_TYPE_FILESYSTEM	7	/* Filesystem Image (any type)	*/
 #define IH_TYPE_FLATDT		8	/* Binary Flat Device Tree Blob	*/
+#define IH_TYPE_KERNEL_NOLOAD  14	/* OS Kernel Image (noload)	*/
 
 /*
  * Compression Types
diff --git a/hw/microblaze/boot.c b/hw/microblaze/boot.c
index 35bfeda7aa..489ab839b7 100644
--- a/hw/microblaze/boot.c
+++ b/hw/microblaze/boot.c
@@ -156,7 +156,7 @@ void microblaze_load_kernel(MicroBlazeCPU *cpu, hwaddr ddr_base,
 
         /* If it wasn't an ELF image, try an u-boot image.  */
         if (kernel_size < 0) {
-            hwaddr uentry, loadaddr;
+            hwaddr uentry, loadaddr = LOAD_UIMAGE_LOADADDR_INVALID;
 
             kernel_size = load_uimage(kernel_filename, &uentry, &loadaddr, 0,
                                       NULL, NULL);
diff --git a/hw/nios2/boot.c b/hw/nios2/boot.c
index 4bb5b601d3..ed5cb28e94 100644
--- a/hw/nios2/boot.c
+++ b/hw/nios2/boot.c
@@ -161,7 +161,7 @@ void nios2_load_kernel(Nios2CPU *cpu, hwaddr ddr_base,
 
         /* If it wasn't an ELF image, try an u-boot image. */
         if (kernel_size < 0) {
-            hwaddr uentry, loadaddr;
+            hwaddr uentry, loadaddr = LOAD_UIMAGE_LOADADDR_INVALID;
 
             kernel_size = load_uimage(kernel_filename, &uentry, &loadaddr, 0,
                                       NULL, NULL);
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index b20fea0dfc..0581e9e3d4 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -995,6 +995,7 @@ void ppce500_init(MachineState *machine)
          * Hrm. No ELF image? Try a uImage, maybe someone is giving us an
          * ePAPR compliant kernel
          */
+        loadaddr = LOAD_UIMAGE_LOADADDR_INVALID;
         payload_size = load_uimage(filename, &bios_entry, &loadaddr, NULL,
                                    NULL, NULL);
         if (payload_size < 0) {
diff --git a/hw/ppc/ppc440_bamboo.c b/hw/ppc/ppc440_bamboo.c
index b8aa55d526..fc06191588 100644
--- a/hw/ppc/ppc440_bamboo.c
+++ b/hw/ppc/ppc440_bamboo.c
@@ -179,7 +179,7 @@ static void bamboo_init(MachineState *machine)
     CPUPPCState *env;
     uint64_t elf_entry;
     uint64_t elf_lowaddr;
-    hwaddr loadaddr = 0;
+    hwaddr loadaddr = LOAD_UIMAGE_LOADADDR_INVALID;
     target_long initrd_size = 0;
     DeviceState *dev;
     int success;
diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
index 4b051c0950..84ea592749 100644
--- a/hw/ppc/sam460ex.c
+++ b/hw/ppc/sam460ex.c
@@ -402,7 +402,7 @@ static void sam460ex_init(MachineState *machine)
     CPUPPCState *env;
     PPC4xxI2CState *i2c[2];
     hwaddr entry = UBOOT_ENTRY;
-    hwaddr loadaddr = 0;
+    hwaddr loadaddr = LOAD_UIMAGE_LOADADDR_INVALID;
     target_long initrd_size = 0;
     DeviceState *dev;
     SysBusDevice *sbdev;
diff --git a/include/hw/loader.h b/include/hw/loader.h
index 0a0ad808ea..84d6bb44b7 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -175,10 +175,15 @@ void load_elf_hdr(const char *filename, void *hdr, bool *is64, Error **errp);
 int load_aout(const char *filename, hwaddr addr, int max_sz,
               int bswap_needed, hwaddr target_page_size);
 
+#define LOAD_UIMAGE_LOADADDR_INVALID (-1)
+
 /** load_uimage_as:
  * @filename: Path of uimage file
  * @ep: Populated with program entry point. Ignored if NULL.
- * @loadaddr: Populated with the load address. Ignored if NULL.
+ * @loadaddr: load address if none specified in the image or when loading a ramdisk.
+ *            Populated with the the load address. Ignored if NULL or
+ *            LOAD_UIMAGE_LOADADDR_INVALID (images which do not specify a load address
+ *            will not be loadable).
  * @is_linux: Is set to true if the image loaded is Linux. Ignored if NULL.
  * @translate_fn: optional function to translate load addresses
  * @translate_opaque: opaque data passed to @translate_fn
-- 
2.17.1


Re: [Qemu-devel] [PATCH v4] Support u-boot noload images for arm as used by, NetBSD/evbarm GENERIC kernel.
Posted by no-reply@patchew.org 5 years, 2 months ago
Patchew URL: https://patchew.org/QEMU/d19529f5-841e-ea06-fe7d-86ccfd528817@gmail.com/



Hi,

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

Subject: [Qemu-devel] [PATCH v4] Support u-boot noload images for arm as used by, NetBSD/evbarm GENERIC kernel.
Type: series
Message-id: d19529f5-841e-ea06-fe7d-86ccfd528817@gmail.com

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

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

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

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
Switched to a new branch 'test'
e2a15da Support u-boot noload images for arm as used by, NetBSD/evbarm GENERIC kernel.

=== OUTPUT BEGIN ===
Checking PATCH 1/1: Support u-boot noload images for arm as used by, NetBSD/evbarm GENERIC kernel....
ERROR: code indent should never use tabs
#89: FILE: hw/core/uboot_image.h:127:
+#define IH_TYPE_KERNEL_NOLOAD  14^I/* OS Kernel Image (noload)^I*/$

WARNING: line over 80 characters
#171: FILE: include/hw/loader.h:183:
+ * @loadaddr: load address if none specified in the image or when loading a ramdisk.

WARNING: line over 80 characters
#173: FILE: include/hw/loader.h:185:
+ *            LOAD_UIMAGE_LOADADDR_INVALID (images which do not specify a load address

ERROR: Missing Signed-off-by: line(s)

total: 2 errors, 2 warnings, 111 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


The full log is available at
http://patchew.org/logs/d19529f5-841e-ea06-fe7d-86ccfd528817@gmail.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
Re: [Qemu-devel] [PATCH v4] Support u-boot noload images for arm as used by, NetBSD/evbarm GENERIC kernel.
Posted by Nick Hudson 5 years, 2 months ago

On 06/01/2019 22:56, no-reply@patchew.org wrote:
> Patchew URL: https://patchew.org/QEMU/d19529f5-841e-ea06-fe7d-86ccfd528817@gmail.com/
> 
> 
> 
> Hi,
> 
> This series seems to have some coding style problems. See output below for
> more information:

The files being touched have lots of coding style problems already.  I'm
avoiding i) fixing these in the same diff for clarity and ii) in the
case of IH_TYPE_KERNEL_NOLOAD, following existing formatting.

I'll send v5 with the loadaddr api documentation formatting fix.

Nick

Re: [Qemu-devel] [PATCH v4] Support u-boot noload images for arm as used by, NetBSD/evbarm GENERIC kernel.
Posted by Peter Maydell 5 years, 2 months ago
On Mon, 7 Jan 2019 at 07:54, Nick Hudson <nicholas.a.hudson@gmail.com> wrote:
> On 06/01/2019 22:56, no-reply@patchew.org wrote:
> > This series seems to have some coding style problems. See output below for
> > more information:
>
> The files being touched have lots of coding style problems already.  I'm
> avoiding i) fixing these in the same diff for clarity and ii) in the
> case of IH_TYPE_KERNEL_NOLOAD, following existing formatting.

Our usual rule of thumb is "keep checkpatch happy", ie don't introduce
new style issues, even if the code being modified is a bit messy.
In this case it's no big deal -- if you've already sent v5 I can
just wrap the long lines when I apply the patch to my tree.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4] Support u-boot noload images for arm as used by, NetBSD/evbarm GENERIC kernel.
Posted by BALATON Zoltan 5 years, 2 months ago
On Sun, 6 Jan 2019, Nick Hudson wrote:
> noload kernels are loaded with the u-boot image header and as a result
> the header size needs adding to the entry point.  Fake up a hdr so the
> kernel image is loaded at the right address and the entry point is
> adjusted appropriately.
>
> The default location for the uboot file is 32MiB above bottom of DRAM.
> This matches the recommendation in Documentation/arm/Booting.
>
> Clarify the load_uimage API to state the passing of a load address when an
> image doesn't specify one, or when loading a ramdisk is expected.
>
> Adjust callers of load_uimage, etc.
> ---
> hw/arm/boot.c          |  8 +++++---
> hw/core/loader.c       | 19 ++++++++++++++++---
> hw/core/uboot_image.h  |  1 +
> hw/microblaze/boot.c   |  2 +-
> hw/nios2/boot.c        |  2 +-
> hw/ppc/e500.c          |  1 +
> hw/ppc/ppc440_bamboo.c |  2 +-
> hw/ppc/sam460ex.c      |  2 +-
> include/hw/loader.h    |  7 ++++++-
> 9 files changed, 33 insertions(+), 11 deletions(-)
>
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index 94fce12802..c7a67af7a9 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -30,8 +30,9 @@
>  * Documentation/arm/Booting and Documentation/arm64/booting.txt
>  * They have different preferred image load offsets from system RAM base.
>  */
> -#define KERNEL_ARGS_ADDR 0x100
> -#define KERNEL_LOAD_ADDR 0x00010000
> +#define KERNEL_ARGS_ADDR   0x100
> +#define KERNEL_NOLOAD_ADDR 0x02000000
> +#define KERNEL_LOAD_ADDR   0x00010000
> #define KERNEL64_LOAD_ADDR 0x00080000
>
> #define ARM64_TEXT_OFFSET_OFFSET    8
> @@ -1082,7 +1083,8 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
>     }
>     entry = elf_entry;
>     if (kernel_size < 0) {
> -        kernel_size = load_uimage_as(info->kernel_filename, &entry, NULL,
> +        uint64_t loadaddr = info->loader_start + KERNEL_NOLOAD_ADDR;
> +        kernel_size = load_uimage_as(info->kernel_filename, &entry, &loadaddr,
>                                      &is_linux, NULL, NULL, as);
>     }
>     if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64) && kernel_size < 0) {
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index fa41842280..c7182dfa64 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -613,13 +613,26 @@ static int load_uboot_image(const char *filename, hwaddr *ep, hwaddr *loadaddr,
>         goto out;
>
>     if (hdr->ih_type != image_type) {
> -        fprintf(stderr, "Wrong image type %d, expected %d\n", hdr->ih_type,
> -                image_type);
> -        goto out;
> +        if (!(image_type == IH_TYPE_KERNEL &&
> +            hdr->ih_type == IH_TYPE_KERNEL_NOLOAD)) {

So this is now effectively

(hdr->ih_type != image_type && !(image_type == IH_TYPE_KERNEL && hdr->ih_type == IH_TYPE_KERNEL_NOLOAD))

Maybe you don't need two if-s just one with this condition?

> +            fprintf(stderr, "Wrong image type %d, expected %d\n", hdr->ih_type,
> +                    image_type);
> +            goto out;
> +        }
>     }
>
>     /* TODO: Implement other image types.  */
>     switch (hdr->ih_type) {
> +    case IH_TYPE_KERNEL_NOLOAD:
> +        if (!loadaddr || *loadaddr == LOAD_UIMAGE_LOADADDR_INVALID) {
> +            fprintf(stderr, "this image format (kernel_noload) cannot be "
> +                    "loaded on this machine type");
> +            goto out;
> +        }
> +
> +        hdr->ih_load = *loadaddr + sizeof(*hdr);
> +        hdr->ih_ep += hdr->ih_load;
> +        /* fall through */
>     case IH_TYPE_KERNEL:
>         address = hdr->ih_load;
>         if (translate_fn) {
> diff --git a/hw/core/uboot_image.h b/hw/core/uboot_image.h
> index 34c11a70a6..608022de6e 100644
> --- a/hw/core/uboot_image.h
> +++ b/hw/core/uboot_image.h
> @@ -124,6 +124,7 @@
> #define IH_TYPE_SCRIPT		6	/* Script file			*/
> #define IH_TYPE_FILESYSTEM	7	/* Filesystem Image (any type)	*/
> #define IH_TYPE_FLATDT		8	/* Binary Flat Device Tree Blob	*/
> +#define IH_TYPE_KERNEL_NOLOAD  14	/* OS Kernel Image (noload)	*/

Do we want to make this an enum like in recent U-Boot? (Not suggesting we 
should just asking if you're touching this anyway.)

> /*
>  * Compression Types
> diff --git a/hw/microblaze/boot.c b/hw/microblaze/boot.c
> index 35bfeda7aa..489ab839b7 100644
> --- a/hw/microblaze/boot.c
> +++ b/hw/microblaze/boot.c
> @@ -156,7 +156,7 @@ void microblaze_load_kernel(MicroBlazeCPU *cpu, hwaddr ddr_base,
>
>         /* If it wasn't an ELF image, try an u-boot image.  */
>         if (kernel_size < 0) {
> -            hwaddr uentry, loadaddr;
> +            hwaddr uentry, loadaddr = LOAD_UIMAGE_LOADADDR_INVALID;
>
>             kernel_size = load_uimage(kernel_filename, &uentry, &loadaddr, 0,
>                                       NULL, NULL);
> diff --git a/hw/nios2/boot.c b/hw/nios2/boot.c
> index 4bb5b601d3..ed5cb28e94 100644
> --- a/hw/nios2/boot.c
> +++ b/hw/nios2/boot.c
> @@ -161,7 +161,7 @@ void nios2_load_kernel(Nios2CPU *cpu, hwaddr ddr_base,
>
>         /* If it wasn't an ELF image, try an u-boot image. */
>         if (kernel_size < 0) {
> -            hwaddr uentry, loadaddr;
> +            hwaddr uentry, loadaddr = LOAD_UIMAGE_LOADADDR_INVALID;
>
>             kernel_size = load_uimage(kernel_filename, &uentry, &loadaddr, 0,
>                                       NULL, NULL);
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index b20fea0dfc..0581e9e3d4 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -995,6 +995,7 @@ void ppce500_init(MachineState *machine)
>          * Hrm. No ELF image? Try a uImage, maybe someone is giving us an
>          * ePAPR compliant kernel
>          */
> +        loadaddr = LOAD_UIMAGE_LOADADDR_INVALID;
>         payload_size = load_uimage(filename, &bios_entry, &loadaddr, NULL,
>                                    NULL, NULL);
>         if (payload_size < 0) {
> diff --git a/hw/ppc/ppc440_bamboo.c b/hw/ppc/ppc440_bamboo.c
> index b8aa55d526..fc06191588 100644
> --- a/hw/ppc/ppc440_bamboo.c
> +++ b/hw/ppc/ppc440_bamboo.c
> @@ -179,7 +179,7 @@ static void bamboo_init(MachineState *machine)
>     CPUPPCState *env;
>     uint64_t elf_entry;
>     uint64_t elf_lowaddr;
> -    hwaddr loadaddr = 0;
> +    hwaddr loadaddr = LOAD_UIMAGE_LOADADDR_INVALID;
>     target_long initrd_size = 0;
>     DeviceState *dev;
>     int success;
> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
> index 4b051c0950..84ea592749 100644
> --- a/hw/ppc/sam460ex.c
> +++ b/hw/ppc/sam460ex.c
> @@ -402,7 +402,7 @@ static void sam460ex_init(MachineState *machine)
>     CPUPPCState *env;
>     PPC4xxI2CState *i2c[2];
>     hwaddr entry = UBOOT_ENTRY;
> -    hwaddr loadaddr = 0;
> +    hwaddr loadaddr = LOAD_UIMAGE_LOADADDR_INVALID;
>     target_long initrd_size = 0;
>     DeviceState *dev;
>     SysBusDevice *sbdev;
> diff --git a/include/hw/loader.h b/include/hw/loader.h
> index 0a0ad808ea..84d6bb44b7 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -175,10 +175,15 @@ void load_elf_hdr(const char *filename, void *hdr, bool *is64, Error **errp);
> int load_aout(const char *filename, hwaddr addr, int max_sz,
>               int bswap_needed, hwaddr target_page_size);
>
> +#define LOAD_UIMAGE_LOADADDR_INVALID (-1)

hwadrr is uint64_t, an unsigned type so maybe -1 is not the best value for 
invalid address. Doesn't this give you a warning?

> +
> /** load_uimage_as:
>  * @filename: Path of uimage file
>  * @ep: Populated with program entry point. Ignored if NULL.
> - * @loadaddr: Populated with the load address. Ignored if NULL.
> + * @loadaddr: load address if none specified in the image or when loading a ramdisk.
> + *            Populated with the the load address. Ignored if NULL or

You have "the the" typo here ---^^^^^^^

> + *            LOAD_UIMAGE_LOADADDR_INVALID (images which do not specify a load address
> + *            will not be loadable).
>  * @is_linux: Is set to true if the image loaded is Linux. Ignored if NULL.
>  * @translate_fn: optional function to translate load addresses
>  * @translate_opaque: opaque data passed to @translate_fn
>

Re: [Qemu-devel] [PATCH v4] Support u-boot noload images for arm as used by, NetBSD/evbarm GENERIC kernel.
Posted by Nick Hudson 5 years, 2 months ago
Thanks for the comments.

On 07/01/2019 00:33, BALATON Zoltan wrote:
> On Sun, 6 Jan 2019, Nick Hudson wrote:
>> noload kernels are loaded with the u-boot image header and as a result
>> the header size needs adding to the entry point.  Fake up a hdr so the
>> kernel image is loaded at the right address and the entry point is
>> adjusted appropriately.
>>
>> The default location for the uboot file is 32MiB above bottom of DRAM.
>> This matches the recommendation in Documentation/arm/Booting.
>>
>> Clarify the load_uimage API to state the passing of a load address 
>> when an
>> image doesn't specify one, or when loading a ramdisk is expected.
>>
>> Adjust callers of load_uimage, etc.
>> ---
>> hw/arm/boot.c          |  8 +++++---
>> hw/core/loader.c       | 19 ++++++++++++++++---
>> hw/core/uboot_image.h  |  1 +
>> hw/microblaze/boot.c   |  2 +-
>> hw/nios2/boot.c        |  2 +-
>> hw/ppc/e500.c          |  1 +
>> hw/ppc/ppc440_bamboo.c |  2 +-
>> hw/ppc/sam460ex.c      |  2 +-
>> include/hw/loader.h    |  7 ++++++-
>> 9 files changed, 33 insertions(+), 11 deletions(-)
>>
>> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
>> index 94fce12802..c7a67af7a9 100644
>> --- a/hw/arm/boot.c
>> +++ b/hw/arm/boot.c
>> @@ -30,8 +30,9 @@
>>  * Documentation/arm/Booting and Documentation/arm64/booting.txt
>>  * They have different preferred image load offsets from system RAM base.
>>  */
>> -#define KERNEL_ARGS_ADDR 0x100
>> -#define KERNEL_LOAD_ADDR 0x00010000
>> +#define KERNEL_ARGS_ADDR   0x100
>> +#define KERNEL_NOLOAD_ADDR 0x02000000
>> +#define KERNEL_LOAD_ADDR   0x00010000
>> #define KERNEL64_LOAD_ADDR 0x00080000
>>
>> #define ARM64_TEXT_OFFSET_OFFSET    8
>> @@ -1082,7 +1083,8 @@ void arm_load_kernel(ARMCPU *cpu, struct 
>> arm_boot_info *info)
>>     }
>>     entry = elf_entry;
>>     if (kernel_size < 0) {
>> -        kernel_size = load_uimage_as(info->kernel_filename, &entry, 
>> NULL,
>> +        uint64_t loadaddr = info->loader_start + KERNEL_NOLOAD_ADDR;
>> +        kernel_size = load_uimage_as(info->kernel_filename, &entry, 
>> &loadaddr,
>>                                      &is_linux, NULL, NULL, as);
>>     }
>>     if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64) && kernel_size < 0) {
>> diff --git a/hw/core/loader.c b/hw/core/loader.c
>> index fa41842280..c7182dfa64 100644
>> --- a/hw/core/loader.c
>> +++ b/hw/core/loader.c
>> @@ -613,13 +613,26 @@ static int load_uboot_image(const char 
>> *filename, hwaddr *ep, hwaddr *loadaddr,
>>         goto out;
>>
>>     if (hdr->ih_type != image_type) {
>> -        fprintf(stderr, "Wrong image type %d, expected %d\n", 
>> hdr->ih_type,
>> -                image_type);
>> -        goto out;
>> +        if (!(image_type == IH_TYPE_KERNEL &&
>> +            hdr->ih_type == IH_TYPE_KERNEL_NOLOAD)) {
> 
> So this is now effectively
> 
> (hdr->ih_type != image_type && !(image_type == IH_TYPE_KERNEL && 
> hdr->ih_type == IH_TYPE_KERNEL_NOLOAD))
> 
> Maybe you don't need two if-s just one with this condition?

I didn't see anything in the style guide that stipulated I couldn't keep
the two ifs.  I've left it as is.

> 
>> +            fprintf(stderr, "Wrong image type %d, expected %d\n", 
>> hdr->ih_type,
>> +                    image_type);
>> +            goto out;
>> +        }
>>     }
>>
>>     /* TODO: Implement other image types.  */
>>     switch (hdr->ih_type) {
>> +    case IH_TYPE_KERNEL_NOLOAD:
>> +        if (!loadaddr || *loadaddr == LOAD_UIMAGE_LOADADDR_INVALID) {
>> +            fprintf(stderr, "this image format (kernel_noload) cannot 
>> be "
>> +                    "loaded on this machine type");
>> +            goto out;
>> +        }
>> +
>> +        hdr->ih_load = *loadaddr + sizeof(*hdr);
>> +        hdr->ih_ep += hdr->ih_load;
>> +        /* fall through */
>>     case IH_TYPE_KERNEL:
>>         address = hdr->ih_load;
>>         if (translate_fn) {
>> diff --git a/hw/core/uboot_image.h b/hw/core/uboot_image.h
>> index 34c11a70a6..608022de6e 100644
>> --- a/hw/core/uboot_image.h
>> +++ b/hw/core/uboot_image.h
>> @@ -124,6 +124,7 @@
>> #define IH_TYPE_SCRIPT        6    /* Script file            */
>> #define IH_TYPE_FILESYSTEM    7    /* Filesystem Image (any type)    */
>> #define IH_TYPE_FLATDT        8    /* Binary Flat Device Tree Blob    */
>> +#define IH_TYPE_KERNEL_NOLOAD  14    /* OS Kernel Image (noload)    */
> 
> Do we want to make this an enum like in recent U-Boot? (Not suggesting 
> we should just asking if you're touching this anyway.)

I guess this is for someone else to answer. I wanted to provide a 
minimal diff.


> 
>> /*
>>  * Compression Types
>> diff --git a/hw/microblaze/boot.c b/hw/microblaze/boot.c
>> index 35bfeda7aa..489ab839b7 100644
>> --- a/hw/microblaze/boot.c
>> +++ b/hw/microblaze/boot.c
>> @@ -156,7 +156,7 @@ void microblaze_load_kernel(MicroBlazeCPU *cpu, 
>> hwaddr ddr_base,
>>
>>         /* If it wasn't an ELF image, try an u-boot image.  */
>>         if (kernel_size < 0) {
>> -            hwaddr uentry, loadaddr;
>> +            hwaddr uentry, loadaddr = LOAD_UIMAGE_LOADADDR_INVALID;
>>
>>             kernel_size = load_uimage(kernel_filename, &uentry, 
>> &loadaddr, 0,
>>                                       NULL, NULL);
>> diff --git a/hw/nios2/boot.c b/hw/nios2/boot.c
>> index 4bb5b601d3..ed5cb28e94 100644
>> --- a/hw/nios2/boot.c
>> +++ b/hw/nios2/boot.c
>> @@ -161,7 +161,7 @@ void nios2_load_kernel(Nios2CPU *cpu, hwaddr 
>> ddr_base,
>>
>>         /* If it wasn't an ELF image, try an u-boot image. */
>>         if (kernel_size < 0) {
>> -            hwaddr uentry, loadaddr;
>> +            hwaddr uentry, loadaddr = LOAD_UIMAGE_LOADADDR_INVALID;
>>
>>             kernel_size = load_uimage(kernel_filename, &uentry, 
>> &loadaddr, 0,
>>                                       NULL, NULL);
>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
>> index b20fea0dfc..0581e9e3d4 100644
>> --- a/hw/ppc/e500.c
>> +++ b/hw/ppc/e500.c
>> @@ -995,6 +995,7 @@ void ppce500_init(MachineState *machine)
>>          * Hrm. No ELF image? Try a uImage, maybe someone is giving us an
>>          * ePAPR compliant kernel
>>          */
>> +        loadaddr = LOAD_UIMAGE_LOADADDR_INVALID;
>>         payload_size = load_uimage(filename, &bios_entry, &loadaddr, 
>> NULL,
>>                                    NULL, NULL);
>>         if (payload_size < 0) {
>> diff --git a/hw/ppc/ppc440_bamboo.c b/hw/ppc/ppc440_bamboo.c
>> index b8aa55d526..fc06191588 100644
>> --- a/hw/ppc/ppc440_bamboo.c
>> +++ b/hw/ppc/ppc440_bamboo.c
>> @@ -179,7 +179,7 @@ static void bamboo_init(MachineState *machine)
>>     CPUPPCState *env;
>>     uint64_t elf_entry;
>>     uint64_t elf_lowaddr;
>> -    hwaddr loadaddr = 0;
>> +    hwaddr loadaddr = LOAD_UIMAGE_LOADADDR_INVALID;
>>     target_long initrd_size = 0;
>>     DeviceState *dev;
>>     int success;
>> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
>> index 4b051c0950..84ea592749 100644
>> --- a/hw/ppc/sam460ex.c
>> +++ b/hw/ppc/sam460ex.c
>> @@ -402,7 +402,7 @@ static void sam460ex_init(MachineState *machine)
>>     CPUPPCState *env;
>>     PPC4xxI2CState *i2c[2];
>>     hwaddr entry = UBOOT_ENTRY;
>> -    hwaddr loadaddr = 0;
>> +    hwaddr loadaddr = LOAD_UIMAGE_LOADADDR_INVALID;
>>     target_long initrd_size = 0;
>>     DeviceState *dev;
>>     SysBusDevice *sbdev;
>> diff --git a/include/hw/loader.h b/include/hw/loader.h
>> index 0a0ad808ea..84d6bb44b7 100644
>> --- a/include/hw/loader.h
>> +++ b/include/hw/loader.h
>> @@ -175,10 +175,15 @@ void load_elf_hdr(const char *filename, void 
>> *hdr, bool *is64, Error **errp);
>> int load_aout(const char *filename, hwaddr addr, int max_sz,
>>               int bswap_needed, hwaddr target_page_size);
>>
>> +#define LOAD_UIMAGE_LOADADDR_INVALID (-1)
> 
> hwadrr is uint64_t, an unsigned type so maybe -1 is not the best value 
> for invalid address. Doesn't this give you a warning?

I don't see a problem here and no warning is issued in my build.

> 
>> +
>> /** load_uimage_as:
>>  * @filename: Path of uimage file
>>  * @ep: Populated with program entry point. Ignored if NULL.
>> - * @loadaddr: Populated with the load address. Ignored if NULL.
>> + * @loadaddr: load address if none specified in the image or when 
>> loading a ramdisk.
>> + *            Populated with the the load address. Ignored if NULL or
> 
> You have "the the" typo here ---^^^^^^^

fixed in v5.

> 
>> + *            LOAD_UIMAGE_LOADADDR_INVALID (images which do not 
>> specify a load address
>> + *            will not be loadable).
>>  * @is_linux: Is set to true if the image loaded is Linux. Ignored if 
>> NULL.
>>  * @translate_fn: optional function to translate load addresses
>>  * @translate_opaque: opaque data passed to @translate_fn
>>


Thanks,
Nick