Re: [Xen-devel] [PATCH] xen/arm: setup: initialize xenheap mappings after boot pages avaiable

[Peng Fan <van.freenix@xxxxxxxxx>]

Hi Julien,

On Mon, May 30, 2016 at 10:53:24PM +0100, Julien Grall wrote:
>Hi Peng,
>
>On 27/05/2016 06:31, Peng Fan wrote:
>>To ARM64, setup_xenheap_mappings may call alloc_boot_pages to allocate
>>first level page table, if there is a big chunk memory (ie, >512GB).
>
>Out of interest, have you found the bug by testing Xen on a platform with
>512GB of RAM? :)

Actually not. My board does not have so large memory. When I am reading
the piece code, I think there is possibility that setup_xenheap_mappings
may need boot pages, but boot pages are not ready.

>
>>So, need to make sure boot pages are ready before setup xenheap mappings.
>
>init_boot_pages is using mfn_to_virt (see bootmem_region_add), which cannot
>work until xenheap_mfn_start is initialized. This is done by
>setup_xenheap_mappings.

My bad. I did not catch this point. Thanks for correcting me.

>
>I would be happy to give you hint on how to solve this, but I am not sure to
>fully understand your issue. Can you give more details?

I did not met issue on my platform. I just think the logic of this piece code
may cause errors on platform with large memory (>512GB).

How about the following patch?
First loop all the memory banks and calculate ram_size/ram_start/ram_end,
then set xenheap_virt_end/start and xenheap_mfn_end.
Now readly for init boot pages and setup_xenheap_mappings.

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index dcb23b7..d3a3af3 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -635,13 +635,24 @@ static void __init setup_mm(unsigned long dtb_paddr, 
size_t dtb_size)
         paddr_t bank_start = bootinfo.mem.bank[bank].start;
         paddr_t bank_size = bootinfo.mem.bank[bank].size;
         paddr_t bank_end = bank_start + bank_size;
-        paddr_t s, e;
 
         ram_size = ram_size + bank_size;
         ram_start = min(ram_start,bank_start);
         ram_end = max(ram_end,bank_end);
+    }
 
-        setup_xenheap_mappings(bank_start>>PAGE_SHIFT, bank_size>>PAGE_SHIFT);
+    total_pages += ram_size >> PAGE_SHIFT;
+
+    xenheap_virt_end = XENHEAP_VIRT_START + ram_end - ram_start;
+    xenheap_mfn_start = ram_start >> PAGE_SHIFT;
+    xenheap_mfn_end = ram_end >> PAGE_SHIFT;
+
+    for ( bank = 0 ; bank < bootinfo.mem.nr_banks; bank++ )
+    {
+        paddr_t bank_start = bootinfo.mem.bank[bank].start;
+        paddr_t bank_size = bootinfo.mem.bank[bank].size;
+        paddr_t bank_end = bank_start + bank_size;
+        paddr_t s, e;
 
         s = bank_start;
         while ( s < bank_end )
@@ -658,18 +669,12 @@ static void __init setup_mm(unsigned long dtb_paddr, 
size_t dtb_size)
             if ( e > bank_end )
                 e = bank_end;
 
-            xenheap_mfn_end = e;
-
             dt_unreserved_regions(s, e, init_boot_pages, 0);
             s = n;
         }
-    }
-
-    total_pages += ram_size >> PAGE_SHIFT;
 
-    xenheap_virt_end = XENHEAP_VIRT_START + ram_end - ram_start;
-    xenheap_mfn_start = ram_start >> PAGE_SHIFT;
-    xenheap_mfn_end = ram_end >> PAGE_SHIFT;
+        setup_xenheap_mappings(bank_start>>PAGE_SHIFT, bank_size>>PAGE_SHIFT);
+    }
 
     /*
      * Need enough mapped pages for copying the DTB.

Thanks,
Peng.

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