ARM: issue with memory reservation from DT

3) If I apply below change - I can boot:

arch/arm/kernel/setup.c +1 -1

index c031063..85ad92b 100644
@@ -917,8 +917,8 @@ void __init setup_arch(char **cmdline_p)
 
        early_paging_init(mdesc, lookup_processor_type(read_cpuid_id()));
        setup_dma_zone(mdesc);
-       sanity_check_meminfo();
        arm_memblock_init(mdesc);
+       sanity_check_meminfo();
 
        paging_init(mdesc);
        request_standard_resources(mdesc);

^^ not sure if it totally safe, because dma_contiguous_reserve(arm_dma_limit); is called from inside arm_memblock_init() and it does bootmem allocations.

Sort Summary:
It looks like all static memory reservation and memory stealing's (calling of memblock_remove()) have to be done before any other operations and before calculating ARM memory limits.

Quoted 16 lines · Grygorii Strashko

3) If I apply below change - I can boot:

arch/arm/kernel/setup.c +1 -1

index c031063..85ad92b 100644
@@ -917,8 +917,8 @@ void __init setup_arch(char **cmdline_p)
 
         early_paging_init(mdesc, lookup_processor_type(read_cpuid_id()));
         setup_dma_zone(mdesc);
-       sanity_check_meminfo();
         arm_memblock_init(mdesc);
+       sanity_check_meminfo();
 
         paging_init(mdesc);
         request_standard_resources(mdesc);

Quoted 2 lines

^^ not sure if it totally safe, because dma_contiguous_reserve(arm_dma_limit); is called from inside arm_memblock_init() and it does bootmem allocations.

It isn't. sanity_check_meminfo() _must_ be called before arm_memblock_init() so that sanity_check_meminfo() can adjust the passed memory description to remove stuff which is inappropriate for the configuration, before it is passed to memblock.

Quoted 4 lines

Sort Summary:
It looks like all static memory reservation and memory stealing's (calling of memblock_remove()) have to be done before any other operations and before calculating ARM memory limits.

No, that should not be the case. The way it is /supposed/ to work is:

- We obtain the memory information and pass it into memblock
- We sanity check the memory in memblock, removing memory which we deem to be unacceptable for the kernel configuration via memblock_remove(). Also calculate the highest address we are prepared to allocate, which is set to the top of the first chunk of memory, or the top of lowmem.
- We then see about reserving memory from memblock. This marks memory as reserved, or in certain cases where we actually want to prevent the kernel taking control of the memory, we completely remove the memory from memblock (via memblock_remove).

Memory removed via memblock_remove() is then not available for any allocations, and should not be touched by the kernel in any way from that point on.

It doesn't matter that the memblock limit is still set higher, because the memory has been removed from the available memory pool, it should not be allocated.

Hi Russell,

Quoted 32 lines · Russell King - ARM Linux

Quoted 16 lines · Grygorii Strashko

3) If I apply below change - I can boot:

arch/arm/kernel/setup.c +1 -1

index c031063..85ad92b 100644
@@ -917,8 +917,8 @@ void __init setup_arch(char **cmdline_p)
 
          early_paging_init(mdesc, lookup_processor_type(read_cpuid_id()));
          setup_dma_zone(mdesc);
-       sanity_check_meminfo();
          arm_memblock_init(mdesc);
+       sanity_check_meminfo();
 
          paging_init(mdesc);
          request_standard_resources(mdesc);

Quoted 2 lines

^^ not sure if it totally safe, because dma_contiguous_reserve(arm_dma_limit); is called from inside arm_memblock_init() and it does bootmem allocations.

It isn't. sanity_check_meminfo() _must_ be called before arm_memblock_init() so that sanity_check_meminfo() can adjust the passed memory description to remove stuff which is inappropriate for the configuration, before it is passed to memblock.

Quoted 4 lines

Sort Summary:
It looks like all static memory reservation and memory stealing's (calling of memblock_remove()) have to be done before any other operations and before calculating ARM memory limits.

No, that should not be the case. The way it is /supposed/ to work is:

