Re: memory management issue (Re: another regression on Efika)

[Robert Millan <rmh@aybabtu.com>]

[-- Attachment #1: Type: text/plain, Size: 2160 bytes --]

On Sat, Jul 14, 2007 at 12:08:30AM +0200, Robert Millan wrote:
> On Tue, Jul 10, 2007 at 03:32:30PM -0500, Hollis Blanchard wrote:
> > > > In short, whatever is causing these crashes is not only an Efika issue.
> > > > I suspect Apple, or Apple powerbooks at least, are also affected. I got
> > > > Sjoerd to try this on his powerbook, and he confirms this fixes for him.
> > > > Paul also tried, with the same results. Not all the powerbooks are the
> > > > very same model, there are different model revisions involved.
> > > 
> > > The error Jordi was seeing before reverting your commits was very similar to
> > > what I get on my Efika (something like "claim failed").
> > > 
> > > Hollis, do you still think this is a firmware bug?
> > 
> > Could somebody please debug it and report back if it is really a GRUB
> > issue? It could be as simple as adjusting the link address, or it could
> > mean instrumenting grub_available_iterate() and checking that output
> > with the contents of /memory/available.
> 
> It turns out that grub_claim_heap() was asked to claim a region that starts
> after the heap limit.  The start address (0x19111e4) is correctly obtained
> from /memory/available.
> 
> Should heap limit be increased?

The attached patch fixes the problem for Efika, although I'm not 1:1 sure of
its correctness.

It seems to me that heaplimit is meant to be used as a relative offset, not
as an absolute memory address.  This makes the current code fail when the
start address in /memory/available is higher than heaplimit, and I don't
think there's anything that can grant us this won't happen.  From this POV,
it's clear that moving from relative to absolute address is wrong.

OTOH, your commit indicates (as per variable names, etc) that this change
was done intentionally; so I have to assume there would be a reason for that.
Please can you clarify?

Btw, the attached patch doesn't fix the problem for Jordi (Mac).  Maybe
we're talking about two separate bugs after all.

-- 
Robert Millan

My spam trap is honeypot@aybabtu.com.  Note: this address is only intended
for spam harvesters.  Writing to it will get you added to my black list.

[-- Attachment #2: memory_fix.diff --]
[-- Type: text/x-diff, Size: 1072 bytes --]

Index: kern/powerpc/ieee1275/init.c
===================================================================
RCS file: /sources/grub/grub2/kern/powerpc/ieee1275/init.c,v
retrieving revision 1.31
diff -u -r1.31 init.c
--- kern/powerpc/ieee1275/init.c	21 Jul 2007 23:32:27 -0000	1.31
+++ kern/powerpc/ieee1275/init.c	23 Jul 2007 19:49:44 -0000
@@ -115,14 +115,16 @@
 /* Claim some available memory in the first /memory node. */
 static void grub_claim_heap (unsigned long heaplimit)
 {
+  unsigned long total = 0;
+
   auto int heap_init (grub_uint64_t addr, grub_uint64_t len);
   int heap_init (grub_uint64_t addr, grub_uint64_t len)
   {
     len -= 1; /* Required for some firmware.  */
 
     /* Don't claim anything above `heaplimit'.  */
-    if (addr + len > heaplimit)
-      len = heaplimit - addr;
+    if (total + len > heaplimit)
+      len = heaplimit - total;
 
     if (len)
       {
@@ -134,6 +136,10 @@
 	grub_mm_init_region ((void *) (grub_addr_t) addr, len);
       }
 
+    total += len;
+    if (total >= heaplimit)
+      return 1;
+
     return 0;
   }