From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=51296 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Og0CH-0002MZ-CJ for qemu-devel@nongnu.org; Mon, 02 Aug 2010 14:57:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1Og0CG-0003Ix-0o for qemu-devel@nongnu.org; Mon, 02 Aug 2010 14:57:13 -0400 Received: from mail-ey0-f173.google.com ([209.85.215.173]:64747) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1Og0CF-0003Ih-OG for qemu-devel@nongnu.org; Mon, 02 Aug 2010 14:57:11 -0400 Received: by eyb6 with SMTP id 6so1727750eyb.4 for ; Mon, 02 Aug 2010 11:57:10 -0700 (PDT) Date: Mon, 2 Aug 2010 20:57:07 +0200 From: "Edgar E. Iglesias" Subject: Re: [Qemu-devel] [PATCH] loader: pad kernel size when loaded from a uImage Message-ID: <20100802185707.GA4527@laped.lan> References: <1280454504-31224-1-git-send-email-hollis@penguinppc.org> <1280454504-31224-2-git-send-email-hollis@penguinppc.org> <20100730225641.GA20899@laped.lan> <20100801123554.GA29468@laped.lan> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Hollis Blanchard Cc: qemu-devel@nongnu.org, kvm-ppc@vger.kernel.org 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 > 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 > >> > --- > >> >  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