On 29 August 2012 15:24, Dave Martin <dave.martin@xxxxxxxxxx> wrote:
> On Wed, Aug 29, 2012 at 03:04:57PM +0100, Ryan Harkin wrote:
>> On 29 August 2012 14:59, Dave Martin <dave.martin@xxxxxxxxxx> wrote:
>> > 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.
>> >
>>
>> Sounds fair. UEFI has an RTSM version, by the way ;-)
>
> Indeed -- part of the point of not investing too much in the bootwrapper
> is that we should ultimately not need it as the UEFI implementation
> becomes more mature. Keeping it small and under our control also makes
> it easy to hack on if we quickly need to experiment with something --
> such as the Hyp mode booting support, for example.
>
>> All I was meaning was that there are now two functions, almost
>> identical, for removing the image header from either uImage or
>> uInitrd. Making one function seemed logical, as they are still only
>> skipping 64 bytes at the start of the image and not reading any u-boot
>> magic. It's just that one modifies the pointer and the other does a
>> memmove. Modifying the pointer for the uImage rather than doing a
>> memmove will shave off a few CPU cycles too.
>
> The kernel should be mmoved because it may not be position-independent
> (zImage is quasi-position-independent, but Image is not -- it we wanted
> to handle both cases we would need to add more code to do the right
> thing etc. etc.)
>
> We could refactor so that the initrd is mmoved unnecessarily, but I'm
> not sure it's that much of a benefit.
>
Agreed. I think we can ignore my review commends in this case.
> 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
