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

[Peter Maydell]

On Thu, 3 Jan 2019 at 16:50, Nick Hudson <address@hidden> wrote:
> On 03/01/2019 16:20, Peter Maydell wrote:
> > On Tue, 11 Dec 2018 at 12:27, Nick Hudson <address@hidden> wrote:
> >> --- 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 ?
>
> Not quite.
>
> They're loaded as if the full noload image (including uboot header)
> was loaded at offset zero, but as load_uboot_image doesn't load the
> header then the image body (minus the header) is loaded at offset
> zero + the size of the u-boot header, i.e. 0x40.
>
> > That's not a great
> > choice, because we put our little mini bootloader at offset 0,
> > and so the two will clash.
>
> Fortuntately, the bootloader fits in the space that the uboot header would
> have occupied.
>
> My commit message has
>
> "The bootloader fits in the space that the uboot header would have occupied."
>
> to try and described this. I could add more of a description if required?

Ah, I missed that. This seems very fragile to me -- 0x40
is only 64 bytes, which is 16 instructions. Currently
our boot loader code is less than that, but only by four
insns or so, so it's very plausible that some future
enhancement to the loader code would take it over the 0x40
boundary, at which point handling of noload images will
silently fail. At a minimum we should have an assertion
that we stay below 0x40; but I'm not keen on restricting
ourselves to 16-instruction bootloaders.

Will a noload image ever in practice also be a Linux kernel
(ie hdr->ih_os == IH_OS_LINUX) ? If not, then we don't lose
anything by not allowing those to be loaded. (And if anybody
does in future complain that their image doesn't work, we
could make it supported by allowing the mini-bootloader to
be placed elsewhere in RAM.)

thanks
-- PMM