ARM: issue with memory reservation from DT

Commit Message

Grygorii Strashko Oct. 21, 2014, 5:01 p.m. UTC

Hi Laura,

On 10/20/2014 11:48 PM, Laura Abbott wrote:
> On 10/17/2014 9:54 AM, Laura Abbott wrote:
>> On 10/17/2014 3:21 AM, Grygorii Strashko wrote:
>>> Hi Laura,
>>>
>>> As I mentioned in first e-mail I've 1G Mem node initially:
>>>    reg = <0x8 0x00000000 0x0 0x40000000>;
>>>
>>> and have memory reservation of 512M in the upper part of memory:
>>>    reserved-memory {
>>>     reg = <0x8 0x20000000 0x0 0x20000000>;
>>>
>>> then in sanity_check_meminfo() initial mem configuration calculated as
>>> following:
>>>
>>> [    0.000000] ======= memblock_limit=0x000000082f800000
>>> arm_lowmem_limit=0x000000082f800000 vmalloc_limit=ef800000
>>> high_memory=0x000000082f800000
>>> and memblock.current_limit == arm_lowmem_limit=0x000000082f800000
>>>
>>> then in arm_memblock_init()->early_init_fdt_scan_reserved_mem() 512M
>>> of memory removed
>>> (not reserved!, because "no-map;" is defined).
>>>
>>> After that Kernel will have only 512M of accessible memory
>>>   memory[0x0]    [0x00000800000000-0x0000081fffffff]
>>>
>>> I've checked of_reserved_mem.c and saw no issues there :(
>>>
>>
>> Yes, I suspect the issue is not with of_reserved_mem.c and instead with
>> sanity_check_meminfo in mmu.c . I'm still traveling so I'll probably
>> take a look on Monday unless I find some time sooner.
>>
> 
> I was able to reproduce a crash on my device by removing all highmem
> as well. It looks like the logic assumes that lowmem limit will only
> ever increase and not need to decrease. This seems like a limitation
> of running with CONFIG_HIGHMEM on a system which doesn't actually
> need highmem. This seems to have been the case even before the meminfo
> removal as well. The following worked for me:
> 
> diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
> index 9f98cec..6696016 100644
> --- a/arch/arm/mm/mmu.c
> +++ b/arch/arm/mm/mmu.c
> @@ -1140,6 +1140,9 @@ void __init sanity_check_meminfo(void)
>                  }
>          }
> 
> +       if (arm_lowmem_limit > memblock_end_of_DRAM())
> +               arm_lowmem_limit = memblock_end_of_DRAM();
> +
>          high_memory = __va(arm_lowmem_limit - 1) + 1;
> 
>          /*
> 
> 
> I'll turn this into an official patch for review if it fixes your
> problem as well.

thanks you for your comments.

No. It doesn't help :( because you've fixed sanity_check_meminfo()
while I've the case when memory is removed (stolen) from arm_memblock_init()
which, in turn, called after sanity_check_meminfo() - see setup_arch().

Below is last things I've found - It seems related to memZones configuration
and in my case CONFIG_ZONE_DMA=y:
== bad case:
[    0.000000] ======= memblock_limit=0x000000082f800000 arm_lowmem_limit=0x000000082f800000 vmalloc_limit=ef800000 high_memory=0x000000082f800000
[    0.000000] ======= min_low_pfn=800000 max_low_pfn=82F800 max_pfn=820000
[    0.000000] ======= zone0 size2F800 holeF800
[    0.000000] ======= zone1 size0 hole0
[    0.000000] ======= zone2 sizeFFFF0800 holeFFFF0800
[    0.000000] ======= zone3 size0 hole0

== good case - can boot (with below fix applied):
[    0.000000] ======= memblock_limit=0x000000082f800000 arm_lowmem_limit=0x000000082f800000 vmalloc_limit=ef800000 high_memory=0x000000082f800000
[    0.000000] ======= min_low_pfn=800000 max_low_pfn=820000 max_pfn=820000
[    0.000000] ======= zone0 size20000 hole0
[    0.000000] ======= zone1 size0 hole0
[    0.000000] ======= zone2 size0 hole0
[    0.000000] ======= zone3 size0 hole0

Also I've found, that before commit "ARM: 8025/1: Get rid of meminfo"
the 'max_low_pfn' was calculated as below:

-       struct meminfo *mi = &meminfo;
-       int i;
-
-       /* This assumes the meminfo array is properly sorted */
-       *min = bank_pfn_start(&mi->bank[0]);
-       for_each_bank (i, mi)
-               if (mi->bank[i].highmem)
-                               break;
-       *max_low = bank_pfn_end(&mi->bank[i - 1]);

