MIPS: fix incorrect mem=X@Y handling

Change 73fbc1eba7ff added a fix to ensure that the memory range between PHYS_OFFSET and low memory address specified by mem= cmdline argument is not later processed by free_all_bootmem.
This change was incorrect for systems where the commandline specifies more than 1 mem argument, as it will cause all memory between PHYS_OFFSET and each of the memory offsets to be marked as reserved, which results in parts of the RAM marked as reserved (Creator CI20's u-boot has a default commandline argument 'mem=256M@0x0 mem=768M@0x30000000').

Change the behaviour to ensure that only the range between PHYS_OFFSET and the lowest start address of the memories is marked as protected.

This change also ensures that the range is marked protected even if it's only defined through the devicetree and not only via commandline arguments.

Reported-by Mathieu Malaterre <[email protected]>

Signed-off-by Marcin Nowakowski <[email protected]>

Fixes 73fbc1eba7ff ("MIPS: fix mem=X@Y commandline processing")

Cc [email protected]

---
 arch/mips/kernel/setup.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

arch/mips/kernel/setup.c +16 -4

index fe39397..a1c39ec 100644
@@ -374,6 +374,7 @@ static void __init bootmem_init(void)
        unsigned long reserved_end;
        unsigned long mapstart = ~0UL;
        unsigned long bootmap_size;
+       phys_addr_t ramstart = ~0UL;
        bool bootmap_valid = false;
        int i;
 

@@ -394,6 +395,21 @@ static void __init bootmem_init(void)
        max_low_pfn = 0;
 
        /*
+        * Reserve any memory between the start of RAM and PHYS_OFFSET
+        */
+       for (i = 0; i < boot_mem_map.nr_map; i++) {
+               if (boot_mem_map.map[i].type != BOOT_MEM_RAM)
+                       continue;
+
+               ramstart = min(ramstart, boot_mem_map.map[i].addr);
+       }
+
+       if (ramstart > PHYS_OFFSET)
+               add_memory_region(PHYS_OFFSET, ramstart - PHYS_OFFSET,
+                                 BOOT_MEM_RESERVED);
+
+
+       /*
         * Find the highest page frame number we have available.
         */
        for (i = 0; i < boot_mem_map.nr_map; i++) {

@@ -663,9 +679,6 @@ static int __init early_parse_mem(char *p)
 
        add_memory_region(start, size, BOOT_MEM_RAM);
 
-       if (start && start > PHYS_OFFSET)
-               add_memory_region(PHYS_OFFSET, start - PHYS_OFFSET,
-                               BOOT_MEM_RESERVED);
        return 0;
 }
 early_param("mem", early_parse_mem);
--

2.7.4

From: Marcin Nowakowski <[email protected]>

Change 73fbc1eba7ff added a fix to ensure that the memory range between PHYS_OFFSET and low memory address specified by mem= cmdline argument is not later processed by free_all_bootmem.
This change was incorrect for systems where the commandline specifies more than 1 mem argument, as it will cause all memory between PHYS_OFFSET and each of the memory offsets to be marked as reserved, which results in parts of the RAM marked as reserved (Creator CI20's u-boot has a default commandline argument 'mem=256M@0x0 mem=768M@0x30000000').

Change the behaviour to ensure that only the range between PHYS_OFFSET and the lowest start address of the memories is marked as protected.

This change also ensures that the range is marked protected even if it's only defined through the devicetree and not only via commandline arguments.

Reported-by Mathieu Malaterre <[email protected]>

Signed-off-by Marcin Nowakowski <[email protected]>

Fixes 73fbc1eba7ff ("MIPS: fix mem=X@Y commandline processing")

Cc <[email protected]> # v4.11

---
v2: Use updated email adress, add tag for stable.

  arch/mips/kernel/setup.c | 19 ++++++++++++++++---
  1 file changed, 16 insertions(+), 3 deletions(-)

arch/mips/kernel/setup.c +16 -3

index 702c678de116..f19d61224c71 100644
@@ -375,6 +375,7 @@ static void __init bootmem_init(void)
  	unsigned long reserved_end;
  	unsigned long mapstart = ~0UL;
  	unsigned long bootmap_size;
+	phys_addr_t ramstart = ~0UL;
  	bool bootmap_valid = false;
  	int i;
 

@@ -395,6 +396,21 @@ static void __init bootmem_init(void)
  	max_low_pfn = 0;
 
  	/*
+	 * Reserve any memory between the start of RAM and PHYS_OFFSET
+	 */
+	for (i = 0; i < boot_mem_map.nr_map; i++) {
+		if (boot_mem_map.map[i].type != BOOT_MEM_RAM)
+			continue;
+
+		ramstart = min(ramstart, boot_mem_map.map[i].addr);
+	}
+
+	if (ramstart > PHYS_OFFSET)
+		add_memory_region(PHYS_OFFSET, ramstart - PHYS_OFFSET,
+				  BOOT_MEM_RESERVED);
+
+
+	/*
  	 * Find the highest page frame number we have available.
  	 */
  	for (i = 0; i < boot_mem_map.nr_map; i++) {

@@ -664,9 +680,6 @@ static int __init early_parse_mem(char *p)
 
  	add_memory_region(start, size, BOOT_MEM_RAM);
 
-	if (start && start > PHYS_OFFSET)
-		add_memory_region(PHYS_OFFSET, start - PHYS_OFFSET,
-				BOOT_MEM_RESERVED);
  	return 0;
  }
  early_param("mem", early_parse_mem);

From: Marcin Nowakowski <[email protected]>

Change 73fbc1eba7ff added a fix to ensure that the memory range between

PHYS_OFFSET and low memory address specified by mem= cmdline argument is not later processed by free_all_bootmem.
This change was incorrect for systems where the commandline specifies more than 1 mem argument, as it will cause all memory between PHYS_OFFSET and each of the memory offsets to be marked as reserved, which results in parts of the RAM marked as reserved (Creator CI20's u-boot has a default commandline argument 'mem=256M@0x0 mem=768M@0x30000000').

Change the behaviour to ensure that only the range between PHYS_OFFSET and the lowest start address of the memories is marked as protected.

This change also ensures that the range is marked protected even if it's only defined through the devicetree and not only via commandline arguments.

Reported-by Mathieu Malaterre <[email protected]>

Signed-off-by Marcin Nowakowski <[email protected]>

Fixes 73fbc1eba7ff ("MIPS: fix mem=X@Y commandline processing")

Cc <[email protected]> # v4.11

---
v2: Use updated email adress, add tag for stable.

 arch/mips/kernel/setup.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

arch/mips/kernel/setup.c +0 -0

index 702c678de116..f19d61224c71 100644

arch/mips/kernel/setup.c +0 -0

 	bool bootmap_valid = false;
 	int i;
 

arch/mips/kernel/setup.c +3 -3

+
+
+	/*
 	 * Find the highest page frame number we have available.
 	 */
 	for (i = 0; i < boot_mem_map.nr_map; i++) {

@@ -664,9 +680,6 @@ static int __init early_parse_mem(char *p)
 
 	add_memory_region(start, size, BOOT_MEM_RAM);
 
-	if (start && start > PHYS_OFFSET)
-		add_memory_region(PHYS_OFFSET, start - PHYS_OFFSET,
-				BOOT_MEM_RESERVED);
 	return 0;
 }
 early_param("mem", early_parse_mem);

Signature · 1 line

2.11.0

Quoted 3 lines · Mathieu Malaterre

From: Marcin Nowakowski <[email protected]>

Change 73fbc1eba7ff added a fix to ensure that the memory range between

Please refer to commits with e.g. commit 73fbc1eba7ff ("MIPS: fix mem=X@Y commandline processing").

Quoted 16 lines

PHYS_OFFSET and low memory address specified by mem= cmdline argument is not later processed by free_all_bootmem.
This change was incorrect for systems where the commandline specifies more than 1 mem argument, as it will cause all memory between PHYS_OFFSET and each of the memory offsets to be marked as reserved, which results in parts of the RAM marked as reserved (Creator CI20's u-boot has a default commandline argument 'mem=256M@0x0 mem=768M@0x30000000').

Change the behaviour to ensure that only the range between PHYS_OFFSET and the lowest start address of the memories is marked as protected.

This change also ensures that the range is marked protected even if it's only defined through the devicetree and not only via commandline arguments.

Quoted 4 lines

Reported-by Mathieu Malaterre <[email protected]>

Signed-off-by Marcin Nowakowski <[email protected]>

Fixes 73fbc1eba7ff ("MIPS: fix mem=X@Y commandline processing")

Cc <[email protected]> # v4.11

I'm guessing that should technically be v4.11+

Quoted 10 lines

---
v2: Use updated email adress, add tag for stable.

 arch/mips/kernel/setup.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

arch/mips/kernel/setup.c +0 -0

index 702c678de116..f19d61224c71 100644

arch/mips/kernel/setup.c static void __init bootmem_init(void) · line 378

Quoted 4 lines

arch/mips/kernel/setup.c +1 -0

@@ -375,6 +375,7 @@ static void __init bootmem_init(void)
      unsigned long reserved_end;
      unsigned long mapstart = ~0UL;
      unsigned long bootmap_size;
+     phys_addr_t ramstart = ~0UL;

Although practically it might not matter, technically phys_addr_t may be 64-bits (CONFIG_PHYS_ADDR_T_64BIT) even on a 32-bit kernels, in which case ~0UL may not be sufficiently large.

Maybe that should be ~(phys_addr_t)0, or perhaps (phys_addr_t)ULLONG_MAX to match add_memory_region().

Quoted 4 lines

arch/mips/kernel/setup.c +0 -0

      bool bootmap_valid = false;
      int i;
 

arch/mips/kernel/setup.c static void __init bootmem_init(void) · line 405

Quoted 10 lines

arch/mips/kernel/setup.c +7 -0

@@ -395,6 +396,21 @@ static void __init bootmem_init(void)
      max_low_pfn = 0;
 
      /*
+      * Reserve any memory between the start of RAM and PHYS_OFFSET
+      */
+     for (i = 0; i < boot_mem_map.nr_map; i++) {
+             if (boot_mem_map.map[i].type != BOOT_MEM_RAM)
+                     continue;
+
+             ramstart = min(ramstart, boot_mem_map.map[i].addr);

Is it worth incorporating this into the existing loop below ...

arch/mips/kernel/setup.c static void __init bootmem_init(void) · line 410

Quoted 5 lines

arch/mips/kernel/setup.c +5 -0

+     }
+
+     if (ramstart > PHYS_OFFSET)
+             add_memory_region(PHYS_OFFSET, ramstart - PHYS_OFFSET,
+                               BOOT_MEM_RESERVED);

... and this then placed below that loop?

Otherwise I can't find fault with this patch, though i'm not intimately familiar with bootmem.

Cheers
James

Quoted 19 lines

arch/mips/kernel/setup.c +3 -4

+
+
+     /*
       * Find the highest page frame number we have available.
       */
      for (i = 0; i < boot_mem_map.nr_map; i++) {

@@ -664,9 +680,6 @@ static int __init early_parse_mem(char *p)
 
      add_memory_region(start, size, BOOT_MEM_RAM);
 
-     if (start && start > PHYS_OFFSET)
-             add_memory_region(PHYS_OFFSET, start - PHYS_OFFSET,
-                             BOOT_MEM_RESERVED);
      return 0;
 }
 early_param("mem", early_parse_mem);
--

2.11.0

Since it's been a week, could you confirm the patch is ok as-is or do you think some comment(s) from James should be incorporated ?

Quoted 24 lines · Mathieu Malaterre

Quoted 3 lines

From: Marcin Nowakowski <[email protected]>

Change 73fbc1eba7ff added a fix to ensure that the memory range between

Please refer to commits with e.g. commit 73fbc1eba7ff ("MIPS: fix mem=X@Y commandline processing").

Quoted 16 lines

PHYS_OFFSET and low memory address specified by mem= cmdline argument is not later processed by free_all_bootmem.
This change was incorrect for systems where the commandline specifies more than 1 mem argument, as it will cause all memory between PHYS_OFFSET and each of the memory offsets to be marked as reserved, which results in parts of the RAM marked as reserved (Creator CI20's u-boot has a default commandline argument 'mem=256M@0x0 mem=768M@0x30000000').

Change the behaviour to ensure that only the range between PHYS_OFFSET and the lowest start address of the memories is marked as protected.

This change also ensures that the range is marked protected even if it's only defined through the devicetree and not only via commandline arguments.

Quoted 6 lines

Quoted 4 lines

Reported-by Mathieu Malaterre <[email protected]>

Signed-off-by Marcin Nowakowski <[email protected]>

Fixes 73fbc1eba7ff ("MIPS: fix mem=X@Y commandline processing")

Cc <[email protected]> # v4.11

I'm guessing that should technically be v4.11+

My fault, if this is the only change, I can re-submit.

Quoted 72 lines

Quoted 10 lines

---
v2: Use updated email adress, add tag for stable.

  arch/mips/kernel/setup.c | 19 ++++++++++++++++---
  1 file changed, 16 insertions(+), 3 deletions(-)

arch/mips/kernel/setup.c +0 -0

index 702c678de116..f19d61224c71 100644

arch/mips/kernel/setup.c static void __init bootmem_init(void) · line 378

Quoted 4 lines

arch/mips/kernel/setup.c +1 -0

@@ -375,6 +375,7 @@ static void __init bootmem_init(void)
       unsigned long reserved_end;
       unsigned long mapstart = ~0UL;
       unsigned long bootmap_size;
+     phys_addr_t ramstart = ~0UL;

Although practically it might not matter, technically phys_addr_t may be 64-bits (CONFIG_PHYS_ADDR_T_64BIT) even on a 32-bit kernels, in which case ~0UL may not be sufficiently large.

Maybe that should be ~(phys_addr_t)0, or perhaps (phys_addr_t)ULLONG_MAX to match add_memory_region().

Quoted 4 lines

arch/mips/kernel/setup.c +0 -0

       bool bootmap_valid = false;
       int i;
 

arch/mips/kernel/setup.c static void __init bootmem_init(void) · line 405

Quoted 10 lines

arch/mips/kernel/setup.c +7 -0

@@ -395,6 +396,21 @@ static void __init bootmem_init(void)
       max_low_pfn = 0;
 
       /*
+      * Reserve any memory between the start of RAM and PHYS_OFFSET
+      */
+     for (i = 0; i < boot_mem_map.nr_map; i++) {
+             if (boot_mem_map.map[i].type != BOOT_MEM_RAM)
+                     continue;
+
+             ramstart = min(ramstart, boot_mem_map.map[i].addr);

Is it worth incorporating this into the existing loop below ...

arch/mips/kernel/setup.c static void __init bootmem_init(void) · line 410

Quoted 5 lines

arch/mips/kernel/setup.c +5 -0

+     }
+
+     if (ramstart > PHYS_OFFSET)
+             add_memory_region(PHYS_OFFSET, ramstart - PHYS_OFFSET,
+                               BOOT_MEM_RESERVED);

... and this then placed below that loop?

Otherwise I can't find fault with this patch, though i'm not intimately familiar with bootmem.

Cheers
James

Quoted 19 lines

arch/mips/kernel/setup.c +3 -4

+
+
+     /*
        * Find the highest page frame number we have available.
        */
       for (i = 0; i < boot_mem_map.nr_map; i++) {

@@ -664,9 +680,6 @@ static int __init early_parse_mem(char *p)
 
       add_memory_region(start, size, BOOT_MEM_RAM);
 
-     if (start && start > PHYS_OFFSET)
-             add_memory_region(PHYS_OFFSET, start - PHYS_OFFSET,
-                             BOOT_MEM_RESERVED);
       return 0;
  }
  early_param("mem", early_parse_mem);
--

2.11.0