[PATCH 0/3] MIPS: Modify early_parse_mem()

[PATCH 0/3] MIPS: Modify early_parse_mem()

[Tiezhu Yang]

Tiezhu Yang (3):
  MIPS: Return -EINVAL if mem parameter is empty in early_parse_mem()
  MIPS: Return -EINVAL if mem parameter is invalid in early_parse_mem()
  MIPS: Use memblock_add_node() in early_parse_mem() under CONFIG_NUMA

 arch/mips/kernel/setup.c | 35 +++++++++++++++++++++++++++++------
 1 file changed, 29 insertions(+), 6 deletions(-)

-- 
2.1.0

[PATCH 1/3] MIPS: Return -EINVAL if mem parameter is empty in early_parse_mem()

[Tiezhu Yang]

In the current code, the users usually need to make sure the value
of mem parameter is correct, but it is better to do some check to
avoid potential boot hangs.

This commit checks whether mem parameter is empty, if yes, return
-EINVAL before call memblock_remove() and memblock_add().

Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
 arch/mips/kernel/setup.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
index f979adf..14aa8bd 100644
--- a/arch/mips/kernel/setup.c
+++ b/arch/mips/kernel/setup.c
@@ -344,6 +344,11 @@ static int __init early_parse_mem(char *p)
 {
 	phys_addr_t start, size;
 
+	if (!p) {
+		pr_err("mem parameter is empty, do nothing\n");
+		return -EINVAL;
+	}
+
 	/*
 	 * If a user specifies memory size, we
 	 * blow away any automatically generated
-- 
2.1.0

[PATCH 2/3] MIPS: Return -EINVAL if mem parameter is invalid in early_parse_mem()

[Tiezhu Yang]

In the current code, the users usually need to make sure the value
of mem parameter is correct, but it is better to do some check to
avoid potential boot hangs.

This commit checks whether the first mem parameter is invalid, if
yes, return -EINVAL before call memblock_remove() and memblock_add().

Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
 arch/mips/kernel/setup.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
index 14aa8bd..c8c8f60 100644
--- a/arch/mips/kernel/setup.c
+++ b/arch/mips/kernel/setup.c
@@ -343,12 +343,19 @@ static int usermem __initdata;
 static int __init early_parse_mem(char *p)
 {
 	phys_addr_t start, size;
+	phys_addr_t pa_start, pa_end;
+	u64 idx;
 
 	if (!p) {
 		pr_err("mem parameter is empty, do nothing\n");
 		return -EINVAL;
 	}
 
+	start = 0;
+	size = memparse(p, &p);
+	if (*p == '@')
+		start = memparse(p + 1, &p);
+
 	/*
 	 * If a user specifies memory size, we
 	 * blow away any automatically generated
@@ -356,13 +363,20 @@ static int __init early_parse_mem(char *p)
 	 */
 	if (usermem == 0) {
 		usermem = 1;
+		for_each_mem_range(idx, &pa_start, &pa_end) {
+			if (start >= pa_start && size <= pa_end - pa_start)
+				break;
+
+			if (idx < memblock.memory.cnt)
+				continue;
+
+			usermem = -1;
+			pr_err("mem parameter is invalid, do nothing\n");
+			return -EINVAL;
+		}
 		memblock_remove(memblock_start_of_DRAM(),
 			memblock_end_of_DRAM() - memblock_start_of_DRAM());
 	}
-	start = 0;
-	size = memparse(p, &p);
-	if (*p == '@')
-		start = memparse(p + 1, &p);
 
 	memblock_add(start, size);
 
@@ -638,7 +652,7 @@ static void __init arch_mem_init(char **cmdline_p)
 
 	parse_early_param();
 
-	if (usermem)
+	if (usermem == 1)
 		pr_info("User-defined physical RAM map overwrite\n");
 
 	check_kernel_sections_mem();
-- 
2.1.0

[PATCH 3/3] MIPS: Use memblock_add_node() in early_parse_mem() under CONFIG_NUMA

[Tiezhu Yang]

Use memblock_add_node to add new memblock region within a NUMA node
in early_parse_mem() under CONFIG_NUMA, otherwise the mem parameter
can not work well.

Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
 arch/mips/kernel/setup.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
index c8c8f60..50cdc08 100644
--- a/arch/mips/kernel/setup.c
+++ b/arch/mips/kernel/setup.c
@@ -37,6 +37,7 @@
 #include <asm/cdmm.h>
 #include <asm/cpu.h>
 #include <asm/debug.h>
+#include <asm/mmzone.h>
 #include <asm/sections.h>
 #include <asm/setup.h>
 #include <asm/smp-ops.h>
@@ -378,7 +379,10 @@ static int __init early_parse_mem(char *p)
 			memblock_end_of_DRAM() - memblock_start_of_DRAM());
 	}
 
