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