Re: [Qemu-devel] [PATCH] loader: pad kernel size when loaded from a uImage

["Edgar E. Iglesias" <edgar.iglesias@gmail.com>]

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.iglesias@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 <hollis@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. BTW, qemu-arm
seems to follow a convention to place initrd 8Mb above RAM base, it
doesn't look at the loaded uimage size when deciding where to place
initrd.


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


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

Take a look at how arm does it in hw/arm_boot.c. CRIS doesn't have
uimage support now, but if it had It would probably do whatever magic
was needed in it's dedicated boot loader file, hw/cris-boot.c.
Microblaze has uimage support, look in hw/petalogix_s3adsp1800_mmu.c.

Maybe we should consider adding a ppc-boot.c with boot-loader magics?

Cheers,
Edgar