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

[Hollis Blanchard]

On Sun, Aug 1, 2010 at 5:36 AM, Edgar E. Iglesias
<address@hidden> 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 <address@hidden>
>> > ---
>> >  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).

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
could argue it's not *possible* to do that accurately), that logic
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
tell me: where should I hardcode initrd loading?

Anyways, you don't even use load_uimage() in microblaze, and if you
did, you wouldn't be obligated to use the "size" return value, so
fixing this issue for everybody else doesn't limit you at all.

-Hollis