On Mon, Aug 02, 2010 at 12:33:54PM -0700, Hollis Blanchard wrote: > On Mon, Aug 2, 2010 at 11:57 AM, Edgar E. Iglesias > <edgar.igles...@gmail.com> wrote: > > On Mon, Aug 02, 2010 at 10:59:11AM -0700, Hollis Blanchard wrote: > >> On Sun, Aug 1, 2010 at 5:36 AM, Edgar E. Iglesias > >> <edgar.igles...@gmail.com> wrote: > >> > On Sat, Jul 31, 2010 at 12:56:42AM +0200, Edgar E. Iglesias wrote: > >> >> On Thu, Jul 29, 2010 at 06:48:24PM -0700, Hollis Blanchard wrote: > >> >> > The kernel's BSS size is lost by mkimage, which only considers file > >> >> > size. As a result, loading other blobs (e.g. device tree, initrd) > >> >> > immediately after the kernel location can result in them being zeroed > >> >> > by > >> >> > the kernel's BSS initialization code. > >> >> > > >> >> > Signed-off-by: Hollis Blanchard <hol...@penguinppc.org> > >> >> > --- > >> >> > hw/loader.c | 7 +++++++ > >> >> > 1 files changed, 7 insertions(+), 0 deletions(-) > >> >> > > >> >> > diff --git a/hw/loader.c b/hw/loader.c > >> >> > index 79a6f95..35bc25a 100644 > >> >> > --- a/hw/loader.c > >> >> > +++ b/hw/loader.c > >> >> > @@ -507,6 +507,13 @@ int load_uimage(const char *filename, > >> >> > target_phys_addr_t *ep, > >> >> > > >> >> > ret = hdr->ih_size; > >> >> > > >> >> > + /* The kernel's BSS size is lost by mkimage, which only considers > >> >> > file > >> >> > + * size. We don't know how big it is, but we do know we can't > >> >> > place > >> >> > + * anything immediately after the kernel. The padding seems like > >> >> > it should > >> >> > + * be proportional to overall file size, but we also make sure > >> >> > it's at > >> >> > + * least 4-byte aligned. */ > >> >> > + ret += (hdr->ih_size / 16) & ~0x3; > >> >> > >> >> Maybe it's only me, but it feels a bit akward to push down this kind of > >> >> knowledge down the abstraction layers. Does it work for you to have your > >> >> caller of load_uimage apply whatever resizing magic needed for your > >> >> kernel > >> >> and arch? > >> > > >> > Ayway, IMO the conventions of where to pass blobs from the bootloader to > >> > the > >> > loaded image are an agreement between the bootloader and the loaded > >> > code. The > >> > formats or mechanisms to load the image should need to be involved that > >> > much. > >> > > >> > For example in this particular case, other archs (e.g, MicroBlaze) might > >> > not > >> > need any magic. The MicroBlaze linux kernel moves cmdline and device > >> > tree blobs > >> > into safe areas prior to .bss initialization. > >> > >> Are you claiming that's the common case? FWIW, PowerPC and ARM don't > >> seem to. I wouldn't expect such logic except in reaction to a specific > >> bug (i.e. a qemu/firmware loader bug). > > > > I'm not trying to claim it's the common case, but it exists. > > It exists and will remain unaffected by this patch, while the common > case will be improved. > > >> The load_uimage() interface claims to report the size of the kernel it > >> loaded. If you argue that it shouldn't try to do that (and indeed you > > > > The way I understand it, it reports the size of what got loaded. > > The difference between "what got loaded" and "the size of the loaded > file in memory" is a subtlety that is not at all clear from the code, > and that is precisely why I propose centralizing the logic to handle > it. > > > It would be very difficult for load_uimage to figure out what memory > > areas are beeing touched prior to .bss init (or the point where the passed > > blob is used). > > > >> could argue it's not *possible* to do that accurately), that logic > > > > Right, its very hard for it to guess what memory areas are safe. > > > >> should be completely removed. The current behavior is worse than not > >> knowing at all: callers *think* they know, but it's guaranteed to be > >> wrong. > >> > >> Of course, if you do want to remove the size, then callers are left > >> with even less information than they had before. In that case, you > > > > I think returning the size of the loaded image has a value, for example > > for archs that move away the blobs before touching any memory outside > > their image. Bootloaders for those archs can put some blobs right after > > the loaded image. > > You mean the one architecture, which by the way doesn't even use this > API? That doesn't seem like a strong argument to me. Anyways, it's
Are we looking at the same code? Grep for load_uimage in qemu. 4 archs use it, PPC, ARM, m68k and MB. Of those archs, only 2 actually use the return value of load_uimage to decide where to place blobs. PPC and MB. MB doesn't want any magic applied to the return value. That leaves us with _ONE_ single arch that needs magic that IMO is broken. You are trying to guess the size of the loaded image's .bss area by adding a 16th of the uimage size? > just as much work to relocate an initrd from a padded address as it is > from a closer address, so there is no downside. > > The fact remains that the current API is broken by design, or to be > charitable "violates the principle of least surprise." With the > exception of microblaze, everybody who calls load_uimage() must > understand the nuances of the return value and adjust it (or ignore > it) accordingly. Why wouldn't we consolidate that logic? I don't see how guessing that the .bss area is a 16th of the loaded image makes this call any less confusing. > >> tell me: where should I hardcode initrd loading? > > > > Not sure, but I'd guess somewhere close to where you are calling > > load_uimage from (it wasn't clear to me where that was). > > Sorry, let me rephrase. At what address should I hardcode my initrd? > What about my device tree? As a followup, why not lower, or higher? You should be putting them at the same addresses as uboot puts them. Cheers
