2005-09-06 22:50:55

by Mark Bellon

[permalink] [raw]
Subject: [PATCH] PPC64: large INITRD causes kernel not to boot [UPDATE]

diff -Naur linux-2.6.13-orig/arch/ppc64/boot/main.c linux-2.6.13/arch/ppc64/boot/main.c
--- linux-2.6.13-orig/arch/ppc64/boot/main.c 2005-08-28 16:41:01.000000000 -0700
+++ linux-2.6.13/arch/ppc64/boot/main.c 2005-09-06 15:42:22.000000000 -0700
@@ -20,7 +20,7 @@
extern void printf(const char *fmt, ...);
extern int sprintf(char *buf, const char *fmt, ...);
void gunzip(void *, int, unsigned char *, int *);
-void *claim(unsigned int, unsigned int, unsigned int);
+void *claim(unsigned long, unsigned long, unsigned long);
void flush_cache(void *, unsigned long);
void pause(void);
extern void exit(void);
@@ -31,7 +31,8 @@

/* Value picked to match that used by yaboot */
#define PROG_START 0x01400000
-#define RAM_END (256<<20) // Fixme: use OF */
+#define RAM_END (512<<20) // Fixme: use OF */
+#define ONE_MB 0x100000

char *avail_ram;
char *begin_avail, *end_avail;
@@ -40,6 +41,7 @@
unsigned int heap_max;

extern char _start[];
+extern char _end[];
extern char _vmlinux_start[];
extern char _vmlinux_end[];
extern char _initrd_start[];
@@ -73,13 +75,13 @@

#undef DEBUG

-static unsigned long claim_base = PROG_START;
+static unsigned long claim_base;

static unsigned long try_claim(unsigned long size)
{
unsigned long addr = 0;

- for(; claim_base < RAM_END; claim_base += 0x100000) {
+ for(; claim_base < RAM_END; claim_base += ONE_MB) {
#ifdef DEBUG
printf(" trying: 0x%08lx\n\r", claim_base);
#endif
@@ -110,7 +112,26 @@
if (getprop(chosen_handle, "stdin", &stdin, sizeof(stdin)) != 4)
exit();

- printf("\n\rzImage starting: loaded at 0x%x\n\r", (unsigned)_start);
+ printf("\n\rzImage starting: loaded at 0x%lx\n\r", (unsigned long) _start);
+
+ /*
+ * The first available claim_base must be above the end of the
+ * the loaded kernel wrapper file (_start to _end includes the
+ * initrd image if it is present) and rounded up to a nice
+ * 1 MB boundary for good measure.
+ */
+
+ claim_base = _ALIGN_UP((unsigned long)_end, ONE_MB);
+
+#if defined(PROG_START)
+ /*
+ * Maintain a "magic" minimum address. This keeps some older
+ * firmware platforms running.
+ */
+
+ if (claim_base < PROG_START)
+ claim_base = PROG_START;
+#endif

/*
* Now we try to claim some memory for the kernel itself
@@ -120,7 +141,7 @@
* size... In practice we add 1Mb, that is enough, but we should really
* consider fixing the Makefile to put a _raw_ kernel in there !
*/
- vmlinux_memsize += 0x100000;
+ vmlinux_memsize += ONE_MB;
printf("Allocating 0x%lx bytes for kernel ...\n\r", vmlinux_memsize);
vmlinux.addr = try_claim(vmlinux_memsize);
if (vmlinux.addr == 0) {


Attachments:
initrd-patch (2.58 kB)

2005-09-06 23:42:19

by Paul Mackerras

[permalink] [raw]
Subject: Re: [PATCH] PPC64: large INITRD causes kernel not to boot [UPDATE]

Mark Bellon writes:

> Simply put the existing code has a fixed reservation (claim) address and
> once the kernel plus initrd image are large enough to pass this address
> all sorts of bad things occur. The fix is the dynamically establish the
> first claim address above the loaded kernel plus initrd (plus some
> "padding" and rounding). If PROG_START is defined this will be used as
> the minimum safe address - currently known to be 0x01400000 for the
> firmwares tested so far.

The idea is fine, but I have some questions about the actual patch:

> -void *claim(unsigned int, unsigned int, unsigned int);
> +void *claim(unsigned long, unsigned long, unsigned long);

What was the motivation for this change? Since the zImage wrapper is
a 32-bit executable, int and long are both 32 bits. I would prefer to
leave the parameters as unsigned int to force people to realize that
the parameters are 32 bits (even if said people have been working on
64-bit programs recently).

> + claim_base = _ALIGN_UP((unsigned long)_end, ONE_MB);
> +
> +#if defined(PROG_START)
> + /*
> + * Maintain a "magic" minimum address. This keeps some older
> + * firmware platforms running.
> + */
> +
> + if (claim_base < PROG_START)
> + claim_base = PROG_START;
> +#endif

This appears to be the meat of the patch, the rest is "cleanup",
right?

Paul.

2005-09-06 23:50:47

by Mark Bellon

[permalink] [raw]
Subject: Re: [PATCH] PPC64: large INITRD causes kernel not to boot [UPDATE]

Paul Mackerras wrote:

>Mark Bellon writes:
>
>
>
>>Simply put the existing code has a fixed reservation (claim) address and
>>once the kernel plus initrd image are large enough to pass this address
>>all sorts of bad things occur. The fix is the dynamically establish the
>>first claim address above the loaded kernel plus initrd (plus some
>>"padding" and rounding). If PROG_START is defined this will be used as
>>the minimum safe address - currently known to be 0x01400000 for the
>>firmwares tested so far.
>>
>>
>
>The idea is fine, but I have some questions about the actual patch:
>
>
>
>>-void *claim(unsigned int, unsigned int, unsigned int);
>>+void *claim(unsigned long, unsigned long, unsigned long);
>>
>>
>
>What was the motivation for this change? Since the zImage wrapper is
>a 32-bit executable, int and long are both 32 bits. I would prefer to
>leave the parameters as unsigned int to force people to realize that
>the parameters are 32 bits (even if said people have been working on
>64-bit programs recently).
>
>
>
The function, claim, is found in prom.c uses longs. The long is the
usual idiom for hiding a pointer, not an int, so I fixed accordingly.
I'm open to further discussion of course.

On a 64 bit machine long and int are different sizes. This would make
things "proper" if things changed in the future.

>>+ claim_base = _ALIGN_UP((unsigned long)_end, ONE_MB);
>>+
>>+#if defined(PROG_START)
>>+ /*
>>+ * Maintain a "magic" minimum address. This keeps some older
>>+ * firmware platforms running.
>>+ */
>>+
>>+ if (claim_base < PROG_START)
>>+ claim_base = PROG_START;
>>+#endif
>>
>>
>
>This appears to be the meat of the patch, the rest is "cleanup", right?
>
>

Correct. The preceding comment explains what is going on. Removing the
magic numbers seemed like a good idea.

mark

>Paul.
>
>