Re: [PATCH] xen/arm: Workaround for memory problem >1gb on last section

Re: [PATCH] xen/arm: Workaround for memory problem >1gb on last section

[Frediano Ziglio]

setup_xenheap_mappings setup head memory on Arm 32 has a limit of 1gb.
On system with large memory is possible that there are no blocks of memory
smaller than 1gb leading xenheap_pages to be more than 1gb.
This cause memory errors trying to access heap after the 1gb limit.

Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com>
---
 xen/arch/arm/setup.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 446de8a..b1a43e6 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -524,6 +524,9 @@ static void __init setup_mm(unsigned long
dtb_paddr, size_t dtb_size)
     if ( ! e )
         panic("Not not enough space for xenheap");

+    if ( xenheap_pages > 1<<(30-PAGE_SHIFT) )
+        xenheap_pages = 1<<(30-PAGE_SHIFT);
+
     domheap_pages = heap_pages - xenheap_pages;

     printk("Xen heap: %"PRIpaddr"-%"PRIpaddr" (%lu pages)\n",
-- 
1.9.1

I don't know if I can add Ian ack by myself as I changed the patch as
suggested by Julien.

Frediano


2014-10-01 14:06 GMT+01:00 Julien Grall <julien.grall@linaro.org>:
> Hello Frediano,
>
> On 29/09/2014 17:02, Frediano Ziglio wrote:
>>
>> setup_xenheap_mappings setup head memory on Arm 32 has a limit of 1GB.
>> On system with large memory is possible that there are no blocks of memory
>
>
> it's
>
>> smaller than 1gb leading xenheap_pages to be more than 1gb.
>
>
> Can you be consistent with the way to write 1GB?
>
>> This cause memory errors trying to access heap after the 1gb limit.
>>
>> I actually consider this patch as RFC as the pages are allocated at the
>> end
>> of the block found however I don't think is safe to assume that the end is
>> aligned to 32mb as required.
>
>
> After Ian's comment, I guess this paragraph should be dropped.
>
>> Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com>
>> ---
>>   xen/arch/arm/setup.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>>
>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>> index 446de8a..34b55b4 100644
>> --- a/xen/arch/arm/setup.c
>> +++ b/xen/arch/arm/setup.c
>> @@ -524,6 +524,11 @@ static void __init setup_mm(unsigned long
>> dtb_paddr, size_t dtb_size)
>>       if ( ! e )
>>           panic("Not not enough space for xenheap");
>>
>> +#ifdef CONFIG_ARM_32
>
>
> This version of setup_mm is only compiled for arm32, therefore the #ifdef is
> not necessary.
>
> Regards,
>
>
> --
> Julien Grall

Re: [PATCH] xen/arm: Workaround for memory problem >1gb on last section

[Julien Grall]

Hello Frediano,

On 10/01/2014 03:27 PM, Frediano Ziglio wrote:
> setup_xenheap_mappings setup head memory on Arm 32 has a limit of 1gb.
> On system with large memory is possible that there are no blocks of memory
> smaller than 1gb leading xenheap_pages to be more than 1gb.
> This cause memory errors trying to access heap after the 1gb limit.
> 
> Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com>
> ---
>  xen/arch/arm/setup.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 446de8a..b1a43e6 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -524,6 +524,9 @@ static void __init setup_mm(unsigned long
> dtb_paddr, size_t dtb_size)
>      if ( ! e )
>          panic("Not not enough space for xenheap");
> 
> +    if ( xenheap_pages > 1<<(30-PAGE_SHIFT) )
> +        xenheap_pages = 1<<(30-PAGE_SHIFT);
> +

Looking closer to the code, can't we add a new constraint before the
loop when we are looking for the maximum heap size?

smth like:

xenheap_pages = (heap_pages/8 + ...)
xenheap_pages = max(xenheap_pages, 128UL<<(20 - PAGE_SHIFT)
xenheap_pages = max(xenheap_pages, 1UL<<(30-PAGE_SHIFT))

Regards,

-- 
Julien Grall

Re: [PATCH] xen/arm: Workaround for memory problem >1gb on last section

[Frediano Ziglio]

setup_xenheap_mappings setup head memory on Arm 32 has a limit of 1gb.
On system with large memory is possible that there are no blocks of memory
smaller than 1gb leading xenheap_pages to be more than 1gb.
This cause memory errors trying to access heap after the 1gb limit.

Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com>
---
 xen/arch/arm/setup.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 446de8a..e223d1b 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -509,6 +509,7 @@ static void __init setup_mm(unsigned long
dtb_paddr, size_t dtb_size)
     heap_pages = ram_pages;
     xenheap_pages = (heap_pages/8 + 0x1fffUL) & ~0x1fffUL;
     xenheap_pages = max(xenheap_pages, 128UL<<(20-PAGE_SHIFT));
+    xenheap_pages = min(xenheap_pages, 1UL<<(30-PAGE_SHIFT));

     do
     {
-- 
1.9.1

I think you mean something like this. Tested and work on my board.
It's mostly the same code put before the loop instead of after. As
there are already some fixes like this I agree that before is better
(more consistent).

Frediano


2014-10-02 12:03 GMT+01:00 Julien Grall <julien.grall@linaro.org>:
> Hello Frediano,
>
> On 10/01/2014 03:27 PM, Frediano Ziglio wrote:
>> setup_xenheap_mappings setup head memory on Arm 32 has a limit of 1gb.
>> On system with large memory is possible that there are no blocks of memory
>> smaller than 1gb leading xenheap_pages to be more than 1gb.
>> This cause memory errors trying to access heap after the 1gb limit.
>>
>> Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com>
>> ---
>>  xen/arch/arm/setup.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>> index 446de8a..b1a43e6 100644
>> --- a/xen/arch/arm/setup.c
>> +++ b/xen/arch/arm/setup.c
>> @@ -524,6 +524,9 @@ static void __init setup_mm(unsigned long
>> dtb_paddr, size_t dtb_size)
>>      if ( ! e )
>>          panic("Not not enough space for xenheap");
>>
>> +    if ( xenheap_pages > 1<<(30-PAGE_SHIFT) )
>> +        xenheap_pages = 1<<(30-PAGE_SHIFT);
>> +
>
> Looking closer to the code, can't we add a new constraint before the
> loop when we are looking for the maximum heap size?
>
> smth like:
>
> xenheap_pages = (heap_pages/8 + ...)
> xenheap_pages = max(xenheap_pages, 128UL<<(20 - PAGE_SHIFT)
> xenheap_pages = max(xenheap_pages, 1UL<<(30-PAGE_SHIFT))
>
> Regards,
>
> --
> Julien Grall

Re: [PATCH] xen/arm: Workaround for memory problem >1gb

[Ian Campbell]

On Thu, 2014-10-02 at 12:03 +0100, Julien Grall wrote:
> Hello Frediano,
> 
> On 10/01/2014 03:27 PM, Frediano Ziglio wrote:
> > setup_xenheap_mappings setup head memory on Arm 32 has a limit of 1gb.
> > On system with large memory is possible that there are no blocks of memory
> > smaller than 1gb leading xenheap_pages to be more than 1gb.
> > This cause memory errors trying to access heap after the 1gb limit.
> > 
> > Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com>
> > ---
> >  xen/arch/arm/setup.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> > index 446de8a..b1a43e6 100644
> > --- a/xen/arch/arm/setup.c
> > +++ b/xen/arch/arm/setup.c
> > @@ -524,6 +524,9 @@ static void __init setup_mm(unsigned long
> > dtb_paddr, size_t dtb_size)
> >      if ( ! e )
> >          panic("Not not enough space for xenheap");
> > 
> > +    if ( xenheap_pages > 1<<(30-PAGE_SHIFT) )
> > +        xenheap_pages = 1<<(30-PAGE_SHIFT);
> > +
> 
> Looking closer to the code, can't we add a new constraint before the
> loop when we are looking for the maximum heap size?
> 
> smth like:
> 
> xenheap_pages = (heap_pages/8 + ...)
> xenheap_pages = max(xenheap_pages, 128UL<<(20 - PAGE_SHIFT)
> xenheap_pages = max(xenheap_pages, 1UL<<(30-PAGE_SHIFT))

I think that would work, and it would have the additional benefit that
for some memory layouts it might cause us to place the xenheap in a
higher block of memory than we would otherwise, which is generally
desirable.

Ian.

Re: [PATCH] xen/arm: Workaround for memory problem >1gb on last section

[Julien Grall]

Hi Frediano,

In general, we add in the subject the version of the subject (for
instance [PATCH v3]).

Sorry to ask you few others changes. The title doesn't seem suitable,
it's not a workaround (it would mean it's temporary) but an error in the
code.

On 10/02/2014 03:07 PM, Frediano Ziglio wrote:
> setup_xenheap_mappings setup head memory on Arm 32 has a limit of 1gb.
> On system with large memory is possible that there are no blocks of memory
> smaller than 1gb leading xenheap_pages to be more than 1gb.
> This cause memory errors trying to access heap after the 1gb limit.
> 
> Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com>
> ---
>  xen/arch/arm/setup.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 446de8a..e223d1b 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -509,6 +509,7 @@ static void __init setup_mm(unsigned long
> dtb_paddr, size_t dtb_size)
>      heap_pages = ram_pages;
>      xenheap_pages = (heap_pages/8 + 0x1fffUL) & ~0x1fffUL;
>      xenheap_pages = max(xenheap_pages, 128UL<<(20-PAGE_SHIFT));
> +    xenheap_pages = min(xenheap_pages, 1UL<<(30-PAGE_SHIFT));

Can you also update the comment above, saying we are requesting at most 1G?

With this 2 minors changes:

Reviewed-by: Julien Grall <julien.grall@linaro.org>

Regards,

-- 
Julien Grall