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

[Nick Hudson]

On 30/11/2018 17:18, Peter Maydell wrote:
> On Thu, 29 Nov 2018 at 20:22, 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.
> >
> > Update the load_uimage API to allow passing of load address when an image
> > doesn't specify one.
> >
> > Signed-off-by: Nick Hudson <address@hidden>
> > ---
> > 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