[Qemu-devel] [PATCH v3 0/2] Support ramdisks with U-Boot header

[Qemu-devel] [PATCH v3 0/2] Support ramdisks with U-Boot header

[Soren Brinkmann]

Sorry, for the delay, but I finally found some time for a follow up. I assume
the asymmety between loading kernels and loading the ramdisk is the blocker for
this series and fixed it up.
Detailed changelog in the following emails.

	Sören

Soren Brinkmann (2):
  hw/loader: Support ramdisk with u-boot header
  hw/arm: Use 'load_ramdisk()' for loading ramdisks w/ U-Boot header

 hw/arm/boot.c       | 14 ++++++---
 hw/core/loader.c    | 84 +++++++++++++++++++++++++++++++++++++----------------
 include/hw/loader.h | 13 +++++++++
 3 files changed, 82 insertions(+), 29 deletions(-)

-- 
1.8.3.2

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

[Soren Brinkmann]

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.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
v3:
 - remove fallback code from load_ramdisk(). This way the function
   loads a u-boot-wrapped ramdisk or returns an error, the arch specific
   loader code is supposed to handle.
   Changed this way to achieve symmetry with the kernel loading code.

v2:
 - document the new 'load_ramdisk()' function
 - print the image type byte in the error messages for unsupported
   u-boot images
 - load_uimage() and load_ramdisk() both use load_uboot_image() to load
   images. Let the callers pass the expected image type to
   load_uboot_image to allow error checking for wrong image types. E.g.
   passing a ramdisk image to the load_uimage routine.


 hw/core/loader.c    | 84 +++++++++++++++++++++++++++++++++++++----------------
 include/hw/loader.h | 13 +++++++++
 2 files changed, 72 insertions(+), 25 deletions(-)

diff --git a/hw/core/loader.c b/hw/core/loader.c
index 44311ff..c3c28cf 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, uint8_t image_type)
 {
     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)
