[PATCH v2] MIPS: fix incorrect mem=X@Y handling

[PATCH v2] MIPS: fix incorrect mem=X@Y handling

[Mathieu Malaterre]

From: Marcin Nowakowski <marcin.nowakowski@mips.com>

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 <mathieu.malaterre@gmail.com>
Signed-off-by: Marcin Nowakowski <marcin.nowakowski@mips.com>
Fixes: 73fbc1eba7ff ("MIPS: fix mem=X@Y commandline processing")
Cc: <stable@vger.kernel.org> # v4.11
---
v2: Use updated email adress, add tag for stable.
 arch/mips/kernel/setup.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
index 702c678de116..f19d61224c71 100644
--- a/arch/mips/kernel/setup.c
+++ b/arch/mips/kernel/setup.c
@@ -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);
-- 
2.11.0

Re: [PATCH v2] MIPS: fix incorrect mem=X@Y handling

[Matt Redfearn]

Hi

On 21/12/17 21:00, Mathieu Malaterre wrote:
> From: Marcin Nowakowski <marcin.nowakowski@mips.com>
> 
> 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 <mathieu.malaterre@gmail.com>
> Signed-off-by: Marcin Nowakowski <marcin.nowakowski@mips.com>
> Fixes: 73fbc1eba7ff ("MIPS: fix mem=X@Y commandline processing")
> Cc: <stable@vger.kernel.org> # v4.11

Tested on:
Creator Ci20
Creator Ci40
MIPS Boston
UTM8 (Cavium Octeon III)

It certainly fixes the ci20 when it's factory default command line args 
of "mem=256M@0x0 mem=768M@0x30000000" are passed. Though those arguments 
appear redundant since without them both memory regions are detected 
through device tree instead, and there is no problem.

Tested-by: Matt Redfearn <matt.redfearn@mips.com>


> ---
> v2: Use updated email adress, add tag for stable.
>   arch/mips/kernel/setup.c | 19 ++++++++++++++++---
>   1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
> index 702c678de116..f19d61224c71 100644
> --- a/arch/mips/kernel/setup.c
> +++ b/arch/mips/kernel/setup.c
> @@ -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);
> 

Re: [PATCH v2] MIPS: fix incorrect mem=X@Y handling

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 3492 bytes --]

On Thu, Dec 21, 2017 at 10:00:59PM +0100, Mathieu Malaterre wrote:
> From: Marcin Nowakowski <marcin.nowakowski@mips.com>
> 
> 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").

> 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 <mathieu.malaterre@gmail.com>
> Signed-off-by: Marcin Nowakowski <marcin.nowakowski@mips.com>
> Fixes: 73fbc1eba7ff ("MIPS: fix mem=X@Y commandline processing")
> Cc: <stable@vger.kernel.org> # v4.11

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

> ---
> v2: Use updated email adress, add tag for stable.
>  arch/mips/kernel/setup.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
> index 702c678de116..f19d61224c71 100644
> --- a/arch/mips/kernel/setup.c
> +++ b/arch/mips/kernel/setup.c
> @@ -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().

>  	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);

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

> +	}
> +
> +	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

> +
> +
> +	/*
>  	 * 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
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] MIPS: fix incorrect mem=X@Y handling

[Mathieu Malaterre]

Hi Marcin,

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 ?

On Tue, Jan 23, 2018 at 3:17 PM, James Hogan <jhogan@kernel.org> wrote:
> On Thu, Dec 21, 2017 at 10:00:59PM +0100, Mathieu Malaterre wrote:
>> From: Marcin Nowakowski <marcin.nowakowski@mips.com>
>>
>> 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").
>
>> 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 <mathieu.malaterre@gmail.com>
>> Signed-off-by: Marcin Nowakowski <marcin.nowakowski@mips.com>
>> Fixes: 73fbc1eba7ff ("MIPS: fix mem=X@Y commandline processing")
>> Cc: <stable@vger.kernel.org> # v4.11
>
> I'm guessing that should technically be v4.11+

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

>> ---
>> v2: Use updated email adress, add tag for stable.
>>  arch/mips/kernel/setup.c | 19 ++++++++++++++++---
>>  1 file changed, 16 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
>> index 702c678de116..f19d61224c71 100644
>> --- a/arch/mips/kernel/setup.c
>> +++ b/arch/mips/kernel/setup.c
>> @@ -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().
>
>>       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);
>
> Is it worth incorporating this into the existing loop below ...
>
>> +     }
>> +
>> +     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
>
>> +
>> +
>> +     /*
>>        * 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
>>

Re: [PATCH v2] MIPS: fix incorrect mem=X@Y handling

[Marcin Nowakowski]

Hi Mathieu,

On 31.01.2018 08:47, Mathieu Malaterre wrote:

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

I'll prepare an updated patch that includes James' suggestions - I think 
they will lead to an overall cleaner (and probably slightly smaller) code.

Marcin


> On Tue, Jan 23, 2018 at 3:17 PM, James Hogan <jhogan@kernel.org> wrote:
>> On Thu, Dec 21, 2017 at 10:00:59PM +0100, Mathieu Malaterre wrote:
>>> From: Marcin Nowakowski <marcin.nowakowski@mips.com>
>>>
>>> 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").
>>
>>> 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 <mathieu.malaterre@gmail.com>
>>> Signed-off-by: Marcin Nowakowski <marcin.nowakowski@mips.com>
>>> Fixes: 73fbc1eba7ff ("MIPS: fix mem=X@Y commandline processing")
>>> Cc: <stable@vger.kernel.org> # v4.11
>>
>> I'm guessing that should technically be v4.11+
> 
> My fault, if this is the only change, I can re-submit.
> 
>>> ---
>>> v2: Use updated email adress, add tag for stable.
>>>   arch/mips/kernel/setup.c | 19 ++++++++++++++++---
>>>   1 file changed, 16 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
>>> index 702c678de116..f19d61224c71 100644
>>> --- a/arch/mips/kernel/setup.c
>>> +++ b/arch/mips/kernel/setup.c
>>> @@ -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().
>>
>>>        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);
>>
>> Is it worth incorporating this into the existing loop below ...
>>
>>> +     }
>>> +
>>> +     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
>>
>>> +
>>> +
>>> +     /*
>>>         * 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
>>>