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

[Peng Fan <van.freenix@xxxxxxxxx>]

Hi Julien,

On Tue, May 31, 2016 at 06:08:38PM +0100, Julien Grall wrote:
>Hi Peng,
>
>On 31/05/16 10:58, Peng Fan wrote:
>>>
>>>>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.
>
>Have you tested this patch? I would be surprised to see it working.

Not tested (:- Just an idea in my mind.

>
>>
>>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_VIRT_START will be replaced by xenheap_virt_start which is not
>initialized at this time. It is done by setup_xenheap_mappings.

Could we move the piece code out to setup_mm, before init_boot_pages and 
setup_xenheap_mappings?
"
        xenheap_virt_start = DIRECTMAP_VIRT_START +
            (base_mfn - mfn) * PAGE_SIZE;
"

>
>In any case, even if the variable are correctly setup the underlying page
>table are not present.

Yeah, but setup_mm is only executed on one cpu and this is only for 
intialization.
Does that really matter if underlying page table are not ready?

>
>The easiest way I can think to fix the problem is splitting the bank with
>chunk of 512GB and calling setup_xenheap_mappings, init_boot_pages for each
>chunk.

Do you mean let process_memory_node handle the >512GB memory bank?

>
>It is not the nicest way, so I will happy if you find a better one.

I have no good idea now (:
I can follow your suggestion to split the >512GB into smaller banks when filling
bootinfo.mem.bank[xx].start and bootinfo.mem.bank[xx].size, if you are happy
with this way.

Thanks,
Peng.

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