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

[Peter Maydell]

On Tue, 11 Dec 2018 at 12:27, Nick Hudson <address@hidden> 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.
>
> 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.
>
> Signed-off-by: Nick Hudson<address@hidden>
>
> ---
>   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(-)

Hi; I tried applying your patch, but unfortunately it looks like
your email client has mangled it to the point that it can't
be applied:
 * it is format=flowed, so long lines have been wrapped
 * some whitespace has been turned into UTF-8 non-breaking space characters
I tried to manually fix these up, but didn't succeed.

It looks like you're using Thunderbird -- the kernel docs
have some config setting suggestions that might help:
https://www.kernel.org/doc/html/v4.11/process/email-clients.html#thunderbird-gui
or you could try switching to 'git send-email'.

I've reviewed the patch for content and it's mostly good:
I just have a couple more questions below.

> 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

If I'm reading the code right, this requests that noload images
are loaded at offset zero into RAM, yes ? That's not a great
choice, because we put our little mini bootloader at offset 0,
and so the two will clash. Can we put them somewhere else
(above BOOTLOADER_MAX_SIZE, probably at a 64K page boundary) ?

If they must go at offset 0 then we'd need to refuse to load
noload images if they set is_linux to true. (We only load the
mini bootloader if is_linux is true, so that's the only case where
there's a clash.)

> +#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..7362197162 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -638,13 +638,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) {

I think you have this condition wrong. Suppose we are
trying to load image_type == IH_TYPE_KERNEL and we are
passed a hdr->ih_type == IH_TYPE_RAMDISK. With this change
we'll stop rejecting that as an error.
I think you want:
    if (!(image_type == IH_TYPE_KERNEL &&
          hdr->ih_type == IH_TYPE_KERNEL_NOLOAD)) {

because the only kind of mismatch we want to accept is
"we're looking for KERNEL and we got KERNEL_NOLOAD".

> +            fprintf(stderr, "Wrong image type %d, expected %d\n",
> hdr->ih_type,
> +                    image_type);
> +            goto out;
> +        }
>       }

> index f5720f979e..70428b1865 100644
> --- a/hw/ppc/ppc440_bamboo.c
> +++ b/hw/ppc/ppc440_bamboo.c
> @@ -180,7 +180,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 5aac58f36e..2d6d5c4402 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;

In both of these cases we could in fact remove the 'loadaddr'
variable entirely (and pass NULL to the load_uimage() 3rd
argument) because the code doesn't ever read the loadaddr value
that is set. But this is the simple minimal change so it's OK:
we can do the cleanup later (or not at all).

thanks
-- PMM