Re: [PATCH] [v3] powerpc/4xx: work around CHIP11 errata in a more

Re: [PATCH] [v3] powerpc/4xx: work around CHIP11 errata in a more

[David Gibson]

On Tue, Nov 11, 2008 at 06:06:46PM -0600, Hollis Blanchard wrote:
> The current CHIP11 errata truncates the device tree memory node, and subtracts
> (hardcoded) 4096 bytes. This breaks kernels with larger PAGE_SIZE, since the
> bootmem allocator assumes that total memory is a multiple of PAGE_SIZE.
> 
> Instead, use a device tree memory reservation to reserve only the 256 bytes
> actually affected by the errata, leaving the total memory size unaltered.
> 
> Signed-off-by: Hollis Blanchard <hollisb@us.ibm.com>

libfdt usage changes look fine to me.

Acked-by: David Gibson <david@gibson.dropbear.id.au>

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH] [v3] powerpc/4xx: work around CHIP11 errata in a more

[Benjamin Herrenschmidt]

On Tue, 2008-11-11 at 18:06 -0600, Hollis Blanchard wrote:
> The current CHIP11 errata truncates the device tree memory node, and subtracts
> (hardcoded) 4096 bytes. This breaks kernels with larger PAGE_SIZE, since the
> bootmem allocator assumes that total memory is a multiple of PAGE_SIZE.
> 
> Instead, use a device tree memory reservation to reserve only the 256 bytes
> actually affected by the errata, leaving the total memory size unaltered.
> 
> Signed-off-by: Hollis Blanchard <hollisb@us.ibm.com>

While I prefer this approach, won't it break kexec ?

I don't understand why we don't just have a bit of code in the kernel
itself that reserve that page on 44x at boot time and be done with it.

It's like we are trying to be too smart and over-engineer the solution.

Cheers,
Ben.

Re: [PATCH] [v3] powerpc/4xx: work around CHIP11 errata in a more

[Josh Boyer]

On Wed, 12 Nov 2008 15:37:43 +1100
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> On Tue, 2008-11-11 at 18:06 -0600, Hollis Blanchard wrote:
> > The current CHIP11 errata truncates the device tree memory node, and subtracts
> > (hardcoded) 4096 bytes. This breaks kernels with larger PAGE_SIZE, since the
> > bootmem allocator assumes that total memory is a multiple of PAGE_SIZE.
> > 
> > Instead, use a device tree memory reservation to reserve only the 256 bytes
> > actually affected by the errata, leaving the total memory size unaltered.
> > 
> > Signed-off-by: Hollis Blanchard <hollisb@us.ibm.com>
> 
> While I prefer this approach, won't it break kexec ?

Break it how?  Particularly given that kexec doesn't work on 4xx (yet).

> I don't understand why we don't just have a bit of code in the kernel
> itself that reserve that page on 44x at boot time and be done with it.
> 
> It's like we are trying to be too smart and over-engineer the solution.

I don't think that's it.  I think it's more that we're opportunistic and
the wrapper is the easiest place to do this, given that U-Boot itself
will be doing the reserve for platforms that don't require the
wrapper.

So we could do the fixup in-kernel, but how do you do that
deterministically given that U-Boot might have already done it?

josh

Re: [PATCH] [v3] powerpc/4xx: work around CHIP11 errata in a more

[Benjamin Herrenschmidt]

On Wed, 2008-11-12 at 06:31 -0500, Josh Boyer wrote:
> On Wed, 12 Nov 2008 15:37:43 +1100
> Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> 
> > On Tue, 2008-11-11 at 18:06 -0600, Hollis Blanchard wrote:
> > > The current CHIP11 errata truncates the device tree memory node, and subtracts
> > > (hardcoded) 4096 bytes. This breaks kernels with larger PAGE_SIZE, since the
> > > bootmem allocator assumes that total memory is a multiple of PAGE_SIZE.
> > > 
> > > Instead, use a device tree memory reservation to reserve only the 256 bytes
> > > actually affected by the errata, leaving the total memory size unaltered.
> > > 
> > > Signed-off-by: Hollis Blanchard <hollisb@us.ibm.com>
> > 
> > While I prefer this approach, won't it break kexec ?
> 
> Break it how?  Particularly given that kexec doesn't work on 4xx (yet).

Allright, wrong wording. It will make kexec more painful since it will
have to also create that reserved area in the target DT.

> I don't think that's it.  I think it's more that we're opportunistic and
> the wrapper is the easiest place to do this, given that U-Boot itself
> will be doing the reserve for platforms that don't require the
> wrapper.
> 
> So we could do the fixup in-kernel, but how do you do that
> deterministically given that U-Boot might have already done it?

Bah, do you know many RAM chip that will chop off the last 4K ?

I still find it a bit tricky to have memory nodes not aligned on nice
fat big boundaries tho.

Cheers,
Ben.

Re: [PATCH] [v3] powerpc/4xx: work around CHIP11 errata in a more

[Hollis Blanchard]

