On 14 June 2013 17:36, Soren Brinkmann <soren.brinkm...@xilinx.com> wrote:
> Introduce 'load_ramdisk()' which can load "normal" ramdisks and ramdisks
> with a u-boot header.
> To enable this and leverage synergies 'load_uimage()' is refactored to
> accomodate this additional use case.
Hi; thanks for this patch; some review comments below.
>
> Signed-off-by: Soren Brinkmann <soren.brinkm...@xilinx.com>
> ---
> hw/core/loader.c | 86
> +++++++++++++++++++++++++++++++++++++----------------
> include/hw/loader.h | 1 +
> 2 files changed, 61 insertions(+), 26 deletions(-)
>
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 7507914..e72d682 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -434,15 +434,17 @@ static ssize_t gunzip(void *dst, size_t dstlen, uint8_t
> *src,
> }
>
> /* Load a U-Boot image. */
> -int load_uimage(const char *filename, hwaddr *ep,
> - hwaddr *loadaddr, int *is_linux)
> +static int load_uboot_image(const char *filename, hwaddr *ep, hwaddr
> *loadaddr,
> + int *is_linux)
> {
> int fd;
> int size;
> + hwaddr address;
> uboot_image_header_t h;
> uboot_image_header_t *hdr = &h;
> uint8_t *data = NULL;
> int ret = -1;
> + int do_uncompress = 0;
>
> fd = open(filename, O_RDONLY | O_BINARY);
> if (fd < 0)
> @@ -458,31 +460,48 @@ int load_uimage(const char *filename, hwaddr *ep,
> goto out;
>
> /* TODO: Implement other image types. */
Doesn't this patch fix this TODO item?
> - if (hdr->ih_type != IH_TYPE_KERNEL) {
> - fprintf(stderr, "Can only load u-boot image type \"kernel\"\n");
> - goto out;
> - }
> + switch (hdr->ih_type) {
> + case IH_TYPE_KERNEL:
> + address = hdr->ih_load;
> + if (loadaddr) {
> + *loadaddr = hdr->ih_load;
> + }
> +
> + switch (hdr->ih_comp) {
> + case IH_COMP_NONE:
> + break;
> + case IH_COMP_GZIP:
> + do_uncompress = 1;
> + break;
> + default:
> + fprintf(stderr,
> + "Unable to load u-boot images with compression type
> %d\n",
> + hdr->ih_comp);
> + goto out;
> + }
> +
> + if (ep) {
> + *ep = hdr->ih_ep;
> + }
>
> - switch (hdr->ih_comp) {
> - case IH_COMP_NONE:
> - case IH_COMP_GZIP:
> + /* TODO: Check CPU type. */
> + if (is_linux) {
> + if (hdr->ih_os == IH_OS_LINUX) {
> + *is_linux = 1;
> + } else {
> + *is_linux = 0;
> + }
> + }
> +
> + break;
> + case IH_TYPE_RAMDISK:
> + address = *loadaddr;
> break;
> default:
> - fprintf(stderr,
> - "Unable to load u-boot images with compression type %d\n",
> - hdr->ih_comp);
> + fprintf(stderr, "Unsupported u-boot image type\n");
You could include the type byte here, the way we do for
unknown compression types.
> goto out;
> }
>
> - /* TODO: Check CPU type. */
> - if (is_linux) {
> - if (hdr->ih_os == IH_OS_LINUX)
> - *is_linux = 1;
> - else
> - *is_linux = 0;
> - }
> -
> - *ep = hdr->ih_ep;
> data = g_malloc(hdr->ih_size);
>
> if (read(fd, data, hdr->ih_size) != hdr->ih_size) {
> @@ -490,7 +509,7 @@ int load_uimage(const char *filename, hwaddr *ep,
> goto out;
> }
>
> - if (hdr->ih_comp == IH_COMP_GZIP) {
> + if (do_uncompress) {
> uint8_t *compressed_data;
> size_t max_bytes;
> ssize_t bytes;
> @@ -508,10 +527,7 @@ int load_uimage(const char *filename, hwaddr *ep,
> hdr->ih_size = bytes;
> }
>
> - rom_add_blob_fixed(filename, data, hdr->ih_size, hdr->ih_load);
> -
> - if (loadaddr)
> - *loadaddr = hdr->ih_load;
> + rom_add_blob_fixed(filename, data, hdr->ih_size, address);
>
> ret = hdr->ih_size;
>
> @@ -522,6 +538,24 @@ out:
> return ret;
> }
>
> +int load_uimage(const char *filename, hwaddr *ep, hwaddr *loadaddr,
> + int *is_linux)
> +{
> + return load_uboot_image(filename, ep, loadaddr, is_linux);
> +}
> +
> +/* Load a ramdisk. */
> +int load_ramdisk(const char *filename, hwaddr addr, uint64_t max_sz)
> +{
> + int size = load_uboot_image(filename, NULL, &addr, NULL);
> +
> + if (size < 0) {
> + size = load_image_targphys(filename, addr, max_sz);
> + }
So I can sort of see why we end up this way, but it's a bit
asymmetric that we handle 'uimage or raw' ramdisk at this
level, but require the caller to do it for the kernel.
> +
> + return size;
> +}
If we're providing separate functions for kernel and
ramdisk loading shouldn't they check that the uimage
has the appropriate type?
> +
> /*
> * Functions for reboot-persistent memory regions.
> * - used for vga bios and option roms.
> diff --git a/include/hw/loader.h b/include/hw/loader.h
> index 0958f06..8ef403e 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -15,6 +15,7 @@ int load_aout(const char *filename, hwaddr addr, int max_sz,
> int bswap_needed, hwaddr target_page_size);
> int load_uimage(const char *filename, hwaddr *ep,
> hwaddr *loadaddr, int *is_linux);
> +int load_ramdisk(const char *filename, hwaddr addr, uint64_t max_sz);
Since this is a new function, it should have a suitably
formatted documentation comment. (the extract and deposit
functions at the bottom of include/qemu/bitops.h are the
examples of the format that I usually crib from.)
thanks
-- PMM
