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

[Julien Grall <julien.grall@arm.com>]

Hello Peng,

On 01/06/16 08:40, Peng Fan wrote:
> On Tue, May 31, 2016 at 06:08:38PM +0100, Julien Grall wrote:
>> 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.

When you send the patch to the ML, please either test it or clearly mark 
them as untested.

>>
>>>
>>> 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;
> "
>

Again no, see why below.

>>
>> 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?

Even if only one CPU is running, the page tables are already setup for 
this CPU (see the call to setup_pagetables just before setup_mm).

The boot allocator requires a bit of memory to keep track of the bootmem 
region (see bootmem_region_add). This memory will be taken from the xenheap.

So the xenheap has to be mapped (at least partially) into the page 
tables before calling for the first time init_boot_pages.

Regards,

-- 
Julien Grall

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