On Wed, Aug 29, 2012 at 01:03:36PM +0100, Ryan Harkin wrote:
> Hi Guodong,
>
> On 29 August 2012 11:36, Dave Martin <dave.martin@xxxxxxxxxx> wrote:
> > On Wed, Aug 29, 2012 at 05:50:47PM +0800, Guodong Xu wrote:
> >> Hi, Peter
> >>
> >> I updated the code:
> >> 1. make it a common code: is_uboot_image_format();
>
> My main comment with this patch is that I think you could make more of
> the code common. I suspect you could write a generic function that
> will check and skip the u-boot header of any image, whether it is
> uImage or uInitrd.
>
> This assumes that you can also move the start pointer to the uImage,
> rather than memmove it.
I don't favour this evolving into anything resembling "proper uImage
support". If we really want that, we should port U-Boot, not try to
turn the bootwrapper into U-Boot.
Maintaining the bootwrapper as a separate thing only makes sense if
it is much less functional than a real bootloader -- otherwise porting
a real bootloader is likely to be the better investment of effort.
Cheers
---Dave
>
>
> >> 2. in case of a match in initrd, move the start pointer. no memory move.
> >>
> >> Could you please have a review? Thanks.
> >>
> >
> >> From 575a7d0ffbcae463b5f2cf572ee5c3d3d0a89d1f Mon Sep 17 00:00:00 2001
> >> From: Guodong Xu <guodong.xu@xxxxxxxxxx>
> >> Date: Wed, 29 Aug 2012 17:26:04 +0800
> >> Subject: [PATCH] semi_loader: Add support to initrd of u-boot image format
> >>
> >> Add support to initrd images in u-boot image format. Image header
> >> is checked against u-boot image header magic. In case of a match,
> >> u-boot image header is ignored.
> >>
> >> Signed-off-by: Guodong Xu <guodong.xu@xxxxxxxxxx>
> >> ---
> >> semi_loader.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++--------
> >> semi_loader.h | 5 ++-
> >> 2 files changed, 52 insertions(+), 11 deletions(-)
> >>
> >
> > I have a few minor comments (see below), but otherwise this looks OK to
> > me.
> >
> > Cheers
> > ---Dave
> >
> >> diff --git a/semi_loader.c b/semi_loader.c
> >> index 2a30c5a..7d8128d 100644
> >> --- a/semi_loader.c
> >> +++ b/semi_loader.c
> >> @@ -299,12 +299,32 @@ static void load_file_essential(void **dest, char const *filename,
> >> fatal(failmsg, ": \"", filename, "\"\n");
> >> }
> >>
> >> +/* is_uboot_image_format - to check an image is in uboot image format or not
> >> + * @start: start address of the image
> >> + * @size: size of the image
> >> + *
> >> + * Returns:
> >> + * 0: no
> >> + * 1: yes
> >> + */
> >> +static int is_uboot_image_format(unsigned start, unsigned size)
> >> +{
> >> + if(size <= UBOOT_IMAGE_HEADER_SIZE)
> >> + return 0;
> >> +
> >> + if(memcmp((char *)start, uboot_image_header_magic,
> >> + sizeof uboot_image_header_magic))
> >> + return 0;
> >> +
> >> + return 1;
> >
> > Or, more concisely:
> >
> > return !memcmp((char *)start, uboot_image_header_magic,
> > sizeof uboot_image_header_magic))
> >
> >> +}
> >> +
> >> /* Move the kernel if necessary, based on the image type: */
> >> static void correct_kernel_location(struct loader_info *info)
> >> {
> >> char *const text_start = (char *)(PHYS_OFFSET + TEXT_OFFSET);
> >> char *const text_end = text_start + info->kernel_size;
> >> - char *const uImage_payload = text_start + UIMAGE_HEADER_SIZE;
> >> + char *const uImage_payload = text_start + UBOOT_IMAGE_HEADER_SIZE;
> >> unsigned long *const zImage_magic_p = (unsigned long *)(
> >> uImage_payload + ZIMAGE_MAGIC_OFFSET);
> >>
> >> @@ -312,10 +332,7 @@ static void correct_kernel_location(struct loader_info *info)
> >> * If the image is not a uImage, then it is a raw Image or zImage,
> >> * and no action is necessary:
> >> */
> >> - if(info->kernel_size <= UIMAGE_HEADER_SIZE)
> >> - return;
> >> -
> >> - if(memcmp(text_start, uImage_magic, sizeof uImage_magic))
> >> + if(!is_uboot_image_format((unsigned)text_start, info->kernel_size))
> >> return;
> >>
> >> warn("Ignoring uImage meta-data\n");
> >> @@ -328,7 +345,7 @@ static void correct_kernel_location(struct loader_info *info)
> >> */
> >> if(text_end >= (char *)&zImage_magic_p[1]
> >> && *zImage_magic_p == ZIMAGE_MAGIC) {
> >> - info->kernel_entry += UIMAGE_HEADER_SIZE;
> >> + info->kernel_entry += UBOOT_IMAGE_HEADER_SIZE;
> >> return;
> >> }
> >>
> >> @@ -337,7 +354,23 @@ static void correct_kernel_location(struct loader_info *info)
> >> * leave the entry point unmodified.
> >> */
> >> memmove(text_start, uImage_payload,
> >> - info->kernel_size - UIMAGE_HEADER_SIZE);
> >> + info->kernel_size - UBOOT_IMAGE_HEADER_SIZE);
> >> +}
> >> +
> >> +/* move initrd to correct place, if necessary */
> >
> > This comment is now wrong, because you don't actually move the initrd
> > image any more.
> >
> >> +static void correct_initrd_location(struct loader_info *info)
> >> +{
> >> + /*
> >> + * if initrd image is in u-boot image format,
> >> + * move initrd_start and initrd_size to ignore the header
> >> + */
> >
> > ... and this comment.
> >
> > I suggest you keep just this second comment, since it is more
> > informative.
> >
> >> + if(is_uboot_image_format(info->initrd_start, info->initrd_size)) {
> >> + warn("Ignoring uInitrd meta-data\n");
> >> + info->initrd_start += UBOOT_IMAGE_HEADER_SIZE;
> >> + info->initrd_size -= UBOOT_IMAGE_HEADER_SIZE;
> >> + }
> >> +
> >> + return;
> >> }
> >>
> >> static char semi_cmdline[SEMI_CMDLINE_MAX];
> >> @@ -496,8 +529,15 @@ args_done:
> >> "Failed to load initrd image");
> >> info("Loaded initrd: ", initrd_arg, "\n");
> >>
> >> - info->initrd_start = atag.initrd.start = start;
> >> - info->initrd_size = atag.initrd.size = (unsigned)phys - start;
> >> + info->initrd_start = start;
> >> + info->initrd_size = (unsigned)phys - start;
> >> +
> >> + /* Move initrd to correct place, if necessary */
> >
> > This comment is wrong too. But you can remove it -- it's fairly
> > obvious from the name of the called function what is being done here.
> >
> >> + correct_initrd_location(info);
> >> +
> >> + atag.initrd.start = info->initrd_start;
> >> + atag.initrd.size = info->initrd_size;
> >> +
> >> } else if(info->initrd_size) {
> >> if(noinitrd_arg) {
> >> info->initrd_size = 0;
> >> diff --git a/semi_loader.h b/semi_loader.h
> >> index 8a1a602..5720fd2 100644
> >> --- a/semi_loader.h
> >> +++ b/semi_loader.h
> >> @@ -46,10 +46,11 @@ struct atag_initrd2 {
> >> unsigned size;
> >> };
> >>
> >> -static const char uImage_magic[] = {
> >> +/* U-Boot Image format is supported */
> >
> > This is a bit optimistic -- we don't really process the U-Boot header
> > at all. Instead, we just ignore it if it is present.
> >
> > I suggest you remove this comment too: the name of the object makes it
> > clear anyway.
> >
> >> +static const char uboot_image_header_magic[] = {
> >> 0x27, 0x05, 0x19, 0x56
> >> };
> >> -#define UIMAGE_HEADER_SIZE 0x40
> >> +#define UBOOT_IMAGE_HEADER_SIZE 0x40
> >>
> >> #define ZIMAGE_MAGIC_OFFSET 36
> >> #define ZIMAGE_MAGIC 0x016f2818UL
> >> --
> >> 1.7.4.1
> >>
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
