| Message ID | 1371227783-23086-2-git-send-email-soren.brinkmann@xilinx.com |
|---|---|
| State | New |
| Headers | show |
On 14 June 2013 17:36, Soren Brinkmann <soren.brinkmann@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.brinkmann@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
Hi Peter, On Fri, Jun 14, 2013 at 05:56:31PM +0100, Peter Maydell wrote: > On 14 June 2013 17:36, Soren Brinkmann <soren.brinkmann@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.brinkmann@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? 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
On 14 June 2013 18:14, Sören Brinkmann <soren.brinkmann@xilinx.com> wrote: > On Fri, Jun 14, 2013 at 05:56:31PM +0100, Peter Maydell wrote: >> 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. I had in mind that these functions should pass the "expected" type byte in to the load_uboot_image() function so it can fail if the blob it is passed isn't actually the right type (eg you pass the kernel to -initrd and vice-versa). thanks -- PMM
On Fri, Jun 14, 2013 at 06:36:06PM +0100, Peter Maydell wrote: > On 14 June 2013 18:14, Sören Brinkmann <soren.brinkmann@xilinx.com> wrote: > > On Fri, Jun 14, 2013 at 05:56:31PM +0100, Peter Maydell wrote: > >> 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. > > I had in mind that these functions should pass the "expected" > type byte in to the load_uboot_image() function so it can > fail if the blob it is passed isn't actually the right type > (eg you pass the kernel to -initrd and vice-versa). Thanks for the clarification. I agree. I'll add such a check. Sören
Hi Soren, On Sat, Jun 15, 2013 at 3:14 AM, Sören Brinkmann <soren.brinkmann@xilinx.com> wrote: > Hi Peter, > > On Fri, Jun 14, 2013 at 05:56:31PM +0100, Peter Maydell wrote: >> On 14 June 2013 17:36, Soren Brinkmann <soren.brinkmann@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. [snip] >> > +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. > I prefer the symmetric approach, The if-else logic on image types is a board/arch specific policy (even though it arguably shouldn't be). Fixing this I think is best left to when we get around to doing a bootloader overhaul. Can we just drop this if and push the targphys fallback to the arch bootloaders? Regards, Peter
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. */ - 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"); 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); + } + + return size; +} + /* * 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); ssize_t read_targphys(const char *name, int fd, hwaddr dst_addr, size_t nbytes);
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> --- hw/core/loader.c | 86 +++++++++++++++++++++++++++++++++++++---------------- include/hw/loader.h | 1 + 2 files changed, 61 insertions(+), 26 deletions(-)