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

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

[Jan Beulich]

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

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>

--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -369,7 +369,7 @@ void __init arch_init_memory(void)
 
                     for ( i = 0; i < l3_table_offset(split_va); ++i )
                         l3tab[i] = l3idle[i];
-                    for ( ; i <= L3_PAGETABLE_ENTRIES; ++i )
+                    for ( ; i < L3_PAGETABLE_ENTRIES; ++i )
                         l3tab[i] = l3e_empty();
                     split_l4e = l4e_from_pfn(virt_to_mfn(l3tab),
                                              __PAGE_HYPERVISOR);
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1002,7 +1002,7 @@ void __init noreturn __start_xen(unsigne
 
     setup_max_pdx(raw_max_page);
     if ( highmem_start )
-        xenheap_max_mfn(PFN_DOWN(highmem_start));
+        xenheap_max_mfn(PFN_DOWN(highmem_start - 1));
 
     /*
      * Walk every RAM region and map it in its entirety (on x86/64, at least)
@@ -1183,9 +1183,6 @@ void __init noreturn __start_xen(unsigne
 
     numa_initmem_init(0, raw_max_page);
 
-    end_boot_allocator();
-    system_state = SYS_STATE_boot;
-
     if ( max_page - 1 > virt_to_mfn(HYPERVISOR_VIRT_END - 1) )
     {
         unsigned long limit = virt_to_mfn(HYPERVISOR_VIRT_END - 1);
@@ -1194,6 +1191,8 @@ void __init noreturn __start_xen(unsigne
         if ( !highmem_start )
             xenheap_max_mfn(limit);
 
+        end_boot_allocator();
+
         /* Pass the remaining memory to the allocator. */
         for ( i = 0; i < boot_e820.nr_map; i++ )
         {
@@ -1217,6 +1216,10 @@ void __init noreturn __start_xen(unsigne
            opt_tmem = 0;
         }
     }
+    else
+        end_boot_allocator();
+
+    system_state = SYS_STATE_boot;
 
     vm_init();
     console_init_ring();
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -404,13 +404,19 @@ void get_outstanding_claims(uint64_t *fr
     spin_unlock(&heap_lock);
 }
 
+static bool_t __read_mostly first_node_initialised;
+#ifndef CONFIG_SEPARATE_XENHEAP
+static unsigned int __read_mostly xenheap_bits;
+#else
+#define xenheap_bits 0
+#endif
+
 static unsigned long init_node_heap(int node, unsigned long mfn,
                                     unsigned long nr, bool_t *use_tail)
 {
     /* First node to be discovered has its heap metadata statically alloced. */
     static heap_by_zone_and_order_t _heap_static;
     static unsigned long avail_static[NR_ZONES];
-    static int first_node_initialised;
     unsigned long needed = (sizeof(**_heap) +
                             sizeof(**avail) * NR_ZONES +
                             PAGE_SIZE - 1) >> PAGE_SHIFT;
@@ -428,14 +434,18 @@ static unsigned long init_node_heap(int 
     }
 #ifdef DIRECTMAP_VIRT_END
     else if ( *use_tail && nr >= needed &&
-              (mfn + nr) <= (virt_to_mfn(eva - 1) + 1) )
+              (mfn + nr) <= (virt_to_mfn(eva - 1) + 1) &&
+              (!xenheap_bits ||
+               !((mfn + nr - 1) >> (xenheap_bits - PAGE_SHIFT))) )
     {
         _heap[node] = mfn_to_virt(mfn + nr - needed);
         avail[node] = mfn_to_virt(mfn + nr - 1) +
                       PAGE_SIZE - sizeof(**avail) * NR_ZONES;
     }
     else if ( nr >= needed &&
-              (mfn + needed) <= (virt_to_mfn(eva - 1) + 1) )
+              (mfn + needed) <= (virt_to_mfn(eva - 1) + 1) &&
+              (!xenheap_bits ||
+               !((mfn + needed - 1) >> (xenheap_bits - PAGE_SHIFT))) )
     {
         _heap[node] = mfn_to_virt(mfn);
         avail[node] = mfn_to_virt(mfn + needed - 1) +
@@ -1543,11 +1553,13 @@ void free_xenheap_pages(void *v, unsigne
 
 #else
 
-static unsigned int __read_mostly xenheap_bits;
-
 void __init xenheap_max_mfn(unsigned long mfn)
 {
-    xenheap_bits = flsl(mfn) + PAGE_SHIFT;
+    ASSERT(!first_node_initialised);
+    ASSERT(!xenheap_bits);
+    BUILD_BUG_ON(PADDR_BITS >= BITS_PER_LONG);
+    xenheap_bits = min(flsl(mfn + 1) - 1 + PAGE_SHIFT, PADDR_BITS);
+    printk(XENLOG_INFO "Xen heap: %u bits\n", xenheap_bits);
 }
 
 void init_xenheap_pages(paddr_t ps, paddr_t pe)



[-- Attachment #2: x86-highmem-start-adjust.patch --]
[-- Type: text/plain, Size: 5090 bytes --]

x86/NUMA: make init_node_heap() respect Xen heap limit

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>

--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -369,7 +369,7 @@ void __init arch_init_memory(void)
 
                     for ( i = 0; i < l3_table_offset(split_va); ++i )
                         l3tab[i] = l3idle[i];
-                    for ( ; i <= L3_PAGETABLE_ENTRIES; ++i )
+                    for ( ; i < L3_PAGETABLE_ENTRIES; ++i )
                         l3tab[i] = l3e_empty();
                     split_l4e = l4e_from_pfn(virt_to_mfn(l3tab),
                                              __PAGE_HYPERVISOR);
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1002,7 +1002,7 @@ void __init noreturn __start_xen(unsigne
 
     setup_max_pdx(raw_max_page);
     if ( highmem_start )
-        xenheap_max_mfn(PFN_DOWN(highmem_start));
+        xenheap_max_mfn(PFN_DOWN(highmem_start - 1));
 
     /*
      * Walk every RAM region and map it in its entirety (on x86/64, at least)
@@ -1183,9 +1183,6 @@ void __init noreturn __start_xen(unsigne
 
     numa_initmem_init(0, raw_max_page);
 
-    end_boot_allocator();
-    system_state = SYS_STATE_boot;
-
     if ( max_page - 1 > virt_to_mfn(HYPERVISOR_VIRT_END - 1) )
     {
         unsigned long limit = virt_to_mfn(HYPERVISOR_VIRT_END - 1);
@@ -1194,6 +1191,8 @@ void __init noreturn __start_xen(unsigne
         if ( !highmem_start )
             xenheap_max_mfn(limit);
 
+        end_boot_allocator();
+
         /* Pass the remaining memory to the allocator. */
         for ( i = 0; i < boot_e820.nr_map; i++ )
         {
@@ -1217,6 +1216,10 @@ void __init noreturn __start_xen(unsigne
            opt_tmem = 0;
         }
     }
+    else
+        end_boot_allocator();
+
+    system_state = SYS_STATE_boot;
 
     vm_init();
     console_init_ring();
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -404,13 +404,19 @@ void get_outstanding_claims(uint64_t *fr
     spin_unlock(&heap_lock);
 }
 
+static bool_t __read_mostly first_node_initialised;
+#ifndef CONFIG_SEPARATE_XENHEAP
+static unsigned int __read_mostly xenheap_bits;
+#else
+#define xenheap_bits 0
+#endif
+
 static unsigned long init_node_heap(int node, unsigned long mfn,
                                     unsigned long nr, bool_t *use_tail)
 {
     /* First node to be discovered has its heap metadata statically alloced. */
     static heap_by_zone_and_order_t _heap_static;
     static unsigned long avail_static[NR_ZONES];
-    static int first_node_initialised;
     unsigned long needed = (sizeof(**_heap) +
                             sizeof(**avail) * NR_ZONES +
                             PAGE_SIZE - 1) >> PAGE_SHIFT;
@@ -428,14 +434,18 @@ static unsigned long init_node_heap(int 
     }
 #ifdef DIRECTMAP_VIRT_END
     else if ( *use_tail && nr >= needed &&
-              (mfn + nr) <= (virt_to_mfn(eva - 1) + 1) )
+              (mfn + nr) <= (virt_to_mfn(eva - 1) + 1) &&
+              (!xenheap_bits ||
+               !((mfn + nr - 1) >> (xenheap_bits - PAGE_SHIFT))) )
     {
         _heap[node] = mfn_to_virt(mfn + nr - needed);
         avail[node] = mfn_to_virt(mfn + nr - 1) +
                       PAGE_SIZE - sizeof(**avail) * NR_ZONES;
     }
     else if ( nr >= needed &&
-              (mfn + needed) <= (virt_to_mfn(eva - 1) + 1) )
+              (mfn + needed) <= (virt_to_mfn(eva - 1) + 1) &&
+              (!xenheap_bits ||
+               !((mfn + needed - 1) >> (xenheap_bits - PAGE_SHIFT))) )
     {
         _heap[node] = mfn_to_virt(mfn);
         avail[node] = mfn_to_virt(mfn + needed - 1) +
@@ -1543,11 +1553,13 @@ void free_xenheap_pages(void *v, unsigne
 
 #else
 
-static unsigned int __read_mostly xenheap_bits;
-
 void __init xenheap_max_mfn(unsigned long mfn)
 {
-    xenheap_bits = flsl(mfn) + PAGE_SHIFT;
+    ASSERT(!first_node_initialised);
+    ASSERT(!xenheap_bits);
+    BUILD_BUG_ON(PADDR_BITS >= BITS_PER_LONG);
+    xenheap_bits = min(flsl(mfn + 1) - 1 + PAGE_SHIFT, PADDR_BITS);
+    printk(XENLOG_INFO "Xen heap: %u bits\n", xenheap_bits);
 }
 
 void init_xenheap_pages(paddr_t ps, paddr_t pe)

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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

[Wei Liu]

CC George (x86 MM maintainer)

On Thu, Aug 27, 2015 at 02:37:17AM -0600, 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>
> 
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -369,7 +369,7 @@ void __init arch_init_memory(void)
>  
>                      for ( i = 0; i < l3_table_offset(split_va); ++i )
>                          l3tab[i] = l3idle[i];
> -                    for ( ; i <= L3_PAGETABLE_ENTRIES; ++i )
> +                    for ( ; i < L3_PAGETABLE_ENTRIES; ++i )
>                          l3tab[i] = l3e_empty();
>                      split_l4e = l4e_from_pfn(virt_to_mfn(l3tab),
>                                               __PAGE_HYPERVISOR);
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1002,7 +1002,7 @@ void __init noreturn __start_xen(unsigne
>  
>      setup_max_pdx(raw_max_page);
>      if ( highmem_start )
> -        xenheap_max_mfn(PFN_DOWN(highmem_start));
> +        xenheap_max_mfn(PFN_DOWN(highmem_start - 1));
>  
>      /*
>       * Walk every RAM region and map it in its entirety (on x86/64, at least)
> @@ -1183,9 +1183,6 @@ void __init noreturn __start_xen(unsigne
>  
>      numa_initmem_init(0, raw_max_page);
>  
> -    end_boot_allocator();
> -    system_state = SYS_STATE_boot;
> -
>      if ( max_page - 1 > virt_to_mfn(HYPERVISOR_VIRT_END - 1) )
>      {
>          unsigned long limit = virt_to_mfn(HYPERVISOR_VIRT_END - 1);
> @@ -1194,6 +1191,8 @@ void __init noreturn __start_xen(unsigne
>          if ( !highmem_start )
>              xenheap_max_mfn(limit);
>  
> +        end_boot_allocator();
> +
>          /* Pass the remaining memory to the allocator. */
>          for ( i = 0; i < boot_e820.nr_map; i++ )
>          {
> @@ -1217,6 +1216,10 @@ void __init noreturn __start_xen(unsigne
>             opt_tmem = 0;
>          }
>      }
> +    else
> +        end_boot_allocator();
> +
> +    system_state = SYS_STATE_boot;
>  
>      vm_init();
>      console_init_ring();
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -404,13 +404,19 @@ void get_outstanding_claims(uint64_t *fr
>      spin_unlock(&heap_lock);
>  }
>  
> +static bool_t __read_mostly first_node_initialised;
> +#ifndef CONFIG_SEPARATE_XENHEAP
> +static unsigned int __read_mostly xenheap_bits;
> +#else
> +#define xenheap_bits 0
> +#endif
> +
>  static unsigned long init_node_heap(int node, unsigned long mfn,
>                                      unsigned long nr, bool_t *use_tail)
>  {
>      /* First node to be discovered has its heap metadata statically alloced. */
>      static heap_by_zone_and_order_t _heap_static;
>      static unsigned long avail_static[NR_ZONES];
> -    static int first_node_initialised;
>      unsigned long needed = (sizeof(**_heap) +
>                              sizeof(**avail) * NR_ZONES +
>                              PAGE_SIZE - 1) >> PAGE_SHIFT;
> @@ -428,14 +434,18 @@ static unsigned long init_node_heap(int 
>      }
>  #ifdef DIRECTMAP_VIRT_END
>      else if ( *use_tail && nr >= needed &&
> -              (mfn + nr) <= (virt_to_mfn(eva - 1) + 1) )
> +              (mfn + nr) <= (virt_to_mfn(eva - 1) + 1) &&
> +              (!xenheap_bits ||
> +               !((mfn + nr - 1) >> (xenheap_bits - PAGE_SHIFT))) )
>      {
>          _heap[node] = mfn_to_virt(mfn + nr - needed);
>          avail[node] = mfn_to_virt(mfn + nr - 1) +
>                        PAGE_SIZE - sizeof(**avail) * NR_ZONES;
>      }
>      else if ( nr >= needed &&
> -              (mfn + needed) <= (virt_to_mfn(eva - 1) + 1) )
> +              (mfn + needed) <= (virt_to_mfn(eva - 1) + 1) &&
> +              (!xenheap_bits ||
> +               !((mfn + needed - 1) >> (xenheap_bits - PAGE_SHIFT))) )
>      {
>          _heap[node] = mfn_to_virt(mfn);
>          avail[node] = mfn_to_virt(mfn + needed - 1) +
> @@ -1543,11 +1553,13 @@ void free_xenheap_pages(void *v, unsigne
>  
>  #else
>  
> -static unsigned int __read_mostly xenheap_bits;
> -
>  void __init xenheap_max_mfn(unsigned long mfn)
>  {
> -    xenheap_bits = flsl(mfn) + PAGE_SHIFT;
> +    ASSERT(!first_node_initialised);
> +    ASSERT(!xenheap_bits);
> +    BUILD_BUG_ON(PADDR_BITS >= BITS_PER_LONG);
> +    xenheap_bits = min(flsl(mfn + 1) - 1 + PAGE_SHIFT, PADDR_BITS);
> +    printk(XENLOG_INFO "Xen heap: %u bits\n", xenheap_bits);
>  }
>  
>  void init_xenheap_pages(paddr_t ps, paddr_t pe)
> 
> 

> x86/NUMA: make init_node_heap() respect Xen heap limit
> 
> 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>
> 
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -369,7 +369,7 @@ void __init arch_init_memory(void)
>  
>                      for ( i = 0; i < l3_table_offset(split_va); ++i )
>                          l3tab[i] = l3idle[i];
> -                    for ( ; i <= L3_PAGETABLE_ENTRIES; ++i )
> +                    for ( ; i < L3_PAGETABLE_ENTRIES; ++i )
>                          l3tab[i] = l3e_empty();
>                      split_l4e = l4e_from_pfn(virt_to_mfn(l3tab),
>                                               __PAGE_HYPERVISOR);
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1002,7 +1002,7 @@ void __init noreturn __start_xen(unsigne
>  
>      setup_max_pdx(raw_max_page);
>      if ( highmem_start )
> -        xenheap_max_mfn(PFN_DOWN(highmem_start));
> +        xenheap_max_mfn(PFN_DOWN(highmem_start - 1));
>  
>      /*
>       * Walk every RAM region and map it in its entirety (on x86/64, at least)
> @@ -1183,9 +1183,6 @@ void __init noreturn __start_xen(unsigne
>  
>      numa_initmem_init(0, raw_max_page);
>  
> -    end_boot_allocator();
> -    system_state = SYS_STATE_boot;
> -
>      if ( max_page - 1 > virt_to_mfn(HYPERVISOR_VIRT_END - 1) )
>      {
>          unsigned long limit = virt_to_mfn(HYPERVISOR_VIRT_END - 1);
> @@ -1194,6 +1191,8 @@ void __init noreturn __start_xen(unsigne
>          if ( !highmem_start )
>              xenheap_max_mfn(limit);
>  
> +        end_boot_allocator();
> +
>          /* Pass the remaining memory to the allocator. */
>          for ( i = 0; i < boot_e820.nr_map; i++ )
>          {
> @@ -1217,6 +1216,10 @@ void __init noreturn __start_xen(unsigne
>             opt_tmem = 0;
>          }
>      }
> +    else
> +        end_boot_allocator();
> +
> +    system_state = SYS_STATE_boot;
>  
>      vm_init();
>      console_init_ring();
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -404,13 +404,19 @@ void get_outstanding_claims(uint64_t *fr
>      spin_unlock(&heap_lock);
>  }
>  
> +static bool_t __read_mostly first_node_initialised;
> +#ifndef CONFIG_SEPARATE_XENHEAP
> +static unsigned int __read_mostly xenheap_bits;
> +#else
> +#define xenheap_bits 0
> +#endif
> +
>  static unsigned long init_node_heap(int node, unsigned long mfn,
>                                      unsigned long nr, bool_t *use_tail)
>  {
>      /* First node to be discovered has its heap metadata statically alloced. */
>      static heap_by_zone_and_order_t _heap_static;
>      static unsigned long avail_static[NR_ZONES];
> -    static int first_node_initialised;
>      unsigned long needed = (sizeof(**_heap) +
>                              sizeof(**avail) * NR_ZONES +
>                              PAGE_SIZE - 1) >> PAGE_SHIFT;
> @@ -428,14 +434,18 @@ static unsigned long init_node_heap(int 
>      }
>  #ifdef DIRECTMAP_VIRT_END
>      else if ( *use_tail && nr >= needed &&
> -              (mfn + nr) <= (virt_to_mfn(eva - 1) + 1) )
> +              (mfn + nr) <= (virt_to_mfn(eva - 1) + 1) &&
> +              (!xenheap_bits ||
> +               !((mfn + nr - 1) >> (xenheap_bits - PAGE_SHIFT))) )
>      {
>          _heap[node] = mfn_to_virt(mfn + nr - needed);
>          avail[node] = mfn_to_virt(mfn + nr - 1) +
>                        PAGE_SIZE - sizeof(**avail) * NR_ZONES;
>      }
>      else if ( nr >= needed &&
> -              (mfn + needed) <= (virt_to_mfn(eva - 1) + 1) )
> +              (mfn + needed) <= (virt_to_mfn(eva - 1) + 1) &&
> +              (!xenheap_bits ||
> +               !((mfn + needed - 1) >> (xenheap_bits - PAGE_SHIFT))) )
>      {
>          _heap[node] = mfn_to_virt(mfn);
>          avail[node] = mfn_to_virt(mfn + needed - 1) +
> @@ -1543,11 +1553,13 @@ void free_xenheap_pages(void *v, unsigne
>  
>  #else
>  
> -static unsigned int __read_mostly xenheap_bits;
> -
>  void __init xenheap_max_mfn(unsigned long mfn)
>  {
> -    xenheap_bits = flsl(mfn) + PAGE_SHIFT;
> +    ASSERT(!first_node_initialised);
> +    ASSERT(!xenheap_bits);
> +    BUILD_BUG_ON(PADDR_BITS >= BITS_PER_LONG);
> +    xenheap_bits = min(flsl(mfn + 1) - 1 + PAGE_SHIFT, PADDR_BITS);
> +    printk(XENLOG_INFO "Xen heap: %u bits\n", xenheap_bits);
>  }
>  
>  void init_xenheap_pages(paddr_t ps, paddr_t pe)

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

[Andrew Cooper]

On 27/08/15 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>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

[Wei Liu]

On Thu, Aug 27, 2015 at 02:37:17AM -0600, 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>

Release-acked-by: Wei Liu <wei.liu2@citrix.com>

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

[Ian Campbell]

On Thu, 2015-08-27 at 02:37 -0600, 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>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

@@ -428,14 +434,18 @@ static unsigned long init_node_heap(int
>      }
>  #ifdef DIRECTMAP_VIRT_END
>      else if ( *use_tail && nr >= needed &&
> -              (mfn + nr) <= (virt_to_mfn(eva - 1) + 1) )
> +              (mfn + nr) <= (virt_to_mfn(eva - 1) + 1) &&
> +              (!xenheap_bits ||
> +               !((mfn + nr - 1) >> (xenheap_bits - PAGE_SHIFT))) )

This logic appears twice (with just s/nr/needed/ the second time), and it
is a reasonably complex set of conditions. Moving it into a helper might be
a nice cleanup, which would also allow a descriptive name to be used and
also perhaps separating the various conditions into their own if (...)
return which might aid readability.

Ian.

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

[Julien Grall]

Hi Jan,

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.

I've noticed it while rebasing my branch on xengit/staging, although
given that there is no aarch64 hardware on osstest this issue won't
be reported.

I gave a try to boot on arm32 and didn't see any failure.

I will do more debug tonight and tomorrow morning to see what's going
on. Stack trace below:

- UART enabled -
- CPU 00000000 booting -
- Current EL 00000008 -
- Xen starting at EL2 -
- Zero BSS -
- Setting up control registers -
- Turning on paging -
- Ready -
(XEN) Checking for initrd in /chosen
(XEN) RAM: 0000004000000000 - 00000043ffffffff
(XEN)
(XEN) MODULE[0]: 0000004000ff9000 - 0000004001000000 Device Tree
(XEN) MODULE[1]: 0000004002000000 - 0000004002a4aa00 Kernel       console=hvc0 root=/dev/sda2 rw earlycon=uart8250,mmio32,0x1c020000 ignore_loglevel
(XEN)  RESVD[0]: 0000004003000000 - 0000004003004000
(XEN)  RESVD[1]: 0000004000000000 - 0000004000010000
(XEN)
(XEN) Command line: conswitch=x console=dtuart dtuart=/soc/serial@1c020000 no-bootscrub noreboot dom0_mem=4G
(XEN) Placing Xen at 0x00000043ffe00000-0x0000004400000000
(XEN) Update BOOTMOD_XEN from 0000004004000000-000000400410ad81 => 00000043ffe00000-00000043fff0ad81
(XEN) Xen heap: 38 bits
(XEN) Domain heap initialised
(XEN) create_xen_entries: L2 failed
(XEN) Hypervisor Trap. HSR=0x96000046 EC=0x25 IL=1 Syndrome=0x46
(XEN) CPU0: Unexpected Trap: Hypervisor
(XEN) ----[ Xen-4.6.0-rc  arm64  debug=y  Not tainted ]----
(XEN) CPU:    0
(XEN) PC:     000000000025a638 clear_page+0x10/0x24
(XEN) LR:     000000000027eb28
(XEN) SP:     00000000002b7db0
(XEN) CPSR:   800003c9 MODE:64-bit EL2h (Hypervisor, handler)
(XEN)      X0: 0000000040000000  X1: 0000000000000040  X2: 0000000000000004
(XEN)      X3: 0000000000000000  X4: 0000000000003fff  X5: 00000000002982e8
(XEN)      X6: 0000000000000004  X7: 00000000002c4a30  X8: 00000000002c4a28
(XEN)      X9: 0000000800037e40 X10: 00000000002c1950 X11: 0000000000000200
(XEN)     X12: 0000000000000000 X13: 0000000000000000 X14: 0000000000000000
(XEN)     X15: 0000000000400000 X16: 000000000028c734 X17: 0000000000000006
(XEN)     X18: 00000000000084d0 X19: 0000000040000000 X20: 0000000000000001
(XEN)     X21: 000000000027b400 X22: 0000000040001000 X23: 000000000027b000
(XEN)     X24: 000000000027b000 X25: 000000000027b000 X26: 0000000000006db7
(XEN)     X27: fffffff800000000 X28: 0000000040000000  FP: 00000000002b7db0
(XEN)
(XEN)   VTCR_EL2: 80000000
(XEN)  VTTBR_EL2: 0000000000000000
(XEN)
(XEN)  SCTLR_EL2: 30cd183d
(XEN)    HCR_EL2: 000000000030643f
(XEN)  TTBR0_EL2: 00000043ffef4000
(XEN)
(XEN)    ESR_EL2: 96000046
(XEN)  HPFAR_EL2: 0000000000000000
(XEN)    FAR_EL2: 0000000040000000
(XEN)
(XEN) Xen stack trace from sp=00000000002b7db0:
(XEN)    00000000002b7e20 0000000000287d6c 00008003ffff9000 0000000004400000
(XEN)    0000004400000000 00000000002a0000 0000000000000000 000000000027b000
(XEN)    00000000002a0048 0000004000000000 0000004400000000 0000000000000001
(XEN)    0000000000000000 0000000000000000 00000043efb181f0 000000000020060c
(XEN)    0000004004000000 0000004003e00000 0000004000ff9000 0000000000000000
(XEN)    0000000000400000 0000000000000000 0000000000000001 0000000000000000
(XEN)    0000000000000000 00000043efb8eff0 0000004000ff9000 0000000000007000
(XEN)    0000000400000000 000000000027d5e8 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000
(XEN) Xen call trace:
(XEN)    [<000000000025a638>] clear_page+0x10/0x24 (PC)
(XEN)    [<000000000027eb28>] vm_init+0x10c/0x1d0 (LR)
(XEN)    [<0000000000287d6c>] start_xen+0x54c/0xc94
(XEN)    [<000000000020060c>] paging+0x84/0xbc
(XEN)
(XEN)
(XEN) ****************************************
(XEN) Panic on CPU 0:
(XEN) CPU0: Unexpected Trap: Hypervisor
(XEN)
(XEN) ****************************************
(XEN)
(XEN) Manual reset required ('noreboot' specified)

Regards,

-- 
Julien Grall

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

[Julien Grall]

On 03/09/2015 21:01, Julien Grall wrote:
> Hi Jan,
> 
> 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.

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).

Regards,

commit d1f2e47326da87c55f3165156407f224d684bccd
Author: Julien Grall <julien.grall@citrix.com>
Date:   Thu Sep 3 21:49:31 2015 +0100

    xen: pagealloc: Correctly calculate the number of xenheap bits

    The commit 88e3ed61642bb393458acc7a9bd2f96edc337190 "x86/NUMA: make
    init_node_heap() respect Xen heap limit" breaks boot on the arm64 board
    X-Gene.

    This is because the last xen heaf MFN is 0x4400000. Which the new way to
    calculate the number bits, the result would 38 bits which doesn't cover
    all the xenheap. Fix it by dropping the -1 in the expression.

    Signed-off-by: Julien Grall <julien.grall@citrix.com>

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 74fc1de..24fb09c 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -1558,7 +1558,7 @@ void __init xenheap_max_mfn(unsigned long mfn)
     ASSERT(!first_node_initialised);
     ASSERT(!xenheap_bits);
     BUILD_BUG_ON(PADDR_BITS >= BITS_PER_LONG);
-    xenheap_bits = min(flsl(mfn + 1) - 1 + PAGE_SHIFT, PADDR_BITS);
+    xenheap_bits = min(flsl(mfn + 1) + PAGE_SHIFT, PADDR_BITS);
     printk(XENLOG_INFO "Xen heap: %u bits\n", xenheap_bits);
 }

-- 
Julien Grall

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

[Jan Beulich]

>>> 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).

Jan

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

[Ian Campbell]

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.

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

[Jan Beulich]

>>> On 04.09.15 at 10:27, <ian.campbell@citrix.com> wrote:
> On Fri, 2015-09-04 at 01:37 -0600, Jan Beulich wrote:
>> > > > On 03.09.15 at 22:58, <julien.grall@citrix.com> wrote:
>> > 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?

Yes, if all memory is always accessible, then no limit should be
enforced at all, i.e. the call be dropped.

Just for the record - even with the call in place it escapes me why
this causes any problem: All it tells the allocator is to not hand out
pages above a certain limit when asked for Xen heap pages. Sadly
I wasn't able to interpret the dumped information (after the crash)
in a way telling me what actually went wrong. IOW - I'm afraid
there's a second problem here that's going to be hidden again
when the call gets removed.

Jan

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

[Ian Campbell]

On Fri, 2015-09-04 at 02:39 -0600, Jan Beulich wrote:
> > 
> > > > On 04.09.15 at 10:27, <ian.campbell@citrix.com> wrote:
> > On Fri, 2015-09-04 at 01:37 -0600, Jan Beulich wrote:
> > > > > > On 03.09.15 at 22:58, <julien.grall@citrix.com> wrote:
> > > > 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?
> 
> Yes, if all memory is always accessible, then no limit should be
> enforced at all, i.e. the call be dropped.
> 
> Just for the record - even with the call in place it escapes me why
> this causes any problem: All it tells the allocator is to not hand out
> pages above a certain limit when asked for Xen heap pages. Sadly
> I wasn't able to interpret the dumped information (after the crash)
> in a way telling me what actually went wrong.

"create_xen_entries: L2 failed" tells me, through code inspection rather
than usefulness of the logging, that alloc_xenheap_page has returned NULL.

I think this is simply because all RAM on Mustang is at physical address
128GB onwards or so, IOW the off by one error has resulted in the xenheap
appearing to be empty since all RAM above the limit.

>  IOW - I'm afraid
> there's a second problem here that's going to be hidden again
> when the call gets removed.
> 
> Jan
> 

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

[Jan Beulich]

>>> On 04.09.15 at 10:52, <ian.campbell@citrix.com> wrote:
> On Fri, 2015-09-04 at 02:39 -0600, Jan Beulich wrote:
>> > 
>> > > > On 04.09.15 at 10:27, <ian.campbell@citrix.com> wrote:
>> > On Fri, 2015-09-04 at 01:37 -0600, Jan Beulich wrote:
>> > > > > > On 03.09.15 at 22:58, <julien.grall@citrix.com> wrote:
>> > > > 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?
>> 
>> Yes, if all memory is always accessible, then no limit should be
>> enforced at all, i.e. the call be dropped.
>> 
>> Just for the record - even with the call in place it escapes me why
>> this causes any problem: All it tells the allocator is to not hand out
>> pages above a certain limit when asked for Xen heap pages. Sadly
>> I wasn't able to interpret the dumped information (after the crash)
>> in a way telling me what actually went wrong.
> 
> "create_xen_entries: L2 failed" tells me, through code inspection rather
> than usefulness of the logging, that alloc_xenheap_page has returned NULL.
> 
> I think this is simply because all RAM on Mustang is at physical address
> 128GB onwards or so, IOW the off by one error has resulted in the xenheap
> appearing to be empty since all RAM above the limit.

Oh, I see - I made the (x86-biased, and hence wrong) assumption
that with Julien mentioning only the ending MFN, the start of RAM
would be at 0.

Jan

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

[Julien Grall]

Hi Jan,

On 04/09/15 10:09, Jan Beulich wrote:
>> "create_xen_entries: L2 failed" tells me, through code inspection rather
>> than usefulness of the logging, that alloc_xenheap_page has returned NULL.
>>
>> I think this is simply because all RAM on Mustang is at physical address
>> 128GB onwards or so, IOW the off by one error has resulted in the xenheap
>> appearing to be empty since all RAM above the limit.

Correct, the RAM bank is:

RAM: 0000004000000000 - 00000043ffffffff

As we expose only 38 bits now, it will hide all the RAM.

> Oh, I see - I made the (x86-biased, and hence wrong) assumption
> that with Julien mentioning only the ending MFN, the start of RAM
> would be at 0.

Sorry, I forgot to mention it in my mail. On ARM, RAM
are divided in memory banks which can start anywhere in the memory. When early printk
is enabled the RAM banks are one of the first things printed in the serial console.

Anyway, I tried your suggestion to dropped the call to xenheap_max_mfn when Xen
setups the xenheap for arm64 and it allows me to boot correctly Xen on X-gene.
See patch below, I can send the patch in a separate thread if necessary.

commit b11ab8e4982228d7944e11010f5b8eec890caf30
Author: Julien Grall <julien.grall@citrix.com>
Date:   Thu Sep 3 21:49:31 2015 +0100

    xen: pagealloc: Correctly calculate the number of xenheap bits
    
    The commit 88e3ed61642bb393458acc7a9bd2f96edc337190 "x86/NUMA: make
    init_node_heap() respect Xen heap limit" breaks boot on the arm64 board
    X-Gene.
    
    The xenheap bits variable is used to know the last RAM MFN always mapped
    in Xen virtual memory. If the value is 0, it means that all the memory is
    always mapped in Xen virtual memory.
    
    On X-gene the RAM bank resides above 128GB and last xenheap MFN is
    0x4400000. With the new way to calculate the number of bits, xenheap_bits
    will be equal to 38 bits. This will result to hide all the RAM and the
    impossibility to allocate xenheap memory.
    
    Given that aarch64 have always all the memory mapped in Xen virtual
    memory, it's not necessary to call xenheap_max_mfn which set the number
    of bits.
    
    Suggested-by: Jan Beulich <jbeulich@suse.com>
    Signed-off-by: Julien Grall <julien.grall@citrix.com>

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 6626eba..48f734f 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -665,7 +665,6 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
     xenheap_virt_end = XENHEAP_VIRT_START + ram_end - ram_start;
     xenheap_mfn_start = ram_start >> PAGE_SHIFT;
     xenheap_mfn_end = ram_end >> PAGE_SHIFT;
-    xenheap_max_mfn(xenheap_mfn_end);
 
     /*
      * Need enough mapped pages for copying the DTB.
-- 
Julien Grall

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

[Jan Beulich]

>>> On 04.09.15 at 13:29, <julien.grall@citrix.com> wrote:
> Anyway, I tried your suggestion to dropped the call to xenheap_max_mfn when 
> Xen
> setups the xenheap for arm64 and it allows me to boot correctly Xen on 
> X-gene.
> See patch below, I can send the patch in a separate thread if necessary.

That would depend on Ian and Wei - to me it looks okay this way,
perhaps minus the title wanting to be re-written.

Jan

> commit b11ab8e4982228d7944e11010f5b8eec890caf30
> Author: Julien Grall <julien.grall@citrix.com>
> Date:   Thu Sep 3 21:49:31 2015 +0100
> 
>     xen: pagealloc: Correctly calculate the number of xenheap bits
>     
>     The commit 88e3ed61642bb393458acc7a9bd2f96edc337190 "x86/NUMA: make
>     init_node_heap() respect Xen heap limit" breaks boot on the arm64 board
>     X-Gene.
>     
>     The xenheap bits variable is used to know the last RAM MFN always mapped
>     in Xen virtual memory. If the value is 0, it means that all the memory is
>     always mapped in Xen virtual memory.
>     
>     On X-gene the RAM bank resides above 128GB and last xenheap MFN is
>     0x4400000. With the new way to calculate the number of bits, xenheap_bits
>     will be equal to 38 bits. This will result to hide all the RAM and the
>     impossibility to allocate xenheap memory.
>     
>     Given that aarch64 have always all the memory mapped in Xen virtual
>     memory, it's not necessary to call xenheap_max_mfn which set the number
>     of bits.
>     
>     Suggested-by: Jan Beulich <jbeulich@suse.com>
>     Signed-off-by: Julien Grall <julien.grall@citrix.com>
> 
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 6626eba..48f734f 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -665,7 +665,6 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
>      xenheap_virt_end = XENHEAP_VIRT_START + ram_end - ram_start;
>      xenheap_mfn_start = ram_start >> PAGE_SHIFT;
>      xenheap_mfn_end = ram_end >> PAGE_SHIFT;
> -    xenheap_max_mfn(xenheap_mfn_end);
>  
>      /*
>       * Need enough mapped pages for copying the DTB.
> -- 
> Julien Grall

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

[Wei Liu]

On Fri, Sep 04, 2015 at 06:02:28AM -0600, Jan Beulich wrote:
> >>> On 04.09.15 at 13:29, <julien.grall@citrix.com> wrote:
> > Anyway, I tried your suggestion to dropped the call to xenheap_max_mfn when 
> > Xen
> > setups the xenheap for arm64 and it allows me to boot correctly Xen on 
> > X-gene.
> > See patch below, I can send the patch in a separate thread if necessary.
> 
> That would depend on Ian and Wei - to me it looks okay this way,
> perhaps minus the title wanting to be re-written.
> 

I only care about the actual fix, not the form it is presented. In the
end I can't apply this patch myself. :-)

BTW if this patch is final I would be happy for it to be applied today.

Wei.

> Jan
> 
> > commit b11ab8e4982228d7944e11010f5b8eec890caf30
> > Author: Julien Grall <julien.grall@citrix.com>
> > Date:   Thu Sep 3 21:49:31 2015 +0100
> > 
> >     xen: pagealloc: Correctly calculate the number of xenheap bits
> >     
> >     The commit 88e3ed61642bb393458acc7a9bd2f96edc337190 "x86/NUMA: make
> >     init_node_heap() respect Xen heap limit" breaks boot on the arm64 board
> >     X-Gene.
> >     
> >     The xenheap bits variable is used to know the last RAM MFN always mapped
> >     in Xen virtual memory. If the value is 0, it means that all the memory is
> >     always mapped in Xen virtual memory.
> >     
> >     On X-gene the RAM bank resides above 128GB and last xenheap MFN is
> >     0x4400000. With the new way to calculate the number of bits, xenheap_bits
> >     will be equal to 38 bits. This will result to hide all the RAM and the
> >     impossibility to allocate xenheap memory.
> >     
> >     Given that aarch64 have always all the memory mapped in Xen virtual
> >     memory, it's not necessary to call xenheap_max_mfn which set the number
> >     of bits.
> >     
> >     Suggested-by: Jan Beulich <jbeulich@suse.com>
> >     Signed-off-by: Julien Grall <julien.grall@citrix.com>
> > 
> > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> > index 6626eba..48f734f 100644
> > --- a/xen/arch/arm/setup.c
> > +++ b/xen/arch/arm/setup.c
> > @@ -665,7 +665,6 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
> >      xenheap_virt_end = XENHEAP_VIRT_START + ram_end - ram_start;
> >      xenheap_mfn_start = ram_start >> PAGE_SHIFT;
> >      xenheap_mfn_end = ram_end >> PAGE_SHIFT;
> > -    xenheap_max_mfn(xenheap_mfn_end);
> >  
> >      /*
> >       * Need enough mapped pages for copying the DTB.
> > -- 
> > Julien Grall
> 
> 

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

[Julien Grall]

On 04/09/15 13:02, Jan Beulich wrote:
>>>> On 04.09.15 at 13:29, <julien.grall@citrix.com> wrote:
>> Anyway, I tried your suggestion to dropped the call to xenheap_max_mfn when 
>> Xen
>> setups the xenheap for arm64 and it allows me to boot correctly Xen on 
>> X-gene.
>> See patch below, I can send the patch in a separate thread if necessary.
> 
> That would depend on Ian and Wei - to me it looks okay this way,
> perhaps minus the title wanting to be re-written.

I took the previous commit message and rewrite it a bit but forgot to
update the title. How about:

"xen/arm64: Remove call to xenheap_max_mfn when setup the xenheap"

>> commit b11ab8e4982228d7944e11010f5b8eec890caf30
>> Author: Julien Grall <julien.grall@citrix.com>
>> Date:   Thu Sep 3 21:49:31 2015 +0100
>>
>>     xen: pagealloc: Correctly calculate the number of xenheap bits
>>     
>>     The commit 88e3ed61642bb393458acc7a9bd2f96edc337190 "x86/NUMA: make
>>     init_node_heap() respect Xen heap limit" breaks boot on the arm64 board
>>     X-Gene.
>>     
>>     The xenheap bits variable is used to know the last RAM MFN always mapped
>>     in Xen virtual memory. If the value is 0, it means that all the memory is
>>     always mapped in Xen virtual memory.
>>     
>>     On X-gene the RAM bank resides above 128GB and last xenheap MFN is
>>     0x4400000. With the new way to calculate the number of bits, xenheap_bits
>>     will be equal to 38 bits. This will result to hide all the RAM and the
>>     impossibility to allocate xenheap memory.
>>     
>>     Given that aarch64 have always all the memory mapped in Xen virtual
>>     memory, it's not necessary to call xenheap_max_mfn which set the number
>>     of bits.
>>     
>>     Suggested-by: Jan Beulich <jbeulich@suse.com>
>>     Signed-off-by: Julien Grall <julien.grall@citrix.com>
>>
>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>> index 6626eba..48f734f 100644
>> --- a/xen/arch/arm/setup.c
>> +++ b/xen/arch/arm/setup.c
>> @@ -665,7 +665,6 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
>>      xenheap_virt_end = XENHEAP_VIRT_START + ram_end - ram_start;
>>      xenheap_mfn_start = ram_start >> PAGE_SHIFT;
>>      xenheap_mfn_end = ram_end >> PAGE_SHIFT;
>> -    xenheap_max_mfn(xenheap_mfn_end);
>>  
>>      /*
>>       * Need enough mapped pages for copying the DTB.
>> -- 
>> Julien Grall
> 
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 


-- 
Julien Grall

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

[Ian Campbell]

On Fri, 2015-09-04 at 13:50 +0100, Julien Grall wrote:
> On 04/09/15 13:02, Jan Beulich wrote:
> > > > > On 04.09.15 at 13:29, <julien.grall@citrix.com> wrote:
> > > Anyway, I tried your suggestion to dropped the call to 
> > > xenheap_max_mfn when 
> > > Xen
> > > setups the xenheap for arm64 and it allows me to boot correctly Xen 
> > > on 
> > > X-gene.
> > > See patch below, I can send the patch in a separate thread if 
> > > necessary.
> > 
> > That would depend on Ian and Wei - to me it looks okay this way,
> > perhaps minus the title wanting to be re-written.
> 
> I took the previous commit message and rewrite it a bit but forgot to
> update the title. How about:
> 
> "xen/arm64: Remove call to xenheap_max_mfn when setup the xenheap"

WFM and my ack would apply with this in place too. (s/setup/setting up/ if
I were feeling pedantic)

> 
> > > commit b11ab8e4982228d7944e11010f5b8eec890caf30
> > > Author: Julien Grall <julien.grall@citrix.com>
> > > Date:   Thu Sep 3 21:49:31 2015 +0100
> > > 
> > >     xen: pagealloc: Correctly calculate the number of xenheap bits
> > >     
> > >     The commit 88e3ed61642bb393458acc7a9bd2f96edc337190 "x86/NUMA: 
> > > make
> > >     init_node_heap() respect Xen heap limit" breaks boot on the arm64 
> > > board
> > >     X-Gene.
> > >     
> > >     The xenheap bits variable is used to know the last RAM MFN always 
> > > mapped
> > >     in Xen virtual memory. If the value is 0, it means that all the 
> > > memory is
> > >     always mapped in Xen virtual memory.
> > >     
> > >     On X-gene the RAM bank resides above 128GB and last xenheap MFN 
> > > is
> > >     0x4400000. With the new way to calculate the number of bits, 
> > > xenheap_bits
> > >     will be equal to 38 bits. This will result to hide all the RAM 
> > > and the
> > >     impossibility to allocate xenheap memory.
> > >     
> > >     Given that aarch64 have always all the memory mapped in Xen 
> > > virtual
> > >     memory, it's not necessary to call xenheap_max_mfn which set the 
> > > number
> > >     of bits.
> > >     
> > >     Suggested-by: Jan Beulich <jbeulich@suse.com>
> > >     Signed-off-by: Julien Grall <julien.grall@citrix.com>
> > > 
> > > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> > > index 6626eba..48f734f 100644
> > > --- a/xen/arch/arm/setup.c
> > > +++ b/xen/arch/arm/setup.c
> > > @@ -665,7 +665,6 @@ static void __init setup_mm(unsigned long 
> > > dtb_paddr, size_t dtb_size)
> > >      xenheap_virt_end = XENHEAP_VIRT_START + ram_end - ram_start;
> > >      xenheap_mfn_start = ram_start >> PAGE_SHIFT;
> > >      xenheap_mfn_end = ram_end >> PAGE_SHIFT;
> > > -    xenheap_max_mfn(xenheap_mfn_end);
> > >  
> > >      /*
> > >       * Need enough mapped pages for copying the DTB.
> > > -- 
> > > Julien Grall
> > 
> > 
> > 
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel
> > 
> 
> 

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

[Ian Campbell]

> commit b11ab8e4982228d7944e11010f5b8eec890caf30
> Author: Julien Grall <julien.grall@citrix.com>
> Date:   Thu Sep 3 21:49:31 2015 +0100
> 
>     xen: pagealloc: Correctly calculate the number of xenheap bits

More accurate would be

    xen: arm64: do not (incorrectly) limit size of xenheap

Or some such?

With that:
    Acked-by: Ian Campbell <ian.campbell@citrix.com>

Will you resend as a "git am" able thing please?

>     
>     The commit 88e3ed61642bb393458acc7a9bd2f96edc337190 "x86/NUMA: make
>     init_node_heap() respect Xen heap limit" breaks boot on the arm64 board
>     X-Gene.
>     
>     The xenheap bits variable is used to know the last RAM MFN always mapped
>     in Xen virtual memory. If the value is 0, it means that all the memory is
>     always mapped in Xen virtual memory.
>     
>     On X-gene the RAM bank resides above 128GB and last xenheap MFN is
>     0x4400000. With the new way to calculate the number of bits, xenheap_bits
>     will be equal to 38 bits. This will result to hide all the RAM and the
>     impossibility to allocate xenheap memory.
>     
>     Given that aarch64 have always all the memory mapped in Xen virtual
>     memory, it's not necessary to call xenheap_max_mfn which set the number
>     of bits.
>     
>     Suggested-by: Jan Beulich <jbeulich@suse.com>
>     Signed-off-by: Julien Grall <julien.grall@citrix.com>
> 
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 6626eba..48f734f 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -665,7 +665,6 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
>      xenheap_virt_end = XENHEAP_VIRT_START + ram_end - ram_start;
>      xenheap_mfn_start = ram_start >> PAGE_SHIFT;
>      xenheap_mfn_end = ram_end >> PAGE_SHIFT;
> -    xenheap_max_mfn(xenheap_mfn_end);
>  
>      /*
>       * Need enough mapped pages for copying the DTB.

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

[Julien Grall]

On 04/09/15 13:52, Ian Campbell wrote:
>> commit b11ab8e4982228d7944e11010f5b8eec890caf30
>> Author: Julien Grall <julien.grall@citrix.com>
>> Date:   Thu Sep 3 21:49:31 2015 +0100
>>
>>     xen: pagealloc: Correctly calculate the number of xenheap bits
> 
> More accurate would be
> 
>     xen: arm64: do not (incorrectly) limit size of xenheap
> 
> Or some such?

Sounds good.

> With that:
>     Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> Will you resend as a "git am" able thing please?

Sure. I will update the patch and add your ack.

Regards,

-- 
Julien Grall