Re: [Qemu-devel] [PATCH 1/2] hw/loader: Support ramdisk with u-boot header

[Peter Maydell]

On 14 June 2013 17:36, Soren Brinkmann <address@hidden> 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 <address@hidden>
> ---
>  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