Quoted 10 lines

- We obtain the memory information and pass it into memblock
- We sanity check the memory in memblock, removing memory which we deem to be unacceptable for the kernel configuration via memblock_remove(). Also calculate the highest address we are prepared to allocate, which is set to the top of the first chunk of memory, or the top of lowmem.
- We then see about reserving memory from memblock. This marks memory as reserved, or in certain cases where we actually want to prevent the kernel taking control of the memory, we completely remove the memory from memblock (via memblock_remove).

In my case amount of removed memory is so high that there is no room for Highmem anymore.

memblock.memory.regions[0].base + size < arm_lowmem_limit and arm_lowmem_limit == memblock.current_limit

Quoted 9 lines

Memory removed via memblock_remove() is then not available for any allocations, and should not be touched by the kernel in any way from that point on.

It doesn't matter that the memblock limit is still set higher, because the memory has been removed from the available memory pool, it should not be allocated.

You are right in general, but seems problem is not in memblock itself :( The problem is with memory control variables like:
- arm_lowmem_limit
- max_low_pfn
- max_pfn

The last thing I've found that issue happens when in bootmem_init()->find_limits() the max_low variable got value greater than
max_high: max_low_pfn > max_pfn.

Then kernel crashes somewhere inside free_all_bootmem();

Below hack allows to boot:
+++ b/arch/arm/mm/init.c

Quoted 78 lines · Grygorii Strashko

Hi Russell,

Quoted 37 lines · Russell King - ARM Linux

Quoted 17 lines · Grygorii Strashko

3) If I apply below change - I can boot:

arch/arm/kernel/setup.c +1 -1

index c031063..85ad92b 100644
@@ -917,8 +917,8 @@ void __init setup_arch(char **cmdline_p)
 
          early_paging_init(mdesc, 
 lookup_processor_type(read_cpuid_id()));
          setup_dma_zone(mdesc);
-       sanity_check_meminfo();
          arm_memblock_init(mdesc);
+       sanity_check_meminfo();
 
          paging_init(mdesc);

          request_standard_resources(mdesc);

Quoted 4 lines

^^ not sure if it totally safe, because dma_contiguous_reserve(arm_dma_limit);
is called from inside arm_memblock_init() and it does bootmem allocations.

It isn't. sanity_check_meminfo() _must_ be called before arm_memblock_init()
so that sanity_check_meminfo() can adjust the passed memory description to
remove stuff which is inappropriate for the configuration, before it is passed to memblock.

Quoted 4 lines

Sort Summary:
It looks like all static memory reservation and memory stealing's (calling of memblock_remove()) have to be done before any other operations and before calculating ARM memory limits.

No, that should not be the case. The way it is /supposed/ to work is:

Quoted 10 lines

- We obtain the memory information and pass it into memblock
- We sanity check the memory in memblock, removing memory which we deem to be unacceptable for the kernel configuration via memblock_remove(). Also calculate the highest address we are prepared to allocate, which is set to the top of the first chunk of memory, or the top of lowmem.
- We then see about reserving memory from memblock. This marks memory as reserved, or in certain cases where we actually want to prevent the kernel taking control of the memory, we completely remove the memory from memblock (via memblock_remove).

In my case amount of removed memory is so high that there is no room for Highmem anymore.

memblock.memory.regions[0].base + size < arm_lowmem_limit and arm_lowmem_limit == memblock.current_limit

Quoted 9 lines

Memory removed via memblock_remove() is then not available for any allocations, and should not be touched by the kernel in any way from that point on.

It doesn't matter that the memblock limit is still set higher, because the memory has been removed from the available memory pool, it should not be allocated.

