On Wed, Aug 29, 2012 at 04:07:23PM +0100, Ryan Harkin wrote: > 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. OK, thanks for reviewing. Cheers ---Dave _______________________________________________ kvmarm mailing list kvmarm@xxxxxxxxxxxxxxxxxxxxx https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