So, I've tried to roll back above functionality and I was able to boot with below change:

In the above code I've tried to take into account, that at the moment
when find_limits() is called memory structure can have following
configurations (may be I've listed not all of them):

1) mmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmm
                       ^
2) mmmmmmmmmmmmmmmmmmmmmmmmmmmmhhhh
                       ^

3) mmmmmmmmmmmmmmmmhhhhhhhhhhhhhhhh (my case)
                       ^

4) mmmmhhhhmmmmmmmmmmmmmmmmmmmmmmmm
                       ^

5) mmmmhhhhmmmmmmmmmmmmmmmmmhhhhmmm
                       ^

6) mmmmmmmmmmmmmmmmhhhhhhhhmmmmmmmm
                       ^
m - available memory
h - hole
^ - position of arm_lowmem_limit/memblock.current_limit & high_memory

Also, Might be we can get rid of arm_lowmem_limit and
replace it with memblock_get_current_limit? Could we?

Unfortunately, this issue has low priority for me
and I'm not sure that I'll be able to continue working on :(

Best regards,
-grygorii

Comments

Laura Abbott Oct. 21, 2014, 6:32 p.m. UTC | #1

On 10/21/2014 10:01 AM, Grygorii Strashko wrote:
> Hi Laura,
>
> On 10/20/2014 11:48 PM, Laura Abbott wrote:
>> On 10/17/2014 9:54 AM, Laura Abbott wrote:
>>> On 10/17/2014 3:21 AM, Grygorii Strashko wrote:
>>>> Hi Laura,
>>>>
>>>> As I mentioned in first e-mail I've 1G Mem node initially:
>>>>     reg = <0x8 0x00000000 0x0 0x40000000>;
>>>>
>>>> and have memory reservation of 512M in the upper part of memory:
>>>>     reserved-memory {
>>>>      reg = <0x8 0x20000000 0x0 0x20000000>;
>>>>
>>>> then in sanity_check_meminfo() initial mem configuration calculated as
>>>> following:
>>>>
>>>> [    0.000000] ======= memblock_limit=0x000000082f800000
>>>> arm_lowmem_limit=0x000000082f800000 vmalloc_limit=ef800000
>>>> high_memory=0x000000082f800000
>>>> and memblock.current_limit == arm_lowmem_limit=0x000000082f800000
>>>>
>>>> then in arm_memblock_init()->early_init_fdt_scan_reserved_mem() 512M
>>>> of memory removed
>>>> (not reserved!, because "no-map;" is defined).
>>>>
>>>> After that Kernel will have only 512M of accessible memory
>>>>    memory[0x0]    [0x00000800000000-0x0000081fffffff]
>>>>
>>>> I've checked of_reserved_mem.c and saw no issues there :(
>>>>
>>>
>>> Yes, I suspect the issue is not with of_reserved_mem.c and instead with
>>> sanity_check_meminfo in mmu.c . I'm still traveling so I'll probably
>>> take a look on Monday unless I find some time sooner.
>>>
>>
>> I was able to reproduce a crash on my device by removing all highmem
>> as well. It looks like the logic assumes that lowmem limit will only
>> ever increase and not need to decrease. This seems like a limitation
>> of running with CONFIG_HIGHMEM on a system which doesn't actually
>> need highmem. This seems to have been the case even before the meminfo
>> removal as well. The following worked for me:
>>
>> diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
>> index 9f98cec..6696016 100644
>> --- a/arch/arm/mm/mmu.c
>> +++ b/arch/arm/mm/mmu.c
>> @@ -1140,6 +1140,9 @@ void __init sanity_check_meminfo(void)
>>                   }
>>           }
>>
>> +       if (arm_lowmem_limit > memblock_end_of_DRAM())
>> +               arm_lowmem_limit = memblock_end_of_DRAM();
>> +
>>           high_memory = __va(arm_lowmem_limit - 1) + 1;
>>
>>           /*
>>
>>
>> I'll turn this into an official patch for review if it fixes your
>> problem as well.
>
> thanks you for your comments.
>
> No. It doesn't help :( because you've fixed sanity_check_meminfo()
> while I've the case when memory is removed (stolen) from arm_memblock_init()
> which, in turn, called after sanity_check_meminfo() - see setup_arch().
>

Okay, yes you are absolutely correct. I didn't read Russell's last
e-mail closely enough and was testing off of a different baseline
where the sanity_check_meminfo did actually fix.

> Below is last things I've found - It seems related to memZones configuration
> and in my case CONFIG_ZONE_DMA=y:
> == bad case:
> [    0.000000] ======= memblock_limit=0x000000082f800000 arm_lowmem_limit=0x000000082f800000 vmalloc_limit=ef800000 high_memory=0x000000082f800000
> [    0.000000] ======= min_low_pfn=800000 max_low_pfn=82F800 max_pfn=820000
> [    0.000000] ======= zone0 size2F800 holeF800
> [    0.000000] ======= zone1 size0 hole0
> [    0.000000] ======= zone2 sizeFFFF0800 holeFFFF0800
> [    0.000000] ======= zone3 size0 hole0
>
> == good case - can boot (with below fix applied):
> [    0.000000] ======= memblock_limit=0x000000082f800000 arm_lowmem_limit=0x000000082f800000 vmalloc_limit=ef800000 high_memory=0x000000082f800000
> [    0.000000] ======= min_low_pfn=800000 max_low_pfn=820000 max_pfn=820000
> [    0.000000] ======= zone0 size20000 hole0
> [    0.000000] ======= zone1 size0 hole0
> [    0.000000] ======= zone2 size0 hole0
> [    0.000000] ======= zone3 size0 hole0
>
> Also I've found, that before commit "ARM: 8025/1: Get rid of meminfo"
> the 'max_low_pfn' was calculated as below:
>
> -       struct meminfo *mi = &meminfo;
> -       int i;
> -
> -       /* This assumes the meminfo array is properly sorted */
> -       *min = bank_pfn_start(&mi->bank[0]);
> -       for_each_bank (i, mi)
> -               if (mi->bank[i].highmem)
> -                               break;
> -       *max_low = bank_pfn_end(&mi->bank[i - 1]);
>
> So, I've tried to roll back above functionality and I was able to boot with below change:
> --- a/arch/arm/mm/init.c
> +++ b/arch/arm/mm/init.c
> @@ -137,7 +137,19 @@ void show_mem(unsigned int filter)
>   static void __init find_limits(unsigned long *min, unsigned long *max_low,
>                                 unsigned long *max_high)
>   {
> -       *max_low = PFN_DOWN(memblock_get_current_limit());
> +       struct memblock_region *reg;
> +
> +       for_each_memblock(memory, reg) {
> +               if (reg->base >= memblock_get_current_limit())
> +                       break;
> +
> +               if ((reg->base + reg->size) > memblock_get_current_limit()) {
> +                       *max_low = PFN_DOWN(memblock_get_current_limit());
> +                       break;
> +               }
> +
> +               *max_low = PFN_DOWN(reg->base + reg->size);
> +       }
>          *min = PFN_UP(memblock_start_of_DRAM());
>          *max_high = PFN_DOWN(memblock_end_of_DRAM());
>   }
>

Yes, I think you've narrowed down the problem well. It seems like this
could be simplified though to

*max_low = min(PFN_DOWN(memblock_get_current_limit()),
			memblock_end_of_DRAM())

I haven't tested that though.

Patch

--- a/arch/arm/mm/init.c
+++ b/arch/arm/mm/init.c
@@ -137,7 +137,19 @@  void show_mem(unsigned int filter)
 static void __init find_limits(unsigned long *min, unsigned long *max_low,
                               unsigned long *max_high)
 {
-       *max_low = PFN_DOWN(memblock_get_current_limit());
+       struct memblock_region *reg;
+
+       for_each_memblock(memory, reg) {
+               if (reg->base >= memblock_get_current_limit())
+                       break;
+
+               if ((reg->base + reg->size) > memblock_get_current_limit()) {
+                       *max_low = PFN_DOWN(memblock_get_current_limit());
+                       break;
+               }
+
+               *max_low = PFN_DOWN(reg->base + reg->size);
+       }
        *min = PFN_UP(memblock_start_of_DRAM());
        *max_high = PFN_DOWN(memblock_end_of_DRAM());
 }