-	memblock_add(start, size);
+	if (IS_ENABLED(CONFIG_NUMA))
+		memblock_add_node(start, size, pa_to_nid(start), MEMBLOCK_NONE);
+	else
+		memblock_add(start, size);
 
 	return 0;
 }
-- 
2.1.0

Re: [PATCH 3/3] MIPS: Use memblock_add_node() in early_parse_mem() under CONFIG_NUMA

[Jiaxun Yang]

在 2022/3/18 15:05, Tiezhu Yang 写道:
> Use memblock_add_node to add new memblock region within a NUMA node
> in early_parse_mem() under CONFIG_NUMA, otherwise the mem parameter
> can not work well.

Hi Tiezhu,

pa_to_nid doesn't exist when CONFIG_NUME is disabled.
So probably you want #ifdef macro instead ？

Thanks.
- Jiaxun

>
> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
> ---
>   arch/mips/kernel/setup.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
> index c8c8f60..50cdc08 100644
> --- a/arch/mips/kernel/setup.c
> +++ b/arch/mips/kernel/setup.c
> @@ -37,6 +37,7 @@
>   #include <asm/cdmm.h>
>   #include <asm/cpu.h>
>   #include <asm/debug.h>
> +#include <asm/mmzone.h>
>   #include <asm/sections.h>
>   #include <asm/setup.h>
>   #include <asm/smp-ops.h>
> @@ -378,7 +379,10 @@ static int __init early_parse_mem(char *p)
>   			memblock_end_of_DRAM() - memblock_start_of_DRAM());
>   	}
>   
> -	memblock_add(start, size);
> +	if (IS_ENABLED(CONFIG_NUMA))
> +		memblock_add_node(start, size, pa_to_nid(start), MEMBLOCK_NONE);
> +	else
> +		memblock_add(start, size);
>   
>   	return 0;
>   }

Re: [PATCH 3/3] MIPS: Use memblock_add_node() in early_parse_mem() under CONFIG_NUMA

[Tiezhu Yang]

On 03/22/2022 09:19 PM, Jiaxun Yang wrote:
>
>
> 在 2022/3/18 15:05, Tiezhu Yang 写道:
>> Use memblock_add_node to add new memblock region within a NUMA node
>> in early_parse_mem() under CONFIG_NUMA, otherwise the mem parameter
>> can not work well.
>
> Hi Tiezhu,
>
> pa_to_nid doesn't exist when CONFIG_NUME is disabled.
> So probably you want #ifdef macro instead ？

Hi Jiaxun,

Thank you for your reply.

As far as I can tell, if CONFIG_NUMA is set, IS_ENABLED(CONFIG_NUMA)
is 1, pa_to_nid() is defined in the platform dependent header file:

	arch/mips/include/asm/mach-ip27/mmzone.h
	arch/mips/include/asm/mach-loongson64/mmzone.h

if CONFIG_NUMA is not set, IS_ENABLED(CONFIG_NUMA) is 0, pa_to_nid()
is always 0 which is defined in arch/mips/include/asm/mmzone.h:

	#ifdef CONFIG_NUMA
	# include <mmzone.h>
	#endif

	#ifndef pa_to_nid
	#define pa_to_nid(addr) 0
	#endif

So pa_to_nid() is defined under both CONFIG_NUMA and !CONFIG_NUMA,
there is no build error.