You are right in general, but seems problem is not in memblock itself :( The problem is with memory control variables like:
- arm_lowmem_limit
- max_low_pfn
- max_pfn

The last thing I've found that issue happens when in bootmem_init()->find_limits() the max_low variable got value greater than
max_high: max_low_pfn > max_pfn.

Then kernel crashes somewhere inside free_all_bootmem();

Review static void __init find_limits(unsigned long *min, · line 146

Quoted 10 lines

Below hack allows to boot:
+++ b/arch/arm/mm/init.c

(quoted diff) +2 -0

@@ -140,6 +140,8 @@ static void __init find_limits(unsigned long *min, 
 unsigned long *max_low,
         *max_low = PFN_DOWN(memblock_get_current_limit());
         *min = PFN_UP(memblock_start_of_DRAM());
         *max_high = PFN_DOWN(memblock_end_of_DRAM());
+       if (*max_low > *max_high)
+               *max_low = *max_high;
  }

Can you print out the values of max_low, min, max_high and call __memblock_dump_all() at this point in time? It would also be helpful to get the pr_debug print outs in drivers/of/of_reserved_mem.c to see what's being reserved there.

FWIW, I've tested something similar and it worked correctly but there are at least two big differences in the case here
1) Removing the top of memory instead of just a block in the middle
2) Working above 32 bit ranges.

Eliminating #2 here may be difficult but can you try just removing 256MB instead of 512MB? (reg = <0x8 0x20000000 0x0 0x10000000>)

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 :(

Quoted 24 lines · Grygorii Strashko

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]

Quoted 2 lines

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.

Quoted 25 lines · Laura Abbott

Quoted 24 lines · Grygorii Strashko

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]

Quoted 7 lines

Quoted 2 lines

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:

arch/arm/mm/mmu.c +3 -0

index 9f98cec..6696016 100644
@@ -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.

Hi Laura,

Quoted 57 lines · Laura Abbott

Quoted 25 lines · Laura Abbott

Quoted 24 lines · Grygorii Strashko

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]

Quoted 7 lines

Quoted 2 lines

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:

arch/arm/mm/mmu.c +3 -0

