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.
>> 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
