Re: [PATCH v3] xen/arm: Fix crash if last memory section is bigger than 1gb

Re: [PATCH v3] xen/arm: Fix crash if last memory section is bigger than 1gb

[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>
Reviewed-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/setup.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 446de8a..c43c776 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -500,7 +500,7 @@ static void __init setup_mm(unsigned long
dtb_paddr, size_t dtb_size)
      *
      *  - must be 32 MiB aligned
      *  - must not include Xen itself or the boot modules
-     *  - must be at most 1/8 the total RAM in the system
+     *  - must be at most 1GB or 1/8 the total RAM in the system if less
      *  - must be at least 128M
      *
      * We try to allocate the largest xenheap possible within these
@@ -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

Update comment and added Reviewed-by.

Frediano


2014-10-02 15:50 GMT+01:00 Julien Grall <julien.grall@linaro.org>:
> Hi Frediano,
>
> On 10/02/2014 03:39 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));
>>
>>      do
>>      {
>>
>
> You forgot to add my Reviewed-by (gave on the latest version) and
> address one thing.
>
> There is a big comment in the code explaining the different constraint
> on to find the heap. You have to update it.
>
> I would replace the line "- must be at most 1/8 the total RAM in the
> system" by " - must be at most 1GB or 1/8 the total RAM in the system if
> less".
>
> Regards,
>
> --
> Julien Grall

Re: [PATCH v3] xen/arm: Fix crash if last memory section is bigger than 1gb

[Ian Campbell]

On Thu, 2014-10-02 at 16:16 +0100, 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.

As I mentioned in a previous round of review this explanation doesn't
really fit.

I think better would be:

        On arm32 the xenheap has a maximum size of 1GB. On systems with
        more than 8GB (so 1/8 total RAM is greater than 1GB) there is no
        point in searching for a region with 1/8 of the total RAM when
        only 1GB will be used. Therefore limit the maximum size to 1GB
        before searching.

If you agree I will make this change upon commit.

> 
> Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com>
> Reviewed-by: Julien Grall <julien.grall@linaro.org>
> ---
>  xen/arch/arm/setup.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 446de8a..c43c776 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -500,7 +500,7 @@ static void __init setup_mm(unsigned long
> dtb_paddr, size_t dtb_size)
>       *
>       *  - must be 32 MiB aligned
>       *  - must not include Xen itself or the boot modules
> -     *  - must be at most 1/8 the total RAM in the system
> +     *  - must be at most 1GB or 1/8 the total RAM in the system if less
>       *  - must be at least 128M
>       *
>       * We try to allocate the largest xenheap possible within these
> @@ -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
>      {

Re: [PATCH v3] xen/arm: Fix crash if last memory section is bigger than 1gb

[Frediano Ziglio]

2014-10-03 10:09 GMT+01:00 Ian Campbell <Ian.Campbell@citrix.com>:
> On Thu, 2014-10-02 at 16:16 +0100, 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.
>
> As I mentioned in a previous round of review this explanation doesn't
> really fit.
>
> I think better would be:
>
>         On arm32 the xenheap has a maximum size of 1GB. On systems with
>         more than 8GB (so 1/8 total RAM is greater than 1GB) there is no
>         point in searching for a region with 1/8 of the total RAM when
>         only 1GB will be used. Therefore limit the maximum size to 1GB
>         before searching.
>
> If you agree I will make this change upon commit.
>

Agree. This explains the real reason of the change.

Frediano

>>
>> Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com>
>> Reviewed-by: Julien Grall <julien.grall@linaro.org>
>> ---
>>  xen/arch/arm/setup.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>> index 446de8a..c43c776 100644
>> --- a/xen/arch/arm/setup.c
>> +++ b/xen/arch/arm/setup.c
>> @@ -500,7 +500,7 @@ static void __init setup_mm(unsigned long
>> dtb_paddr, size_t dtb_size)
>>       *
>>       *  - must be 32 MiB aligned
>>       *  - must not include Xen itself or the boot modules
>> -     *  - must be at most 1/8 the total RAM in the system
>> +     *  - must be at most 1GB or 1/8 the total RAM in the system if less
>>       *  - must be at least 128M
>>       *
>>       * We try to allocate the largest xenheap possible within these
>> @@ -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
>>      {
>
>

Re: [PATCH v3] xen/arm: Fix crash if last memory section is bigger than 1gb

[Ian Campbell]

On Fri, 2014-10-03 at 11:15 +0100, Frediano Ziglio wrote:
> 2014-10-03 10:09 GMT+01:00 Ian Campbell <Ian.Campbell@citrix.com>:
> > On Thu, 2014-10-02 at 16:16 +0100, 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.
> >
> > As I mentioned in a previous round of review this explanation doesn't
> > really fit.
> >
> > I think better would be:
> >
> >         On arm32 the xenheap has a maximum size of 1GB. On systems with
> >         more than 8GB (so 1/8 total RAM is greater than 1GB) there is no
> >         point in searching for a region with 1/8 of the total RAM when
> >         only 1GB will be used. Therefore limit the maximum size to 1GB
> >         before searching.
> >
> > If you agree I will make this change upon commit.
> >
> 
> Agree. This explains the real reason of the change.

Acked + applied with this new text. Thanks.