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 12:33:54PM -0700, Hollis Blanchard wrote:
> On Mon, Aug 2, 2010 at 11:57 AM, Edgar E. Iglesias
> <edgar.iglesias@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.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.
> 
> 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