Additionally, use #ifdef CONFIG_NUMA is also OK, but I prefer to
use IS_ENABLED(CONFIG_NUMA).

If I am missing something, please let me know, thank you.

Thanks,
Tiezhu

>
> Thanks.
> - Jiaxun
>
>>
>> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
>> ---
>>   arch/mips/kernel/setup.c | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
>> index c8c8f60..50cdc08 100644
>> --- a/arch/mips/kernel/setup.c
>> +++ b/arch/mips/kernel/setup.c
>> @@ -37,6 +37,7 @@
>>   #include <asm/cdmm.h>
>>   #include <asm/cpu.h>
>>   #include <asm/debug.h>
>> +#include <asm/mmzone.h>
>>   #include <asm/sections.h>
>>   #include <asm/setup.h>
>>   #include <asm/smp-ops.h>
>> @@ -378,7 +379,10 @@ static int __init early_parse_mem(char *p)
>>               memblock_end_of_DRAM() - memblock_start_of_DRAM());
>>       }
>>   -    memblock_add(start, size);
>> +    if (IS_ENABLED(CONFIG_NUMA))
>> +        memblock_add_node(start, size, pa_to_nid(start), MEMBLOCK_NONE);
>> +    else
>> +        memblock_add(start, size);
>>         return 0;
>>   }

Re: [PATCH 3/3] MIPS: Use memblock_add_node() in early_parse_mem() under CONFIG_NUMA

[Jiaxun Yang]

在2022年3月23日三月 上午2:42，Tiezhu Yang写道：
> On 03/22/2022 09:19 PM, Jiaxun Yang wrote:
>>
>>
>> 在 2022/3/18 15:05, Tiezhu Yang 写道:
>>> Use memblock_add_node to add new memblock region within a NUMA node
>>> in early_parse_mem() under CONFIG_NUMA, otherwise the mem parameter
>>> can not work well.
>>
>> Hi Tiezhu,
>>
>> pa_to_nid doesn't exist when CONFIG_NUME is disabled.
>> So probably you want #ifdef macro instead ？
>
> Hi Jiaxun,
>
> Thank you for your reply.
>
> As far as I can tell, if CONFIG_NUMA is set, IS_ENABLED(CONFIG_NUMA)
> is 1, pa_to_nid() is defined in the platform dependent header file:

Yep you're right.
Apologies for the noise.

For the whole series:
Reviewed-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
Tested-by: Jiaxun Yang <jiaxun.yang@flygoat.com>

Thanks.

>
> 	arch/mips/include/asm/mach-ip27/mmzone.h
> 	arch/mips/include/asm/mach-loongson64/mmzone.h
>
> if CONFIG_NUMA is not set, IS_ENABLED(CONFIG_NUMA) is 0, pa_to_nid()
> is always 0 which is defined in arch/mips/include/asm/mmzone.h:
>
> 	#ifdef CONFIG_NUMA
> 	# include <mmzone.h>
> 	#endif
>
> 	#ifndef pa_to_nid
> 	#define pa_to_nid(addr) 0
> 	#endif
>
> So pa_to_nid() is defined under both CONFIG_NUMA and !CONFIG_NUMA,
> there is no build error.
>
> Additionally, use #ifdef CONFIG_NUMA is also OK, but I prefer to
> use IS_ENABLED(CONFIG_NUMA).
>
> If I am missing something, please let me know, thank you.
>
> Thanks,
> Tiezhu
>
>>
>> Thanks.
>> - Jiaxun
>>
>>>
>>> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
>>> ---
>>>   arch/mips/kernel/setup.c | 6 +++++-
>>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
>>> index c8c8f60..50cdc08 100644
>>> --- a/arch/mips/kernel/setup.c
>>> +++ b/arch/mips/kernel/setup.c
>>> @@ -37,6 +37,7 @@
>>>   #include <asm/cdmm.h>
>>>   #include <asm/cpu.h>
>>>   #include <asm/debug.h>
>>> +#include <asm/mmzone.h>
>>>   #include <asm/sections.h>
>>>   #include <asm/setup.h>
>>>   #include <asm/smp-ops.h>
>>> @@ -378,7 +379,10 @@ static int __init early_parse_mem(char *p)
>>>               memblock_end_of_DRAM() - memblock_start_of_DRAM());
>>>       }
>>>   -    memblock_add(start, size);
>>> +    if (IS_ENABLED(CONFIG_NUMA))
>>> +        memblock_add_node(start, size, pa_to_nid(start), MEMBLOCK_NONE);
>>> +    else
>>> +        memblock_add(start, size);
>>>         return 0;
>>>   }