On Wed, 2008-11-12 at 22:52 +1100, Benjamin Herrenschmidt wrote:
> On Wed, 2008-11-12 at 06:31 -0500, Josh Boyer wrote:
> > On Wed, 12 Nov 2008 15:37:43 +1100
> > Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> > 
> > > On Tue, 2008-11-11 at 18:06 -0600, Hollis Blanchard wrote:
> > > > The current CHIP11 errata truncates the device tree memory node, and subtracts
> > > > (hardcoded) 4096 bytes. This breaks kernels with larger PAGE_SIZE, since the
> > > > bootmem allocator assumes that total memory is a multiple of PAGE_SIZE.
> > > > 
> > > > Instead, use a device tree memory reservation to reserve only the 256 bytes
> > > > actually affected by the errata, leaving the total memory size unaltered.
> > > > 
> > > > Signed-off-by: Hollis Blanchard <hollisb@us.ibm.com>
> > > 
> > > While I prefer this approach, won't it break kexec ?
> > 
> > Break it how?  Particularly given that kexec doesn't work on 4xx (yet).
> 
> Allright, wrong wording. It will make kexec more painful since it will
> have to also create that reserved area in the target DT.
> 
> > I don't think that's it.  I think it's more that we're opportunistic and
> > the wrapper is the easiest place to do this, given that U-Boot itself
> > will be doing the reserve for platforms that don't require the
> > wrapper.
> > 
> > So we could do the fixup in-kernel, but how do you do that
> > deterministically given that U-Boot might have already done it?
> 
> Bah, do you know many RAM chip that will chop off the last 4K ?

Forget pages. The errata is about the last 256 bytes of physical memory.

> I still find it a bit tricky to have memory nodes not aligned on nice
> fat big boundaries tho.

I don't know what you're referring to. The patch I sent doesn't touch
memory nodes, so they are indeed still aligned on nice fat big
boundaries.

I don't think this is overengineering at all. We can't touch the last
256 bytes, so we mark it reserved, and then we won't. Altering memory
nodes is far more complicated and error-prone.

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [PATCH] [v3] powerpc/4xx: work around CHIP11 errata in a more

[Benjamin Herrenschmidt]

On Wed, 2008-11-12 at 09:11 -0600, Hollis Blanchard wrote:
> Forget pages. The errata is about the last 256 bytes of physical
> memory.
> 
> > I still find it a bit tricky to have memory nodes not aligned on
> nice
> > fat big boundaries tho.
> 
> I don't know what you're referring to. The patch I sent doesn't touch
> memory nodes, so they are indeed still aligned on nice fat big
> boundaries.

My last comment was about the approach of modifying the memory node.

> I don't think this is overengineering at all. We can't touch the last
> 256 bytes, so we mark it reserved, and then we won't. Altering memory
> nodes is far more complicated and error-prone.

But your approach is going to be painful for kexec which will have to
duplicate that logic.

Again, why can't we just stick something in the kernel code that
reserves the last page ? It could be in prom.c or it could be called by
affected 4xx platforms by the platform code, whatever, but the reserve
map isn't really meant for that and will not be passed over from kernel
to kernel by kexec.

Ben.

Re: [PATCH] [v3] powerpc/4xx: work around CHIP11 errata in a more

[Josh Boyer]

On Thu, 13 Nov 2008 07:44:56 +1100
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> On Wed, 2008-11-12 at 09:11 -0600, Hollis Blanchard wrote:
> > Forget pages. The errata is about the last 256 bytes of physical
> > memory.
> > 
> > > I still find it a bit tricky to have memory nodes not aligned on
> > nice
> > > fat big boundaries tho.
> > 
> > I don't know what you're referring to. The patch I sent doesn't touch
> > memory nodes, so they are indeed still aligned on nice fat big
> > boundaries.
> 
> My last comment was about the approach of modifying the memory node.
> 
> > I don't think this is overengineering at all. We can't touch the last
> > 256 bytes, so we mark it reserved, and then we won't. Altering memory
> > nodes is far more complicated and error-prone.
> 
> But your approach is going to be painful for kexec which will have to
> duplicate that logic.
> 
> Again, why can't we just stick something in the kernel code that
> reserves the last page ? It could be in prom.c or it could be called by
> affected 4xx platforms by the platform code, whatever, but the reserve
> map isn't really meant for that and will not be passed over from kernel
> to kernel by kexec.

Again, because newer U-Boot is doing the fixup on memsize for us
already.  This is why it was done in the wrapper to begin with, since
it depends on the version of U-Boot that you happen to be using.

If you have a good idea on how to figure that out in-kernel, do the
fixup when needed, and not make people's eyes bleed, I'm all for it.

josh

Re: [PATCH] [v3] powerpc/4xx: work around CHIP11 errata in a more

[Hollis Blanchard]

On Thu, 2008-11-13 at 07:44 +1100, Benjamin Herrenschmidt wrote:
> 
> Again, why can't we just stick something in the kernel code that
> reserves the last page ? It could be in prom.c or it could be called by
> affected 4xx platforms by the platform code, whatever, but the reserve
> map isn't really meant for that and will not be passed over from kernel
> to kernel by kexec.

Reserving a page is overkill; only the last 256 bytes are affected. We
need to intercept at the LMB level, because allocations are already done
there, so by the time we hit bootmem it's way too late.

I simply don't see a good place to do this in the kernel. It would have
to be before the first lmb_alloc() call, which for safety would put it
inside early_init_devtree() -- along with the other lmb_reserve()
calls.[1]

However, ppc_md.probe() hasn't even been called yet, so there's no way
of knowing if we're on an affected system, unless you want to add a
special of_scan_flat_dt() call here.

I'm open to suggestions, but I don't see a better way than what I
already sent. I think the important part is to call lmb_add() for all
memory, but lmb_reserve() the last 256 bytes before lmb_alloc() happens.

It sounds like kexec must have some knowledge of the platform and device
tree already, so is this really a big deal? At any rate, this
conversation is somewhat academic, since there is no kexec on 44x... so
maybe this can be re-addressed when that becomes a real issue.

[1] This is exactly where flat device tree reservations are done, and
that's why the patch I submitted works.

-- 
Hollis Blanchard
IBM Linux Technology Center