index 9f98cec..6696016 100644
@@ -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;
 
          /*

Quoted 2 lines

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:

(quoted diff) +0 -9

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

arch/arm/mm/init.c +6 -1

@@ -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;
+

Hi Russell,

Quoted 32 lines · Russell King - ARM Linux

Quoted 16 lines · Grygorii Strashko

3) If I apply below change - I can boot:

arch/arm/kernel/setup.c +1 -1

index c031063..85ad92b 100644
@@ -917,8 +917,8 @@ void __init setup_arch(char **cmdline_p)
 
          early_paging_init(mdesc, lookup_processor_type(read_cpuid_id()));
          setup_dma_zone(mdesc);
-       sanity_check_meminfo();
          arm_memblock_init(mdesc);
+       sanity_check_meminfo();
 
          paging_init(mdesc);
          request_standard_resources(mdesc);

Quoted 2 lines

^^ not sure if it totally safe, because dma_contiguous_reserve(arm_dma_limit); is called from inside arm_memblock_init() and it does bootmem allocations.

It isn't. sanity_check_meminfo() _must_ be called before arm_memblock_init() so that sanity_check_meminfo() can adjust the passed memory description to remove stuff which is inappropriate for the configuration, before it is passed to memblock.

Quoted 4 lines

Sort Summary:
It looks like all static memory reservation and memory stealing's (calling of memblock_remove()) have to be done before any other operations and before calculating ARM memory limits.

No, that should not be the case. The way it is /supposed/ to work is:

Quoted 10 lines

- We obtain the memory information and pass it into memblock
- We sanity check the memory in memblock, removing memory which we deem to be unacceptable for the kernel configuration via memblock_remove(). Also calculate the highest address we are prepared to allocate, which is set to the top of the first chunk of memory, or the top of lowmem.
- We then see about reserving memory from memblock. This marks memory as reserved, or in certain cases where we actually want to prevent the kernel taking control of the memory, we completely remove the memory from memblock (via memblock_remove).

In my case amount of removed memory is so high that there is no room for Highmem anymore.

memblock.memory.regions[0].base + size < arm_lowmem_limit and arm_lowmem_limit == memblock.current_limit

Quoted 9 lines

Memory removed via memblock_remove() is then not available for any allocations, and should not be touched by the kernel in any way from that point on.

It doesn't matter that the memblock limit is still set higher, because the memory has been removed from the available memory pool, it should not be allocated.

You are right in general, but seems problem is not in memblock itself :( The problem is with memory control variables like:
- arm_lowmem_limit
- max_low_pfn
- max_pfn

The last thing I've found that issue happens when in bootmem_init()->find_limits() the max_low variable got value greater than
max_high: max_low_pfn > max_pfn.

Quoted 72 lines · Grygorii Strashko

Hi Russell,

Quoted 37 lines · Russell King - ARM Linux

Quoted 17 lines · Grygorii Strashko

3) If I apply below change - I can boot:

arch/arm/kernel/setup.c +1 -1

index c031063..85ad92b 100644
@@ -917,8 +917,8 @@ void __init setup_arch(char **cmdline_p)
 
          early_paging_init(mdesc, 
 lookup_processor_type(read_cpuid_id()));
          setup_dma_zone(mdesc);
-       sanity_check_meminfo();
          arm_memblock_init(mdesc);
+       sanity_check_meminfo();
 
          paging_init(mdesc);

          request_standard_resources(mdesc);

Quoted 4 lines

^^ not sure if it totally safe, because dma_contiguous_reserve(arm_dma_limit);
is called from inside arm_memblock_init() and it does bootmem allocations.

It isn't. sanity_check_meminfo() _must_ be called before arm_memblock_init()
so that sanity_check_meminfo() can adjust the passed memory description to
remove stuff which is inappropriate for the configuration, before it is passed to memblock.

Quoted 4 lines

Sort Summary:
It looks like all static memory reservation and memory stealing's (calling of memblock_remove()) have to be done before any other operations and before calculating ARM memory limits.

No, that should not be the case. The way it is /supposed/ to work is:

Quoted 10 lines

- We obtain the memory information and pass it into memblock
- We sanity check the memory in memblock, removing memory which we deem to be unacceptable for the kernel configuration via memblock_remove(). Also calculate the highest address we are prepared to allocate, which is set to the top of the first chunk of memory, or the top of lowmem.
- We then see about reserving memory from memblock. This marks memory as reserved, or in certain cases where we actually want to prevent the kernel taking control of the memory, we completely remove the memory from memblock (via memblock_remove).

In my case amount of removed memory is so high that there is no room for Highmem anymore.

memblock.memory.regions[0].base + size < arm_lowmem_limit and arm_lowmem_limit == memblock.current_limit

Quoted 9 lines

Memory removed via memblock_remove() is then not available for any allocations, and should not be touched by the kernel in any way from that point on.

It doesn't matter that the memblock limit is still set higher, because the memory has been removed from the available memory pool, it should not be allocated.

You are right in general, but seems problem is not in memblock itself :( The problem is with memory control variables like:
- arm_lowmem_limit
- max_low_pfn
- max_pfn

Quoted 4 lines

The last thing I've found that issue happens when in bootmem_init()->find_limits() the max_low variable got value greater than
max_high: max_low_pfn > max_pfn.

Without getting too much into details, I don't see much point to let kernel know about memory and then just to remove a huge block of it which is it never gonna see it. It creates hole in Linear memory which you can avoid by doing that memory partition and letting kernel know about memory which it needs to deal with it.

If you are just playing around then its fine.

Oh. Yes you are right in general - I've just not expected from kernel to crash silently, so my intention was to report issue first of all.

Quoted 2 lines · Grygorii Strashko

Oh. Yes you are right in general - I've just not expected from kernel to crash silently, so my intention was to report issue first of all.

The problem here is that each time someone finds a new way to break it, the code gets more complicated, which means there's yet more ways to break it.

As an example, try reading through sanity_check_meminfo() and working out exactly what each variable in there does - it'll take some considerable time to work it out. It /used/ to be pretty obvious.

It's pretty scary too that it's walking a list of memblock regions, while modifying that list, potentially removing the entry that it's currently at. I suspect that would cause it to skip a region given how the for() loop works.

Either way, I think we need to get some of this stuff back to being coded in a simple and obvious-to-understand manner.