Patchwork [1/2] powerpc: add 16K/64K pages support for the 44x PPC32 architectures.

login
register
mail settings
Submitter Hollis Blanchard
Date Oct. 31, 2008, 11:23 p.m.
Message ID <fb412d760810311623i2b0f93a3r5aa0eb297e154b4a@mail.gmail.com>
Download mbox | patch
Permalink /patch/6757/
State Superseded, archived
Headers show

Comments

Hollis Blanchard - Oct. 31, 2008, 11:23 p.m.
On Wed, Oct 22, 2008 at 9:28 AM, Christian Ehrhardt
<ehrhardt@linux.vnet.ibm.com> wrote:
> Hi Ilya,
> I just tried your patch on my 440 board because it would help us in our
> environment.
> Unfortunately I run into a bug on early boot (mark_bootmem).
>
> A log can be found in this mail, this is the bug when running with 64k page
> size.
> I tried this with and without your 2/2 265k patch and also with page size
> configured to 16k, the error is the same in all cases.
>
> I used an earlier version of your patch in the past and it worked fine.
> Applying this old patch causes the same problem.
> Therefore I expect that there was some other code changed that breaks with
> page size != 4k.

This patch seems to solve the problem for me, but I have to run and
haven't yet worked out if it's the right fix.


Looks like the breakage may have been accidentally introduced by
Johannes Weiner <hannes@saeurebad.de> on Jul 24 (post 2.6.26).

FWIW, the boards Christian and I are hitting the problem on are
Sequoias with 256MB of RAM. cuImage is reporting only 0xffff000 bytes
of RAM though, which may be exacerbating the situation.

