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();
> 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
