Message-ID: <e89f6ce6-d9f4-4744-b6a6-9a82412795a0@loongson.cn>
Date: Thu, 3 Jul 2025 09:36:58 +0800
From: Ming Wang <wangming01@...ngson.cn>
To: Yanteng Si <si.yanteng@...ux.dev>, Huacai Chen <chenhuacai@...nel.org>,
 WANG Xuerui <kernel@...0n.name>, Andrew Morton <akpm@...ux-foundation.org>,
 Bibo Mao <maobibo@...ngson.cn>, Hari Bathini <hbathini@...ux.ibm.com>,
 Guo Weikang <guoweikang.kernel@...il.com>,
 Sourabh Jain <sourabhjain@...ux.ibm.com>, Usama Arif
 <usamaarif642@...il.com>, loongarch@...ts.linux.dev,
 linux-kernel@...r.kernel.org
Cc: lixuefeng@...ngson.cn, chenhuacai@...ngson.cn, gaojuxin@...ngson.cn
Subject: Re: [PATCH] LoongArch: Support mem=SIZE kernel parameter

Hi Yanteng,

Thanks for reviewing the patch and for your insightful question.

On 7/2/25 10:11, Yanteng Si wrote:
> 在 7/1/25 5:04 PM, Ming Wang 写道:
>> The LoongArch mem= parameter parser was previously limited to the
>> mem=SIZE@...RT format. This was inconvenient for the common use case
>> of simply capping the total system memory, as it forced users to
>> manually specify a start address. It was also inconsistent with the
>> behavior on other architectures.
>>
>> This patch enhances the parser in early_parse_mem() to also support the
>> more user-friendly mem=SIZE format. The implementation now checks for
>> the presence of the '@' symbol to determine the user's intent:
>>
>> - If mem=SIZE is provided (no '@'), the kernel now calls
>>    memblock_enforce_memory_limit(). This trims memory from the top down
>>    to the specified size.
>> - If mem=SIZE@...RT is used, the original behavior is retained for
>>    backward compatibility. This allows for defining specific memory
>>    banks.
>>
>> This change introduces an important usage rule reflected in the code's
>> comments: the mem=SIZE format should only be specified once on the
>> kernel command line. It acts as a single, global cap on total memory. In
>> contrast, the mem=SIZE@...RT format can be used multiple times to
>> define several distinct memory regions.
>>
>> Signed-off-by: Ming Wang <wangming01@...ngson.cn>
>> ---
>>   arch/loongarch/kernel/setup.c | 18 ++++++++++--------
>>   1 file changed, 10 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/loongarch/kernel/setup.c b/arch/loongarch/kernel/ 
>> setup.c
>> index b99fbb388fe0..af59ba180dc2 100644
>> --- a/arch/loongarch/kernel/setup.c
>> +++ b/arch/loongarch/kernel/setup.c
>> @@ -191,6 +191,16 @@ static int __init early_parse_mem(char *p)
>>           return -EINVAL;
>>       }
>> +    start = 0;
>> +    size = memparse(p, &p);
>> +    if (*p == '@')    /* Every mem=... should contain '@' */
>> +        start = memparse(p + 1, &p);
>> +    else {            /* Only one mem=... is allowed if no '@' */
>> +        usermem = 1;
>> +        memblock_enforce_memory_limit(size);
>> +        return 0;
>> +    }
>> +
>>       /*
>>        * If a user specifies memory size, we
>>        * blow away any automatically generated
>> @@ -201,14 +211,6 @@ static int __init early_parse_mem(char *p)
>>           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);
>> -    else {
>> -        pr_err("Invalid format!\n");
>> -        return -EINVAL;
>> -    }
> I don't understand. Isn't it better to modify the else{} directly here?
> 
You've raised a very good point. The reason for moving the parsing logic 
to the top, rather than just modifying the original else block, is to 
handle the fundamentally different behaviors required for mem=SIZE 
versus mem=SIZE@...RT. The key lies in thisexisting block of code which 
handles the mem=SIZE@...RT case:

```
/*
* If a user specifies memory size, we
* blow away any automatically generated
* size.
*/
if (usermem == 0) {
	usermem = 1;
	memblock_remove(memblock_start_of_DRAM(),
			memblock_end_of_DRAM() - memblock_start_of_DRAM());
}
```

This code is destructive. As the comment says, it "blows away" the 
entire memory map discovered from the firmware (UEFI/ACPI). After this 
call, memblock is essentially empty, waiting for new regions to be added 
via memblock_add(). This is the correct behavior for mem=SIZE@...RT.

However, the new mem=SIZE functionality is meant to be non-destructive. 
It should take the existing firmware-provided memory map and simply trim 
it down to the desired size. The function 
memblock_enforce_memory_limit() is designed for this purpose—it operates 
on the current state of memblock.

If we were to keep the parsing logic at the end and only modify the else 
block, the destructive memblock_remove() call would have already 
executed for both cases. By that point, calling 
memblock_enforce_memory_limit() would be meaningless, as there would be 
no memory regions left in memblock to limit.

Therefore, the patch moves the parsing logic to the very beginning to 
create a clean separation:
1. It first checks if the format is mem=SIZE (no '@').
2. If it is, it performs the non-destructive limit on the intact memory 
map and returns immediately, completely bypassing the destructive 
memblock_remove() logic.
3. If the format is mem=SIZE@...RT, it falls through to the original 
destructive path as before.

I hope this explanation clarifies why the code structure was changed 
this way. It's crucial to ensure the non-destructive path is handled 
before any memory map information is lost.

Best regards,
Ming


> Thanks,
> Yanteng
> 

Powered by blists - more mailing lists