* [PATCH] xen/arm: setup: initialize xenheap mappings after boot pages avaiable
@ 2016-05-27 5:31 Peng Fan
2016-05-30 21:53 ` Julien Grall
0 siblings, 1 reply; 6+ messages in thread
From: Peng Fan @ 2016-05-27 5:31 UTC (permalink / raw)
To: julien.grall, sstabellini; +Cc: van.freenix, xen-devel
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).
So, need to make sure boot pages are ready before setup xenheap mappings.
Signed-off-by: Peng Fan <van.freenix@gmail.com>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
---
xen/arch/arm/setup.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index dcb23b7..24cf9d3 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -641,8 +641,6 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_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);
-
s = bank_start;
while ( s < bank_end )
{
@@ -663,6 +661,8 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
dt_unreserved_regions(s, e, init_boot_pages, 0);
s = n;
}
+
+ setup_xenheap_mappings(bank_start>>PAGE_SHIFT, bank_size>>PAGE_SHIFT);
}
total_pages += ram_size >> PAGE_SHIFT;
--
2.6.2
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] xen/arm: setup: initialize xenheap mappings after boot pages avaiable
2016-05-27 5:31 [PATCH] xen/arm: setup: initialize xenheap mappings after boot pages avaiable Peng Fan
@ 2016-05-30 21:53 ` Julien Grall
2016-05-31 9:58 ` Peng Fan
0 siblings, 1 reply; 6+ messages in thread
From: Julien Grall @ 2016-05-30 21:53 UTC (permalink / raw)
To: Peng Fan, sstabellini; +Cc: xen-devel
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? :)
> 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.
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?
Regards,
>
> Signed-off-by: Peng Fan <van.freenix@gmail.com>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> ---
> xen/arch/arm/setup.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index dcb23b7..24cf9d3 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -641,8 +641,6 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_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);
> -
> s = bank_start;
> while ( s < bank_end )
> {
> @@ -663,6 +661,8 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
> dt_unreserved_regions(s, e, init_boot_pages, 0);
> s = n;
> }
> +
> + setup_xenheap_mappings(bank_start>>PAGE_SHIFT, bank_size>>PAGE_SHIFT);
> }
>
> total_pages += ram_size >> PAGE_SHIFT;
>
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xen/arm: setup: initialize xenheap mappings after boot pages avaiable
2016-05-30 21:53 ` Julien Grall
@ 2016-05-31 9:58 ` Peng Fan
2016-05-31 17:08 ` Julien Grall
0 siblings, 1 reply; 6+ messages in thread
From: Peng Fan @ 2016-05-31 9:58 UTC (permalink / raw)
To: Julien Grall; +Cc: sstabellini, xen-devel
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@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] xen/arm: setup: initialize xenheap mappings after boot pages avaiable
2016-05-31 9:58 ` Peng Fan
@ 2016-05-31 17:08 ` Julien Grall
2016-06-01 7:40 ` Peng Fan
0 siblings, 1 reply; 6+ messages in thread
From: Julien Grall @ 2016-05-31 17:08 UTC (permalink / raw)
To: Peng Fan; +Cc: sstabellini, xen-devel
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.
>
> 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.
In any case, even if the variable are correctly setup the underlying
page table are not present.
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.
It is not the nicest way, so I will happy if you find a better one.
Regards,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xen/arm: setup: initialize xenheap mappings after boot pages avaiable
2016-05-31 17:08 ` Julien Grall
@ 2016-06-01 7:40 ` Peng Fan
2016-06-01 11:13 ` Julien Grall
0 siblings, 1 reply; 6+ messages in thread
From: Peng Fan @ 2016-06-01 7:40 UTC (permalink / raw)
To: Julien Grall; +Cc: sstabellini, xen-devel
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@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xen/arm: setup: initialize xenheap mappings after boot pages avaiable
2016-06-01 7:40 ` Peng Fan
@ 2016-06-01 11:13 ` Julien Grall
0 siblings, 0 replies; 6+ messages in thread
From: Julien Grall @ 2016-06-01 11:13 UTC (permalink / raw)
To: Peng Fan; +Cc: sstabellini, xen-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-06-01 11:13 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-27 5:31 [PATCH] xen/arm: setup: initialize xenheap mappings after boot pages avaiable Peng Fan
2016-05-30 21:53 ` Julien Grall
2016-05-31 9:58 ` Peng Fan
2016-05-31 17:08 ` Julien Grall
2016-06-01 7:40 ` Peng Fan
2016-06-01 11:13 ` Julien Grall
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).