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 bootloader fits in the space that the uboot header would have occupied.

Update the load_uimage API to allow passing of load address when an image
doesn't specify one.

Signed-off-by: Nick Hudson <skrll@netbsd.org>
---
 hw/arm/boot.c         |  8 +++++---
 hw/core/loader.c      | 15 ++++++++++++---
 hw/core/uboot_image.h |  1 +
 include/hw/loader.h   |  3 ++-
 4 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 586baa9b64..450267566a 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 0x00000000
+#define KERNEL_LOAD_ADDR   0x00010000
 #define KERNEL64_LOAD_ADDR 0x00080000

 #define ARM64_TEXT_OFFSET_OFFSET    8
@@ -1078,7 +1079,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 aa0b3fc867..5537e88817 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -638,13 +638,22 @@ 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 == NULL)
+            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/include/hw/loader.h b/include/hw/loader.h
index 67a0af84ac..10ff0ff76d 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -163,7 +163,8 @@ int load_aout(const char *filename, hwaddr addr, int max_sz,
 /** 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. Populated with the
+ *            load address. Ignored if NULL.
  * @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

On Thu, 29 Nov 2018 at 20:22, Nick Hudson <nicholas.a.hudson@gmail.com> 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 bootloader fits in the space that the uboot header would have occupied.
>
> Update the load_uimage API to allow passing of load address when an image
> doesn't specify one.
>
> Signed-off-by: Nick Hudson <skrll@netbsd.org>
> ---

> diff --git a/include/hw/loader.h b/include/hw/loader.h
> index 67a0af84ac..10ff0ff76d 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -163,7 +163,8 @@ int load_aout(const char *filename, hwaddr addr, int max_sz,
>  /** 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. Populated with the
> + *            load address. Ignored if NULL.

This API change will break some existing users. For instance
hw/microblaze/boot.c does this:
            hwaddr uentry, loadaddr;

            kernel_size = load_uimage(kernel_filename, &uentry, &loadaddr, 0,
                                      NULL, NULL);

which is a legitimate use of the current API, but your changes will
mean that load_uboot_image() will be using an uninitialized
variable. Getting every call site that cares about the returned
result from loadaddr to also provide a valid default load
address as input is going to be really painful, so I think we
need to figure out an API for this function that doesn't
make the input (default load address) argument the same
parameter as the output (where we actually loaded the image)
argument.

Off the top of my head, the first thing that comes to mind is
adding another argument to the function:
 @defaultloadaddr: Load address to use if none specified in
          the image. If NULL, no default address is available
          (and images which do not specify a load address will
          not be loadable).
but there might be a neater approach than that that I haven't
thought of.

We should also make the load_uboot_image() function print an
error message if we don't load the file because it needs a
default load address and the caller didn't pass one
("this image format cannot be loaded on this machine type",
or similar).

PS: our coding style wants {} on all if()s -- scripts/checkpatch.pl
can help catch this kind of nit.

thanks
-- PMM

On 30/11/2018 17:18, Peter Maydell wrote:
> On Thu, 29 Nov 2018 at 20:22, Nick Hudson <nicholas.a.hudson@gmail.com> 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 bootloader fits in the space that the uboot header would have occupied.
>>
>> Update the load_uimage API to allow passing of load address when an image
>> doesn't specify one.
>>
>> Signed-off-by: Nick Hudson <skrll@netbsd.org>
>> ---
>> diff --git a/include/hw/loader.h b/include/hw/loader.h
>> index 67a0af84ac..10ff0ff76d 100644
>> --- a/include/hw/loader.h
>> +++ b/include/hw/loader.h
>> @@ -163,7 +163,8 @@ int load_aout(const char *filename, hwaddr addr, int max_sz,
>>   /** 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. Populated with the
>> + *            load address. Ignored if NULL.
> This API change will break some existing users. For instance
> hw/microblaze/boot.c does this:
>              hwaddr uentry, loadaddr;
>
>              kernel_size = load_uimage(kernel_filename, &uentry, &loadaddr, 0,
>                                        NULL, NULL);

I did a bit more digging and the same (ab)use of loadaddr to pass

a value is used by load_ramdisk{,_as}.  Hopefully, I can do the same

for kernel_noload?



> which is a legitimate use of the current API, but your changes will
> mean that load_uboot_image() will be using an uninitialized
> variable. Getting every call site that cares about the returned
> result from loadaddr to also provide a valid default load
> address as input is going to be really painful, so I think we
> need to figure out an API for this function that doesn't
> make the input (default load address) argument the same
> parameter as the output (where we actually loaded the image)
> argument.

I have version 3 ready with fixing these callers.


>
> We should also make the load_uboot_image() function print an
> error message if we don't load the file because it needs a
> default load address and the caller didn't pass one
> ("this image format cannot be loaded on this machine type",
> or similar).
Fixed in version 3.
>
> PS: our coding style wants {} on all if()s -- scripts/checkpatch.pl
> can help catch this kind of nit.

There are lots of errors from this without my changes :(


> thanks
> -- PMM


Thanks,

Nick