[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] loader: pad kernel size when loaded from a uIma
Edgar E. Iglesias
Re: [Qemu-devel] [PATCH] loader: pad kernel size when loaded from a uImage
Mon, 2 Aug 2010 20:57:07 +0200
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
> <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).
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
> 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
> 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?