@@ -457,32 +459,55 @@ int load_uimage(const char *filename, hwaddr *ep,
     if (hdr->ih_magic != IH_MAGIC)
         goto out;
 
-    /* TODO: Implement other image types.  */
-    if (hdr->ih_type != IH_TYPE_KERNEL) {
-        fprintf(stderr, "Can only load u-boot image type \"kernel\"\n");
+    if (hdr->ih_type != image_type) {
+        fprintf(stderr, "Wrong image type %d, expected %d\n", hdr->ih_type,
+                image_type);
         goto out;
     }
 
-    switch (hdr->ih_comp) {
-    case IH_COMP_NONE:
-    case IH_COMP_GZIP:
+    /* TODO: Implement other image types.  */
+    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;
+        }
+
+        /* 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 %d\n", hdr->ih_type);
         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 +515,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 +533,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 +544,18 @@ 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, IH_TYPE_KERNEL);
+}
+
+/* Load a ramdisk.  */
+int load_ramdisk(const char *filename, hwaddr addr, uint64_t max_sz)
+{
+    return load_uboot_image(filename, NULL, &addr, NULL, IH_TYPE_RAMDISK);
+}
+
 /*
  * 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 15d4cc9..eb9c9a3 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -17,6 +17,19 @@ int load_aout(const char *filename, hwaddr addr, int max_sz,
 int load_uimage(const char *filename, hwaddr *ep,
                 hwaddr *loadaddr, int *is_linux);
 
+/**
+ * load_ramdisk:
+ * @filename: Path to the ramdisk image
+ * @addr: Memory address to load the ramdisk to
+ * @max_sz: Maximum allowed ramdisk size (for non-u-boot ramdisks)
+ *
+ * Load a ramdisk image with U-Boot header to the specified memory
+ * address.
+ *
+ * Returns the size of the loaded image on success, -1 otherwise.
+ */
+int load_ramdisk(const char *filename, hwaddr addr, uint64_t max_sz);
+
 ssize_t read_targphys(const char *name,
                       int fd, hwaddr dst_addr, size_t nbytes);
 void pstrcpy_targphys(const char *name,
-- 
1.8.3.2

[Qemu-devel] [PATCH v3 2/2] hw/arm: Use 'load_ramdisk()' for loading ramdisks w/ U-Boot header

[Soren Brinkmann]

The load_ramdisk function is used to load ramdisk featuring a U-Boot
header.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
v2:
 - try to load a u-boot ramdisk using the new load_ramdisk() function
   first. And then, in case of an error, fall back to the traditional
   way for loading a ramdisk. In order to keep ramdisk and kernel
   loading symmetrical.

 hw/arm/boot.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index 7c0090f..a356b53 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -425,10 +425,16 @@ void arm_load_kernel(ARMCPU *cpu, struct arm_boot_info *info)
     info->entry = entry;
     if (is_linux) {
         if (info->initrd_filename) {
-            initrd_size = load_image_targphys(info->initrd_filename,
-                                              info->initrd_start,
-                                              info->ram_size -
-                                              info->initrd_start);
+            initrd_size = load_ramdisk(info->initrd_filename,
+                                       info->initrd_start,
+                                       info->ram_size -
+                                       info->initrd_start);
+            if (initrd_size < 0) {
+                initrd_size = load_image_targphys(info->initrd_filename,
+                                                  info->initrd_start,
+                                                  info->ram_size -
+                                                  info->initrd_start);
+            }
             if (initrd_size < 0) {
                 fprintf(stderr, "qemu: could not load initrd '%s'\n",
                         info->initrd_filename);
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH v3 0/2] Support ramdisks with U-Boot header

[Sören Brinkmann]

ping?
any comments?

	Thanks,
	Sören

On Mon, Jul 08, 2013 at 03:40:00PM -0700, Soren Brinkmann wrote:
> Sorry, for the delay, but I finally found some time for a follow up. I assume
> the asymmety between loading kernels and loading the ramdisk is the blocker for
> this series and fixed it up.
> Detailed changelog in the following emails.
> 
> 	Sören
> 
> Soren Brinkmann (2):
>   hw/loader: Support ramdisk with u-boot header
>   hw/arm: Use 'load_ramdisk()' for loading ramdisks w/ U-Boot header
> 
>  hw/arm/boot.c       | 14 ++++++---
>  hw/core/loader.c    | 84 +++++++++++++++++++++++++++++++++++++----------------
>  include/hw/loader.h | 13 +++++++++
>  3 files changed, 82 insertions(+), 29 deletions(-)
> 
> -- 
> 1.8.3.2
> 
> 

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

[Peter Maydell]

On 8 July 2013 23:40, Soren Brinkmann <soren.brinkmann@xilinx.com> wrote:
> +
> +        if (ep) {
> +            *ep = hdr->ih_ep;
> +        }

(Allowing ep to be NULL for IH_TYPE_KERNEL is new behaviour,
but it makes sense for consistency with the other pointer
argument handling.)

> +
> +        /* 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;

This is inconsistent -- for IH_TYPE_KERNEL we accept
a NULL loadaddr, but for IH_TYPE_RAMDISK we don't.

-- PMM

Re: [Qemu-devel] [PATCH v3 2/2] hw/arm: Use 'load_ramdisk()' for loading ramdisks w/ U-Boot header

[Peter Maydell]

On 8 July 2013 23:40, Soren Brinkmann <soren.brinkmann@xilinx.com> wrote:
> The load_ramdisk function is used to load ramdisk featuring a U-Boot
> header.
>
> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM

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

[Sören Brinkmann]

On Fri, Jul 19, 2013 at 01:04:20PM +0100, Peter Maydell wrote:
> On 8 July 2013 23:40, Soren Brinkmann <soren.brinkmann@xilinx.com> wrote:
> > +
> > +        if (ep) {
> > +            *ep = hdr->ih_ep;
> > +        }
> 
> (Allowing ep to be NULL for IH_TYPE_KERNEL is new behaviour,
> but it makes sense for consistency with the other pointer
> argument handling.)
> 
> > +
> > +        /* 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;
> 
> This is inconsistent -- for IH_TYPE_KERNEL we accept
> a NULL loadaddr, but for IH_TYPE_RAMDISK we don't.
The thing is in case of a ramdisk, it's a mandatory input argument which has
to be provided, whereas for the kernel, it's an optional output value.
And other than the load_ramdisk() and load_kernel() routines nobody is
supposed to call this function directly, IMHO. And I think plausibility
checks should be done in those routines when possible. And
load_ramdisk() ensures that the loadaddr pointer is valid.

What would be your suggested change?

	Soren

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

[Peter Maydell]

On 19 July 2013 18:39, Sören Brinkmann <soren.brinkmann@xilinx.com> wrote:
> On Fri, Jul 19, 2013 at 01:04:20PM +0100, Peter Maydell wrote:
>> On 8 July 2013 23:40, Soren Brinkmann <soren.brinkmann@xilinx.com> wrote:
>> > +
>> > +        if (ep) {
>> > +            *ep = hdr->ih_ep;
>> > +        }
>>
>> (Allowing ep to be NULL for IH_TYPE_KERNEL is new behaviour,
>> but it makes sense for consistency with the other pointer
>> argument handling.)
>>
>> > +
>> > +        /* 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;
>>
>> This is inconsistent -- for IH_TYPE_KERNEL we accept
>> a NULL loadaddr, but for IH_TYPE_RAMDISK we don't.
> The thing is in case of a ramdisk, it's a mandatory input argument which has
> to be provided, whereas for the kernel, it's an optional output value.
> And other than the load_ramdisk() and load_kernel() routines nobody is
> supposed to call this function directly, IMHO. And I think plausibility
> checks should be done in those routines when possible. And
> load_ramdisk() ensures that the loadaddr pointer is valid.

Well, by that argument you shouldn't introduce the "allow
ep to be NULL" change...

> What would be your suggested change?

I don't mind as long as we're consistent one way or the other
about whether non-optional pointer arguments are checked for
NULL.

-- PMM

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

[Sören Brinkmann]

On Fri, Jul 19, 2013 at 06:46:57PM +0100, Peter Maydell wrote:
> On 19 July 2013 18:39, Sören Brinkmann <soren.brinkmann@xilinx.com> wrote:
> > On Fri, Jul 19, 2013 at 01:04:20PM +0100, Peter Maydell wrote:
> >> On 8 July 2013 23:40, Soren Brinkmann <soren.brinkmann@xilinx.com> wrote:
> >> > +
> >> > +        if (ep) {
> >> > +            *ep = hdr->ih_ep;
> >> > +        }
> >>
> >> (Allowing ep to be NULL for IH_TYPE_KERNEL is new behaviour,
> >> but it makes sense for consistency with the other pointer
> >> argument handling.)
> >>
> >> > +
> >> > +        /* 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;
> >>
> >> This is inconsistent -- for IH_TYPE_KERNEL we accept
> >> a NULL loadaddr, but for IH_TYPE_RAMDISK we don't.
> > The thing is in case of a ramdisk, it's a mandatory input argument which has
> > to be provided, whereas for the kernel, it's an optional output value.
> > And other than the load_ramdisk() and load_kernel() routines nobody is
> > supposed to call this function directly, IMHO. And I think plausibility
> > checks should be done in those routines when possible. And
> > load_ramdisk() ensures that the loadaddr pointer is valid.
> 
> Well, by that argument you shouldn't introduce the "allow
> ep to be NULL" change...
I didn't introduce it, that is the current state for loading a kernel.
I introduced making it mandatory for the ramdisk case.

> 
> > What would be your suggested change?
> 
> I don't mind as long as we're consistent one way or the other
> about whether non-optional pointer arguments are checked for
> NULL.
As I said, depending on whether we call this function to load a kernel
or ramdisk that argument is optional or mandatory.
When it's optional we do a NULL check. In the mandatory case the caller
ensures validity.

So, if leaving it as is, is not an option.
How about this:
Moving the NULL check to load_kernel() and passing an always valid
pointer to load_uboot_image() and removing the NULL check there?
Then load_kernel() can pass on this value or not depending on the
validity of the pointer it received.

	Soren

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

[Peter Maydell]

On 19 July 2013 18:53, Sören Brinkmann <soren.brinkmann@xilinx.com> wrote:
> On Fri, Jul 19, 2013 at 06:46:57PM +0100, Peter Maydell wrote:
>> On 19 July 2013 18:39, Sören Brinkmann <soren.brinkmann@xilinx.com> wrote:
>> > On Fri, Jul 19, 2013 at 01:04:20PM +0100, Peter Maydell wrote:
>> >> On 8 July 2013 23:40, Soren Brinkmann <soren.brinkmann@xilinx.com> wrote:
>> >> > +
>> >> > +        if (ep) {
>> >> > +            *ep = hdr->ih_ep;
>> >> > +        }
>> >>
>> >> (Allowing ep to be NULL for IH_TYPE_KERNEL is new behaviour,
>> >> but it makes sense for consistency with the other pointer
>> >> argument handling.)
>> >>
>> >> > +
>> >> > +        /* 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;
>> >>
>> >> This is inconsistent -- for IH_TYPE_KERNEL we accept
>> >> a NULL loadaddr, but for IH_TYPE_RAMDISK we don't.
>> > The thing is in case of a ramdisk, it's a mandatory input argument which has
>> > to be provided, whereas for the kernel, it's an optional output value.
>> > And other than the load_ramdisk() and load_kernel() routines nobody is
>> > supposed to call this function directly, IMHO. And I think plausibility
>> > checks should be done in those routines when possible. And
>> > load_ramdisk() ensures that the loadaddr pointer is valid.
>>
>> Well, by that argument you shouldn't introduce the "allow
>> ep to be NULL" change...
> I didn't introduce it, that is the current state for loading a kernel.

Current code:
    *ep = hdr->ih_ep;

Your patch:
+        if (ep) {
+            *ep = hdr->ih_ep;
+        }

On reflection I feel like this is making a mountain out of
a molehill, though, so:

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM

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

[Sören Brinkmann]

On Sat, Jul 20, 2013 at 12:20:48AM +0100, Peter Maydell wrote:
> On 19 July 2013 18:53, Sören Brinkmann <soren.brinkmann@xilinx.com> wrote:
> > On Fri, Jul 19, 2013 at 06:46:57PM +0100, Peter Maydell wrote:
> >> On 19 July 2013 18:39, Sören Brinkmann <soren.brinkmann@xilinx.com> wrote:
> >> > On Fri, Jul 19, 2013 at 01:04:20PM +0100, Peter Maydell wrote:
> >> >> On 8 July 2013 23:40, Soren Brinkmann <soren.brinkmann@xilinx.com> wrote:
> >> >> > +
> >> >> > +        if (ep) {
> >> >> > +            *ep = hdr->ih_ep;
> >> >> > +        }
> >> >>
> >> >> (Allowing ep to be NULL for IH_TYPE_KERNEL is new behaviour,
> >> >> but it makes sense for consistency with the other pointer
> >> >> argument handling.)
> >> >>
> >> >> > +
> >> >> > +        /* 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;
> >> >>
> >> >> This is inconsistent -- for IH_TYPE_KERNEL we accept
> >> >> a NULL loadaddr, but for IH_TYPE_RAMDISK we don't.
> >> > The thing is in case of a ramdisk, it's a mandatory input argument which has
> >> > to be provided, whereas for the kernel, it's an optional output value.
> >> > And other than the load_ramdisk() and load_kernel() routines nobody is
> >> > supposed to call this function directly, IMHO. And I think plausibility
> >> > checks should be done in those routines when possible. And
> >> > load_ramdisk() ensures that the loadaddr pointer is valid.
> >>
> >> Well, by that argument you shouldn't introduce the "allow
> >> ep to be NULL" change...
> > I didn't introduce it, that is the current state for loading a kernel.
> 
> Current code:
>     *ep = hdr->ih_ep;
> 
> Your patch:
> +        if (ep) {
> +            *ep = hdr->ih_ep;
> +        }
> 
> On reflection I feel like this is making a mountain out of
> a molehill, though, so:
I'm sorry you're right. I was looking at 'loadaddr' and 'is_linux'. Looking
at ep, I wonder why I changed it, too.
Maybe I wanted to make it a little more robust against wrongly calling
load_ramdisk() with a kernel image. In my original patch that would have
gone through and resulted in a seg-fault because of a missing NULL
check, I think. And it would have been easily triggered just by mixing
up the '-initrd' and '-kernel' command line parameters.
But in v2, I added your proposed header type checking. So, this error
scenario would be recognized properly and the check for 'ep' is
obsolete.

Up to you, leaving as is and have consistent NULL checking on all
pointer arguments, or changing this back to what it was?

	Sören