Re: [PATCH] x86/NUMA: make init_node_heap() respect Xen heap limit

[Ian Campbell <ian.campbell@citrix.com>]

On Fri, 2015-09-04 at 01:37 -0600, Jan Beulich wrote:
> > 
> > > > On 03.09.15 at 22:58, <julien.grall@citrix.com> wrote:
> > On 03/09/2015 21:01, Julien Grall wrote:
> > > On 27/08/2015 09:37, Jan Beulich wrote:
> > > > On NUMA systems, where we try to use node local memory for the 
> > > > basic
> > > > control structures of the buddy allocator, this special case needs 
> > > > to
> > > > take into consideration a possible address width limit placed on 
> > > > the
> > > > Xen heap. In turn this (but also other, more abstract 
> > > > considerations)
> > > > requires that xenheap_max_mfn() not be called more than once (at 
> > > > most
> > > > we might permit it to be called a second time with a larger value 
> > > > than
> > > > was passed the first time), and be called only before calling
> > > > end_boot_allocator().
> > > > 
> > > > While inspecting all the involved code, a couple of off-by-one 
> > > > issues
> > > > were found (and are being corrected here at once):
> > > > - arch_init_memory() cleared one too many page table slots
> > > > - the highmem_start based invocation of xenheap_max_mfn() passed 
> > > > too
> > > >     big a value
> > > > - xenheap_max_mfn() calculated the wrong bit count in edge cases
> > > > 
> > > > Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > > 
> > > This patch is breaking boot on aarch64 platform (particularly X
> > > -gene).
> > > 
> > > I think this should be considered as a block until I find a way to
> > > fix it.
> 
> Agreed.
> 
> > And found why! The last xenheap_bits changed from 39 to 38.
> > 
> > On x-gene the last max mfn used for the xenheap is 0x4400000, which the
> > new computation, it will give 38 bits which doesn't cover the entire
> > xenheap range.
> > 
> > I have wrote a patch to fix the issue, but I'm not sure that it's
> > the right things to do (see below).
> 
> No, this is wrong: xenheap_bits isn't meant to cover all RAM, it is
> meant to indicate how much (as an exact power of 2) of RAM is
> always accessible. I'm surprised anyway that ARM64 uses
> xenheap_max_mfn() (and even unconditionally); I thought all RAM
> is always accessible there. The invocation is off by one now in any
> case, but rather than correcting it that way the proper fix likely
> will involve more than just this simple an adjustment, as it looks
> like its use was wrong from the beginning (commit 5263507b1b).

What is the correct thing which arm64 should be doing, given that today all
RAM is indeed always mapped? Not call xenheap_max_mfn at all, therefore
leaving xenheap_bits == 0?

Or does it need to do something else different as well as dropping the use
of xenheap_max_mfn?

page_alloc.c's interface to the arch code is rather complex, especially
given the multiple modes it can be used in (some, but not all, of which are
apparent via CONFIG_SEPARATE_XENHEAP), reverse engineering the interface
which arch code is expected to abide by has always been something I've
struggled with.

Ian.