-Hollis
Josh Boyer - Nov. 1, 2008, 11:30 a.m.
On Fri, Oct 31, 2008 at 06:23:28PM -0500, Hollis Blanchard wrote:
>On Wed, Oct 22, 2008 at 9:28 AM, Christian Ehrhardt
><ehrhardt@linux.vnet.ibm.com> wrote:
>> Hi Ilya,
>> I just tried your patch on my 440 board because it would help us in our
>> environment.
>> Unfortunately I run into a bug on early boot (mark_bootmem).
>>
>> A log can be found in this mail, this is the bug when running with 64k page
>> size.
>> I tried this with and without your 2/2 265k patch and also with page size
>> configured to 16k, the error is the same in all cases.
>>
>> I used an earlier version of your patch in the past and it worked fine.
>> Applying this old patch causes the same problem.
>> Therefore I expect that there was some other code changed that breaks with
>> page size != 4k.
>
>This patch seems to solve the problem for me, but I have to run and
>haven't yet worked out if it's the right fix.
>
>diff --git a/mm/bootmem.c b/mm/bootmem.c
>--- a/mm/bootmem.c
>+++ b/mm/bootmem.c
>@@ -300,7 +300,7 @@ static int __init mark_bootmem(unsigned
>                unsigned long max;
>
>                if (pos < bdata->node_min_pfn ||
>-                   pos >= bdata->node_low_pfn) {
>+                   pos > bdata->node_low_pfn) {
>                        BUG_ON(pos != start);
>                        continue;
>                }
>@@ -399,7 +399,7 @@ int __init reserve_bootmem(unsigned long
>        unsigned long start, end;
>
>        start = PFN_DOWN(addr);
>-       end = PFN_UP(addr + size);
>+       end = PFN_DOWN(addr + size);
>
>        return mark_bootmem(start, end, 1, flags);
> }
>
>Looks like the breakage may have been accidentally introduced by
>Johannes Weiner <hannes@saeurebad.de> on Jul 24 (post 2.6.26).
>
>FWIW, the boards Christian and I are hitting the problem on are
>Sequoias with 256MB of RAM. cuImage is reporting only 0xffff000 bytes
>of RAM though, which may be exacerbating the situation.

That is on purpose.  The chip has an errata that causes badness if
you use the last XX bytes of DRAM.  I forget exactly what XX is, but
we just remove the last page.

josh
Benjamin Herrenschmidt - Nov. 1, 2008, 9:55 p.m.
On Sat, 2008-11-01 at 07:30 -0400, Josh Boyer wrote:
> 
> That is on purpose.  The chip has an errata that causes badness if
> you use the last XX bytes of DRAM.  I forget exactly what XX is, but
> we just remove the last page.

Doing that from the device-tree is very hairy tho... you end up with
informations in there that aren't aligned etc... oh well.

Ben.
Josh Boyer - Nov. 2, 2008, 1:41 p.m.
On Sun, Nov 02, 2008 at 08:55:02AM +1100, Benjamin Herrenschmidt wrote:
>On Sat, 2008-11-01 at 07:30 -0400, Josh Boyer wrote:
>> 
>> That is on purpose.  The chip has an errata that causes badness if
>> you use the last XX bytes of DRAM.  I forget exactly what XX is, but
>> we just remove the last page.
>
>Doing that from the device-tree is very hairy tho... you end up with
>informations in there that aren't aligned etc... oh well.

What?  -ENOTVERBOSEENOUGH.

I don't see how this is really different from U-Boot just passing in
a smaller memory size in the old arch/ppc world.  (And I think U-Boot
will actually fixup the device tree in a similar manner itself these
days.)  So if there are problems with this, please do tell.

josh
Benjamin Herrenschmidt - Nov. 2, 2008, 9:33 p.m.
On Sun, 2008-11-02 at 08:41 -0500, Josh Boyer wrote:
> On Sun, Nov 02, 2008 at 08:55:02AM +1100, Benjamin Herrenschmidt wrote:
> >On Sat, 2008-11-01 at 07:30 -0400, Josh Boyer wrote:
> >> 
> >> That is on purpose.  The chip has an errata that causes badness if
> >> you use the last XX bytes of DRAM.  I forget exactly what XX is, but
> >> we just remove the last page.
> >
> >Doing that from the device-tree is very hairy tho... you end up with
> >informations in there that aren't aligned etc... oh well.
> 
> What?  -ENOTVERBOSEENOUGH.
> 
> I don't see how this is really different from U-Boot just passing in
> a smaller memory size in the old arch/ppc world.  (And I think U-Boot
> will actually fixup the device tree in a similar manner itself these
> days.)  So if there are problems with this, please do tell.

Is it cropping the memory nodes or using the reserve map ?

Ben.
Josh Boyer - Nov. 3, 2008, 12:33 a.m.
On Mon, 03 Nov 2008 08:33:16 +1100
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> On Sun, 2008-11-02 at 08:41 -0500, Josh Boyer wrote:
> > On Sun, Nov 02, 2008 at 08:55:02AM +1100, Benjamin Herrenschmidt wrote:
> > >On Sat, 2008-11-01 at 07:30 -0400, Josh Boyer wrote:
> > >> 
> > >> That is on purpose.  The chip has an errata that causes badness if
> > >> you use the last XX bytes of DRAM.  I forget exactly what XX is, but
> > >> we just remove the last page.
> > >
> > >Doing that from the device-tree is very hairy tho... you end up with
> > >informations in there that aren't aligned etc... oh well.
> > 
> > What?  -ENOTVERBOSEENOUGH.
> > 
> > I don't see how this is really different from U-Boot just passing in
> > a smaller memory size in the old arch/ppc world.  (And I think U-Boot
> > will actually fixup the device tree in a similar manner itself these
> > days.)  So if there are problems with this, please do tell.
> 
> Is it cropping the memory nodes or using the reserve map ?

Cropping the size of the memory node.  That was simplest to do from the
cuboot wrapper at the time.  If marking it reserved via a reserve map
is more elegant and correct, we could do that.

But I will still like to know what about the other way is hairy please.

josh
Benjamin Herrenschmidt - Nov. 3, 2008, 12:43 a.m.
> Cropping the size of the memory node.  That was simplest to do from the
> cuboot wrapper at the time.  If marking it reserved via a reserve map
> is more elegant and correct, we could do that.
> 
> But I will still like to know what about the other way is hairy please.

I don't like it :-) Bad feeling ... don't like having a memory
node entry that isn't aligned to some large power of two typically.

Cheers,
Ben.
Josh Boyer - Nov. 3, 2008, 11:26 a.m.
On Mon, 03 Nov 2008 11:43:54 +1100
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> 
> > Cropping the size of the memory node.  That was simplest to do from the
> > cuboot wrapper at the time.  If marking it reserved via a reserve map
> > is more elegant and correct, we could do that.
> > 
> > But I will still like to know what about the other way is hairy please.
> 
> I don't like it :-) Bad feeling ... don't like having a memory
> node entry that isn't aligned to some large power of two typically.

Erm, ok.  And does your heebie-geebies extend to people using the mem=
parameter in a similar fashion?

josh
Benjamin Herrenschmidt - Nov. 3, 2008, 8:17 p.m.
On Mon, 2008-11-03 at 06:26 -0500, Josh Boyer wrote:
> On Mon, 03 Nov 2008 11:43:54 +1100
> Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> 
> > 
> > > Cropping the size of the memory node.  That was simplest to do from the
> > > cuboot wrapper at the time.  If marking it reserved via a reserve map
> > > is more elegant and correct, we could do that.
> > > 
> > > But I will still like to know what about the other way is hairy please.
> > 
> > I don't like it :-) Bad feeling ... don't like having a memory
> > node entry that isn't aligned to some large power of two typically.
> 
> Erm, ok.  And does your heebie-geebies extend to people using the mem=
> parameter in a similar fashion?

Nah, not really. It's not that it won't work, I suppose it does, though
I would have preferred a way to "reserve" that memory rather than take
it off. In fact, that last page could be used for other things, for
example it could be used as a dummy page to point stale DMA to or
whatever else.

Ben.
Josh Boyer - Nov. 11, 2008, 1:19 p.m.
On Fri, Oct 31, 2008 at 06:23:28PM -0500, Hollis Blanchard wrote:
>On Wed, Oct 22, 2008 at 9:28 AM, Christian Ehrhardt
><ehrhardt@linux.vnet.ibm.com> wrote:
>> Hi Ilya,
>> I just tried your patch on my 440 board because it would help us in our
>> environment.
>> Unfortunately I run into a bug on early boot (mark_bootmem).
>>
>> A log can be found in this mail, this is the bug when running with 64k page
>> size.
>> I tried this with and without your 2/2 265k patch and also with page size
>> configured to 16k, the error is the same in all cases.
>>
>> I used an earlier version of your patch in the past and it worked fine.
>> Applying this old patch causes the same problem.
>> Therefore I expect that there was some other code changed that breaks with
>> page size != 4k.
>
>This patch seems to solve the problem for me, but I have to run and
>haven't yet worked out if it's the right fix.
>
>diff --git a/mm/bootmem.c b/mm/bootmem.c
>--- a/mm/bootmem.c
>+++ b/mm/bootmem.c
>@@ -300,7 +300,7 @@ static int __init mark_bootmem(unsigned
>                unsigned long max;
>
>                if (pos < bdata->node_min_pfn ||
>-                   pos >= bdata->node_low_pfn) {
>+                   pos > bdata->node_low_pfn) {
>                        BUG_ON(pos != start);
>                        continue;
>                }
>@@ -399,7 +399,7 @@ int __init reserve_bootmem(unsigned long
>        unsigned long start, end;
>
>        start = PFN_DOWN(addr);
>-       end = PFN_UP(addr + size);
>+       end = PFN_DOWN(addr + size);
>
>        return mark_bootmem(start, end, 1, flags);
> }


Hollis,  if I'm understanding things correctly this patch is no
longer needed if we do the memory reserve in the boot wrapper for
the errata.  Correct?

josh
Hollis Blanchard - Nov. 11, 2008, 3 p.m.
On Tue, 2008-11-11 at 08:19 -0500, Josh Boyer wrote:
> On Fri, Oct 31, 2008 at 06:23:28PM -0500, Hollis Blanchard wrote:
> >On Wed, Oct 22, 2008 at 9:28 AM, Christian Ehrhardt
> ><ehrhardt@linux.vnet.ibm.com> wrote:
> >> Hi Ilya,
> >> I just tried your patch on my 440 board because it would help us in our
> >> environment.
> >> Unfortunately I run into a bug on early boot (mark_bootmem).
> >>
> >> A log can be found in this mail, this is the bug when running with 64k page
> >> size.
> >> I tried this with and without your 2/2 265k patch and also with page size
> >> configured to 16k, the error is the same in all cases.
> >>
> >> I used an earlier version of your patch in the past and it worked fine.
> >> Applying this old patch causes the same problem.
> >> Therefore I expect that there was some other code changed that breaks with
> >> page size != 4k.
> >
> >This patch seems to solve the problem for me, but I have to run and
> >haven't yet worked out if it's the right fix.
> >
> >diff --git a/mm/bootmem.c b/mm/bootmem.c
> >--- a/mm/bootmem.c
> >+++ b/mm/bootmem.c
> >@@ -300,7 +300,7 @@ static int __init mark_bootmem(unsigned
> >                unsigned long max;
> >
> >                if (pos < bdata->node_min_pfn ||
> >-                   pos >= bdata->node_low_pfn) {
> >+                   pos > bdata->node_low_pfn) {
> >                        BUG_ON(pos != start);
> >                        continue;
> >                }
> >@@ -399,7 +399,7 @@ int __init reserve_bootmem(unsigned long
> >        unsigned long start, end;
> >
> >        start = PFN_DOWN(addr);
> >-       end = PFN_UP(addr + size);
> >+       end = PFN_DOWN(addr + size);
> >
> >        return mark_bootmem(start, end, 1, flags);
> > }
> 
> 
> Hollis,  if I'm understanding things correctly this patch is no
> longer needed if we do the memory reserve in the boot wrapper for
> the errata.  Correct?

Correct.

Patch

diff --git a/mm/bootmem.c b/mm/bootmem.c
--- a/mm/bootmem.c
+++ b/mm/bootmem.c
@@ -300,7 +300,7 @@  static int __init mark_bootmem(unsigned
                unsigned long max;

                if (pos < bdata->node_min_pfn ||
-                   pos >= bdata->node_low_pfn) {
+                   pos > bdata->node_low_pfn) {
                        BUG_ON(pos != start);
                        continue;
                }
@@ -399,7 +399,7 @@  int __init reserve_bootmem(unsigned long
        unsigned long start, end;

        start = PFN_DOWN(addr);
-       end = PFN_UP(addr + size);
+       end = PFN_DOWN(addr + size);

        return mark_bootmem(start, end, 1, flags);
 }