[PATCH] Gunzip call fix for PPC kernel images >4MB

[PATCH] Gunzip call fix for PPC kernel images >4MB

[Benjamin Heyne]

If Kernel images for PPC grow >4MB inflating of the kernel fails.
Increasing the link/load address doesn't help. Problem is 
solved by replacing the fixed address of the gunzip() call in
misc-embedded.c with CONFIG_BOOT_LOAD.

Signed-off-by: Benjamin Heyne <benjamin.heyne@uni-dortmund.de>
---

--- arch/ppc/boot/simple/misc-embedded.c.orig 2006-08-16 10:56:10.000000000 +0200 
+++ arch/ppc/boot/simple/misc-embedded.c 2006-08-16 10:56:55.000000000 +0200 
@@ -213,7 +213,7 @@ load_kernel(unsigned long load_addr, int 
	*cp = 0;
 	puts("\nUncompressing Linux...");
 
-	gunzip(0, 0x400000, zimage_start, &zimage_size);
+	gunzip(0, CONFIG_BOOT_LOAD, zimage_start, &zimage_size);
 	flush_instruction_cache();
 	puts("done.\n");
 	{

-- 
Benjamin Heyne
=======================================================
 Arbeitsgebiet Datentechnik, Universitaet Dortmund
 Information Processing Lab, University of Dortmund

 Otto-Hahn-Strasse 4, 44227 Dortmund, Germany
 Fon: +49 231 755 7017, Fax: +49 231 755 7019
 www.dt.e-technik.uni-dortmund.de, Raum/Room P1-04-217

Re: [PATCH] Gunzip call fix for PPC kernel images >4MB

[Tom Rini]

On Wed, Aug 16, 2006 at 11:31:37AM +0200, Benjamin Heyne wrote:

> If Kernel images for PPC grow >4MB inflating of the kernel fails.
> Increasing the link/load address doesn't help. Problem is 
> solved by replacing the fixed address of the gunzip() call in
> misc-embedded.c with CONFIG_BOOT_LOAD.

I've thought about this a bit and I think the general concept is OK, but
this works with a bit of luck I think.

The arch/ppc/boot code makes some assumptions such as that 4MB-8MB (except
on 40x which is end of the wrapper code to 8MB, and BOOT_LOAD defaults
to 4MB here) is free to use for malloc()'ing for the inflate routine.

It also assumes that the vmlinux.gz will live at BOOT_LOAD + a bit, when
the wrapper is set to kick off linux.

Finally, in arch/ppc/boot/ (for both OF and !OF cases) gunzip looks
like:
void gunzip(void *dst, int dstlen, unsigned char *src, int *lenp)

So I see 2 problems here.

First, there are other calls to gunzip with dstlen hard-coded to 4MB, so
the problem would still exist.

Second, and more complexly we want to make sure that we don't tell
gunzip that the destination size grows into the area it would also be
using to malloc buffers, unless someone can (and please do if possible)
explain that it's really OK to uncompress into our zalloc space (there
is no zfree so maybe it's going to be OK, but I don't know that the
algorithm wouldn't try and reuse buffers which would be bad).

Thanks!

-- 
Tom Rini

Re: [PATCH] Gunzip call fix for PPC kernel images >4MB

[Dan Malek]

On Aug 17, 2006, at 1:04 AM, Tom Rini wrote:


> The arch/ppc/boot code makes some assumptions such as that 4MB-8MB  
> (except
> on 40x which is end of the wrapper code to 8MB, and BOOT_LOAD defaults
> to 4MB here) is free to use for malloc()'ing for the inflate routine.
>
> It also assumes that the vmlinux.gz will live at BOOT_LOAD + a bit,  
> when
> the wrapper is set to kick off linux.

First, why do you have a 4 MB kernel?

Second, another bit of luck is that most processors rely on
the boot information passed to be in the lower 8M of memory.
There are many comments about this in the initialization code,
I don't know how many processors actually rely on this.  So,
if you allow large kernels to be loaded into memory, there should
be some test to ensure we still have space for these boot
parameters, however they are passed.

Thanks.


	-- Dan

Re: [PATCH] Gunzip call fix for PPC kernel images >4MB

[Tom Rini]

On Thu, Aug 17, 2006 at 12:09:56PM -0400, Dan Malek wrote:
> 
> On Aug 17, 2006, at 1:04 AM, Tom Rini wrote:
> 
> 
> >The arch/ppc/boot code makes some assumptions such as that 4MB-8MB  
> >(except
> >on 40x which is end of the wrapper code to 8MB, and BOOT_LOAD defaults
> >to 4MB here) is free to use for malloc()'ing for the inflate routine.
> >
> >It also assumes that the vmlinux.gz will live at BOOT_LOAD + a bit,  
> >when
> >the wrapper is set to kick off linux.
> 
> First, why do you have a 4 MB kernel?
> 
> Second, another bit of luck is that most processors rely on
> the boot information passed to be in the lower 8M of memory.
> There are many comments about this in the initialization code,
> I don't know how many processors actually rely on this.  So,
> if you allow large kernels to be loaded into memory, there should
> be some test to ensure we still have space for these boot
> parameters, however they are passed.

I think this second case is handled as I believe we end up relocating
this information to a 'safer' spot as if it's too low it may be eaten by
the kernel BSS.  But this is indeed something else to be sure will be OK
before we change this code.

-- 
Tom Rini

Re: [PATCH] Gunzip call fix for PPC kernel images >4MB

[Benjamin Heyne]

And so it came to pass, that on Wed, 16 Aug 2006 22:04:08 -0700
Tom Rini <trini@kernel.crashing.org> wrote as thus:

> On Wed, Aug 16, 2006 at 11:31:37AM +0200, Benjamin Heyne wrote:
> 
> > If Kernel images for PPC grow >4MB inflating of the kernel fails.
> > Increasing the link/load address doesn't help. Problem is 
> > solved by replacing the fixed address of the gunzip() call in
> > misc-embedded.c with CONFIG_BOOT_LOAD.
> 
> The arch/ppc/boot code makes some assumptions such as that 4MB-8MB (except
> on 40x which is end of the wrapper code to 8MB, and BOOT_LOAD defaults
> to 4MB here) is free to use for malloc()'ing for the inflate routine.

Well, I just could find this assumptions hardcoded in misc - Not in misc-embedded.
But I am relatively new to kernel programming, so maybe I have missed something.

But despite of that: Shouldn't it be possible to shift this range without
having to change fixed addresses at several places?

> 
> It also assumes that the vmlinux.gz will live at BOOT_LOAD + a bit, when
> the wrapper is set to kick off linux.

As I understand it, the lowest address at which reasonable data starts is
BOOT_LOAD. The space below is used for inflating, right? At least this
seems to be the case for my target (Virtex, ML300) - Do other targets
behave differently?

> 
> Finally, in arch/ppc/boot/ (for both OF and !OF cases) gunzip looks
> like:
> void gunzip(void *dst, int dstlen, unsigned char *src, int *lenp)

Ok - The dstlen parameter just tells zlib_inflate() something about the
available space. If the inflated image grows larger, an error occurs.
Therefore, BOOT_LOAD should be a reasonable border because board data
starts beyond - not before.

> 
> So I see 2 problems here.
> 
> First, there are other calls to gunzip with dstlen hard-coded to 4MB, so
> the problem would still exist.

Well, the other one exists in misc.c - But this one doesn't apply to the
embedded target, right?

> 
> Second, and more complexly we want to make sure that we don't tell
> gunzip that the destination size grows into the area it would also be
> using to malloc buffers, unless someone can (and please do if possible)
> explain that it's really OK to uncompress into our zalloc space (there
> is no zfree so maybe it's going to be OK, but I don't know that the
> algorithm wouldn't try and reuse buffers which would be bad).
> 

Well, as I said I am new to kernel programming - So I don't
know exactly where the malloc space starts. I just assumed it would be 
beyond BOOT_START because it seemed to be reasonable (If you have a kernel
which inflated size is exactly 4MB, inflation should also work...does it?).

Probably I missed something - But I don't really see the problem. Would you
please clarify it a little bit?

Best regards
-- 
Benjamin Heyne

Re: [PATCH] Gunzip call fix for PPC kernel images >4MB

[Tom Rini]

Sorry about the late reply, this got burried in my inbox.

On Fri, Aug 18, 2006 at 10:10:24AM +0200, Benjamin Heyne wrote:
> And so it came to pass, that on Wed, 16 Aug 2006 22:04:08 -0700
> Tom Rini <trini@kernel.crashing.org> wrote as thus:
> 
> > On Wed, Aug 16, 2006 at 11:31:37AM +0200, Benjamin Heyne wrote:
> > 
> > > If Kernel images for PPC grow >4MB inflating of the kernel fails.
> > > Increasing the link/load address doesn't help. Problem is 
> > > solved by replacing the fixed address of the gunzip() call in
> > > misc-embedded.c with CONFIG_BOOT_LOAD.
> > 
> > The arch/ppc/boot code makes some assumptions such as that 4MB-8MB (except
> > on 40x which is end of the wrapper code to 8MB, and BOOT_LOAD defaults
> > to 4MB here) is free to use for malloc()'ing for the inflate routine.
> 
> Well, I just could find this assumptions hardcoded in misc - Not in misc-embedded.
> But I am relatively new to kernel programming, so maybe I have missed something.
> 
> But despite of that: Shouldn't it be possible to shift this range without
> having to change fixed addresses at several places?

Maybe, just maybe.  The code is quite a bit complex in parts and I'd
really rather not try and make changes here as it's also on its way out.

> > It also assumes that the vmlinux.gz will live at BOOT_LOAD + a bit, when
> > the wrapper is set to kick off linux.
> 
> As I understand it, the lowest address at which reasonable data starts is
> BOOT_LOAD. The space below is used for inflating, right? At least this
> seems to be the case for my target (Virtex, ML300) - Do other targets
> behave differently?

At various points in the cycle, all targets make assumptions about what
can and can't live where.  Relocation is one of the big issues here as
we have to accept and work when we're loaded above, or below, BOOT_LOAD
initially and move ourself and anything useful we've been passed in,
around.

> > Finally, in arch/ppc/boot/ (for both OF and !OF cases) gunzip looks
> > like:
> > void gunzip(void *dst, int dstlen, unsigned char *src, int *lenp)
> 
> Ok - The dstlen parameter just tells zlib_inflate() something about the
> available space. If the inflated image grows larger, an error occurs.
> Therefore, BOOT_LOAD should be a reasonable border because board data
> starts beyond - not before.

Yes, I'm saying it's possible that some of that range is not available
space, for uncompressing as it's used for buffers in zlib_inflate.

> > So I see 2 problems here.
> > 
> > First, there are other calls to gunzip with dstlen hard-coded to 4MB, so
> > the problem would still exist.
> 
> Well, the other one exists in misc.c - But this one doesn't apply to the
> embedded target, right?

You can't really change one without changing the rest as there's similar
issues in all.

> > Second, and more complexly we want to make sure that we don't tell
> > gunzip that the destination size grows into the area it would also be
> > using to malloc buffers, unless someone can (and please do if possible)
> > explain that it's really OK to uncompress into our zalloc space (there
> > is no zfree so maybe it's going to be OK, but I don't know that the
> > algorithm wouldn't try and reuse buffers which would be bad).
> 
> Well, as I said I am new to kernel programming - So I don't
> know exactly where the malloc space starts. I just assumed it would be 
> beyond BOOT_START because it seemed to be reasonable (If you have a kernel
> which inflated size is exactly 4MB, inflation should also work...does it?).

I'm sorry to say you didn't pick the easiest spot to cut your teeth on,
so to speak.  The malloc space always beyond BOOT_LOAD because we don't
know how high we can go.  We know that from 0 to BOOT_LOAD is valid (or
we would have failed by now).  We have to assume that BOOT_LOAD to the
end ourself is also valid, or we'd have blown up (and hopefully in a way
that the user would figure out they need to tweak BOOT_LOAD or make the
image smaller).  But beyond that we don't always have a way to know how
much memory we have.

-- 
Tom Rini