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

[Sören Brinkmann]

Hi Peter,

On Fri, Jun 14, 2013 at 05:56:31PM +0100, Peter Maydell wrote:
> 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?
Well, partly. There are a couple of more image types which are still
not supported. So, I didnt' bother touching this comment just because
this adds support for a second out of 10 or something types.

> 
> > -    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.
good call. I'll add that.

> 
> >          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.
I agree it's not symmetric. The question is: Leaving this as is and hope
somebody changes the kernel loading. Or should this be changed with
having the "normal" ramdisk fallback at caller level?
I like this solution better, but well, that's probably just me.

> 
> > +
> > +    return size;
> > +}
> 
> If we're providing separate functions for kernel and
> ramdisk loading shouldn't they check that the uimage
> has the appropriate type?
I'm not sure I understand what you mean here. Moving the check for the
appropriate u-boot header type down here? I tried to find some
reasonable way to further split the load_uboot_image() routine to move
some of it's functionality into the two functions down here. But it all
seemed pretty messy and I left pretty much all functionality in
load_uboot_image() and just pass errors through.

> 
> > +
> >  /*
> >   * 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.)
Okay, I'll have a look at those.

        Sören