-- 
- Jiaxun

Re: [PATCH 0/3] MIPS: Modify early_parse_mem()

[Tiezhu Yang]

On 03/18/2022 11:05 PM, Tiezhu Yang wrote:
> Tiezhu Yang (3):
>   MIPS: Return -EINVAL if mem parameter is empty in early_parse_mem()
>   MIPS: Return -EINVAL if mem parameter is invalid in early_parse_mem()
>   MIPS: Use memblock_add_node() in early_parse_mem() under CONFIG_NUMA
>
>  arch/mips/kernel/setup.c | 35 +++++++++++++++++++++++++++++------
>  1 file changed, 29 insertions(+), 6 deletions(-)
>

Hi Thomas,

Any comments? Are you OK with these changes?

https://lore.kernel.org/linux-mips/1647615920-23103-1-git-send-email-yangtiezhu@loongson.cn/T/#u

Thanks,
Tiezhu

Re: [PATCH 0/3] MIPS: Modify early_parse_mem()

[Thomas Bogendoerfer]

On Mon, May 09, 2022 at 03:30:11PM +0800, Tiezhu Yang wrote:
> 
> 
> On 03/18/2022 11:05 PM, Tiezhu Yang wrote:
> > Tiezhu Yang (3):
> >   MIPS: Return -EINVAL if mem parameter is empty in early_parse_mem()
> >   MIPS: Return -EINVAL if mem parameter is invalid in early_parse_mem()
> >   MIPS: Use memblock_add_node() in early_parse_mem() under CONFIG_NUMA
> > 
> >  arch/mips/kernel/setup.c | 35 +++++++++++++++++++++++++++++------
> >  1 file changed, 29 insertions(+), 6 deletions(-)
> > 
> 
> Hi Thomas,
> 
> Any comments? Are you OK with these changes?

first and last patch are ok with me. The second patch changes semantics
for mem=, which I don't want to change. Iirc the latest idea to solve
your problem was to use mem=XX@ syntax to limit detected memory, which
is the preferred way for me, too.

If you want I'll take patch 1 and 3 out of this series.

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH 0/3] MIPS: Modify early_parse_mem()

[Tiezhu Yang]

On 05/23/2022 09:28 PM, Thomas Bogendoerfer wrote:
> On Mon, May 09, 2022 at 03:30:11PM +0800, Tiezhu Yang wrote:
>>
>>
>> On 03/18/2022 11:05 PM, Tiezhu Yang wrote:
>>> Tiezhu Yang (3):
>>>   MIPS: Return -EINVAL if mem parameter is empty in early_parse_mem()
>>>   MIPS: Return -EINVAL if mem parameter is invalid in early_parse_mem()
>>>   MIPS: Use memblock_add_node() in early_parse_mem() under CONFIG_NUMA
>>>
>>>  arch/mips/kernel/setup.c | 35 +++++++++++++++++++++++++++++------
>>>  1 file changed, 29 insertions(+), 6 deletions(-)
>>>
>>
>> Hi Thomas,
>>
>> Any comments? Are you OK with these changes?
>
> first and last patch are ok with me. The second patch changes semantics
> for mem=, which I don't want to change. Iirc the latest idea to solve
> your problem was to use mem=XX@ syntax to limit detected memory, which
> is the preferred way for me, too.
>
> If you want I'll take patch 1 and 3 out of this series.
>
> Thomas.
>

OK, thank you.

Let me rebase and send v2 later.

Thanks,
Tiezhu